Inclusive configuration of resources to propagate

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

Tested: e2e-testing covering secrets resource in 'Allow' 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

@@ -46,6 +46,10 @@ const (
 
 	// Remove all existing propagated copies.
 	Remove SynchronizationMode = "Remove"
+
+	// Allow allows propagation of objects from ancestors to descendants
+	// and deletes obsolete descendants only if a an annotation is set on the object
+	Allow SynchronizationMode = "Allow"
 )
 
 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;Allow
 	Mode SynchronizationMode `json:"mode,omitempty"`
 }
 

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

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

internal/hierarchyconfig/validator.go

@@ -246,10 +246,10 @@ 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.GetMode() == api.Propagate || t.GetMode() == api.Allow {
 			conflicts = append(conflicts, v.getConflictingObjectsOfType(t.GetGVK(), newParent, ns)...)
 		}
 	}

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 `Allow` 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.GetMode() == api.Propagate || ts.GetMode() == api.Allow {
 			numSrc := 0
 			nms := r.Forest.GetNamespaceNames()
 			for _, nm := range nms {

internal/kubectl/configdescribe.go

@@ -37,6 +37,8 @@ var configDescribeCmd = &cobra.Command{
 				action = "Propagating"
 			case api.Remove:
 				action = "Removing"
+			case api.Allow:
+				action = "Allow"
 			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.Allow),
 	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" +
@@ -48,8 +48,8 @@ var setResourceCmd = &cobra.Command{
 		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.Allow && !force {
+					fmt.Printf("Switching directly from 'Ignore' to 'Propagate' mode or 'Allow' 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 Allow")
+	setResourceCmd.Flags().BoolP("force", "f", false, "Allow the synchronization mode to be changed directly from Ignore to Propagate or Allow 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.Allow:
 		return mode
 	case "":
 		log.Info("Sync mode is unset; using default 'Propagate'")
@@ -388,11 +388,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 'Allow' 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.Allow {
+		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 +465,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())
 
@@ -726,6 +728,16 @@ func (r *Reconciler) shouldPropagateSource(log logr.Logger, inst *unstructured.U
 	case r.Mode == api.Remove:
 		return false
 
+	case r.Mode == api.Allow:
+		if exists, err := selectors.SelectorExists(inst, nsLabels); err != nil {
+			log.Error(err, "Cannot propagate")
+			r.EventRecorder.Event(inst, "Warning", api.EventCannotParseSelector, err.Error())
+			return false
+		} else if !exists {
+			return false
+		}
+		return true
+
 	// Object with nonempty finalizer list is not propagated
 	case hasFinalizers(inst):
 		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,32 @@ 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 'Allow'", func() {
+		AddToHNCConfig(ctx, "", "secrets", api.Allow)
+		// 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
+		Eventually(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{})
+		// Give the reconciler some time to remove the object if it's going to.
+		time.Sleep(500 * time.Millisecond)
+		Eventually(HasObject(ctx, "secrets", barName, "foo-sec")).Should(BeFalse())
+	})
 })
 
 func modifyRole(ctx context.Context, nsName, roleName string) {

internal/objects/validator.go

@@ -79,10 +79,10 @@ func (v *Validator) Handle(ctx context.Context, req admission.Request) admission
 	if !config.IsManagedNamespace(req.Namespace) {
 		return webhooks.Allow("unmanaged namespace " + req.Namespace)
 	}
-	// Allow changes to the types that are not in propagate mode. This is to dynamically enable/disable
+	// Allow changes to the types that are not in Propagate or Allow mode. This is to dynamically enable/disable
 	// object webhooks based on the types configured in hncconfig. Since the current admission rules only
 	// apply to propagated objects, we can disable object webhooks on all other non-propagate-mode types.
-	if !v.isPropagateType(req.Kind) {
+	if !v.isPropagateType(req.Kind) && !v.isAllowType(req.Kind) {
 		return webhooks.Allow("Non-propagate-mode types")
 	}
 	// Finally, let the HNC SA do whatever it wants.
@@ -114,6 +114,14 @@ func (v *Validator) isPropagateType(gvk metav1.GroupVersionKind) bool {
 	return ts != nil && ts.GetMode() == api.Propagate
 }
 
+func (v *Validator) isAllowType(gvk metav1.GroupVersionKind) bool {
+	v.Forest.Lock()
+	defer v.Forest.Unlock()
+
+	ts := v.Forest.GetTypeSyncerFromGroupKind(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind})
+	return ts != nil && ts.GetMode() == api.Allow
+}
+
 // handle implements the non-webhook-y businesss logic of this validator, allowing it to be more
 // easily unit tested (ie without constructing an admission.Request, setting up user infos, etc).
 func (v *Validator) handle(ctx context.Context, req *request) admission.Response {
@@ -142,6 +150,9 @@ func (v *Validator) handle(ctx context.Context, req *request) admission.Response
 		if err := validateNoneSelectorChange(inst, oldInst); err != nil {
 			return webhooks.DenyBadRequest(err)
 		}
+		if err := validateAllSelectorChange(inst, oldInst); err != nil {
+			return webhooks.DenyBadRequest(err)
+		}
 		if msg := validateSelectorUniqueness(inst, oldInst); msg != "" {
 			return webhooks.DenyBadRequest(errors.New(msg))
 		}
@@ -169,15 +180,16 @@ func validateSelectorAnnot(inst *unstructured.Unstructured) string {
 		}
 		msg := "invalid HNC exceptions annotation: %v, should be one of the following: " +
 			api.AnnotationSelector + "; " + api.AnnotationTreeSelector + "; " +
-			api.AnnotationNoneSelector
+			api.AnnotationNoneSelector + "; " + api.AnnotationAllSelector
 		// If this annotation is part of HNC metagroup, we check if the prefix value is valid
 		if segs[0] != api.AnnotationPropagatePrefix {
 			return fmt.Sprintf(msg, key)
 		}
 		// check if the suffix is valid by checking the whole annotation key
 		if key != api.AnnotationSelector &&
 			key != api.AnnotationTreeSelector &&
-			key != api.AnnotationNoneSelector {
+			key != api.AnnotationNoneSelector &&
+			key != api.AnnotationAllSelector {
 			return fmt.Sprintf(msg, key)
 		}
 	}
@@ -188,12 +200,14 @@ func validateSelectorUniqueness(inst, oldInst *unstructured.Unstructured) string
 	sel := selectors.GetSelectorAnnotation(inst)
 	treeSel := selectors.GetTreeSelectorAnnotation(inst)
 	noneSel := selectors.GetNoneSelectorAnnotation(inst)
+	allSel := selectors.GetAllSelectorAnnotation(inst)
 
 	oldSel := selectors.GetSelectorAnnotation(oldInst)
 	oldTreeSel := selectors.GetTreeSelectorAnnotation(oldInst)
 	oldNoneSel := selectors.GetNoneSelectorAnnotation(oldInst)
+	oldAllSel := selectors.GetAllSelectorAnnotation(oldInst)
 
-	isSelectorChange := oldSel != sel || oldTreeSel != treeSel || oldNoneSel != noneSel
+	isSelectorChange := oldSel != sel || oldTreeSel != treeSel || oldNoneSel != noneSel || oldAllSel != allSel
 	if !isSelectorChange {
 		return ""
 	}
@@ -207,6 +221,9 @@ func validateSelectorUniqueness(inst, oldInst *unstructured.Unstructured) string
 	if noneSel != "" {
 		found = append(found, api.AnnotationNoneSelector)
 	}
+	if allSel != "" {
+		found = append(found, api.AnnotationAllSelector)
+	}
 	if len(found) <= 1 {
 		return ""
 	}
@@ -244,6 +261,16 @@ func validateNoneSelectorChange(inst, oldInst *unstructured.Unstructured) error
 	return err
 }
 
+func validateAllSelectorChange(inst, oldInst *unstructured.Unstructured) error {
+	oldSelectorStr := selectors.GetAllSelectorAnnotation(oldInst)
+	newSelectorStr := selectors.GetAllSelectorAnnotation(inst)
+	if newSelectorStr == "" || oldSelectorStr == newSelectorStr {
+		return nil
+	}
+	_, err := selectors.GetAllSelector(inst)
+	return err
+}
+
 func (v *Validator) handleInherited(ctx context.Context, req *request, newSource, oldSource string) admission.Response {
 	op := req.op
 	inst := req.obj