Feature/private network codebuild support

[sirirako]

Summary

Adds full support for deploying the IDP Accelerator in private network / air-gapped environments where CodeBuild cannot reach the internet and all dependencies must come from internal registries (Artifactory, private ECR, etc.).

Changes

Infrastructure (template.yaml, patterns/unified/template.yaml)

• New optional CloudFormation parameters: PrivateRegistryUrl, PrivateEcrRegistryUrl, AirgappedCodeBuild, CodeBuildVpcId, CodeBuildSubnetIds, CodeBuildSecurityGroupIds
• When AirgappedCodeBuild=true, CodeBuild runs inside the customer's VPC with access to internal registries only
• All new parameters are optional — existing deployments are unaffected

Build (Dockerfile.optimized, patterns/unified/buildspec.yml)

• Dockerfile.optimized accepts PRIVATE_REGISTRY_URL and PRIVATE_ECR_REGISTRY_URL build args, substituting ghcr.io/astral-sh/uv and public.ecr.aws/lambda/python with internal equivalents when set
• buildspec.yml passes private registry args to Docker and configures pip/uv/npm to use Artifactory when PRIVATE_REGISTRY_URL is set

Setup Script (scripts/setup-airgapped-codebuild.sh)

• New script that automates one-time seeding of an air-gapped environment: pulls Docker base images from public registries and re-pushes to private ECR, downloads Python wheels and npm packages and uploads to Artifactory, and validates connectivity to all internal endpoints before proceeding

Documentation (docs/deployment-private-network.md, docs/artifactory-dependency-workaround.md)

• deployment-private-network.md — new air-gapped CodeBuild section with step-by-step deployment instructions and full parameter reference table
• artifactory-dependency-workaround.md — new Option 5: Automated Manifest Generation section documenting the approach proposed in PR Bump axios in /src/ui #1 (generate_lockfiles.py) as the recommended pre-step for seeding Artifactory; updated decision guide and quick reference

Note: The actual generate_lockfiles.py script and generated manifests are proposed in PR #1. This PR documents that approach in the workaround guide so it can be evaluated and used alongside this feature.

Test Plan

• Deploy with default parameters — verify no behavior change for standard deployments
• Deploy with AirgappedCodeBuild=true and valid VPC/subnet/SG IDs — verify CodeBuild runs inside VPC
• Run setup-airgapped-codebuild.sh on a bridge machine — verify Docker images and packages are uploaded to internal registries
• Review docs/artifactory-dependency-workaround.md Option 5 section matches PR Bump axios in /src/ui #1's implementation

[rstrahan]

Can you address the concerns raised in Claude's review below, @sirirako ? Thanks!

My recommendation: NOT ready to merge. The direction and infrastructure work is solid, but there are several concrete quality and completeness issues the author should address first.

---

1. Scope & composition

The PR does three logically separate things bundled together:

1. Air‑gapped Docker/PyPI plumbing (Dockerfile + buildspec + CFN params + IAM)
2. Optional placing CodeBuild inside a VPC (+ 3 new VPC endpoints for ecr.api, ecr.dkr, codebuild)
3. A helper scripts/setup-airgapped-codebuild.sh to mirror public images to ECR and a big docs update

The commit history is messy:

• Two unrelated dompurify commits (patches 1 & 2) were rolled into this branch (a 3.3.3→3.4.0 bump and its revert). They net to zero diff but pollute history.
• docs/artifactory-dependency-workaround.md was added (+587), massively extended (+176), then deleted (−745) in patch 8 with "moving to a separate PR" — good final outcome, but a 1200‑line round trip of churn.
• Author identity is corrupted: commits show "Sirirat <"siriratk@amazon.com">" with curly quotes, i.e. author's local git config user.name/email is mis‑quoted. Signed/Verified commits will likely fail; author should fix .gitconfig.

---

2. Correctness / functional bugs

🔴 NpmRegistryUrl is declared but never actually used where it matters

The commit message for patch 7 says "UICodeBuildProject gets NpmRegistryUrl env var + npm registry injection in buildspec", but the real diff shows:

• NpmRegistryUrl is added as a parameter in both template.yaml and patterns/unified/template.yaml, and passed through the nested stack.
• It is added as an env var (NPM_REGISTRY_URL) only on DockerBuildProject (which does no npm).
• UICodeBuildProject (the one that runs npm ci/npm run build for the React UI) is not modified in the diff at all.

Net effect: setting NpmRegistryUrl in an air‑gapped deployment does nothing — the UI build will still try to reach registry.npmjs.org and fail. This is the main gap a user in an air‑gapped VPC would hit immediately after getting past Docker.

