Return appropriate status codes for rejected requests #97
Conversation
Signed-off-by: Max Smythe <smythe@google.com>
|
@liggitt this should fix the return code on GK's side. I think there may be some API tweaks needed in the controller-runtime library to allow the consumer of the webhook API to set the response code. |
|
lgtm |
|
at minimum, the controller-runtime library should be setting code to something >= 400 if allowed=false I could see an additional helper that accepts a status reason and maps that to the standard code |
|
LGTM |
| if vResp.Response.Result == nil { | ||
| vResp.Response.Result = &metav1.Status{} | ||
| } | ||
| vResp.Response.Result.Code = http.StatusForbidden |
There was a problem hiding this comment.
Don't you want to send back 422 (Unprocessable entity) instead of 403 (Forbidden)
403 is what is sent back in an RBAC/AUTH failure and this is going to confuse the living daylights out of users.
We use 422 in a number of other places to indicate validation failures:
|
Quick comment, I don't think that 403 is the right response code in this case. 403 is an RBAC/Authn/Authz related code, I think 422 is what we typically use for "parseable, but invalid data" |
|
Is this invalid data, or valid data forbidden by policy? By the time we get here, the object has passed API validation and in the absence of this plugin, would be allowed. I can see it both ways… as a point of reference, policy-type admission plugins in-tree return forbidden errors (e.g. https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/security/podsecuritypolicy/admission.go#L149) |
|
I think the message that accompanies the forbidden error makes all the difference in helping the user understand the reason |
|
Hrm, I don't think that matches with the standard usage of HTTP codes. I agree that the specification is vague enough that you could interpret it this way, I think in practice almost no one expects that a 403 means "I don't like that object" The key sentence in the spec is: Do we really think of policy as 'authorizing' a request? I really think it is 'validating' a request. |
|
This is the bit that makes the most sense to me:
400 and 422 have implications that don't seem quite right either (as if we don't understand their request or cannot accept their request… in this case, we understand it perfectly, and it would normally be considered valid, but a policy is forbidding it) Like I said, I can see it both ways, I'm not sure any of them are a perfect match |
|
My perspective is that constraints are a form of authorization in that they set boundaries on what users are allowed to do vs. what they theoretically could do on an unconstrained system. This implies 403. There is certainly a lot of gray area here. I think the most correct code depends on the intent of the admin implementing the constraint, which we cannot know a priori. Two examples: If an admin creates a constraint that limits users to requesting amounts of RAM that are actually bookable, the most appropriate code seems to be 422 If an admin requires everyone to use a blessed container repository, then the appropriate code seems to be 403. My guess is that the primary use case is policy-based (403) vs correctness-based (422), though I may be wrong. |
|
@maxsmythe I agree there's lots of gray. What if the admin requires certain labels (e.g. team metadata like contact info) that feels more like 422 (e.g. the RAM example) vs the container registry (which is more like forbidden) Anyway, I'm happy to drop this, we should probably document it somewhere, and definitely make sure that the error messages are on-point so that no one gets confused. |
Currently we are relying on the framework to set status codes, which always returns 200 OK, which is not okay in some situations.
This fixes a bug where rejected create requests appear to apply successfully and cause kubectl to return an exit code of zero.
Signed-off-by: Max Smythe smythe@google.com