Allow enabling admission plugins (with configurations) via DefaultAdmissionConfig and stop passing DefaultAdmissionConfig to admission plugins. #16505

aveshagarwal · 2017-09-22T13:40:56Z

In OpenShift, admission plugins can be enabled and disabled by using DefaultAdmissionConfig.
However enabling of admission plugins with configurations (like ResourceQuota, PodTolerationRestriction etc) via DefaultAdmissionConfig as follows:

admissionConfig:
  pluginConfig:
    PodTolerationRestriction: 
      configuration:
        kind: DefaultAdmissionConfig
        apiVersion: v1
        disable: false

fails with following error:
F0922 09:36:24.937280 5768 start_master.go:115] Couldn't init admission plugin "PodTolerationRestriction": no kind "DefaultAdmissionConfig" is registered for version "v1"

The reason for this error is that DefaultAdmissionConfig is being passed to admission plugins which dont understand it, and so the error. This error does not happen with admission plugins that do not accept any configurations.

Though admission plugins with configurations can also be enabled by passing their own configurations, it might be confusing for users why some plugins can be enabled by DefaultAdmissionConfig and some not.

This PR addresses the above issue.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1492999

@sjenning @deads2k

openshift-merge-robot · 2017-09-22T13:41:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aveshagarwal
We suggest the following additional approver: mfojtik

Assign the PR to them by writing /assign @mfojtik in a comment when ready.

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

Needs approval from an approver in each of these OWNERS Files:

pkg/cmd/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

aveshagarwal · 2017-09-22T16:02:53Z

/test cmd
/test end_to_end

deads2k · 2017-09-22T16:29:22Z

pkg/cmd/server/api/latest/helpers.go

