Skip to content

Conversation

@newlinedeveloper
Copy link
Contributor

Issue

Closes #32909.

Reason for this change

When deleting an EKS FargateProfile, the onDelete event directly uses physicalResourceId as the profile name without validating its length. If physicalResourceId exceeds 100 characters (AWS EKS's maximum limit), deletion fails with: "The Fargate profile name parameter should not be greater than 100 characters."

The onCreate event already handles this by generating a valid name when needed, but this logic was missing in onDelete.

Description of changes

Updated onDelete() to use the same name resolution logic as onCreate():

  • If fargateProfileName is explicitly provided in config: use it
  • If physicalResourceId is valid (≤ 100 chars): use it directly
  • If physicalResourceId exceeds 100 chars: generate a name using generateProfileName()

Files changed:

  • fargate.ts: Updated onDelete() method
  • fargate-resource-provider.test.ts: Added 5 unit tests covering all scenarios

Description of how you validated changes

Added comprehensive unit tests:

  • Normal deletion with valid physicalResourceId
  • Explicit fargateProfileName handling
  • Edge case with physicalResourceId > 100 chars
  • Explicit name precedence
  • Boundary condition (exactly 100 chars)

All tests pass: yarn test fargate-resource-provider.test.ts

Checklist

  • My code adheres to the CONTRIBUTING GUIDE
  • Unit tests added/updated
  • No breaking changes introduced

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

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 15, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team November 15, 2025 12:13
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Nov 15, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@newlinedeveloper
Copy link
Contributor Author

Exemption Request

Requesting exemption from integration testing requirements for this bug fix:

  1. Bug fix reusing existing logic: The fix applies the same proven logic from onCreate() to onDelete()
  2. Comprehensive unit test coverage: 5 new unit tests cover all scenarios including edge cases
  3. Low risk: Minimal change that follows existing patterns
  4. Integration test complexity: Would require creating EKS clusters and reproducing rare edge cases (> 100 char names), which is expensive and difficult to reliably test

The unit tests provide sufficient coverage for this straightforward fix.

@newlinedeveloper
Copy link
Contributor Author

Hi @pahud . Need review for this PR. Thanks

@pahud
Copy link
Contributor

pahud commented Nov 18, 2025

Hi @pahud . Need review for this PR. Thanks

I just clicked CI approval button to trigger the CI run for this PR. After CI passes you should get pr/pending-community-review label and you are all set. Do not click Update branch after your PR is ready as this triggers another CI build attempt. Thank you.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 18, 2025
Abogical
Abogical previously approved these changes Nov 18, 2025
@Abogical
Copy link
Member

Abogical commented Nov 18, 2025

Sorry, I meant to review your other PR: #35988
Approval retracted.

@Abogical Abogical self-assigned this Nov 18, 2025
@Abogical Abogical self-requested a review November 18, 2025 14:56
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 18, 2025
@Abogical Abogical removed their assignment Nov 18, 2025
@Abogical Abogical dismissed their stale review November 18, 2025 15:54

Made in error

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(aws-eks): Inconsistent fargateProfileName Handling Causes Deletion Failure When PhysicalResourceId Exceeds 100 Characters

4 participants