[stable/nginx-ingress] Add RBAC support #1235

jsulinski · 2017-06-04T04:56:57Z

Enable RBAC as per: https://github.com/kubernetes/ingress/tree/master/examples/rbac/nginx and kubernetes/ingress-nginx#266

k8s-ci-robot · 2017-06-04T04:56:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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-ci-robot · 2017-06-04T04:57:04Z

Hi @jsulinski. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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-ci-robot · 2017-06-04T05:07:08Z

@jsulinski: you can't request testing unless you are a kubernetes member.

In response to this:

/retest

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.

cknowles · 2017-06-04T08:39:01Z

stable/nginx-ingress/templates/controller-deployment.yaml

@@ -22,6 +25,9 @@ spec:
        component: "{{ .Values.controller.name }}"
        release: {{ .Release.Name }}
    spec:
+{{- if .Values.controller.rbac.enabled }}
+      serviceAccountName: {{ template "fullname" . }}
+{{- end }}


I submitted a similar update for Traefik in #1225. When switching RBAC enabled from true to false and helm installing, I found the serviceAccountName stays as the Traefik specific one rather than being reset to default. I wondered if you had encountered that issue here?

No, I didn't experience this. I would presume the issue is elsewhere in your case, as this conditional logic is very simple.

@jsulinski hmmm strange the Traefik chart doesn't have any complex logic here, the switch is pretty much identical.

Should this also have a default service account? https://github.com/kubernetes/charts/blob/master/stable/kube2iam/templates/daemonset.yaml#L24

I believe you'll also want to apply this to the controller-daemonset.yaml.

I agree. Pulled your changes.

linki · 2017-06-07T15:33:33Z

stable/nginx-ingress/templates/controller-deployment.yaml

@@ -9,6 +9,9 @@ metadata:
    heritage: {{ .Release.Service }}
    release: {{ .Release.Name }}
  name: {{ template "controller.fullname" . }}
+{{- if .Values.controller.rbac.enabled }}
+  namespace: {{ .Release.Namespace }}


This seems weird. I think we prefer to leave out the namespace in the metadata section of manifests and let helm take care of that when the --namespace flag is used.

However, you probably need it when you reference the subject in the role bindings, but it shouldn't be required here. Can you give it a try? (including the other manfiests)

Updated, thanks so much! You were spot on.

bison · 2017-06-18T23:41:42Z

stable/nginx-ingress/templates/controller-role.yaml

+      # Here: "<ingress-controller-leader>-<nginx>"
+      # This has to be adapted if you change either parameter
+      # when launching the nginx-ingress-controller.
+      - "ingress-controller-leader-nginx"


This seems like it should be a user-supplied list defaulting to ["ingress-controller-leader-nginx"].

Agreed. Will update.

bison · 2017-06-18T23:43:52Z

This looks great. Hope it's merged soon. I think the example RBAC rules here have been updated in the last few days. Might want to pull in the latest version.

flah00 · 2017-06-26T17:29:15Z

@jsulinski I have been testing this PR with the latest nginx-ingress-controller 0.9.0 beta and found that I had to add create, delete, and patch for configmaps. It seems that the controller maintains a configmap named ingress-controller-leader-nginx.

Disregard, I made a mistake, when I merged your branch ... derp.

jamielennox · 2017-06-28T00:54:46Z

stable/nginx-ingress/values.yaml

@@ -36,6 +36,10 @@ controller:
  ##
  kind: Deployment

+  ## enable RBAC as per: https://github.com/kubernetes/ingress/tree/master/examples/rbac/nginx and https://github.com/kubernetes/ingress/issues/266
+  rbac:
+    enabled: false


nit: The two charts with optional RBAC support that have merged seem to be stable/etcd-operator and incubator/istio and both use .Values.rbac.install as the flag. Personally i prefer enabled, but thought i'd point it out for consistency.

Great point. I'll update when I have a free moment.

fyi, looks like the Traefik chart is merged w/ rbac.enabled, 👍

https://github.com/kubernetes/charts/blob/master/stable/traefik/values.yaml#L63

Sticking with rbac.enabled.

ReSearchITEng · 2017-07-05T20:04:26Z

Once merged, it will solve: #927

icereval · 2017-07-10T18:22:30Z

stable/nginx-ingress/templates/controller-cluster-role.yaml

+  - apiGroups:
+      - ""
+    resources:
+        - events


nit; spacing is off just a bit

icereval · 2017-07-10T18:23:37Z

stable/nginx-ingress/templates/controller-cluster-role.yaml

+  - apiGroups:
+      - ""
+    resources:
+      - nodes


when using --watch-namespace I had to add namespaces here to avoid the following error:

F0710 18:20:42.008164 10 launch.go:145] no watchNamespace with name stage-1-osf found: User "system:serviceaccount:stage-1-osf:mfjzdy-nginx-ingress" cannot get namespaces in the namespace "stage-1-osf".: "Unknown user \"system:serviceaccount:stage-1-osf:mfjzdy-nginx-ingress\"" (get namespaces stage-1-osf)

