Skip to content

Commit e50bacf

Browse files
oliverbaehleroliverbaehler
authored
fix: validate only changed metadata on updates (#2118)
Signed-off-by: Oliver Baehler <oliver@sudo-i.net>
1 parent e531879 commit e50bacf

5 files changed

Lines changed: 514 additions & 33 deletions

File tree

‎e2e/rules_enforce_metadata_test.go‎

Lines changed: 287 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package e2e
55

66
import (
77
"context"
8+
"encoding/json"
89
"fmt"
910
"strings"
1011
"time"
@@ -29,7 +30,14 @@ import (
2930
)
3031

3132
var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("tenant", "rules", "enforce", "metadata", "generic"), func() {
32-
const ownerName = "e2e-rules-metadata"
33+
const (
34+
ownerName = "e2e-rules-metadata"
35+
secondOwnerName = "e2e-rules-metadata-second"
36+
ownerLabel = "example.corp/label-a"
37+
secondOwnerLabel = "example.corp/label-b"
38+
ownerAnnotation = "example.corp/annotation-a"
39+
secondOwnerAnnotation = "example.corp/annotation-b"
40+
)
3341

3442
var (
3543
tnt *capsulev1beta2.Tenant
@@ -112,6 +120,65 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t
112120
return rule
113121
}
114122

123+
audienceRule := func(
124+
audience rules.Audience,
125+
rule *rules.NamespaceRuleBodyTenant,
126+
) *rules.NamespaceRuleBodyTenant {
127+
rule.Audience = []rules.Audience{audience}
128+
129+
return rule
130+
}
131+
132+
isolatedMetadataRules := func() []*rules.NamespaceRuleBodyTenant {
133+
return []*rules.NamespaceRuleBodyTenant{
134+
audienceRule(
135+
rules.Audience{
136+
Kind: rules.AudienceKindCustom,
137+
Name: string(rules.CustomAudienceCapsuleUser),
138+
},
139+
metadataRule(
140+
rules.ActionTypeDeny,
141+
"v1",
142+
[]string{"ConfigMap"},
143+
map[string]rules.MetadataValueRule{
144+
".*": metadataValueRule(false, metadataByExpression(".*")),
145+
},
146+
map[string]rules.MetadataValueRule{
147+
".*": metadataValueRule(false, metadataByExpression(".*")),
148+
},
149+
),
150+
),
151+
audienceRule(
152+
rules.Audience{Kind: rules.AudienceKindUser, Name: ownerName},
153+
metadataRule(
154+
rules.ActionTypeAllow,
155+
"v1",
156+
[]string{"ConfigMap"},
157+
map[string]rules.MetadataValueRule{
158+
ownerLabel: metadataValueRule(false, metadataByExact("owner-a")),
159+
},
160+
map[string]rules.MetadataValueRule{
161+
ownerAnnotation: metadataValueRule(false, metadataByExact("owner-a")),
162+
},
163+
),
164+
),
165+
audienceRule(
166+
rules.Audience{Kind: rules.AudienceKindUser, Name: secondOwnerName},
167+
metadataRule(
168+
rules.ActionTypeAllow,
169+
"v1",
170+
[]string{"ConfigMap"},
171+
map[string]rules.MetadataValueRule{
172+
secondOwnerLabel: metadataValueRule(false, metadataByExact("owner-b")),
173+
},
174+
map[string]rules.MetadataValueRule{
175+
secondOwnerAnnotation: metadataValueRule(false, metadataByExact("owner-b")),
176+
},
177+
),
178+
),
179+
}
180+
}
181+
115182
baseTenantRules := func() []*rules.NamespaceRuleBodyTenant {
116183
return []*rules.NamespaceRuleBodyTenant{
117184
metadataRule(
@@ -190,6 +257,14 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t
190257
},
191258
},
192259
},
260+
{
261+
CoreOwnerSpec: rbac.CoreOwnerSpec{
262+
UserSpec: rbac.UserSpec{
263+
Name: secondOwnerName,
264+
Kind: "User",
265+
},
266+
},
267+
},
193268
},
194269
Rules: tenantRules,
195270
},
@@ -574,6 +649,62 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t
574649
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
575650
}
576651

