Inclusive configuration of resources to propagate

See issue kubernetes-retired#16. To allow inclusive propagation of resources an
additional `SyncornizationMode` called 'AllowPropagate' which only enables
propagation when a selector is set is added. An 'all' selector is also addded.

Tested: e2e-testing covering secrets resource in 'AllowPropagate' mode and checking
propagation when selectors are set and unset ('select', 'treeSelect', 'none', 'all').
Unit testing is also modified to account for the new 'all' selection

Signed-off-by: mzeevi <[email protected]>

Author: mzeevi

api/v1alpha2/hierarchy_types.go

@@ -37,6 +37,7 @@ const (
 	AnnotationSelector     = AnnotationPropagatePrefix + "/select"
 	AnnotationTreeSelector = AnnotationPropagatePrefix + "/treeSelect"
 	AnnotationNoneSelector = AnnotationPropagatePrefix + "/none"
+	AnnotationAllSelector  = AnnotationPropagatePrefix + "/all"
 
 	// LabelManagedByStandard will eventually replace our own managed-by annotation (we didn't know
 	// about this standard label when we invented our own).

api/v1alpha2/hnc_config.go

@@ -31,7 +31,7 @@ const (
 )
 
 // SynchronizationMode describes propagation mode of objects of the same kind.
-// The only three modes currently supported are "Propagate", "Ignore", and "Remove".
+// The only four modes currently supported are "Propagate", "AllowPropagate", "Ignore", and "Remove".
 // See detailed definition below. An unsupported mode will be treated as "ignore".
 type SynchronizationMode string
 
@@ -46,6 +46,10 @@ const (
 
 	// Remove all existing propagated copies.
 	Remove SynchronizationMode = "Remove"
+
+	// AllowPropagate allows propagation of objects from ancestors to descendants
+	// and deletes obsolete descendants only if a an annotation is set on the object
+	AllowPropagate SynchronizationMode = "AllowPropagate"
 )
 
 const (
@@ -94,7 +98,7 @@ type ResourceSpec struct {
 	// Synchronization mode of the kind. If the field is empty, it will be treated
 	// as "Propagate".
 	// +optional
-	// +kubebuilder:validation:Enum=Propagate;Ignore;Remove
+	// +kubebuilder:validation:Enum=Propagate;Ignore;Remove;AllowPropagate
 	Mode SynchronizationMode `json:"mode,omitempty"`
 }
 

config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml

@@ -64,6 +64,7 @@ spec:
                       - Propagate
                       - Ignore
                       - Remove
+                      - AllowPropagate
                       type: string
                     resource:
                       description: Resource to be configured.

internal/forest/forest.go

@@ -51,6 +51,9 @@ type TypeSyncer interface {
 	// GetMode gets the propagation mode of objects that are handled by the reconciler who implements the interface.
 	GetMode() api.SynchronizationMode
 
+	// CanPropagate returns true if Propagate mode or AllowPropagate mode is set
+	CanPropagate() bool
+
 	// GetNumPropagatedObjects returns the number of propagated objects on the apiserver.
 	GetNumPropagatedObjects() int
 }

internal/hierarchyconfig/validator.go

@@ -246,16 +246,21 @@ func (v *Validator) getConflictingObjects(newParent, ns *forest.Namespace) []str
 	if newParent == nil {
 		return nil
 	}
-	// Traverse all the types with 'Propagate' mode to find any conflicts.
+	// Traverse all the types with 'Propagate' mode or 'Allow' mode to find any conflicts.
 	conflicts := []string{}
 	for _, t := range v.Forest.GetTypeSyncers() {
-		if t.GetMode() == api.Propagate {
+		if t.CanPropagate() {
 			conflicts = append(conflicts, v.getConflictingObjectsOfType(t.GetGVK(), newParent, ns)...)
 		}
 	}
 	return conflicts
 }
 
+func (v *Validator) isPropagateType(gvk metav1.GroupVersionKind) bool {
+	ts := v.Forest.GetTypeSyncerFromGroupKind(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind})
+	return ts != nil && ts.GetMode() == api.Propagate
+}
+
 // getConflictingObjectsOfType returns a list of namespaced objects if there's
 // any conflict between the new ancestors and the descendants.
 func (v *Validator) getConflictingObjectsOfType(gvk schema.GroupVersionKind, newParent, ns *forest.Namespace) []string {
@@ -266,7 +271,7 @@ func (v *Validator) getConflictingObjectsOfType(gvk schema.GroupVersionKind, new
 		// If the user has chosen not to propagate the object to this descendant,
 		// then it should not be included in conflict checks
 		o := v.Forest.Get(nnm.Namespace).GetSourceObject(gvk, nnm.Name)
-		if ok, _ := selectors.ShouldPropagate(o, o.GetLabels()); ok {
+		if ok, _ := selectors.ShouldPropagate(o, o.GetLabels(), v.isPropagateType(metav1.GroupVersionKind(gvk))); ok {
 			newAnsSrcObjs[nnm.Name] = true
 		}
 	}

internal/hncconfig/reconciler.go

@@ -361,7 +361,7 @@ func (r *Reconciler) writeCondition(inst *api.HNCConfiguration, tp, reason, msg
 }
 
 // setTypeStatuses adds Status.Resources for types configured in the spec. Only the status of types
-// in `Propagate` and `Remove` modes will be recorded. The Status.Resources is sorted in
+// in `Propagate`, `Remove` and `AllowPropagate` modes will be recorded. The Status.Resources is sorted in
 // alphabetical order based on Group and Resource.
 func (r *Reconciler) setTypeStatuses(inst *api.HNCConfiguration) {
 	// We lock the forest here so that other reconcilers cannot modify the
@@ -394,7 +394,7 @@ func (r *Reconciler) setTypeStatuses(inst *api.HNCConfiguration) {
 		}
 
 		// Only add NumSourceObjects if we are propagating objects of this type.
-		if ts.GetMode() == api.Propagate {
+		if ts.CanPropagate() {
 			numSrc := 0
 			nms := r.Forest.GetNamespaceNames()
 			for _, nm := range nms {

internal/hncconfig/validator.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/go-logr/logr"
 	k8sadm "k8s.io/api/admission/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/rest"
@@ -155,6 +156,11 @@ func (v *Validator) checkConflictsForGVK(gvk schema.GroupVersionKind) []string {
 	return conflicts
 }
 
+func (v *Validator) isPropagateType(gvk metav1.GroupVersionKind) bool {
+	ts := v.Forest.GetTypeSyncerFromGroupKind(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind})
+	return ts != nil && ts.GetMode() == api.Propagate
+}
+
 // checkConflictsForTree check for all the gvk objects in the given namespaces, to see if they
 // will be potentially overwritten by the objects on the ancestor namespaces
 func (v *Validator) checkConflictsForTree(gvk schema.GroupVersionKind, ao ancestorObjects, ns *forest.Namespace) []string {
@@ -167,7 +173,7 @@ func (v *Validator) checkConflictsForTree(gvk schema.GroupVersionKind, ao ancest
 		for _, nnm := range objs[onm] {
 			// check if the existing ns will propagate this object to the current ns
 			inst := v.Forest.Get(nnm).GetSourceObject(gvk, onm)
-			if ok, _ := selectors.ShouldPropagate(inst, ns.GetLabels()); ok {
+			if ok, _ := selectors.ShouldPropagate(inst, ns.GetLabels(), v.isPropagateType(metav1.GroupVersionKind(gvk))); ok {
 				conflicts = append(conflicts, fmt.Sprintf("  Object %q in namespace %q would overwrite the one in %q", onm, nnm, ns.Name()))
 			}
 		}

internal/hrq/rqreconciler.go

@@ -202,8 +202,7 @@ func (r *ResourceQuotaReconciler) deleteSingleton(ctx context.Context, log logr.
 //   and its ancestors.
 // - Updates `hrq.used.local` of the current namespace and `hrq.used.subtree`
 //   of the current namespace and its ancestors based on `ResourceQuota.Status.Used`.
-func (r *ResourceQuotaReconciler) syncWithForest(log logr.Logger,
-	inst *v1.ResourceQuota) bool {
+func (r *ResourceQuotaReconciler) syncWithForest(log logr.Logger, inst *v1.ResourceQuota) bool {
 	r.Forest.Lock()
 	defer r.Forest.Unlock()
 	ns := r.Forest.Get(inst.ObjectMeta.Namespace)

internal/kubectl/configdescribe.go

@@ -37,6 +37,8 @@ var configDescribeCmd = &cobra.Command{
 				action = "Propagating"
 			case api.Remove:
 				action = "Removing"
+			case api.AllowPropagate:
+				action = "AllowPropagate"
 			default:
 				action = "Ignoring"
 			}

internal/kubectl/configset.go

@@ -27,8 +27,8 @@ import (
 )
 
 var setResourceCmd = &cobra.Command{
-	Use: fmt.Sprintf("set-resource RESOURCE [--group GROUP] [--force] --mode <%s|%s|%s>",
-		api.Propagate, api.Remove, api.Ignore),
+	Use: fmt.Sprintf("set-resource RESOURCE [--group GROUP] [--force] --mode <%s|%s|%s|%s>",
+		api.Propagate, api.Remove, api.Ignore, api.AllowPropagate),
 	Short: "Sets the HNC configuration of a specific resource",
 	Example: fmt.Sprintf("  # Set configuration of a core type\n" +
 		"  kubectl hns config set-resource secrets --mode Ignore\n\n" +
@@ -40,16 +40,16 @@ var setResourceCmd = &cobra.Command{
 		flags := cmd.Flags()
 		group, _ := flags.GetString("group")
 		modeStr, _ := flags.GetString("mode")
-		mode := api.SynchronizationMode(cases.Title(language.English).String(modeStr))
+		mode := api.SynchronizationMode(cases.Title(language.English, cases.NoLower).String(modeStr))
 		force, _ := flags.GetBool("force")
 		config := client.getHNCConfig()
 
 		exist := false
 		for i := 0; i < len(config.Spec.Resources); i++ {
 			r := &config.Spec.Resources[i]
 			if r.Group == group && r.Resource == resource {
-				if r.Mode == api.Ignore && mode == api.Propagate && !force {
-					fmt.Printf("Switching directly from 'Ignore' to 'Propagate' mode could cause existing %q objects in "+
+				if r.Mode == api.Ignore && mode == api.Propagate && !force || r.Mode == api.Ignore && mode == api.AllowPropagate && !force {
+					fmt.Printf("Switching directly from 'Ignore' to 'Propagate' mode or 'AllowPropagate' mode could cause existing %q objects in "+
 						"child namespaces to be overwritten by objects from ancestor namespaces.\n", resource)
 					fmt.Println("If you are sure you want to proceed with this operation, use the '--force' flag.")
 					fmt.Println("If you are not sure and would like to see what source objects would be overwritten," +
@@ -78,7 +78,7 @@ var setResourceCmd = &cobra.Command{
 
 func newSetResourceCmd() *cobra.Command {
 	setResourceCmd.Flags().String("group", "", "The group of the resource; may be omitted for core resources (or explicitly set to the empty string)")
-	setResourceCmd.Flags().String("mode", "", "The synchronization mode: one of Propagate, Remove or Ignore")
-	setResourceCmd.Flags().BoolP("force", "f", false, "Allow the synchronization mode to be changed directly from Ignore to Propagate despite the dangers of doing so")
+	setResourceCmd.Flags().String("mode", "", "The synchronization mode: one of Propagate, Remove, Ignore and AllowPropagate")
+	setResourceCmd.Flags().BoolP("force", "f", false, "Allow the synchronization mode to be changed directly from Ignore to Propagate or AllowPropagate despite the dangers of doing so")
 	return setResourceCmd
 }

internal/objects/reconciler.go

@@ -165,7 +165,7 @@ func (r *Reconciler) GetMode() api.SynchronizationMode {
 // treated as api.Ignore.
 func GetValidateMode(mode api.SynchronizationMode, log logr.Logger) api.SynchronizationMode {
 	switch mode {
-	case api.Propagate, api.Ignore, api.Remove:
+	case api.Propagate, api.Ignore, api.Remove, api.AllowPropagate:
 		return mode
 	case "":
 		log.Info("Sync mode is unset; using default 'Propagate'")
@@ -198,6 +198,11 @@ func (r *Reconciler) SetMode(ctx context.Context, log logr.Logger, mode api.Sync
 	return nil
 }
 
+// CanPropagate returns true if Propagate mode or AllowPropagate mode is set
+func (r *Reconciler) CanPropagate() bool {
+	return (r.GetMode() == api.Propagate || r.GetMode() == api.AllowPropagate)
+}
+
 // GetNumPropagatedObjects returns the number of propagated objects of the GVK handled by this object reconciler.
 func (r *Reconciler) GetNumPropagatedObjects() int {
 	r.propagatedObjectsLock.Lock()
@@ -388,11 +393,13 @@ func (r *Reconciler) shouldSyncAsPropagated(log logr.Logger, inst *unstructured.
 	}
 
 	// If there's a conflicting source in the ancestors (excluding itself) and the
-	// the type has 'Propagate' mode, the object will be overwritten.
+	// the type has 'Propagate' mode or 'AllowPropagate' mode, the object will be overwritten.
 	mode := r.Forest.GetTypeSyncer(r.GVK).GetMode()
-	if mode == api.Propagate && srcInst != nil {
-		log.Info("Conflicting object found in ancestors namespace; will overwrite this object", "conflictingAncestor", srcInst.GetNamespace())
-		return true, srcInst
+	if mode == api.Propagate || mode == api.AllowPropagate {
+		if srcInst != nil {
+			log.Info("Conflicting object found in ancestors namespace; will overwrite this object", "conflictingAncestor", srcInst.GetNamespace())
+			return true, srcInst
+		}
 	}
 
 	return false, nil
@@ -463,7 +470,7 @@ func (r *Reconciler) syncPropagated(inst, srcInst *unstructured.Unstructured) (s
 func (r *Reconciler) syncSource(log logr.Logger, src *unstructured.Unstructured) {
 	// Update or create a copy of the source object in the forest. We now store
 	// all the source objects in the forests no matter if the mode is 'Propagate'
-	// or not, because HNCConfig webhook will also check the non-'Propagate' mode
+	// or not, because HNCConfig webhook will also check the non-'Propagate' or 'Allow' modes
 	// source objects in the forest to see if a mode change is allowed.
 	ns := r.Forest.Get(src.GetNamespace())
 
@@ -712,7 +719,7 @@ func hasPropagatedLabel(inst *unstructured.Unstructured) bool {
 // - Service Account token secrets
 func (r *Reconciler) shouldPropagateSource(log logr.Logger, inst *unstructured.Unstructured, dst string) bool {
 	nsLabels := r.Forest.Get(dst).GetLabels()
-	if ok, err := selectors.ShouldPropagate(inst, nsLabels); err != nil {
+	if ok, err := selectors.ShouldPropagate(inst, nsLabels, r.Mode == api.Propagate); err != nil {
 		log.Error(err, "Cannot propagate")
 		r.EventRecorder.Event(inst, "Warning", api.EventCannotParseSelector, err.Error())
 		return false

internal/objects/reconciler_test.go

@@ -51,6 +51,7 @@ var _ = Describe("Exceptions", func() {
 			selector     string
 			treeSelector string
 			noneSelector string
+			allSelector  string
 			want         []string
 			notWant      []string
 		}{{
@@ -120,6 +121,20 @@ var _ = Describe("Exceptions", func() {
 			noneSelector: "true",
 			want:         []string{},
 			notWant:      []string{c1, c2, c3},
+		}, {
+			name:         "not propagate when allSelector and noneSelector are both true",
+			noneSelector: "true",
+			allSelector:  "all",
+			want:         []string{},
+			notWant:      []string{c1, c2, c3},
+		}, {
+			name:         "only propagate the intersection of four selectors",
+			selector:     c1 + api.LabelTreeDepthSuffix,
+			treeSelector: c1 + ", " + c2,
+			noneSelector: "true",
+			allSelector:  "true",
+			want:         []string{},
+			notWant:      []string{c1, c2, c3},
 		}}
 
 		for _, tc := range tests {
@@ -146,6 +161,7 @@ var _ = Describe("Exceptions", func() {
 					api.AnnotationSelector:     tc.selector,
 					api.AnnotationTreeSelector: tc.treeSelector,
 					api.AnnotationNoneSelector: tc.noneSelector,
+					api.AnnotationAllSelector:  tc.allSelector,
 				})
 				for _, ns := range tc.want {
 					ns = ReplaceStrings(ns, names)
@@ -171,6 +187,7 @@ var _ = Describe("Exceptions", func() {
 			selector     string
 			treeSelector string
 			noneSelector string
+			allSelector  string
 			want         []string
 			notWant      []string
 		}{{
@@ -188,6 +205,11 @@ var _ = Describe("Exceptions", func() {
 			noneSelector: "true",
 			want:         []string{},
 			notWant:      []string{c1, c2, c3},
+		}, {
+			name:        "update allSelector",
+			allSelector: "true",
+			want:        []string{c1, c2, c3},
+			notWant:     []string{},
 		}}
 
 		for _, tc := range tests {
@@ -207,6 +229,7 @@ var _ = Describe("Exceptions", func() {
 				SetParent(ctx, names[c3], names[p])
 				tc.selector = ReplaceStrings(tc.selector, names)
 				tc.treeSelector = ReplaceStrings(tc.treeSelector, names)
+				tc.allSelector = ReplaceStrings(tc.allSelector, names)
 
 				// Create a Role and verify it's propagated
 				MakeObject(ctx, api.RoleResource, names[p], "testrole")
@@ -219,6 +242,7 @@ var _ = Describe("Exceptions", func() {
 					api.AnnotationSelector:     tc.selector,
 					api.AnnotationTreeSelector: tc.treeSelector,
 					api.AnnotationNoneSelector: tc.noneSelector,
+					api.AnnotationAllSelector:  tc.allSelector,
 				})
 				// make sure the changes are propagated
 				for _, ns := range tc.notWant {
@@ -724,6 +748,30 @@ var _ = Describe("Basic propagation", func() {
 			return nil
 		}).Should(Succeed(), "waiting for annot-a to be unpropagated")
 	})
+
+	It("should avoid propagating when no selector is set if the sync mode is 'AllowPropagate'", func() {
+		AddToHNCConfig(ctx, "", "secrets", api.AllowPropagate)
+		// Set tree as bar -> foo(root).
+		SetParent(ctx, barName, fooName)
+		MakeObject(ctx, "secrets", fooName, "foo-sec")
+		Eventually(HasObject(ctx, "secrets", fooName, "foo-sec")).Should(BeTrue())
+
+		// Ensure the object is not propagated
+		Consistently(HasObject(ctx, "secrets", barName, "foo-sec")).Should(BeFalse())
+
+		// Update the secret object with the treeSelector annotation
+		UpdateObjectWithAnnotations(ctx, "secrets", fooName, "foo-sec", map[string]string{
+			api.AnnotationTreeSelector: barName,
+		})
+
+		// Ensure the object is now propagated
+		Eventually(HasObject(ctx, "secrets", barName, "foo-sec")).Should(BeTrue())
+		Expect(ObjectInheritedFrom(ctx, "secrets", barName, "foo-sec")).Should(Equal(fooName))
+
+		// Remove the annotation from the secret and ensure the object is deleted from the descendant
+		UpdateObjectWithAnnotations(ctx, "secrets", fooName, "foo-sec", map[string]string{})
+		Eventually(HasObject(ctx, "secrets", barName, "foo-sec")).Should(BeFalse())
+	})
 })
 
 func modifyRole(ctx context.Context, nsName, roleName string) {