Repository navigation
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Features 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.
|
Maintainer assistance requested: I cannot run the real AWS integration deployment because no AWS test environment is available to me. As permitted by the contribution guide, could a maintainer please run the integration test and generate/commit the deployment-backed snapshots?
Local validation is recorded in the PR description. The real AWS test has not been run and its deployment-backed snapshot is missing; I have not used dry-run or manually fabricated snapshots. I am marking this PR ready for code review with that limitation explicit. This is a request to complete the required validation, not an integration-test exemption or a claim that the PR is ready to merge. |
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR adds an optional 🔴 1 blocking · 🟡 0 recommended · ⚪ 0 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| import { ExpectedResult, IntegTest } from '@aws-cdk/integ-tests-alpha'; | ||
| import * as cdk from 'aws-cdk-lib'; | ||
| import * as ec2 from 'aws-cdk-lib/aws-ec2'; | ||
|
|
||
| const app = new cdk.App(); | ||
| const stack = new cdk.Stack(app, 'nat-instances-imdsv2'); | ||
| const provider = ec2.NatProvider.instanceV2({ | ||
| instanceType: new ec2.InstanceType('t3.micro'), | ||
| requireImdsv2: true, | ||
| defaultAllowedTraffic: ec2.NatTrafficDirection.OUTBOUND_ONLY, | ||
| }); | ||
| const vpc = new ec2.Vpc(stack, 'Vpc', { maxAzs: 1, natGateways: 1, natGatewayProvider: provider }); | ||
| provider.connections.allowFrom(ec2.Peer.ipv4(vpc.vpcCidrBlock), ec2.Port.allTraffic()); | ||
| const integ = new IntegTest(app, 'NatImdsv2', { testCases: [stack] }); | ||
| integ.assertions.awsApiCall('EC2', 'describeInstances', { InstanceIds: [provider.gatewayInstances[0].instanceId] }) | ||
| .assertAtPath('Reservations.0.Instances.0.MetadataOptions.HttpTokens', ExpectedResult.exact('required')); |
There was a problem hiding this comment.
🔴 Blocking — This new integration test ships without its companion deployment-backed snapshot: the changed files are only this .ts, nat.ts, the README, and the unit test, with no integ.nat-imdsv2.js.snapshot/ directory. A CDK integration snapshot is the synthesized CloudFormation template produced by a real deployment, so a new integ test must be deployed against a real AWS account and its generated snapshot committed. Without it, the integ check cannot pass and the committed test does not prove the template actually deploys or that MetadataOptions.HttpTokens resolves to required on the NAT instance — the whole point of the test's awsApiCall/ExpectedResult assertion (INTEGRATION_TESTS.md).
Suggested change: Deploy the test against a real account with yarn integ integ.nat-imdsv2.ts --update-on-failed (a real deploy, never --dry-run) and commit the generated integ.nat-imdsv2.js.snapshot/ directory, confirming its nat-instances-imdsv2 template contains the AWS::EC2::LaunchTemplate with HttpTokens: required before merge. Maintainer assistance may be needed if the author cannot provision an account.
Issue # (if applicable)
Closes #23803.
Description of changes
Expose optional requireImdsv2 on NatInstanceProps and forward it through both legacy and V2 providers. Omitting the option preserves the EC2 Instance default. Custom AMIs must support IMDSv2.
Validation: 193 tests passed on this independent branch; changed TypeScript files passed ESLint. Strict JSII/TypeScript compilation passed with all five independent changes combined on a local validation branch.
Deployment pending: added integ.nat-imdsv2.ts with an EC2 describeInstances assertion. No AWS account/credentials are available. A maintainer must run the actual integration deployment and generate snapshots; no dry-run snapshot is included.
The changed README example compiled successfully with Rosetta (one snippet, no compiler diagnostics). The new integration source passed strict TypeScript checking.
Maintainer assistance requested: real AWS integration validation
This PR is ready for code review, but deployment validation is incomplete. I do not have an AWS account/environment available to run the real integration deployment. Local validation is documented above; it does not replace AWS deployment validation.
Following the CDK contribution guidance for contributors who cannot run integration tests, could a maintainer please run the integration test below in an AWS test environment and generate/commit the deployment-backed snapshots?
packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-imdsv2.tsNo real AWS deployment has been performed, and no dry-run or manually fabricated snapshot has been committed. The required deployment-backed snapshot is still missing, so the integration snapshot checks cannot yet pass. This request is for help completing the validation, not for waiving it; the PR should not be merged until the required validation and review are complete.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license