From 9a1e59f38232de768348aa4eec0ceffc222b25e8 Mon Sep 17 00:00:00 2001 From: mzeevi Date: Thu, 25 Aug 2022 11:39:03 +0300 Subject: [PATCH] Inclusive configuration of resources to propagate See issue #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 --- api/v1alpha2/hierarchy_types.go | 1 + api/v1alpha2/hnc_config.go | 8 ++- .../bases/hnc.x-k8s.io_hncconfigurations.yaml | 1 + internal/forest/forest.go | 3 + internal/hierarchyconfig/validator.go | 10 ++-- internal/hncconfig/reconciler.go | 4 +- internal/hncconfig/validator.go | 29 +++++----- internal/kubectl/configdescribe.go | 2 + internal/kubectl/configset.go | 26 ++++++--- internal/objects/reconciler.go | 21 ++++--- internal/objects/reconciler_test.go | 48 +++++++++++++++ internal/objects/validator.go | 56 ++++++++++++++---- internal/objects/validator_test.go | 36 ++++++++++++ internal/selectors/selectors.go | 58 +++++++++++++++---- test/e2e/issues_test.go | 48 +++++++++++++++ test/e2e/kubectl_test.go | 7 ++- 16 files changed, 300 insertions(+), 58 deletions(-) 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/test/e2e/issues_test.go b/test/e2e/issues_test.go index 5a2abc42c..e9511f5a3 100644 --- a/test/e2e/issues_test.go +++ b/test/e2e/issues_test.go @@ -339,6 +339,54 @@ 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") + + MustRun("kubectl hns config set-resource secrets --mode Ignore") + }) }) 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") }) })