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.

Sign up for GitHub

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

Jump to bottom

Refactor iamserviceaccounts #3135

Merged
merged 8 commits into from Feb 1, 2021
Merged

Refactor iamserviceaccounts #3135

merged 8 commits into from Feb 1, 2021

Conversation

aclevername
Copy link
Contributor

@aclevername aclevername commented Jan 22, 2021 •
edited

Description

This PR was just intended to be a refactor, but in the process found a few bugs as shown.

Get

Previous output

Previously get specifying just --name was broken, e.g.:

$ eksctl get iamserviceaccount --cluster jk --name baz
NAMESPACE       NAME    ROLE ARN
baz             baz     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-baz-Role1-A1PVCNCBI0WX
baz             foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-foo-Role1-1SDQ5H4X68B2P
default         bar     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-ba-Role1-1CPB5MICLNMXR
default         foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-fo-Role1-SUWVUTGQKJX7

Also when the namespace didn't exist you would get strange output:

eksctl get iamserviceaccount --cluster jk --namespace not-existing
Error: unable to append include globs in namespace "not-existing"

When specifying not-existing namespace and a name it would just return everything:

eksctl get iamserviceaccount --cluster jk --namespace not-existing --name foo
NAMESPACE       NAME    ROLE ARN
baz             baz     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-baz-Role1-A1PVCNCBI0WX
baz             foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-foo-Role1-1SDQ5H4X68B2P
default         bar     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-ba-Role1-1CPB5MICLNMXR
default         foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-fo-Role1-SUWVUTGQKJX7

New output

$ ./eksctl get iamserviceaccount --cluster jk
[ℹ]  eksctl version 0.37.0-dev+a487044b.2021-01-22T14:30:12Z
[ℹ]  using region us-west-2
NAMESPACE       NAME    ROLE ARN
baz             baz     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-baz-Role1-A1PVCNCBI0WX
baz             foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-foo-Role1-1SDQ5H4X68B2P
default         bar     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-ba-Role1-1CPB5MICLNMXR
default         foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-fo-Role1-SUWVUTGQKJX7

$ ./eksctl get iamserviceaccount --cluster jk --name foo
[ℹ]  eksctl version 0.37.0-dev+a487044b.2021-01-22T14:30:12Z
[ℹ]  using region us-west-2
NAMESPACE       NAME    ROLE ARN
baz             foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-foo-Role1-1SDQ5H4X68B2P
default         foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-default-fo-Role1-SUWVUTGQKJX7

$ ./eksctl get iamserviceaccount --cluster jk --namespace baz
[ℹ]  eksctl version 0.37.0-dev+a487044b.2021-01-22T14:30:12Z
[ℹ]  using region us-west-2
NAMESPACE       NAME    ROLE ARN
baz             baz     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-baz-Role1-A1PVCNCBI0WX
baz             foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-foo-Role1-1SDQ5H4X68B2P

$ ./eksctl get iamserviceaccount --cluster jk --name foo --namespace baz
[ℹ]  eksctl version 0.37.0-dev+a487044b.2021-01-22T14:30:12Z
[ℹ]  using region us-west-2
NAMESPACE       NAME    ROLE ARN
baz             foo     arn:aws:iam::<redcated>:role/eksctl-jk-addon-iamserviceaccount-baz-foo-Role1-1SDQ5H4X68B2P

$ ./eksctl get iamserviceaccount --cluster jk --name not-existing --namespace defo-not-existing
[ℹ]  eksctl version 0.37.0-dev+a487044b.2021-01-22T14:30:12Z
[ℹ]  using region us-west-2
No iamserviceaccounts found

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 🌟

@aclevername aclevername self-assigned this Jan 22, 2021
@aclevername aclevername mentioned this pull request Jan 25, 2021
Merged
7 tasks
@aclevername aclevername force-pushed the refactor-iam branch 3 times, most recently from 991012f to d58d516 Compare January 25, 2021 11:59
@aclevername aclevername marked this pull request as ready for review January 25, 2021 11:59
@aclevername aclevername requested a review from a team January 25, 2021 12:01
@aclevername
Copy link
Contributor Author

aclevername commented Jan 25, 2021 •
edited

⚠️ don't merge yet! all good now

michaelbeaumont
michaelbeaumont reviewed Jan 25, 2021
View reviewed changes
pkg/actions/iam/get.go Outdated Show resolved Hide resolved
pkg/ctl/get/iamserviceaccount.go
saFilter.AppendIncludeNames(serviceAccount.NameString())
} else if cmd.CobraCommand.Flag("namespace").Changed { // only namespace was given
notFoundErr = fmt.Errorf("no iamserviceaccounts found in namespace %q", serviceAccount.Namespace)
err = saFilter.AppendIncludeGlobs(remoteServiceAccounts, serviceAccount.Namespace+"/*")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean globs aren't supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think this has removed that functionality, i'm happy to bring it back.

Before I do that I think its worth noting that the --help output shows: --namespace string namespace to look for iamserviceaccount (default "default"), so I'm not sure if anyone ever knew it was even supported. I'm sort of open to just not adding it back in 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can probably live without it 🤷‍♀️

@aclevername aclevername force-pushed the refactor-iam branch 2 times, most recently from ee8b94a to ec916ba Compare January 25, 2021 14:53
@aclevername
Copy link
Contributor Author

/rebase

Callisto13
Callisto13 reviewed Feb 1, 2021
View reviewed changes
pkg/actions/irsa/irsa.go Show resolved Hide resolved
pkg/actions/irsa/get.go Show resolved Hide resolved
pkg/ctl/get/iamserviceaccount.go Outdated Show resolved Hide resolved
Callisto13
Callisto13 approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@Callisto13 Callisto13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

michaelbeaumont
michaelbeaumont suggested changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit

@weaveworksbot weaveworksbot merged commit 1645925 into master Feb 1, 2021
@weaveworksbot weaveworksbot deleted the refactor-iam branch February 1, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelbeaumont michaelbeaumont michaelbeaumont approved these changes

@Callisto13 Callisto13 Callisto13 approved these changes

@cPu1 cPu1 Awaiting requested review from cPu1

Assignees

@aclevername aclevername

Labels
kind/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aclevername @michaelbeaumont @Callisto13 @weaveworksbot