New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dry run in admission plugins #66391

Merged
merged 1 commit into from Aug 7, 2018

Conversation

Projects
None yet
6 participants
@jennybuckley
Contributor

jennybuckley commented Jul 19, 2018

What this PR does / why we need it:
Adds support for dry run to admission controllers as outlined by kubernetes/community#2387

  • add IsDryRun() to admission.Attributes interface
  • add dry run support to NamespaceAutoProvision
  • add dry run support to ResourceQuota
  • add dry run support to EventRateLimit

The following is being done in a follow up PR:

  • add DryRun to admission.k8s.io/v1beta1.AdmissionReview
  • add DryRunnable to admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration
  • add dry run support to (Valid|Mut)atingAdmissionWebhook

/sig api-machinery

Release note:

In clusters where the DryRun feature is enabled, dry-run requests will go through the normal admission chain. Because of this, ImagePolicyWebhook authors should especially make sure that their webhooks do not rely on side effects.

Here is a list of the admission controllers that were considered when making this PR:

  • AlwaysAdmit: No side effects
  • AlwaysPullImages: No side effects
  • LimitPodHardAntiAffinityTopology: No side effects
  • DefaultTolerationSeconds: No side effects
  • AlwaysDeny: No side effects
  • EventRateLimit: Has side possible effect of affecting the rate, skipping this entire plugin in dry-run case since it won't correspond to an actual write to etcd anyway
  • DenyEscalatingExec: No side effects
  • DenyExecOnPrivileged: Deprecated, and has no side effects
  • ExtendedResourceToleration: No side effects
  • OwnerReferencesPermissionEnforcement: No side effects
  • ImagePolicyWebhook: No side effects* (*this uses a webhook but it is very specialized. It only sees pod container images, for the purpose of accepting or rejecting certain image sources, so it is very unlikely that it would rely on side effects.)
  • LimitRanger: No side effects
  • NamespaceAutoProvision: Has possible side effect of creating a namespace, skipping the create in the dry-run case
  • NamespaceExists: No side effects
  • NodeRestriction: No side effects
  • PodNodeSelector: No side effects
  • PodPreset: No side effects
  • PodTolerationRestriction: No side effects
  • Priority: No side effects
  • ResourceQuota: Has side possible effect of taking up quota, will only check quota but skip changing quota in the dry-run case
  • PodSecurityPolicy: No side effects
  • SecurityContextDeny: No side effects
  • ServiceAccount: No side effects
  • PersistentVolumeLabel: No side effects
  • PersistentVolumeClaimResize: No side effects
  • DefaultStorageClass: No side effects
  • StorageObjectInUseProtection: No side effects
  • Initializers: No side effects
  • NamespaceLifecycle: No side effects
  • MutatingAdmissionWebhook: Same as below
  • ValidatingAdmissionWebhook: Has possible side effects depending on if webhook authors depend on side effects and a reconciliation mechanism. To fix this we will expose whether or not a request is dry-run to webhooks through AdmissionReview, and require that all called webhooks understand the field by checking if DryRunnable true is specified in the webhook config. This will be done in a separate PR because it requires an api-change

@k8s-ci-robot k8s-ci-robot requested review from deads2k and enj Jul 19, 2018

@jennybuckley jennybuckley changed the title from [WIP] Support dry run in admission controllers to Support dry run in admission controllers Jul 20, 2018

@apelisse

This comment has been minimized.

Show comment
Hide comment
@apelisse

apelisse Jul 20, 2018

Member

/hold cancel

Member

apelisse commented Jul 20, 2018

/hold cancel

@apelisse

This comment has been minimized.

Show comment
Hide comment

@jennybuckley jennybuckley changed the title from Support dry run in admission controllers to Support dry run in admission plugins Jul 23, 2018

@apelisse

This comment has been minimized.

Show comment
Hide comment
@apelisse
Member

apelisse commented Jul 27, 2018

LGTM, PTAL @deads2k @liggitt

