added rbac example discussed in kubernetes/ingress issue #266 #747
Conversation
|
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.
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
added
the
cncf-cla: no
|
This change is |
|
@nevetS please sign the CLA |
|
I have now signed the agreement. |
k8s-ci-robot
added
cncf-cla: yes
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: nginx-ingress-serviceaccount |
There was a problem hiding this comment.
I'd omit the -serviceaccount since it's inherent in the kind
There was a problem hiding this comment.
I thought about changing this, but I'm reluctant and here's why:
Because this is an example, I think it should be clear what maps to what without having to do any mental gymnastics. The ClusterRole, namespace, ServiceAccount, Role, RoleBinding, and ClusterRoleBinding can all drop the -thingtype extension, but then we'd have 6 objects all named nginx-ingress and it might cause confusion to people who are making modifications to the example in their own systems. While it's a bit of extra text and seems obvious, it's a little more clear for someone new.
There was a problem hiding this comment.
@nevetS l think you can review the ingress's source code, and then you can write a streamline rabc yaml.
| rules: | ||
| - apiGroups: | ||
| - "" | ||
| - "extensions" |
There was a problem hiding this comment.
None of these resources are in the extensions API group, can omit that from this rule
There was a problem hiding this comment.
This change has been made and committed.
| - get | ||
| - list | ||
| - watch | ||
| - update |
There was a problem hiding this comment.
Why does it need update rights on services?
There was a problem hiding this comment.
@liggitt because when the Ingress rule uses a namedPort (string, like web) we don't have the required information in the endpoint
There was a problem hiding this comment.
That explains the need for read access, but not write access
There was a problem hiding this comment.
Sorry, I forgot to mentioned that we update the service adding an annotation here
There was a problem hiding this comment.
Does an ingress named port refer to the name of the serviceport or the name of the container port? The name of the service port is available in the endpoint object.
Also, that implementation of resolving the named port to a single numeric port doesn't work if pods are non-homogenous (some expose port 80 as "web", some expose port 90 as "web")
There was a problem hiding this comment.
@liggitt I just pushed a refactoring to remove the annotation.
There was a problem hiding this comment.
@nevetS please remove the update right in the service
There was a problem hiding this comment.
i think use does't to create role , you can only create serviceAccount , culster-role, rolebinding , it 's sample and correct.
There was a problem hiding this comment.
updated service permissions and reflected the updated in README.md.
| - "extensions" | ||
| resources: | ||
| - configmaps | ||
| - secrets |
There was a problem hiding this comment.
Global list/watch on secrets is very escalating. Is this always required? Could it be limited to individual gets?
There was a problem hiding this comment.
Global list/watch on secrets is very escalating
Ingress rules can be created/modified dynamically so is not possible to know the namespace/name of the secrets
Could it be limited to individual gets?
Is possible in the case of the ConfigMap but we don't know the name beforehand
There was a problem hiding this comment.
Can the secret be retrieved at time of use by name?
There was a problem hiding this comment.
Yes I can retrieve the secret by name but I need to know then a secret is updated or removed (and that is the reason of the use of informers)
There was a problem hiding this comment.
I'm not sure if I follow here. Should I make a change to the example?
|
@liggitt thank you for reviewing this PR! |
|
@nevetS also please squash the commits |
|
Coverage decreased (-0.5%) to 46.614% when pulling f445bf501f0525f1a302cb17c15cb81f893e8d86 on nevetS:master into b6d11ca on kubernetes:master. |
|
@huangjiasingle - your comment to review the ingress code, get rid of the role and streamline the permissions is the only one that I haven't addressed. The reason for separating the role permissions from the cluster role permissions is to separate what is needed for the controller from what is needed for the entire cluster. That way, the example shows the most secure implementation of role based access control. You are correct in asserting that I have not reviewed the code. I've relied on the discussion in issue #266 to build this, along with some similar postings that I've found on the web and my own experimentation stripping away permissions and testing with a simple implementation. I don't have the time to do such a review right at this moment as I have competing priorities and I'm unfamiliar with the code, but I am happy to make changes that reflect the original goal. I'm leery to strip the role permissions completely because I have stripped individual role permissions during my own testing and found that they are necessary. |
|
@nevetS please squash the commits so we can merge this PR. I a later PR we can improve the example. |
| resources: | ||
| - events | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
In my case to get TLS working with multiple namespaces, I had to add patch as well (#575 (comment))
examples/rbac/nginx/README.md
Outdated
| permissions are granted to the Role named `nginx-ingress-role` | ||
|
|
||
| * `configmaps`,`secrets`,`pods`: get | ||
| * `endpoints`: get, create, update |
There was a problem hiding this comment.
- Could you maybe order the list alphabetically ascending, i.e.
create, get, update? - I had to specify
endpoints: patchas well (Kubeadm using 1.6 - Ingess-Controller can't access API #575 (comment))
| rules: | ||
| - apiGroups: | ||
| - "" | ||
| resources: |
There was a problem hiding this comment.
I think it would be nice to order the resources alphabetically ascending to keep future diffs lean
|
I have made the requested changes and squashed the commits. Thank you to everyone! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@nevetS thanks! |
|
hi, @nevetS the serviceaccount |
|
In the example shown in the readme, the controller is in the nginx-ingress namespace. The reason for the separation in this example is so that the reader has a clear understanding of what is required for the controller itself, and what is required for the entire cluster. If you want to run in a namespace other than what is specified in the example, you can adjust the Role permissions namespace to the namespace where you have the nginx-ingress-controller implemented. You would also need to adjust the namespace of the service account, and you would not need the namespace defined in the example yml file. If you have already implemented the example file without realizing you have a controller in a different namespace, you can use i.e. kubectl --namespace=nginx-ingress delete ServiceAccount nginx-ingress-serviceaccount
kubectl --namespace=nginx-ingress delete Role nginx-ingress-role
kubectl --namespace=nginx-ingress delete Namespace nginx-ingress
# then create a file with the service account and role defined in your desired namespace
kubectl create -f ./serviceaccount.yml
kubectl create -f ./role.yml
# and then reapply the bindings to the service account - again, in a new file (or you can have them all defined in a single file if you want)
kubectl create -f ./rolebindings.yml
# and then you'd likely need to update your nginx-ingress-controller to reflect a new serviceaccount
kubectl delete Deployment nginx-ingress-controller
#edit the nginx-ingress-controller file to include your service account
kubectl create -f ./yourcontrollerdefinition.yml
#and since your ip address/external hostname may have changed if you are in a cloud provider,
# adjust any dns records that aren't automatically updated |
|
@jeremywu0127 I should also point out that when creating permissions, it's very easy to give access to secrets to the default service account. I like to have my ingress controller in a separate namespace because I don't want any other process to inadvertently gain access to my externally facing tls certificate secrets. |
After struggling a bit myself with implementing an ingress controller, I ran into issue #266. Following the advice in that thread, I was able to move forward with my own implementation.
The discussion mentions creating an example a couple of times, but no example exists and no pull request exists that I can see, so I put one together.
I'm happy to revise this as necessary to have it included in the project.