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

add rbac-user plugin #69

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@saada
Copy link

commented Feb 13, 2019


Checklist for plugin developers:

  • Read the Plugin Naming Guide (for new plugins)
  • Verify the installation from URL or a local archive works (kubectl krew install --manifest=[...] --archive=[...])
@googlebot

This comment has been minimized.

Copy link

commented Feb 13, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
1 similar comment
@googlebot

This comment has been minimized.

Copy link

commented Feb 13, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
@saada

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

I signed it!

@googlebot

This comment has been minimized.

Copy link

commented Feb 13, 2019

CLAs look good, thanks!

1 similar comment
@googlebot

This comment has been minimized.

Copy link

commented Feb 13, 2019

CLAs look good, thanks!

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

👋 Hello! Thanks for your plugin submission.

Currently I have issues with the naming as it stands today. Neither the name: rbac-user nor shortDescription: Simplify user management with rbac explain what the plugin actually does.

Can you please reconsider the name/description/shortDescription values while asking yourself “can I understand what this plugin does by looking at its name?”.

@saada saada force-pushed the saada:rbac-user branch from 46e6ce6 to 4e1a870 Feb 13, 2019

@saada

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

Hey @ahmetb , thanks for reviewing. I updated the descriptions as requested. Let me know if you still don't like the name. There's a better description here: https://github.com/saada/kubectl-rbac-user

* jq
* bash
description: |
Easily create and delete ServiceAccounts. This plugin allows you to bind them to existing ClusterRoles in your Kubernetes cluster. The plugin simplifies the management of user access by taking care of deleting associated RoleBindings and ClusterRoleBindings associated with a ServiceAccount on deletion. After creating a user, it's easy to share a kubeconfig with the `view-serviceaccount-kubeconfig` plugin. Although it's named `rbac-user`, you can use it to also manage service access in general since it's just ServiceAccounts.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 13, 2019

Contributor

You can break this into multiple lines

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

There's a better description here: saada/kubectl-rbac-user

Ideally the description should be good enough here to explain what the plugin does.

Let me know if you still don't like the name.

It's not me not liking the name. I’m claiming right now most people won’t get what the plugin does by its name. We have other rbac plugins like

rbac-lookup                    Reverse lookup for RBAC                            installed
rbac-view                      A tool to visualize your RBAC permissions.         available

and their name is pretty clear and self-explanatory.

After looking at your repo:

  • Your plugin creates service accounts (optionally with specified role, in a specified namespace, with permissions/restrictions in other namespaces)
    • it does all these (svcacct create + bindings create) in one shot
    • similarly it deletes bindings + serviceaccounts in one shot
  • I think you should probably consider adding the "Usage" section in "caveats" (or link to your documentation from "caveats").

I think your user experience should be something like:

kubectl rbac-binding create <svcacct> [--namespace/-n] [--role=] [...]
kubectl rbac-binding delete <svcacct> [--namespace/-n] [--role=] [...]

What do you think?

@ahmetb ahmetb added the hold label Feb 26, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

/hold

@saada

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

I started refactoring code. Will submit update sometime this month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.