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

Disable the public cadvisor port by default #63881

Merged
merged 2 commits into from May 22, 2018

Conversation

@luxas
Copy link
Member

luxas commented May 15, 2018

What this PR does / why we need it:
Quoting @tallclair in #56523:

We should add the deprecation warning in 1.10 along with a release note, but not change the default. The notes should warn that the default will change in 1.11. We disable the flag by default in 1.11, and remove it entirely in 1.12 or 1.13.
If you currently depend on the UI or the API, speak up! Going forward, the recommended way of taking advantage of those features will be to run cAdvisor as a DaemonSet.

Disabling the publicly-available cAdvisor port is beneficial for security, as you might not want to expose the UI with lots of information about what your system is doing. We already did this for all kubeadm deployments in v1.7, and haven't recieved any issues for that. This should be okay to do at this stage, as this flag was deprecated in v1.10. Given we need to support this flag for one more release (v1.11), it makes perfect sense to instead switch it off in preparation for v1.12 when we can delete it (see the deprecation policy)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #56523

Special notes for your reviewer:

I removed the e2e test that expects cAdvisor to be running, as we don't expect it to be anymore.

Release note:

[action required] The formerly publicly-available cAdvisor web UI that the kubelet ran on port 4194 by default is now turned off by default. The flag configuring what port to run this UI on `--cadvisor-port` was deprecated in v1.10. Now the default is `--cadvisor-port=0`, in other words, to not run the web server. The recommended way to run cAdvisor if you still need it, is via a DaemonSet. The `--cadvisor-port` will be removed in v1.12

cc @kubernetes/sig-cluster-lifecycle-pr-reviews @kubernetes/sig-auth-pr-reviews @kubernetes/sig-node-pr-reviews

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented May 15, 2018

Will it have been depreciated for 6 months as of the 1.11 release? I don't think it has been 6 months since the 1.10 release at this point...

Rule #5b: CLI elements of admin-facing components (e.g. kubelet) must function after their announced deprecation for no less than:

  * GA: 6 months or 1 release (whichever is longer)
@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented May 15, 2018

Reading it more closely, I guess it doesn't prevent us from changing defaults. @mtaufen WDYT?

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 15, 2018

Exactly. We need to wait 6 months for removal (two releases, until v1.12), but to provide a smooth removal path we'll disable it already in v1.11.

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented May 15, 2018

This seems reasonable to me. I feel like we should get an "ack" from a top-level API approver though.
/assign @thockin
/assign @dchen1107

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 15, 2018

/retest

1 similar comment
@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 15, 2018

/retest

@timothysc
Copy link
Member

timothysc left a comment

/lgtm

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 15, 2018

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce

@thockin thockin removed their assignment May 16, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 21, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 21, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 21, 2018

@luxas: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce 703dc82 link /test pull-kubernetes-kubemark-e2e-gce

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.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 22, 2018

Automatic merge from submit-queue (batch tested with PRs 63881, 64046, 63409, 63402, 63221). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 99e9db5 into kubernetes:master May 22, 2018

16 of 18 checks passed

pull-kubernetes-kubemark-e2e-gce Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce
Details
cla/linuxfoundation luxas 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-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request May 23, 2018

Kubernetes Submit Queue
Merge pull request #63887 from luxas/kubeadm_kubelet_integration
Automatic merge from submit-queue (batch tested with PRs 63914, 63887, 64116, 64026, 62933). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Write kubelet config file to disk and persist in-cluster

**What this PR does / why we need it**:
In order to make configuration flow from the cluster level to node level, we need a way for kubeadm to tell the kubelet what config to use. As of v1.10 (I think) the kubelet can read `--config` using the kubelet Beta ComponentConfiguration API, so now we have an interface to talk to the kubelet properly.

This PR:
 - Writes the kubelet ComponentConfig to `/var/lib/kubelet/config.yaml` on init and join
 - Writes an environment file to source in the kubelet systemd dropin `/var/lib/kubelet/kubeadm-flags.env`. This file contain runtime flags that should be passed to the kubelet.
 - Uploads a ConfigMap with the name `kubelet-config-1.X`
 - Patches the node object so that it starts using the ConfigMap with updates using Dynamic Kubelet Configuration, **only if the feature gate is set** (currently alpha and off by default, not intended to be switched on in v1.11)
 - Updates the phase commands to reflect this new flow

