RBAC setup behind the aggregator. #936

MHBauer · 2017-06-12T20:41:28Z

yaml was generated by putting rbac into permissive mode and seeing what it would have denied. then @liggitt processed the log messages to create the output and left some suggested TODOs.

@pmorie do you undestand the leader election TODOs?

Service accounts were done to see what each server was specificly doing. We can back that out and adapt the yaml.

Need to have the auth setup as in the auth docs.

MHBauer · 2017-06-12T21:05:17Z

@arschles @krancour can you help me understand if there's something I should be helm-izing and how to do it?

MHBauer · 2017-06-13T00:39:38Z

resolves #880

MHBauer · 2017-06-13T03:22:59Z

@kibbles-n-bytes is rbac on in jenkins CI? I'm not sure why this would fail over there.

liggitt · 2017-06-13T04:09:49Z

contrib/svc-cat-rbac.yaml

+  - apiGroup: ""
+    kind: ServiceAccount
+    name: server
+    namespace: catalog


Is this namespace right or does it need to be parameterized to match SVCCAT_NAMESPACE

Unspecified assumption currently. Needs to be integrated into the helm chart to get parameterized.

@MHBauer It would be best parameterized in the values.yaml file. Ping me if you want help with that

arschles

@MHBauer I think I answered a few helm questions, and I do think the namespace should at least be parameterized in the helm chart (even though that's separate from the shell scripts, it's a step in the right direction).

Also, I had a few additions to the markdown file.

Overall, this is great.

arschles · 2017-06-14T21:29:23Z

contrib/svc-cat-rbac.yaml

+  - apiGroup: ""
+    kind: ServiceAccount
+    name: server
+    namespace: catalog


After https://github.com/kubernetes-incubator/service-catalog/pull/936/files#r121578665 is done, this should read:

namespace: {{ .Values.namespace }}

arschles · 2017-06-14T21:29:42Z

contrib/svc-cat-rbac.yaml

+  - apiGroup: ""
+    kind: ServiceAccount
+    name: server
+    namespace: catalog


Same here RE the parameterized namespace. There are some other similar changes below, but I won't comment on them all.

arschles · 2017-06-14T21:32:27Z

docs/rbac-setup.md

+# RBAC Setup 
+
+If rbac is enabled, allow the service catalog to grant access for it's
+service token.


@MHBauer could you add a bit here on what would have been set in the original helm install command, so that people know whether they enabled RBAC? It may not be obvious to a newcomer.

@arschles Gone!

arschles · 2017-06-14T21:33:12Z

docs/rbac-setup.md

+
+## Install our rbac rules
+
+See the [script in contrib](../contrib/rbac-setup.sh). See the


Should they run the script in contrib to install the RBAC rules? If so, can you add that little piece to the docs

arschles · 2017-06-14T21:35:30Z

charts/catalog/templates/apiserver-deployment.yaml

@@ -20,6 +20,7 @@ spec:
        release: "{{ .Release.Name }}"
        heritage: "{{ .Release.Service }}"
    spec:
+      serviceAccountName: server


I think this and the serviceAccountName in controller-manager-deployment.yaml would be worth parameterizing in the values.yaml file as well

The walkthrough test fails because contrib/rbac-setup.sh isn't run in contrib/hack/test_walkthrough.sh, so the service account additions here break the deployment.

We should either make these serviceAccountNames optional, or update both contrib/hack/test_walkthrough.sh and the walkthrough docs to require running contrib/rbac-setup.sh.

sheesh. You mentioned it and tagged it for me and I still missed it. sorry guys.

kibbles-n-bytes · 2017-06-15T17:16:25Z

@MHBauer I was out earlier this week, looking into this today.

kibbles-n-bytes · 2017-06-15T18:12:39Z

charts/catalog/templates/apiserver-deployment.yaml

@@ -20,6 +20,7 @@ spec:
        release: "{{ .Release.Name }}"
        heritage: "{{ .Release.Service }}"
    spec:
