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.

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

update RBAC file #7

Closed
pohly opened this issue Dec 11, 2018 · 10 comments
Closed

update RBAC file #7

pohly opened this issue Dec 11, 2018 · 10 comments

Comments

@pohly
Copy link
Contributor

pohly commented Dec 11, 2018

The RBAC file was copied unmodified from driver-registrar and probably is out-dated now.

The introduction still refers to "external provisioner" (was already broken when creating that file initially for driver-registrar).

@msau42
Copy link
Collaborator

msau42 commented Dec 11, 2018

I opened #8 to remove the old driver registration mechanism.

Then I think we can remove the whole rbac file.

I also scanned through the code and couldn't find anything using k8s events

@pohly
Copy link
Contributor Author

pohly commented Dec 21, 2018

@msau42 @jarrpa: 6d58825 removed the rbac.yaml. Was that intentionally or by mistake?

Neither the commit message nor the PR description mention the removal of the rbac.yaml, and people who want to know what RBAC rules are needed no longer find that information in this repo. For the sake of consistency there should be a deploy directory with a README.md saying that the lack of a rbac.yaml file is intentional.

@msau42
Copy link
Collaborator

msau42 commented Dec 21, 2018

Node driver registrar doesn't interact with the k8s API anymore so no rbac rules are needed.

We can update the README in this repo to be more descriptive about what node driver registrar does

@jarrpa
Copy link
Contributor

jarrpa commented Dec 21, 2018

While the README could use a touch-up, I think the removal of the RBAC file is only confusing if you knew the file was there to begin with. If we just say "it's a sidecar container" there should be no reason to assume it requires any API privileges unless stated otherwise. At best, a deploy/ directory would contain a sample DaemonSet for something like the hostPath driver that uses this sidecar, right?

@pohly
Copy link
Contributor Author

pohly commented Dec 21, 2018 via email

@msau42
Copy link
Collaborator

msau42 commented Dec 21, 2018

I think it's sufficient if we document the readme for all the sidecars with the functionality.

For this repo, we can say that it plugs into the kubelet plugin registration mechanism by exposing a socket, etc.

For other repos, we can say, it watches and operates on these kubernetes objects.

@msau42
Copy link
Collaborator

msau42 commented Dec 21, 2018

And each readme can have a permissions section that describes what kind of rbacs and other roles it may need

@msau42
Copy link
Collaborator

msau42 commented Dec 27, 2018

Updating docs here: #13

@msau42
Copy link
Collaborator

msau42 commented Dec 27, 2018

README has been updated. Please update as you see fit.
/close

@k8s-ci-robot
Copy link
Contributor

@msau42: Closing this issue.

In response to this:

README has been updated. Please update as you see fit.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants