From b04dd8e1745df01f864dae6ba7de595bfe3018e9 Mon Sep 17 00:00:00 2001 From: Dudu Edri Date: Fri, 2 Jun 2023 11:26:48 +0300 Subject: [PATCH] adding new --exclude-resources-with-label manager CLI argument Changed default exclusion by label mechanism to use this CLI argument instead of a hardcoded list of labels. Moved the default Rancher label exclusion to be a default CLI argument on the manager. Fixed a bug that caused excluded resources to be propagated with the propagate.hnc.x-k8s.io/all annotation. Tested: made sure the current test for default exclusion fails after my CLI argument change, changed the test to reflect the changes, test now passes as expected. Wrote a test that verifies the propagate.hnc.x-k8s.io/all annotation bug doesn't occur, which failed before the bugfix. Executed e2e tests and they pass. Manually tested the manager by adding the new CLI argument and with that a labeled resource was skipped, then removed the argument and it has propagated. Did the same with two different annotations specified together as two CLI arguments, and resources that had any of those labels were skipped as expected. Manually tried to cause an excluded resource to propagate by adding the propagate.hnc.x-k8s.io/all annotation, and it didn't propagate. --- cmd/manager/main.go | 23 +++++++++++++++++++++++ config/manager/manager.yaml | 3 +++ docs/user-guide/concepts.md | 5 ++++- docs/user-guide/how-to.md | 9 +++++++++ internal/config/default_config.go | 11 +++++++++++ internal/objects/reconciler_test.go | 19 ++++++++++++++++++- internal/selectors/selectors.go | 22 ++++++---------------- 7 files changed, 74 insertions(+), 18 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 20de7f0dd..1f382ed9b 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -18,6 +18,7 @@ package main import ( "errors" "flag" + "fmt" "net/http" "os" "strings" @@ -72,6 +73,7 @@ var ( excludedNamespaces arrayArg managedNamespaceLabels arrayArg managedNamespaceAnnots arrayArg + nopropagationLabel arrayArg includedNamespacesRegex string webhooksOnly bool enableHRQ bool @@ -156,10 +158,31 @@ func parseFlags() { flag.BoolVar(&enableHRQ, "enable-hrq", false, "Enables hierarchical resource quotas") flag.StringVar(&hncNamespace, "namespace", "hnc-system", "Namespace where hnc-manager and hnc resources deployed") flag.DurationVar(&hrqSyncInterval, "hrq-sync-interval", 1*time.Minute, "Frequency to double-check that all HRQ usages are up-to-date (shouldn't be needed)") + flag.Var(&nopropagationLabel, "nopropagation-label", "A label specified as key=val that, if present, will cause HNC to skip objects that match this label. May be specified multiple times, with each key=value pair specifying one label. See the user guide for more information.") flag.Parse() // Assign the array args to the configuration variables after the args are parsed. config.UnpropagatedAnnotations = unpropagatedAnnotations + + // Assign the exclusion label args to the configuration + for _, label := range nopropagationLabel { + keyVal := strings.Split(label, "=") + if len(keyVal) != 2 { + setupLog.Info(fmt.Sprintf("--nopropagation-label must contain exactly one equal sign (=): %s", label)) + os.Exit(1) + } + labelKey := keyVal[0] + labelValue := keyVal[1] + + if len(labelKey) == 0 { + setupLog.Info(fmt.Sprintf("--nopropagation-label must not have an empty key for the label: %s", label)) + os.Exit(1) + } + + setupLog.Info(fmt.Sprintf("Will exclude objects that match label %s", label)) + config.NoPropagationLabels = append(config.NoPropagationLabels, config.NoPropagationLabel{Key: labelKey, Value: labelValue}) + } + config.SetNamespaces(includedNamespacesRegex, excludedNamespaces...) if err := config.SetManagedMeta(managedNamespaceLabels, managedNamespaceAnnots); err != nil { setupLog.Error(err, "Illegal flag values") diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 1073eb074..bc6d752f2 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -49,6 +49,9 @@ spec: - "--excluded-namespace=kube-public" - "--excluded-namespace=hnc-system" - "--excluded-namespace=kube-node-lease" + # Preserves the existing behavior of skipping Rancher objects. + # Please don't add any more nopropagation labels to the list of args. + - "--nopropagation-label=cattle.io/creator=norman" ports: - containerPort: 9443 name: webhook-server diff --git a/docs/user-guide/concepts.md b/docs/user-guide/concepts.md index 8778a2409..b1b0160a9 100644 --- a/docs/user-guide/concepts.md +++ b/docs/user-guide/concepts.md @@ -445,7 +445,10 @@ objects from being propagated by HNC. auto-created in new namespaces by Istio and Kubernetes respectively * Any objects with the label `cattle.io/creator:norman`, which are [inserted by Rancher to support - Projects](https://rancher.com/docs/rancher/v2.6/en/system-tools/#remove)) + Projects](https://rancher.com/docs/rancher/v2.6/en/system-tools/#remove)). + Can be disabled by removing the default command line argument on the manager + `--nopropagation-label=cattle.io/creator=norman`. Refer to [How-to: + Modify command-line arguments](how-to.md#modify-command-line-arguments) for more details. * *HNC v1.1+:* Secrets with type `helm.sh/release.v1`, which is auto-created in the namespaces where their respective Helm releases are deployed to. diff --git a/docs/user-guide/how-to.md b/docs/user-guide/how-to.md index 22af4feac..38a335d28 100644 --- a/docs/user-guide/how-to.md +++ b/docs/user-guide/how-to.md @@ -972,3 +972,12 @@ Interesting parameters include: load on your metrics database (through increased metric cardinality) and also by increasing how carefully you need to guard your metrics against unauthorized viewers. +* `--nopropagation-label`: has the same effect as the `propagate.hnc.x-k8s.io/none` + annotation as specified in [limiting propagation](how-to.md#limit-the-propagation-of-an-object-to-descendant-namespaces), + but is useful when there's no control over what annotations the object has in order + to disable the propagation of that object. This argument may be specified multiple + times, with each parameter representing one `key=val` pair of a label that should + exclude an object from propagation. + * Rancher objects that have the label `cattle.io/creator=norman` are not propagated + by the default manifests (refer to [Concepts: built in exceptions](concepts.md#built-in-exceptions) + for more information). \ No newline at end of file diff --git a/internal/config/default_config.go b/internal/config/default_config.go index b6c1a830f..f35e6a03a 100644 --- a/internal/config/default_config.go +++ b/internal/config/default_config.go @@ -8,3 +8,14 @@ package config // This value is controlled by the --unpropagated-annotation command line, which may be set multiple // times. var UnpropagatedAnnotations []string + +// NoPropagationLabel specifies a label Key and Value which will cause an object to be excluded +// from propagation if the object defines that label with this specific value. +type NoPropagationLabel struct { + Key string + Value string +} + +// NoPropagationLabels is a configuration slice that contains all NoPropagationLabel labels that should +// cause objects to be ignored from propagation. +var NoPropagationLabels []NoPropagationLabel diff --git a/internal/objects/reconciler_test.go b/internal/objects/reconciler_test.go index d6c4d8361..d6b4b2a0f 100644 --- a/internal/objects/reconciler_test.go +++ b/internal/objects/reconciler_test.go @@ -446,15 +446,20 @@ var _ = Describe("Basic propagation", func() { Eventually(HasObject(ctx, "secrets", barName, "helm-secret")).Should(BeFalse()) }) - It("should not propagate builtin exclusions by labels", func() { + It("should not propagate objects excluded by labels", func() { SetParent(ctx, barName, fooName) + config.NoPropagationLabels = append(config.NoPropagationLabels, config.NoPropagationLabel{Key: "cattle.io/creator", Value: "norman"}) + config.NoPropagationLabels = append(config.NoPropagationLabels, config.NoPropagationLabel{Key: "ignore-label", Value: "label"}) + MakeObjectWithLabels(ctx, "roles", fooName, "role-with-labels-blocked", map[string]string{"cattle.io/creator": "norman"}) + MakeObjectWithLabels(ctx, "roles", fooName, "role-with-labels-blocked-2", map[string]string{"ignore-label": "label"}) MakeObjectWithLabels(ctx, "roles", fooName, "role-with-labels-wrong-value", map[string]string{"cattle.io/creator": "testme"}) MakeObjectWithLabels(ctx, "roles", fooName, "role-with-labels-something", map[string]string{"app": "hello-world"}) AddToHNCConfig(ctx, api.RBACGroup, api.RoleKind, api.Propagate) // the first one should not propagate, everything else should Consistently(HasObject(ctx, "roles", barName, "role-with-labels-blocked")).Should(BeFalse()) + Consistently(HasObject(ctx, "roles", barName, "role-with-labels-blocked-2")).Should(BeFalse()) Eventually(HasObject(ctx, "roles", barName, "role-with-labels-wrong-value")).Should(BeTrue()) Eventually(HasObject(ctx, "roles", barName, "role-with-labels-something")).Should(BeTrue()) @@ -467,6 +472,18 @@ var _ = Describe("Basic propagation", func() { Eventually(HasObject(ctx, "configmaps", barName, "cm-with-label-2")).Should(BeTrue()) }) + It("should not propagate objects excluded by labels when using all annotation", func() { + SetParent(ctx, barName, fooName) + config.NoPropagationLabels = append(config.NoPropagationLabels, config.NoPropagationLabel{Key: "cattle.io/creator", Value: "norman"}) + + MakeObjectWithLabels(ctx, "roles", fooName, "role-with-labels-blocked", map[string]string{"cattle.io/creator": "norman"}) + UpdateObjectWithAnnotations(ctx, "roles", fooName, "role-with-labels-blocked", map[string]string{"propagate.hnc.x-k8s.io/all": "true"}) + AddToHNCConfig(ctx, api.RBACGroup, api.RoleKind, api.Propagate) + + // The object should not be propagated even though it uses the `all` annotation + Consistently(HasObject(ctx, "roles", barName, "role-with-labels-blocked")).Should(BeFalse()) + }) + It("should be removed if the hierarchy changes", func() { SetParent(ctx, barName, fooName) SetParent(ctx, bazName, barName) diff --git a/internal/selectors/selectors.go b/internal/selectors/selectors.go index 380c06ba9..c2177b68e 100644 --- a/internal/selectors/selectors.go +++ b/internal/selectors/selectors.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" + "sigs.k8s.io/hierarchical-namespaces/internal/config" ) // ShouldPropagate returns true if the given object should be propagated @@ -43,8 +44,10 @@ func ShouldPropagate(inst *unstructured.Unstructured, nsLabels labels.Set, mode if none, err := GetNoneSelector(inst); err != nil || none { return false, err } - if all, err := GetAllSelector(inst); err != nil || all { - return true, err + if all, err := GetAllSelector(inst); err != nil { + return false, err + } else if all { + propIfNotExcluded = true } if excluded, err := isExcluded(inst); excluded { return false, err @@ -197,19 +200,6 @@ func GetAllSelector(inst *unstructured.Unstructured) (bool, error) { // 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"} -// A label as a key, value pair, used to exclude resources matching this label (both key and value). -type ExclusionByLabelsSpec struct { - Key string - Value string -} - -// ExclusionByLabelsSpec are known label key-value pairs which are excluded from propagation. Right -// now only used to exclude resources created by Rancher, see "System Tools > Remove" -// (https://rancher.com/docs/rancher/v2.6/en/system-tools/#remove) -var exclusionByLabels = []ExclusionByLabelsSpec{ - {Key: "cattle.io/creator", Value: "norman"}, -} - // A annotation as a key, value pair, used to exclude resources matching this annotation type ExclusionByAnnotationsSpec struct { Key string @@ -246,7 +236,7 @@ func isExcluded(inst *unstructured.Unstructured) (bool, error) { } // exclusion by labels - for _, res := range exclusionByLabels { + for _, res := range config.NoPropagationLabels { gotLabelValue, ok := inst.GetLabels()[res.Key] // check for presence has to be explicit, as empty label values are allowed and a // nonexisting key in the `labels` map will also return an empty string ("") - potentially