+      serviceAccountName: server


The walkthrough test fails because contrib/rbac-setup.sh isn't run in contrib/hack/test_walkthrough.sh, so the service account additions here break the deployment.

We should either make these serviceAccountNames optional, or update both contrib/hack/test_walkthrough.sh and the walkthrough docs to require running contrib/rbac-setup.sh.

kibbles-n-bytes · 2017-06-15T18:15:10Z

contrib/rbac-setup.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+SVCCAT_NAMESPACE=catalog


This assumes the namespace will be "catalog". This should probably be parameterized. A simple way would be:

SVCCAT_NAMESPACE="${SVCCAT_NAMESPACE:-catalog}"

kibbles-n-bytes · 2017-06-15T18:16:51Z

contrib/rbac-setup.sh

+kubectl create namespace ${SVCCAT_NAMESPACE}
+kubectl create serviceaccount server --namespace=${SVCCAT_NAMESPACE}
+kubectl create serviceaccount controller --namespace=${SVCCAT_NAMESPACE}
+kubectl create -f svc-cat-rbac.yaml


This assumes we're running this file while in contrib/. I'd add

ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"

to the top of the file, and then change the file reference to "${ROOT}/contrib/svc-cat-rbac.yaml

MHBauer · 2017-06-16T20:12:14Z

I agree that the account names need to be parameterized here. The account names need to match the rbac definition yaml.

The service account creation should become part of the charts.

The rbac yaml should become part of the charts.

MHBauer · 2017-06-16T22:48:27Z

contrib/jenkins/init_cluster.sh

+    --clusterrole=cluster-admin --user="${ACCOUNT_NAME}" \
+  || { echo 'Cannot not create cluster-admin role for service account.'; exit 1; }
+
+${ROOT}/contrib/rbac-setup.sh \


maybe won't need this script at all.

Since you got rid of it now, lines 66-68 should be removed as well.

I am really dumb some times.

MHBauer · 2017-06-17T04:35:52Z

I think we're good now. rbac by default. At the very least it shouldn't break anything on someone running without rbac on.
@kibbles-n-bytes thanks much for helping me solve jenkins issue.

kibbles-n-bytes · 2017-06-19T17:07:12Z

charts/catalog/templates/controller-manager-deployment.yaml

@@ -20,6 +20,7 @@ spec:
        release: "{{ .Release.Name }}"
        heritage: "{{ .Release.Service }}"
    spec:
+      serviceAccountName: controller


You still need to templatize the definition here and in apiserver-deployment.yaml.

🦈 good eye. I'll take another pass through. Thanks!

MHBauer · 2017-06-19T21:14:45Z

@kibbles-n-bytes @arschles I can't believe you both tagged some things and I still overlooked them. Sorry about that. We should be good now. Not sure why jenkins is failing. Looks to me like it successfully installs.

MHBauer · 2017-06-19T21:51:25Z

This can go in before or after #895, but you'll need to manually disable rbac if #895 goes in first.

arschles · 2017-06-20T21:06:52Z

@MHBauer is the jenkins failure a flake?

kibbles-n-bytes · 2017-06-21T17:04:08Z

@arschles @MHBauer I'm looking into it now. At first it was because the RBAC stuff needed a newer version of Helm than I had installed, but there is still something causing it to consistently fail.

liggitt · 2017-06-21T17:12:06Z

if you're testing with an apiserver build from master, kubernetes/kubernetes#46812 (comment) would have broken this a couple days ago

MHBauer · 2017-06-21T17:34:37Z

@liggitt our apiserver stuff is somewhere between levels v1.5.5 and v1.6.0-rc1

MHBauer · 2017-06-21T17:34:43Z

undo!

MHBauer · 2017-06-21T17:42:49Z

looking at https://service-catalog-jenkins.appspot.com/job/service-catalog-PR-testing2/885/
I see

Error syncing pod, skipping: failed to "StartContainer" for "etcd" with ImagePullBackOff: "Back-off pulling image \"quay.io/coreos/etcd:latest\""

