fix(eks): fargateCluster compatibility with AuthenticationMode.API #31267

pahud · 2024-08-30T12:46:33Z

Issue # (if applicable)

This PR ensures the eks fargateCluster compatibility with AuthenticationMode.API

Reason for this change

The FargateCluster assumes the authentication mode is always config map and create the podExectionRole mapping using props.cluster.awsAuth.addRoleMapping(). This won't work when authenticationMode is API because in this mode, config map is not supported and this statement would just fail.

We need to add an conditional check, only when the cluster supports configmap will it run the addRoleMapping() statement. At this moment, the following authenticationMode would support configmap:

undefined
CONFIG_MAP
API_AND_CONFIG_MAP

Description of changes

Description of how you validated changes

update the integ test
manual deployments

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

…ode (#31258) ### Issue # (if applicable) This PR improve the compatibility for `albController` with `authenticationMode.API` related to #30888 We will address Fargate compatibility in #31267 ### Reason for this change - When `authenticationMode.API` is specified, no aws-auth configMap should be created - albController should not depend on `cluster.awsAuth` because that would create aws-auth configmap, which is not required in `API` mode. ### Description of changes ### Description of how you validated changes **unit tests** - validate the behavior in all conditions of the `authenticationMode` **integ test** - add a new integ test with API mode to ensure successful deployment ## debugger ```js { "version": "0.2.0", "configurations": [ { "type": "node", "request": "launch", "name": "Jest", "program": "${workspaceFolder}/node_modules/jest/bin/jest.js", "cwd": "${workspaceFolder}/packages/aws-cdk-lib", "args": [ "--verbose", "-i", "--no-cache", "test/alb-controller.test.ts", ], "console": "integratedTerminal", "internalConsoleOptions": "neverOpen", "skipFiles": [ "<node_internals>/**" ], "outFiles": [ "${workspaceFolder}/**/*.(m|c|)js", "!**/node_modules/**" ], } ] } ``` ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rix0rrr · 2024-09-04T12:28:36Z

packages/aws-cdk-lib/aws-eks/lib/fargate-profile.ts

+    const supportConfigMap = props.cluster.authenticationMode !== AuthenticationMode.API ? true : false;
+    if (supportConfigMap) {


Seems to me like something that was unconditional is now conditional, and PR body doesn't tell me anything about what the problem was or why this is the fix.

That's leaving me to wonder: what are the backwards compatibility implications of this?

Thank you. Just fixed.

rix0rrr · 2024-09-04T12:29:37Z

packages/aws-cdk-lib/aws-eks/lib/fargate-profile.ts

-        'system:node-proxier',
-      ],
-    });
+    const supportConfigMap = props.cluster.authenticationMode !== AuthenticationMode.API ? true : false;


Concluding support for something by exclusion works in a closed world, but I'm not sure the world is actually closed. If a 4th authentication method is added in the future, will this condition have to be updated? Isn't is safer to inclusively check for one of the CONFIG_MAP options?

rix0rrr · 2024-09-04T12:32:41Z

packages/aws-cdk-lib/aws-eks/README.md

@@ -1142,6 +1142,37 @@ You can disable granting the cluster admin permissions to the cluster creator ro

 > **Note** - Switching `bootstrapClusterCreatorAdminPermissions` on an existing cluster would cause cluster replacement and should be avoided in production.

+When a `FargateCluster` is created with `AuthenticationMode.API`, by default the cluster creator role would be added into the AccessEntry with `AmazonEKSClusterAdminPolicy` unless `bootstrapClusterCreatorAdminPermissions` is disabled.


I'm not sure what this line is trying to tell me, exactly.

Isn't the behavior of adding the creator user, except when you set a flag, the same as the behavior describer 2 sentences above? What does the qualification about the AuthenticationMode add? Why is this a separate line and not integrated into the previous description?

Please refrain from using non-present tenses like "would". It's not the Amazon docs writing style, and is unnecessarily indirect. We can just say "will".

I can spot it just above this change as well. I know you didn't change it in this PR, but improving the overall writing style and condensing/rewriting bad or redundant docs will be a help overall. Better than endlessly adding standalone lines just to have covered the new feature, that will make the docs unreadable over time.

Thank you! Yes it is redundant after reviewing the description. I just removed it from the README.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

pahud · 2024-09-10T02:30:29Z

Hi @rix0rrr , all addressed. Thank you.

GavinZZ

LGTM, I think Pahud has addressed Rico's feedback. Approving on behalf for Rico. In summary, this shouldn't be a breaking change because the original code won't work when authenticationMode is API because in this mode, config map is not supported and this statement would just fail.

aws-cdk-automation · 2024-09-11T00:01:06Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

GavinZZ · 2024-09-11T19:34:14Z

@mergify update

mergify · 2024-09-11T19:34:21Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

Dismissing stale review as I believe Rico's comments are addressed.

mergify · 2024-09-11T21:09:19Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-09-11T21:57:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 960ff89
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-09-11T21:57:50Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-09-11T21:58:11Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

initial commit

4472451

github-actions bot added the p2 label Aug 30, 2024

aws-cdk-automation requested a review from a team August 30, 2024 12:46

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 30, 2024

