Fix assigning IPs to Windows pods #3067

cPu1 · 2021-01-13T11:33:41Z

Description

The VPC controller requires a subset of the IAM policies of the VPC CNI plugin to work. In a cluster with no OIDC provider, the policies required by the CNI plugin are attached to the node role, so the VPC resource controller ends up using it. When a cluster has an OIDC provider (iam.withOIDC), however, eksctl uses IRSA for the VPC CNI plugin, causing the VPC controller to be unable to access AWS APIs.

This PR makes the VPC controller use IRSA if an OIDC provider is associated with the cluster, attaching only the policies required by the VPC controller instead of using the more permissive AmazonEKS_CNI_Policy managed policy.

Fixes #2721

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/cfn/manager/iam.go

pkg/addons/vpc_controller.go

Callisto13 · 2021-01-14T13:37:15Z

pkg/addons/irsa_helper.go

+type serviceAccountCreator interface {
+	NewTasksToCreateIAMServiceAccounts(serviceAccounts []*api.ClusterIAMServiceAccount, oidc *iamoidc.OpenIDConnectManager, clientSetGetter kubernetes.ClientSetGetter, replaceExistingRole bool) *tasks.TaskTree
+}


Can I ask why we have this little interface here?

It took me fractionally longer to parse the composition than if stackcollection (or stackmanager, whatever it is) were simply called from the helper struct on line 51, and I have a preference for code which lets me be lazy 😉

Because irsaHelper only needs this method and not the entire StackCollection. This makes testing the VPCController and IRSAHelper easier. It's somewhat similar to saying why, e.g, ioutil.ReadAll takes an io.Reader and not an os.File or a net.TCPConn.

Oh I couldn't find tests in this PR (I did assume that to start), are they still coming?

I meant that as a general comment. I haven't added tests as the bulk of the logic is making Kubernetes and AWS calls.

aclevername · 2021-01-15T15:25:39Z

pkg/cfn/manager/iam.go

+			logger.Debug("CFN stack for IRSA already exists, replacing it with a new stack")
+			if err := c.DeleteStackByNameSync(name); err != nil {
+				close(errs)
+				return errors.Wrap(err, "error deleting stack")
+			}
+			return c.createIAMServiceAccountTask(errs, spec, oidc, false)


Am I right in thinking that VPC controller installation only occurs during cluster creation? If so then when will the scenario occur where the SA already exists occur?

If so then when will the scenario occur where the SA already exists occur?

The VPC controller can be installed on an existing cluster using eksctl utils install-vpc-controllers.

So is the scenario here when a user runs eksctl utils install-vpc-controllers after a cluster installation that already installed the VPC controller? If so why do we need to delete & recreate, can't just check for its existence and exit early if it does?

It is to allow applying any changes to the policy document. If we just check for existence, any changes made to the policy document in code will not be applied.

but is it needed for this PR? I'm just trying to understand when this codepath is used in relation to vpc-controller 😄

It might be that this isn't needed as we will add the ability to update IAM policies without deleting here #3064

I'm just trying to understand when this codepath is used in relation to vpc-controller

This codepath is invoked when utils install-vpc-controllers is run on a cluster that already has the VPC controller installed (either as part of create cluster or with utils install-vpc-controllers itself). The ServiceAccount created for the role needs the role ARN, so if we skip creation of this stack if it already exists, we'll have to fetch it and get the role ARN, and any changes to the policy document will be ignored, causing the VPC controller to misbehave.

Ah I see, so is this just for the scenario in which: I have an already created eksctl cluster with oidc enabled and the VPC controller installed, but its missing the policies because I created it with an older version of eksctl? If so that makes sense 😄

Apart from that scenario you'd never need to run utils install-vpc-controllers if its already installed correct?

Ah I see, so is this just for the scenario in which: I have an already created eksctl cluster with oidc enabled and the VPC controller installed, but its missing the policies because I created it with an older version of eksctl?

That's right.

Apart from that scenario you'd never need to run utils install-vpc-controllers if its already installed correct?

Users may also run it to upgrade to a newer version of the VPC controller.

Thanks! Makes sense. Does this add more downtime for users when they update the VPC controller as we will always delete and recreate the policies even if they are unchanged? Very minor, but it could be removed by using the update functionality #3064

Does this add more downtime for users when they update the VPC controller as we will always delete and recreate the policies even if they are unchanged?

Yes.

Very minor, but it could be removed by using the update functionality #3064

Yeah, the ideal way would be to update the policies in the existing stack, but since we didn't already have code that updates the policies for IRSA, I went with recreating it. I was aware of your PR but it wasn't in master when I was working on this PR. After your PR is merged, I can change this code to use the update feature (or it can be done as an improvement later).

aclevername · 2021-01-15T15:31:09Z

The VPC controller requires a subset of the IAM policies of the VPC CNI plugin to work. In a cluster with no OIDC provider, the policies required by the CNI plugin are attached to the node role, so the VPC resource controller ends up using it