which would be a flake to me.

kibbles-n-bytes · 2017-07-07T17:16:12Z

@MHBauer As I mentioned on Slack, I believe that's the same issue I hit when trying to run this on GKE. I had to create a ClusterRoleBinding with the "cluster-admin" role before doing any of the RBAC stuff.

MHBauer · 2017-07-07T17:39:53Z

contrib/jenkins/init_cluster.sh

+# cluster roles. Needed for RBAC setup.
+ACCOUNT_NAME="$(gcloud info | grep Account | sed 's/.*\[\(.*\)\]/\1/')"
+kubectl create clusterrolebinding jenkins-cluster-admin-binding \
+    --clusterrole=cluster-admin --user="${ACCOUNT_NAME}" \


@arschles @kibbles-n-bytes for default minikube I did
kubectl create clusterrolebinding minikube-cluster-admin-binding --clusterrole=cluster-admin --user=minikube

Nope, did not help, same
Error: release catalog failed: clusterroles.rbac.authorization.k8s.io "servicecatalog.k8s.io:apiserver" is forbidden: attempt to grant extra privileges: [PolicyRule{Resources:["namespaces"], APIGroups:[""], Verbs:["get"]} PolicyRule{Resources:["namespaces"], APIGroups:[""], Verbs:["list"]} PolicyRule{Resources:["namespaces"], APIGroups:[""], Verbs:["watch"]}] user=&{system:serviceaccount:kube-system:default d6100657-632b-11e7-8c06-befc308e6091 [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] map[]} ownerrules=[] ruleResolutionErrors=[]

rbac wasn't on in minikube, oops.

add --extra-config=apiserver.Authorization.Mode=RBAC

We can add this to the walkthrough docs in a follow-up

Tracked in #1021

kibbles-n-bytes · 2017-07-07T17:43:56Z

@MHBauer Still investigating this failure, should have a fix out soon.

MHBauer · 2017-07-07T20:23:52Z

@kibbles-n-bytes rebased on to 34ed5cd.

arschles · 2017-07-12T18:33:31Z

Service accounts were done to see what each server was specificly doing. We can back that out and adapt the yaml.

@MHBauer IMO it's fine to leave these

luxas · 2017-07-12T19:17:43Z

@MHBauer ready to review and merge?

MHBauer · 2017-07-12T21:25:12Z

charts/catalog/templates/rbac.yaml

@@ -0,0 +1,127 @@
+apiVersion: v1


@pmorie @arschles @luxas @liggitt should I make a helm chart and wrap this whole thing in it? Not sure if it's okay to unconditionally send rbac objects. Would helm fail?

@MHBauer I believe we should be able to use the following to test whether the rbac API group is available:

{{- if .Capabilities.APIVersions.Has "rbac.k8s.io/v1beta1" }} // stuff {{- end }}

pmorie

Looking great, just a couple suggestions / gaps to close.

pmorie · 2017-07-13T05:53:48Z

charts/catalog/templates/rbac.yaml

@@ -0,0 +1,127 @@
+apiVersion: v1


@MHBauer I believe we should be able to use the following to test whether the rbac API group is available:

{{- if .Capabilities.APIVersions.Has "rbac.k8s.io/v1beta1" }} // stuff {{- end }}

pmorie · 2017-07-13T05:56:29Z

charts/catalog/templates/rbac.yaml

+items:
+
+# TODO: if this is just for namespace lifecycle admission, move to a generic role
+- apiVersion: rbac.authorization.k8s.io/v1beta1


I believe this should be the only use for the API server, and does constitute what is necessary for namespace lifecycle admission. I'm cool calling this role something like namespace-lifecycle-admission, your choice.

Is there more to do to address the todo? or a followup.

pmorie · 2017-07-13T05:57:41Z

charts/catalog/templates/rbac.yaml