-func IsAdmissionPluginActivated(reader io.Reader, defaultValue bool) (bool, error) {
+// It also returns true if configapi.DefaultAdmissionConfig is passed or false if the admission plugin's
+// configuration is passed to avoid passing configapi.DefaultAdmissionConfig to admission plugins.
+func IsAdmissionPluginActivated(reader io.Reader, defaultValue bool) (bool, bool, error) {


What about making this an enumerated string?

EnabledWithDefaultAdmissionConfig

EnabledWithCustomConfig

Disabled

right now you have four possible cases and one of them isn't useful.

deads2k · 2017-09-22T16:30:11Z

pkg/cmd/server/api/latest/helpers.go

+
+// SplitStream reads the stream bytes and constructs two copies of it.
+// This is copied from kubernetes
+func SplitStream(config io.Reader) (io.Reader, io.Reader, error) {


move to point of use and make private

deads2k · 2017-09-22T16:31:02Z

pkg/cmd/server/origin/admission/register.go

-}
-
-func IsAdmissionPluginActivated(name string, config io.Reader) bool {
+func IsAdmissionPluginActivated(name string, config io.Reader) (bool, bool) {


Thsi looks really similar to the upstream method now that we've pushed changes upstream. Any chance we can do a more invasive refactor to figure out what we want?

deads2k · 2017-09-22T16:31:48Z

pkg/cmd/server/origin/admission/register.go

-}
-
-func IsAdmissionPluginActivated(name string, config io.Reader) bool {
+func IsAdmissionPluginActivated(name string, config io.Reader) (bool, bool) {


godoc, the name looks really similar to below. I'd like to know the diffference.

deads2k · 2017-09-22T16:33:20Z

pkg/cmd/server/origin/master_config.go

 				return nil, err
 			}
+
+			if enabled {


if !enabled {continue} to reduce nesting?

Why to send disabled plugin all the way down when what we get in return is just plugin as nil.

Why to send disabled plugin all the way down when what we get in return is just plugin as nil.

I'm trying to see why its important to skip this and return in the next if instead of eliminating this nesting in keeping with golang style.

sjenning · 2017-09-22T16:31:41Z

pkg/cmd/server/origin/admission/register.go

+		return false, nil, err
+	}
+
+	enabled, isDefault := IsAdmissionPluginActivated(name, input)


eating the error here

this one in register.go does not return error.

aveshagarwal · 2017-09-22T17:34:31Z

@deads2k @sjenning thanks, I will look into other comments and update it accordingly.

deads2k · 2017-09-22T17:36:18Z

pkg/cmd/server/origin/master_config.go

@@ -675,11 +675,20 @@ func newAdmissionChain(pluginNames []string, admissionConfigFilename string, plu
 				return nil, err
 			}

-			plugin, err = admissionregistry.OriginAdmissionPlugins.InitPlugin(pluginName, pluginConfigReader, admissionInitializer)
+			plugin = nil


this looks extraneous

deads2k · 2017-09-22T17:37:28Z

pkg/cmd/server/origin/master_config.go

@@ -675,11 +675,20 @@ func newAdmissionChain(pluginNames []string, admissionConfigFilename string, plu
 				return nil, err
 			}

-			plugin, err = admissionregistry.OriginAdmissionPlugins.InitPlugin(pluginName, pluginConfigReader, admissionInitializer)
+			plugin = nil
+			enabled, pluginConfigReaderCopy, err := admissionregistry.IsAdmissionPluginEnabled(pluginName, pluginConfigReader)


Why isn't this the upstream admission.PluginEnabledFn ?

How would you avoid passing DefaultAdmissionConfig to admission plugins if you did that? Also by doing this here gives us 2 advantages as I understand:

we do not need to override upstream admission.PluginEnabledFn

We do not need to send disabled plugins all the way down as I said above.

We do not need to send disabled plugins all the way down as I said above.

What do you mean by "all the way down"?

we do not need to override upstream admission.PluginEnabledFn

We created that for exactly this purpose.

What do you mean by "all the way down"?

I mean to avoid InitPlugin->getPlugin-> (splitstream, PluginEnabledFn) inside kubernetes.

We created that for exactly this purpose.

I agree but how would you avoid DefaultAdmissionConfig passing to admission plugins if we used that. We need a way to process DefaultAdmissionConfig and a plugin's own config differently.

sjenning · 2017-09-29T18:26:38Z

@aveshagarwal can you update this?

aveshagarwal · 2017-09-29T19:34:26Z

@sjenning will update soon.

…issionConfig and stop passing DefaultAdmissionConfig to admission plugins.

aveshagarwal · 2017-10-02T14:23:51Z

@deads2k @sjenning updated based on feedback, PTAL.

openshift-ci-robot · 2017-10-02T15:17:26Z

@aveshagarwal: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/cmd	`914cbef`	link	`/test cmd`
ci/openshift-jenkins/verify	`914cbef`	link	`/test verify`
ci/openshift-jenkins/integration	`914cbef`	link	`/test integration`
ci/openshift-jenkins/extended_networking_minimal	`914cbef`	link	`/test extended_networking_minimal`
ci/openshift-jenkins/extended_conformance_gce	`914cbef`	link	`/test extended_conformance_gce`
ci/openshift-jenkins/end_to_end	`914cbef`	link	`/test end_to_end`
ci/openshift-jenkins/extended_conformance_install_update	`914cbef`	link	`/test extended_conformance_install_update`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

deads2k · 2017-10-02T16:07:53Z

We eventually want to call https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/options/admission.go#L78 . I think that #16639 aligns more cleanly. Compare it with this?

Automatic merge from submit-queue (batch tested with PRs 16657, 16607, 16647, 16639, 16655). filter out 'turn this on' config structs for admission Alternative to #16505 to allow our enablement of config. I think this aligns more closely with a goal of calling the "normal" https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/options/admission.go#L78 path.

Automatic merge from submit-queue (batch tested with PRs 16657, 16607, 16647, 16639, 16655). filter out 'turn this on' config structs for admission Alternative to openshift/origin#16505 to allow our enablement of config. I think this aligns more closely with a goal of calling the "normal" https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/options/admission.go#L78 path. Origin-commit: 105055ed88d0029dfb69d43eefedae45005044b7

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 22, 2017

openshift-merge-robot assigned simo5 and csrwng Sep 22, 2017

deads2k reviewed Sep 22, 2017

View reviewed changes

sjenning reviewed Sep 22, 2017

View reviewed changes

aveshagarwal assigned deads2k and sjenning and unassigned csrwng and simo5 Sep 22, 2017

deads2k reviewed Sep 22, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2017

Allow enabling admission plugins (with configurations) via DefaultAdm…

914cbef

…issionConfig and stop passing DefaultAdmissionConfig to admission plugins.

aveshagarwal force-pushed the master-admission-issues branch from 8054912 to 914cbef Compare October 2, 2017 14:22

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2017

deads2k mentioned this pull request Oct 2, 2017

filter out 'turn this on' config structs for admission #16639

Merged

aveshagarwal closed this Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow enabling admission plugins (with configurations) via DefaultAdmissionConfig and stop passing DefaultAdmissionConfig to admission plugins. #16505

Allow enabling admission plugins (with configurations) via DefaultAdmissionConfig and stop passing DefaultAdmissionConfig to admission plugins. #16505

aveshagarwal commented Sep 22, 2017

openshift-merge-robot commented Sep 22, 2017

aveshagarwal commented Sep 22, 2017 •

edited

Loading

deads2k Sep 22, 2017

deads2k Sep 22, 2017

deads2k Sep 22, 2017

deads2k Sep 22, 2017

deads2k Sep 22, 2017

aveshagarwal Sep 22, 2017

deads2k Sep 22, 2017

sjenning Sep 22, 2017

aveshagarwal Sep 22, 2017

aveshagarwal commented Sep 22, 2017

deads2k Sep 22, 2017

deads2k Sep 22, 2017

aveshagarwal Sep 22, 2017

deads2k Sep 22, 2017

aveshagarwal Sep 22, 2017

sjenning commented Sep 29, 2017

aveshagarwal commented Sep 29, 2017

aveshagarwal commented Oct 2, 2017

openshift-ci-robot commented Oct 2, 2017

deads2k commented Oct 2, 2017

Allow enabling admission plugins (with configurations) via DefaultAdmissionConfig and stop passing DefaultAdmissionConfig to admission plugins. #16505

Allow enabling admission plugins (with configurations) via DefaultAdmissionConfig and stop passing DefaultAdmissionConfig to admission plugins. #16505

Conversation

aveshagarwal commented Sep 22, 2017

openshift-merge-robot commented Sep 22, 2017

aveshagarwal commented Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aveshagarwal commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented Sep 29, 2017

aveshagarwal commented Sep 29, 2017

aveshagarwal commented Oct 2, 2017

openshift-ci-robot commented Oct 2, 2017

deads2k commented Oct 2, 2017

aveshagarwal commented Sep 22, 2017 •

edited

Loading