-
Notifications
You must be signed in to change notification settings - Fork 57
feat: thread vpcId for container build VPC placement (#297) #1671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
723e96a
feat(schema): add optional networkConfig.vpcId for Container VPC builds
aidandaly24 d7db7b9
feat(cli): add --vpc-id validation for Container VPC builds
aidandaly24 f9155f0
feat(cli): collect and persist networkConfig.vpcId across create/impo…
aidandaly24 ab56c25
fix(schema): require networkConfig.vpcId for container harness builds…
aidandaly24 1369d69
feat(cli): collect vpcId in create/add-agent/add-harness TUI for Cont…
aidandaly24 ec83dd3
feat(cli): add --vpc-id to add harness and cap container-build securi…
aidandaly24 0e91fe0
fix(cli): persist vpcId from the interactive create wizard for Contai…
aidandaly24 735ae1c
fix(cli): tighten vpc/subnet/sg ID validation to real AWS format
aidandaly24 b0db735
refactor(schema): centralize AWS resource-ID patterns as single sourc…
aidandaly24 39b5231
fix(cli): validate --vpc-id at CLI layer for container harness builds…
aidandaly24 1bd733f
fix(cli): resolve networkConfig.vpcId via DescribeSubnets on import/e…
aidandaly24 dc8e8ca
fix(cli): resolve vpcId via DescribeSubnets on starter-toolkit YAML i…
aidandaly24 0d87560
fix(cli): only resolve vpcId for container builds on import/export, n…
aidandaly24 709e9ef
fix(cli): resolve vpcId for ALL container exports incl prebuilt conta…
aidandaly24 4485c61
fix(vpc): address review — unify container-build detection, backfill …
aidandaly24 e8f71d5
style: prettier formatting for review-fix changes
aidandaly24 59bbde0
fix(create): thread vpcId + VPC validation through the create-harness…
aidandaly24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
110 changes: 110 additions & 0 deletions
110
src/cli/commands/add/__tests__/harness-vpc-guard.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import type { AddHarnessCliOptions } from '../types'; | ||
| import { validateAddHarnessOptions } from '../validate'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| const base: AddHarnessCliOptions = { | ||
| name: 'h1', | ||
| modelProvider: 'bedrock', | ||
| modelId: 'us.anthropic.claude-haiku-4-5-20251001-v1:0', | ||
| }; | ||
|
|
||
| describe('validateAddHarnessOptions — VPC network-mode guard', () => { | ||
| it('rejects VPC mode without subnets', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| networkMode: 'VPC', | ||
| securityGroups: 'sg-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) expect(result.error).toContain('--subnets is required'); | ||
| }); | ||
|
|
||
| it('rejects VPC mode without security groups', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| networkMode: 'VPC', | ||
| subnets: 'subnet-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) expect(result.error).toContain('--security-groups is required'); | ||
| }); | ||
|
|
||
| it('accepts valid VPC options for a non-container harness', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| networkMode: 'VPC', | ||
| subnets: 'subnet-0a1b2c3d', | ||
| securityGroups: 'sg-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(true); | ||
| }); | ||
|
|
||
| it('rejects a container (dockerfile) harness with --network-mode VPC and no --vpc-id', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| container: './Dockerfile', | ||
| networkMode: 'VPC', | ||
| subnets: 'subnet-0a1b2c3d', | ||
| securityGroups: 'sg-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) expect(result.error).toContain('--vpc-id is required'); | ||
| }); | ||
|
|
||
| it('accepts a container (dockerfile) harness with --network-mode VPC and a valid --vpc-id', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| container: './Dockerfile', | ||
| networkMode: 'VPC', | ||
| subnets: 'subnet-0a1b2c3d', | ||
| securityGroups: 'sg-0a1b2c3d', | ||
| vpcId: 'vpc-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(true); | ||
| }); | ||
|
|
||
| it('requires --vpc-id for a container URI harness in VPC mode (a prebuilt image still runs CodeBuild)', () => { | ||
| // A --container <ecr-uri> harness is a container build: export emits a `FROM <uri>` Dockerfile | ||
| // that CodeBuild builds, so it needs a vpcId just like a dockerfile harness. Previously this | ||
| // slipped through and dead-ended at deploy/export. | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| container: '123456789012.dkr.ecr.us-east-1.amazonaws.com/my-image:latest', | ||
| networkMode: 'VPC', | ||
| subnets: 'subnet-0a1b2c3d', | ||
| securityGroups: 'sg-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) expect(result.error).toContain('--vpc-id is required'); | ||
| }); | ||
|
|
||
| it('accepts a container URI harness in VPC mode when --vpc-id is provided', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| container: '123456789012.dkr.ecr.us-east-1.amazonaws.com/my-image:latest', | ||
| networkMode: 'VPC', | ||
| subnets: 'subnet-0a1b2c3d', | ||
| securityGroups: 'sg-0a1b2c3d', | ||
| vpcId: 'vpc-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(true); | ||
| }); | ||
|
|
||
| it('rejects subnets provided without --network-mode VPC', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| subnets: 'subnet-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) expect(result.error).toContain('only valid with --network-mode VPC'); | ||
| }); | ||
|
|
||
| it('rejects --vpc-id provided without --network-mode VPC', () => { | ||
| const result = validateAddHarnessOptions({ | ||
| ...base, | ||
| vpcId: 'vpc-0a1b2c3d', | ||
| }); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) expect(result.error).toContain('only valid with --network-mode VPC'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor layering nit (not blocking):
src/cli/aws/agentcore-control.tsnow importsresolveVpcIdFromSubnetsfrom../commands/shared/vpc-utils. The general direction in this repo iscommands/ → aws/, not the reverse; pulling a commands-layer helper back into a core AWS client file inverts that and risks creating an import cycle the next time someone wires another helper throughvpc-utils.No actual cycle today (
vpc-utils → aws/account,agentcore-control → vpc-utils → aws/accountis fine), butresolveVpcIdFromSubnetsis itself just an EC2 SDK wrapper — it would fit naturally next to the existing AWS clients insrc/cli/aws/(e.g. a newec2.ts), and bothagentcore-control.tsandvpc-utils.tscould import it from there. Worth doing while the helper is new and has exactly two callers.