Relax validations on eks.Cluster instance_roles #1227

t0yv0 · 2024-07-03T20:37:07Z

Proposed changes

This change allows a program to set empty instance_roles=[] on an eks.Cluster with custom authentication_mode without triggering a warning on these properties being incompatible.

roleMappings and userMappings are treated symmetrically.

Related issues (optional)

Fixes #1220

t0yv0 · 2024-07-03T20:38:30Z

I've confirmed this fixes preview of the following program. Checking if it fixes pulumi up as well.

import pulumi
import pulumi_aws as aws
import pulumi_awsx as awsx
import pulumi_eks as eks

vpc = awsx.ec2.Vpc("vpc")

cluster = eks.Cluster(
    "zd-5423",
    vpc_id=vpc.vpc_id,
    public_subnet_ids=vpc.public_subnet_ids,
    private_subnet_ids=vpc.private_subnet_ids,
    authentication_mode=eks.AuthenticationMode.API,
    skip_default_node_group=True,
    instance_roles=[],
    use_default_vpc_cni=False,
)

github-actions · 2024-07-03T20:39:35Z

Does the PR have any schema changes?

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

nodejs/eks/authenticationMode.ts

github-actions · 2024-07-04T00:26:00Z

Does the PR have any schema changes?

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

t0yv0 · 2024-07-04T00:26:58Z

nodejs/eks/authenticationMode.ts

