Adding a metadata proxy addon #45565

Q-Lee · 2017-05-09T22:44:07Z

What this PR does / why we need it: adds a metadata server proxy daemonset to hide kubelet secrets.

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

Special notes for your reviewer:

Release note: the gce metadata server can be hidden behind a proxy, hiding the kubelet's token.

The gce metadata server can be hidden behind a proxy, hiding the kubelet's token.

k8s-reviewable · 2017-05-09T22:44:16Z

This change is

Q-Lee · 2017-05-09T22:45:27Z

You can find the proxy image here: kubernetes-retired/contrib#2576

Q-Lee · 2017-05-10T00:24:15Z

@k8s-bot kops aws e2e test this

gmarek · 2017-05-10T09:15:51Z

cluster/addons/metadata-proxy/metadata-proxy.yaml

+      # Note that this does not guarantee admission on the nodes (#40573).
+      annotations:
+        scheduler.alpha.kubernetes.io/critical-pod: ''
+    spec:


Resource requests are missing.

gmarek · 2017-05-10T09:19:03Z

cluster/gce/gci/configure-helper.sh

@@ -1423,6 +1423,9 @@ function start-kube-addons {
  if [[ "${ENABLE_DEFAULT_STORAGE_CLASS:-}" == "true" ]]; then
    setup-addon-manifests "addons" "storage-class/gce"
  fi
+  if [[ "${ENABLE_METADATA_PROXY:-}" == "simple" ]]; then


Why only in GCI? You should add it at least to debian, as some of our tests run on it.

OK, I've tried adding it to configure-vm.sh and the salt files.

piosz · 2017-05-11T13:38:17Z

cluster/addons/metadata-proxy/metadata-proxy.yaml

+          - name: config-volume
+            mountPath: /etc/nginx/
+      nodeSelector:
+        beta.kubernetes.io/metadata-proxy-ready: "true"


Why do you need this label? Similar label was introduced for fluentd because historically it ran as a manifest pod and we wanted to avoid situation where we will end up with 2 instances of fluentd.

The label is so we'll only run it on node pools where the daemonset is enabled.

Do you have any use where you want to have it enabled for some nodes and disabled for others within the same cluster?

Testing and migration of existing clusters.

Do you need to run metadata proxy addon on nodes in version 1.x+ and do not run it on nodes in version <1.x?

That is also the case.

Q-Lee · 2017-05-16T06:26:14Z

@k8s-bot unit test this

roberthbailey · 2017-05-17T06:18:41Z

cluster/addons/metadata-proxy/OWNERS

@@ -0,0 +1,4 @@
+approvers:
+- q-lee


it'd be better to have at least one other name here to avoid the bus problem

Added mike/cj.

roberthbailey · 2017-05-17T06:19:05Z

cluster/addons/metadata-proxy/README.md

+==============
+
+This metadata proxy returns a 403 for kubelet's secrets, but otherwise allows
+pods through to the metadata server.


s/through/access/ ?

cjcullen · 2017-05-19T01:24:34Z

cluster/addons/metadata-proxy/README.md

+# Metadata proxy
+==============
+
+This metadata proxy returns a 403 for kubelet's secrets, but otherwise allows


"secrets" is probably misleading here. Maybe say that it returns a 403 for the configuration data stored in kube-env.

cjcullen · 2017-05-19T01:31:35Z

cluster/gce/config-default.sh

-NODE_LABELS="${KUBE_NODE_LABELS:-beta.kubernetes.io/fluentd-ds-ready=true}"
+NODE_LABELS="${KUBE_NODE_LABELS:-beta.kubernetes.io/fluentd-ds-ready=true,beta.kubernetes.io/metadata-proxy-ready=true}"
+
+# Turn the simple metadata proxy on by default.


Is defaulting this on safe? Seems like this could break workloads that happen to be depending on something they shouldn't.

Dependencies on kube-env cannot be supported. If we're required to support unknown dependencies there, then we're unable to make just about any internal change whatsoever.

Q-Lee · 2017-05-23T01:02:06Z

@k8s-bot pull-kubernetes-unit test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

Q-Lee · 2017-05-23T20:13:44Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

cjcullen · 2017-05-24T16:55:16Z

Squash commits, and then this ells gee to me.

destijl · 2017-05-24T16:55:17Z

cluster/addons/metadata-proxy/metadata-proxy-configmap.yaml

@@ -0,0 +1,60 @@
+kind: ConfigMap


Is there an integration or e2e test that tests this configuration? i.e. the allow/deny rules are correct and work as intended?

The happy cases should be well tested by feature tests (e.g., logging and monitoring tests should fail in the absence of a metadata server). I intend to push a negative case test after this goes in.

destijl · 2017-05-24T16:59:49Z

cluster/addons/metadata-proxy/metadata-proxy-configmap.yaml

+  labels:
+    addonmanager.kubernetes.io/mode: EnsureExists
+data:
+  nginx.conf: |-


We should think about minimizing the work the proxy is doing here to keep it simple. I took a quick tour through options here:
http://nginx.org/en/docs/http/ngx_http_proxy_module.html

proxy_cache: do we care? Maybe this should be off, might reduce any hard-to-debug cache weirdness.

I think proxy_cache is off by default.

http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache

Ah, so it is. This LGTM.

jbeda · 2017-05-26T23:24:54Z

Hate to swoop in here but this thing should be named gce-metadata-proxy as it is very GCE specific.

Q-Lee · 2017-05-27T00:53:37Z

@jbeda How gce specific is it really? Is the aws metadata server laid out much differently (that's not a rhetorical question)? Are there no secrets on the aws metadata server we would want to block using a different nginx config? Could someone want to run kube2iam using this mechanism?

I did consider the directory structure metadata-proxy/gce. Perhaps that's better? It defers a lot of thinking.

Q-Lee · 2017-05-31T01:47:17Z

@jbeda - does "metadata-proxy/gce" sound like a reasonable solution?
@gmarek - any chance of an lgtm?

gmarek · 2017-05-31T07:54:18Z

IIRC @piosz were reviewing this

Q-Lee · 2017-05-31T23:10:42Z

Fair enough, pinging @piosz

Q-Lee · 2017-06-01T00:49:21Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this
@k8s-bot pull-kubernetes-unit test this

piosz · 2017-06-01T08:38:29Z

/lgtm
for configuration part

Q-Lee · 2017-06-01T19:31:37Z

/assign @mikedanese

Can this get approval?

mikedanese · 2017-06-01T21:34:47Z

/approve

k8s-github-robot · 2017-06-01T21:43:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Q-Lee, mikedanese, piosz

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

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

~~cluster/OWNERS~~ [mikedanese]

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 · 2017-06-03T15:55:32Z

Automatic merge from submit-queue

Q-Lee requested review from cjcullen and piosz May 9, 2017 22:44

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2017

k8s-github-robot assigned roberthbailey and gmarek May 9, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 9, 2017

gmarek reviewed May 10, 2017

View reviewed changes

piosz self-assigned this May 11, 2017

piosz reviewed May 11, 2017

View reviewed changes

Q-Lee force-pushed the mds branch from fec1010 to 970a7ee Compare May 16, 2017 05:29

roberthbailey assigned destijl May 16, 2017

roberthbailey reviewed May 17, 2017

View reviewed changes

Q-Lee force-pushed the mds branch from b6c942c to 4901ade Compare May 19, 2017 00:20

cjcullen reviewed May 19, 2017

View reviewed changes

roberthbailey removed their assignment May 19, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017

Q-Lee force-pushed the mds branch from 587a126 to 416caea Compare May 23, 2017 00:11

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017

Q-Lee force-pushed the mds branch from 416caea to 8b1d600 Compare May 23, 2017 19:25

destijl reviewed May 24, 2017

View reviewed changes

Q-Lee force-pushed the mds branch from 8b1d600 to 29cc8e5 Compare May 26, 2017 03:53

Adding a metadata proxy addon to gce

1bfed01

Q-Lee force-pushed the mds branch from 29cc8e5 to 1bfed01 Compare May 31, 2017 23:23

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 1, 2017

Q-Lee added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jun 1, 2017

k8s-ci-robot assigned mikedanese Jun 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2017

Q-Lee added this to the v1.7 milestone Jun 2, 2017

k8s-github-robot merged commit 3473b8a into kubernetes:master Jun 3, 2017

vincentwoo mentioned this pull request Jun 19, 2017

Filter/restrict access to http://metadata from containers #8867

Closed

Adding a metadata proxy addon #45565

Adding a metadata proxy addon #45565

Conversation

Q-Lee commented May 9, 2017 • edited Loading

k8s-reviewable commented May 9, 2017

Q-Lee commented May 9, 2017

Q-Lee commented May 10, 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

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

Q-Lee commented May 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Q-Lee commented May 23, 2017

Q-Lee commented May 23, 2017

cjcullen commented May 24, 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

jbeda commented May 26, 2017

Q-Lee commented May 27, 2017 • edited Loading

Q-Lee commented May 31, 2017

gmarek commented May 31, 2017

Q-Lee commented May 31, 2017

Q-Lee commented Jun 1, 2017

piosz commented Jun 1, 2017

Q-Lee commented Jun 1, 2017

mikedanese commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

k8s-github-robot commented Jun 3, 2017

Q-Lee commented May 9, 2017 •

edited

Loading

Q-Lee commented May 27, 2017 •

edited

Loading