Update iamserviceaccount role policies #3064

aclevername · 2021-01-12T12:56:49Z

Description

Adds support for updating the policies to an existing IAMServiceAccount.

The implementation flow is very similar to create, but rather than creating a stack & service account it just applys a change set to the existing stack.

Related issue: #1497

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), target version (e.g. version/0.12.0) 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 🌟

pkg/eks/tasks.go

aclevername · 2021-01-14T14:50:01Z

As part of moving everything into actions/iam the refactor was getting too large and messy for this PR, I'm going to split it out into another PR to make the diff easier to understand

aclevername · 2021-01-14T14:50:34Z

pkg/actions/iam/create.go

+package iam
+
+import (
+	api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
+	"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
+	"github.com/weaveworks/eksctl/pkg/kubernetes"
+)
+
+func (a *Manager) CreateIAMServiceAccount(iamServiceAccounts []*api.ClusterIAMServiceAccount, plan bool) error {
+	taskTree := a.stackManager.NewTasksToCreateIAMServiceAccounts(iamServiceAccounts, a.oidcManager, kubernetes.NewCachedClientSet(a.clientSet))
+	taskTree.PlanMode = plan
+
+	err := doTasks(taskTree)
+
+	cmdutils.LogPlanModeWarning(plan && len(iamServiceAccounts) > 0)
+
+	return err
+}


I've left this here for now, if folks want me to remove this small refactor and put it into the refactor PR let me know.

pkg/ctl/update/iamserviceaccount.go

Callisto13

LGTM 👍

michaelbeaumont · 2021-01-14T18:51:48Z

pkg/actions/iam/iam.go

