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

Remove e2e-rbac-bindings. #46750

Merged
merged 1 commit into from
Jun 9, 2017
Merged

Remove e2e-rbac-bindings. #46750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions cluster/addons/cluster-monitoring/google/heapster-controller.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@
{% set nanny_memory = (90 * 1024 + num_nodes * nanny_memory_per_node)|string + "Ki" -%}
{% endif -%}

apiVersion: v1
kind: ServiceAccount
metadata:
name: heapster
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
Expand Down Expand Up @@ -134,6 +143,7 @@ spec:
- name: usr-ca-certs
hostPath:
path: "/usr/share/ca-certificates"
serviceAccountName: heapster
tolerations:
- key: "CriticalAddonsOnly"
operator: "Exists"
10 changes: 10 additions & 0 deletions cluster/addons/cluster-monitoring/googleinfluxdb/heapster-controller-combined.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@
{% set nanny_memory = (90 * 1024 + num_nodes * nanny_memory_per_node)|string + "Ki" -%}
{% endif -%}

apiVersion: v1
kind: ServiceAccount
metadata:
name: heapster
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
Expand Down Expand Up @@ -135,6 +144,7 @@ spec:
- name: usr-ca-certs
hostPath:
path: "/usr/share/ca-certificates"
serviceAccountName: heapster
tolerations:
- key: "CriticalAddonsOnly"
operator: "Exists"
58 changes: 58 additions & 0 deletions cluster/addons/cluster-monitoring/heapster-rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRoleBinding
metadata:
name: heapster-binding
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: system:heapster
subjects:
- kind: ServiceAccount
name: heapster
namespace: kube-system
---
# Heapster's pod_nanny monitors the heapster deployment & its pod(s), and scales
# the resources of the deployment if necessary.
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: Role
metadata:
name: system:pod-nanny
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
rules:
- apiGroups:
- ""
resources:
- pods
verbs:
- get
Copy link
Member

Choose a reason for hiding this comment

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

