Set Forbidden as the response status reason

[regadas]

What this PR does / why we need it:
Recently while integrating gatekeeper I noticed that a denied admission webook response will look somewhat like this:

Message and Reason kinda have the same info.

errors.StatusError{
    ErrStatus:v1.Status{
        TypeMeta:v1.TypeMeta{Kind:\"\", APIVersion:\"\"},
        ListMeta:v1.ListMeta{
            SelfLink:\"\",
            ResourceVersion:\"\",
            Continue:\"\",
            RemainingItemCount:(*int64)(nil)},
        Status:\"Failure\",
        Message:\"admission webhook \\\"validation.gatekeeper.sh\\\" denied the request: [enforce-resource-quota] resource exceeded cpu quota\",
        Reason:\"[enforce-resource-quota] resource exceeded cpu quota\",
        Details:(*v1.StatusDetails)(nil),
        Code:403}
}

I guess this is generally ok! However, if one is using "k8s.io/apimachinery/pkg/api/errors" to determine the type of error we are out of luck since it relies on the Reason property.

Given the above example:

errors.IsForbidden(err) // False

I suggest a small change in the StatusError and give Reason the proper description keeping Message as the human-readable description of this operation.

errors.StatusError{
    ErrStatus:v1.Status{
        TypeMeta:v1.TypeMeta{Kind:\"\", APIVersion:\"\"},
        ListMeta:v1.ListMeta{
            SelfLink:\"\",
            ResourceVersion:\"\",
            Continue:\"\",
            RemainingItemCount:(*int64)(nil)},
        Status:\"Failure\",
        Message:\"admission webhook \\\"validation.gatekeeper.sh\\\" denied the request: [enforce-resource-quota] resource exceeded cpu quota\",
        Reason:\"Forbidden\",
        Details:(*v1.StatusDetails)(nil),
        Code:403}
}

Fixes #1693

[maxsmythe]

Thanks for the PR! I think this makes sense, but if raises a few questions:

Why does controller-runtime not currently work this way? Do we run the risk of breaking something else by changing this? Is it possible that this should be fixed at the controller-runtime level?

For this PR, I'm mainly concerned about the second question.

[liggitt]

Why does controller-runtime not currently work this way? Do we run the risk of breaking something else by changing this? Is it possible that this should be fixed at the controller-runtime level?

it's a bug in controller-runtime, fix proposed in kubernetes-sigs/controller-runtime#1539

[regadas]

Hi @maxsmythe @liggitt!

Thanks, I was unaware of that proposed fix in kubernetes-sigs/controller-runtime#1539 which makes sense to me. However, it seems to me that there's no agreement if it should go in which will delay things a bit.

Do we run the risk of breaking something else by changing this?

Possibly:

• requests that didn't match isForbidden will now match (but this is a good thing)
• parsing Reason when they should be parsing Message

IMO, I think it still makes sense to address this in `gatekeeper because users on current versions or earlier will still benefit from it.

Let me know what do you think. I'll propose a new change that's in line with the above PR and safe for future updates.

[maxsmythe]

Thanks @liggitt for the context! Knowing that it's a bug in controller-runtime, it sounds like we're probably safe to fix it here.

Chesterton's fence is ready for removal ;)

[maxsmythe]

LGTM

[codecov-commenter]

Codecov Report

Merging #1692 (7bbc27f) into master (09bdc07) will increase coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1692      +/-   ##
==========================================
+ Coverage   51.81%   51.93%   +0.11%     
==========================================
  Files          98       98              
  Lines        8711     8718       +7     
==========================================
+ Hits         4514     4528      +14     
+ Misses       3833     3831       -2     
+ Partials      364      359       -5     

Flag	Coverage Δ
unittests	51.93% <0.00%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/webhook/policy.go	28.37% <0.00%> (-0.56%)	⬇️
pkg/watch/replay.go	78.97% <0.00%> (-2.28%)	⬇️
...onstrainttemplate/constrainttemplate_controller.go	58.17% <0.00%> (+4.32%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3d029f...7bbc27f. Read the comment docs.

[ritazh]

LGTM

[regadas]

Hi @ritazh @priyamshet; quick question: In which version are you aiming to have this in? currently I'm running a fork of gatekeeper because this change is a blocker for me.

Just want to understand for how long I need to keep it 😄

Thanks.

[ritazh]

@regadas This commit should be included in the upcoming release v3.8.0 in the next couple of weeks. Thanks for your patience!