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

[bhcleek]

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]

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]

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

[bhcleek]

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]

Is there anything we can do to move this forward?

[bowei]

/ok-to-test

[bhcleek]

/retest

[alvaroaleman]

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

[bhcleek]

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]

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]

/remove-lifecycle stale

[bhcleek]

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?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, bhcleek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[k8s-triage-robot]

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]

/remove-lifecycle stale

[k8s-triage-robot]

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]

/remove-lifecycle stale

[sbueringer]

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]

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]

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]

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

[sbueringer]

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]

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]

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]

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

[k8s-triage-robot]

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]

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]

/remove-lifecycle rotten

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

[alvaroaleman]

/hold cancel