pahud mentioned this pull request Aug 30, 2024

fix(eks): albController incompatibility with AuthenticationMode.API mode #31258

Merged

1 task

aws-cdk-automation previously requested changes Aug 30, 2024

View reviewed changes

pahud added 3 commits August 30, 2024 15:42

update snapshots

385c164

update unit tests

46c4272

update README

8c868fd

update README

7c9a2f5

pahud marked this pull request as ready for review August 31, 2024 01:33

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 and removed p2 labels Aug 31, 2024

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 31, 2024

github-actions bot mentioned this pull request Sep 1, 2024

Monthly PR metrics report #31279

Closed

optimize the test

a8602d8

github-actions bot mentioned this pull request Sep 2, 2024

Weekly PR metrics report #31284

Closed

rix0rrr previously requested changes Sep 4, 2024

View reviewed changes

rix0rrr self-assigned this Sep 4, 2024

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 4, 2024

pahud added 2 commits September 4, 2024 18:15

fix

cbda801

minor

26b9c9a

aws-cdk-automation previously requested changes Sep 4, 2024

View reviewed changes

lint

ddf65bb

pahud changed the title ~~feat(eks): fargateCluster compatibility with AuthenticationMode.API~~ chore(eks): fargateCluster compatibility with AuthenticationMode.API Sep 9, 2024

pahud changed the title ~~chore(eks): fargateCluster compatibility with AuthenticationMode.API~~ fix(eks): fargateCluster compatibility with AuthenticationMode.API Sep 9, 2024

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Sep 9, 2024

pahud force-pushed the fargatecluster-api-mode branch 2 times, most recently from 56f1a72 to ddf65bb Compare September 9, 2024 23:58

pahud added 2 commits September 10, 2024 00:00

delete stale snapshots

fae1dfa

update

75cb15a

pahud force-pushed the fargatecluster-api-mode branch from 10c58a4 to 75cb15a Compare September 10, 2024 00:33

Merge branch 'main' into fargatecluster-api-mode

58c6d3a

aws-cdk-automation previously requested changes Sep 10, 2024

View reviewed changes

update snapshots

3d3bafb

update snapshots

9f0af45

pahud requested a review from rix0rrr September 10, 2024 02:29

GavinZZ approved these changes Sep 10, 2024

View reviewed changes

GavinZZ added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Sep 11, 2024

Merge branch 'main' into fargatecluster-api-mode

4ec073d

GavinZZ removed pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr/needs-cli-test-run This PR needs CLI tests run against it. labels Sep 11, 2024

Merge branch 'main' into fargatecluster-api-mode

960ff89

mergify bot merged commit 4d12833 into aws:main Sep 11, 2024
9 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eks): fargateCluster compatibility with AuthenticationMode.API #31267

fix(eks): fargateCluster compatibility with AuthenticationMode.API #31267

pahud commented Aug 30, 2024 •

edited

Loading

aws-cdk-automation left a comment

rix0rrr Sep 4, 2024

pahud Sep 10, 2024

rix0rrr Sep 4, 2024

rix0rrr Sep 4, 2024

pahud Sep 4, 2024

aws-cdk-automation left a comment

aws-cdk-automation left a comment

pahud commented Sep 10, 2024

GavinZZ left a comment

aws-cdk-automation commented Sep 11, 2024

GavinZZ commented Sep 11, 2024

mergify bot commented Sep 11, 2024

mergify bot commented Sep 11, 2024

aws-cdk-automation commented Sep 11, 2024

mergify bot commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

		const supportConfigMap = props.cluster.authenticationMode !== AuthenticationMode.API ? true : false;
		if (supportConfigMap) {

		@@ -1142,6 +1142,37 @@ You can disable granting the cluster admin permissions to the cluster creator ro

		> Note - Switching `bootstrapClusterCreatorAdminPermissions` on an existing cluster would cause cluster replacement and should be avoided in production.

		When a `FargateCluster` is created with `AuthenticationMode.API`, by default the cluster creator role would be added into the AccessEntry with `AmazonEKSClusterAdminPolicy` unless `bootstrapClusterCreatorAdminPermissions` is disabled.

fix(eks): fargateCluster compatibility with AuthenticationMode.API #31267

fix(eks): fargateCluster compatibility with AuthenticationMode.API #31267

Conversation

pahud commented Aug 30, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rix0rrr Sep 4, 2024

Choose a reason for hiding this comment

pahud Sep 10, 2024

Choose a reason for hiding this comment

rix0rrr Sep 4, 2024

Choose a reason for hiding this comment

rix0rrr Sep 4, 2024

Choose a reason for hiding this comment

pahud Sep 4, 2024

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

pahud commented Sep 10, 2024

GavinZZ left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 11, 2024

GavinZZ commented Sep 11, 2024

mergify bot commented Sep 11, 2024

❌ Mergify doesn't have permission to update

mergify bot commented Sep 11, 2024

aws-cdk-automation commented Sep 11, 2024

AWS CodeBuild CI Report

mergify bot commented Sep 11, 2024

github-actions bot commented Sep 11, 2024

pahud commented Aug 30, 2024 •

edited

Loading