@@ -44,14 +45,15 @@ type attributesRecord struct {
annotationsLock sync.RWMutex
}
func NewAttributesRecord(object runtime.Object, oldObject runtime.Object, kind schema.GroupVersionKind, namespace, name string, resource schema.GroupVersionResource, subresource string, operation Operation, userInfo user.Info) Attributes {
func NewAttributesRecord(object runtime.Object, oldObject runtime.Object, kind schema.GroupVersionKind, namespace, name string, resource schema.GroupVersionResource, subresource string, operation Operation, dryRun bool, userInfo user.Info) Attributes {

This comment has been minimized.

@deads2k

deads2k Aug 2, 2018

Contributor

I think this is the right idea.

@deads2k

deads2k Aug 2, 2018

Contributor

I think this is the right idea.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Aug 2, 2018

Contributor

I'm guessing you went through all admission plugins and chose these as the ones that have side-effects. Can you post that list in the description so we don't all have to build it from scratch?

Contributor

deads2k commented Aug 2, 2018

I'm guessing you went through all admission plugins and chose these as the ones that have side-effects. Can you post that list in the description so we don't all have to build it from scratch?

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 2, 2018

Contributor

/retest
@deads2k This should be good now, as the non api-changing part. I've also added the list to the description

Contributor

jennybuckley commented Aug 2, 2018

/retest
@deads2k This should be good now, as the non api-changing part. I've also added the list to the description

@jennybuckley jennybuckley referenced this pull request Aug 2, 2018

Merged

Support dry run in admission webhooks #66936

3 of 3 tasks complete
@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

Okay, rebased and fixed nits

Contributor

jennybuckley commented Aug 6, 2018

Okay, rebased and fixed nits

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

/retest

Contributor

jennybuckley commented Aug 6, 2018

/retest

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Aug 6, 2018

Contributor

/lgtm

/hold cancel

Contributor

deads2k commented Aug 6, 2018

/lgtm

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Aug 6, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 6, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jennybuckley

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Contributor

k8s-ci-robot commented Aug 6, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jennybuckley

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

/test pull-kubernetes-kubemark-e2e-gce-big
this one isn't updating for some reason

Contributor

jennybuckley commented Aug 6, 2018

/test pull-kubernetes-kubemark-e2e-gce-big
this one isn't updating for some reason

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 6, 2018

Member

This looks good, I'd like one more thing before we merge, which is in the code that's going to call mutating and validating webhooks: until we fix the API, I'd like that to check for dry-run-ness immediately before calling and fail instead of calling if the request is dry-run.

With that change, I think this PR will get us to the "safe but not feature complete" stage.

Member

lavalamp commented Aug 6, 2018

This looks good, I'd like one more thing before we merge, which is in the code that's going to call mutating and validating webhooks: until we fix the API, I'd like that to check for dry-run-ness immediately before calling and fail instead of calling if the request is dry-run.

With that change, I think this PR will get us to the "safe but not feature complete" stage.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 6, 2018

Member

Oh, I guess if you already got an LGTM that can come in a follow-up.

Member

lavalamp commented Aug 6, 2018

Oh, I guess if you already got an LGTM that can come in a follow-up.

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

@lavalamp I agree with the idea, but actually I think it will still be safe how it is, because right now the dry-run requests are all unconditionally rejected before they even get to this part of the code.

Something like:

if isDryRun(req.URL) {
scope.err(errors.NewBadRequest("dryRun is not supported yet"), w, req)
return
}

is already in all the handlers.

Before we allow dry run requests through at all, something like this commit will be have to be added

Contributor

jennybuckley commented Aug 6, 2018

@lavalamp I agree with the idea, but actually I think it will still be safe how it is, because right now the dry-run requests are all unconditionally rejected before they even get to this part of the code.

Something like:

if isDryRun(req.URL) {
scope.err(errors.NewBadRequest("dryRun is not supported yet"), w, req)
return
}

is already in all the handlers.

Before we allow dry run requests through at all, something like this commit will be have to be added

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 6, 2018

Member

Right--my thinking is that it would then be safe to swap that out for a feature flag gate instead of an unconditional fail.

Member

lavalamp commented Aug 6, 2018

Right--my thinking is that it would then be safe to swap that out for a feature flag gate instead of an unconditional fail.

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley

jennybuckley Aug 6, 2018

Contributor

@lavalamp ah, I see. should I add this commit? or do it in a separate PR?

Contributor

jennybuckley commented Aug 6, 2018

@lavalamp ah, I see. should I add this commit? or do it in a separate PR?

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 6, 2018

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Aug 6, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 6, 2018

Member
Member

lavalamp commented Aug 6, 2018

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 7, 2018

Contributor

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Contributor

k8s-merge-robot commented Aug 7, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 6fe7f9f into kubernetes:master Aug 7, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

chakri-nelluri pushed a commit to chakri-nelluri/kubernetes that referenced this pull request Aug 7, 2018

Merge pull request #67085 from jennybuckley/dry-run-admission-2
Automatic merge from submit-queue (batch tested with PRs 67085, 66559, 67089). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Block dry-run if a webhook would be called

**What this PR does / why we need it**:
Follow up to kubernetes#66391
Suggested in kubernetes#66391 (comment)

Makes dry-run safe in case kubernetes#66936 takes a long time to merge

**Release note**:

```release-note
NONE
```

/sig api-machinery
cc @lavalamp

k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Aug 7, 2018

Merge pull request #67085 from jennybuckley/dry-run-admission-2
Automatic merge from submit-queue (batch tested with PRs 67085, 66559, 67089). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Block dry-run if a webhook would be called

**What this PR does / why we need it**:
Follow up to kubernetes/kubernetes#66391
Suggested in kubernetes/kubernetes#66391 (comment)

Makes dry-run safe in case kubernetes/kubernetes#66936 takes a long time to merge

**Release note**:

```release-note
NONE
```

/sig api-machinery
cc @lavalamp

Kubernetes-commit: 47878f2bd1642145e311d2336e3103c6edcd50de

sttts pushed a commit to sttts/apiserver that referenced this pull request Aug 8, 2018

Merge pull request #67085 from jennybuckley/dry-run-admission-2
Automatic merge from submit-queue (batch tested with PRs 67085, 66559, 67089). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Block dry-run if a webhook would be called

**What this PR does / why we need it**:
Follow up to kubernetes/kubernetes#66391
Suggested in kubernetes/kubernetes#66391 (comment)

Makes dry-run safe in case kubernetes/kubernetes#66936 takes a long time to merge

**Release note**:

```release-note
NONE
```

/sig api-machinery
cc @lavalamp

Kubernetes-commit: 47878f2bd1642145e311d2336e3103c6edcd50de

k8s-merge-robot added a commit that referenced this pull request Aug 23, 2018

Merge pull request #66936 from jennybuckley/dry-run-webhooks
Automatic merge from submit-queue (batch tested with PRs 67576, 66936). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Support dry run in admission webhooks

**What this PR does / why we need it**:
Follow up to #66391
- [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview```
- [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration```
- [x] add dry run support to (Valid|Mut)atingAdmissionWebhook

Includes all the api-changes outlined by kubernetes/community#2387

/sig api-machinery

**Release note**:
```release-note
To address the possibility dry-run requests overwhelming admission webhooks that rely on side effects and a reconciliation mechanism, a new field is being added to admissionregistration.k8s.io/v1beta1.ValidatingWebhookConfiguration and admissionregistration.k8s.io/v1beta1.MutatingWebhookConfiguration so that webhooks can explicitly register as having dry-run support. If a dry-run request is made on a resource that triggers a non dry-run supporting webhook, the request will be completely rejected, with "400: Bad Request". Additionally, a new field is being added to the admission.k8s.io/v1beta1.AdmissionReview API object, exposing to webhooks whether or not the request being reviewed is a dry-run.
```

k8s-publishing-bot added a commit to kubernetes/api that referenced this pull request Aug 23, 2018

Merge pull request #66936 from jennybuckley/dry-run-webhooks
Automatic merge from submit-queue (batch tested with PRs 67576, 66936). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Support dry run in admission webhooks

**What this PR does / why we need it**:
Follow up to kubernetes/kubernetes#66391
- [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview```
- [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration```
- [x] add dry run support to (Valid|Mut)atingAdmissionWebhook

Includes all the api-changes outlined by kubernetes/community#2387

/sig api-machinery

**Release note**:
```release-note
To address the possibility dry-run requests overwhelming admission webhooks that rely on side effects and a reconciliation mechanism, a new field is being added to admissionregistration.k8s.io/v1beta1.ValidatingWebhookConfiguration and admissionregistration.k8s.io/v1beta1.MutatingWebhookConfiguration so that webhooks can explicitly register as having dry-run support. If a dry-run request is made on a resource that triggers a non dry-run supporting webhook, the request will be completely rejected, with "400: Bad Request". Additionally, a new field is being added to the admission.k8s.io/v1beta1.AdmissionReview API object, exposing to webhooks whether or not the request being reviewed is a dry-run.
```

Kubernetes-commit: 5a16163c87fe2a90916a51b52771a668bcaf2a0d

k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Aug 23, 2018

Merge pull request #66936 from jennybuckley/dry-run-webhooks
Automatic merge from submit-queue (batch tested with PRs 67576, 66936). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Support dry run in admission webhooks

**What this PR does / why we need it**:
Follow up to kubernetes/kubernetes#66391
- [x] add DryRun to ```admission.k8s.io/v1beta1.AdmissionReview```
- [x] add DryRunnable to ```admissionregistration.k8s.io/v1beta1.(Valid|Mut)atingWebhookConfiguration```
- [x] add dry run support to (Valid|Mut)atingAdmissionWebhook

Includes all the api-changes outlined by kubernetes/community#2387

/sig api-machinery

**Release note**:
```release-note
To address the possibility dry-run requests overwhelming admission webhooks that rely on side effects and a reconciliation mechanism, a new field is being added to admissionregistration.k8s.io/v1beta1.ValidatingWebhookConfiguration and admissionregistration.k8s.io/v1beta1.MutatingWebhookConfiguration so that webhooks can explicitly register as having dry-run support. If a dry-run request is made on a resource that triggers a non dry-run supporting webhook, the request will be completely rejected, with "400: Bad Request". Additionally, a new field is being added to the admission.k8s.io/v1beta1.AdmissionReview API object, exposing to webhooks whether or not the request being reviewed is a dry-run.
```

Kubernetes-commit: 5a16163c87fe2a90916a51b52771a668bcaf2a0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment