adds instanceProfileName field to nodegroups #1496

jdavredbeard · 2024-11-18T14:20:05Z

Proposed changes

Add instanceProfileName as an Input<string> field to NodeGroup and NodeGroupV2. This would unblock pulumi/pulumi-self-hosted-installers#181 by creating a path for users to create an InstanceProfile in a separate stack from the NodeGroup and pass the instance profile's name by stack reference. Currently pulumi/pulumi#17515, prevents NodeGroup components from accepting InstanceProfile values created in another stack and provided to the NodeGroup's stack by aws.iam.InstanceProfile.get(), and the instanceProfile parameter cannot accept an InstanceProfile resource passed by stack reference because the instanceProfile parameter is a plain InstanceProfile type, not an Input<InstanceProfile>.

Related issues (optional)

Closes #1497

github-actions · 2024-11-18T14:23:41Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

flostadler

Left some comments. Love the unit tests! But it would be great if we could add an integration test for this as well.

nodejs/eks/cluster.ts

flostadler · 2024-11-19T22:07:34Z

nodejs/eks/nodegroup.ts

@@ -652,19 +653,54 @@ export function createNodeGroup(
    return createNodeGroupInternal(name, args, pulumi.output(core), parent, provider);
 }

+export function resolveOrGetInstanceProfile(


Could you please add function docs explaining what this function is exactly doing and when it's throwing errors. Thanks!

nodejs/eks/nodegroup.ts

flostadler · 2024-11-19T22:11:53Z

nodejs/eks/yarn.lock

@@ -4,28 +4,28 @@

 "@ampproject/remapping@^2.2.0":
  version "2.3.0"
-  resolved "https://registry.yarnpkg.com/@ampproject/remapping/-/remapping-2.3.0.tgz#ed441b6fa600072520ce18b43d2c8cc8caecc7f4"


These seem to be unrelated changes

flostadler · 2024-11-19T22:19:24Z

nodejs/eks/nodegroup.ts

+    if (args.instanceProfile || c.nodeGroupOptions.instanceProfile) {
+        return args.instanceProfile ?? c.nodeGroupOptions.instanceProfile!;
+    }
+    return aws.iam.InstanceProfile.get(


I think pulling in the resource with the .get function would incur RUM costs. We circumvent this by using the data source instead.

It seems like we're only interested in the ARN, right? Instance Profile ARNs follow this format: arn:aws:iam::<account-id>:instance-profile/<profile-name>.
If we know the name we can construct it without having to load the resource.

In that case we need other props, we could use iam.getInstanceProfileOutput instead.

flostadler · 2024-11-19T22:20:23Z

provider/cmd/pulumi-gen-eks/main.go

+		},
+		"instanceProfileName": {
+			TypeSpec:    schema.TypeSpec{Type: "string"},
+			Description: "The name of the IAM InstanceProfile to use on the NodeGroup.",


We should point out here that instanceProfile and instanceProfileName is mutually exclusive.

flostadler · 2024-11-21T15:40:30Z

nodejs/eks/nodegroup.ts

    name: string,
    args: Omit<NodeGroupOptions, "cluster">,
-    c: pulumi.UnwrappedObject<CoreData>,


Why did you have to make this change?

flostadler · 2024-11-21T15:42:10Z

nodejs/eks/nodegroup.ts

-        resolveOrGetInstanceProfile(name, args, c, parent, provider),
-    );
-
+    const instanceProfileName = resolveInstanceProfileName(name, args, core, parent)


Why not do core.apply(c => resolveInstanceProfileName(...)) instead like before?
That way resolveInstanceProfileName can stay out of the output-space and operate on promptly available data instead. That would simplify testing

The function would change to this in that case:

export function resolveInstanceProfileName( nodeGroupName: string, args: Omit<NodeGroupOptions, "cluster">, c: pulumi.UnwrappedObject<CoreData>, parent: pulumi.ComponentResource, ): pulumi.Output<string> { if ( !args.instanceProfile && !args.instanceProfileName && !c.nodeGroupOptions.instanceProfile && !c.nodeGroupOptions.instanceProfileName ) { throw new pulumi.ResourceError( `an instanceProfile or instanceProfileName is required`, parent, ); } if ( (args.instanceProfile || c.nodeGroupOptions.instanceProfile) && (args.instanceProfileName || c.nodeGroupOptions.instanceProfileName) ) { throw new pulumi.ResourceError( `invalid args for node group ${name}, instanceProfile and instanceProfileName are mutually exclusive`, parent, ); } return args.instanceProfile?.name ?? pulumi.output(c.nodeGroupOptions.instanceProfileName!) }

I'd also slightly change the definedness checks to check for all possible cases:

export function resolveInstanceProfileName( name: string, args: Omit<NodeGroupOptions, "cluster">, c: pulumi.UnwrappedObject<CoreData>, parent: pulumi.ComponentResource, ): pulumi.Output<string> { // todo check the mutual exclusive'ness if (args.instanceProfileName) { return pulumi.output(args.instanceProfileName); } else if (c.nodeGroupOptions.instanceProfileName) { return pulumi.output(c.nodeGroupOptions.instanceProfileName); } else if (args.instanceProfile) { return args.instanceProfile.name; } else if (c.nodeGroupOptions.instanceProfile) { return c.nodeGroupOptions.instanceProfile.name; } else { throw new pulumi.ResourceError( `an instanceProfile or instanceProfileName is required`, parent, ); } }

flostadler · 2024-11-21T15:43:10Z

nodejs/eks/nodegroup.ts

-        (args.instanceProfile || c.nodeGroupOptions.instanceProfile) &&
-        (args.instanceProfileName || c.nodeGroupOptions.instanceProfileName)
-    ) {
+): pulumi.Output<string> | undefined {


Shouldn't this always return a value?

hema, updates sdk, adds acceptance test

flostadler · 2024-11-25T13:38:20Z

tests/testdata/programs/self-managed-ng-instanceProfile/index.ts

+    }
+  },
+  // Needed for AL2023 until the CNI is managed with an addon because our custom CNI is too old
+  useDefaultVpcCni: true,


