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.

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 setting admission plugins on local-up-cluster.sh #69243

Conversation

jfchevrette
Copy link
Contributor

What this PR does / why we need it:

hack/local-up-cluster.sh currently do not honor setting ENABLE_ADMISSION_PLUGINS because the list is overriden later on in the script before the apiserver is started.

This PR moves the list higher up as a default list of admission plugins and then allows overriding the defaults via the expected ENV variable when starting the cluster.

Which issue(s) this PR fixes
No issue created. Should I create one?

Special notes for your reviewer:
I have not found tests for this script. I did run it and was able to manually validate that the list of admission plugin is either set to default or to those I chose via the environment variable.

This is a first contribution. I will appreciate any comments on how I can improve future contributions.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2018
@dims
Copy link
Member

dims commented Sep 29, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 29, 2018
@jfchevrette
Copy link
Contributor Author

/assign @dims
PTAL

ENABLE_ADMISSION_PLUGINS=${ENABLE_ADMISSION_PLUGINS:-""}
# Default list of admission Controllers to invoke prior to persisting objects in cluster
# The order defined here does not matter.
DEFAULT_ENABLE_ADMISSION_PLUGINS="LimitRanger,ServiceAccount,DefaultStorageClass,DefaultTolerationSeconds,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota,StorageObjectInUseProtection"
Copy link
Member

Choose a reason for hiding this comment

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

the default on plugins are defined in:

defaultOnPlugins := sets.NewString(
lifecycle.PluginName, //NamespaceLifecycle
limitranger.PluginName, //LimitRanger
serviceaccount.PluginName, //ServiceAccount
setdefault.PluginName, //DefaultStorageClass
resize.PluginName, //PersistentVolumeClaimResize
defaulttolerationseconds.PluginName, //DefaultTolerationSeconds
mutatingwebhook.PluginName, //MutatingAdmissionWebhook
validatingwebhook.PluginName, //ValidatingAdmissionWebhook
resourcequota.PluginName, //ResourceQuota
)

so we can trim the list, amiright?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only plugin that is in the list but not in the default on plugins list is StorageObjectInUseProtection which according to the cli feature gates docs is enabled by default.

Regardless of the above, as a user using local-up-cluster.sh I would expect that the default plugins are loaded unless I specify additional plugins using ENABLE_ADMISSION_PLUGINS (or disable plugins using DISABLE_ADMISSION_PLUGINS)

WDYT?

# Admission Controllers to invoke prior to persisting objects in cluster
#
# The order defined here dose not matter.
ENABLE_ADMISSION_PLUGINS=LimitRanger,ServiceAccount${security_admission},DefaultStorageClass,DefaultTolerationSeconds,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota,StorageObjectInUseProtection
Copy link
Member

Choose a reason for hiding this comment

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

what about removing L106 and change this line to

ENABLE_ADMISSION_PLUGINS=${ENABLE_ADMISSION_PLUGINS:-""}${security_admission}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the additional DEFAULT_ variable but left (and updated) the list to match the recommended defaults from the docs

It is my understanding that if --enable-admission-plugins is passed with an empty value/list, no plugins will be loaded.

Copy link
Member

@yue9944882 yue9944882 left a comment

Choose a reason for hiding this comment

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

thx so much for fixing this @jfchevrette ! if memory serves, i hit the exact same problem before. 🎉

@yue9944882
Copy link
Member

yue9944882 commented Sep 29, 2018

/assign
/sig cluster-lifecycle
/sig api-machinery
/kind bug

@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 29, 2018
@jfchevrette
Copy link
Contributor Author

@yue9944882 I just found out that kube-apiserver / hyperkube can return the list of default plugins. We can use that to generate the list dynamically and that would avoid the need to keep a static list up-to-date.

hyperkube apiserver -h | grep -E '^\s+\-\-enable\-admission\-plugins' | sed  's/.*(\(.*\)).*/\1/' | tr -d ' '

@dims
Copy link
Member

dims commented Oct 1, 2018

/test pull-kubernetes-e2e-kops-aws

@jennybuckley
Copy link

/assign

@dims
Copy link
Member

dims commented Oct 2, 2018

/approve
/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 2, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jfchevrette

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 2, 2018
@k8s-ci-robot k8s-ci-robot merged commit a94755c into kubernetes:master Oct 3, 2018
@jfchevrette jfchevrette deleted the local_cluster_default_admission_plugins branch March 12, 2019 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants