From 3e7ec483c09d10c45d793d1f8e56ef00a54f20b6 Mon Sep 17 00:00:00 2001 From: Alexej Disterhoft Date: Mon, 24 Jan 2022 15:36:35 +0100 Subject: [PATCH] Add per-object-level filtering based on labels The set of rules is hardcoded as of now, as proposed in #104. The current rules are Rancher-specific, as Rancher does some propagation of roles, rolebindings and serviceaccounts on its own via its "projects" (used to group namespaces). Label equality is checked against both key and value - no regex support for now. Tested: added a new test to check whether roles / configmap propagation is prevented if the label is matched. --- docs/user-guide/concepts.md | 8 +++++++ internal/integtest/helpers.go | 21 +++++++++++++---- internal/objects/reconciler_test.go | 35 +++++++++++++++++++++++------ internal/selectors/selectors.go | 33 ++++++++++++++++++++++++--- 4 files changed, 83 insertions(+), 14 deletions(-) diff --git a/docs/user-guide/concepts.md b/docs/user-guide/concepts.md index 8f41cf03c..face2b95e 100644 --- a/docs/user-guide/concepts.md +++ b/docs/user-guide/concepts.md @@ -361,6 +361,14 @@ adding annotations is not possible and HNC will by default exclude them. Similarly, Kubernetes ServiceAccount Secrets will also by default be excluded from propagation. +In addition, propagation exclusions are also used for Rancher-managed Kubernetes +clusters. Rancher uses a "project" concept that bundles namespaces and thus sets +roles, rolebindings, etc. for all namespaces of a project. This leads to +conflicts with HNC, so all resources created by Rancher (which are automatically +labeled with `"cattle.io/creator": "norman"` by Rancher, cf. [their +docs](https://rancher.com/docs/rancher/v2.6/en/system-tools/#remove)) are +excluded from propagation. + ## Administration diff --git a/internal/integtest/helpers.go b/internal/integtest/helpers.go index 7c0f9bcf7..bc633e8e1 100644 --- a/internal/integtest/helpers.go +++ b/internal/integtest/helpers.go @@ -300,9 +300,9 @@ func MakeObject(ctx context.Context, resource string, nsName, name string) { createdObjects = append(createdObjects, inst) } -// MakeObjectWithAnnotation creates an empty object with annotation given kind in a specific +// MakeObjectWithAnnotations creates an empty object with annotation given kind in a specific // namespace. The kind and its corresponding GVK should be included in the GVKs map. -func MakeObjectWithAnnotation(ctx context.Context, resource string, nsName, +func MakeObjectWithAnnotations(ctx context.Context, resource string, nsName, name string, a map[string]string) { inst := &unstructured.Unstructured{} inst.SetGroupVersionKind(GVKs[resource]) @@ -313,9 +313,22 @@ func MakeObjectWithAnnotation(ctx context.Context, resource string, nsName, createdObjects = append(createdObjects, inst) } -// UpdateObjectWithAnnotation gets an object given it's kind, nsName and name, adds the annotation +// MakeObjectWithLabels creates an empty object with label given kind in a specific +// namespace. The kind and its corresponding GVK should be included in the GVKs map. +func MakeObjectWithLabels(ctx context.Context, resource string, nsName, + name string, l map[string]string) { + inst := &unstructured.Unstructured{} + inst.SetGroupVersionKind(GVKs[resource]) + inst.SetNamespace(nsName) + inst.SetName(name) + inst.SetLabels(l) + ExpectWithOffset(1, K8sClient.Create(ctx, inst)).Should(Succeed()) + createdObjects = append(createdObjects, inst) +} + +// UpdateObjectWithAnnotations gets an object given it's kind, nsName and name, adds the annotation // and updates this object -func UpdateObjectWithAnnotation(ctx context.Context, resource string, nsName, +func UpdateObjectWithAnnotations(ctx context.Context, resource string, nsName, name string, a map[string]string) error { nnm := types.NamespacedName{Namespace: nsName, Name: name} inst := &unstructured.Unstructured{} diff --git a/internal/objects/reconciler_test.go b/internal/objects/reconciler_test.go index f94c86ca5..64bfe9094 100644 --- a/internal/objects/reconciler_test.go +++ b/internal/objects/reconciler_test.go @@ -142,7 +142,7 @@ var _ = Describe("Exceptions", func() { tc.treeSelector = ReplaceStrings(tc.treeSelector, names) // Create a Role with the selector and treeSelector annotation - MakeObjectWithAnnotation(ctx, api.RoleResource, names[p], "testrole", map[string]string{ + MakeObjectWithAnnotations(ctx, api.RoleResource, names[p], "testrole", map[string]string{ api.AnnotationSelector: tc.selector, api.AnnotationTreeSelector: tc.treeSelector, api.AnnotationNoneSelector: tc.noneSelector, @@ -215,7 +215,7 @@ var _ = Describe("Exceptions", func() { } // update the role with the selector and treeSelector annotation - UpdateObjectWithAnnotation(ctx, api.RoleResource, names[p], "testrole", map[string]string{ + UpdateObjectWithAnnotations(ctx, api.RoleResource, names[p], "testrole", map[string]string{ api.AnnotationSelector: tc.selector, api.AnnotationTreeSelector: tc.treeSelector, api.AnnotationNoneSelector: tc.noneSelector, @@ -232,7 +232,7 @@ var _ = Describe("Exceptions", func() { } // remove the annotation and verify that the object is back for every namespace - UpdateObjectWithAnnotation(ctx, api.RoleResource, names[p], "testrole", map[string]string{}) + UpdateObjectWithAnnotations(ctx, api.RoleResource, names[p], "testrole", map[string]string{}) for _, ns := range names { Eventually(HasObject(ctx, api.RoleResource, ns, "testrole")).Should(BeTrue(), "When propagating testrole to %s", ns) } @@ -304,7 +304,7 @@ var _ = Describe("Exceptions", func() { Eventually(HasObject(ctx, api.RoleResource, names[nolabelchild], "testrole")).Should(BeTrue(), "When propagating testrole to %s", names[nolabelchild]) // Add `select` exception annotation with propagate label and verify the // object is only propagated to children with the label. - UpdateObjectWithAnnotation(ctx, api.RoleResource, names[p], "testrole", map[string]string{ + UpdateObjectWithAnnotations(ctx, api.RoleResource, names[p], "testrole", map[string]string{ api.AnnotationSelector: label, }) Eventually(HasObject(ctx, api.RoleResource, names[nolabelchild], "testrole")).Should(BeFalse(), "When propagating testrole to %s", names[nolabelchild]) @@ -398,7 +398,7 @@ var _ = Describe("Basic propagation", func() { Expect(ObjectInheritedFrom(ctx, "configmaps", barName, "foo-config")).Should(Equal(fooName)) }) - It("should not propagate builtin exclusions", func() { + It("should not propagate builtin exclusions by name", func() { SetParent(ctx, barName, fooName) MakeObject(ctx, "configmaps", fooName, "istio-ca-root-cert") MakeObject(ctx, "configmaps", fooName, "kube-root-ca.crt") @@ -411,6 +411,27 @@ var _ = Describe("Basic propagation", func() { Eventually(HasObject(ctx, "configmaps", barName, "kube-root-ca.crt")).Should(BeFalse()) }) + It("should not propagate builtin exclusions by labels", func() { + SetParent(ctx, barName, fooName) + MakeObjectWithLabels(ctx, "roles", fooName, "role-with-labels-blocked", map[string]string{"cattle.io/creator": "norman"}) + 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()) + Eventually(HasObject(ctx, "roles", barName, "role-with-labels-wrong-value")).Should(BeTrue()) + Eventually(HasObject(ctx, "roles", barName, "role-with-labels-something")).Should(BeTrue()) + + // lets try the same with config maps, they are also filtered and should not propagate + MakeObjectWithLabels(ctx, "configmaps", fooName, "cm-with-label-1", map[string]string{"cattle.io/creator": "norman"}) + MakeObjectWithLabels(ctx, "configmaps", fooName, "cm-with-label-2", map[string]string{"app": "hello-world"}) + AddToHNCConfig(ctx, "", "configmaps", api.Propagate) + + Consistently(HasObject(ctx, "configmaps", barName, "cm-with-label-1")).Should(BeFalse()) + Eventually(HasObject(ctx, "configmaps", barName, "cm-with-label-2")).Should(BeTrue()) + }) + It("should be removed if the hierarchy changes", func() { SetParent(ctx, barName, fooName) SetParent(ctx, bazName, barName) @@ -658,7 +679,7 @@ var _ = Describe("Basic propagation", func() { It("should avoid propagating banned annotations", func() { SetParent(ctx, barName, fooName) - MakeObjectWithAnnotation(ctx, "roles", fooName, "foo-annot-role", map[string]string{ + MakeObjectWithAnnotations(ctx, "roles", fooName, "foo-annot-role", map[string]string{ "annot-a": "value-a", "annot-b": "value-b", }) @@ -682,7 +703,7 @@ var _ = Describe("Basic propagation", func() { // Tell the HNC config not to propagate annot-a and verify that this time, it's not annotated config.UnpropagatedAnnotations = []string{"annot-a"} - MakeObjectWithAnnotation(ctx, "roles", fooName, "foo-annot-role", map[string]string{ + MakeObjectWithAnnotations(ctx, "roles", fooName, "foo-annot-role", map[string]string{ "annot-a": "value-a", "annot-b": "value-b", }) diff --git a/internal/selectors/selectors.go b/internal/selectors/selectors.go index 8e718752d..afd8d842c 100644 --- a/internal/selectors/selectors.go +++ b/internal/selectors/selectors.go @@ -158,19 +158,46 @@ func GetNoneSelector(inst *unstructured.Unstructured) (bool, error) { return noneSelector, nil } -// cmExclusions are known (istio and kube-root) CA configmap which are excluded from propagation -var cmExclusions = []string{"istio-ca-root-cert", "kube-root-ca.crt"} +// 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"}, +} // isExcluded returns true to indicate that this object is excluded from being propagated func isExcluded(inst *unstructured.Unstructured) (bool, error) { name := inst.GetName() kind := inst.GetKind() group := inst.GroupVersionKind().Group + labels := inst.GetLabels() - for _, excludedResourceName := range cmExclusions { + // exclusion by name + for _, excludedResourceName := range cmExclusionsByName { if group == "" && kind == "ConfigMap" && name == excludedResourceName { return true, nil } } + + // exclusion by labels + for _, res := range exclusionByLabels { + gotLabelValue, ok := labels[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 + // causing false matches if `ok` is not checked + if ok && gotLabelValue == res.Value { + return true, nil + } + } + return false, nil }