+  # TODO: do not grant global access, limit to particular secrets referenced from bindings
+  - apiGroups: [""]
+    resources: ["secrets"]
+    verbs:     ["get","create"]


The controller manager deletes secrets when bindings are deleted, so delete is definitely needed.

pmorie · 2017-07-13T05:58:26Z

charts/catalog/templates/rbac.yaml

+    resources: ["secrets"]
+    verbs:     ["get","create"]
+  - apiGroups: [""]
+    resources: ["configmaps"]


I don't think we have any uses of configmaps in the code today, do we? Does auth delegation rely on it?

pmorie · 2017-07-13T05:58:41Z

charts/catalog/templates/rbac.yaml

+    resources: ["secrets"]
+    verbs:     ["get","create"]
+  - apiGroups: [""]
+    resources: ["configmaps"]


(It might be handy to comment saying what each of these permissions is used for)

pmorie · 2017-07-13T05:59:53Z

charts/catalog/templates/rbac.yaml

+  - apiGroups: [""]
+    resources: ["namespaces"]
+    verbs:     ["get", "list", "watch"]
+- apiVersion: rbac.authorization.k8s.io/v1beta1


Looks like we are missing RBAC rules for settings API group

What should that look like?

MHBauer · 2017-07-14T00:42:26Z

I know that the 'requestheader-ca' information is looked up by the apiserver in a configmap. I know for api aggregation it's needed. Maybe for auth delegation as well. Any comment @DirectXMan12 ?

MHBauer · 2017-07-14T00:46:43Z

What's the proper way to drop comments in? Is there a way to get the comment into kube so we know what it does later? I obviously want a very specific name, but there's only so many characters I can use in there.

luxas · 2017-07-14T15:40:34Z

I know that the 'requestheader-ca' information is looked up by the apiserver in a configmap. I know for api aggregation it's needed. Maybe for auth delegation as well. Any comment @DirectXMan12 ?

The extension-apiserver-authentication-reader Role in kube-system grants that for you, not needed to grant access to all configmaps.

What's the proper way to drop comments in? Is there a way to get the comment into kube so we know what it does later? I obviously want a very specific name, but there's only so many characters I can use in there.

I think commenting in the yaml spec is good enough.

MHBauer · 2017-07-14T16:20:47Z

apiVersion: rbac.authorization.k8s.io/v1beta1
kind: Role
metadata:
  annotations:
    rbac.authorization.kubernetes.io/autoupdate: "true"
  creationTimestamp: 2017-07-12T16:58:34Z
  labels:
    kubernetes.io/bootstrapping: rbac-defaults
  name: extension-apiserver-authentication-reader
  namespace: kube-system
  resourceVersion: "137"
  selfLink: /apis/rbac.authorization.k8s.io/v1beta1/namespaces/kube-system/roles/extension-apiserver-authentication-reader
  uid: 5726faf6-6723-11e7-97c3-02420ac00002
rules:
- apiGroups:
  - ""
  resourceNames:
  - extension-apiserver-authentication
  resources:
  - configmaps
  verbs:
  - get

I see that now.

This was always intended as a rough draft of "what happened" when I ran some tests with the rbac in permissive mode. I don't know of any more definitive way to determine the exact permissions we may need. This is a bad UX.

I'll take configmaps out and test, but it's going to delay this again.

MHBauer · 2017-07-14T18:04:43Z

@kibbles-n-bytes rebase why?

kibbles-n-bytes · 2017-07-14T18:06:15Z

@MHBauer The GKE cluster version time bomb went off. 😅 #1023

MHBauer · 2017-07-14T18:12:16Z

gotcha gotch. I'll do some refactoring of the commits while I'm rebasing.

- RBAC in chart - add the ServiceAccount definitions to chart - parameterize namespace and ServiceAccount names Thanks to - liggitt for helping me with discovering RBAC rules. - DirectXMan12 for helping me troubleshoot aggregator setup.

MHBauer · 2017-07-14T20:11:52Z

/retest

kibbles-n-bytes · 2017-07-14T20:12:48Z

