Fix subresource update logic. (#2546)

* Fix subresource update logic.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Rename IsInSubResourceUpdate => GetUpdatedSubresource

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

apis/contexts.go

@@ -85,14 +85,21 @@ func IsInUpdate(ctx context.Context) bool {
 	return ctx.Value(inUpdateKey{}) != nil
 }
 
-// IsInStatusUpdate checks whether the context is an Update.
-func IsInStatusUpdate(ctx context.Context) bool {
+// GetUpdatedSubresource returns the subresource being updated or "" if there
+// is no subresource that's being updated. Examples are "status" for Status
+// updates, or "scale" for scaling Deployment.
+func GetUpdatedSubresource(ctx context.Context) string {
 	value := ctx.Value(inUpdateKey{})
 	if value == nil {
-		return false
+		return ""
 	}
 	up := value.(*updatePayload)
-	return up.subresource == "status"
+	return up.subresource
+}
+
+// IsInStatusUpdate checks whether the context is an Update.
+func IsInStatusUpdate(ctx context.Context) bool {
+	return GetUpdatedSubresource(ctx) == "status"
 }
 
 // GetBaseline returns the baseline of the update, or nil when we

testing/resource.go

@@ -55,6 +55,9 @@ type ResourceSpec struct {
 	FieldForCallbackDefaultingUsername       string `json:"fieldForCallbackDefaultingUsername,omitempty"`
 }
 
+// Reject any udpates to this subresource.
+const disallowedSubresource = "badbadsubresource"
+
 // GetGroupVersionKind returns the GroupVersionKind.
 func (r *Resource) GetGroupVersionKind() schema.GroupVersionKind {
 	return SchemeGroupVersion.WithKind("Resource")
@@ -86,6 +89,7 @@ func (r *Resource) Validate(ctx context.Context) *apis.FieldError {
 	if apis.IsInUpdate(ctx) {
 		original := apis.GetBaseline(ctx).(*Resource)
 		err = err.Also(r.CheckImmutableFields(ctx, original))
+		err = err.Also(r.CheckAllowedSubresourceUpdate(ctx, original))
 	}
 	return err
 }
@@ -146,6 +150,16 @@ func (r *Resource) CheckImmutableFields(ctx context.Context, original *Resource)
 	return nil
 }
 
+func (r *Resource) CheckAllowedSubresourceUpdate(ctx context.Context, original *Resource) *apis.FieldError {
+	if apis.GetUpdatedSubresource(ctx) == disallowedSubresource {
+		return &apis.FieldError{
+			Message: "Disallowed subresource update",
+			Details: fmt.Sprintf("Disallowed subresource update: %s", disallowedSubresource),
+		}
+	}
+	return nil
+}
+
 // GetListType implements apis.Listable
 func (r *Resource) GetListType() runtime.Object {
 	return &ResourceList{}

webhook/resourcesemantics/validation/validation_admit.go

@@ -141,13 +141,13 @@ func (ac *reconciler) decodeRequestAndPrepareContext(
 		ctx = apis.WithDryRun(ctx)
 	}
 
-	if newObj != nil && oldObj != nil && req.SubResource == "" {
-		ctx = apis.WithinSubResourceUpdate(ctx, oldObj, req.SubResource)
-	}
-
 	switch req.Operation {
 	case admissionv1.Update:
-		ctx = apis.WithinUpdate(ctx, oldObj)
+		if req.SubResource != "" {
+			ctx = apis.WithinSubResourceUpdate(ctx, oldObj, req.SubResource)
+		} else {
+			ctx = apis.WithinUpdate(ctx, oldObj)
+		}
 	case admissionv1.Create:
 		ctx = apis.WithinCreate(ctx)
 	case admissionv1.Delete:

webhook/resourcesemantics/validation/validation_admit_test.go

@@ -468,10 +468,11 @@ func createCreateResource(ctx context.Context, t *testing.T, r *Resource) *admis
 
 func TestAdmitUpdates(t *testing.T) {
 	tests := []struct {
-		name      string
-		setup     func(context.Context, *Resource)
-		mutate    func(context.Context, *Resource)
-		rejection string
+		name        string
+		setup       func(context.Context, *Resource)
+		mutate      func(context.Context, *Resource)
+		subresource string
+		rejection   string
 	}{{
 		name: "test simple update (no diff)",
 		setup: func(ctx context.Context, r *Resource) {
@@ -499,6 +500,23 @@ func TestAdmitUpdates(t *testing.T) {
 			r.Spec.FieldWithValidation = "not what's expected"
 		},
 		rejection: "invalid value",
+	}, {
+		name: "bad mutation (invalid subresource)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+		},
+		subresource: "badbadsubresource",
+		rejection:   "validation failed: Disallowed subresource update: \nDisallowed subresource update: badbadsubresource",
+	}, {
+		name: "good mutation with valid subresource",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+		},
+		subresource: "goodgoodsubresource",
 	}}
 
 	for _, tc := range tests {
@@ -521,7 +539,7 @@ func TestAdmitUpdates(t *testing.T) {
 			tc.mutate(ctx, new)
 
 			_, ac := newNonRunningTestResourceAdmissionController(t)
-			resp := ac.Admit(ctx, createUpdateResource(ctx, t, old, new))
+			resp := ac.Admit(ctx, createUpdateResource(ctx, t, old, new, tc.subresource))
 
 			if tc.rejection == "" {
 				ExpectAllowed(t, resp)
@@ -532,7 +550,7 @@ func TestAdmitUpdates(t *testing.T) {
 	}
 }
 
-func createUpdateResource(ctx context.Context, t *testing.T, old, new *Resource) *admissionv1.AdmissionRequest {
+func createUpdateResource(ctx context.Context, t *testing.T, old, new *Resource, subresource string) *admissionv1.AdmissionRequest {
 	t.Helper()
 	req := &admissionv1.AdmissionRequest{
 		Operation: admissionv1.Update,
@@ -541,7 +559,8 @@ func createUpdateResource(ctx context.Context, t *testing.T, old, new *Resource)
 			Version: "v1alpha1",
 			Kind:    "Resource",
 		},
-		UserInfo: *apis.GetUserInfo(ctx),
+		UserInfo:    *apis.GetUserInfo(ctx),
+		SubResource: subresource,
 	}
 	marshaled, err := json.Marshal(new)
 	if err != nil {

webhook/resourcesemantics/validation/validation_deprecated_test.go

@@ -47,7 +47,7 @@ func TestStrictValidation(t *testing.T) {
 
 	newUpdateReq := func(old, new []byte) *admissionv1.AdmissionRequest {
 		req := &admissionv1.AdmissionRequest{
-			Operation: admissionv1.Create,
+			Operation: admissionv1.Update,
 			Kind: metav1.GroupVersionKind{
 				Group:   "pkg.knative.dev",
 				Version: "v1alpha1",