Merge pull request #293 from duduedri96/dev/excluded-labels-cli-args

Changed default excluded labels from hardcoded values to be provided as CLI args, fixed incorrect exclusion bypass with annotation

Author: k8s-ci-robot

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")

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

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.
 

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).

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

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)

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