is there a way we could make it more explicit in the code that the VPC Controller also uses/needs this policy when OIDC is disabled? It feels almost accidental that the VPC Controller worked

cPu1 · 2021-01-18T09:11:35Z

The VPC controller requires a subset of the IAM policies of the VPC CNI plugin to work. In a cluster with no OIDC provider, the policies required by the CNI plugin are attached to the node role, so the VPC resource controller ends up using it

is there a way we could make it more explicit in the code that the VPC Controller also uses/needs this policy when OIDC is disabled? It feels almost accidental that the VPC Controller worked

There's little value in attaching the VPC controller-specific policies to the node role because they're a subset of the policies required by the VPC CNI plugin. Although I agree that a comment documenting this behaviour would be useful.

michaelbeaumont

My changes addressed, LGTM

aclevername

LGTM

cPu1 added kind/bug area/aws-iam area/add-ons labels Jan 13, 2021

cPu1 force-pushed the windows-ip-2721 branch from e57ebf2 to 776d2a3 Compare January 13, 2021 12:06

michaelbeaumont reviewed Jan 13, 2021

View reviewed changes

pkg/cfn/manager/iam.go Outdated Show resolved Hide resolved

pkg/cfn/manager/iam.go Show resolved Hide resolved

cPu1 force-pushed the windows-ip-2721 branch 3 times, most recently from 0e6a131 to a291d39 Compare January 14, 2021 12:15

michaelbeaumont reviewed Jan 14, 2021

View reviewed changes

pkg/addons/vpc_controller.go Outdated Show resolved Hide resolved

Callisto13 reviewed Jan 14, 2021

View reviewed changes

cPu1 force-pushed the windows-ip-2721 branch 2 times, most recently from aa6ab08 to dee47f0 Compare January 15, 2021 12:51

aclevername reviewed Jan 15, 2021

View reviewed changes

cPu1 force-pushed the windows-ip-2721 branch 4 times, most recently from 4af15e4 to 1b0e72e Compare January 20, 2021 11:11

cPu1 changed the base branch from master to release-0.36 January 20, 2021 12:33

cPu1 force-pushed the windows-ip-2721 branch 2 times, most recently from a43e624 to fab451f Compare January 20, 2021 12:34

michaelbeaumont approved these changes Jan 20, 2021

View reviewed changes

cPu1 added 7 commits January 20, 2021 19:11

Fix typos

e949f2a

Use IRSA for VPC resource controller

2877858

Export and reuse VPCControllerTask

88f432f

Make installing VPC controller idempotent

7c10906

Generate assets

1d17445

Fix lint error

ac84b61

Extract out policy document

c2ea76d

cPu1 added 2 commits January 20, 2021 19:11

Document use of CNI plugin policy by the VPC controller

1522338

Generate assets

7abdd59

cPu1 force-pushed the windows-ip-2721 branch from d4a6e2b to 7abdd59 Compare January 20, 2021 13:41

aclevername approved these changes Jan 20, 2021

View reviewed changes

cPu1 merged commit ac55ba7 into eksctl-io:release-0.36 Jan 20, 2021

cPu1 deleted the windows-ip-2721 branch January 20, 2021 13:57

aclevername mentioned this pull request Feb 17, 2021

New Windows nodes unable to assign IP to pods #2721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix assigning IPs to Windows pods #3067

Fix assigning IPs to Windows pods #3067

cPu1 commented Jan 13, 2021 •

edited

Callisto13 Jan 14, 2021 •

edited

cPu1 Jan 14, 2021

Callisto13 Jan 14, 2021

cPu1 Jan 14, 2021

aclevername Jan 15, 2021

cPu1 Jan 18, 2021

aclevername Jan 18, 2021

cPu1 Jan 18, 2021 •

edited

aclevername Jan 18, 2021

cPu1 Jan 18, 2021 •

edited

aclevername Jan 18, 2021

cPu1 Jan 18, 2021

aclevername Jan 18, 2021

cPu1 Jan 18, 2021

aclevername commented Jan 15, 2021

cPu1 commented Jan 18, 2021

michaelbeaumont left a comment

aclevername left a comment

Fix assigning IPs to Windows pods #3067

Fix assigning IPs to Windows pods #3067

Conversation

cPu1 commented Jan 13, 2021 • edited

Description

Checklist

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

Callisto13 Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cPu1 Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cPu1 Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aclevername commented Jan 15, 2021

cPu1 commented Jan 18, 2021

michaelbeaumont left a comment

Choose a reason for hiding this comment

aclevername left a comment

Choose a reason for hiding this comment

cPu1 commented Jan 13, 2021 •

edited

Callisto13 Jan 14, 2021 •

edited

cPu1 Jan 18, 2021 •

edited

cPu1 Jan 18, 2021 •

edited