Add command to manipulate IAM identity mappings #814

rndstr · 2019-05-23T00:15:24Z

Description

Adds the following commands to get/create/delete IAM role mappings to
Kubernetes username and groups.

eksctl get iamidentitymapping [--role arn]
eksctl create iamidentitymapping --role <arn> [--username=USER] --group=GROUP0 [--group=GROUP1]
eksctl delete iamidentitymapping --role <arn> [--all]

eksctl get iamidentitymapping
Returns all mappings; if role filter is given it returns all matching
roles (can be more than one).

eksctl create iamidentitymapping
Allows creating duplicates. Will warn if duplicates exist.

eksctl delete iamidentitymapping
Deletes a single mapping FIFO unless --all is given in which case it
removes all matching. Will warn if more mappings matching this role are
found.

Closes #625
Some remarks about implementation at #625 (comment)

Checklist

Code compiles correctly (i.e make build)
Added tests that cover your change (if possible)
All unit tests passing (i.e. make test)
All integration tests passing (i.e. make integration-test)
Added/modified documentation as required (such as the README.md, and examples directory)

Example of interaction

$ eksctl get iamidentitymapping --cluster=roli-dev                                                                                      
ROLE                                                                                     USERNAME                                GROUPS                                       
arn:aws:iam::376248598259:role/eksctl-roli-dev-nodegroup-ng1-pub-NodeInstanceRole-5SO5IE system:node:{{EC2PrivateDNSName}}       system:bootstrappers,system:nodes            
arn:aws:iam::376248598259:role/eksctl-roli-dev-nodegroup-ng2-pub-NodeInstanceRole-4SVDI  system:node:{{EC2PrivateDNSName}}       system:bootstrappers,system:nodes            
arntest                                                                                  roli                                    foo1,bar2                                    
foo                                                                                                                              bar                                          
foo                                                                                                                              bar


$ eksctl delete iamidentitymapping --cluster=roli-dev --role foo
[ℹ]  removing role "foo" from auth ConfigMap (username = "", groups = ["bar"])
[!]  there are 1 mappings left with same role "foo" (use --all to delete them at once)

$ eksctl create iamidentitymapping --cluster=roli-dev --role foo --group=moo                                                            
[!]  found 1 mappings with same role "foo" (which will be shadowed by your new mapping)
[ℹ]  adding role "foo" to auth ConfigMap

$ eksctl get iamidentitymapping --cluster=roli-dev --role foo                                                                           
ROLE    USERNAME        GROUPS
foo                     bar
foo                     moo

$ eksctl delete iamidentitymapping --cluster=roli-dev --role foo --all                                                                  
[ℹ]  removing role "foo" from auth ConfigMap (username = "", groups = ["bar"])
[ℹ]  removing role "foo" from auth ConfigMap (username = "", groups = ["moo"])

martina-if · 2019-05-23T15:10:22Z

Looks good to me! Would it be possible to add some tests?

whereisaaron · 2019-05-23T16:11:55Z

@rndstr thanks for your work. This will be very useful.

Does this, or how is it suggested to use the commands for the following use cases. I am guessing some loops/tests based on exit codes. Is that the intention?

Ensure user x is in the map without creating a duplicate? (exit code from get?)
User x is mapped to group a, how to map to group b as well? (delete until error then create?)
User x is mapped to groups a, b, c, how to remove from group b? (delete until error then create?)
Ensure user x is not mapped? (keep deleteing until error code?)

Questions:

When there are duplicates in the map, will get for an arn get them all, or just one?
Does get (optionally) produce JSON or YAML output?
Does delete output or exit code indicate if it deleted anything?
Do you know how EKS processes duplicates? If two entries for a user do both group mappings apply, just the first?

rndstr · 2019-05-23T17:59:45Z

