Skip to content

Limit v1 webhooks to None and NoneOnDryRun side effects classes#81046

Merged
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
liggitt:admission-side-effects
Aug 9, 2019
Merged

Limit v1 webhooks to None and NoneOnDryRun side effects classes#81046
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
liggitt:admission-side-effects

Conversation

@liggitt
Copy link
Copy Markdown
Member

@liggitt liggitt commented Aug 6, 2019 •
edited
Loading

What type of PR is this?
/kind api-change
/kind feature

What this PR does / why we need it:
Requires admission webhooks registered via v1 to not have side effects (only None and NoneOnDryRun are allowed as side effects classes)

Which issue(s) this PR fixes:
Fixes #68839

Does this PR introduce a user-facing change?:
(will add to v1 promotion PR release note in #79549)

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 6, 2019
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Aug 6, 2019

/cc @jpbetz @apelisse

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2019
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Aug 6, 2019

/sig api-machinery
/priority important-soon

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 6, 2019
@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Aug 6, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@liggitt liggitt force-pushed the admission-side-effects branch from 358641d to 8294313 Compare August 6, 2019 22:34
@liggitt liggitt removed request for piosz and pwittrock August 6, 2019 22:36
@liggitt liggitt force-pushed the admission-side-effects branch from 8294313 to 0a37724 Compare August 7, 2019 00:59
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 7, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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

The pull request process is described here

Details 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

apelisse
apelisse reviewed Aug 7, 2019
View reviewed changes
staging/src/k8s.io/api/admissionregistration/v1/types.go

// SideEffects states whether this webhookk has side effects.
// Acceptable values are: Unknown, None, Some, NoneOnDryRun
// SideEffects states whether this webhook has side effects.
Copy link
Copy Markdown
Member

@apelisse apelisse Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth clarifying what happens when not specified (can it, and should we default it)?

Copy link
Copy Markdown
Member Author

@liggitt liggitt Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it cannot be omitted in v1. it's required and not defaulted

Copy link
Copy Markdown
Member

@apelisse apelisse Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU, the distinction between None and NoneOnDryRun makes no difference, so I'm not really sure why we force people to specify a field that adds no value?

Copy link
Copy Markdown
Member Author

@liggitt liggitt Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making it explicit in v1 ensures all manifests ack that no side effects are allowed. the distinction between None and NoneOnDryRun doesn't matter for the dispatcher, but allows expressing that there is a difference in behavior specifically for dry run

@fedebongio
Copy link
Copy Markdown
Contributor

fedebongio commented Aug 8, 2019

/assign @apelisse @jpbetz

@apelisse
Copy link
Copy Markdown
Member

apelisse commented Aug 9, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2019
@k8s-ci-robot k8s-ci-robot merged commit 31bade3 into kubernetes:master Aug 9, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 9, 2019
@liggitt liggitt deleted the admission-side-effects branch August 12, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apelisse apelisse apelisse left review comments

@jpbetz jpbetz Awaiting requested review from jpbetz

Assignees

@jpbetz jpbetz

@apelisse apelisse

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/admission-control area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v1.16

Development

Successfully merging this pull request may close these issues.

Change default for SideEffect in admissionregistration Webhook v1

6 participants

@liggitt @fejta-bot @k8s-ci-robot @fedebongio @apelisse @jpbetz