+// Validate promptly if possible, otherwise validate in the Promise chain underlying the Output and ensure that the
+// input is gated on the validation. Unfortunately since apply always unwraps, the validate function required here needs
+// to be able to handle both unwrapped and normal forms.
+function validatedInput<T>(


This trick didn't work as neatly as I hoped. I hoped to make a pulumi.Input<T> that is gated on the validation. Unfortunately I couldn't wrestle the jest framework to be able to assert against failing pulumi.Output values :/ so simply coercing pulumi.Input<T> to pulumi.Output<T> does not cut it because I couldn't fix the unit tests. Instead this is further refined to handle immediately available T on the stack and only jump into an apply chain if that is not possible. Unfortunately since apply always unwraps and there doesn't seem to be a non-unwrapping variant available, the validate function has this signature:

validate: (value: T|pulumi.Unwrap<T>|undefined) => void

where the ideal signature would be:

validate: (value: T|undefined) => void

t0yv0 · 2024-07-04T00:28:50Z

nodejs/eks/authenticationMode.test.ts

@@ -70,10 +70,20 @@ describe("validateAuthenticationMode", () => {
        };

        expect(() => validateAuthenticationMode(args)).toThrowError(
-            "The 'instanceRoles' property is not supported when 'authenticationMode' is set to 'API'.",
+            "The 'instanceRoles' property does not support non-empty values when 'authenticationMode' is set to 'API'.",


Actually something must be of as this test shouldn't compile.

const args = { authenticationMode: "API", instanceRoles: ["roleArn"], };

"roleArn" is not a valid aws.eks.Role.

The test isn't type checked because it's executed as js. If you'd like to satisfy the type checker you could pass the args as ClusterOptions

I've added it to tsconfig.json and fixed it up, it now is type-checked.

t0yv0 · 2024-07-04T00:33:12Z

Wish list for the Node SDK here.

should be a simple way to assert that Output<T> values are failing in jest, such as transforming Output<T> to Promise<OutputResult<T>> and asserting that this promise resolves with an exception
should be a way to apply (x: T)=>U to an Output<T> and receive Output<U> and not Output<Unwrap<U>> opting out of the unwrapping

flostadler · 2024-07-15T10:07:43Z

nodejs/eks/authenticationMode.ts

    if (!supportsConfigMap(args.authenticationMode)) {
-        configMapOnlyProperties.forEach((prop) => {
-            if (args[prop]) {
+        const check: (prop: string) => (propertyValue: any|undefined) => void = (prop) => (pv) => {


Nit: prop could be keyof ClusterOptions. What do you think about renaming this to checkIsUndefined? That way it's clearer what it is actually checking

flostadler · 2024-07-15T10:11:37Z

nodejs/eks/authenticationMode.ts

                throw new Error(
                    `The '${prop}' property is not supported when 'authenticationMode' is set to '${args.authenticationMode}'.`,
                );
            }
+        };
+
+        args.roleMappings = validatedInput(args.roleMappings, check("roleMappings"));


Why don't we extend this to roleMappings and userMappings as well? Would be a nice improvement and it's a bit odd imho if roleMappings/userMappings don't allow empty arrays while instanceRoles does

flostadler · 2024-07-15T10:13:14Z

nodejs/eks/authenticationMode.test.ts

+    it("should not throw an error for instanceRoles=[] when authentication mode is set to API", () => {
+        const args = {
+            authenticationMode: "API",
+            instanceRoles: [],


Could we also test passing instanceRoles as a Promise and Output?

That'd be great but I was running into limitations that seem to make failing outputs un-testable. I can ask around for help.

Might be worth adding an integration test tif that doesn't work

I filed pulumi/pulumi#16665 - there's something odd, possibly a bug in Node SDK that makes it extra difficult. I'll look into an integration test.

github-actions · 2024-07-15T16:42:56Z

Does the PR have any schema changes?

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

flostadler

LGTM, just one comment about CI

flostadler · 2024-07-16T11:29:49Z

nodejs/eks/authenticationMode.test.ts

 describe("validateAuthenticationMode", () => {
+
+    const testRole: aws.iam.Role = (<any>{"arn": "testRole"});


flostadler · 2024-07-16T11:34:22Z

provider/provider_test.go

@@ -89,6 +90,50 @@ func TestReportUpgradeCoverage(t *testing.T) {
 	providertest.ReportUpgradeCoverage(t)
 }

+func TestEksClusterInputValidations(t *testing.T) {


This is executed as part of the upgrade tests right now in CI. What do you think about moving it to it's own job?

Hmm, interesting, I thought that we're trying to put tests that are not useful as examples to end-users now under provider/* instead of under examples/*, I can backlog cleaning this up.

pulumi-bot · 2024-07-18T18:46:18Z

This PR has been shipped in release v2.7.4.

pulumi-bot · 2024-07-19T18:14:57Z

This PR has been shipped in release v2.7.5.

pulumi-bot · 2024-07-23T19:09:24Z

This PR has been shipped in release v2.7.6.

### Proposed changes  This change allows a program to set empty `instance_roles=[]` on an eks.Cluster with custom `authentication_mode` without triggering a warning on these properties being incompatible. roleMappings and userMappings are treated symmetrically. ### Related issues (optional)  Fixes #1220

t0yv0 requested review from flostadler and corymhall July 3, 2024 20:37

flostadler reviewed Jul 3, 2024

View reviewed changes

nodejs/eks/authenticationMode.ts Outdated Show resolved Hide resolved

t0yv0 requested a review from flostadler July 4, 2024 00:23

t0yv0 commented Jul 4, 2024

View reviewed changes

mjeffryes assigned t0yv0 Jul 12, 2024

flostadler reviewed Jul 15, 2024

View reviewed changes

t0yv0 mentioned this pull request Jul 15, 2024

Unable to unit-test failing outputs in Node pulumi/pulumi#16665

Open

t0yv0 added 5 commits July 15, 2024 12:40

Permit instanceRoles=[] with every authenticationMode

e06e1a9

Fix typo

235fa44

Add a unit test

fa3361c

Handle non-prompt values per PR feedback

6ddff39

Fix the input trick

e4f4afd

t0yv0 force-pushed the t0yv0/fix-1220 branch from 2589931 to e4f4afd Compare July 15, 2024 16:40

t0yv0 added 6 commits July 15, 2024 12:44

PR feedback: rename func

2e6aeee

PR suggestion

d69164e

Handle role and user mappings similarly

b00976d

Adjust test expectations accordingly

6afbd02

Integration-level test for cluster input validations

92d0911

Ensure test files are type-checked

b52c59c

t0yv0 requested a review from flostadler July 15, 2024 18:26

flostadler approved these changes Jul 16, 2024

View reviewed changes

t0yv0 merged commit 986c528 into master Jul 16, 2024
43 checks passed

t0yv0 deleted the t0yv0/fix-1220 branch July 16, 2024 14:31

This was referenced Jul 18, 2024

Workflow failure: master #1234

Closed

Cluster with auth mode API can not be created with workaround that prevents instance role creation #1220

Closed

mjeffryes added this to the 0.107 milestone Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax validations on eks.Cluster instance_roles #1227

Relax validations on eks.Cluster instance_roles #1227

t0yv0 commented Jul 3, 2024 •

edited

Loading

t0yv0 commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

github-actions bot commented Jul 4, 2024

t0yv0 Jul 4, 2024

t0yv0 Jul 4, 2024

flostadler Jul 15, 2024 •

edited

Loading

t0yv0 Jul 15, 2024

t0yv0 commented Jul 4, 2024

flostadler Jul 15, 2024

flostadler Jul 15, 2024

t0yv0 Jul 15, 2024

flostadler Jul 15, 2024

t0yv0 Jul 15, 2024

flostadler Jul 15, 2024

t0yv0 Jul 15, 2024

github-actions bot commented Jul 15, 2024

flostadler left a comment

flostadler Jul 16, 2024

flostadler Jul 16, 2024 •

edited

Loading

t0yv0 Jul 16, 2024

t0yv0 Jul 16, 2024

pulumi-bot commented Jul 18, 2024

pulumi-bot commented Jul 19, 2024

pulumi-bot commented Jul 23, 2024

		describe("validateAuthenticationMode", () => {

		const testRole: aws.iam.Role = (<any>{"arn": "testRole"});

Relax validations on eks.Cluster instance_roles #1227

Relax validations on eks.Cluster instance_roles #1227

Conversation

t0yv0 commented Jul 3, 2024 • edited Loading

Proposed changes

Related issues (optional)

t0yv0 commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

Does the PR have any schema changes?

github-actions bot commented Jul 4, 2024

Does the PR have any schema changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flostadler Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Jul 4, 2024

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

github-actions bot commented Jul 15, 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

flostadler Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulumi-bot commented Jul 18, 2024

pulumi-bot commented Jul 19, 2024

pulumi-bot commented Jul 23, 2024

t0yv0 commented Jul 3, 2024 •

edited

Loading

flostadler Jul 15, 2024 •

edited

Loading

flostadler Jul 16, 2024 •

edited

Loading