⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

bhcleek · 2021-05-23T18:42:44Z

Use Result.Message instead of Result.Reason for the user supplied
portion. Reason is intended to be machine readable while Message is
intended to be human readable. While each is documented as being
informational only, the Is* family of functions in
k8s.io/apimachinery/pkg/api/errors rely on the StatusReason.

This change allows controllers to rely on
"k8s.io/apimachinery/pkg/api/errors".IsForbidden to deal with errors
consistently regardless of whether the operation failed due to an
admission webhook implemented with controller-runtime, a standard
kubernetes failure (e.g related to RBAC), or another controller not
implemented with controller-runtime.

See kubernetes/kubernetes#101926 for more information

k8s-ci-robot · 2021-05-23T18:42:52Z

Hi @bhcleek. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

coderanger · 2021-05-23T19:12:50Z

Trying to think of ways this compat break could bite someone. We should be very certain we can't think of any.

bhcleek · 2021-05-23T19:24:31Z

Trying to think of ways this compat break could bite someone. We should be very certain we can't think of any.

I was torn on whether to mark this with ⚠️ or not, but decided to err on the conservative side.

I don't think this will break any controller-runtime clients, but it will cause "k8s.io/apimachinery/pkg/errors".IsForbidden calls to match Deny responses where previously it would not. This is actually a good thing, because controller code often checks IsForbidden (and other functions in the Is* family) to decide whether to retry an operation or not.

bhcleek · 2021-06-30T22:36:11Z

Is there anything we can do to move this forward?

bowei · 2021-07-11T19:56:25Z

/ok-to-test

bhcleek · 2021-07-11T23:07:46Z

/retest

Use Result.Message instead of Result.Reason for the user supplied portion. Reason is intended to be machine readable while Message is intended to be human readable. While each is documented as being informational only, the Is* family of functions in k8s.io/apimachinery/pkg/api/errors rely on the StatusReason. This change allows controllers to rely on "k8s.io/apimachinery/pkg/api/errors".IsForbidden to deal with errors consistently regardless of whether the operation failed due to an admission webhook implemented with controller-runtime, a standard kubernetes failure (e.g related to RBAC), or another controller not implemented with controller-runtime. See kubernetes/kubernetes#101926 for more information

joelanford

I think the piece of this that gives me some pause is that this change is API compatible, but it breaks semantics. So calling code will compile and run just fine, but what's happening may not be what a caller expects (e.g. perhaps a caller really does want to set a custom reason)

I do agree that it makes more sense to expose the message and auto-populate the reason.

I would almost prefer that we find a way to break the Go API so that users are confronted with this breakage.

joelanford · 2021-07-21T15:43:42Z

pkg/webhook/admission/response.go

 			},
 		},
 	}
-	if len(reason) > 0 {
-		resp.Result.Reason = metav1.StatusReason(reason)
+	if len(message) > 0 {


Nit: Do we need to check the length of message or can we just always set resp.Result.Message? metav1.Status.Message is a string type, so leaving it "unset" is the same as setting it to an empty string.

You're right, we no longer need to check the length here. Is it worth changing or should we keep it as-is if there aren't other changes to be made?

bhcleek

I think the piece of this that gives me some pause is that this change is API compatible, but it breaks semantics. So calling code will compile and run just fine, but what's happening may not be what a caller expects (e.g. perhaps a caller really does want to set a custom reason)

I do agree that it makes more sense to expose the message and auto-populate the reason.

I would almost prefer that we find a way to break the Go API so that users are confronted with this breakage.

Given that controller-runtime is still pre-1.0, this kind of breaking change in semantics seems appropriate to me, but I'll happily defer to you if you feel strongly. I see a couple of possibilities for addressing your concerns about the changing semantics:

break the API by removing the second parameter of ValidationResponse entirely so that existing code will fail on compile and introduce a new ValidationResponseWithReasonAndMessage to allow both the reason and message to be provided.
Keep API compatibility, but introduce func ValidationResponseWithReason(allowed bool, reason, message string) Response to allow users to migrate if they really do want to provide a custom reason.

What do you think?

bhcleek · 2021-07-21T20:38:05Z

pkg/webhook/admission/response.go

 			},
 		},
 	}