Can you elaborate on this?

The controller is attempting to request a list of namespaces and due to lack of permissions throws an exception (get namespaces stage-1-osf). This command option essentially allows you to have the controller only watch a specified namespace for ingress' tagged with nginx, rather than a single global ingress nginx controller for the entire cluster.

In our case we are evaluating dedicated ip/ingress for each significant component of a larger application, thinking more towards a potential future of adding ModSecurity at this controller ingress point.

Can you share an example? I haven't used --watch-namespace yet.

.─────────────────. _.─' `──. ( Internet ) `───. _.──' `───────────────' │ ▼ ┌───────────────────────┐ │ Google Load Balancer │ └───────────────────────┘ │ ┌─────────────────┴──────────────────┐ ┌─Cluster #1────────┼────────────────────────────────────┼───────────────────┐ │ ▼ ▼ │ │ ┌Namespace (Testing)──────────────┐ ┌Namespace (Staging)──────────────┐ │ │ │┌──────────────┐ ┌──────────────┐│ │┌──────────────┐ ┌──────────────┐│ │ │ ││ Ingress │ │ Default ││ ││ Ingress │ │ Default ││ │ │ ││ Controller │ │ Backend ││ ││ Controller │ │ Backend ││ │ │ │└──────────────┘ └──────────────┘│ │└──────────────┘ └──────────────┘│ │ │ │ │ ▲ │ │ │ ▲ │ │ │ │ └───────┬────────┘ │ │ └───────┬────────┘ │ │ │ │ │ │ │ │ │ │ │ │ ┌────────────────┐ │ │ ┌────────────────┐ │ │ │ │ │ ingress.yaml │ │ │ │ ingress.yaml │ │ │ │ │ └────────────────┘ │ │ └────────────────┘ │ │ │ │ │ │ │ │ │ │ │ │ ┌──────────┼──────────┐ │ │ ┌──────────┼──────────┐ │ │ │ │ ▼ ▼ ▼ │ │ ▼ ▼ ▼ │ │ │ │ ┌───────┐ ┌───────┐ ┌───────┐ │ │ ┌───────┐ ┌───────┐ ┌───────┐ │ │ │ │ │ Pod 1 │ │ Pod 2 │ │ Pod 3 │ │ │ │ Pod 1 │ │ Pod 2 │ │ Pod 3 │ │ │ │ │ └───────┘ └───────┘ └───────┘ │ │ └───────┘ └───────┘ └───────┘ │ │ │ └─────────────────────────────────┘ └─────────────────────────────────┘ │ │ │ └────────────────────────────────────────────────────────────────────────────┘

please tell me how you made that diagram! (programatically i hope)

Thank you! I'll work on this as soon as I have a moment.

@viglesiasce, using this awesome tool https://monodraw.helftone.com/

jsulinski · 2017-07-10T18:23:42Z

I'm working on testing revisions but having some issues with my RBAC enabled Kube cluster and chart. I'll push the changes without testing if I can't make progress soon.

based on: helm/charts#1235

jsulinski · 2017-07-12T05:34:21Z

This is tested with all changes except making resourceNames a value. I'll do that as soon as I can, or anyone else is welcome to.

jsulinski · 2017-07-12T05:34:43Z

Thanks @icereval!

jsulinski · 2017-07-12T05:47:53Z

@bison Thanks, updated.

k8s-ci-robot · 2017-07-14T23:46:25Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

jsulinski · 2017-08-02T02:06:34Z

@icereval Do you want to add watchNamespace too? I haven't had time to test this yet but it would be good to add. Thanks!

mgoodness · 2017-08-02T02:10:59Z

I can take care of watchNamespace, too, since it's RBAC-related. Will ping you all when I think it's ready.

mgoodness · 2017-08-02T05:00:15Z

I think this will do it. RBAC for --watch-namespace is a bit funky. If controller.scope.enabled=true and controller.scope.namespace is set (meaning we're watching a namespace that isn't .Release.Namespace), we'll add get namespaces for that namespace to the ClusterRole. If controller.scope.enabled=true and controller.scope.namespace is not set, then we don't add get namespaces to the ClusterRole and simply rely on permission granted by the Role.

I've not actually tested this, but wanted to document it before I shut down for the night.

jsulinski · 2017-08-05T21:50:57Z

Thanks a lot! I tested this and it works for my albeit limited use case. I have not tested namespaces, however.

The design you described for clusterRole makes sense to me, but I would take that with a grain of salt. :)

based on: helm#1235

* this produces the correctly formatted output we are looking for, specifically with regard to the extra carriage return which didn't want to be chomped away by the usual `-` method.

mgoodness · 2017-08-14T19:19:41Z

/ok-to-test

mgoodness · 2017-08-14T19:27:35Z

/lgtm

jsulinski · 2017-08-14T19:51:44Z

Awesome. Thanks a lot for helping get this over the finish line! Really appreciate it.

@jsulinski

This option was added helm#1235 by @jsulinski, but as best as I can tell, `defaultSSLCertificate` was not wired up.

* nginx-ingress: fix spacing for events * Add RBAC support * Add RBAC support for nginx-ingress based on: helm/charts#1235 * Pull in new RBAC changes from kubernetes/ingress-nginx@4618fd2 * Move resourceNames to a value * templates: use a range loop to render the resourceNames * this produces the correctly formatted output we are looking for, specifically with regard to the extra carriage return which didn't want to be chomped away by the usual `-` method. * Tweak RBAC * Bump chart versions * Fix README * Restrict namespace RBAC if scoped

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 4, 2017

jsulinski changed the title ~~Add RBAC support~~ [stable/nginx-ingress] Add RBAC support Jun 4, 2017

cknowles reviewed Jun 4, 2017

View reviewed changes

linki reviewed Jun 7, 2017

View reviewed changes

jsulinski force-pushed the rbac branch from d76a225 to 17cfeea Compare June 7, 2017 23:39

bison reviewed Jun 18, 2017

View reviewed changes

jamielennox reviewed Jun 28, 2017

View reviewed changes

cknowles mentioned this pull request Jun 28, 2017

[stable/traefik] RBAC support for Traefik #1225

Merged

ReSearchITEng mentioned this pull request Jul 5, 2017

[stable/nginx-ingress] k8s 1.6 - perms #927

Closed

icereval reviewed Jul 10, 2017

View reviewed changes

icereval added a commit to CenterForOpenScience/helm-charts that referenced this pull request Jul 11, 2017

Add RBAC support for nginx-ingress

02899ee

based on: helm/charts#1235

jsulinski force-pushed the rbac branch from 17cfeea to d66d929 Compare July 12, 2017 05:26

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 14, 2017

mgoodness force-pushed the rbac branch from e359b0b to 8718f77 Compare August 2, 2017 02:30

mgoodness mentioned this pull request Aug 2, 2017

Add RBAC setting to ingress controller #1582

Closed

jsulinski and others added 10 commits August 14, 2017 14:12

Add RBAC support

70561f5

nginx-ingress: fix spacing for events

9930560

Add RBAC support for nginx-ingress

bd9bb54

based on: helm#1235

Pull in new RBAC changes from kubernetes/ingress-nginx@4618fd2

4f97fb9

Move resourceNames to a value

0394da3

templates: use a range loop to render the resourceNames

c00a2d3

* this produces the correctly formatted output we are looking for, specifically with regard to the extra carriage return which didn't want to be chomped away by the usual `-` method.

Tweak RBAC

f3bbc01

Bump chart versions

358b8ff

Fix README

a5eca07

Restrict namespace RBAC if scoped

fbe521c

mgoodness force-pushed the rbac branch from 0f38417 to fbe521c Compare August 14, 2017 19:18

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 14, 2017

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2017

mgoodness merged commit c1ede62 into helm:master Aug 14, 2017

erickt mentioned this pull request Oct 31, 2017

[stable/nginx-ingress] Enable configuration of default SSL certificate #2624

Closed

erickt added a commit to erickt/charts that referenced this pull request Oct 31, 2017

[stable/nginx-ingress] Enable configuration of default SSL certificate

bf03eba

This option was added helm#1235 by @jsulinski, but as best as I can tell, `defaultSSLCertificate` was not wired up.

MarkusTeufelberger mentioned this pull request Nov 6, 2017

nginx-ingress does not provide RBAC support #1623

Closed

jokogr mentioned this pull request Mar 19, 2019

[stable/grafana] Enable extra init containers and extra emptyDir mounts #12343

Merged

3 tasks

[stable/nginx-ingress] Add RBAC support #1235

[stable/nginx-ingress] Add RBAC support #1235

Conversation

jsulinski commented Jun 4, 2017

k8s-ci-robot commented Jun 4, 2017

k8s-ci-robot commented Jun 4, 2017

k8s-ci-robot commented Jun 4, 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

linki Jun 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

bison commented Jun 18, 2017

flah00 commented Jun 26, 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

ReSearchITEng commented Jul 5, 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

icereval Jul 16, 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

jsulinski commented Jul 10, 2017

jsulinski commented Jul 12, 2017

jsulinski commented Jul 12, 2017

jsulinski commented Jul 12, 2017

k8s-ci-robot commented Jul 14, 2017

jsulinski commented Aug 2, 2017

mgoodness commented Aug 2, 2017

mgoodness commented Aug 2, 2017

jsulinski commented Aug 5, 2017

mgoodness commented Aug 14, 2017

mgoodness commented Aug 14, 2017

jsulinski commented Aug 14, 2017

linki Jun 7, 2017 •

edited

Loading

flah00 commented Jun 26, 2017 •

edited

Loading

icereval Jul 16, 2017 •

edited

Loading