heapster only gets pods in the kube-system namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw the "pod_nanny" (one of heapster's containers) getting the heapster pod from the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

the sidecar dynamically resizes the heapster deployment, so it'll also need to do a put/patch on deployments to resize itself.

@Q-Lee

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya, you need write on deployments.

Copy link
Contributor

Choose a reason for hiding this comment

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

the sidecar dynamically resizes the heapster deployment, so it'll also need to do a put/patch on deployments to resize itself.

This seems like a questionable power to grant to a pod. If its given the power to modify its own deployment, then a compromise of a particular pod cannot be resolved by simply deleting the pod since it has been allowed to modify its own config.

In addition, by doing it inside of the kube-system namespace, if you compromise the heapster pod, you will have compromised every deployment, rs, rc, secret, etc in the cluster because it gained the power to change its service account which gave it the power to mount any SA token in the kube-system namespace where controller SAs are located.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a questionable power to grant to a pod

Agree, and since not all installations run the pod nanny, it shouldn't be part of the standard heapster role.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could assign service accounts to individual pods. I wonder if there's a work around.

Could we create a "heapster-nanny" role, bind it to a service account, and mount the service account's secret where SAs are usually mounted?

Copy link
Member Author

Choose a reason for hiding this comment

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

All 5 of the supported CLUSTER_MONITORING setups have the pod-nanny running with heapster, and we don't create this role/binding unless one of those is selected.

I renamed that role to system:pod-nanny.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it is not optimal, but it's better than what currently exists.

- apiGroups:
- "extensions"
resources:
- deployments
verbs:
- get
- update
---
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: RoleBinding
metadata:
name: heapster-binding
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: system:pod-nanny
subjects:
- kind: ServiceAccount
name: heapster
namespace: kube-system
---
10 changes: 10 additions & 0 deletions cluster/addons/cluster-monitoring/influxdb/heapster-controller.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@
{% set nanny_memory = (90 * 1024 + num_nodes * nanny_memory_per_node)|string + "Ki" -%}
{% endif -%}

apiVersion: v1
kind: ServiceAccount
metadata:
name: heapster
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
Expand Down Expand Up @@ -113,6 +122,7 @@ spec:
- --container=eventer
- --poll-period=300000
- --estimator=exponential
serviceAccountName: heapster
tolerations:
- key: "CriticalAddonsOnly"
operator: "Exists"
10 changes: 10 additions & 0 deletions cluster/addons/cluster-monitoring/stackdriver/heapster-controller.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@
{% set nanny_memory = (90 * 1024 + num_nodes * nanny_memory_per_node)|string + "Ki" -%}
{% endif -%}

apiVersion: v1
kind: ServiceAccount
metadata:
name: heapster
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
Expand Down Expand Up @@ -91,6 +100,7 @@ spec:
- name: usr-ca-certs
hostPath:
path: "/usr/share/ca-certificates"
serviceAccountName: heapster
tolerations:
- key: "CriticalAddonsOnly"
operator: "Exists"
10 changes: 10 additions & 0 deletions cluster/addons/cluster-monitoring/standalone/heapster-controller.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@
{% set nanny_memory = (90 * 1024 + num_nodes * nanny_memory_per_node)|string + "Ki" -%}
{% endif -%}

apiVersion: v1
kind: ServiceAccount
metadata:
name: heapster
namespace: kube-system
labels:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
Expand Down Expand Up @@ -75,6 +84,7 @@ spec:
- --container=heapster
- --poll-period=300000
- --estimator=exponential
serviceAccountName: heapster
tolerations:
- key: "CriticalAddonsOnly"
operator: "Exists"
5 changes: 0 additions & 5 deletions cluster/addons/e2e-rbac-bindings/README.md
View file
Open in desktop

This file was deleted.

20 changes: 0 additions & 20 deletions cluster/addons/e2e-rbac-bindings/random-addon-grabbag.yaml
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions ...ns/e2e-rbac-bindings/kubelet-binding.yaml → cluster/addons/rbac/kubelet-binding.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
# The GKE environments don't have kubelets with certificates that
# identify the system:nodes group. They use the kubelet identity
# TODO cjcullen should figure out how wants to manage his upgrade
# this will only hold the e2e tests until we get an authorizer
# which authorizes particular nodes
# TODO: remove this once new nodes are granted individual identities and the
# NodeAuthorizer is enabled.
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRoleBinding
metadata:
Expand Down
5 changes: 3 additions & 2 deletions cluster/gce/container-linux/configure-helper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,8 +1126,8 @@ function start-kube-addons {
local -r src_dir="${KUBE_HOME}/kube-manifests/kubernetes/gci-trusty"
local -r dst_dir="/etc/kubernetes/addons"

# prep the additional bindings that are particular to e2e users and groups
setup-addon-manifests "addons" "e2e-rbac-bindings"
Copy link
Member

@liggitt liggitt Jun 4, 2017 •
edited

Choose a reason for hiding this comment

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

does this script set up the rbac addon which now contains the replacement kubelet binding?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does now... :)

# prep addition kube-up specific rbac objects
setup-addon-manifests "addons" "rbac"

# Set up manifests of other addons.
if [[ "${ENABLE_CLUSTER_MONITORING:-}" == "influxdb" ]] || \
Expand All @@ -1136,6 +1136,7 @@ function start-kube-addons {
[[ "${ENABLE_CLUSTER_MONITORING:-}" == "standalone" ]] || \
[[ "${ENABLE_CLUSTER_MONITORING:-}" == "googleinfluxdb" ]]; then
local -r file_dir="cluster-monitoring/${ENABLE_CLUSTER_MONITORING}"
setup-addon-manifests "addons" "cluster-monitoring"
setup-addon-manifests "addons" "${file_dir}"
# Replace the salt configurations with variable values.
base_metrics_memory="140Mi"
Expand Down
5 changes: 1 addition & 4 deletions cluster/gce/gci/configure-helper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1481,10 +1481,6 @@ function start-kube-addons {
local -r src_dir="${KUBE_HOME}/kube-manifests/kubernetes/gci-trusty"
local -r dst_dir="/etc/kubernetes/addons"

# TODO(mikedanese): only enable these in e2e
# prep the additional bindings that are particular to e2e users and groups
setup-addon-manifests "addons" "e2e-rbac-bindings"
Copy link
Contributor

Choose a reason for hiding this comment

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

should this line be

setup-addon-manifests "addons" "rbac"

as above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, it was already in this file.


# prep addition kube-up specific rbac objects
setup-addon-manifests "addons" "rbac"

Expand All @@ -1495,6 +1491,7 @@ function start-kube-addons {
[[ "${ENABLE_CLUSTER_MONITORING:-}" == "standalone" ]] || \
[[ "${ENABLE_CLUSTER_MONITORING:-}" == "googleinfluxdb" ]]; then
local -r file_dir="cluster-monitoring/${ENABLE_CLUSTER_MONITORING}"
setup-addon-manifests "addons" "cluster-monitoring"
setup-addon-manifests "addons" "${file_dir}"
# Replace the salt configurations with variable values.
base_metrics_memory="140Mi"
Expand Down