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

kubeadm: do not use --admission-control for the API server #64165

Merged
merged 1 commit into from May 29, 2018

Conversation

@neolit123
Copy link
Member

neolit123 commented May 22, 2018

What this PR does / why we need it:
The API server argument --admission-control is deprecated.
Use the following arguments instead:
--enable-admission-plugins=NodeRestriction
--disable-admission-plugins=PersistentVolumeLabel

Add comment that PersistentVolumeLabel should be removed at some
point in 1.11.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Updates kubernetes/kubeadm#840

Special notes for your reviewer:
NONE

Release note:

kubeadm: when starting the API server use the arguments --enable-admission-plugins and --disable-admission-plugins instead of the deprecated --admission-control.

@luxas
@kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm

@@ -154,7 +154,8 @@ func TestGetAPIServerCommand(t *testing.T) {
expected: []string{
"kube-apiserver",
"--insecure-port=0",
"--admission-control=NamespaceLifecycle,LimitRanger,ServiceAccount,DefaultStorageClass,DefaultTolerationSeconds,NodeRestriction,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota",
"--enable-admission-plugin=NodeRestriction",
"--disable-admission-plugin=PersistentVolumeLabel",

This comment has been minimized.

@timothysc

timothysc May 22, 2018

Member

If this is deprecated I don't know why we need to explicitly specify.

This comment has been minimized.

@neolit123

neolit123 May 22, 2018

Author Member

@luxas can comment further i guess, i was just following the guide here:
kubernetes/kubeadm#840

This comment has been minimized.

@luxas

luxas May 23, 2018

Member

It's still enabled by default which is a bug. Until that bug is fixed, let's leave it out here.

This comment has been minimized.

@timothysc

timothysc May 24, 2018

Member

I'm fine with defaulting off, but lets cross-link an issue.

This comment has been minimized.

@neolit123

neolit123 May 24, 2018

Author Member

should i cross link to this (i.e. adding a comment with link in the code)?:
kubernetes/kubeadm#840

This comment has been minimized.

@timothysc

timothysc May 25, 2018

Member

@neolit123 I'm thinking about the api-server deprecation default issue, should be in the main repo somewhere.

This comment has been minimized.

@neolit123

neolit123 May 25, 2018

Author Member

i can't seem to find it. only found the PR that deprecated admission-control: #58123

This comment has been minimized.

@luxas
@luxas

luxas approved these changes May 22, 2018

Copy link
Member

luxas left a comment

Thanks @neolit123! Can you use kubeadm with this patch to start up both a v1.10 cluster and a v1.11-beta.0 API server and post the final admission chain that was loaded (it's printed in the API server logs in the beginning), so we have a double-check that everything we want to be running actually is?
/approve

@@ -143,7 +143,8 @@ func getAPIServerCommand(cfg *kubeadmapi.MasterConfiguration) []string {
defaultArguments := map[string]string{
"advertise-address": cfg.API.AdvertiseAddress,
"insecure-port": "0",
"admission-control": defaultAdmissionControl,

This comment has been minimized.

@luxas

luxas May 22, 2018

Member

please remove the unused variable

This comment has been minimized.

@neolit123

neolit123 May 22, 2018

Author Member

will do.

edit: done

@luxas luxas self-assigned this May 22, 2018

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented May 22, 2018

Thanks @neolit123! Can you use kubeadm with this patch to start up both a v1.10 cluster and a v1.11-beta.0 API server and post the final admission chain that was loaded (it's printed in the API server logs in the beginning), so we have a double-check that everything we want to be running actually is?
/approve

ha!! as long as my control plane even starts. my setup is beyond broken ATM and it's not really kubeadm's fault. i can try...

@stealthybox

This comment has been minimized.

Copy link
Contributor

stealthybox commented May 22, 2018

@neolit123 if your personal lab is non-functional, this should serve the need:
https://github.com/kubernetes/kubeadm/tree/master/vagrant

just change the package versions in the Vagrantfile to 1.10.2 instead of 1.9.7 and then follow the guide for comparing different builds with the already installed kubeadm binary

@neolit123 neolit123 force-pushed the neolit123:admission-control branch from 3cad80a to c16ff56 May 22, 2018

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented May 22, 2018

yeah @stealthybox . we spoke about it at KubeCon.
i didn't had the time to try it, but i definitely need to find the time...

@neolit123 neolit123 force-pushed the neolit123:admission-control branch from c16ff56 to 876547f May 25, 2018

@@ -143,7 +141,9 @@ func getAPIServerCommand(cfg *kubeadmapi.MasterConfiguration) []string {
defaultArguments := map[string]string{
"advertise-address": cfg.API.AdvertiseAddress,
"insecure-port": "0",
"admission-control": defaultAdmissionControl,
// https://github.com/kubernetes/kubeadm/issues/840

This comment has been minimized.

@neolit123

neolit123 May 25, 2018

Author Member

^ added link to the issue here.

@neolit123 neolit123 force-pushed the neolit123:admission-control branch from 876547f to 077ba44 May 25, 2018

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented May 25, 2018

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-node-e2e

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented May 25, 2018

/test pull-kubernetes-node-e2e
/test pull-kubernetes-kubemark-e2e-gce-big

@luxas
Copy link
Member

luxas left a comment

Please fix these comments. After that this LGTM

"admission-control": defaultAdmissionControl,
"advertise-address": cfg.API.AdvertiseAddress,
"insecure-port": "0",
// https://github.com/kubernetes/kubernetes/pull/58123

This comment has been minimized.

@luxas

luxas May 26, 2018

Member

I'm not sure we should have this here...

"insecure-port": "0",
// https://github.com/kubernetes/kubernetes/pull/58123
"enable-admission-plugin": "NodeRestriction",
"disable-admission-plugin": "PersistentVolumeLabel",

This comment has been minimized.

@luxas

luxas May 26, 2018

Member

Add a TODO to remove this in kubeadm v1.11, as it's automatically disabled in v1.11, and reference #64326.
We can't skip it now as we support v1.10 clusters still.

"advertise-address": cfg.API.AdvertiseAddress,
"insecure-port": "0",
// https://github.com/kubernetes/kubernetes/pull/58123
"enable-admission-plugin": "NodeRestriction",

This comment has been minimized.

@luxas

luxas May 26, 2018

Member

These are actually called enable/disable-admission-plugins

@@ -154,7 +154,8 @@ func TestGetAPIServerCommand(t *testing.T) {
expected: []string{
"kube-apiserver",
"--insecure-port=0",
"--admission-control=NamespaceLifecycle,LimitRanger,ServiceAccount,DefaultStorageClass,DefaultTolerationSeconds,NodeRestriction,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota",
"--enable-admission-plugin=NodeRestriction",
"--disable-admission-plugin=PersistentVolumeLabel",

This comment has been minimized.

@luxas

@neolit123 neolit123 force-pushed the neolit123:admission-control branch from 077ba44 to c61004c May 26, 2018

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented May 26, 2018

updated.

  • fixed typo - missing s in argument keys
  • add TODO comment above PersistentVolumeLabel usage

added the same TODO note here and also made this PR not close the issue:
kubernetes/kubeadm#840

kubeadm: do not use --admission-control for the API server
The API server argument --admission-control is deprecated.
Use the following arguments instead:
  --enable-admission-plugins=NodeRestriction
  --disable-admission-plugins=PersistentVolumeLabel

Add comment that PersistentVolumeLabel should be removed at some
point in 1.11.

@neolit123 neolit123 force-pushed the neolit123:admission-control branch from c61004c to 8d84ef6 May 26, 2018

@luxas

luxas approved these changes May 28, 2018

Copy link
Member

luxas left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 28, 2018

@luxas

This comment has been minimized.

Copy link
Member

luxas commented May 28, 2018

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, neolit123

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

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented May 28, 2018

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-device-plugin-gpu

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 28, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 29, 2018

Automatic merge from submit-queue (batch tested with PRs 64308, 64367, 64165, 64274). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit c56e049 into kubernetes:master May 29, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation neolit123 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Jun 27, 2018

Using minikube to create a 1.11.0 cluster puts this in /etc/kubernetes/manifests/kube-apiserver.yaml :

spec:
  containers:
  - command:
    - kube-apiserver
    - --admission-control=Initializers,NamespaceLifecycle,LimitRanger,ServiceAccount,DefaultStorageClass,DefaultTolerationSeconds,NodeRestriction,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota
    <snip>
    - --disable-admission-plugins=PersistentVolumeLabel
    - --enable-admission-plugins=NodeRestriction

so apiserver fails to start because admission-control and enable-admission-plugins/disable-admission-plugins flags are mutually exclusive

$ minikube version

minikube version: v0.28.0

and inside the cluster VM:

$ kubeadm version 

kubeadm version: &version.Info{Major:"1", Minor:"11", GitVersion:"v1.11.0", GitCommit:"91e7b4fd31fcd3d5f436da26c980becec37ceefe", GitTreeState:"clean", BuildDate:"2018-06-27T20:14:41Z", GoVersion:"go1.10.2", Compiler:"gc", Platform:"linux/amd64"}

The enable/disable parameters are presumably coming because of this PR, so is there some place left that's still creating the default admission control parameter list?

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Jun 27, 2018

the --admission-control part should not be there. it's unclear to me what's adding it.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Jun 28, 2018

Ah, it's coming from the MasterConfiguration/apiServerExtraArgs in the config file passed to kubeadm. So this is probably minikube creating the file incorrectly. Will follow up there.

@neolit123

This comment has been minimized.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Jun 28, 2018

Yeah, just found it. I'll file a bug there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.