Also related: UICodeBuildProject never gets VpcConfig, so setting CodeBuildVpcId only moves the Docker build inside the VPC; the UI build still runs outside it. In a truly internet‑blocked account this will also break.

🟡 Stale / wrong PR description

The PR body lists parameter names that don't exist in the code:

• PrivateRegistryUrl, PrivateEcrRegistryUrl, AirgappedCodeBuild, CodeBuildSecurityGroupIds — none of these are the actual implemented names.
• Actual names are UvImage, LambdaBaseImage, UvIndexUrl, ArtifactoryDockerUrl, ArtifactoryCredentialsSecretArn, NpmRegistryUrl, CodeBuildVpcId, CodeBuildSubnetIds, CodeBuildSecurityGroupId (singular).
• It references a PR Bump axios in /src/ui #1 / generate_lockfiles.py / "Option 5" that is not part of this PR at all.

Description must be rewritten to match reality before merge.

🟡 Broken internal doc link

docs/deployment-private-network.md references ./artifactory-dependency-workaround.md in two places (the "See also" note and the troubleshooting row), but that file was deleted in patch 8. Those become broken links at merge time.

🟡 Missing CFN Rules enforcing parameter co‑dependencies

• CodeBuildVpcId requires CodeBuildSubnetIds and CodeBuildSecurityGroupId.
• ArtifactoryDockerUrl requires ArtifactoryCredentialsSecretArn.
• None of these are enforced. A deploy with a half‑set of params fails at resource creation with a cryptic CFN error instead of at parameter validation. Cheap to add.

🟡 CodeBuildSecurityGroupId is singular

PR body (and common CodeBuild patterns) suggest plural. The param is Type: String accepting a single SG. Fine as long as that is the intended design, but the description table in deployment-private-network.md says "must allow outbound HTTPS (443)" which is one‑SG wording too. Just flag: if a customer needs additive SG attachment (e.g. for Artifactory allow‑list vs endpoint allow‑list) this forces them to merge rules into one SG.

🟡 CodeBuildSubnetIds default handling

Declared as Type: CommaDelimitedList, Default: "". With !Ref, this evaluates to [""] (a list with one empty string), not an empty list. The patch‑9 fix !Join [",", !Ref CodeBuildSubnetIds] is correct and matches the LambdaSubnetIds pattern — good catch by the author mid‑PR. Just note that the child template also declares it CommaDelimitedList, Default: "", so when unused it becomes [""] again; that's fine only because VpcConfig is gated by UseCodeBuildVpc.

---

3. Security / IAM

Mostly sound:

• Secrets Manager read is scoped to the exact ARN (good — not *).
• Credentials are fetched at build time via aws secretsmanager get-secret-value and piped to docker login --password-stdin (good — no hardcoded creds; they do transit through shell vars briefly and are unset afterwards, acceptable).
• UvIndexUrl is marked NoEcho: true at the parent, with a comment explaining it may embed creds (good).

Nits:

• The same UvIndexUrl is not NoEcho in patterns/unified/template.yaml, so if a customer uses the nested pattern directly or inspects nested‑stack parameters, the URL (potentially with embedded credentials) is still visible. Should be NoEcho: true in both places for consistency.
• ArtifactoryDockerUrl doesn't require NoEcho (hostname, fine).
• New EC2 ENI actions in DockerBuildRole use Resource: "*" — standard practice for CodeBuild VPC config and required; acceptable.
• New VPC endpoints (ecr.api, ecr.dkr, codebuild) use the shared VpcEndpointSecurityGroup already in scripts/vpc-endpoints.yaml — consistent with the existing pattern.

---

4. Risk of regression for existing deployments

Very low. All new CFN parameters default to empty, all new behaviors are behind !If … !Ref AWS::NoValue or shell if [ -n "$VAR" ] guards. Dockerfile ARGs default to the same public images that were hardcoded before. deploy-vpc-endpoints.py only gains a new --codebuild-endpoints flag (defaulted off) and a --security-group-id override with fallback to existing behavior. vpc-endpoints.yaml adds 3 new Create* params all defaulted "false".

The one thing to watch: deploy-vpc-endpoints.py changed its error messaging ("Make sure the stack is CREATE_COMPLETE and the stack name is correct" vs the previous AppSync‑specific message) — no behavior change, just UX.

---

5. Completeness / test plan

