Add design details for Custom resource definition webhook validation. #1418
Conversation
k8s-ci-robot
added
sig/api-machinery
|
Yeah, that was my suggestion. Just let me repeat that I'd like to get some miles on the webhooks before adding an auto-configuration layer. |
| @@ -381,6 +381,28 @@ Note: | |||
|
|
|||
| 2. For migration of CRDs with no validation to CRDs with validation, we can create a controller that will validate and annotate invalid CRs once the spec changes, so that the custom controller can choose to delete them (this is also essentially the status condition of the CRD). This can be achieved, but it is not part of the proposal. | |||
|
|
|||
| ### Admission Webhook | |||
|
|
|||
| Custom resource definitions use the normal REST endpoint implementation and only customizes the registry and the codecs consequentaly dynamic web-hook admission controllers can be used | |||
There was a problem hiding this comment.
Reword, hard to read-- this is missing a comma or something.
brendandburns
force-pushed
the
master
branch
from
92e7bfd
to
2a9287c
Compare
k8s-ci-robot
added
the
approved
|
@lavalamp comment addressed, please re-check. Thanks! |
| @@ -381,6 +381,29 @@ Note: | |||
|
|
|||
| 2. For migration of CRDs with no validation to CRDs with validation, we can create a controller that will validate and annotate invalid CRs once the spec changes, so that the custom controller can choose to delete them (this is also essentially the status condition of the CRD). This can be achieved, but it is not part of the proposal. | |||
|
|
|||
| ### Admission Webhook | |||
|
|
|||
| Custom resource definitions use the the same REST endpoint as built-in Kubernetes API objects. Consequentaly standard | |||
| create arbitrary admission controllers, it is necessary to have an alternate solution for | ||
| adding web-hook validation to custom resource definitions. | ||
|
|
||
| The solution is that the `CustomResourceDefinition` itself has an array of webhooks in the |
There was a problem hiding this comment.
technically two arrays: mutating, non-mutating
| and in many cases the creator of a `CustomResourceDefinition` is third-party extension code | ||
| (e.g. an instance of the _operator pattern_) which should not have the ability to | ||
| create arbitrary admission controllers, it is necessary to have an alternate solution for | ||
| adding web-hook validation to custom resource definitions. |
There was a problem hiding this comment.
This raises the question about how to create+distribute the necessary certificates. Currently, we don't have a mechanism to auto-create and inject
- the webhook server key+certificate (trusted by the apiextensions-apiserver)
- and the kube-apiserver proxy client CA (trusted and verified by the webhook).
We need both. I don't want to see every CRD controller using this implementing its own certificate generation code, and even worse creating something insecure.
In OpenShift we have a controller that's listening on a custom annotation on apiserver pods and then creates a secret on-demand. This gives a smooth and secure setup with zero config.
There was a problem hiding this comment.
We need to solve the certificate problem. I had a brief chat with @deads2k a few weeks ago about extracting the service signing certificate controller from OpenShift and making it generally available. Whether we do that or find another solution, I believe we need to do something. Otherwise, I feel it's too much of a burden to ask users to figure out certificate generation, signing, etc. to be able to do things like this.
There was a problem hiding this comment.
I think @ncdc is saying this, but I think this is a general problem with dynamic webhooks, not specific to this proposal. We need to make it much easier for people to build dynamic webhooks in general.
There was a problem hiding this comment.
Added some text about this.
brendandburns
force-pushed
the
master
branch
from
2a9287c
to
c00e99e
Compare
|
Comments addressed, please take another look. |
| This ensures that third-party extension code can not register admission controllers for | ||
| arbitrary API objects. | ||
|
|
||
| To achieve this, the `CustomResourceDefinition` has two arrays of `admissionregistration.Webhook` |
There was a problem hiding this comment.
nit: not much new in this paragraph. just move the type admissionregistration.Webhook into the previous paragraph.
|
One nit, otherwise lgtm. There are a number of details like what of |
brendandburns
force-pushed
the
master
branch
from
c00e99e
to
a1ceb56
Compare
|
@sttts comments addressed, please re-check. Thanks! |
| However, because the creation of admission controllers is a fairly high-privilege activity, | ||
| and in many cases the creator of a `CustomResourceDefinition` is third-party extension code | ||
| (e.g. an instance of the _operator pattern_) which should not have the ability to | ||
| create arbitrary admission controllers, it is necessary to have an alternate solution for |
There was a problem hiding this comment.
Seeing operators in the wild that install CRDs and RBAC rules, I question that there is a real difference to "creation of admission controllers" today. Of course, I totally agree that there should be. But for that we need a similar in-CRD mechanism for RBAC rules.
There was a problem hiding this comment.
That is clearly a job for a future design PR :)
There was a problem hiding this comment.
sure, just thinking out loud here ;)
| definition itself. The arrays will be of type `admissionregistration.Webhook`. | ||
| One array will contain non-mutating admission controllers for things like | ||
| validation, and one will have mutating admission controllers for the purposes of defaulting. | ||
| The custom resource controller (a piece of trusted code), will register |
There was a problem hiding this comment.
custom resource definition controller
|
Comments addressed, please re-check. Thanks! |
|
Thinking about the overall flow of creating a CRD, I see this
Given that the first two items require cluster-admin access and the RBAC cluster role implies the power to grant access to any resources, I don't see an avenue for attack that this locks down. If I can create a useful and accessible CRD, I can already read every pod and secret in the system (or grant access to do so). |
|
I think Brendan probably wants to make things easy, not lock them down.
…On Thu, Jan 4, 2018 at 4:50 AM, David Eads ***@***.***> wrote:
Thinking about the overall flow of creating a CRD, I see this
1. Create a CRD and affect global discovery information, including
shadowing existing resources and stealing shortnames. This is currently
reserved to cluster admins by default.
2. Create an RBAC cluster role to allow users to use the new custom
resource. This requires passing an access escalation check. Since the
custom resource won't exist, that means it is reserved to cluster admins
too.
3. Create an AdmissionRegistration. This currently requires
cluster-admin as well.
Given that the first two items require cluster-admin access and the RBAC
cluster role implies the power to grant access to any resources, I don't
see an avenue for attack that this locks down. If I can create a useful and
accessible CRD, I can already read every pod and secret in the system (or
grant access to do so).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1418 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglj1EHhJbJt5POpYzjfVRYPve1Nqkks5tHMkUgaJpZM4Qjw4_>
.
|
I'm not sure where this comment is going. In a cluster that cares about security, I think my point stands and if you grant permissions creating separate resources works fine. In a cluster that doesn't care about security, you don't have a restriction on those things and creating separate resources works fine. Are you suggesting there's a third case? Could you describe it? |
|
Maybe I should let Brendan say what he wants, but I think it is something
along the lines of "in a cluster that doesn't care about security I only
have to submit the CRD to the system, no other configuration objects".
This is about making the CRD developer and installer's life easy.
(I think we would do well to adopt that goal in general, but I am
personally ambivalent about whether this particular thing goes very far in
service of this goal, esp. if a CRD author generally ends up running a
controller etc. anyway.)
…On Fri, Jan 5, 2018 at 5:33 AM, David Eads ***@***.***> wrote:
I think Brendan probably wants to make things easy, not lock them down.
I'm not sure where this comment is going. In a cluster that cares about
security, I think my point stands and if you grant permissions creating
separate resources works fine. In a cluster that doesn't care about
security, you don't have a restriction on those things and creating
separate resources works fine. Are you suggesting there's a third case?
Could you describe it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1418 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAngln8VvTMm1x_N5ax6z9xa2sg_JS_Rks5tHiSsgaJpZM4Qjw4_>
.
|
|
(sorry returning to this) @deads2k regarding your point, there are two things:
Cluster-admin creates a stub CRD with names/short-names but no real details. In this model, you can keep CRD creation with cluster-admin, but delegate the details of the CRD definition to the CRD operator. I don't see anything in your concerns that should block merging this design proposal so that we can start building it, let me know if you feel differently and I'll address those concerns. Thanks |
k8s-github-robot
added
the
kind/design
My comments and concerns address the core concept of this pull and I think that until we have agreement in concept, we should not merge this change.
The power to update the The CRD-operator should not get to decide the shape of the CRD that it uses and it also not be able to control the validation associated with it. When the CRD is created, it should contain its validation rules. This means the responsibility lies with the CRD creator who is also creating the roles. It looks like a cluster-admin to me. |
|
Maybe we would do well to work on having some common goal / requirements around usability + security for CRDs. I don't think we can come to agreement on a specific proposal until we agree what we're even trying to accomplish. |
|
@deads2k given that that is up to the cluster admin to decide via RBAC, I don't see the issue here. IF the cluster admin wants to force manual install for all CRDs that's their call. That said, I think there are lots of clusters were people want things to behave like plugins and just dynamically work. But forcing CRD developers to learn about admission controllers and to configure them correctly just adds complexity and work to the job of the CRD developer. Could you describe the disagreement you see in the core concept? For me the core-concept(s) are:
Nothing in this proposal is affecting the security model one way or the other, the cluster admin is free to set RBAC however they choose. Please clarify, thanks! |
This piece. It isn't obvious to me what value this adds. You're duplicating an existing API in a different object to save setting three fields. In the use-cases we have already the actor creating a useful CRD already has full permission on cluster, so there isn't much value in duplicating the API. You're still going to be writing the admission webhook code either way. You're just eliminating writing a rule from the resource manifest. Also, writing a library to make the admission plugin webhook is fairly easy. See https://github.com/openshift/generic-admission-server as an example in golang. |
|
The most important feature of this isn't the simplification (though I think that's important too) it's preventing someone from making mistakes and accidentally registering an Admission Controller that has broader scope than just the particular CRD. Given that your objection is based on not seeing the value (and I believe there is the value outlined above) can we agree to disagree and move forward with this proposal? I don't think it hurts anything, and I personally believe there is significant value in terms of ease of use and prevention of mistakes. |
I'd prefer we not expand/nest APIs if we don't have to. I'd rather see API additions justified by enabling use cases that are not currently possible. |
|
I don't think this is nesting, this is like the fact that One of the recurring benefits of CRDs (and the reason why they are way more popular than aggregated API servers) is ease of use. If you tell someone that if they want to validate their CRD they need to either learn Swagger or learn about admission controllers, you are erecting serious barriers to entry. I think you need to focus on the usability for the average user, not the person who spends their time reading sig-api-machinery. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
k8s-ci-robot
removed
the
approved
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@kubernetes/sig-api-machinery-api-reviews @lavalamp