diff --git a/api/v1alpha2/hierarchy_types.go b/api/v1alpha2/hierarchy_types.go index 436792527..051329b7f 100644 --- a/api/v1alpha2/hierarchy_types.go +++ b/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). diff --git a/api/v1alpha2/hnc_config.go b/api/v1alpha2/hnc_config.go index decee4771..25883e03d 100644 --- a/api/v1alpha2/hnc_config.go +++ b/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"` } diff --git a/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml b/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml index b99de63d1..6a8db924e 100644 --- a/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml +++ b/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. diff --git a/internal/forest/forest.go b/internal/forest/forest.go index 8a196a9dc..c95d5d541 100644 --- a/internal/forest/forest.go +++ b/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 } diff --git a/internal/hierarchyconfig/validator.go b/internal/hierarchyconfig/validator.go index 2b0b7bea4..770dfe312 100644 --- a/internal/hierarchyconfig/validator.go +++ b/internal/hierarchyconfig/validator.go @@ -246,11 +246,11 @@ 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 'AllowPropogate' mode to find any conflicts. conflicts := []string{} for _, t := range v.Forest.GetTypeSyncers() { - if t.GetMode() == api.Propagate { - conflicts = append(conflicts, v.getConflictingObjectsOfType(t.GetGVK(), newParent, ns)...) + if t.CanPropagate() { + conflicts = append(conflicts, v.getConflictingObjectsOfType(t.GetGVK(), t.GetMode(), newParent, ns)...) } } return conflicts @@ -258,7 +258,7 @@ func (v *Validator) getConflictingObjects(newParent, ns *forest.Namespace) []str // 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 { +func (v *Validator) getConflictingObjectsOfType(gvk schema.GroupVersionKind, mode api.SynchronizationMode, newParent, ns *forest.Namespace) []string { // Get all the source objects in the new ancestors that would be propagated // into the descendants. newAnsSrcObjs := make(map[string]bool) @@ -266,7 +266,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(), mode); ok { newAnsSrcObjs[nnm.Name] = true } } diff --git a/internal/hncconfig/reconciler.go b/internal/hncconfig/reconciler.go index 6354d17d4..adb694097 100644 --- a/internal/hncconfig/reconciler.go +++ b/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 { diff --git a/internal/hncconfig/validator.go b/internal/hncconfig/validator.go index 60bfa0209..0293621a4 100644 --- a/internal/hncconfig/validator.go +++ b/internal/hncconfig/validator.go @@ -127,14 +127,14 @@ func (v *Validator) checkForest(ts gvkSet) admission.Response { v.Forest.Lock() defer v.Forest.Unlock() - // Get types that are changed from other modes to "Propagate" mode. + // Get types that are changed from other modes to "Propagate" mode or "AllowPropagate" mode. gvks := v.getNewPropagateTypes(ts) // Check if user-created objects would be overwritten by these mode changes. - for gvk := range gvks { - conflicts := v.checkConflictsForGVK(gvk) + for gvk, mode := range gvks { + conflicts := v.checkConflictsForGVK(gvk, mode) if len(conflicts) != 0 { - msg := fmt.Sprintf("Cannot update configuration because setting type %q to 'Propagate' mode would overwrite user-created object(s):\n", gvk) + msg := fmt.Sprintf("Cannot update configuration because setting type %q to 'Propagate' mode or 'AllowPropagate' mode would overwrite user-created object(s):\n", gvk) msg += strings.Join(conflicts, "\n") msg += "\nTo fix this, please rename or remove the conflicting objects first." err := errors.New(msg) @@ -147,17 +147,17 @@ func (v *Validator) checkForest(ts gvkSet) admission.Response { } // checkConflictsForGVK looks for conflicts from top down for each tree. -func (v *Validator) checkConflictsForGVK(gvk schema.GroupVersionKind) []string { +func (v *Validator) checkConflictsForGVK(gvk schema.GroupVersionKind, mode api.SynchronizationMode) []string { conflicts := []string{} for _, ns := range v.Forest.GetRoots() { - conflicts = append(conflicts, v.checkConflictsForTree(gvk, ancestorObjects{}, ns)...) + conflicts = append(conflicts, v.checkConflictsForTree(gvk, ancestorObjects{}, ns, mode)...) } return conflicts } // 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 { +func (v *Validator) checkConflictsForTree(gvk schema.GroupVersionKind, ao ancestorObjects, ns *forest.Namespace, mode api.SynchronizationMode) []string { conflicts := []string{} // make a local copy of the ancestorObjects so that the original copy doesn't get modified objs := ao.copy() @@ -167,7 +167,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(), mode); ok { conflicts = append(conflicts, fmt.Sprintf(" Object %q in namespace %q would overwrite the one in %q", onm, nnm, ns.Name())) } } @@ -178,26 +178,29 @@ func (v *Validator) checkConflictsForTree(gvk schema.GroupVersionKind, ao ancest // it's impossible to get cycles from non-root. for _, cnm := range ns.ChildNames() { cns := v.Forest.Get(cnm) - conflicts = append(conflicts, v.checkConflictsForTree(gvk, objs, cns)...) + conflicts = append(conflicts, v.checkConflictsForTree(gvk, objs, cns, mode)...) } return conflicts } // getNewPropagateTypes returns a set of types that are changed from other modes -// to `Propagate` mode. +// to `Propagate` or `AllowPropagate` mode. func (v *Validator) getNewPropagateTypes(ts gvkSet) gvkSet { - // Get all "Propagate" mode types in the new configuration. + // Get all "Propagate" mode and "AllowPropagate" mode types in the new configuration. newPts := gvkSet{} for gvk, mode := range ts { if mode == api.Propagate { newPts[gvk] = api.Propagate } + if mode == api.AllowPropagate { + newPts[gvk] = api.AllowPropagate + } } - // Remove all existing "Propagate" mode types in the forest (current configuration). + // Remove all existing "Propagate" mode and "AllowPropagate" mode types in the forest (current configuration). for _, t := range v.Forest.GetTypeSyncers() { _, exist := newPts[t.GetGVK()] - if t.GetMode() == api.Propagate && exist { + if t.CanPropagate() && exist { delete(newPts, t.GetGVK()) } } diff --git a/internal/kubectl/configdescribe.go b/internal/kubectl/configdescribe.go index ebd46940d..b4da5c22c 100644 --- a/internal/kubectl/configdescribe.go +++ b/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" } diff --git a/internal/kubectl/configset.go b/internal/kubectl/configset.go index 4872a7a40..83f78d93e 100644 --- a/internal/kubectl/configset.go +++ b/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,7 +40,7 @@ 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 := normalizeMode(modeStr) force, _ := flags.GetBool("force") config := client.getHNCConfig() @@ -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 || 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,19 @@ 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 } + +// normalizeMode takes a user-input mode and returns +// a SynchronizationMode in a format HNC expects +func normalizeMode(modeStr string) api.SynchronizationMode { + mode := api.SynchronizationMode(cases.Title(language.English).String(modeStr)) + + if mode == "Allowpropagate" { + return api.AllowPropagate + } + + return mode +} diff --git a/internal/objects/reconciler.go b/internal/objects/reconciler.go index e16375213..c2d1431e4 100644 --- a/internal/objects/reconciler.go +++ b/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 non-'AllowPropagate' 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); err != nil { log.Error(err, "Cannot propagate") r.EventRecorder.Event(inst, "Warning", api.EventCannotParseSelector, err.Error()) return false diff --git a/internal/objects/reconciler_test.go b/internal/objects/reconciler_test.go index ce9b24899..09fff0cea 100644 --- a/internal/objects/reconciler_test.go +++ b/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) { diff --git a/internal/objects/validator.go b/internal/objects/validator.go index 3649b2052..9d2407eb1 100644 --- a/internal/objects/validator.go +++ b/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 AllowPropagate 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.canPropagateType(req.Kind) { return webhooks.Allow("Non-propagate-mode types") } // Finally, let the HNC SA do whatever it wants. @@ -106,12 +106,26 @@ func (v *Validator) Handle(ctx context.Context, req admission.Request) admission return resp } +func (v *Validator) syncType(gvk metav1.GroupVersionKind) api.SynchronizationMode { + ts := v.Forest.GetTypeSyncerFromGroupKind(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind}) + if ts == nil { + return api.Ignore + } + return ts.GetMode() +} + func (v *Validator) isPropagateType(gvk metav1.GroupVersionKind) bool { + return v.syncType(gvk) == api.Propagate +} + +func (v *Validator) isAllowPropagateType(gvk metav1.GroupVersionKind) bool { + return v.syncType(gvk) == api.AllowPropagate +} + +func (v *Validator) canPropagateType(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.Propagate + return (v.isPropagateType(gvk) || v.isAllowPropagateType(gvk)) } // handle implements the non-webhook-y businesss logic of this validator, allowing it to be more @@ -142,6 +156,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)) } @@ -168,8 +185,10 @@ func validateSelectorAnnot(inst *unstructured.Unstructured) string { continue } msg := "invalid HNC exceptions annotation: %v, should be one of the following: " + - api.AnnotationSelector + "; " + api.AnnotationTreeSelector + "; " + - api.AnnotationNoneSelector + api.AnnotationSelector + + "; " + api.AnnotationTreeSelector + + "; " + 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) @@ -177,7 +196,8 @@ func validateSelectorAnnot(inst *unstructured.Unstructured) string { // 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 +208,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 +229,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 +269,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 @@ -347,7 +382,8 @@ func (v *Validator) hasConflict(inst *unstructured.Unstructured) (bool, []string // If the user have chosen not to propagate the object to this descendant, // there shouldn't be any conflict reported here nsLabels := v.Forest.Get(inst.GetNamespace()).GetLabels() - if ok, _ := selectors.ShouldPropagate(inst, nsLabels); ok { + mode := v.syncType(metav1.GroupVersionKind(gvk)) + if ok, _ := selectors.ShouldPropagate(inst, nsLabels, mode); ok { conflicts = append(conflicts, desc) } } diff --git a/internal/objects/validator_test.go b/internal/objects/validator_test.go index 1fa82aab1..8faa57031 100644 --- a/internal/objects/validator_test.go +++ b/internal/objects/validator_test.go @@ -540,6 +540,34 @@ func TestUserChanges(t *testing.T) { }, }, }, + }, { + name: "Deny creation of object with invalid all annotation", + fail: true, + inst: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + api.AnnotationAllSelector: "foo", + }, + }, + }, + }, + }, { + name: "Allow creation of object with valid all annotation", + fail: false, + inst: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + api.AnnotationAllSelector: "true", + }, + }, + }, + }, }, { name: "Deny creation of object with invalid selector and valid treeSelect annotation", fail: true, @@ -752,8 +780,16 @@ func TestCreatingConflictSource(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup + // validator needs to know whether resource has Propagate mode or AllowPropagate mode + // in order to check for conflicts in propagation, hence a reconciler is + // initialized and is added to the forest + r := &Reconciler{ + GVK: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, + Mode: api.Propagate, + } g := NewWithT(t) f := foresttest.Create(tc.forest) + f.AddTypeSyncer(r) foresttest.CreateSecret(tc.conflictInstName, tc.conflictNamespace, f) v := &Validator{Forest: f} op := k8sadm.Create diff --git a/internal/selectors/selectors.go b/internal/selectors/selectors.go index 40c45ada9..6659e5b88 100644 --- a/internal/selectors/selectors.go +++ b/internal/selectors/selectors.go @@ -15,24 +15,42 @@ import ( api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" ) -func ShouldPropagate(inst *unstructured.Unstructured, nsLabels labels.Set) (bool, error) { +// ShouldPropagate returns true if the given object should be propagated +// based on the SynchronizationMode and on the selectors of the object. +// Propagation will be done only with 'Propagate' or 'AllowPropagate' modes. +// When selectors are set, 'AllowPropagate' follows the same logic as 'Propagate' mode, +// when they are not set, then if 'Propagate' mode is used then propagate by default; +// if 'AllowPropagate' mode is used then do not propagate by default. +func ShouldPropagate(inst *unstructured.Unstructured, nsLabels labels.Set, mode api.SynchronizationMode) (bool, error) { + propIfNotExcluded := (mode == api.Propagate) + if sel, err := GetSelector(inst); err != nil { return false, err - } else if sel != nil && !sel.Matches(nsLabels) { - return false, nil + } else if sel != nil && !sel.Empty() { + propIfNotExcluded = true + if !sel.Matches(nsLabels) { + return false, nil + } } if sel, err := GetTreeSelector(inst); err != nil { return false, err - } else if sel != nil && !sel.Matches(nsLabels) { - return false, nil + } else if sel != nil && !sel.Empty() { + propIfNotExcluded = true + if !sel.Matches(nsLabels) { + return false, nil + } } if none, err := GetNoneSelector(inst); err != nil || none { return false, err } + if all, err := GetAllSelector(inst); err != nil || all { + return true, err + } if excluded, err := isExcluded(inst); excluded { return false, err } - return true, nil + + return propIfNotExcluded, nil } func GetSelectorAnnotation(inst *unstructured.Unstructured) string { @@ -50,6 +68,11 @@ func GetNoneSelectorAnnotation(inst *unstructured.Unstructured) string { return annot[api.AnnotationNoneSelector] } +func GetAllSelectorAnnotation(inst *unstructured.Unstructured) string { + annot := inst.GetAnnotations() + return annot[api.AnnotationAllSelector] +} + // GetTreeSelector is similar to a regular selector, except that it adds the LabelTreeDepthSuffix to every string // To transform a tree selector into a regular label selector, we follow these steps: // 1. get the treeSelector annotation if it exists @@ -148,16 +171,29 @@ func GetNoneSelector(inst *unstructured.Unstructured) (bool, error) { noneSelector, err := strconv.ParseBool(noneSelectorStr) if err != nil { // Returning false here in accordance to the Go standard - return false, - fmt.Errorf("invalid %s value: %w", - api.AnnotationNoneSelector, - err, - ) + return false, fmt.Errorf("invalid %s value: %w", api.AnnotationNoneSelector, err) } return noneSelector, nil } +// GetAllSelector returns true indicates that user do wants this object to be propagated +func GetAllSelector(inst *unstructured.Unstructured) (bool, error) { + allSelectorStr := GetAllSelectorAnnotation(inst) + // Empty string is treated as 'false'. In other selector cases, the empty string is auto converted to + // a selector that matches everything. + if allSelectorStr == "" { + return false, nil + } + allSelector, err := strconv.ParseBool(allSelectorStr) + if err != nil { + // Returning false here in accordance to the Go standard + return false, fmt.Errorf("invalid %s value: %w", api.AnnotationAllSelector, err) + + } + return allSelector, nil +} + // cmExclusionsByName are known (istio and kube-root) CA configmap which are excluded from propagation var cmExclusionsByName = []string{"istio-ca-root-cert", "kube-root-ca.crt"} diff --git a/pkg/testutils/testutils.go b/pkg/testutils/testutils.go index a465d94ec..1c2399cc4 100644 --- a/pkg/testutils/testutils.go +++ b/pkg/testutils/testutils.go @@ -276,6 +276,12 @@ func CleanupTestNamespaces() { cleanupNamespaces(nses...) } +// CleanupTestResourceSyncMode changes the synchronization mode of resources +// used in testing back to its default value +func CleanupTestResourceSyncMode() { + MustRun("kubectl hns config set-resource secrets --mode Ignore") +} + // CleanupCRDIfExists checks whether the eetests.e2e.hnc.x-k8s.io CRD is present, if so, delete it. func CleanupTestCRDIfExists() { crd := "eetests.e2e.hnc.x-k8s.io" diff --git a/test/e2e/issues_test.go b/test/e2e/issues_test.go index 5a2abc42c..7280f7015 100644 --- a/test/e2e/issues_test.go +++ b/test/e2e/issues_test.go @@ -22,6 +22,7 @@ var _ = Describe("Issues", func() { BeforeEach(func() { CleanupTestNamespaces() + CleanupTestResourceSyncMode() }) AfterEach(func() { @@ -339,6 +340,52 @@ spec: MustApplyYAML(eetestUpdated) FieldShouldContainWithTimeout("eetests.e2e.hnc.x-k8s.io", nsChild, "eetest-sample", ".spec.foo", "bar", 30) }) + + It("Should allow objects to be propagated only when a selector is set in AllowPropagate mode - issue #16", func() { + // create a simple structure and get an object propagated + CreateNamespace(nsParent) + CreateNamespace(nsChild) + MustRun("kubectl hns set", nsChild, "--parent", nsParent) + + // creare secret on parent namespace + MustRun("kubectl -n", nsParent, "create secret generic my-creds --from-literal=password=iamteamb") + + // after setting Propagate mode, the object shows up in the child namespace + MustRun("kubectl hns config set-resource secrets --mode Propagate --force") + RunShouldContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + + // after setting AllowPropagate mode, the object is removed from the child namespace + MustRun("kubectl hns config set-resource secrets --mode AllowPropagate --force") + RunShouldNotContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + + // after adding a Selector, the object shows up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/select="+nsChild+".tree.hnc.x-k8s.io/depth") + RunShouldContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + // after removing a Selector, the object does not show up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/select-") + RunShouldNotContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + + // after adding a treeSelector, the object shows up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/treeSelect="+nsChild) + RunShouldContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + // after removing a treeSelector, the object does not show up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/treeSelect-") + RunShouldNotContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + + // after adding a noneSelector, the object does not show up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/none=true") + RunShouldNotContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + // after removing a noneSelector, the object does not show up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/none-") + RunShouldNotContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + + // after adding a allSelector, the object shows up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/all=true") + RunShouldContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + // after removing a allSelector, the object does not show up in the child namespace + MustRun("kubectl annotate secret my-creds -n", nsParent, "propagate.hnc.x-k8s.io/all-") + RunShouldNotContain("my-creds", defTimeout, "kubectl -n", nsChild, "get secrets") + }) }) var _ = Describe("Issues with bad anchors", func() { diff --git a/test/e2e/kubectl_test.go b/test/e2e/kubectl_test.go index 015093458..5737de56d 100644 --- a/test/e2e/kubectl_test.go +++ b/test/e2e/kubectl_test.go @@ -6,11 +6,16 @@ import ( ) var _ = Describe("HNS set-config", func() { - It("Should use '--force' flag to change from 'Ignore' to 'Propagate'", func() { + It("Should use '--force' flag to change from 'Ignore' to 'Propagate' or 'AllowPropagate'", func() { MustRun("kubectl hns config set-resource secrets --mode Ignore") + MustNotRun("kubectl hns config set-resource secrets --mode Propagate") MustRun("kubectl hns config set-resource secrets --mode Propagate --force") // check that we don't need '--force' flag when changing it back MustRun("kubectl hns config set-resource secrets --mode Ignore") + + MustNotRun("kubectl hns config set-resource secrets --mode AllowPropagate") + MustRun("kubectl hns config set-resource secrets --mode AllowPropagate --force") + MustRun("kubectl hns config set-resource secrets --mode Ignore") }) })