@whereisaaron Some good questions. (I added a link to #625 (comment) which might be helpful)

I am guessing some loops/tests based on exit codes. Is that the intention?

Yep, tests are still to come, just wanted to post this to kick off the discussion some more.

Do you know how EKS processes duplicates? If two entries for a user do both group mappings apply, just the first?

It should be the last, actually. The code is at https://github.com/kubernetes-sigs/aws-iam-authenticator/blob/c2d2884d23a3bc04fa8cc4d5b032022b38469104/pkg/server/server.go#L167-L174 and it will overwrite previous mappings with the same role.

When there are duplicates in the map, will get for an arn get them all, or just one?

Just one currently. I think returning them all might be better, though.

I did think about adding the following log messages

(any command): warn if there are duplicate roles with mismatching username/groups
delete: after removing it, inform there are more of those roles

Does get (optionally) produce JSON or YAML output?

Yes, -o yaml|json.

Does delete output or exit code indicate if it deleted anything?

Yes, it errors if it can't find the role.

I'm thinking about doing a write-up that should cover more questions that I can think of.

whereisaaron · 2019-05-23T18:23:36Z

When there are duplicates in the map, will get for an arn get them all, or just one?

Just one currently. I think returning them all might be better, though.

Yes please, and I suggest for the -o yaml | json that it always returns a list, even if just one result. It is easy to de-nest and preferable to have a consistent structure back to any invocation. Likewise, no match should return and empty list and maybe no error?

I did think about adding the following log messages
(any command): warn if there are duplicate roles with mismatching username/groups
delete: after removing it, inform there are more of those roles

Sounds good for interactive users, but I'd suggest those warnings not impact the exist code. Scripts can use another get to work this out.

delete should also warn interactively if no match found to delete, but maybe that isn't an exit code error either?

rndstr · 2019-05-23T19:24:28Z

Just one currently. I think returning them all might be better, though.

My initial thought was it's not going to be representative to what aws-iam-authenticator sees.

But since delete only deletes one, it probably becomes helpful for get --role <arn> to show all matches. But maybe show a hint here too?

Likewise, no match should return and empty list and maybe no error?

If a filter is given (get iamidentitymapping --role <arn>) then it should error to keep it consistent with get cluster --name <cluster>. But for get iamidentitymapping it won't error but just display an empty list.

Sounds good for interactive users, but I'd suggest those warnings not impact the exist code.

Yes, that log output would need to go to stderr or not log anything at all (could do log level debug if stderr is not an option). Not yet convinced this is a good idea, will see how we do things in other places.

delete should also warn interactively if no match found to delete, but maybe that isn't an exit code error either?

it should be an exit code error to be consistent with other eksctl delete <res> calls. (can use get & delete if you want to prevent an error.) also helps with surfacing typos.

rndstr · 2019-05-24T00:43:52Z

@whereisaaron @martina-if this is now ready for review in case you want to give it a go. Added tests and updated the PR description to reflect latest changes and an example of interaction.

rndstr · 2019-05-24T00:45:26Z

pkg/iam/mapping.go

+// Valid ensures the identity is proper.
+func (i Identity) Valid() error {
+	if len(i.Groups) == 0 {
+		return errors.New("identity mapping needs at least 1 group")


is this actually true? or is a role mapping valid with just a rolearn and username?

martina-if

Looks good! I left one minor comment

integration/creategetdelete_test.go

cpaika · 2019-05-25T15:59:32Z

This solves a big problem for us so much appreciated for setting this up - awesome.

When its merged can we use this functionality in the cluster.yaml file , or is it only supported in those three commands at first?
We're trying to keep our clusters configuration as declarative as possible currently, but totally can make do using those commands in our cluster pipeline setup so no big deal.

cpaika · 2019-05-28T20:20:56Z

For what its worth, I built this PR locally since I needed the functionality - everything works well for me 👍

rndstr · 2019-05-28T22:24:30Z

When its merged can we use this functionality in the cluster.yaml file , or is it only supported in those three commands at first?

It is commands only in this PR.

martina-if · 2019-05-29T10:41:27Z

LGTM 🎉 but let's wait for Ilya's review

errordeveloper

LGTM, overall. Would like take another look soon.

errordeveloper · 2019-06-03T13:47:21Z

@rndstr is it correct to assume that these new commands don't support --config-file yet? It'd be great to add it, I'd be happy to work on it as a follow-up, unless you'd rather take care of it.

rndstr · 2019-06-03T17:47:00Z

@errordeveloper I won't get to it until next week so feel free to do a follow-up.

Adds the following commands to get/create/delete IAM role mappings to Kubernetes username and groups. eksctl get iamidentitymapping [--role arn] eksctl create iamidentitymapping --role <arn> [--username=USER] --group=GROUP0 [--group=GROUP1] eksctl delete iamidentitymapping --role <arn> [--all] `eksctl get iamidentitymapping` Returns all mappings; if role filter is given it returns all matching roles (can be more than one). `eksctl create iamidentitymapping` Allows creating duplicates. Will warn if duplicates exist. `eksctl delete iamidentitymapping` Deletes a single mapping FIFO unless `--all` is given in which case it removes all matching. Will warn if more mappings matching this role are found.

rndstr · 2019-06-05T00:24:33Z

@errordeveloper rebased, needs another approval

patstrom · 2019-06-12T14:48:54Z

It might be a little late to bring this up but is there a specific reason why only IAM roles (in the mapRoles yaml key) are supported and not also IAM users (in the mapUsers yaml key)?

Just wanted to make sure this hasn't already been discussed outside Github before creating an issue.

errordeveloper · 2019-06-12T14:53:27Z

@patstrom I don't recall having a discussion about it, please open a new issue. It should be easy to add mapUsers.

nikolai-derzhak-distillery · 2019-06-20T16:33:02Z

I noticed that eksctl get iamidentitymapping does not work with assume-role in kube config (generated by aws --region $AWS_REGION eks update-kubeconfig --name $CLUSTER_NAME --role-arn $AWS_ROLE ) :

$ eksctl get iamidentitymapping --name=$CLUSTER_NAME
[✖]  getting auth ConfigMap: Unauthorized

while eksctl get nodegroup --cluster $CLUSTER_NAME works fine.

Also I can just edit aws-auth yaml like this:

kubectl edit -n kube-system configmap/aws-auth

Do you need ticket on this ?

Guess related to this feature request : #749

rndstr · 2019-06-20T17:37:54Z

@nikolai-derzhak-distillery a ticket would be great, thanks!

nikolai-derzhak-distillery · 2019-06-20T17:55:07Z

Here we go: #916 YW

neilharris123 · 2020-03-30T08:17:22Z

@rndstr is it correct to assume that these new commands don't support --config-file yet? It'd be great to add it, I'd be happy to work on it as a follow-up, unless you'd rather take care of it.

@errordeveloper, Hello, is this still in the pipeline? It would be really useful!

Use the old topology key for e2e tests

rndstr force-pushed the authconfigmap-command branch 2 times, most recently from 865d8c3 to 9562731 Compare May 23, 2019 03:47

rndstr force-pushed the authconfigmap-command branch 4 times, most recently from 4f24c08 to 6446eef Compare May 24, 2019 00:32

rndstr marked this pull request as ready for review May 24, 2019 00:33

rndstr force-pushed the authconfigmap-command branch from 6446eef to bab02fb Compare May 24, 2019 00:40

rndstr commented May 24, 2019

View reviewed changes

martina-if reviewed May 24, 2019

View reviewed changes

integration/creategetdelete_test.go Show resolved Hide resolved

rndstr force-pushed the authconfigmap-command branch 2 times, most recently from 76f4280 to b4ca9a4 Compare May 24, 2019 19:40

rndstr requested a review from errordeveloper May 28, 2019 22:34

errordeveloper reviewed Jun 3, 2019

View reviewed changes

errordeveloper previously approved these changes Jun 4, 2019

View reviewed changes

rndstr added 2 commits June 4, 2019 16:51

Remove unused interface

e2006cf

rndstr dismissed errordeveloper’s stale review via e2006cf June 4, 2019 23:51

rndstr force-pushed the authconfigmap-command branch from b4ca9a4 to e2006cf Compare June 4, 2019 23:51

rndstr requested a review from errordeveloper June 5, 2019 00:24

errordeveloper approved these changes Jun 5, 2019

View reviewed changes

errordeveloper merged commit 2841a38 into master Jun 5, 2019

errordeveloper deleted the authconfigmap-command branch June 5, 2019 16:12

timfallmk mentioned this pull request Jun 10, 2019

iamidentitymapping prints incorrect error message #863

Closed

nikolai-derzhak-distillery mentioned this pull request Jun 20, 2019

Add mapUsers support to iamidentitymapping command #876

Closed

torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022

Merge pull request eksctl-io#814 from ayberk/fix_e2e

19d0de2

Use the old topology key for e2e tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command to manipulate IAM identity mappings #814

Add command to manipulate IAM identity mappings #814

rndstr commented May 23, 2019 •

edited

Loading

martina-if commented May 23, 2019

whereisaaron commented May 23, 2019

rndstr commented May 23, 2019

whereisaaron commented May 23, 2019

rndstr commented May 23, 2019

rndstr commented May 24, 2019

rndstr May 24, 2019

martina-if left a comment

cpaika commented May 25, 2019 •

edited

Loading

cpaika commented May 28, 2019

rndstr commented May 28, 2019

martina-if commented May 29, 2019

errordeveloper left a comment

errordeveloper commented Jun 3, 2019

rndstr commented Jun 3, 2019

rndstr commented Jun 5, 2019

patstrom commented Jun 12, 2019

errordeveloper commented Jun 12, 2019

nikolai-derzhak-distillery commented Jun 20, 2019 •

edited

Loading

rndstr commented Jun 20, 2019

nikolai-derzhak-distillery commented Jun 20, 2019 •

edited

Loading

neilharris123 commented Mar 30, 2020 •

edited

Loading

Add command to manipulate IAM identity mappings #814

Add command to manipulate IAM identity mappings #814

Conversation

rndstr commented May 23, 2019 • edited Loading

Description

Checklist

Example of interaction

martina-if commented May 23, 2019

whereisaaron commented May 23, 2019

rndstr commented May 23, 2019

whereisaaron commented May 23, 2019

rndstr commented May 23, 2019

rndstr commented May 24, 2019

rndstr May 24, 2019

Choose a reason for hiding this comment

martina-if left a comment

Choose a reason for hiding this comment

cpaika commented May 25, 2019 • edited Loading

cpaika commented May 28, 2019

rndstr commented May 28, 2019

martina-if commented May 29, 2019

errordeveloper left a comment

Choose a reason for hiding this comment

errordeveloper commented Jun 3, 2019

rndstr commented Jun 3, 2019

rndstr commented Jun 5, 2019

patstrom commented Jun 12, 2019

errordeveloper commented Jun 12, 2019

nikolai-derzhak-distillery commented Jun 20, 2019 • edited Loading

rndstr commented Jun 20, 2019

nikolai-derzhak-distillery commented Jun 20, 2019 • edited Loading

neilharris123 commented Mar 30, 2020 • edited Loading

rndstr commented May 23, 2019 •

edited

Loading

cpaika commented May 25, 2019 •

edited

Loading

nikolai-derzhak-distillery commented Jun 20, 2019 •

edited

Loading

nikolai-derzhak-distillery commented Jun 20, 2019 •

edited

Loading

neilharris123 commented Mar 30, 2020 •

edited

Loading