Repository navigation
fix(eks): allow access policies on AccessEntryType.EC2 access entries - #38782
Conversation
|
Automated review A maintainer will still review this — treat the notes below as a starting point. This PR fixes a bug in both the The change itself is small, well-scoped, symmetric across the two modules, and backed by AWS documentation; the API surface, synthesized template behavior, and documentation are all consistent and correct. One point is worth attention: an unrelated feature-flag block was removed from the integ test's 🔴 0 blocking · 🟡 1 recommended · ⚪ 0 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
|
Addressed the automated review note: updated the |
|
|
||||||||||||||
|
|
||||||||||||||
kumvprat
left a comment
There was a problem hiding this comment.
Thanks @krantboy for the contribution. It closes the p1 (#37496) by removing AccessEntryType.EC2 from restrictedTypes in validateAccessPoliciesForRestrictedTypes in both aws-eks and aws-eks-v2, so EC2 access entries can now carry access policies while HYBRID_LINUX/HYPERPOD_LINUX keep their guard.
The integ tests passed through our automated integration test workflow so deployment should be fine with this new feature.
I have added an inline comment on the integration tests, can you have a look ? Apart from that the PR changes look ready to be merged.
Merge notes: the branch is behind main, so please rebase and regenerate the snapshot.
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
On the specific feature flag toggles below :
const app = new App({
postCliContext: {
'@aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy': true,
'@aws-cdk/aws-lambda:useCdkManagedLogGroup': false,
},
});
Can you check if these are the default values of these feature flags now ? If so we can remove these overloads and check again if the integration tests pass
There was a problem hiding this comment.
Checked both, and neither is at its default:
useCdkManagedLogGroupis pinnedfalsewhilerecommendedValueistrue. Removing it adds two managed log groups.createNewPoliciesWithAddToRolePolicyis pinnedtruewhilerecommendedValueisfalse. Removing it restructures the provider IAM policies.
They're stale, but dropping either changes what the test synthesizes, so I've left them as they are.
Also rebased on main. The snapshot verifies unchanged against current main, so there was nothing to regenerate. The force-push reset the workflow approvals, so CI will need a nudge from you to run.
There was a problem hiding this comment.
They're stale, but dropping either changes what the test synthesizes, so I've left them as they are.
If these are not the specific requirements to make this feature work, can we remove the overrides and re-synth the test ? We can run the integration test automation post that to test the functionality
Approved the previous CI runs.
There was a problem hiding this comment.
Removed both overrides as requested and re-synthed. The test now runs on defaults.
Note the force-push reset the workflow approvals again, and the integ deployment needs additional approvals to release the deployment-integ-test environment.
984ce2a to
9e2f364
Compare
`validateAccessPoliciesForRestrictedTypes` listed `AccessEntryType.EC2` alongside `HYBRID_LINUX` and `HYPERPOD_LINUX`, so passing `accessPolicies` to an EC2 type `AccessEntry` (or calling `addAccessPolicies()` on one) threw a ValidationError. The EKS API does support access policies on EC2 type entries -- attaching `AmazonEKSAutoNodePolicy` to an EC2 type entry is the documented way to grant an EKS Auto Mode node class access to the cluster. Removes `EC2` from the restricted list in both `aws-eks` and `aws-eks-v2`, and corrects the enum JSDoc and README notes that claimed otherwise. The aws-eks-v2 README already documented an EC2 type `grantAccess` carrying `AmazonEKSAutoNodePolicy`, an example that threw at synth time. fixes aws#37496
…integ test The integ test pinned two Lambda feature flags that are unrelated to the EKS access entry feature under test, and both pinned the opposite of their recommended value. Removing them lets the test synthesize on current defaults. Dropping `useCdkManagedLogGroup: false` adds CDK-managed log groups, and dropping `createNewPoliciesWithAddToRolePolicy: true` folds the provider inline policies back into the default policies.
9e2f364 to
8c50077
Compare
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
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). |
Merge Queue Status
This pull request spent 2 hours 48 minutes 36 seconds in the queue, including 1 hour 1 minute 57 seconds running CI. Required conditions to merge
|
|
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). |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #37496.
Reason for this change
AccessEntryrejectedaccessPoliciesonAccessEntryType.EC2entries at synth time, even though the EKS API accepts them.validateAccessPoliciesForRestrictedTypesgroupedEC2withHYBRID_LINUXandHYPERPOD_LINUX:That guard runs from both the
AccessEntryconstructor andaddAccessPolicies(), so either path threwAccess entry type 'EC2' cannot have access policies attached.Attaching
AmazonEKSAutoNodePolicyto anEC2type entry is the documented way to grant an EKS Auto Mode node class access to the cluster — see Create node class access entry, which creates the entry with--type EC2and then associates that policy. The equivalentCfnAccessEntry(Type: "EC2"withAccessPolicies) deploys fine, so the L2 was strictly more restrictive than the resource underneath it and users had to drop to L1 to express it.aws-eks-v2's README already documented exactly this call:directly above a note saying
EC2cannot have policies. That example throws at synth today. README blocks are compiled by Rosetta but never executed, so nothing caught the contradiction.Description of changes
AccessEntryType.EC2fromrestrictedTypesinaws-eksandaws-eks-v2.HYBRID_LINUXandHYPERPOD_LINUXkeep their guard — this PR makes no claim about those.EC2enum JSDoc and the README notes in both modules, which asserted the opposite of the new behaviour.No feature flag. This relaxes a synth-time guard that rejected input CloudFormation would have accepted, so no app that synthesizes today can change behaviour — per CONTRIBUTING, a flag is required for the opposite direction (newly rejecting input that used to work).
Two decisions worth a maintainer's opinion:
AmazonEKSAutoNodePolicybecause it is the documented real-world pairing, on the existing non-Auto-Mode cluster in that test.AmazonEKSViewPolicywould exercise the same code path with less dependence on Auto Mode specifics if you would prefer that.EC2entry to theaws-eks-v2integ test, since its own comments stateEC2requires an Auto Mode cluster. That file gets a comment correction only, no resource change and no snapshot impact.Describe any new or updated permissions being added
None. No IAM policy is generated or changed by CDK here — this only stops CDK from rejecting an
AccessPoliciesvalue the user supplies, which is then passed through toAWS::EKS::AccessEntryunchanged.Description of how you validated changes
Unit tests in both modules —
EC2moved from the two throws-lists into the two allows-lists, so it is now asserted to accept policies both at construction and viaaddAccessPolicies(). 56 tests pass acrossaws-eks/test/access-entry.test.tsandaws-eks-v2/test/access-entry.test.ts.I confirmed the tests are load-bearing rather than vacuous: re-adding
EC2torestrictedTypesfails exactly 3 of them (creates a new AccessEntry for AccessEntryType EC2,allows EC2 type with access policies,allows adding policies to EC2 type via addAccessPolicies()).Integ test: added an
EC2type entry carryingAmazonEKSAutoNodePolicytointeg.eks-grant-access-with-type.ts. The snapshot diff is the two intended resources:The integration test has since been deployed against a live EKS cluster through the automated integration test workflow, and passed.
Note on the
postCliContextremoval: the integ test pinned two Lambda feature flags unrelated to this fix (createNewPoliciesWithAddToRolePolicy,useCdkManagedLogGroup), both set opposite to theirrecommendedValue, so it was exercising legacy behaviour. Removing them lets the test synthesize on current defaults. It is isolated in its own commit, and the effect is confined to the test's own nested stacks: the provider inline policies fold back into the default policies and CDK-managed log groups are added. No library code changes.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license