652+
patchConfigMapMetadata := func(
653+
cs kubernetes.Interface,
654+
nsName string,
655+
cmName string,
656+
labels map[string]string,
657+
annotations map[string]string,
658+
) error {
659+
metadataPatch := map[string]any{}
660+
if labels != nil {
661+
metadataPatch["labels"] = labels
662+
}
663+
if annotations != nil {
664+
metadataPatch["annotations"] = annotations
665+
}
666+
667+
patch, err := json.Marshal(map[string]any{"metadata": metadataPatch})
668+
if err != nil {
669+
return err
670+
}
671+
672+
_, err = cs.CoreV1().ConfigMaps(nsName).Patch(
673+
context.Background(),
674+
cmName,
675+
k8stypes.MergePatchType,
676+
patch,
677+
metav1.PatchOptions{},
678+
)
679+
680+
return err
681+
}
682+
683+
patchConfigMapMetadataAndExpectDenied := func(
684+
cs kubernetes.Interface,
685+
nsName string,
686+
cmName string,
687+
labels map[string]string,
688+
annotations map[string]string,
689+
substrings ...string,
690+
) {
691+
Eventually(func() error {
692+
err := patchConfigMapMetadata(cs, nsName, cmName, labels, annotations)
693+
if err == nil {
694+
return fmt.Errorf("expected configmap metadata patch to be denied, but it succeeded")
695+
}
696+
697+
message := err.Error()
698+
for _, substring := range substrings {
699+
if !strings.Contains(message, substring) {
700+
return fmt.Errorf("expected error to contain %q, got: %s", substring, message)
701+
}
702+
}
703+
704+
return nil
705+
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
706+
}
707+
577708
createServiceAndExpectAllowed := func(cs kubernetes.Interface, nsName string, svc *corev1.Service) {
578709
EventuallyCreation(func() error {
579710
_, 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
719850
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
720851
}
721852

853+
prepareIsolatedMetadataTest := func(name string) (*corev1.Namespace, kubernetes.Interface, kubernetes.Interface, *corev1.ConfigMap) {
854+
updateTenantRules(isolatedMetadataRules())
855+
856+
ns := createNamespace(nil)
857+
waitForProjectedMetadata(ns.Name, secondOwnerLabel, nil, nil)
858+
859+
owner := ownerClient(tnt.Spec.Owners[0].UserSpec)
860+
secondOwner := ownerClient(tnt.Spec.Owners[1].UserSpec)
861+
cm := configMap(
862+
name,
863+
map[string]string{ownerLabel: "owner-a"},
864+
map[string]string{ownerAnnotation: "owner-a"},
865+
)
866+
867+
createConfigMapAndExpectAllowed(owner, ns.Name, cm)
868+
869+
return ns, owner, secondOwner, cm
870+
}
871+
722872
BeforeEach(func() {
723873
tenantRules = baseTenantRules()
724874
})
@@ -1358,6 +1508,142 @@ var _ = Describe("enforcing generic metadata namespace rules", Ordered, Label("t
13581508
)
13591509
})
13601510

1511+
It("allows an audience to patch its metadata while another audience's metadata is unchanged", func() {
1512+
ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-isolated-allowed")
1513+
1514+
By("patching only the label and annotation assigned to the second owner")
1515+
Eventually(func() error {
1516+
return patchConfigMapMetadata(
1517+
secondOwner,
1518+
ns.Name,
1519+
cm.Name,
1520+
map[string]string{secondOwnerLabel: "owner-b"},
1521+
map[string]string{secondOwnerAnnotation: "owner-b"},
1522+
)
1523+
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
1524+
1525+
By("preserving both audiences' metadata")
1526+
Eventually(func(g Gomega) {
1527+
current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get(
1528+
context.Background(),
1529+
cm.Name,
1530+
metav1.GetOptions{},
1531+
)
1532+
g.Expect(err).NotTo(HaveOccurred())
1533+
g.Expect(current.Labels).To(HaveKeyWithValue(ownerLabel, "owner-a"))
1534+
g.Expect(current.Labels).To(HaveKeyWithValue(secondOwnerLabel, "owner-b"))
1535+
g.Expect(current.Annotations).To(HaveKeyWithValue(ownerAnnotation, "owner-a"))
1536+
g.Expect(current.Annotations).To(HaveKeyWithValue(secondOwnerAnnotation, "owner-b"))
1537+
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
1538+
})
1539+
1540+
It("denies patches to metadata assigned to another audience", func() {
1541+
ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-other-audience-denied")
1542+
1543+
By("denying a change to the first owner's label")
1544+
patchConfigMapMetadataAndExpectDenied(
1545+
secondOwner,
1546+
ns.Name,
1547+
cm.Name,
1548+
map[string]string{ownerLabel: "tampered"},
1549+
nil,
1550+
ownerLabel,
1551+
"tampered",
1552+
"denied",
1553+
)
1554+
1555+
By("denying a change to the first owner's annotation")
1556+
patchConfigMapMetadataAndExpectDenied(
1557+
secondOwner,
1558+
ns.Name,
1559+
cm.Name,
1560+
nil,
1561+
map[string]string{ownerAnnotation: "tampered"},
1562+
ownerAnnotation,
1563+
"tampered",
1564+
"denied",
1565+
)
1566+
1567+
By("preserving the first owner's metadata")
1568+
Eventually(func(g Gomega) {
1569+
current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get(
1570+
context.Background(),
1571+
cm.Name,
1572+
metav1.GetOptions{},
1573+
)
1574+
g.Expect(err).NotTo(HaveOccurred())
1575+
g.Expect(current.Labels).To(HaveKeyWithValue(ownerLabel, "owner-a"))
1576+
g.Expect(current.Annotations).To(HaveKeyWithValue(ownerAnnotation, "owner-a"))
1577+
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
1578+
})
1579+
1580+
It("denies disallowed values for metadata assigned to the updating audience", func() {
1581+
ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-own-value-denied")
1582+
1583+
By("denying a disallowed value for the second owner's label")
1584+
patchConfigMapMetadataAndExpectDenied(
1585+
secondOwner,
1586+
ns.Name,
1587+
cm.Name,
1588+
map[string]string{secondOwnerLabel: "blocked"},
1589+
nil,
1590+
secondOwnerLabel,
1591+
"blocked",
1592+
"denied",
1593+
)
1594+
1595+
By("denying a disallowed value for the second owner's annotation")
1596+
patchConfigMapMetadataAndExpectDenied(
1597+
secondOwner,
1598+
ns.Name,
1599+
cm.Name,
1600+
nil,
1601+
map[string]string{secondOwnerAnnotation: "blocked"},
1602+
secondOwnerAnnotation,
1603+
"blocked",
1604+
"denied",
1605+
)
1606+
})
1607+
1608+
It("allows non-metadata updates when another audience's metadata is unchanged", func() {
1609+
ns, _, secondOwner, cm := prepareIsolatedMetadataTest("metadata-update-data-allowed")
1610+
1611+
By("updating ConfigMap data as the second owner")
1612+
Eventually(func() error {
1613+
current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get(
1614+
context.Background(),
1615+
cm.Name,
1616+
metav1.GetOptions{},
1617+
)
1618+
if err != nil {
1619+
return err
1620+
}
1621+
1622+
current.Data["key"] = "updated"
1623+
1624+
_, err = secondOwner.CoreV1().ConfigMaps(ns.Name).Update(
1625+
context.Background(),
1626+
current,
1627+
metav1.UpdateOptions{},
1628+
)
1629+
1630+
return err
1631+
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
1632+
1633+
By("preserving the metadata and the data update")
1634+
Eventually(func(g Gomega) {
1635+
current, err := secondOwner.CoreV1().ConfigMaps(ns.Name).Get(
1636+
context.Background(),
1637+
cm.Name,
1638+
metav1.GetOptions{},
1639+
)
1640+
g.Expect(err).NotTo(HaveOccurred())
1641+
g.Expect(current.Data).To(HaveKeyWithValue("key", "updated"))
1642+
g.Expect(current.Labels).To(HaveKeyWithValue(ownerLabel, "owner-a"))
1643+
g.Expect(current.Annotations).To(HaveKeyWithValue(ownerAnnotation, "owner-a"))
1644+
}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
1645+
})
1646+
13611647
It("denies an update when a metadata value becomes invalid", func() {
13621648
updateTenantRules([]*rules.NamespaceRuleBodyTenant{
13631649
metadataRule(

‎internal/webhook/rules/generic/validation/factory.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ func evaluateGenericRules[R any](
4545
}
4646

4747
type genericRuleValidator func(
48-
genericObject,
49-
schema.GroupVersionKind,
50-
[]*apirules.NamespaceRuleEnforceBody,
48+
oldObj genericObject,
49+
obj genericObject,
50+
gvk schema.GroupVersionKind,
51+
enforceBodies []*apirules.NamespaceRuleEnforceBody,
5152
) (*ruleengine.Evaluation, error)
5253

5354
type genericRules struct {
@@ -94,7 +95,7 @@ func (h *genericRules) OnCreate(
9495

9596
enforceBodies := ruleengine.EnforceBodiesFromNamespaceRules(bodies)
9697

97-
if err := h.validateGenericRules(ctx, req, obj, gvk, tnt, recorder, enforceBodies); err != nil {
98+
if err := h.validateGenericRules(ctx, req, nil, obj, gvk, tnt, recorder, enforceBodies); err != nil {
9899
return ad.Deny(err.Error())
99100
}
100101

@@ -105,7 +106,7 @@ func (h *genericRules) OnCreate(
105106
func (h *genericRules) OnUpdate(
106107
_ client.Client,
107108
_ client.Reader,
108-
_ genericObject,
109+
oldObj genericObject,
109110
obj genericObject,
110111
_ admission.Decoder,
111112
recorder events.EventRecorder,
@@ -120,7 +121,7 @@ func (h *genericRules) OnUpdate(
120121

121122
enforceBodies := ruleengine.EnforceBodiesFromNamespaceRules(bodies)
122123

123-
if err := h.validateGenericRules(ctx, req, obj, gvk, tnt, recorder, enforceBodies); err != nil {
124+
if err := h.validateGenericRules(ctx, req, oldObj, obj, gvk, tnt, recorder, enforceBodies); err != nil {
124125
return ad.Deny(err.Error())
125126
}
126127

@@ -145,6 +146,7 @@ func (h *genericRules) OnDelete(
145146
func (h *genericRules) validateGenericRules(
146147
ctx context.Context,
147148
req admission.Request,
149+
oldObj genericObject,
148150
obj genericObject,
149151
gvk schema.GroupVersionKind,
150152
tnt *capsulev1beta2.Tenant,
@@ -162,7 +164,7 @@ func (h *genericRules) validateGenericRules(
162164
}
163165

164166
for _, evaluate := range h.rules {
165-
evaluation, err := evaluate(obj, gvk, enforceBodies)
167+
evaluation, err := evaluate(oldObj, obj, gvk, enforceBodies)
166168
if err != nil {
167169
return err
168170
}

0 commit comments

Comments
 (0)