• PR is draft and all 4 test‑plan checkboxes in the description are unchecked.
• Only evidence of real testing is in patch 9's commit message ("IDP-ALB stack deployed successfully with ECR images ... DockerBuild SUCCEEDED, stack reached UPDATE_COMPLETE"). That's one happy path (ECR mirror scenario, likely not air‑gapped, likely no Artifactory, likely no CodeBuildVpcId). None of:
  • full AirgappedCodeBuild VPC mode with all VPC endpoints
  • Artifactory Docker registry login path
  • UvIndexUrl‑only mode
  • UI CodeBuild in air‑gapped mode
    ...appear to have been exercised end‑to‑end. Given bug Bump axios in /src/ui #1 above (UI side not wired), that's unsurprising — that path cannot work yet.
• No unit tests. This is infra/CFN/shell, so tests would likely be cfn-lint/cfn-guard or a smoke deploy. At minimum I'd expect a note that cfn-lint was run against both templates.

---

6. Quality & style nits

• The Dockerfile.optimized fix in patch 7 (explicit --index-url $UV_INDEX_URL flag in addition to ENV UV_INDEX_URL) is a good defensive improvement — kudos.
• The ECR pattern in buildspec uses tag uv-0.9.6 / lambda-python-3.12-arm64. When the project bumps uv to 0.9.7 (already a moving target), Dockerfile.optimized, buildspec.yml, and setup-airgapped-codebuild.sh all have the version hardcoded and must be updated in lockstep. A single source of truth (.uv-version file or Make var) would prevent future drift. Not blocking, but worth a follow‑up.
• scripts/setup-airgapped-codebuild.sh:
  • set -euo pipefail + success: echo with NC — good.
  • Writes airgapped-params-<region>.env to the current working directory — fine, but unmentioned in docs.
  • Does not handle the "public ECR login fails" case gracefully on an ARM host — but the warn is good.
  • Help text flag is --account (matches usage). Author noticed the doc had --account-id mismatch and fixed it in patch 7. Good.
• patterns/unified/buildspec.yml — the three if [ -n "$X" ] echo blocks default the shell vars if empty, but because UV_IMAGE / LAMBDA_BASE_IMAGE are set as CodeBuild EnvironmentVariables with !If [HasX, !Ref X, ""], they are always defined (possibly empty). The default‑substitution logic works, just slightly redundant with the Dockerfile's own ARG defaults. Not a bug.
• Lots of em‑dashes and Unicode box‑drawing characters in YAML/shell comments — purely cosmetic, harmless.

---

7. Summary

Area	Assessment
Problem worth solving	✅ Yes — customers in private/air‑gapped VPCs have no existing path
Approach	✅ Sound — optional params, backward compatible, secrets via Secrets Manager
Backward compatibility	✅ Defaults preserve current behavior
Regression risk	🟢 Low
Correctness — Docker/ECR path	🟢 Looks good (and verified by author)
Correctness — UI/npm path	🔴 NpmRegistryUrl not wired to UICodeBuildProject; no VpcConfig on UI build either
Security	🟢 NoEcho on parent UvIndexUrl, scoped IAM; ⚠ mirror NoEcho in child template
CFN quality	🟡 Missing Rules for co‑dependent params
Docs	🔴 Broken links to deleted file; 🟡 PR description doesn't match implementation
Tests	🟡 Only one happy‑path manual test; cfn-lint run unknown
History hygiene	🟡 Author identity malformed; unrelated dompurify patches included

Must‑fix before merge

1. Wire NpmRegistryUrl into UICodeBuildProject (or explicitly scope it out and remove the param) — otherwise it's a broken promise.
2. Either also VpcConfig UICodeBuildProject under UseCodeBuildVpc, or document clearly that UI build still needs internet.
3. Fix broken ./artifactory-dependency-workaround.md links in docs/deployment-private-network.md.
4. Update PR description to match implemented parameter names.
5. Add NoEcho: true on UvIndexUrl in patterns/unified/template.yaml.
6. Fix author identity so commits are attributable.

Should‑fix before merge

7. Add CFN Rules enforcing CodeBuildVpcId↔CodeBuildSubnetIds/CodeBuildSecurityGroupId and ArtifactoryDockerUrl↔ArtifactoryCredentialsSecretArn co‑dependency.
8. Squash the two no‑op dompurify commits out of the branch (or rebase on current develop).
9. Run cfn-lint on both templates and paste output.
10. Add a short integration test summary to the PR body (which scenarios were deployed and verified).

Nice‑to‑have

11. Centralize the uv version string in one place.
12. Document the generated airgapped-params-<region>.env file in the script's docs section.

Once items 1–6 are addressed, this would be in good shape to review for merge. I'd be glad to help with any of the fixes — let me know and toggle to Act mode if you want me to push a commit against this branch (or draft a review comment for the PR).