-	if len(reason) > 0 {
-		resp.Result.Reason = metav1.StatusReason(reason)
+	if len(message) > 0 {


You're right, we no longer need to check the length here. Is it worth changing or should we keep it as-is if there aren't other changes to be made?

alvaroaleman · 2021-08-16T21:27:33Z

@bhcleek is this still needed now that kubernetes/kubernetes#101926 merged? Like Joe I'd prefer not to introduce breaking changes the compiler can't point out.

bhcleek · 2021-08-16T23:50:36Z

While this isn't strictly needed with the next version of Kubernetes or later, it's still useful for people on the current version of Kubernetes or earlier.

I'd also add, that IMHO this is closer to the expectations that people may have given how this message can be surfaced to users.

k8s-triage-robot · 2021-11-15T00:11:14Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

bhcleek · 2021-11-15T18:17:19Z

/remove-lifecycle stale

bhcleek · 2022-02-02T17:25:51Z

Where do we stand with this? Should we close it or is there something we can do to push forward on the alignment with kubernetes error expectations?

alvaroaleman

/hold
@joelanford are you good with this?

k8s-triage-robot · 2022-05-05T15:15:11Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

bhcleek · 2022-06-01T16:13:42Z

/remove-lifecycle stale

k8s-triage-robot · 2022-08-30T16:41:54Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

bhcleek · 2022-09-03T20:15:30Z

/remove-lifecycle stale

sbueringer · 2022-09-05T12:24:12Z

I think it would be good if this change is surfaced via compile errors to consumers.

I would suggest:

Allowed() Response

Doesn't need any reason or message.
If folks want to set them anyway for an allowed request they can use ValidationResponse, but this shouldn't be neccessary:

  // Result contains extra details into why an admission request was denied.
  // This field IS NOT consulted in any way if "Allowed" is "true".
  // +optional
  Result *metav1.Status `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"`

Denied() Response
- Essentially a shortcut to return deny with metav1.StatusReasonForbidden set
Patched(patches ...jsonpatch.JsonPatchOperation) Response
- Doesn't need reason/message as it's also an allowed request (see above)
ValidationResponse(allowed bool, reason, message string) Response
- Just one simple func where everything can be provided
- If reason is "" and allowed is false I would set reason to metav1.StatusReasonForbidden

I think this would signal clearly to consumers that the API changed and the new set of funcs should cover relevant use cases.

bhcleek · 2022-09-05T15:50:19Z

I can make that change. The reason, though, should probably be a metav1.StatusReason instead of a string so that users cannot easily swap the reason and message arguments unintentionally. But what value should be used for the reason when allowed is set?

edit: formatting

sbueringer · 2022-09-05T15:52:48Z

I can make that change. The reason, though, should probably be a metav1.StatusReason instead of a string so that users cannot easily swap the reason and message arguments unintentionally. But what value should be used for the reason when allowed is set?

Sounds good. I don't think we have to set the reason when allowed is true. reason is part of Result and Result " contains extra details into why an admission request was denied". So I think if the request is allowed we don't have to provide details about that.

(btw just my opinion, would be probably good to get consensus before investing the effort to change the implementation)

bhcleek · 2022-09-05T15:56:18Z

Yeah, the few places I need it, metav1.StatusReasonUnknown can be used for success (e.g. Allowed())

sbueringer · 2022-09-05T16:06:25Z

Yeah, the few places I need it, metav1.StatusReasonUnknown can be used for success (e.g. Allowed())

Why is a reason needed for Allowed?

bhcleek · 2022-09-05T16:07:40Z

If ValidationResponse is to have three parameters as you suggested with ValidationResponse(allowed bool, reason, message string), then an argument for the reason has to be provided.

bhcleek · 2022-09-05T16:26:42Z

After making the changes locally that you suggested, @sbueringer, I'm less convinced that's the direction this should go. Being able to provide the human readable message to Allowed and Denied is useful and requiring users provide the reason to ValidationResponse puts more burden on users to understand all the possible StatusReason values.

I know you're trying to avoid unexpected breakages, but given that controller-runtime regularly breaks across releases and effectively abuses semantic versioning anyway in order to be able to break backward compatibility without violating semantic versioning coupled with the greater burden that those suggestions would put on users, I'm not sure we should make those changes. 🤔

Having expressed my skepticism, I want to be clear that I will submit the changes if there's consensus.

sbueringer · 2022-09-05T17:18:24Z

Okay no worries :). Just my opinion, I'm fine either way.

k8s-triage-robot · 2022-12-04T18:04:13Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2023-01-03T18:56:44Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

bhcleek · 2023-01-03T21:44:46Z

/remove-lifecycle rotten

@alvaroaleman with only 21 open PRs, I'm wondering if there's anything we can do to move this forward.

alvaroaleman · 2023-01-03T22:32:35Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 23, 2021

k8s-ci-robot requested review from alexeldeib and mengqiy May 23, 2021 18:42

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 23, 2021

bhcleek changed the title ~~⚠️ pkg/webhook/admission: use Result.Message instead of Result.Reason~~ (:warning:, major) pkg/webhook/admission: use Result.Message instead of Result.Reason May 23, 2021

bhcleek changed the title ~~(:warning:, major) pkg/webhook/admission: use Result.Message instead of Result.Reason~~ ⚠ (:warning:, major) pkg/webhook/admission: use Result.Message instead of Result.Reason May 23, 2021

bhcleek changed the title ~~⚠ (:warning:, major) pkg/webhook/admission: use Result.Message instead of Result.Reason~~ ⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason May 23, 2021

bhcleek mentioned this pull request May 24, 2021

check APIStatus.Code in Is* family of functions kubernetes/kubernetes#101926

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2021

bhcleek force-pushed the message-human-readable branch from 3ff152d to 2c95a03 Compare July 14, 2021 04:41

joelanford reviewed Jul 21, 2021

View reviewed changes

bhcleek commented Jul 21, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2021

liggitt mentioned this pull request Nov 30, 2021

Set Forbidden as the response status reason open-policy-agent/gatekeeper#1692

Merged

alvaroaleman approved these changes Feb 4, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 5, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 30, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 3, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 4, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 3, 2023

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 3, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2023

k8s-ci-robot merged commit 056869c into kubernetes-sigs:master Jan 3, 2023

bhcleek deleted the message-human-readable branch January 3, 2023 23:34

This was referenced Feb 12, 2023

Follow-up tasks for CR bump to v0.14.0 kubernetes-sigs/cluster-api#7925

Closed

Follow-up tasks for CR bump to (upcoming) v0.15.0 kubernetes-sigs/cluster-api#8093

Closed

ary1992 mentioned this pull request Jul 13, 2023

Upgrade k8s.io/* to v0.27, sigs.k8s.io/controller-runtime to v0.15 gardener/gardener#7991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

bhcleek commented May 23, 2021

k8s-ci-robot commented May 23, 2021

coderanger commented May 23, 2021

bhcleek commented May 23, 2021

bhcleek commented Jun 30, 2021

bowei commented Jul 11, 2021

bhcleek commented Jul 11, 2021

joelanford left a comment

joelanford Jul 21, 2021

bhcleek Jul 21, 2021

bhcleek left a comment

bhcleek Jul 21, 2021

alvaroaleman commented Aug 16, 2021

bhcleek commented Aug 16, 2021

k8s-triage-robot commented Nov 15, 2021

bhcleek commented Nov 15, 2021

bhcleek commented Feb 2, 2022

alvaroaleman left a comment

k8s-triage-robot commented May 5, 2022

bhcleek commented Jun 1, 2022

k8s-triage-robot commented Aug 30, 2022

bhcleek commented Sep 3, 2022

sbueringer commented Sep 5, 2022 •

edited

Loading

bhcleek commented Sep 5, 2022 •

edited

Loading

sbueringer commented Sep 5, 2022 •

edited

Loading

bhcleek commented Sep 5, 2022

sbueringer commented Sep 5, 2022

bhcleek commented Sep 5, 2022

bhcleek commented Sep 5, 2022

sbueringer commented Sep 5, 2022

k8s-triage-robot commented Dec 4, 2022

k8s-triage-robot commented Jan 3, 2023

bhcleek commented Jan 3, 2023

alvaroaleman commented Jan 3, 2023

⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

⚠ pkg/webhook/admission: use Result.Message instead of Result.Reason #1539

Conversation

bhcleek commented May 23, 2021

k8s-ci-robot commented May 23, 2021

coderanger commented May 23, 2021

bhcleek commented May 23, 2021

bhcleek commented Jun 30, 2021

bowei commented Jul 11, 2021

bhcleek commented Jul 11, 2021

joelanford left a comment

Choose a reason for hiding this comment

joelanford Jul 21, 2021

Choose a reason for hiding this comment

bhcleek Jul 21, 2021

Choose a reason for hiding this comment

bhcleek left a comment

Choose a reason for hiding this comment

bhcleek Jul 21, 2021

Choose a reason for hiding this comment

alvaroaleman commented Aug 16, 2021

bhcleek commented Aug 16, 2021

k8s-triage-robot commented Nov 15, 2021

bhcleek commented Nov 15, 2021

bhcleek commented Feb 2, 2022

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-triage-robot commented May 5, 2022

bhcleek commented Jun 1, 2022

k8s-triage-robot commented Aug 30, 2022

bhcleek commented Sep 3, 2022

sbueringer commented Sep 5, 2022 • edited Loading

bhcleek commented Sep 5, 2022 • edited Loading

sbueringer commented Sep 5, 2022 • edited Loading

bhcleek commented Sep 5, 2022

sbueringer commented Sep 5, 2022

bhcleek commented Sep 5, 2022

bhcleek commented Sep 5, 2022

sbueringer commented Sep 5, 2022

k8s-triage-robot commented Dec 4, 2022

k8s-triage-robot commented Jan 3, 2023

bhcleek commented Jan 3, 2023

alvaroaleman commented Jan 3, 2023

sbueringer commented Sep 5, 2022 •

edited

Loading

bhcleek commented Sep 5, 2022 •

edited

Loading

sbueringer commented Sep 5, 2022 •

edited

Loading