@MHBauer Check my latest comment on contrib/jenkins/init_cluster.sh!

MHBauer · 2017-07-14T20:15:52Z

@kibbles-n-bytes thanks

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

liggitt reviewed Jun 13, 2017

View reviewed changes

arschles reviewed Jun 14, 2017

View reviewed changes

pmorie added this to the 0.0.11 milestone Jun 14, 2017

kibbles-n-bytes reviewed Jun 15, 2017

View reviewed changes

MHBauer commented Jun 16, 2017

View reviewed changes

MHBauer requested review from duglin, pmorie and vaikas June 17, 2017 04:36

kibbles-n-bytes reviewed Jun 19, 2017

View reviewed changes

MHBauer closed this Jun 21, 2017

MHBauer reopened this Jun 21, 2017

arschles modified the milestones: 0.0.11, 0.0.12 Jun 21, 2017

MHBauer commented Jul 7, 2017

View reviewed changes

kibbles-n-bytes mentioned this pull request Jul 7, 2017

make e2e look for pods' existence before checking status #1012

Merged

kibbles-n-bytes added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2017

kibbles-n-bytes removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2017

arschles mentioned this pull request Jul 12, 2017

Add RBAC setup instructions for minikube to the walkthrough docs #1021

Closed

MHBauer commented Jul 12, 2017

View reviewed changes

pmorie reviewed Jul 13, 2017

View reviewed changes

kibbles-n-bytes added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2017

RBAC

bdea977

- RBAC in chart - add the ServiceAccount definitions to chart - parameterize namespace and ServiceAccount names Thanks to - liggitt for helping me with discovering RBAC rules. - DirectXMan12 for helping me troubleshoot aggregator setup.

MHBauer removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2017

add RBAC setup to Jenkins cluster initialization

401d03f

pmorie added the LGTM1 label Jul 14, 2017

kibbles-n-bytes merged commit 8ec0874 into kubernetes-retired:master Jul 17, 2017

MHBauer mentioned this pull request Jul 19, 2017

document rbac setup #880

Closed

wallrj mentioned this pull request Oct 31, 2017

Leader election fails when RBAC is enabled jetstack/navigator#68

Closed


		## Install our rbac rules

		See the [script in contrib](../contrib/rbac-setup.sh). See the

RBAC setup behind the aggregator. #936

RBAC setup behind the aggregator. #936

Conversation

MHBauer commented Jun 12, 2017

MHBauer commented Jun 12, 2017

MHBauer commented Jun 13, 2017

MHBauer commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles left a 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

kibbles-n-bytes commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Jun 17, 2017

kibbles-n-bytes Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Jun 19, 2017

MHBauer commented Jun 19, 2017

arschles commented Jun 20, 2017

kibbles-n-bytes commented Jun 21, 2017

liggitt commented Jun 21, 2017

MHBauer commented Jun 21, 2017

MHBauer commented Jun 21, 2017 • edited Loading

MHBauer commented Jun 21, 2017

kibbles-n-bytes commented Jul 7, 2017

Choose a reason for hiding this comment

MHBauer Jul 7, 2017 • edited Loading

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

kibbles-n-bytes commented Jul 7, 2017

MHBauer commented Jul 7, 2017

arschles commented Jul 12, 2017

luxas commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie left a 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

MHBauer commented Jul 14, 2017

MHBauer commented Jul 14, 2017

luxas commented Jul 14, 2017

MHBauer commented Jul 14, 2017

MHBauer commented Jul 14, 2017

kibbles-n-bytes commented Jul 14, 2017

MHBauer commented Jul 14, 2017

MHBauer commented Jul 14, 2017

kibbles-n-bytes commented Jul 14, 2017

MHBauer commented Jul 14, 2017

kibbles-n-bytes Jun 19, 2017 •

edited

Loading

MHBauer commented Jun 21, 2017 •

edited

Loading

MHBauer Jul 7, 2017 •

edited

Loading