From e50bacffd737fb94d12175a21e2276d04a07daa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20B=C3=A4hler?= <26610571+oliverbaehler@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:54:31 +0200 Subject: [PATCH] fix: validate only changed metadata on updates (#2118) Signed-off-by: Oliver Baehler --- e2e/rules_enforce_metadata_test.go | 288 +++++++++++++++++- .../rules/generic/validation/factory.go | 16 +- .../rules/generic/validation/factory_test.go | 51 ++-- .../rules/generic/validation/metadata.go | 56 +++- .../rules/generic/validation/metadata_test.go | 136 ++++++++- 5 files changed, 514 insertions(+), 33 deletions(-) diff --git a/e2e/rules_enforce_metadata_test.go b/e2e/rules_enforce_metadata_test.go index 8fcfd357..e4857d5f 100644 --- a/e2e/rules_enforce_metadata_test.go +++ b/e2e/rules_enforce_metadata_test.go @@ -5,6 +5,7 @@ package e2e import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -29,7 +30,14 @@ import ( ) var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("tenant", "rules", "enforce", "metadata", "generic"), func() { - const ownerName = "e2e-rules-metadata" + const ( + ownerName = "e2e-rules-metadata" + secondOwnerName = "e2e-rules-metadata-second" + ownerLabel = "example.corp/label-a" + secondOwnerLabel = "example.corp/label-b" + ownerAnnotation = "example.corp/annotation-a" + secondOwnerAnnotation = "example.corp/annotation-b" + ) var ( tnt *capsulev1beta2.Tenant @@ -112,6 +120,65 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t return rule } + audienceRule := func( + audience rules.Audience, + rule *rules.NamespaceRuleBodyTenant, + ) *rules.NamespaceRuleBodyTenant { + rule.Audience = []rules.Audience{audience} + + return rule + } + + isolatedMetadataRules := func() []*rules.NamespaceRuleBodyTenant { + return []*rules.NamespaceRuleBodyTenant{ + audienceRule( + rules.Audience{ + Kind: rules.AudienceKindCustom, + Name: string(rules.CustomAudienceCapsuleUser), + }, + metadataRule( + rules.ActionTypeDeny, + "v1", + []string{"ConfigMap"}, + map[string]rules.MetadataValueRule{ + ".*": metadataValueRule(false, metadataByExpression(".*")), + }, + map[string]rules.MetadataValueRule{ + ".*": metadataValueRule(false, metadataByExpression(".*")), + }, + ), + ), + audienceRule( + rules.Audience{Kind: rules.AudienceKindUser, Name: ownerName}, + metadataRule( + rules.ActionTypeAllow, + "v1", + []string{"ConfigMap"}, + map[string]rules.MetadataValueRule{ + ownerLabel: metadataValueRule(false, metadataByExact("owner-a")), + }, + map[string]rules.MetadataValueRule{ + ownerAnnotation: metadataValueRule(false, metadataByExact("owner-a")), + }, + ), + ), + audienceRule( + rules.Audience{Kind: rules.AudienceKindUser, Name: secondOwnerName}, + metadataRule( + rules.ActionTypeAllow, + "v1", + []string{"ConfigMap"}, + map[string]rules.MetadataValueRule{ + secondOwnerLabel: metadataValueRule(false, metadataByExact("owner-b")), + }, + map[string]rules.MetadataValueRule{ + secondOwnerAnnotation: metadataValueRule(false, metadataByExact("owner-b")), + }, + ), + ), + } + } + baseTenantRules := func() []*rules.NamespaceRuleBodyTenant { return []*rules.NamespaceRuleBodyTenant{ metadataRule( @@ -190,6 +257,14 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t }, }, }, + { + CoreOwnerSpec: rbac.CoreOwnerSpec{ + UserSpec: rbac.UserSpec{ + Name: secondOwnerName, + Kind: "User", + }, + }, + }, }, Rules: tenantRules, }, @@ -574,6 +649,62 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) } + patchConfigMapMetadata := func( + cs kubernetes.Interface, + nsName string, + cmName string, + labels map[string]string, + annotations map[string]string, + ) error { + metadataPatch := map[string]any{} + if labels != nil { + metadataPatch["labels"] = labels + } + if annotations != nil { + metadataPatch["annotations"] = annotations + } + + patch, err := json.Marshal(map[string]any{"metadata": metadataPatch}) + if err != nil { + return err + } + + _, err = cs.CoreV1().ConfigMaps(nsName).Patch( + context.Background(), + cmName, + k8stypes.MergePatchType, + patch, + metav1.PatchOptions{}, + ) + + return err + } + + patchConfigMapMetadataAndExpectDenied := func( + cs kubernetes.Interface, + nsName string, + cmName string, + labels map[string]string, + annotations map[string]string, + substrings ...string, + ) { + Eventually(func() error { + err := patchConfigMapMetadata(cs, nsName, cmName, labels, annotations) + if err == nil { + return fmt.Errorf("expected configmap metadata patch to be denied, but it succeeded") + } + + message := err.Error() + for _, substring := range substrings { + if !strings.Contains(message, substring) { + return fmt.Errorf("expected error to contain %q, got: %s", substring, message) + } + } + + return nil + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + } + createServiceAndExpectAllowed := func(cs kubernetes.Interface, nsName string, svc *corev1.Service) { EventuallyCreation(func() error { _, err := cs.CoreV1().Services(nsName).Create(context.Background(), svc, metav1.CreateOptions{}) @@ -719,6 +850,25 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) } + prepareIsolatedMetadataTest := func(name string) (*corev1.Namespace, kubernetes.Interface, kubernetes.Interface, *corev1.ConfigMap) { + updateTenantRules(isolatedMetadataRules()) + + ns := createNamespace(nil) + waitForProjectedMetadata(ns.Name, secondOwnerLabel, nil, nil) + + owner := ownerClient(tnt.Spec.Owners[0].UserSpec) + secondOwner := ownerClient(tnt.Spec.Owners[1].UserSpec) + cm := configMap( + name, + map[string]string{ownerLabel: "owner-a"}, + map[string]string{ownerAnnotation: "owner-a"}, + ) + + createConfigMapAndExpectAllowed(owner, ns.Name, cm) + + return ns, owner, secondOwner, cm + } + BeforeEach(func() { tenantRules = baseTenantRules() }) @@ -1358,6 +1508,142 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t ) }) + It("allows an audience to patch its metadata while another audience's metadata is unchanged", func() { + ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-isolated-allowed") + + By("patching only the label and annotation assigned to the second owner") + Eventually(func() error { + return patchConfigMapMetadata( + secondOwner, + ns.Name, + cm.Name, + map[string]string{secondOwnerLabel: "owner-b"}, + map[string]string{secondOwnerAnnotation: "owner-b"}, + ) + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + + By("preserving both audiences' metadata") + Eventually(func(g Gomega) { + current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get( + context.Background(), + cm.Name, + metav1.GetOptions{}, + ) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(current.Labels).To(HaveKeyWithValue(ownerLabel, "owner-a")) + g.Expect(current.Labels).To(HaveKeyWithValue(secondOwnerLabel, "owner-b")) + g.Expect(current.Annotations).To(HaveKeyWithValue(ownerAnnotation, "owner-a")) + g.Expect(current.Annotations).To(HaveKeyWithValue(secondOwnerAnnotation, "owner-b")) + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + }) + + It("denies patches to metadata assigned to another audience", func() { + ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-other-audience-denied") + + By("denying a change to the first owner's label") + patchConfigMapMetadataAndExpectDenied( + secondOwner, + ns.Name, + cm.Name, + map[string]string{ownerLabel: "tampered"}, + nil, + ownerLabel, + "tampered", + "denied", + ) + + By("denying a change to the first owner's annotation") + patchConfigMapMetadataAndExpectDenied( + secondOwner, + ns.Name, + cm.Name, + nil, + map[string]string{ownerAnnotation: "tampered"}, + ownerAnnotation, + "tampered", + "denied", + ) + + By("preserving the first owner's metadata") + Eventually(func(g Gomega) { + current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get( + context.Background(), + cm.Name, + metav1.GetOptions{}, + ) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(current.Labels).To(HaveKeyWithValue(ownerLabel, "owner-a")) + g.Expect(current.Annotations).To(HaveKeyWithValue(ownerAnnotation, "owner-a")) + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + }) + + It("denies disallowed values for metadata assigned to the updating audience", func() { + ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-own-value-denied") + + By("denying a disallowed value for the second owner's label") + patchConfigMapMetadataAndExpectDenied( + secondOwner, + ns.Name, + cm.Name, + map[string]string{secondOwnerLabel: "blocked"}, + nil, + secondOwnerLabel, + "blocked", + "denied", + ) + + By("denying a disallowed value for the second owner's annotation") + patchConfigMapMetadataAndExpectDenied( + secondOwner, + ns.Name, + cm.Name, + nil, + map[string]string{secondOwnerAnnotation: "blocked"}, + secondOwnerAnnotation, + "blocked", + "denied", + ) + }) + + It("allows non-metadata updates when another audience's metadata is unchanged", func() { + ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-data-allowed") + + By("updating ConfigMap data as the second owner") + Eventually(func() error { + current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get( + context.Background(), + cm.Name, + metav1.GetOptions{}, + ) + if err != nil { + return err + } + + current.Data["key"] = "updated" + + _, err = secondOwner.CoreV1().ConfigMaps(ns.Name).Update( + context.Background(), + current, + metav1.UpdateOptions{}, + ) + + return err + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + + By("preserving the metadata and the data update") + Eventually(func(g Gomega) { + current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get( + context.Background(), + cm.Name, + metav1.GetOptions{}, + ) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(current.Data).To(HaveKeyWithValue("key", "updated")) + g.Expect(current.Labels).To(HaveKeyWithValue(ownerLabel, "owner-a")) + g.Expect(current.Annotations).To(HaveKeyWithValue(ownerAnnotation, "owner-a")) + }, defaultTimeoutInterval, defaultPollInterval).Should(Succeed()) + }) + It("denies an update when a metadata value becomes invalid", func() { updateTenantRules([]*rules.NamespaceRuleBodyTenant{ metadataRule( diff --git a/internal/webhook/rules/generic/validation/factory.go b/internal/webhook/rules/generic/validation/factory.go index d963e3e8..28050c49 100644 --- a/internal/webhook/rules/generic/validation/factory.go +++ b/internal/webhook/rules/generic/validation/factory.go @@ -45,9 +45,10 @@ func evaluateGenericRules[R any]( } type genericRuleValidator func( - genericObject, - schema.GroupVersionKind, - []*apirules.NamespaceRuleEnforceBody, + oldObj genericObject, + obj genericObject, + gvk schema.GroupVersionKind, + enforceBodies []*apirules.NamespaceRuleEnforceBody, ) (*ruleengine.Evaluation, error) type genericRules struct { @@ -94,7 +95,7 @@ func (h *genericRules) OnCreate( enforceBodies := ruleengine.EnforceBodiesFromNamespaceRules(bodies) - if err := h.validateGenericRules(ctx, req, obj, gvk, tnt, recorder, enforceBodies); err != nil { + if err := h.validateGenericRules(ctx, req, nil, obj, gvk, tnt, recorder, enforceBodies); err != nil { return ad.Deny(err.Error()) } @@ -105,7 +106,7 @@ func (h *genericRules) OnCreate( func (h *genericRules) OnUpdate( _ client.Client, _ client.Reader, - _ genericObject, + oldObj genericObject, obj genericObject, _ admission.Decoder, recorder events.EventRecorder, @@ -120,7 +121,7 @@ func (h *genericRules) OnUpdate( enforceBodies := ruleengine.EnforceBodiesFromNamespaceRules(bodies) - if err := h.validateGenericRules(ctx, req, obj, gvk, tnt, recorder, enforceBodies); err != nil { + if err := h.validateGenericRules(ctx, req, oldObj, obj, gvk, tnt, recorder, enforceBodies); err != nil { return ad.Deny(err.Error()) } @@ -145,6 +146,7 @@ func (h *genericRules) OnDelete( func (h *genericRules) validateGenericRules( ctx context.Context, req admission.Request, + oldObj genericObject, obj genericObject, gvk schema.GroupVersionKind, tnt *capsulev1beta2.Tenant, @@ -162,7 +164,7 @@ func (h *genericRules) validateGenericRules( } for _, evaluate := range h.rules { - evaluation, err := evaluate(obj, gvk, enforceBodies) + evaluation, err := evaluate(oldObj, obj, gvk, enforceBodies) if err != nil { return err } diff --git a/internal/webhook/rules/generic/validation/factory_test.go b/internal/webhook/rules/generic/validation/factory_test.go index 2c70bed1..d5121069 100644 --- a/internal/webhook/rules/generic/validation/factory_test.go +++ b/internal/webhook/rules/generic/validation/factory_test.go @@ -157,7 +157,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { t.Fatalf("validator must not be called") return nil, nil @@ -171,6 +171,7 @@ func TestValidateGenericRules(t *testing.T) { context.Background(), admission.Request{}, nil, + nil, baseGVK, testTenant(), testEventRecorder{}, @@ -188,7 +189,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(obj genericObject, got schema.GroupVersionKind, _ []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(_ genericObject, obj genericObject, got schema.GroupVersionKind, _ []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { if got != baseGVK { t.Fatalf("expected gvk %s, got %s", baseGVK.String(), got.String()) } @@ -211,6 +212,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, obj, baseGVK, testTenant(), @@ -244,6 +246,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ func( + genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody, @@ -263,6 +266,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, obj, coreGVK("ConfigMap"), &capsulev1beta2.Tenant{ @@ -293,7 +297,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { called = true return nil, nil @@ -306,6 +310,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, genericMetadataObject(map[string]string{ "managed-by": "human", }, nil), @@ -329,12 +334,12 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { calls = append(calls, "first") return nil, nil }, - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { calls = append(calls, "second") return &ruleengine.Evaluation{}, nil @@ -347,6 +352,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, genericMetadataObject(nil, nil), baseGVK, testTenant(), @@ -376,7 +382,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(_ genericObject, _ schema.GroupVersionKind, got []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(_ genericObject, _ genericObject, _ schema.GroupVersionKind, got []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { if len(got) != len(enforceBodies) { t.Fatalf("expected %d enforce bodies, got %d", len(enforceBodies), len(got)) } @@ -391,6 +397,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, genericMetadataObject(nil, nil), baseGVK, testTenant(), @@ -410,10 +417,10 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { return nil, expected }, - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { secondCalled = true return nil, nil @@ -426,6 +433,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, genericMetadataObject(nil, nil), baseGVK, testTenant(), @@ -447,7 +455,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { calls++ return &ruleengine.Evaluation{ @@ -465,7 +473,7 @@ func TestValidateGenericRules(t *testing.T) { }, }, nil }, - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { calls++ return nil, nil @@ -478,6 +486,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, genericMetadataObject(nil, nil), baseGVK, testTenant(), @@ -499,7 +508,7 @@ func TestValidateGenericRules(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { return &ruleengine.Evaluation{ Blocking: &ruleengine.Decision{ SetName: "metadata label", @@ -513,7 +522,7 @@ func TestValidateGenericRules(t *testing.T) { }, }, nil }, - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { secondCalled = true return nil, nil @@ -526,6 +535,7 @@ func TestValidateGenericRules(t *testing.T) { err := h.validateGenericRules( context.Background(), admission.Request{}, + nil, genericMetadataObject(nil, nil), baseGVK, testTenant(), @@ -561,7 +571,7 @@ func TestGenericRulesOnCreate(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { called = true return nil, nil @@ -638,7 +648,7 @@ func TestGenericRulesOnCreate(t *testing.T) { h := &genericRules{ rules: []genericRuleValidator{ - func(genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + func(genericObject, genericObject, schema.GroupVersionKind, []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { return nil, errors.New("validator failed") }, }, @@ -672,18 +682,18 @@ func TestGenericRulesOnCreate(t *testing.T) { func TestGenericRulesOnUpdate(t *testing.T) { t.Parallel() - t.Run("uses new object and allows when validators return nil", func(t *testing.T) { + t.Run("uses old and new objects and allows when validators return nil", func(t *testing.T) { t.Parallel() - calledWithNewObject := false + calledWithObjects := false oldObj := genericMetadataObject(map[string]string{"old": "true"}, nil) newObj := genericMetadataObject(map[string]string{"new": "true"}, nil) h := &genericRules{ rules: []genericRuleValidator{ - func(obj genericObject, _ schema.GroupVersionKind, _ []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { - calledWithNewObject = obj.GetLabels()["new"] == "true" + func(old, obj genericObject, _ schema.GroupVersionKind, _ []*apirules.NamespaceRuleEnforceBody) (*ruleengine.Evaluation, error) { + calledWithObjects = old.GetLabels()["old"] == "true" && obj.GetLabels()["new"] == "true" return nil, nil }, @@ -707,8 +717,8 @@ func TestGenericRulesOnUpdate(t *testing.T) { if resp != nil { t.Fatalf("expected nil response, got %#v", resp) } - if !calledWithNewObject { - t.Fatalf("expected validator to receive the new object") + if !calledWithObjects { + t.Fatalf("expected validator to receive the old and new objects") } }) @@ -1041,6 +1051,7 @@ func TestGenericRulesWithRealMetadataValidatorSmoke(t *testing.T) { h := GenericRules(cache.NewRegexCache()).(*genericRules) evaluation, err := h.validateMetadata( + nil, genericMetadataObject(map[string]string{"env": "prod"}, nil), schema.GroupVersionKind{ Group: "", diff --git a/internal/webhook/rules/generic/validation/metadata.go b/internal/webhook/rules/generic/validation/metadata.go index 97bed94f..bd5eb6d8 100644 --- a/internal/webhook/rules/generic/validation/metadata.go +++ b/internal/webhook/rules/generic/validation/metadata.go @@ -33,6 +33,7 @@ type metadataEntry struct { } func (h *genericRules) validateMetadata( + oldObj genericObject, obj genericObject, gvk schema.GroupVersionKind, enforceBodies []*apirules.NamespaceRuleEnforceBody, @@ -41,7 +42,7 @@ func (h *genericRules) validateMetadata( return nil, nil } - entries, err := h.controlledMetadataEntries(obj, gvk, enforceBodies) + entries, err := h.controlledMetadataEntries(oldObj, obj, gvk, enforceBodies) if err != nil { return nil, err } @@ -178,6 +179,7 @@ func (h *genericRules) metadataSet( //nolint:gocognit func (h *genericRules) controlledMetadataEntries( + oldObj genericObject, obj genericObject, gvk schema.GroupVersionKind, enforceBodies []*apirules.NamespaceRuleEnforceBody, @@ -185,6 +187,15 @@ func (h *genericRules) controlledMetadataEntries( labels := obj.GetLabels() annotations := obj.GetAnnotations() + // Creation has no old object, so all present metadata is evaluated. During + // updates, unchanged entries still satisfy Required but skip value matching. + var oldLabels, oldAnnotations map[string]string + + if oldObj != nil { + oldLabels = oldObj.GetLabels() + oldAnnotations = oldObj.GetAnnotations() + } + seen := make(map[string]metadataEntry) for _, enforce := range enforceBodies { @@ -224,7 +235,14 @@ func (h *genericRules) controlledMetadataEntries( matchedAny = true - h.addMetadataEntry(seen, metadataFieldLabel, key, value, true, required) + h.addMetadataEntryIfChanged( + seen, + oldLabels, + metadataFieldLabel, + key, + value, + required, + ) } if !matchedAny && required { @@ -256,7 +274,14 @@ func (h *genericRules) controlledMetadataEntries( matchedAny = true - h.addMetadataEntry(seen, metadataFieldAnnotation, key, value, true, required) + h.addMetadataEntryIfChanged( + seen, + oldAnnotations, + metadataFieldAnnotation, + key, + value, + required, + ) } if !matchedAny && required { @@ -282,6 +307,31 @@ func (h *genericRules) controlledMetadataEntries( return out, nil } +func metadataValueUnchanged( + oldMetadata map[string]string, + key string, + value string, +) bool { + oldValue, ok := oldMetadata[key] + + return ok && oldValue == value +} + +func (h *genericRules) addMetadataEntryIfChanged( + seen map[string]metadataEntry, + oldMetadata map[string]string, + field metadataField, + key string, + value string, + required bool, +) { + if metadataValueUnchanged(oldMetadata, key, value) { + return + } + + h.addMetadataEntry(seen, field, key, value, true, required) +} + func (h *genericRules) matchesMetadataKey(selector, key string) (bool, error) { return h.regexCache.MatchRegex(apirules.MetadataKeyExpression(selector), key) } diff --git a/internal/webhook/rules/generic/validation/metadata_test.go b/internal/webhook/rules/generic/validation/metadata_test.go index 6dd76227..d3e9526d 100644 --- a/internal/webhook/rules/generic/validation/metadata_test.go +++ b/internal/webhook/rules/generic/validation/metadata_test.go @@ -356,7 +356,7 @@ func TestValidateMetadata(t *testing.T) { h := newMetadataTestRules(nil, nil) - got, err := h.validateMetadata(tt.obj, tt.gvk, tt.enforceBodies) + got, err := h.validateMetadata(nil, tt.obj, tt.gvk, tt.enforceBodies) if tt.wantMessage == "error" { if err == nil { t.Fatalf("expected error") @@ -419,6 +419,137 @@ func TestValidateMetadata(t *testing.T) { } } +func TestValidateMetadataUpdate(t *testing.T) { + t.Parallel() + + denyAll := enforceMetadata( + apirules.ActionTypeDeny, + []string{"*"}, + []string{"ConfigMap"}, + map[string]apirules.MetadataValueRule{ + ".*": metadataPolicy(false, expression(".*")), + }, + map[string]apirules.MetadataValueRule{ + ".*": metadataPolicy(false, expression(".*")), + }, + ) + allowB := enforceMetadata( + apirules.ActionTypeAllow, + []string{"*"}, + []string{"ConfigMap"}, + map[string]apirules.MetadataValueRule{ + "label-b": metadataPolicy(false, expression(".*")), + }, + map[string]apirules.MetadataValueRule{ + "annotation-b": metadataPolicy(false, expression(".*")), + }, + ) + requireEnvironment := enforceMetadata( + apirules.ActionTypeAllow, + []string{"*"}, + []string{"ConfigMap"}, + map[string]apirules.MetadataValueRule{ + "env": metadataPolicy(true, exact("prod")), + }, + nil, + ) + + tests := []struct { + name string + oldObj genericObject + obj genericObject + enforceBodies []*apirules.NamespaceRuleEnforceBody + wantNil bool + wantBlocking bool + wantPath string + }{ + { + name: "allows changed metadata authorized for the caller while ignoring unchanged metadata", + oldObj: metadataObject( + map[string]string{"label-a": "old"}, + map[string]string{"annotation-a": "old"}, + ), + obj: metadataObject( + map[string]string{"label-a": "old", "label-b": "new"}, + map[string]string{"annotation-a": "old", "annotation-b": "new"}, + ), + enforceBodies: []*apirules.NamespaceRuleEnforceBody{denyAll, allowB}, + }, + { + name: "denies a changed label not authorized for the caller", + oldObj: metadataObject(map[string]string{"label-a": "old"}, nil), + obj: metadataObject(map[string]string{"label-a": "new"}, nil), + enforceBodies: []*apirules.NamespaceRuleEnforceBody{denyAll, allowB}, + wantBlocking: true, + wantPath: `metadata.labels["label-a"]`, + }, + { + name: "ignores an unchanged required value", + oldObj: metadataObject(map[string]string{"env": "legacy"}, nil), + obj: metadataObject(map[string]string{"env": "legacy", "other": "new"}, nil), + enforceBodies: []*apirules.NamespaceRuleEnforceBody{requireEnvironment}, + wantNil: true, + }, + { + name: "denies a changed required value that is invalid", + oldObj: metadataObject(map[string]string{"env": "prod"}, nil), + obj: metadataObject(map[string]string{"env": "stage"}, nil), + enforceBodies: []*apirules.NamespaceRuleEnforceBody{requireEnvironment}, + wantBlocking: true, + wantPath: `metadata.labels["env"]`, + }, + { + name: "denies removal of required metadata", + oldObj: metadataObject(map[string]string{"env": "prod"}, nil), + obj: metadataObject(nil, nil), + enforceBodies: []*apirules.NamespaceRuleEnforceBody{requireEnvironment}, + wantBlocking: true, + wantPath: `metadata.labels["env"]`, + }, + { + name: "ignores removal of optional denied metadata", + oldObj: metadataObject(map[string]string{"label-a": "old"}, nil), + obj: metadataObject(nil, nil), + enforceBodies: []*apirules.NamespaceRuleEnforceBody{denyAll}, + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + h := newMetadataTestRules(nil, nil) + + got, err := h.validateMetadata(tt.oldObj, tt.obj, coreGVK("ConfigMap"), tt.enforceBodies) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if tt.wantNil { + if got != nil { + t.Fatalf("expected nil evaluation, got %#v", got) + } + + return + } + if got == nil { + t.Fatal("expected evaluation") + } + + blockingErr := got.BlockingError() + if tt.wantBlocking && blockingErr == nil { + t.Fatal("expected blocking error") + } + if !tt.wantBlocking && blockingErr != nil { + t.Fatalf("expected no blocking error, got %v", blockingErr) + } + if tt.wantPath != "" && got.Blocking.Value.Path != tt.wantPath { + t.Fatalf("expected path %q, got %q", tt.wantPath, got.Blocking.Value.Path) + } + }) + } +} + func TestControlledMetadataEntriesMatchesKeyPatternsWithRegexCache(t *testing.T) { t.Parallel() @@ -439,6 +570,7 @@ func TestControlledMetadataEntriesMatchesKeyPatternsWithRegexCache(t *testing.T) }} entries, err := h.controlledMetadataEntries( + nil, obj, schema.GroupVersionKind{Version: "v1", Kind: "Namespace"}, enforce, @@ -966,7 +1098,7 @@ func TestControlledMetadataEntries(t *testing.T) { h := newMetadataTestRules(tt.managedLabels, tt.managedAnnotations) - got, err := h.controlledMetadataEntries(tt.obj, tt.gvk, tt.enforceBodies) + got, err := h.controlledMetadataEntries(nil, tt.obj, tt.gvk, tt.enforceBodies) if err != nil { t.Fatalf("controlledMetadataEntries() error = %v", err) }