use the upstream admission plugin construction for most plugins #16682
Conversation
openshift-ci-robot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
openshift-merge-robot
added
approved
deads2k
force-pushed
the
server-47-collapse-init
branch
from
d73bec4
to
b169ef9
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/retest |
1 similar comment
|
/retest |
| if openshiftConfig.Configuration != nil { | ||
| configBytes, err := runtime.Encode(Codec, openshiftConfig.Configuration) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
is this intended, when one fails, fail all? why not continue with others?
There was a problem hiding this comment.
is this intended, when one fails, fail all? why not continue with others?
We don't ever want to construct a partial admission chain since that could leave us unprotected by the admission validation we expect. Missing config could produce that problem.
There was a problem hiding this comment.
I agree completely. Though I wonder, for that if we should have a validation check (if we dont have one already) to make sure final admission plugin chain (that are going to be enabled) is secure instead of relying on this. That'd also help providing warning to admins in case they have not configured a secure chain.
There was a problem hiding this comment.
I agree completely. Though I wonder, for that if we should have a validation check (if we dont have one already) to make sure final admission plugin chain (that are going to be enabled) is secure instead of relying on this. That'd also help providing warning to admins in case they have not configured a secure chain.
We leave the final determination of on and off to the cluster-admin. If they intentionally make it insecure by disablign plugins (without producing errors in the configuration), then we allow them to do that.
| @@ -158,3 +160,31 @@ func IsAdmissionPluginActivated(reader io.Reader, defaultValue bool) (bool, erro | |||
|
|
|||
| return !activationConfig.Disable, nil | |||
| } | |||
|
|
|||
| func ConvertOpenshiftAdmissionConfigToKubeAdmissionConfig(in map[string]configapi.AdmissionPluginConfig) (*apiserver.AdmissionConfiguration, error) { | |||
There was a problem hiding this comment.
If you want to have it in helpers.go, wouldn't be better to have it only for one? and then call on multiple plugins where needed?
There was a problem hiding this comment.
If you want to have it in helpers.go, wouldn't be better to have it only for one? and then call on multiple plugins where needed?
Good question. Ordinarily I'd agree with you, but this actually matches a type in our server config here: https://github.com/openshift/origin/blob/master/pkg/cmd/server/api/types.go#L1504, so when we're done, it will operate directly on the config instance variable.
|
@deads2k lgtm. |
|
Automatic merge from submit-queue (batch tested with PRs 16545, 16684, 16643, 16459, 16682). |
Automatic merge from submit-queue (batch tested with PRs 16545, 16684, 16643, 16459, 16682). use the upstream admission plugin construction for most plugins We still have four special ones that really need to take config. That may or may not happen in 3.7 @aveshagarwal you can start to see the pay-off here. This has exposed some debt-y config problems where the controller configuration was being built from the admission config. /assign soltysh /assign mfojtik @mfojtik This collapses our admission path and eliminates drift for 3.7. We probably need to fix the build controller config problem (spoke with @bparees already) for 3.7.
We still have four special ones that really need to take config. That may or may not happen in 3.7
@aveshagarwal you can start to see the pay-off here. This has exposed some debt-y config problems where the controller configuration was being built from the admission config.
/assign soltysh
/assign mfojtik
@mfojtik This collapses our admission path and eliminates drift for 3.7. We probably need to fix the build controller config problem (spoke with @bparees already) for 3.7.