The kubelet dropin file I used now looks like this:
```
# v1.11.x dropin as-is at HEAD
# /etc/systemd/system/kubelet.service.d/10-kubeadm.conf
---
[Service]
Environment="KUBELET_KUBECONFIG_ARGS=--bootstrap-kubeconfig=/etc/kubernetes/bootstrap-kubelet.conf --kubeconfig=/etc/kubernetes/kubelet.conf"
Environment="KUBELET_CONFIG_ARGS=--config=/var/lib/kubelet/config.yaml"
EnvironmentFile-=/var/lib/kubelet/kubeadm-flags.env
# Should default to 0 in v1.11: #63881, and hence not be here in the real v1.11 manifest
Environment="KUBELET_CADVISOR_ARGS=--cadvisor-port=0"
# Should be configurable via the config file: #63878, and hence be configured using the file in v1.11
Environment="KUBELET_CERTIFICATE_ARGS=--rotate-certificates=true"
ExecStart=
ExecStart=/usr/bin/kubelet $KUBELET_KUBECONFIG_ARGS $KUBELET_CONFIG_ARGS $KUBELET_KUBEADM_ARGS $KUBELET_CADVISOR_ARGS $KUBELET_CERTIFICATE_ARGS $KUBELET_EXTRA_ARGS
---
# v1.11.x dropin end goal
# /etc/systemd/system/kubelet.service.d/10-kubeadm.conf
---
[Service]
Environment="KUBELET_KUBECONFIG_ARGS=--bootstrap-kubeconfig=/etc/kubernetes/bootstrap-kubelet.conf --kubeconfig=/etc/kubernetes/kubelet.conf"
Environment="KUBELET_CONFIG_ARGS=--config=/var/lib/kubelet/config.yaml"
EnvironmentFile-=/var/lib/kubelet/kubeadm-flags.env
ExecStart=
ExecStart=/usr/bin/kubelet $KUBELET_KUBECONFIG_ARGS $KUBELET_CONFIG_ARGS $KUBELET_KUBEADM_ARGS $KUBELET_EXTRA_ARGS
---
# Environment file dynamically created at runtime by "kubeadm init"
# /var/lib/kubelet/kubeadm-flags.env
KUBELET_KUBEADM_ARGS=--cni-bin-dir=/opt/cni/bin --cni-conf-dir=/etc/cni/net.d --network-plugin=cni
```

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

**Special notes for your reviewer**:

**Release note**:

```release-note
"kubeadm init" now writes a structured and versioned kubelet ComponentConfiguration file to `/var/lib/kubelet/config.yaml` and an environment file with runtime flags (you can source this file in the systemd kubelet dropin) to `/var/lib/kubelet/kubeadm-flags.env`.
```
@kubernetes/sig-cluster-lifecycle-pr-reviews @mtaufen

mvladev added a commit to mvladev/gardener that referenced this pull request May 23, 2018

mvladev added a commit to mvladev/gardener that referenced this pull request May 23, 2018

mvladev added a commit to mvladev/gardener that referenced this pull request May 23, 2018

mvladev added a commit to mvladev/gardener that referenced this pull request May 23, 2018

mvladev added a commit to mvladev/gardener that referenced this pull request May 23, 2018

k8s-github-robot pushed a commit that referenced this pull request May 25, 2018

Kubernetes Submit Queue
Merge pull request #64184 from dixudx/e2e_cadvisor_skew
Automatic merge from submit-queue.

Prevent 1.10 e2es testing deprecated CAdvisorPort in 1.11

**What this PR does / why we need it**:
The public cadvisor port by default is disabled in #63881, targeted for v1.11.

But 1.10 e2e tests get run against v1.11+ masters during upgrade tests.

https://k8s-testgrid.appspot.com/sig-release-master-upgrade#gce-1.10-master-upgrade-cluster-parallel

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #64158

**Special notes for your reviewer**:
/cc luxas BenTheElder krzyzacy
 
**Release note**:

```release-note
Prevent 1.10 e2es testing deprecated CAdvisorPort in v1.11
```

k8s-github-robot pushed a commit that referenced this pull request Jul 7, 2018

Kubernetes Submit Queue
Merge pull request #65707 from dims/remove-deprecated-cadvisor-port
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove --cadvisor-port - has been deprecated since v1.10

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #56523

**Special notes for your reviewer**:
- Deprecated in #59827 (v1.10)
- Disabled in #63881 (v1.11)

**Release note**:

```release-note
[action required] The formerly publicly-available cAdvisor web UI that the kubelet started using `--cadvisor-port` is now entirely removed in 1.12. The recommended way to run cAdvisor if you still need it, is via a DaemonSet.
```

@tallclair tallclair modified the milestones: v1.11, v1.12 Aug 1, 2018

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Aug 1, 2018

It looks like this didn't make it into 1.11. Retagging as 1.12.

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Aug 2, 2018

@tallclair ?? This made it into v1.11 for sure.

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Aug 2, 2018

Huh, sorry. I don't know what I was looking at before. Maybe I got my tabs crossed.

@tallclair tallclair modified the milestones: v1.12, v1.11 Aug 2, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Aug 4, 2018

No problem

@jimangel

This comment has been minimized.

Copy link
Member

jimangel commented Aug 17, 2018

@luxas do you have a PR against the 1.12 docs branch open for this? I have a feeling this will create a snowball of doc PRs, but maybe you could help find a spot for a general announcement? Thanks!

cpick pushed a commit to cpick/prometheus-operator that referenced this pull request Jan 31, 2019

Chris Pick
Fix exported-kubelets on GKE >=1.11
K8s 1.11 [disables](kubernetes/kubernetes#63881)
the separate cAdvisor port (4194) on kubelets.

In GKE >=1.11 this data must now be fetched from the same port/endpoint
as the rest of the metrics (under a separate path).
Add a `httpSeparateCadvisor` setting to support this.
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.