-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Fix decoding of admission config file #58439
Conversation
c489a4d
to
34328ea
Compare
cc @kubernetes/sig-api-machinery-pr-reviews |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt Associated issue: #58426 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
/retest |
1 similar comment
/retest |
Automatic merge from submit-queue (batch tested with PRs 57868, 58284, 56370, 58400, 58439). If you want to cherry-pick this change to another branch, please follow the instructions here. |
@@ -125,7 +125,7 @@ func (a *AdmissionOptions) ApplyTo( | |||
pluginNames = a.enabledPluginNames() | |||
} | |||
|
|||
pluginsConfigProvider, err := admission.ReadAdmissionConfiguration(pluginNames, a.ConfigFile, scheme) | |||
pluginsConfigProvider, err := admission.ReadAdmissionConfiguration(pluginNames, a.ConfigFile, configScheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. Which brings us back to the observation that the testing of all of this is too weak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzxuzhonghu if you have capacity. improving the test coverage here would be awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to.
@liggitt we need a cherry-pick in origin, if you haven't done so yet. |
Automatic merge from submit-queue. Cherrypick of #58439 #56408: Fix decoding of admission config file Manual pick of #58439 and #56408 into release-1.9 ```release-note kube-apiserver: fixes loading of `--admission-control-config-file` containing AdmissionConfiguration apiserver.k8s.io/v1alpha1 config object ```
Fixes #58426
1a552bb#diff-eb9532eb476083e1ab31da9dd6f83eceR41 attempted to use a locally constructed scheme, but the name
scheme
was shadowed by a function arg.Attempts to run the apiserver with a structured plugin config file would fail to decode (since the passed scheme didn't know about the AdmissionConfiguration type), then fall back to treating the file as a legacy config, and silently continue without correct config