Skip to content

Commit 1f0174b

Browse files
akutzakutz
authored
Merge pull request vmware-tanzu#1399 from DimpleRajaVamsi/dimplerajavamsi/protected-annotations-fix
🌱 Removing the protected annotations checks from validating webhook
2 parents c759931 + ad27f31 commit 1f0174b

File tree

4 files changed

+8
-284
lines changed

4 files changed

+8
-284
lines changed

‎pkg/builder/validating_webhook.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,6 @@ func (h *validatingWebhookHandler) Handle(ctx context.Context, req admission.Req
175175
func (h *validatingWebhookHandler) HandleValidate(req admission.Request, ctx *pkgctx.WebhookRequestContext) admission.Response {
176176

177177
if _, ok := ctx.Obj.GetAnnotations()[pkgconst.SkipValidationAnnotationKey]; ok {
178-
if !ctx.IsPrivilegedAccount {
179-
// Only privileged users may set the skip validation annotation.
180-
return webhook.Denied(SkipValidationDenied)
181-
}
182-
183178
// The object has the skip validation annotation, so allow the object to
184179
// bypass validation.
185180
return webhook.Allowed(SkipValidationAllowed)

‎pkg/builder/validating_webhook_test.go‎

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -223,18 +223,10 @@ var _ = Describe("NewValidatingWebhook", func() {
223223
pkgconst.SkipValidationAnnotationKey: "",
224224
})
225225
})
226-
When("privileged user", func() {
227-
It("should return true", func() {
228-
req := getAdmissionRequest(obj, admissionv1.Create)
229-
req.UserInfo.Groups = []string{"system:masters"}
230-
assertOkay(req, pkgbuilder.SkipValidationAllowed)
231-
})
232-
})
233-
When("unprivileged user", func() {
234-
It("should return false", func() {
235-
req := getAdmissionRequest(obj, admissionv1.Create)
236-
assertNotOkay(req, 403, pkgbuilder.SkipValidationDenied)
237-
})
226+
227+
It("should return true", func() {
228+
req := getAdmissionRequest(obj, admissionv1.Create)
229+
assertOkay(req, pkgbuilder.SkipValidationAllowed)
238230
})
239231
})
240232
})
@@ -282,18 +274,10 @@ var _ = Describe("NewValidatingWebhook", func() {
282274
pkgconst.SkipValidationAnnotationKey: "",
283275
})
284276
})
285-
When("privileged user", func() {
286-
It("should return true", func() {
287-
req := getAdmissionRequest(obj, admissionv1.Update)
288-
req.UserInfo.Groups = []string{"system:masters"}
289-
assertOkay(req, pkgbuilder.SkipValidationAllowed)
290-
})
291-
})
292-
When("unprivileged user", func() {
293-
It("should return false", func() {
294-
req := getAdmissionRequest(obj, admissionv1.Update)
295-
assertNotOkay(req, 403, pkgbuilder.SkipValidationDenied)
296-
})
277+
278+
It("should return true", func() {
279+
req := getAdmissionRequest(obj, admissionv1.Update)
280+
assertOkay(req, pkgbuilder.SkipValidationAllowed)
297281
})
298282
})
299283
})

‎webhooks/virtualmachine/validation/virtualmachine_validator.go‎

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -2130,52 +2130,6 @@ func (v validator) validateAvailabilityZone(
21302130
return allErrs
21312131
}
21322132