+
+func doTasks(taskTree *tasks.TaskTree) error {
+	logger.Info(taskTree.Describe())
+	if errs := taskTree.DoAllSync(); len(errs) > 0 {


hmm doesn't this exist in some form in the tasks package?

I couldn't see anything 🤔

michaelbeaumont · 2021-01-14T18:58:39Z

pkg/actions/iam/tasks.go

+func (t *updateIAMServiceAccountTask) Do(errorCh chan error) error {
+	stackName := makeIAMServiceAccountStackName(t.clusterName, t.sa.Namespace, t.sa.Name)
+	go func() {
+		errorCh <- nil


I see this is done elsewhere in the code with tasks, is this necessary? I'm assuming something won't make progress but I feel like functions should be EITHER only returning synchronous errors OR only sending errors over a channel. Making a note to myself

Yeah I was unsure regarding this too. I don't know why we have two ways of returning errors, I understand that we need the channel for when the Do sends off a go routine and returns, but when its just a single process it could just return the err down the channel anyway, removing the need for the returning error completely

is this necessary

I believe we have to otherwise it will block?

michaelbeaumont · 2021-01-14T19:00:03Z

pkg/actions/iam/update.go

+		}
+
+		var templateBody manager.TemplateBody = template
+		taskTree := UpdateIAMServiceAccountTask(a.clusterName, iamServiceAccount, a.stackManager, templateBody)


Couldn't this whole loop body be done async inside the task?

I think the top part of the loop that checks if the SA exists should remain here to ensure it gets checked when --approve is false. Your right though the resource set part can get moved, I will move it.

michaelbeaumont

see above

michaelbeaumont · 2021-01-15T12:33:51Z

pkg/actions/iam/update.go

+
+		if len(stacks) == 0 {
+			logger.Info("Cannot update IAMServiceAccount %s/%s as it does not exist", iamServiceAccount.Namespace, iamServiceAccount.Name)
+			nonExistingSAs = append(nonExistingSAs, fmt.Sprintf("%s/%s", iamServiceAccount.Namespace, iamServiceAccount.Name))


shouldn't this be handled by the exclude/include filters?

~~I didn't spot that we used filtering for create iamserviceaccount, I'll update it so we use that here too~~ on second thought the filtering code looks to be a bit more complicated than needed, here we only care about stacks that exist and were listed, we don't need a more robust exclude/include functionality. I also like this business logic sitting here

pkg/actions/iam/update.go

moving into seperate PR This reverts commit 329a3ac.

cPu1

LGTM 🎉

pkg/actions/iam/update.go

Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>

old

pkg/actions/iam/tasks.go

aclevername added area/aws-iam kind/feature New feature or request labels Jan 13, 2021

aclevername commented Jan 14, 2021

View reviewed changes

pkg/eks/tasks.go Outdated Show resolved Hide resolved

aclevername commented Jan 14, 2021

View reviewed changes

aclevername force-pushed the update-iam branch from cd0a5e4 to 30085ac Compare January 14, 2021 14:55

aclevername marked this pull request as ready for review January 14, 2021 14:55

aclevername requested a review from a team January 14, 2021 14:55

Callisto13 reviewed Jan 14, 2021

View reviewed changes

pkg/ctl/update/iamserviceaccount.go Outdated Show resolved Hide resolved

Callisto13 approved these changes Jan 14, 2021

View reviewed changes

michaelbeaumont reviewed Jan 14, 2021

View reviewed changes

michaelbeaumont previously requested changes Jan 14, 2021

View reviewed changes

aclevername force-pushed the update-iam branch from ac117e6 to e5dcda9 Compare January 15, 2021 10:21

aclevername requested a review from michaelbeaumont January 15, 2021 10:22

michaelbeaumont reviewed Jan 15, 2021

View reviewed changes

aclevername force-pushed the update-iam branch from e5dcda9 to 698ef53 Compare January 15, 2021 15:13

michaelbeaumont reviewed Jan 15, 2021

View reviewed changes

pkg/actions/iam/update.go Outdated Show resolved Hide resolved

aclevername mentioned this pull request Jan 18, 2021

Fix assigning IPs to Windows pods #3067

Merged

7 tasks

michaelbeaumont mentioned this pull request Jan 18, 2021

Update iamserviceaccount for existing role #1785

Closed

aclevername mentioned this pull request Jan 18, 2021

Need update iamserviceaccount to be able to attach/detach Policies, without breaking running applications and cross-account Trust Relationships #1497

Closed

aclevername requested a review from michaelbeaumont January 18, 2021 17:12

aclevername force-pushed the update-iam branch from aea472b to 8fc8dcb Compare January 18, 2021 17:41

aclevername requested a review from cPu1 January 19, 2021 11:43

aclevername added 7 commits January 19, 2021 11:44

implement basic update iamserviceaccount

b03a113

use tasks for upgradE

9153a70

handle non-existing SA's gracefully during update

3768616

commit tasks

f768529

Remove redundant test

89ac242

add integration test for updating iamserviceaccount

2eeab55

move create into pkg/actions/iam and move tasks around

1886f9c

aclevername added 8 commits January 19, 2021 11:44

Revert "move create into pkg/actions/iam and move tasks around"

23b6df5

moving into seperate PR This reverts commit 329a3ac.

improve description

0e338f8

update cli flag description

471f21a

fix failing test

4d2b6ee

move resource set building into task creation

3c9a7f6

reduce cfn calls and minor refactors

4262557

map->set, map[string]string -> map[string]struct{}

e20e643

fix failing unit test

47c3b30

aclevername force-pushed the update-iam branch from 8fc8dcb to 47c3b30 Compare January 19, 2021 11:45

cPu1 approved these changes Jan 20, 2021

View reviewed changes

pkg/actions/iam/update.go Outdated Show resolved Hide resolved

pkg/actions/iam/update.go Outdated Show resolved Hide resolved

aclevername and others added 2 commits January 20, 2021 11:45

Grammar fix

9891acd

Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>

format list

8dfe824

Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>

aclevername force-pushed the update-iam branch from c0faf7c to 8dfe824 Compare January 20, 2021 11:48

Merge branch 'master' into update-iam

65d4c07

michaelbeaumont approved these changes Jan 22, 2021

View reviewed changes

Merge branch 'master' into update-iam

f303006

aclevername force-pushed the update-iam branch from 12586e0 to f303006 Compare January 22, 2021 11:04

aclevername merged commit d3c9f8b into master Jan 22, 2021

aclevername deleted the update-iam branch January 22, 2021 12:19

Legion2 reviewed Jan 23, 2021

View reviewed changes

pkg/actions/iam/tasks.go Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update iamserviceaccount role policies #3064

Update iamserviceaccount role policies #3064

aclevername commented Jan 12, 2021 •

edited

aclevername commented Jan 14, 2021

aclevername Jan 14, 2021 •

edited

Callisto13 left a comment

michaelbeaumont Jan 14, 2021

aclevername Jan 15, 2021

michaelbeaumont Jan 14, 2021 •

edited

aclevername Jan 15, 2021 •

edited

michaelbeaumont Jan 14, 2021

aclevername Jan 15, 2021

michaelbeaumont left a comment

michaelbeaumont Jan 15, 2021

aclevername Jan 15, 2021 •

edited

cPu1 left a comment

Update iamserviceaccount role policies #3064

Update iamserviceaccount role policies #3064

Conversation

aclevername commented Jan 12, 2021 • edited

Description

Checklist

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

aclevername commented Jan 14, 2021

aclevername Jan 14, 2021 • edited

Choose a reason for hiding this comment

Callisto13 left a comment

Choose a reason for hiding this comment

michaelbeaumont Jan 14, 2021

Choose a reason for hiding this comment

aclevername Jan 15, 2021

Choose a reason for hiding this comment

michaelbeaumont Jan 14, 2021 • edited

Choose a reason for hiding this comment

aclevername Jan 15, 2021 • edited

Choose a reason for hiding this comment

michaelbeaumont Jan 14, 2021

Choose a reason for hiding this comment

aclevername Jan 15, 2021

Choose a reason for hiding this comment

michaelbeaumont left a comment

Choose a reason for hiding this comment

michaelbeaumont Jan 15, 2021

Choose a reason for hiding this comment

aclevername Jan 15, 2021 • edited

Choose a reason for hiding this comment

cPu1 left a comment

Choose a reason for hiding this comment

aclevername commented Jan 12, 2021 •

edited

aclevername Jan 14, 2021 •

edited

michaelbeaumont Jan 14, 2021 •

edited

aclevername Jan 15, 2021 •

edited

aclevername Jan 15, 2021 •

edited