-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 command eksctl utils aws-auth
#623
Conversation
f6a000e
to
5306e57
Compare
eksctl utils aws-auth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! I think this is great even at this point.
The benefit of this improvement and the addition of addAccount()
is that (to me) this opens up possibility to manage additional mapRoles
and mapAccounts
in our ClusterConfig.
Perhaps your original motivation could be either to:
- implement
eksctl utils aws-auth [map|unmap] [account --id AWS_ACCOUNT_ID|role --arn ROLE_ARN --username USERNAME --groups GROUP1,GROUP2]
on top of this, or - enhance ClusterConfig and add
eksctl utils apply-aws-auth -f cluster.yaml
to update the aws-auth configmap according to the mappings in ClusterConfig.
If that's the case, I would have thought that I liked the latter, but WDYT?
@mumoshu thanks for the suggestions. this PR is motivated by the former (sorry for scarce PR description). I briefly went over this with @errordeveloper beforehand and we were talking about a separate command. What I'm personally looking for is the functions to simplify adding/removing roles/accounts, so the command is secondary to me. I prefer your suggestion over mine (dislike the verbs in my args), so I'll change this. |
@mumoshu wdyt about omitting the I reckon an argument against this would be the convention to use the first arg as cluster name seen in most commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall, just a few nits :)
@rndstr I don't mind merging it as is, and we can have a separate conversation about the CLI design. I think it would make sense to consider this:
But it also seems plausible to provide full create/get/delete, and perhaps we should think about treating it as a first-class resource, i.e.:
I'm not sure what's best, also the underlying details may change as EKS may finally move to a CRD instead of this configmap... so whatever we do now may have to change sooner or later. I'd really vote for keeping it as is for now. |
@errordeveloper what about
which elaborates on what kind of account or role this is. i first preferred
but I think a command to display the current version would be nice too, which would be |
On Tue, 12 Mar 2019, 9:20 pm Roli Schilter, ***@***.***> wrote:
@errordeveloper <https://github.com/errordeveloper> what about
eksctl <create|get|delete> auth-account
eksctl <create|get|delete> auth-role
which elaborates on what kind of account or role this is.
Yes, but it is odd because it breaks out a narrow concept of what is a single
object inside the cluster into several new concepts. Most resources we have
at the moment are the opposite, e.g. a cluster brings together great many
things, and so does a nodegroup (albeit to a lesser degree). And that config map always gets updated, so it only naturally maps to
the update verb, maybe get and update verbs are what we would actually want here? We do have verbs that are used with just certain resources, most notably 'eksctl scale ng'.
Perhaps when we add IAM as a resource (along with VPC), this could be
somehow part of that, or perhaps it will become clearer when we have
'eksclt create|get|delete cluster-iam|nodegroup-iam' (or whatever we decide
to call it).
I would rather merge this PR as is and continue design discussion elsewhere
:)
|
So my conclusion is that we either move along, merge it and review later
on, or move current implementation to 'eksctl update iam-auth-bindings'
(considering that other '*iam*' resources will materialise in the future).
Feel free to keep the flags as they are. If you feel like that would be the
right thing, 'eksclt get iam-auth-bindings' would be nice to have, of
course.
On Tue, 12 Mar 2019, 11:53 pm Ilya Dmitrichenko, <[email protected]>
wrote:
… On Tue, 12 Mar 2019, 9:20 pm Roli Schilter, ***@***.***>
wrote:
> @errordeveloper <https://github.com/errordeveloper> what about
>
> eksctl <create|get|delete> auth-account
> eksctl <create|get|delete> auth-role
>
> which elaborates on what kind of account or role this is.
>
Yes, it is odd cause it breaks out a narrow concept of what is a single
object inside the cluster into several new concepts. Most resources we have
at the moment are the opposite, e.g. a cluster brings together great many
things, and so does a nodegroup (albeit to a lesser degree).
Perhaps when we add IAM as a resource (along with VPC), this could be
somehow part of that, or perhaps it will become clearer when we have
'eksclt create|get|delete cluster-iam|nodegroup-iam' (or whatever we decide
to call it).
I would rather merge this PR as is and continue design discussion
elsewhere :)
|
Some refactoring around the authconfigmap while allowing to specify the groups when adding new roles.
The command `eksctl util -aws-auth` allows to manipulate the `aws-auth` configmap in `kube-system`. It supports the following actions - `--add-admin-role=<arn>` - `--remove-role=<arn>` - `--add-account=<number>` - `--remove-account=<number>`
Discussion about location and hierarchy deferred into issue.
@errordeveloper created #625 and removed the command itself for now |
hm... integration tests failed for me, seems like it's to do with this change?
@rndstr wdyt? |
Adds the command
to allow modifying the
kube-system/aws-auth
object which is a mapping from IAM entities to Kubernetes groups.