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.

Sign up for GitHub

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

Jump to bottom

kubeadm: Turn off insecure apiserver access on localhost:8080 #42066

Merged
merged 1 commit into from
Mar 1, 2017
Merged

kubeadm: Turn off insecure apiserver access on localhost:8080 #42066

merged 1 commit into from
Mar 1, 2017
+35 −22

Conversation

luxas
Copy link
Member

@luxas luxas commented Feb 24, 2017 •
edited
Loading

What this PR does / why we need it:

ref: kubernetes/kubeadm#181
depends on: #41897

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Insecure access to the API Server at localhost:8080 will be turned off in v1.6 when using kubeadm

@jbeda @liggitt @deads2k @pires @lukemarsden @mikedanese @errordeveloper

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 24, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@luxas luxas added this to the v1.6 milestone Feb 24, 2017
pires
pires reviewed Feb 24, 2017
View reviewed changes
cmd/kubeadm/app/master/manifests.go Outdated
@@ -304,7 +304,7 @@ func getAPIServerCommand(cfg *kubeadmapi.MasterConfiguration, selfHosted bool) [
}

command = append(getComponentBaseCommand(apiServer),
"--insecure-bind-address=127.0.0.1",
"--insecure-port=0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this disable or randomly selects a port?

Copy link
Contributor

Choose a reason for hiding this comment

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

Disables. See https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/options/serving.go#L269-L285

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to get the "randomly selects a port" behavior? It would be useful for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to get the "randomly selects a port" behavior? It would be useful for testing.

I think that belongs outside this layer. See https://github.com/kubernetes/kubernetes/pull/42059/files#diff-a8c1e5100912cb27840594582aa0904bR66 as an example of how to wire it.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH -- this is a little confusing. Generally setting a port to 0 means "randomly select port". But that isn't a kubeadm issue.

pires
pires approved these changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@pires pires left a comment

Choose a reason for hiding this comment

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

/lgtm

@luxas
Copy link
Member Author

luxas commented Feb 24, 2017

This one also depends on #41882

@luxas luxas added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 24, 2017
@luxas
Copy link
Member Author

luxas commented Feb 24, 2017

Applying lgtm given the general consensus in the Slack discussion, but this depends on two PRs in the Submit Queue yet, so it can't/shouldn't be merged until this one can rebase upon them

jbeda
jbeda approved these changes Feb 25, 2017
View reviewed changes
cmd/kubeadm/app/master/manifests.go Outdated
@@ -304,7 +304,7 @@ func getAPIServerCommand(cfg *kubeadmapi.MasterConfiguration, selfHosted bool) [
}

command = append(getComponentBaseCommand(apiServer),
"--insecure-bind-address=127.0.0.1",
"--insecure-port=0",
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH -- this is a little confusing. Generally setting a port to 0 means "randomly select port". But that isn't a kubeadm issue.

@jbeda
Copy link
Contributor

jbeda commented Feb 26, 2017

The release note should specify that this is for kubeadm only.

Also -- have you tested this? With 1.5, the admission controllers talk to the api server via a loopback address. They use the insecure port by default. If the insecure port is turned off you need to specify a new flag (the poorly named --tls-ca-file) and ensure the serving cert for the API server includes localhost.

@liggitt
Copy link
Member

liggitt commented Feb 26, 2017

If the insecure port is turned off you need to specify a new flag (the poorly named --tls-ca-file) and ensure the serving cert for the API server includes localhost.

That is no longer necessary. We ensure the loopback connection is secured without the user having to provide a localhost cert.

@liggitt
Copy link
Member

liggitt commented Feb 26, 2017

Admission, controllers, and scheduler are all running against the secured port in CI, but agree that this needs to be tested specifically with kubeadm

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017
@luxas
Copy link
Member Author

luxas commented Feb 26, 2017

@jbeda

This one also depends on #41882

Now it will work as @liggitt said. It has now merged, so I'll rebase, test this again and remove do-not-merge.

@luxas luxas force-pushed the kubeadm_remove_unsecure_port branch from eb5d45c to e6f2457 Compare February 27, 2017 17:08
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 27, 2017
@luxas luxas removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 27, 2017
@luxas
Copy link
Member Author

luxas commented Feb 27, 2017

Removing do-not-merge on this.
This is LGTM'd already, but I want @jbeda to make the final call.
This would make kubectl commands to ~
sudo KUBECONFIG=/etc/kubernetes/admin.conf kubectl get po

@luxas
Copy link
Member Author

luxas commented Feb 27, 2017

@k8s-bot gce etcd3 e2e test this

@jbeda
Copy link
Contributor

jbeda commented Feb 27, 2017

This is super exciting.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
@luxas luxas added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 27, 2017
@luxas
Copy link
Member Author

luxas commented Feb 27, 2017

Temporarily adding do-not-merge because I don't wanna result in a rebase for #41772. Will remove that block when #41772 is merged and I have rebased.

@luxas luxas force-pushed the kubeadm_remove_unsecure_port branch from e6f2457 to 3f59284 Compare February 28, 2017 15:52
@luxas
Copy link
Member Author

luxas commented Feb 28, 2017

Removing do-not-merge since #41772 is merged, this one is rebased and ready to merge!

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: jbeda, luxas

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

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

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017
@luxas luxas added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Feb 28, 2017
@luxas
Copy link
Member Author

luxas commented Mar 1, 2017

@k8s-bot test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit fed7cea into kubernetes:master Mar 1, 2017
@luxas luxas mentioned this pull request Mar 2, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MHBauer MHBauer MHBauer left review comments

@deads2k deads2k deads2k left review comments

@pires pires pires approved these changes

@jbeda jbeda jbeda approved these changes

Assignees

@jbeda jbeda

@liggitt liggitt

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
9 participants
@luxas @k8s-reviewable @jbeda @liggitt @k8s-github-robot @pires @deads2k @MHBauer @k8s-ci-robot