This can actually not needed anymore. I guess this is copied from

pulumi-eks/tests/testdata/programs/self-managed-ng-os/index.ts

Line 32 in a1061b6

// Needed for AL2023 until the CNI is managed with an addon because our custom CNI is too old

?

I'll remove it there as well

flostadler · 2024-11-25T13:49:12Z

tests/testdata/programs/self-managed-ng-instanceProfile/index.ts

+});
+
+export const passedInstanceProfileName = instanceProfile.name
+export const kubeconfig = cluster_NoIp_NoIpn.kubeconfig;


You need to export the kubeconfig of all clusters and run acceptance tests using those. Right now only one cluster is tested.

The test is creating a lot of clusters and node groups right now. Aren't the unit tests already checking that we pick the right option for instance profile?
I think it would be good enough if you added a single cluster and two node groups (v1/v2) using the instanceProfileName arg

Whoops missed exporting the other kubeconfigs... sure I can reduce the scope of the tests, I was going for a more completionist approach

flostadler

Nice! Thanks a lot

pulumi-bot · 2024-11-26T14:18:39Z

This PR has been shipped in release v3.3.0.

adds instanceProfileName field to nodegroups

d777d06

jdavredbeard marked this pull request as draft November 18, 2024 15:15

Jonathan Davenport added 4 commits November 18, 2024 10:25

adds handling for instanceProfileName to cluster and nodegroup

a5b770e

linting

db2fac1

more linting

f116756

adds unit tests, factors logic into function for unit testing

4adbb6b

jdavredbeard marked this pull request as ready for review November 19, 2024 14:39

makes better use of mocks

0f7d0d2

jdavredbeard requested review from corymhall, flostadler and rquitales November 19, 2024 21:53

flostadler reviewed Nov 19, 2024

View reviewed changes

rewrites as resolveInstanceProfileName

8ef3f3b

flostadler reviewed Nov 21, 2024

View reviewed changes

Jonathan Davenport added 4 commits November 21, 2024 11:50

cleans up function and unit tests

8766cbd

more cleanup

1d7de89

updates property doc in sc

abc9633

hema, updates sdk, adds acceptance test

restores yarn.lock

51681ce

jdavredbeard requested a review from flostadler November 23, 2024 00:04

flostadler reviewed Nov 25, 2024

View reviewed changes

removes extraneous nodegroup/v2 test cases

4139df2

jdavredbeard requested a review from flostadler November 25, 2024 15:56

flostadler approved these changes Nov 25, 2024

View reviewed changes

flostadler merged commit 230eba5 into master Nov 25, 2024
34 checks passed

flostadler deleted the jdavredbeard/addInstanceProfileName branch November 25, 2024 17:03

jdavredbeard mentioned this pull request Nov 25, 2024

upgrade to eks 3.0.1 pulumi/pulumi-self-hosted-installers#181

Open

This was referenced Nov 26, 2024

Add instanceProfileName resource property to NodeGroup/V2 #1497

Closed

Workflow failure: master #1234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds instanceProfileName field to nodegroups #1496

adds instanceProfileName field to nodegroups #1496

jdavredbeard commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024

flostadler left a comment

flostadler Nov 19, 2024

flostadler Nov 19, 2024

flostadler Nov 19, 2024

flostadler Nov 19, 2024

flostadler Nov 21, 2024

flostadler Nov 21, 2024

flostadler Nov 21, 2024 •

edited

Loading

flostadler Nov 21, 2024 •

edited

Loading

flostadler Nov 21, 2024

flostadler Nov 25, 2024

flostadler Nov 25, 2024

jdavredbeard Nov 25, 2024

flostadler left a comment

pulumi-bot commented Nov 26, 2024

adds instanceProfileName field to nodegroups #1496

adds instanceProfileName field to nodegroups #1496

Conversation

jdavredbeard commented Nov 18, 2024 • edited Loading

Proposed changes

Related issues (optional)

github-actions bot commented Nov 18, 2024

Does the PR have any schema changes?

flostadler left a 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

Choose a reason for hiding this comment

flostadler Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

flostadler Nov 21, 2024 • edited Loading

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

flostadler left a comment

Choose a reason for hiding this comment

pulumi-bot commented Nov 26, 2024

jdavredbeard commented Nov 18, 2024 •

edited

Loading

flostadler Nov 21, 2024 •

edited

Loading

flostadler Nov 21, 2024 •

edited

Loading