Skip to content

Fixed registration of the BanFlunder admission plugin #68417

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
Merged
merged 1 commit into from
Oct 3, 2018
Merged

Fixed registration of the BanFlunder admission plugin #68417

merged 1 commit into from
Oct 3, 2018

Conversation

MikeSpreitzer
Copy link
Member

@MikeSpreitzer MikeSpreitzer commented Sep 7, 2018 •
edited
Loading

What this PR does / why we need it:
This PR advances some mutations in k8s.io/sample-apiserver/pkg/cmd/server/start.go from Config() to Complete() so that they happen before Validate(). Without this fix, the BanFlunder admission plugin can not be used because it is not yet registered when Validate() happens.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #68395

Special notes for your reviewer:
This fix reveals one other problem (which I opened as #68418): the instructions for standalone testing do not work when the BanFlunder admission plugin is enabled, its informer gets 404s. However, testing of a similarly patched version of another extension server in its real context does not suffer those 404s.

Release note:

Fixes the sample-apiserver so that its BanFlunder admission plugin can be used.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 7, 2018
@MikeSpreitzer MikeSpreitzer mentioned this pull request Sep 7, 2018
Closed
@MikeSpreitzer MikeSpreitzer changed the title Fixed registration of the BanFlunder AdmissionController Fixed registration of the BanFlunder admission plugin Sep 7, 2018
@MikeSpreitzer MikeSpreitzer force-pushed the sample-apiserver-admission-fixes branch from e7c5675 to a5f9c40 Compare September 7, 2018 19:11
@MikeSpreitzer
Copy link
Member Author

/retest

@roycaihw
Copy link
Member

/assign @cheftako @jennybuckley

liggitt
liggitt reviewed Sep 14, 2018
View reviewed changes
staging/src/k8s.io/sample-apiserver/pkg/admission/plugin/banflunder/admission.go Outdated
func Register(plugins *admission.Plugins) {
plugins.Register("BanFlunder", func(config io.Reader) (admission.Interface, error) {
func Register(options *serveroptions.AdmissionOptions) {
options.RecommendedPluginOrder = append(options.RecommendedPluginOrder, "BanFlunder")
Copy link
Member

Choose a reason for hiding this comment

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

Plugins appending themselves doesn't seem right… they don't know about other plugins, or what order makes sense, right?

Copy link
Member

Choose a reason for hiding this comment

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

+1, This seems like something which should be done in k8s.io/sample-apiserver/pkg/cmd/server/start.go.

liggitt
liggitt reviewed Sep 14, 2018
View reviewed changes
staging/src/k8s.io/sample-apiserver/pkg/cmd/server/start.go Outdated
// register admission plugins
banflunder.Register(o.RecommendedOptions.Admission.Plugins)
banflunder.Register(o.RecommendedOptions.Admission)

// TODO have a "real" external address
if err := o.RecommendedOptions.SecureServing.MaybeDefaultWithSelfSignedCerts("localhost", nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@sttts does this call have the info it needs in Complete()?

Copy link
Member

Choose a reason for hiding this comment

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

If we fix plugin append I think this can go back to being in Config.

cheftako
cheftako reviewed Sep 14, 2018
View reviewed changes
Copy link
Member

@cheftako cheftako left a comment

Choose a reason for hiding this comment

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

Please consider fixing the issues @liggitt brought up.

staging/src/k8s.io/sample-apiserver/pkg/admission/plugin/banflunder/admission.go Outdated
func Register(plugins *admission.Plugins) {
plugins.Register("BanFlunder", func(config io.Reader) (admission.Interface, error) {
func Register(options *serveroptions.AdmissionOptions) {
options.RecommendedPluginOrder = append(options.RecommendedPluginOrder, "BanFlunder")
Copy link
Member

Choose a reason for hiding this comment

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

+1, This seems like something which should be done in k8s.io/sample-apiserver/pkg/cmd/server/start.go.

staging/src/k8s.io/sample-apiserver/pkg/cmd/server/start.go Outdated
// register admission plugins
banflunder.Register(o.RecommendedOptions.Admission.Plugins)
banflunder.Register(o.RecommendedOptions.Admission)

// TODO have a "real" external address
if err := o.RecommendedOptions.SecureServing.MaybeDefaultWithSelfSignedCerts("localhost", nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

If we fix plugin append I think this can go back to being in Config.

@MikeSpreitzer
Copy link
Member Author

Thanks. I will look into that.

@MikeSpreitzer MikeSpreitzer force-pushed the sample-apiserver-admission-fixes branch from a5f9c40 to ace4f05 Compare September 15, 2018 02:19
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 15, 2018
@MikeSpreitzer MikeSpreitzer force-pushed the sample-apiserver-admission-fixes branch from ace4f05 to a5a8885 Compare September 15, 2018 03:12
@MikeSpreitzer
Copy link
Member Author

/retest

@MikeSpreitzer
Copy link
Member Author

@liggitt, @cheftako: I revised as suggested. PTAL.

@liggitt
Copy link
Member

liggitt commented Oct 3, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, MikeSpreitzer

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2018
@k8s-ci-robot k8s-ci-robot merged commit 34bf116 into kubernetes:master Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheftako cheftako cheftako left review comments

@liggitt liggitt liggitt left review comments

@deads2k deads2k Awaiting requested review from deads2k

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@cheftako cheftako

@liggitt liggitt

@jennybuckley jennybuckley

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sample apiserver does not allow actual use of its admission plugin
6 participants
@MikeSpreitzer @roycaihw @liggitt @k8s-ci-robot @cheftako @jennybuckley