2133-
// protectedAnnotationRegex matches annotations with keys matching the pattern:
2134-
// ^.+\.protected(/.+)?$
2135-
//
2136-
// Examples that match:
2137-
// - fu.bar.protected
2138-
// - hello.world.protected/sub-key
2139-
// - vmoperator.vmware.com.protected/reconcile-priority
2140-
//
2141-
// Examples that do NOT match:
2142-
// - protected.fu.bar
2143-
// - hello.world.protected.against/sub-key
2144-
var protectedAnnotationRegex = regexp.MustCompile(`^.+\.protected(/.*)?$`)
2145-
2146-
// validateProtectedAnnotations validates that annotations matching the
2147-
// protected annotation pattern can only be modified by privileged users.
2148-
func (v validator) validateProtectedAnnotations(vm, oldVM *vmopv1.VirtualMachine) field.ErrorList {
2149-
var allErrs field.ErrorList
2150-
annotationPath := field.NewPath("metadata", "annotations")
2151-
2152-
// Collect all protected annotation keys from both old and new VMs
2153-
protectedKeys := make(map[string]struct{})
2154-
2155-
for k := range vm.Annotations {
2156-
if protectedAnnotationRegex.MatchString(k) {
2157-
protectedKeys[k] = struct{}{}
2158-
}
2159-
}
2160-
2161-
for k := range oldVM.Annotations {
2162-
if protectedAnnotationRegex.MatchString(k) {
2163-
protectedKeys[k] = struct{}{}
2164-
}
2165-
}
2166-
2167-
// Check if any protected annotations have been modified
2168-
for k := range protectedKeys {
2169-
if vm.Annotations[k] != oldVM.Annotations[k] {
2170-
allErrs = append(allErrs, field.Forbidden(
2171-
annotationPath.Key(k),
2172-
modifyAnnotationNotAllowedForNonAdmin))
2173-
}
2174-
}
2175-
2176-
return allErrs
2177-
}
2178-
21792133
func (v validator) validateAnnotation(ctx *pkgctx.WebhookRequestContext, vm, oldVM *vmopv1.VirtualMachine) field.ErrorList {
21802134
var allErrs field.ErrorList
21812135

@@ -2200,8 +2154,6 @@ func (v validator) validateAnnotation(ctx *pkgctx.WebhookRequestContext, vm, old
22002154
oldVM = &vmopv1.VirtualMachine{}
22012155
}
22022156

2203-
allErrs = append(allErrs, v.validateProtectedAnnotations(vm, oldVM)...)
2204-
22052157
if vm.Annotations[vmopv1.InstanceIDAnnotation] != oldVM.Annotations[vmopv1.InstanceIDAnnotation] {
22062158
allErrs = append(allErrs, field.Forbidden(annotationPath.Key(vmopv1.InstanceIDAnnotation), modifyAnnotationNotAllowedForNonAdmin))
22072159
}

‎webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go‎

Lines changed: 0 additions & 207 deletions
Original file line numberDiff line numberDiff line change
@@ -84,52 +84,6 @@ type testParams struct {
8484
skipSetControllerForPVC bool
8585
}
8686

87-
type protectedAnnotationTestCase struct {
88-
annotationKey string
89-
oldValue string
90-
newValue string
91-
}
92-
93-
// When defining any new protected annotations, be sure to add them to this
94-
// list so they are covered via test cases. A protected condition is one whose
95-
// key matches the regex `^.+\.protected(/.+)?$`.
96-
//
97-
// Examples that match:
98-
// - fu.bar.protected
99-
// - hello.world.protected/sub-key
100-
// - vmoperator.vmware.com.protected/reconcile-priority
101-
//
102-
// Examples that do NOT match:
103-
// - protected.fu.bar
104-
// - hello.world.protected.against/sub-key
105-
var protectedAnnotationTestCases = []protectedAnnotationTestCase{
106-
{
107-
annotationKey: pkgconst.ReconcilePriorityAnnotationKey,
108-
oldValue: "100",
109-
newValue: "200",
110-
},
111-
{
112-
annotationKey: pkgconst.SkipDeletePlatformResourceKey,
113-
oldValue: "true",
114-
newValue: "false",
115-
},
116-
{
117-
annotationKey: pkgconst.ApplyPowerStateTimeAnnotation,
118-
oldValue: time.Now().Format(time.RFC3339Nano),
119-
newValue: time.Now().Add(time.Hour).Format(time.RFC3339Nano),
120-
},
121-
{
122-
annotationKey: "hello.world.protected/condition-status",
123-
oldValue: "red",
124-
newValue: "green",
125-
},
126-
{
127-
annotationKey: "condition.vmware.vmoperator.com.protected/hello-world",
128-
oldValue: "True",
129-
newValue: "False",
130-
},
131-
}
132-
13387
func bypassUpgradeCheck(ctx *context.Context, objects ...metav1.Object) {
13488
pkgcfg.SetContext(*ctx, func(config *pkgcfg.Config) {
13589
config.BuildVersion = fake
@@ -1474,56 +1428,6 @@ func unitTestsValidateCreate() {
14741428
),
14751429
)
14761430

1477-
getProtectedAnnotationTableAllowCreate := func() []any {
1478-
table := []any{
1479-
func(tc protectedAnnotationTestCase) {
1480-
doTest(testParams{
1481-
setup: func(ctx *unitValidatingWebhookContext) {
1482-
ctx.IsPrivilegedAccount = true
1483-
ctx.vm.Annotations[tc.annotationKey] = tc.newValue
1484-
},
1485-
expectAllowed: true,
1486-
})
1487-
},
1488-
}
1489-
1490-
for i := range protectedAnnotationTestCases {
1491-
tc := protectedAnnotationTestCases[i]
1492-
table = append(table, Entry("should allow create with "+tc.annotationKey, tc))
1493-
}
1494-
1495-
return table
1496-
}
1497-
1498-
getProtectedAnnotationTableDisallowCreate := func() []any {
1499-
table := []any{
1500-
func(tc protectedAnnotationTestCase) {
1501-
doTest(testParams{
1502-
setup: func(ctx *unitValidatingWebhookContext) {
1503-
ctx.vm.Annotations[tc.annotationKey] = tc.newValue
1504-
},
1505-
validate: doValidateWithMsg(
1506-
field.Forbidden(annotationPath.Key(tc.annotationKey), "modifying this annotation is not allowed for non-admin users").Error(),
1507-
),
1508-
})
1509-
},
1510-
}
1511-
1512-
for i := range protectedAnnotationTestCases {
1513-
tc := protectedAnnotationTestCases[i]
1514-
table = append(table, Entry("should disallow create with "+tc.annotationKey, tc))
1515-
}
1516-
1517-
return table
1518-
}
1519-
1520-
DescribeTable("disallow create with protected annotations by non-privileged user",
1521-
getProtectedAnnotationTableDisallowCreate()...,
1522-
)
1523-
1524-
DescribeTable("allow create with protected annotations by non-privileged user",
1525-
getProtectedAnnotationTableAllowCreate()...,
1526-
)
15271431
})
15281432

15291433
Context("Label", func() {
@@ -4752,117 +4656,6 @@ func unitTestsValidateUpdate() { //nolint:gocyclo
47524656
),
47534657
)
47544658

4755-
getProtectedAnnotationTableAllowUpdate := func() []any {
4756-
table := []any{
4757-
func(tc protectedAnnotationTestCase) {
4758-
doTest(testParams{
4759-
setup: func(ctx *unitValidatingWebhookContext) {
4760-
ctx.IsPrivilegedAccount = true
4761-
ctx.oldVM.Annotations[tc.annotationKey] = tc.oldValue
4762-
if tc.newValue != "" {
4763-
ctx.vm.Annotations[tc.annotationKey] = tc.newValue
4764-
}
4765-
},
4766-
expectAllowed: true,
4767-
})
4768-
},
4769-
}
4770-
4771-
for i := range protectedAnnotationTestCases {
4772-
tc := protectedAnnotationTestCases[i]
4773-
table = append(table, Entry("should allow update of "+tc.annotationKey, tc))
4774-
}
4775-
4776-
return table
4777-
}
4778-
4779-
getProtectedAnnotationTableAllowRemoval := func() []any {
4780-
table := []any{
4781-
func(tc protectedAnnotationTestCase) {
4782-
doTest(testParams{
4783-
setup: func(ctx *unitValidatingWebhookContext) {
4784-
ctx.IsPrivilegedAccount = true
4785-
ctx.oldVM.Annotations[tc.annotationKey] = tc.oldValue
4786-
delete(ctx.vm.Annotations, tc.annotationKey)
4787-
},
4788-
expectAllowed: true,
4789-
})
4790-
},
4791-
}
4792-
4793-
for i := range protectedAnnotationTestCases {
4794-
tc := protectedAnnotationTestCases[i]
4795-
table = append(table, Entry("should allow removal of "+tc.annotationKey, tc))
4796-
}
4797-
4798-
return table
4799-
}
4800-
4801-
getProtectedAnnotationTableDisallowUpdate := func() []any {
4802-
table := []any{
4803-
func(tc protectedAnnotationTestCase) {
4804-
doTest(testParams{
4805-
setup: func(ctx *unitValidatingWebhookContext) {
4806-
bypassUpgradeCheck(&ctx.Context, ctx.vm, ctx.oldVM)
4807-
ctx.oldVM.Annotations[tc.annotationKey] = tc.oldValue
4808-
if tc.newValue != "" {
4809-
ctx.vm.Annotations[tc.annotationKey] = tc.newValue
4810-
}
4811-
},
4812-
validate: doValidateWithMsg(
4813-
field.Forbidden(annotationPath.Key(tc.annotationKey), "modifying this annotation is not allowed for non-admin users").Error(),
4814-
),
4815-
})
4816-
},
4817-
}
4818-
4819-
for i := range protectedAnnotationTestCases {
4820-
tc := protectedAnnotationTestCases[i]
4821-
table = append(table, Entry("should disallow update of "+tc.annotationKey, tc))
4822-
}
4823-
4824-
return table
4825-
}
4826-
4827-
getProtectedAnnotationTableDisallowRemoval := func() []any {
4828-
table := []any{
4829-
func(tc protectedAnnotationTestCase) {
4830-
doTest(testParams{
4831-
setup: func(ctx *unitValidatingWebhookContext) {
4832-
bypassUpgradeCheck(&ctx.Context, ctx.vm, ctx.oldVM)
4833-
ctx.oldVM.Annotations[tc.annotationKey] = tc.oldValue
4834-
delete(ctx.vm.Annotations, tc.annotationKey)
4835-
},
4836-
validate: doValidateWithMsg(
4837-
field.Forbidden(annotationPath.Key(tc.annotationKey), "modifying this annotation is not allowed for non-admin users").Error(),
4838-
),
4839-
})
4840-
},
4841-
}
4842-
4843-
for i := range protectedAnnotationTestCases {
4844-
tc := protectedAnnotationTestCases[i]
4845-
table = append(table, Entry("should disallow removal of "+tc.annotationKey, tc))
4846-
}
4847-
4848-
return table
4849-
}
4850-
4851-
DescribeTable("disallow update of protected annotations by non-privileged user",
4852-
getProtectedAnnotationTableDisallowUpdate()...,
4853-
)
4854-
4855-
DescribeTable("disallow removal of protected annotations by non-privileged user",
4856-
getProtectedAnnotationTableDisallowRemoval()...,
4857-
)
4858-
4859-
DescribeTable("allow update of protected annotations by privileged user",
4860-
getProtectedAnnotationTableAllowUpdate()...,
4861-
)
4862-
4863-
DescribeTable("allow removal of protected annotations by privileged user",
4864-
getProtectedAnnotationTableAllowRemoval()...,
4865-
)
48664659
})
48674660

48684661
Context("Bootstrap", func() {

0 commit comments

Comments
 (0)