Repository: knative/pkg
Pull request #2546 (2022-07-15 16:09:40)

Fix subresource update logic.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

<!-- Thanks for sending a pull request! -->

# Changes
While working on: https://github.com/sigstore/policy-controller/pull/88 I noticed that subresources are not being correctly handled.

🐛 Fix a bug where subresource was not correctly set when doing webhook validation.

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

-
-
-

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind bug

<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

<!-- Please include the 'why' behind your changes if no issue exists -->

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->
```release-note
Fix a bug where subresource updates were being ignored.
```

**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [knative/docs]: <issue or pr link>
- [Feature Track]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2546#" title="Author self-approved">vaikas</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/main/OWNERS)~~ [vaikas]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2546?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2546](https://codecov.io/gh/knative/pkg/pull/2546?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (3e662c0) into [main](https://codecov.io/gh/knative/pkg/commit/418e13833beae314fd3073cfc67591263e941984?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (418e138) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##             main    #2546   +/-   ##
=======================================
  Coverage   81.64%   81.64%           
=======================================
  Files         163      163           
  Lines        9745     9748    +3     
=======================================
+ Hits         7956     7959    +3     
  Misses       1551     1551           
  Partials      238      238           
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2546?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [apis/contexts.go](https://codecov.io/gh/knative/pkg/pull/2546/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9jb250ZXh0cy5nbw==) | `100.00% <100.00%> (ø)` | |
| [...k/resourcesemantics/validation/validation\_admit.go](https://codecov.io/gh/knative/pkg/pull/2546/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-d2ViaG9vay9yZXNvdXJjZXNlbWFudGljcy92YWxpZGF0aW9uL3ZhbGlkYXRpb25fYWRtaXQuZ28=) | `93.12% <100.00%> (+0.05%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2546?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/2546?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [418e138...3e662c0](https://codecov.io/gh/knative/pkg/pull/2546?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

knative-prow[bot]: @vaikas: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit-tests_pkg_main | ceefc8748b3455d351caf8b9885e0611b64fe618 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2546/unit-tests_pkg_main/1547995113215496192) | true | `/test unit-tests_pkg_main`

[Your PR dashboard](https://prow.knative.dev/pr).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
dprotaso: fixes: https://github.com/knative/pkg/issues/2184

/lgtm
/approve
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2546#issuecomment-1185801723" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2546#" title="Author self-approved">vaikas</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/main/OWNERS)~~ [dprotaso,vaikas]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/apis/contexts.go b/apis/contexts.go
index b9835fc00..a3550dceb 100644
--- a/apis/contexts.go
+++ b/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
diff --git a/webhook/resourcesemantics/validation/validation_admit.go b/webhook/resourcesemantics/validation/validation_admit.go
index d1e7da94d..8ac55e805 100644
--- a/webhook/resourcesemantics/validation/validation_admit.go
+++ b/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:


Test patch:
diff --git a/testing/resource.go b/testing/resource.go
index e26ca317e..97d424b3b 100644
--- a/testing/resource.go
+++ b/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{}
diff --git a/webhook/resourcesemantics/validation/validation_admit_test.go b/webhook/resourcesemantics/validation/validation_admit_test.go
index 43d049b12..b63e5788d 100644
--- a/webhook/resourcesemantics/validation/validation_admit_test.go
+++ b/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 {
diff --git a/webhook/resourcesemantics/validation/validation_deprecated_test.go b/webhook/resourcesemantics/validation/validation_deprecated_test.go
index 0998c7f65..c973e20f0 100644
--- a/webhook/resourcesemantics/validation/validation_deprecated_test.go
+++ b/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",