Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix scale and rollback subresources with admission webhooks, add integration tests #76849

Merged
merged 5 commits into from
Apr 22, 2019
Merged

Fix scale and rollback subresources with admission webhooks, add integration tests #76849

merged 5 commits into from
Apr 22, 2019

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Apr 19, 2019 •
edited

What type of PR is this?
/kind feature

What this PR does / why we need it:

  • Fixes scale subresources when admission webhooks are present
  • Fixes rollback subresources when admission webhooks are present
  • Begins complete test coverage of validating and mutating admission webhooks (exempts some resources/subresources/verbs that need custom test implementations)

Which issue(s) this PR fixes:
Fixes #76649
Fixes #76107
Fixes #67221

Does this PR introduce a user-facing change?:

Admission webhooks are now properly called for `scale` and `deployments/rollback` subresources

/cc @caesarxuchao
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 19, 2019
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 19, 2019
@liggitt
Copy link
Member Author

liggitt commented Apr 19, 2019

/cc @sbezverk

@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 19, 2019
@liggitt liggitt force-pushed the crd_webhook_integration_tests branch from 7a7c18c to bc0a0d3 Compare April 19, 2019 22:27
@caesarxuchao
Copy link
Member

/assign

@liggitt
Copy link
Member Author

liggitt commented Apr 19, 2019 •
edited

oh, I need to tag it as an integration test for bazel, and maybe put it in a different package, since it uses the existing etcd setup. Passes locally when run standalone in ~15 seconds

@roycaihw
Copy link
Member

/cc

caesarxuchao
caesarxuchao reviewed Apr 19, 2019
View reviewed changes
Copy link
Member

@caesarxuchao caesarxuchao left a comment

Choose a reason for hiding this comment

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

Some initial thoughts.

test/integration/apimachinery/admission_test.go Outdated Show resolved Hide resolved
test/integration/apimachinery/admission_test.go Outdated Show resolved Hide resolved
test/integration/apimachinery/admission_test.go Outdated Show resolved Hide resolved
test/integration/apimachinery/admission_test.go Outdated Show resolved Hide resolved
test/integration/framework/etcd.go Outdated Show resolved Hide resolved
test/integration/apimachinery/admission_test.go Outdated Show resolved Hide resolved
@liggitt liggitt force-pushed the crd_webhook_integration_tests branch from bc0a0d3 to f09532a Compare April 20, 2019 03:16
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 20, 2019
@liggitt liggitt force-pushed the crd_webhook_integration_tests branch 2 times, most recently from 0b49198 to ea28d56 Compare April 20, 2019 03:30
@liggitt liggitt mentioned this pull request Apr 20, 2019
Closed
@liggitt liggitt force-pushed the crd_webhook_integration_tests branch from ea28d56 to ad2cf9b Compare April 20, 2019 06:17
@liggitt liggitt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 20, 2019
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 20, 2019
@k8s-ci-robot k8s-ci-robot removed the release-note-none Denotes a PR that doesn't merit a release note. label Apr 20, 2019
@k8s-ci-robot
Copy link
Contributor

[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

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

@liggitt liggitt force-pushed the crd_webhook_integration_tests branch from ad2cf9b to 4f36558 Compare April 20, 2019 06:21
@liggitt liggitt changed the title Add integration tests covering admission webhooks Fix scale and rollback subresources with admission webhooks, add integration tests Apr 20, 2019
@liggitt liggitt force-pushed the crd_webhook_integration_tests branch 2 times, most recently from ab3102e to e2b3dfa Compare April 20, 2019 06:54
@liggitt
Copy link
Member Author

liggitt commented Apr 20, 2019

/retest

@liggitt liggitt force-pushed the crd_webhook_integration_tests branch from e2b3dfa to eb0ccf4 Compare April 20, 2019 07:04
@liggitt liggitt force-pushed the crd_webhook_integration_tests branch from eb0ccf4 to 69042da Compare April 20, 2019 07:33
@liggitt
Copy link
Member Author

liggitt commented Apr 20, 2019

Comments addressed, ready for re-review

caesarxuchao
caesarxuchao reviewed Apr 21, 2019
View reviewed changes
Copy link
Member

@caesarxuchao caesarxuchao left a comment

Choose a reason for hiding this comment

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

Asked for a test to test the "custeromResource/scale" endpoint. Otherwise lgtm.

test/integration/apiserver/admissionwebhook/admission_test.go Show resolved Hide resolved
test/integration/apiserver/admissionwebhook/admission_test.go Show resolved Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go Show resolved Hide resolved
pkg/registry/apps/deployment/storage/storage.go Show resolved Hide resolved
@caesarxuchao
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit 716344f into kubernetes:master Apr 22, 2019
@liggitt liggitt mentioned this pull request Apr 22, 2019
Closed
5 tasks
@liggitt liggitt deleted the crd_webhook_integration_tests branch April 22, 2019 21:34
@liggitt liggitt mentioned this pull request Apr 23, 2019
Merged
This was referenced Aug 2, 2019
Closed
Closed
@liggitt liggitt added this to the v1.15 milestone Aug 6, 2019
@liggitt liggitt added this to Complete in Admission Webhooks Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao caesarxuchao left review comments

@sbezverk sbezverk Awaiting requested review from sbezverk

@roycaihw roycaihw Awaiting requested review from roycaihw

Assignees

@caesarxuchao caesarxuchao

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Admission Webhooks
Complete
Milestone
v1.15
4 participants
@liggitt @caesarxuchao @roycaihw @k8s-ci-robot