Prevent duplication, and add flag to disable shadowing of IAM Identity Mappings #4963

DameonSmith · 2022-03-16T23:19:53Z

Description

I implemented functionality that will prevent duplication of aws_auth entries. The goal of this was to make the eksctl create iamidentitymapping command more automation friendly. I had been using this command in some IAC code and ended up having a large number of copies of the exact same record.

This involved changes in 2 areas primarily:

The create command itself to exit when a duplicate Identity is found.
Creation of a compare function for Identities.

This might be more robust if the Identity groups were sorted before being compared. Wasn't sure if it made sense to implement that way.

I also added a flag to the eksctl create iamidentitymapping called --no-shadow when this flag is set the command will error should there be an Identity using the same ARN already in place. The goal of this change is to be able to provide better guarantees that configurations aren't accidentally changed.

Let me know if there's anything here that doesn't match the projects style or format, or if I'm just contributing to technical debt by working it in the way that I did.

Also as a side note the tests ran extremely slowly on my machine (1-2 second per package), is that expected behavior?

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the userdocs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes
(Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

… mapping.

…wo auth records with the same Arn

Skarlso · 2022-03-17T08:07:53Z

Hi!

Thank you for your contribution! :)

Also as a side note the tests ran extremely slowly on my machine (1-2 second per package), is that expected behavior?

Unit tests? All tests? Hmm, they can be slow, since they set up a lot of mocks and extra things and a fake http router potentially and a kubernetes client... :D A lot of things.

I'll leave review comments in code.

pkg/ctl/create/iamidentitymapping.go

pkg/iam/mapping.go

Skarlso

Add tests and code to check unsorted groups are equal.

…with same elements are identical.

Skarlso

LGTM

aclevername

excellent tests ✨ lgtm!

Skarlso · 2022-04-06T15:27:13Z

pkg/ctl/create/iamidentitymapping.go

@@ -133,6 +137,15 @@ func doCreateIAMIdentityMapping(cmd *cmdutils.Cmd, options iamIdentityMappingOpt
 		for _, identity := range identities {
 			arn := identity.ARN()

+			if iam.CompareIdentity(id, identity) {


Hey @DameonSmith!

This check here, should have been behind the below if, right? :D :D

I think we missed that. :)

Or rather, I think this also needs the && options.NoDuplicateArns.

This check was to just ignore any duplicated identity entirely. Regardless of whether or not the duplicate arn flags was set.
I was trying to avoid having this kind of pollution:

arn: arn1, namespace: default, groups: [group1, group2] arn: arn1, namespace: default, groups: [group1, group2] arn: arn1, namespace: default, groups: [group2, group1] etc...

The context was specifically using eskctl in pipelines and IaC to create default roles and users for a cluster. The pipeline was run multiple times and would just input the same config over and over again. My assumption was that if a record was in the aws_auth config map it should just be ignored without an error as there's no functional difference.

The NoDuplicateArns flag was meant to prevent silent shadowing of existing roles with different permissions. I wanted to ability to have better visibility on something like the "ReadOnlyUsers" role accidentally got the masters group added to it.

Hopefully that makes sense, and there's not something about my assumptions that were mistaken. Let me know if I need to fix something.

But we specifically allow duplicate entries. We even have an integration test for it that was now failing. 🤔

Oh I didn't realize that was a feature, Is there a reason to allow duplicates?

Either way I can add that flag as a requirement if that's what you'd like. Should I open another PR or is there some way to open this one?

Yeah opened one here #5080 :)

…y Mappings (eksctl-io#4963) * Adding functionality to compare when iam Identity values are equal. * Adding check to skip identitcal iam Identities when creating identity mapping. * Added a the flag '--no-shadow' to throw error when trying to create two auth records with the same Arn * Updating Identity comparison function to verify that unsorted groups with same elements are identical. * Fixing formatting recommended in PR. * Changing name of --no-shadow flag to --no-duplicate-arns Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

DameonSmith added 3 commits March 16, 2022 15:22

Adding functionality to compare when iam Identity values are equal.

f09f016

Adding check to skip identitcal iam Identities when creating identity…

95c4290

… mapping.

Added a the flag '--no-shadow' to throw error when trying to create t…

20aff12

…wo auth records with the same Arn

Skarlso added the kind/feature New feature or request label Mar 17, 2022

Skarlso requested a review from a team March 17, 2022 11:27

Skarlso reviewed Mar 17, 2022

View reviewed changes

pkg/ctl/create/iamidentitymapping.go Outdated Show resolved Hide resolved

Skarlso reviewed Mar 17, 2022

View reviewed changes

pkg/iam/mapping.go Outdated Show resolved Hide resolved

Skarlso reviewed Mar 17, 2022

View reviewed changes

pkg/iam/mapping.go Show resolved Hide resolved

Skarlso suggested changes Mar 17, 2022

View reviewed changes

DameonSmith added 3 commits March 17, 2022 08:37

Updating Identity comparison function to verify that unsorted groups …

5dcf149

…with same elements are identical.

Fixing formatting recommended in PR.

fe09f9b

Changing name of --no-shadow flag to --no-duplicate-arns

50d06a9

Skarlso approved these changes Mar 17, 2022

View reviewed changes

Skarlso requested a review from a team March 17, 2022 20:26

Merge branch 'main' into iam-mapping-shadow-improvements

89c7c74

aclevername approved these changes Apr 6, 2022

View reviewed changes

Merge branch 'main' into iam-mapping-shadow-improvements

be41c63

Skarlso enabled auto-merge (squash) April 6, 2022 09:29

Skarlso merged commit d847830 into eksctl-io:main Apr 6, 2022

Skarlso reviewed Apr 6, 2022

View reviewed changes

This was referenced Apr 6, 2022

Fix iamserviceaccount create duplicate entry check not being behind the created flag #5080

Merged

[Bug] #5081

Closed

hspencer77 mentioned this pull request Jul 8, 2022

Upgrade eksctl brave-intl/eksctl#44

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplication, and add flag to disable shadowing of IAM Identity Mappings #4963

Prevent duplication, and add flag to disable shadowing of IAM Identity Mappings #4963

DameonSmith commented Mar 16, 2022 •

edited by Skarlso

Loading

Skarlso commented Mar 17, 2022

Skarlso left a comment

Skarlso left a comment

aclevername left a comment

Skarlso Apr 6, 2022

Skarlso Apr 6, 2022

DameonSmith Apr 6, 2022

Skarlso Apr 6, 2022

DameonSmith Apr 6, 2022

Skarlso Apr 6, 2022

Prevent duplication, and add flag to disable shadowing of IAM Identity Mappings #4963

Prevent duplication, and add flag to disable shadowing of IAM Identity Mappings #4963

Conversation

DameonSmith commented Mar 16, 2022 • edited by Skarlso Loading

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Skarlso commented Mar 17, 2022

Skarlso left a comment

Choose a reason for hiding this comment

Skarlso left a comment

Choose a reason for hiding this comment

aclevername left a comment

Choose a reason for hiding this comment

Skarlso Apr 6, 2022

Choose a reason for hiding this comment

Skarlso Apr 6, 2022

Choose a reason for hiding this comment

DameonSmith Apr 6, 2022

Choose a reason for hiding this comment

Skarlso Apr 6, 2022

Choose a reason for hiding this comment

DameonSmith Apr 6, 2022

Choose a reason for hiding this comment

Skarlso Apr 6, 2022

Choose a reason for hiding this comment

DameonSmith commented Mar 16, 2022 •

edited by Skarlso

Loading