From 2eeb2b3e3f4aa7743da7eeed506c67c4829e61c1 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Fri, 15 May 2026 10:15:11 +0300 Subject: [PATCH 01/24] feat: add ECS and AgentCore container build/package/deploy support Extends SAM CLI to build, package, and deploy container images for AWS::ECS::TaskDefinition and AWS::BedrockAgentCore::Runtime resources using the same Metadata convention as Lambda Image functions. Fixes #8933 --- PR_DESCRIPTION.md | 68 +++++ .../container_image_builds_ecs_agentcore.md | 234 ++++++++++++++++ samcli/commands/build/build_context.py | 35 ++- samcli/commands/build/command.py | 2 + .../companion_stack_manager.py | 8 + samcli/lib/build/app_builder.py | 86 +++++- samcli/lib/build/build_graph.py | 81 ++++++ samcli/lib/package/artifact_exporter.py | 2 + samcli/lib/package/packageable_resources.py | 97 +++++++ .../lib/providers/sam_container_provider.py | 100 +++++++ .../lib/sync/flows/ecs_container_sync_flow.py | 222 +++++++++++++++ samcli/lib/sync/sync_flow_factory.py | 20 ++ samcli/lib/utils/resources.py | 10 + .../test_build_cmd_container_image.py | 63 +++++ .../buildcmd/container_image/agent/Dockerfile | 4 + .../buildcmd/container_image/template.yaml | 33 +++ .../test_container_build_definition.py | 103 +++++++ .../test_container_build_integration.py | 259 ++++++++++++++++++ .../providers/test_sam_container_provider.py | 141 ++++++++++ 19 files changed, 1564 insertions(+), 4 deletions(-) create mode 100644 PR_DESCRIPTION.md create mode 100644 designs/container_image_builds_ecs_agentcore.md create mode 100644 samcli/lib/providers/sam_container_provider.py create mode 100644 samcli/lib/sync/flows/ecs_container_sync_flow.py create mode 100644 tests/integration/buildcmd/test_build_cmd_container_image.py create mode 100644 tests/integration/testdata/buildcmd/container_image/agent/Dockerfile create mode 100644 tests/integration/testdata/buildcmd/container_image/template.yaml create mode 100644 tests/unit/lib/build_module/test_container_build_definition.py create mode 100644 tests/unit/lib/build_module/test_container_build_integration.py create mode 100644 tests/unit/lib/providers/test_sam_container_provider.py diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md new file mode 100644 index 00000000000..2afc1823192 --- /dev/null +++ b/PR_DESCRIPTION.md @@ -0,0 +1,68 @@ +#### Which issue(s) does this change fix? + +Fixes #8933 + +#### Why is this change necessary? + +SAM CLI provides an excellent developer experience for Lambda Image functions (`sam build && sam deploy`), but users deploying containerized workloads to ECS (Fargate) or Bedrock AgentCore must manage their Docker build/push/deploy pipeline separately — even when these resources live in the same CloudFormation template. This creates a fragmented workflow requiring external tooling for an identical operation: build image → push to ECR → deploy. + +#### How does it address the issue? + +Extends the existing Lambda Image build pipeline to recognize `AWS::ECS::TaskDefinition` and `AWS::BedrockAgentCore::Runtime` resources with a `Metadata` block containing `Dockerfile` and `DockerContext`. No new commands — `sam build`, `sam package`, `sam deploy`, and `sam sync` gain awareness of these resource types. + +**Template example:** +```yaml +Resources: + MyAgent: + Type: AWS::BedrockAgentCore::Runtime + Metadata: + Dockerfile: Dockerfile + DockerContext: ./agent + Architecture: arm64 + Properties: + AgentRuntimeArtifact: + ContainerConfiguration: + ContainerUri: placeholder + + MyTask: + Type: AWS::ECS::TaskDefinition + Metadata: + Dockerfile: Dockerfile + DockerContext: ./app + ContainerName: web + Properties: + ContainerDefinitions: + - Name: web + Image: placeholder +``` + +**Key implementation details:** +- Reuses `_build_lambda_image()` — same Docker build logic, buildkit support included +- `--resolve-image-repos` auto-creates ECR repos via companion stack +- `ContainerName` metadata targets specific containers in multi-container TaskDefinitions +- `Architecture` metadata sets `--platform` (e.g., `arm64` for AgentCore) +- `ARTIFACT_TYPE = ZIP` to pass the `PackageType` filter (these resources don't have `PackageType`) +- No SAM Transform changes needed — uses native CloudFormation resource types + +**Design document:** `designs/container_image_builds_ecs_agentcore.md` + +#### What side effects does this change have? + +- `sam build` logs "Found N container service resource(s) to build" when applicable resources are present. No behavior change for templates without these resources. +- `--resolve-image-repos` creates ECR repos for ECS/AgentCore in addition to Lambda Image functions. +- `_update_built_resource` adds an optional `metadata` parameter (backward compatible, defaults to `None`). + +#### Mandatory Checklist +**PRs will only be reviewed after checklist is complete** + +- [x] Review the [generative AI contribution guidelines](https://github.com/aws/aws-sam-cli/blob/develop/CONTRIBUTING.md#ai-usage) +- [x] Add input/output [type hints](https://docs.python.org/3/library/typing.html) to new functions/methods +- [x] Write design document if needed ([Do I need to write a design document?](https://github.com/aws/aws-sam-cli/blob/develop/DEVELOPMENT_GUIDE.md#design-document)) +- [x] Write/update unit tests +- [x] Write/update integration tests +- [x] Write/update functional tests if needed +- [x] `make pr` passes +- [x] `make update-reproducible-reqs` if dependencies were changed +- [ ] Write documentation + +By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license](https://www.apache.org/licenses/LICENSE-2.0). diff --git a/designs/container_image_builds_ecs_agentcore.md b/designs/container_image_builds_ecs_agentcore.md new file mode 100644 index 00000000000..1d67bbf21f4 --- /dev/null +++ b/designs/container_image_builds_ecs_agentcore.md @@ -0,0 +1,234 @@ +Container Image Builds for ECS and AgentCore +============================================= + +This is the design for extending `sam build`, `sam package`, `sam deploy`, and `sam sync` +to support building and deploying container images for non-Lambda resources: +`AWS::ECS::TaskDefinition` and `AWS::BedrockAgentCore::Runtime`. + +What is the problem? +-------------------- + +SAM CLI provides an excellent developer experience for Lambda Image functions: a single +`sam build && sam deploy` builds the Docker image, pushes to ECR, and deploys via +CloudFormation. However, users deploying containerized workloads to ECS (Fargate) or +Bedrock AgentCore must manage their Docker build/push/deploy pipeline separately, even +when these resources live in the same CloudFormation template alongside Lambda functions. + +This creates a fragmented workflow where developers need external tooling (shell scripts, +Makefiles, or CI/CD steps) for the identical operation: build image → push to ECR → +update template with ECR URI → deploy. + +What will be changed? +--------------------- + +We extend the existing Lambda Image build pipeline to recognize `AWS::ECS::TaskDefinition` +and `AWS::BedrockAgentCore::Runtime` resources that have a `Metadata` block with +`Dockerfile` and `DockerContext`. No new commands are introduced — the existing +`sam build`, `sam package`, `sam deploy`, and `sam sync` gain awareness of these +resource types. + +### Design Principles + +1. **Same convention** — Uses the identical Metadata block as Lambda Image functions + (Dockerfile, DockerContext, DockerTag, DockerBuildArgs, DockerBuildTarget) +2. **No Transform changes** — Works with native CloudFormation resource types +3. **Opt-in** — Only resources with the Metadata block are affected; existing templates + work unchanged +4. **Reuse** — Delegates to the same `_build_lambda_image()` Docker build logic + +Success criteria for the change +------------------------------- + +1. `sam build` discovers and builds container images for ECS TaskDefinitions and + AgentCore Runtimes that have Dockerfile metadata +2. `sam deploy --resolve-image-repos` auto-creates ECR repos for these resources +3. `sam package` / `sam deploy` pushes images to ECR and rewrites the template with + the ECR URI at the correct property path +4. `sam sync` builds, pushes, and triggers redeployment for these resources +5. Multi-container ECS TaskDefinitions can target a specific container via `ContainerName` +6. Architecture can be specified via `Architecture` metadata (e.g., `arm64`) +7. Buildkit support works automatically (shared with Lambda Image builds) +8. No regressions for existing Lambda, Layer, or API builds + +User Experience +--------------- + +### Template Format + +```yaml +Resources: + # AgentCore Runtime + MyAgent: + Type: AWS::BedrockAgentCore::Runtime + Metadata: + Dockerfile: Dockerfile + DockerContext: ./agent + DockerTag: latest + Architecture: arm64 + Properties: + AgentRuntimeName: my_agent + AgentRuntimeArtifact: + ContainerConfiguration: + ContainerUri: placeholder + NetworkConfiguration: + NetworkMode: PUBLIC + RoleArn: !GetAtt AgentRole.Arn + + # ECS TaskDefinition (multi-container) + MyTask: + Type: AWS::ECS::TaskDefinition + Metadata: + Dockerfile: Dockerfile + DockerContext: ./app + DockerTag: latest + ContainerName: web + Properties: + Family: my-app + ContainerDefinitions: + - Name: envoy + Image: public.ecr.aws/envoy:latest + - Name: web + Image: placeholder +``` + +### CLI Usage + +```bash +# Build container images +sam build + +# Deploy with auto ECR repo creation +sam deploy --resolve-image-repos + +# Or with explicit repo +sam deploy --image-repositories SimpleAgent=123456789012.dkr.ecr.us-east-1.amazonaws.com/repo + +# Live sync +sam sync --stack-name my-stack --watch --resolve-image-repos +``` + +### Metadata Fields + +| Field | Required | Description | +|-------|----------|-------------| +| `Dockerfile` | Yes | Path to Dockerfile relative to DockerContext | +| `DockerContext` | Yes | Build context directory relative to template | +| `DockerTag` | No | Image tag (default: `latest`) | +| `DockerBuildArgs` | No | Dict of build args | +| `DockerBuildTarget` | No | Multi-stage build target | +| `DockerBuildExtraParams` | No | List of extra docker build params | +| `Architecture` | No | Target platform: `x86_64` (default) or `arm64` | +| `ContainerName` | No | ECS only: target container in multi-container TaskDefinition | + +Implementation +-------------- + +### Architecture + +``` +┌─────────────────────────────────────────────────────────────────┐ +│ sam build │ +├─────────────────────────────────────────────────────────────────┤ +│ BuildContext.run() │ +│ ├── builder.build() → Lambda functions + layers │ +│ └── _build_container_images() → ECS + AgentCore containers │ +│ ├── SamContainerServiceProvider (discovery) │ +│ ├── ContainerBuildDefinition (build graph) │ +│ └── ApplicationBuilder.build_container_images() │ +│ └── _build_lambda_image() (shared Docker logic) │ +├─────────────────────────────────────────────────────────────────┤ +│ sam package / sam deploy │ +│ ├── sync_ecr_stack() → auto-creates ECR repos (companion) │ +│ ├── ECSTaskDefinitionImageResource.export() → push + rewrite │ +│ └── AgentCoreRuntimeImageResource.export() → push + rewrite │ +├─────────────────────────────────────────────────────────────────┤ +│ sam sync │ +│ └── ECSContainerSyncFlow │ +│ ├── gather_resources() → build image │ +│ ├── sync() → push to ECR + force ECS deployment │ +│ └── SyncFlowFactory (registered for both types) │ +└─────────────────────────────────────────────────────────────────┘ +``` + +### Key Components + +**`samcli/lib/providers/sam_container_provider.py`** (new) +- `SamContainerServiceProvider`: Scans stacks for ECS/AgentCore resources with + Dockerfile+DockerContext metadata. Returns `ContainerService` NamedTuples. + +**`samcli/lib/build/build_graph.py`** (modified) +- `ContainerBuildDefinition`: Parallel to `FunctionBuildDefinition`. Holds resource + identifier, type, metadata, and architecture. Reads `Architecture` from metadata. + +**`samcli/lib/build/app_builder.py`** (modified) +- `build_container_images()`: Iterates container definitions and builds each. +- `_build_container_image()`: Delegates to `_build_lambda_image()` — same Docker logic. +- `_update_built_resource()`: Extended for ECS (`ContainerDefinitions[N].Image`) and + AgentCore (`AgentRuntimeArtifact.ContainerConfiguration.ContainerUri`). Accepts + optional `metadata` param for `ContainerName` targeting. + +**`samcli/lib/package/packageable_resources.py`** (modified) +- `ECSTaskDefinitionImageResource`: Custom export for nested `ContainerDefinitions[0].Image`. +- `AgentCoreRuntimeImageResource`: Export using jmespath for deeply nested property path. +- Both use `ARTIFACT_TYPE = ZIP` to pass the `PackageType` filter (these resources + don't have a `PackageType` property). + +**`samcli/lib/sync/flows/ecs_container_sync_flow.py`** (new) +- `ECSContainerSyncFlow`: Builds image, pushes to ECR, forces ECS service redeployment + by finding services using the task definition family. + +**`samcli/lib/bootstrap/companion_stack/companion_stack_manager.py`** (modified) +- `sync_ecr_stack()`: Extended to include container service resources when creating + ECR repos via the companion stack. + +### Property Path Mapping + +| Resource Type | Property Path for Image URI | +|---------------|----------------------------| +| `AWS::Serverless::Function` | `ImageUri` | +| `AWS::Lambda::Function` | `Code.ImageUri` | +| `AWS::ECS::TaskDefinition` | `ContainerDefinitions[N].Image` | +| `AWS::BedrockAgentCore::Runtime` | `AgentRuntimeArtifact.ContainerConfiguration.ContainerUri` | + +Alternatives Considered +----------------------- + +### 1. New SAM Transform resource type (e.g., `AWS::Serverless::ContainerService`) + +**Rejected because:** +- Requires changes to the SAM Transform (separate repo, separate approval process) +- Adds coupling between SAM CLI and the Transform +- Users would need to wait for Transform support in all regions +- Native CFN types already work and are well-understood + +### 2. Separate `sam container build` command + +**Rejected because:** +- Fragments the workflow — users would need to remember different commands +- Doesn't integrate with `sam deploy` and `sam sync` naturally +- The existing `sam build` already handles image builds for Lambda + +### 3. Using `PackageType: Image` on ECS/AgentCore resources + +**Rejected because:** +- `PackageType` is a Lambda-specific concept not present on ECS or AgentCore resources +- Would require CloudFormation schema changes +- The Metadata-based approach is already the established pattern + +Breaking Changes +---------------- + +None. This is purely additive: +- Templates without ECS/AgentCore Metadata are unaffected +- The `_update_built_resource` signature change is backward compatible (optional param) +- No existing CLI flags or behaviors change + +Future Extensions +----------------- + +1. **Multiple Dockerfiles per ECS TaskDefinition** — Build multiple containers from + one resource using a list of Metadata entries +2. **`sam local start-ecs`** — Local testing of ECS containers (similar to `sam local start-api`) +3. **Health check integration** — Wait for container health before marking sync complete +4. **Build caching** — Layer-aware caching for container builds (currently rebuilds fully) +5. **`sam init` templates** — Starter templates for ECS+SAM and AgentCore+SAM projects diff --git a/samcli/commands/build/build_context.py b/samcli/commands/build/build_context.py index 1c6355b030c..d1fbb743063 100644 --- a/samcli/commands/build/build_context.py +++ b/samcli/commands/build/build_context.py @@ -29,7 +29,7 @@ BuildError, UnsupportedBuilderLibraryVersionError, ) -from samcli.lib.build.build_graph import DEFAULT_DEPENDENCIES_DIR +from samcli.lib.build.build_graph import DEFAULT_DEPENDENCIES_DIR, ContainerBuildDefinition from samcli.lib.build.bundler import EsbuildBundlerManager from samcli.lib.build.exceptions import ( BuildInsideContainerError, @@ -45,6 +45,7 @@ from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable from samcli.lib.providers.provider import LayerVersion, ResourcesToBuildCollector, Stack from samcli.lib.providers.sam_api_provider import SamApiProvider +from samcli.lib.providers.sam_container_provider import SamContainerServiceProvider from samcli.lib.providers.sam_function_provider import SamFunctionProvider from samcli.lib.providers.sam_layer_provider import SamLayerProvider from samcli.lib.providers.sam_stack_provider import SamLocalStackProvider @@ -302,6 +303,11 @@ def run(self) -> None: self._build_result = builder.build() + # Build container images for ECS/AgentCore resources + container_artifacts = self._build_container_images(builder) + if container_artifacts: + self._build_result.artifacts.update(container_artifacts) + self._handle_build_post_processing(builder, self._build_result) click.secho("\nBuild Succeeded", fg="green") @@ -1192,6 +1198,33 @@ def collect_all_build_resources(self) -> ResourcesToBuildCollector: ) return result + def _build_container_images(self, builder: ApplicationBuilder) -> Dict[str, str]: + """ + Discover and build container images for ECS/AgentCore resources. + + Returns + ------- + Dict[str, str] + Map of resource full_path to built image tag + """ + container_provider = SamContainerServiceProvider(self.stacks) + container_services = list(container_provider.get_all()) + if not container_services: + return {} + + LOG.info("Found %d container service resource(s) to build", len(container_services)) + + container_build_defs = [] + for service in container_services: + build_def = ContainerBuildDefinition( + resource_identifier=service.full_path, + resource_type=service.resource_type, + metadata=service.metadata, + ) + container_build_defs.append(build_def) + + return builder.build_container_images(container_build_defs) + @property def is_building_specific_resource(self) -> bool: """ diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index 7a77f19d53c..8c58edfcaf4 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -54,6 +54,8 @@ 2. AWS::Lambda::Function\n 3. AWS::Serverless::LayerVersion\n 4. AWS::Lambda::LayerVersion\n + 5. AWS::ECS::TaskDefinition (container image)\n + 6. AWS::BedrockAgentCore::Runtime (container image)\n \b Supported Runtimes ------------------ diff --git a/samcli/lib/bootstrap/companion_stack/companion_stack_manager.py b/samcli/lib/bootstrap/companion_stack/companion_stack_manager.py index cfe95e675d9..746d8f1beb4 100644 --- a/samcli/lib/bootstrap/companion_stack/companion_stack_manager.py +++ b/samcli/lib/bootstrap/companion_stack/companion_stack_manager.py @@ -312,6 +312,14 @@ def sync_ecr_stack( function_logical_ids = [ function.full_path for function in function_provider.get_all() if function.packagetype == IMAGE ] + + # Also include ECS/AgentCore container resources that need ECR repos + from samcli.lib.providers.sam_container_provider import SamContainerServiceProvider + + container_provider = SamContainerServiceProvider(stacks) + container_logical_ids = [service.full_path for service in container_provider.get_all()] + function_logical_ids.extend(container_logical_ids) + manager.set_functions(function_logical_ids, image_repositories) image_repositories.update(manager.get_repository_mapping()) manager.sync_repos() diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index c30c948c007..158d6569c2a 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -19,7 +19,12 @@ from aws_lambda_builders.exceptions import LambdaBuilderError from samcli.commands._utils.experimental import get_enabled_experimental_flags -from samcli.lib.build.build_graph import BuildGraph, FunctionBuildDefinition, LayerBuildDefinition +from samcli.lib.build.build_graph import ( + BuildGraph, + ContainerBuildDefinition, + FunctionBuildDefinition, + LayerBuildDefinition, +) from samcli.lib.build.build_strategy import ( BuildStrategy, CachedOrIncrementalBuildStrategyWrapper, @@ -51,7 +56,9 @@ from samcli.lib.utils.packagetype import IMAGE, ZIP from samcli.lib.utils.path_utils import check_path_valid_type, convert_path_to_unix_path from samcli.lib.utils.resources import ( + AWS_BEDROCK_AGENTCORE_RUNTIME, AWS_CLOUDFORMATION_STACK, + AWS_ECS_TASK_DEFINITION, AWS_LAMBDA_FUNCTION, AWS_LAMBDA_LAYERVERSION, AWS_SERVERLESS_APPLICATION, @@ -378,7 +385,11 @@ def update_template( if has_build_artifact: ApplicationBuilder._update_built_resource( - built_artifacts[full_path], properties, resource_type or "", store_path + built_artifacts[full_path], + properties, + resource_type or "", + store_path, + resource.get("Metadata"), ) if is_stack: @@ -391,7 +402,9 @@ def update_template( return template_dict @staticmethod - def _update_built_resource(path: str, resource_properties: Dict, resource_type: str, absolute_path: str) -> None: + def _update_built_resource( + path: str, resource_properties: Dict, resource_type: str, absolute_path: str, metadata: Optional[Dict] = None + ) -> None: if resource_type == AWS_SERVERLESS_FUNCTION and resource_properties.get("PackageType", ZIP) == ZIP: resource_properties["CodeUri"] = absolute_path if resource_type == AWS_LAMBDA_FUNCTION and resource_properties.get("PackageType", ZIP) == ZIP: @@ -404,6 +417,26 @@ def _update_built_resource(path: str, resource_properties: Dict, resource_type: resource_properties["Code"] = {"ImageUri": path} if resource_type == AWS_SERVERLESS_FUNCTION and resource_properties.get("PackageType", ZIP) == IMAGE: resource_properties["ImageUri"] = path + if resource_type == AWS_ECS_TASK_DEFINITION: + container_defs = resource_properties.get("ContainerDefinitions", []) + if container_defs: + target_name = metadata.get("ContainerName") if metadata else None + if target_name: + for container_def in container_defs: + if container_def.get("Name") == target_name: + container_def["Image"] = path + break + else: + raise DockerBuildFailed( + f"Metadata.ContainerName '{target_name}' does not match any " + f"container in ContainerDefinitions" + ) + else: + container_defs[0]["Image"] = path + if resource_type == AWS_BEDROCK_AGENTCORE_RUNTIME: + artifact = resource_properties.setdefault("AgentRuntimeArtifact", {}) + container_config = artifact.setdefault("ContainerConfiguration", {}) + container_config["ContainerUri"] = path def _build_lambda_image(self, function_name: str, metadata: Dict, architecture: str) -> str: """ @@ -538,6 +571,53 @@ def _load_lambda_image(self, image_archive_path: str) -> str: except (docker.errors.APIError, OSError, ContainerArchiveImageLoadFailedException) as ex: raise DockerBuildFailed(msg=str(ex)) from ex + def build_container_images(self, container_build_definitions: List[ContainerBuildDefinition]) -> Dict[str, str]: + """ + Build container images for non-Lambda resources (ECS, AgentCore). + + Parameters + ---------- + container_build_definitions : List[ContainerBuildDefinition] + List of container build definitions to build + + Returns + ------- + Dict[str, str] + Map of resource identifier to the built image tag + """ + artifacts: Dict[str, str] = {} + for container_def in container_build_definitions: + if not container_def.metadata: + continue + image_tag = self._build_container_image( + container_def.resource_identifier, + container_def.metadata, + container_def.architecture, + ) + artifacts[container_def.resource_identifier] = image_tag + return artifacts + + def _build_container_image(self, resource_name: str, metadata: Dict, architecture: str) -> str: + """ + Build a container image for an ECS/AgentCore resource. Uses the same Metadata + convention as Lambda Image builds (Dockerfile, DockerContext, DockerBuildArgs, etc.) + + Parameters + ---------- + resource_name : str + Logical ID or identifier of the resource + metadata : dict + Metadata block from the resource + architecture : str + Target architecture + + Returns + ------- + str + The full tag of the built image + """ + return self._build_lambda_image(resource_name, metadata, architecture) + def _build_layer( self, layer_name: str, diff --git a/samcli/lib/build/build_graph.py b/samcli/lib/build/build_graph.py index c31ee039438..e2199e79902 100644 --- a/samcli/lib/build/build_graph.py +++ b/samcli/lib/build/build_graph.py @@ -200,12 +200,14 @@ class BuildGraph: # global table build definitions key FUNCTION_BUILD_DEFINITIONS = "function_build_definitions" LAYER_BUILD_DEFINITIONS = "layer_build_definitions" + CONTAINER_BUILD_DEFINITIONS = "container_build_definitions" def __init__(self, build_dir: str) -> None: # put build.toml file inside .aws-sam folder self._filepath = Path(build_dir).parent.joinpath(DEFAULT_BUILD_GRAPH_FILE_NAME) self._function_build_definitions: List["FunctionBuildDefinition"] = [] self._layer_build_definitions: List["LayerBuildDefinition"] = [] + self._container_build_definitions: List["ContainerBuildDefinition"] = [] self._atomic_read() def get_function_build_definitions(self) -> Tuple["FunctionBuildDefinition", ...]: @@ -214,6 +216,21 @@ def get_function_build_definitions(self) -> Tuple["FunctionBuildDefinition", ... def get_layer_build_definitions(self) -> Tuple["LayerBuildDefinition", ...]: return tuple(self._layer_build_definitions) + def get_container_build_definitions(self) -> Tuple["ContainerBuildDefinition", ...]: + return tuple(self._container_build_definitions) + + def put_container_build_definition(self, container_build_definition: "ContainerBuildDefinition") -> None: + """ + Puts the newly read container build definition into existing build graph. + Each container build definition is unique per resource identifier. + """ + if container_build_definition not in self._container_build_definitions: + LOG.debug( + "Adding container build definition: %s", + container_build_definition, + ) + self._container_build_definitions.append(container_build_definition) + def get_function_build_definition_with_full_path( self, function_full_path: str ) -> Optional["FunctionBuildDefinition"]: @@ -701,3 +718,67 @@ def __eq__(self, other: Any) -> bool: and self.env_vars == other.env_vars and self.architecture == other.architecture ) + + +class ContainerBuildDefinition(AbstractBuildDefinition): + """ + ContainerBuildDefinition holds information about each unique container image build + for non-Lambda resources (ECS TaskDefinitions, AgentCore, etc.) + """ + + def __init__( + self, + resource_identifier: str, + resource_type: str, + metadata: Optional[Dict], + source_hash: str = "", + manifest_hash: str = "", + env_vars: Optional[Dict] = None, + architecture: str = X86_64, + ) -> None: + # Allow metadata to override architecture (e.g., "arm64" for AgentCore) + if metadata and metadata.get("Architecture"): + architecture = metadata["Architecture"] + super().__init__(source_hash, manifest_hash, env_vars, architecture) + self.resource_identifier = resource_identifier + self.resource_type = resource_type + + metadata_copied = deepcopy(metadata) if metadata else {} + metadata_copied.pop(SAM_RESOURCE_ID_KEY, "") + metadata_copied.pop(SAM_IS_NORMALIZED, "") + self.metadata = metadata_copied + + @property + def dockerfile(self) -> Optional[str]: + return self.metadata.get("Dockerfile") if self.metadata else None + + @property + def docker_context(self) -> Optional[str]: + return self.metadata.get("DockerContext") if self.metadata else None + + @property + def docker_build_args(self) -> Dict: + return self.metadata.get("DockerBuildArgs", {}) if self.metadata else {} + + @property + def docker_build_target(self) -> Optional[str]: + return self.metadata.get("DockerBuildTarget") if self.metadata else None + + def get_resource_full_paths(self) -> str: + return self.resource_identifier + + def __str__(self) -> str: + return ( + f"ContainerBuildDefinition({self.resource_identifier}, {self.resource_type}, " + f"{self.source_hash}, {self.uuid}, {self.metadata}, {self.architecture})" + ) + + def __eq__(self, other: Any) -> bool: + if not isinstance(other, ContainerBuildDefinition): + return False + return ( + self.resource_identifier == other.resource_identifier + and self.resource_type == other.resource_type + and self.metadata == other.metadata + and self.architecture == other.architecture + ) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index e672c6a75b9..cfa06123a3b 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -492,6 +492,7 @@ def export(self) -> Dict: # Export code resources exporter = exporter_class(self.uploaders, self.code_signer, cache) exporter.parent_parameter_values = self.parameter_values + exporter.resource_metadata = resource.get("Metadata") exporter.export(full_path, resource_dict, self.template_dir) return self.template_dict @@ -519,6 +520,7 @@ def delete(self, retain_resources: List): continue # Delete code resources exporter = exporter_class(self.uploaders, None) + exporter.resource_metadata = resource.get("Metadata") exporter.delete(resource_id, resource_dict) def get_ecr_repos(self): diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index d63136df8fb..87f8c484891 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -33,9 +33,11 @@ AWS_APPSYNC_FUNCTIONCONFIGURATION, AWS_APPSYNC_GRAPHQLSCHEMA, AWS_APPSYNC_RESOLVER, + AWS_BEDROCK_AGENTCORE_RUNTIME, AWS_CLOUDFORMATION_MODULEVERSION, AWS_CLOUDFORMATION_RESOURCEVERSION, AWS_ECR_REPOSITORY, + AWS_ECS_TASK_DEFINITION, AWS_ELASTICBEANSTALK_APPLICATIONVERSION, AWS_GLUE_JOB, AWS_LAMBDA_FUNCTION, @@ -688,6 +690,99 @@ def export(self, resource_id: str, resource_dict: Optional[Dict], parent_dir: st shutil.rmtree(temp_dir) +class ECSTaskDefinitionImageResource(ResourceImage): + """ + Represents an ECS TaskDefinition with a container image that needs to be pushed to ECR. + The image reference is at ContainerDefinitions[0].Image. + """ + + RESOURCE_TYPE = AWS_ECS_TASK_DEFINITION + PROPERTY_NAME = RESOURCES_WITH_IMAGE_COMPONENT[RESOURCE_TYPE][0] + ARTIFACT_TYPE = ZIP # No PackageType property; defaults to ZIP in the filter + + def _get_target_index(self, container_defs): + """Find the target container index using ContainerName from Metadata.""" + metadata = getattr(self, "resource_metadata", None) or {} + target_name = metadata.get("ContainerName") + if target_name: + for i, cd in enumerate(container_defs): + if cd.get("Name") == target_name: + return i + raise exceptions.ExportFailedError( + resource_id="", + property_name=self.PROPERTY_NAME, + property_value=target_name, + ex=ValueError( + f"Metadata.ContainerName '{target_name}' does not match any " f"container in ContainerDefinitions" + ), + ) + return 0 + + def export(self, resource_id, resource_dict, parent_dir): + if resource_dict is None: + return + + container_defs = resource_dict.get("ContainerDefinitions", []) + if not container_defs: + return + + target_idx = self._get_target_index(container_defs) + property_value = container_defs[target_idx].get("Image") + if not property_value or isinstance(property_value, dict) or is_ecr_url(property_value): + return + + try: + uploaded_url = self.uploader.upload(property_value, resource_id) + container_defs[target_idx]["Image"] = uploaded_url + except Exception as ex: + LOG.debug("Unable to export", exc_info=ex) + raise exceptions.ExportFailedError( + resource_id=resource_id, + property_name=self.PROPERTY_NAME, + property_value=property_value, + ex=ex, + ) + + def delete(self, resource_id, resource_dict): + if resource_dict is None: + return + container_defs = resource_dict.get("ContainerDefinitions", []) + if not container_defs: + return + target_idx = self._get_target_index(container_defs) + remote_path = container_defs[target_idx].get("Image") + if isinstance(remote_path, str) and is_ecr_url(remote_path): + self.uploader.delete_artifact( + image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME + ) + + +class AgentCoreRuntimeImageResource(ResourceImage): + """ + Represents a Bedrock AgentCore Runtime resource with a container image. + Property path: AgentRuntimeArtifact.ContainerConfiguration.ContainerUri + """ + + RESOURCE_TYPE = AWS_BEDROCK_AGENTCORE_RUNTIME + PROPERTY_NAME = RESOURCES_WITH_IMAGE_COMPONENT[RESOURCE_TYPE][0] + ARTIFACT_TYPE = ZIP # No PackageType property; defaults to ZIP in the filter + + def do_export(self, resource_id, resource_dict, parent_dir): + uploaded_url = upload_local_image_artifacts( + resource_id, resource_dict, self.PROPERTY_NAME, parent_dir, self.uploader + ) + set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, uploaded_url) + + def delete(self, resource_id, resource_dict): + if resource_dict is None: + return + remote_path = jmespath.search(self.PROPERTY_NAME, resource_dict) + if isinstance(remote_path, str) and is_ecr_url(remote_path): + self.uploader.delete_artifact( + image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME + ) + + RESOURCES_EXPORT_LIST = [ ServerlessFunctionResource, ServerlessFunctionImageResource, @@ -713,6 +808,8 @@ def export(self, resource_id: str, resource_dict: Optional[Dict], parent_dir: st CloudFormationModuleVersionModulePackage, CloudFormationResourceVersionSchemaHandlerPackage, ECRResource, + ECSTaskDefinitionImageResource, + AgentCoreRuntimeImageResource, GraphQLApiSchemaResource, GraphQLApiCodeResource, ] diff --git a/samcli/lib/providers/sam_container_provider.py b/samcli/lib/providers/sam_container_provider.py new file mode 100644 index 00000000000..db9689a4104 --- /dev/null +++ b/samcli/lib/providers/sam_container_provider.py @@ -0,0 +1,100 @@ +""" +Class that provides container service resources (ECS, AgentCore) from a given SAM/CFN template +""" + +import logging +from typing import Dict, Iterator, List, NamedTuple, Optional + +from samcli.lib.providers.provider import Stack, get_full_path +from samcli.lib.utils.resources import ( + AWS_BEDROCK_AGENTCORE_RUNTIME, + AWS_ECS_TASK_DEFINITION, +) + +LOG = logging.getLogger(__name__) + +# Resource types that support container image builds +CONTAINER_IMAGE_RESOURCE_TYPES = [ + AWS_ECS_TASK_DEFINITION, + AWS_BEDROCK_AGENTCORE_RUNTIME, +] + + +class ContainerService(NamedTuple): + """ + Represents a container service resource that needs an image built. + """ + + # Logical ID of the resource + resource_id: str + # Full path including stack path + full_path: str + # Resource type (e.g., AWS::ECS::TaskDefinition) + resource_type: str + # Metadata dict containing Dockerfile, DockerContext, etc. + metadata: Dict + # Resource properties + properties: Dict + # Stack path + stack_path: str = "" + + +class SamContainerServiceProvider: + """ + Extracts container service resources from SAM/CFN templates that have + Metadata with Dockerfile and DockerContext, indicating they need image builds. + """ + + def __init__(self, stacks: List[Stack]) -> None: + self._stacks = stacks + self._container_services: Dict[str, ContainerService] = {} + self._extract_container_services() + + def _extract_container_services(self) -> None: + for stack in self._stacks: + resources = getattr(stack, "resources", None) + if not resources or not isinstance(resources, dict): + continue + for logical_id, resource in resources.items(): + resource_type = resource.get("Type", "") + if resource_type not in CONTAINER_IMAGE_RESOURCE_TYPES: + continue + + metadata = resource.get("Metadata", {}) + if not self._has_container_build_metadata(metadata): + continue + + full_path = get_full_path(stack.stack_path, logical_id) + properties = resource.get("Properties", {}) + + container_service = ContainerService( + resource_id=logical_id, + full_path=full_path, + resource_type=resource_type, + metadata=metadata, + properties=properties, + stack_path=stack.stack_path, + ) + self._container_services[full_path] = container_service + LOG.debug("Found container service resource: %s (%s)", full_path, resource_type) + + @staticmethod + def _has_container_build_metadata(metadata: Optional[Dict]) -> bool: + """Check if metadata contains the required fields for a container image build.""" + if not metadata: + return False + return bool(metadata.get("Dockerfile") and metadata.get("DockerContext")) + + def get(self, name: str) -> Optional[ContainerService]: + """Get a container service by full path or logical ID.""" + if name in self._container_services: + return self._container_services[name] + # Try matching by logical ID alone + for full_path, service in self._container_services.items(): + if service.resource_id == name: + return service + return None + + def get_all(self) -> Iterator[ContainerService]: + """Return all container services.""" + yield from self._container_services.values() diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py new file mode 100644 index 00000000000..4d4149525be --- /dev/null +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -0,0 +1,222 @@ +"""SyncFlow for ECS TaskDefinition container image resources""" + +import logging +from typing import TYPE_CHECKING, Any, Dict, List, Optional + +from botocore.exceptions import ClientError +from docker.client import DockerClient + +from samcli.lib.build.app_builder import ApplicationBuilder, ApplicationBuildResult +from samcli.lib.build.build_graph import ContainerBuildDefinition +from samcli.lib.package.ecr_uploader import ECRUploader +from samcli.lib.providers.provider import Stack +from samcli.lib.providers.sam_container_provider import SamContainerServiceProvider +from samcli.lib.sync.sync_flow import ApiCallTypes, ResourceAPICall, SyncFlow +from samcli.local.docker.utils import get_validated_container_client + +if TYPE_CHECKING: # pragma: no cover + from samcli.commands.build.build_context import BuildContext + from samcli.commands.deploy.deploy_context import DeployContext + from samcli.commands.sync.sync_context import SyncContext + +LOG = logging.getLogger(__name__) + + +class ECSContainerSyncFlow(SyncFlow): + """SyncFlow for ECS TaskDefinition and AgentCore container image resources. + + Builds the container image, pushes to ECR, and triggers an ECS service update. + """ + + _resource_identifier: str + _ecr_client: Optional[Any] + _docker_client: Optional[DockerClient] + _ecs_client: Optional[Any] + _image_name: Optional[str] + + def __init__( + self, + resource_identifier: str, + build_context: "BuildContext", + deploy_context: "DeployContext", + sync_context: "SyncContext", + physical_id_mapping: Dict[str, str], + stacks: List[Stack], + application_build_result: Optional[ApplicationBuildResult], + ): + super().__init__( + build_context, + deploy_context, + sync_context, + physical_id_mapping, + f"ECSContainer {resource_identifier}", + stacks, + application_build_result, + ) + self._resource_identifier = resource_identifier + self._ecr_client = None + self._docker_client = None + self._ecs_client = None + self._image_name = None + + @property + def sync_state_identifier(self) -> str: + return self._resource_identifier + + def _get_docker_client(self) -> DockerClient: + if not self._docker_client: + self._docker_client = get_validated_container_client() + return self._docker_client + + def _get_ecr_client(self) -> Any: + if not self._ecr_client: + self._ecr_client = self._boto_client("ecr") + return self._ecr_client + + def _get_ecs_client(self) -> Any: + if not self._ecs_client: + self._ecs_client = self._boto_client("ecs") + return self._ecs_client + + def gather_resources(self) -> None: + """Build the container image.""" + if self._application_build_result: + self._image_name = self._application_build_result.artifacts.get(self._resource_identifier) + else: + self._build_from_scratch() + + if self._image_name: + docker_img = self._get_docker_client().images.get(self._image_name) + if docker_img and docker_img.attrs.get("Id"): + self._local_sha = str(docker_img.attrs.get("Id")) + + def _build_from_scratch(self) -> None: + """Build the container image from scratch using the provider.""" + container_provider = SamContainerServiceProvider(self._stacks or []) + service = container_provider.get(self._resource_identifier) + if not service: + LOG.warning("Cannot find container service resource '%s'", self._resource_identifier) + return + + build_def = ContainerBuildDefinition( + resource_identifier=service.full_path, + resource_type=service.resource_type, + metadata=service.metadata, + ) + + builder = ApplicationBuilder( + ( + self._build_context.collect_build_resources(self._resource_identifier) + if hasattr(self._build_context, "collect_build_resources") + else self._build_context.get_resources_to_build() + ), + self._build_context.build_dir, + self._build_context.base_dir, + self._build_context.cache_dir, + cached=False, + is_building_specific_resource=True, + manifest_path_override=self._build_context.manifest_path_override, + container_manager=self._build_context.container_manager, + mode=self._build_context.mode, + build_in_source=self._build_context.build_in_source, + ) + artifacts = builder.build_container_images([build_def]) + self._image_name = artifacts.get(self._resource_identifier) + + def compare_remote(self) -> bool: + return False + + def sync(self) -> None: + if not self._image_name: + LOG.debug("%sSkipping sync. Image name is None.", self.log_prefix) + return + + # Get ECR repo from deploy context + ecr_repo = self._deploy_context.image_repository + if ( + not ecr_repo + and self._deploy_context.image_repositories + and isinstance(self._deploy_context.image_repositories, dict) + ): + ecr_repo = self._deploy_context.image_repositories.get(self._resource_identifier) + + if not ecr_repo: + LOG.warning( + "%sNo ECR repository configured for %s. " "Use --image-repository or --image-repositories.", + self.log_prefix, + self._resource_identifier, + ) + return + + # Push image to ECR + ecr_uploader = ECRUploader(self._get_docker_client(), self._get_ecr_client(), ecr_repo, None) + ecr_uploader.upload(self._image_name, self._resource_identifier) + + # Force new deployment of ECS service if one is associated + self._force_ecs_deployment() + + def _force_ecs_deployment(self) -> None: + """Force new deployment for ECS services in the stack that use this task definition.""" + physical_id = self._physical_id_mapping.get(self._resource_identifier) + if not physical_id: + LOG.debug("%sNo physical ID found for %s, skipping ECS update", self.log_prefix, self._resource_identifier) + return + + try: + ecs_client = self._get_ecs_client() + + # Find ECS services in the same stack by looking at physical_id_mapping + # ECS Service physical IDs are ARNs like arn:aws:ecs:region:account:service/cluster/name + for resource_id, resource_physical_id in self._physical_id_mapping.items(): + if not resource_physical_id or "service/" not in str(resource_physical_id): + continue + # Check if this service uses our task definition + try: + # Extract cluster and service from the ARN + parts = resource_physical_id.rsplit("/", 2) + if len(parts) < 3: + continue + cluster = parts[-2] + service_name = parts[-1] + + svc_response = ecs_client.describe_services(cluster=cluster, services=[service_name]) + for svc in svc_response.get("services", []): + svc_task_def = svc.get("taskDefinition", "") + # Check if this service references our task definition family + if physical_id in svc_task_def or ( + svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0] + == physical_id.rsplit("/", 1)[-1].split(":", 1)[0] + ): + ecs_client.update_service( + cluster=cluster, + service=service_name, + forceNewDeployment=True, + ) + LOG.info( + "%sForced new deployment for service %s", + self.log_prefix, + service_name, + ) + except ClientError: + LOG.warning( + "%sFailed to update service %s", + self.log_prefix, + resource_id, + exc_info=True, + ) + except ClientError: + LOG.warning("%sFailed to force ECS deployment", self.log_prefix, exc_info=True) + + def gather_dependencies(self) -> List[SyncFlow]: + return [] + + def _get_resource_api_calls(self) -> List[ResourceAPICall]: + return [ + ResourceAPICall( + self._resource_identifier, + [ApiCallTypes.UPDATE_FUNCTION_CODE], + ) + ] + + def _equality_keys(self) -> Any: + return self._resource_identifier diff --git a/samcli/lib/sync/sync_flow_factory.py b/samcli/lib/sync/sync_flow_factory.py index 4fbdda54b3b..0fb33e22cd8 100644 --- a/samcli/lib/sync/sync_flow_factory.py +++ b/samcli/lib/sync/sync_flow_factory.py @@ -12,6 +12,7 @@ from samcli.lib.package.utils import is_local_folder, is_zip_file from samcli.lib.providers.provider import Function, FunctionBuildInfo, ResourceIdentifier, Stack from samcli.lib.sync.flows.auto_dependency_layer_sync_flow import AutoDependencyLayerParentSyncFlow +from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow from samcli.lib.sync.flows.function_sync_flow import FunctionSyncFlow from samcli.lib.sync.flows.http_api_sync_flow import HttpApiSyncFlow from samcli.lib.sync.flows.image_function_sync_flow import ImageFunctionSyncFlow @@ -39,6 +40,8 @@ from samcli.lib.utils.resources import ( AWS_APIGATEWAY_RESTAPI, AWS_APIGATEWAY_V2_API, + AWS_BEDROCK_AGENTCORE_RUNTIME, + AWS_ECS_TASK_DEFINITION, AWS_LAMBDA_FUNCTION, AWS_LAMBDA_LAYERVERSION, AWS_SERVERLESS_API, @@ -338,6 +341,21 @@ def _create_stepfunctions_flow( self._stacks, ) + def _create_ecs_container_flow( + self, + resource_identifier: ResourceIdentifier, + application_build_result: Optional[ApplicationBuildResult], + ) -> Optional[SyncFlow]: + return ECSContainerSyncFlow( + str(resource_identifier), + self._build_context, + self._deploy_context, + self._sync_context, + self._physical_id_mapping, + self._stacks, + application_build_result, + ) + GeneratorFunction = Callable[ ["SyncFlowFactory", ResourceIdentifier, Optional[ApplicationBuildResult]], Optional[SyncFlow] ] @@ -352,6 +370,8 @@ def _create_stepfunctions_flow( AWS_APIGATEWAY_V2_API: _create_api_flow, AWS_SERVERLESS_STATEMACHINE: _create_stepfunctions_flow, AWS_STEPFUNCTIONS_STATEMACHINE: _create_stepfunctions_flow, + AWS_ECS_TASK_DEFINITION: _create_ecs_container_flow, + AWS_BEDROCK_AGENTCORE_RUNTIME: _create_ecs_container_flow, } # SyncFlow mapping between resource type and creation function diff --git a/samcli/lib/utils/resources.py b/samcli/lib/utils/resources.py index cb99abc8d61..0d8a75e681b 100644 --- a/samcli/lib/utils/resources.py +++ b/samcli/lib/utils/resources.py @@ -58,6 +58,14 @@ AWS_SERVERLESS_STATEMACHINE = "AWS::Serverless::StateMachine" AWS_STEPFUNCTIONS_STATEMACHINE = "AWS::StepFunctions::StateMachine" AWS_ECR_REPOSITORY = "AWS::ECR::Repository" + +# ECS +AWS_ECS_TASK_DEFINITION = "AWS::ECS::TaskDefinition" +AWS_ECS_SERVICE = "AWS::ECS::Service" + +# AgentCore +AWS_BEDROCK_AGENTCORE_RUNTIME = "AWS::BedrockAgentCore::Runtime" + AWS_APPLICATION_INSIGHTS = "AWS::ApplicationInsights::Application" AWS_RESOURCE_GROUP = "AWS::ResourceGroups::Group" @@ -99,6 +107,8 @@ AWS_SERVERLESS_FUNCTION: ["ImageUri"], AWS_LAMBDA_FUNCTION: ["Code.ImageUri"], AWS_ECR_REPOSITORY: ["RepositoryName"], + AWS_ECS_TASK_DEFINITION: ["ContainerDefinitions.Image"], + AWS_BEDROCK_AGENTCORE_RUNTIME: ["AgentRuntimeArtifact.ContainerConfiguration.ContainerUri"], } NESTED_STACKS_RESOURCES = { diff --git a/tests/integration/buildcmd/test_build_cmd_container_image.py b/tests/integration/buildcmd/test_build_cmd_container_image.py new file mode 100644 index 00000000000..c34bd7cab90 --- /dev/null +++ b/tests/integration/buildcmd/test_build_cmd_container_image.py @@ -0,0 +1,63 @@ +"""Integration test for ECS/AgentCore container image builds""" + +import os +import shutil +import tempfile +from pathlib import Path +from unittest import TestCase + +import yaml + +from samcli.commands.build.build_context import BuildContext + + +class TestContainerImageBuild(TestCase): + """Test that samdev build correctly handles ECS and AgentCore container resources.""" + + template_path = str( + Path(__file__).resolve().parents[1] / "testdata" / "buildcmd" / "container_image" / "template.yaml" + ) + + def setUp(self): + self.build_dir = tempfile.mkdtemp() + self.cache_dir = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.build_dir, ignore_errors=True) + shutil.rmtree(self.cache_dir, ignore_errors=True) + + def test_build_discovers_and_builds_container_resources(self): + """Verify that sam build discovers AgentCore and ECS resources and produces built artifacts.""" + with BuildContext( + resource_identifier=None, + template_file=self.template_path, + base_dir=None, + build_dir=self.build_dir, + cache_dir=self.cache_dir, + cached=False, + parallel=False, + mode=None, + ) as ctx: + ctx.run() + + # Verify the output template was created + output_template = Path(self.build_dir) / "template.yaml" + self.assertTrue(output_template.exists(), "Built template should exist") + + with open(output_template) as f: + built_template = yaml.safe_load(f) + + resources = built_template["Resources"] + + # AgentCore: ContainerUri should be replaced with the built image tag + agent_uri = resources["SimpleAgent"]["Properties"]["AgentRuntimeArtifact"]["ContainerConfiguration"][ + "ContainerUri" + ] + self.assertNotEqual(agent_uri, "placeholder") + self.assertIn("simpleagent", agent_uri.lower()) + + # ECS: The 'web' container (second one) should be updated, sidecar unchanged + container_defs = resources["ECSTask"]["Properties"]["ContainerDefinitions"] + self.assertEqual(container_defs[0]["Image"], "public.ecr.aws/envoy:latest") # sidecar untouched + self.assertNotEqual(container_defs[1]["Image"], "placeholder") # web updated + self.assertIn("ecstask", container_defs[1]["Image"].lower()) diff --git a/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile b/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile new file mode 100644 index 00000000000..57f45c64404 --- /dev/null +++ b/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile @@ -0,0 +1,4 @@ +FROM python:3.12-slim +WORKDIR /app +RUN echo "hello" > /app/test.txt +CMD ["python", "-c", "print('test')"] diff --git a/tests/integration/testdata/buildcmd/container_image/template.yaml b/tests/integration/testdata/buildcmd/container_image/template.yaml new file mode 100644 index 00000000000..b261dc611fa --- /dev/null +++ b/tests/integration/testdata/buildcmd/container_image/template.yaml @@ -0,0 +1,33 @@ +AWSTemplateFormatVersion: '2010-09-09' +Description: Integration test for ECS/AgentCore container builds + +Resources: + SimpleAgent: + Type: AWS::BedrockAgentCore::Runtime + Metadata: + Dockerfile: Dockerfile + DockerContext: ./agent + DockerTag: latest + Properties: + AgentRuntimeName: test_agent + AgentRuntimeArtifact: + ContainerConfiguration: + ContainerUri: placeholder + NetworkConfiguration: + NetworkMode: PUBLIC + RoleArn: arn:aws:iam::123456789012:role/TestRole + + ECSTask: + Type: AWS::ECS::TaskDefinition + Metadata: + Dockerfile: Dockerfile + DockerContext: ./agent + DockerTag: latest + ContainerName: web + Properties: + Family: test-task + ContainerDefinitions: + - Name: sidecar + Image: public.ecr.aws/envoy:latest + - Name: web + Image: placeholder diff --git a/tests/unit/lib/build_module/test_container_build_definition.py b/tests/unit/lib/build_module/test_container_build_definition.py new file mode 100644 index 00000000000..1ab39fc7b95 --- /dev/null +++ b/tests/unit/lib/build_module/test_container_build_definition.py @@ -0,0 +1,103 @@ +"""Tests for ContainerBuildDefinition in build_graph.py""" + +from unittest import TestCase + +from samcli.lib.build.build_graph import ContainerBuildDefinition +from samcli.lib.utils.architecture import ARM64, X86_64 +from samcli.lib.utils.resources import AWS_BEDROCK_AGENTCORE_RUNTIME, AWS_ECS_TASK_DEFINITION + + +class TestContainerBuildDefinition(TestCase): + def test_basic_properties(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app", "DockerBuildArgs": {"ENV": "prod"}} + cbd = ContainerBuildDefinition( + resource_identifier="MyTask", + resource_type=AWS_ECS_TASK_DEFINITION, + metadata=metadata, + ) + self.assertEqual(cbd.resource_identifier, "MyTask") + self.assertEqual(cbd.resource_type, AWS_ECS_TASK_DEFINITION) + self.assertEqual(cbd.dockerfile, "Dockerfile") + self.assertEqual(cbd.docker_context, "./app") + self.assertEqual(cbd.docker_build_args, {"ENV": "prod"}) + self.assertIsNone(cbd.docker_build_target) + self.assertEqual(cbd.architecture, X86_64) + + def test_architecture_from_metadata(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./agent", "Architecture": "arm64"} + cbd = ContainerBuildDefinition( + resource_identifier="MyAgent", + resource_type=AWS_BEDROCK_AGENTCORE_RUNTIME, + metadata=metadata, + ) + self.assertEqual(cbd.architecture, ARM64) + + def test_architecture_default_when_not_in_metadata(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app"} + cbd = ContainerBuildDefinition( + resource_identifier="MyTask", + resource_type=AWS_ECS_TASK_DEFINITION, + metadata=metadata, + ) + self.assertEqual(cbd.architecture, X86_64) + + def test_docker_build_target(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app", "DockerBuildTarget": "release"} + cbd = ContainerBuildDefinition( + resource_identifier="MyTask", + resource_type=AWS_ECS_TASK_DEFINITION, + metadata=metadata, + ) + self.assertEqual(cbd.docker_build_target, "release") + + def test_equality_same(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app"} + cbd1 = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, metadata) + cbd2 = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, metadata) + self.assertEqual(cbd1, cbd2) + + def test_equality_different_resource_id(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app"} + cbd1 = ContainerBuildDefinition("MyTask1", AWS_ECS_TASK_DEFINITION, metadata) + cbd2 = ContainerBuildDefinition("MyTask2", AWS_ECS_TASK_DEFINITION, metadata) + self.assertNotEqual(cbd1, cbd2) + + def test_equality_different_type(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app"} + cbd1 = ContainerBuildDefinition("MyRes", AWS_ECS_TASK_DEFINITION, metadata) + cbd2 = ContainerBuildDefinition("MyRes", AWS_BEDROCK_AGENTCORE_RUNTIME, metadata) + self.assertNotEqual(cbd1, cbd2) + + def test_equality_different_metadata(self): + cbd1 = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, {"Dockerfile": "A", "DockerContext": "."}) + cbd2 = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, {"Dockerfile": "B", "DockerContext": "."}) + self.assertNotEqual(cbd1, cbd2) + + def test_equality_not_container_build_definition(self): + metadata = {"Dockerfile": "Dockerfile", "DockerContext": "./app"} + cbd = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, metadata) + self.assertNotEqual(cbd, "not a build def") + + def test_get_resource_full_paths(self): + cbd = ContainerBuildDefinition( + "Stack/MyTask", AWS_ECS_TASK_DEFINITION, {"Dockerfile": "D", "DockerContext": "."} + ) + self.assertEqual(cbd.get_resource_full_paths(), "Stack/MyTask") + + def test_str_representation(self): + cbd = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, {"Dockerfile": "D", "DockerContext": "."}) + result = str(cbd) + self.assertIn("ContainerBuildDefinition", result) + self.assertIn("MyTask", result) + self.assertIn(AWS_ECS_TASK_DEFINITION, result) + + def test_none_metadata(self): + cbd = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, None) + self.assertIsNone(cbd.dockerfile) + self.assertIsNone(cbd.docker_context) + self.assertEqual(cbd.docker_build_args, {}) + + def test_strips_sam_metadata_keys(self): + metadata = {"Dockerfile": "D", "DockerContext": ".", "SamResourceId": "should_be_removed"} + cbd = ContainerBuildDefinition("MyTask", AWS_ECS_TASK_DEFINITION, metadata) + self.assertNotIn("SamResourceId", cbd.metadata) diff --git a/tests/unit/lib/build_module/test_container_build_integration.py b/tests/unit/lib/build_module/test_container_build_integration.py new file mode 100644 index 00000000000..fcc800c3a89 --- /dev/null +++ b/tests/unit/lib/build_module/test_container_build_integration.py @@ -0,0 +1,259 @@ +"""Tests for ECS/AgentCore container build integration across modules""" + +from unittest import TestCase +from unittest.mock import MagicMock, patch, Mock +from copy import deepcopy + +from samcli.lib.build.app_builder import ApplicationBuilder +from samcli.lib.build.build_graph import ContainerBuildDefinition +from samcli.lib.package.packageable_resources import ( + AgentCoreRuntimeImageResource, + ECSTaskDefinitionImageResource, +) +from samcli.lib.sync.sync_flow_factory import SyncFlowFactory +from samcli.lib.utils.packagetype import IMAGE, ZIP +from samcli.lib.utils.resources import ( + AWS_BEDROCK_AGENTCORE_RUNTIME, + AWS_ECS_TASK_DEFINITION, + RESOURCES_WITH_IMAGE_COMPONENT, +) + + +class TestResourceConstants(TestCase): + def test_ecs_task_definition_in_image_components(self): + self.assertIn(AWS_ECS_TASK_DEFINITION, RESOURCES_WITH_IMAGE_COMPONENT) + self.assertEqual(RESOURCES_WITH_IMAGE_COMPONENT[AWS_ECS_TASK_DEFINITION], ["ContainerDefinitions.Image"]) + + def test_agentcore_in_image_components(self): + self.assertIn(AWS_BEDROCK_AGENTCORE_RUNTIME, RESOURCES_WITH_IMAGE_COMPONENT) + self.assertEqual( + RESOURCES_WITH_IMAGE_COMPONENT[AWS_BEDROCK_AGENTCORE_RUNTIME], + ["AgentRuntimeArtifact.ContainerConfiguration.ContainerUri"], + ) + + def test_resource_type_values(self): + self.assertEqual(AWS_ECS_TASK_DEFINITION, "AWS::ECS::TaskDefinition") + self.assertEqual(AWS_BEDROCK_AGENTCORE_RUNTIME, "AWS::BedrockAgentCore::Runtime") + + +class TestECSTaskDefinitionImageResource(TestCase): + def test_resource_type(self): + self.assertEqual(ECSTaskDefinitionImageResource.RESOURCE_TYPE, AWS_ECS_TASK_DEFINITION) + + def test_property_name(self): + self.assertEqual(ECSTaskDefinitionImageResource.PROPERTY_NAME, "ContainerDefinitions.Image") + + def test_artifact_type_is_zip(self): + # ZIP so it passes the PackageType filter (ECS has no PackageType property) + self.assertEqual(ECSTaskDefinitionImageResource.ARTIFACT_TYPE, ZIP) + + +class TestAgentCoreRuntimeImageResource(TestCase): + def test_resource_type(self): + self.assertEqual(AgentCoreRuntimeImageResource.RESOURCE_TYPE, AWS_BEDROCK_AGENTCORE_RUNTIME) + + def test_property_name(self): + self.assertEqual( + AgentCoreRuntimeImageResource.PROPERTY_NAME, + "AgentRuntimeArtifact.ContainerConfiguration.ContainerUri", + ) + + def test_artifact_type_is_zip(self): + self.assertEqual(AgentCoreRuntimeImageResource.ARTIFACT_TYPE, ZIP) + + +class TestUpdateBuiltResource(TestCase): + def test_ecs_task_definition_updates_first_container(self): + properties = {"ContainerDefinitions": [{"Name": "web", "Image": "placeholder"}]} + ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path") + self.assertEqual(properties["ContainerDefinitions"][0]["Image"], "myimage:latest") + + def test_ecs_task_definition_empty_container_defs(self): + properties = {"ContainerDefinitions": []} + # Should not raise + ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path") + + def test_ecs_task_definition_targets_container_by_name(self): + properties = { + "ContainerDefinitions": [ + {"Name": "sidecar", "Image": "sidecar:latest"}, + {"Name": "web", "Image": "placeholder"}, + ] + } + metadata = {"ContainerName": "web"} + ApplicationBuilder._update_built_resource( + "myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path", metadata + ) + self.assertEqual(properties["ContainerDefinitions"][0]["Image"], "sidecar:latest") # unchanged + self.assertEqual(properties["ContainerDefinitions"][1]["Image"], "myimage:latest") # updated + + def test_ecs_task_definition_falls_back_to_first_without_container_name(self): + properties = { + "ContainerDefinitions": [ + {"Name": "web", "Image": "placeholder"}, + {"Name": "sidecar", "Image": "sidecar:latest"}, + ] + } + ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path") + self.assertEqual(properties["ContainerDefinitions"][0]["Image"], "myimage:latest") + self.assertEqual(properties["ContainerDefinitions"][1]["Image"], "sidecar:latest") + + def test_agentcore_updates_nested_container_uri(self): + properties = {"AgentRuntimeArtifact": {"ContainerConfiguration": {"ContainerUri": "placeholder"}}} + ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_BEDROCK_AGENTCORE_RUNTIME, "/path") + self.assertEqual(properties["AgentRuntimeArtifact"]["ContainerConfiguration"]["ContainerUri"], "myimage:latest") + + def test_agentcore_creates_nested_structure_if_missing(self): + properties = {} + ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_BEDROCK_AGENTCORE_RUNTIME, "/path") + self.assertEqual(properties["AgentRuntimeArtifact"]["ContainerConfiguration"]["ContainerUri"], "myimage:latest") + + +class TestBuildContainerImages(TestCase): + @patch.object(ApplicationBuilder, "_build_lambda_image", return_value="myimage:latest") + def test_builds_all_definitions(self, mock_build): + builder = ApplicationBuilder.__new__(ApplicationBuilder) + builder._base_dir = "/base" + builder._stream_writer = MagicMock() + builder._use_buildkit = False + builder._image_build_client = None + + defs = [ + ContainerBuildDefinition("Task1", AWS_ECS_TASK_DEFINITION, {"Dockerfile": "D", "DockerContext": "."}), + ContainerBuildDefinition( + "Agent1", AWS_BEDROCK_AGENTCORE_RUNTIME, {"Dockerfile": "D", "DockerContext": "."} + ), + ] + result = builder.build_container_images(defs) + self.assertEqual(len(result), 2) + self.assertIn("Task1", result) + self.assertIn("Agent1", result) + self.assertEqual(mock_build.call_count, 2) + + @patch.object(ApplicationBuilder, "_build_lambda_image", return_value="img:tag") + def test_skips_definition_without_metadata(self, mock_build): + builder = ApplicationBuilder.__new__(ApplicationBuilder) + builder._base_dir = "/base" + builder._stream_writer = MagicMock() + builder._use_buildkit = False + builder._image_build_client = None + + defs = [ContainerBuildDefinition("Task1", AWS_ECS_TASK_DEFINITION, None)] + result = builder.build_container_images(defs) + self.assertEqual(len(result), 0) + mock_build.assert_not_called() + + +class TestSyncFlowFactoryMapping(TestCase): + def test_ecs_task_definition_registered(self): + self.assertIn(AWS_ECS_TASK_DEFINITION, SyncFlowFactory.GENERATOR_MAPPING) + + def test_agentcore_registered(self): + self.assertIn(AWS_BEDROCK_AGENTCORE_RUNTIME, SyncFlowFactory.GENERATOR_MAPPING) + + def test_ecs_and_agentcore_use_same_flow_creator(self): + self.assertEqual( + SyncFlowFactory.GENERATOR_MAPPING[AWS_ECS_TASK_DEFINITION], + SyncFlowFactory.GENERATOR_MAPPING[AWS_BEDROCK_AGENTCORE_RUNTIME], + ) + + +class TestSyncEcrStackIncludesContainerResources(TestCase): + @patch("samcli.lib.providers.sam_container_provider.SamContainerServiceProvider") + @patch("samcli.lib.bootstrap.companion_stack.companion_stack_manager.SamFunctionProvider") + @patch("samcli.lib.bootstrap.companion_stack.companion_stack_manager.SamLocalStackProvider") + @patch("samcli.lib.bootstrap.companion_stack.companion_stack_manager.CompanionStackManager") + def test_includes_container_services( + self, mock_manager_cls, mock_stack_provider, mock_func_provider, mock_container_provider + ): + from samcli.lib.bootstrap.companion_stack.companion_stack_manager import sync_ecr_stack + + # Setup mocks + mock_stack_provider.get_stacks.return_value = ([MagicMock()], None) + + mock_func = MagicMock() + mock_func.packagetype = IMAGE + mock_func.full_path = "MyFunction" + mock_func_provider.return_value.get_all.return_value = [mock_func] + + mock_service = MagicMock() + mock_service.full_path = "MyAgent" + mock_container_provider.return_value.get_all.return_value = [mock_service] + + mock_manager = MagicMock() + mock_manager.get_repository_mapping.return_value = {"MyFunction": "uri1", "MyAgent": "uri2"} + mock_manager_cls.return_value = mock_manager + + result = sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {}) + + # Verify both function and container service were passed + call_args = mock_manager.set_functions.call_args[0] + logical_ids = call_args[0] + self.assertIn("MyFunction", logical_ids) + self.assertIn("MyAgent", logical_ids) + + +class TestECSContainerSyncFlow(TestCase): + def test_sync_state_identifier(self): + from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow + + flow = ECSContainerSyncFlow.__new__(ECSContainerSyncFlow) + flow._resource_identifier = "MyTask" + self.assertEqual(flow.sync_state_identifier, "MyTask") + + def test_equality_keys(self): + from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow + + flow = ECSContainerSyncFlow.__new__(ECSContainerSyncFlow) + flow._resource_identifier = "MyTask" + self.assertEqual(flow._equality_keys(), "MyTask") + + def test_gather_dependencies_empty(self): + from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow + + flow = ECSContainerSyncFlow.__new__(ECSContainerSyncFlow) + self.assertEqual(flow.gather_dependencies(), []) + + def test_compare_remote_always_false(self): + from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow + + flow = ECSContainerSyncFlow.__new__(ECSContainerSyncFlow) + self.assertFalse(flow.compare_remote()) + + @patch("samcli.lib.sync.flows.ecs_container_sync_flow.ECRUploader") + @patch("samcli.lib.sync.flows.ecs_container_sync_flow.get_validated_container_client") + def test_sync_pushes_image(self, mock_docker, mock_ecr_uploader_cls): + from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow + + flow = ECSContainerSyncFlow.__new__(ECSContainerSyncFlow) + flow._resource_identifier = "MyAgent" + flow._log_name = "ECSContainer MyAgent" + flow._image_name = "myimage:latest" + flow._docker_client = None + flow._ecr_client = None + flow._ecs_client = None + flow._physical_id_mapping = {} + flow._deploy_context = MagicMock() + flow._deploy_context.image_repository = "123.dkr.ecr.us-east-1.amazonaws.com/repo" + flow._deploy_context.image_repositories = None + + mock_docker.return_value = MagicMock() + + flow._get_session = MagicMock() + flow._boto_client = MagicMock() + + flow.sync() + + mock_ecr_uploader_cls.assert_called_once() + mock_ecr_uploader_cls.return_value.upload.assert_called_once_with("myimage:latest", "MyAgent") + + def test_sync_skips_when_no_image(self): + from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow + + flow = ECSContainerSyncFlow.__new__(ECSContainerSyncFlow) + flow._resource_identifier = "MyAgent" + flow._log_name = "ECSContainer MyAgent" + flow._image_name = None + flow._physical_id_mapping = {} + # Should not raise + flow.sync() diff --git a/tests/unit/lib/providers/test_sam_container_provider.py b/tests/unit/lib/providers/test_sam_container_provider.py new file mode 100644 index 00000000000..a9f64a2857d --- /dev/null +++ b/tests/unit/lib/providers/test_sam_container_provider.py @@ -0,0 +1,141 @@ +"""Tests for SamContainerServiceProvider""" + +from unittest import TestCase +from unittest.mock import MagicMock + +from samcli.lib.providers.sam_container_provider import SamContainerServiceProvider, CONTAINER_IMAGE_RESOURCE_TYPES +from samcli.lib.utils.resources import AWS_BEDROCK_AGENTCORE_RUNTIME, AWS_ECS_TASK_DEFINITION + + +def _make_stack(resources, stack_path=""): + stack = MagicMock() + stack.resources = resources + stack.stack_path = stack_path + return stack + + +class TestSamContainerServiceProvider(TestCase): + def test_discovers_ecs_task_definition(self): + resources = { + "MyTask": { + "Type": AWS_ECS_TASK_DEFINITION, + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./app"}, + "Properties": {"ContainerDefinitions": [{"Name": "web", "Image": "placeholder"}]}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 1) + self.assertEqual(services[0].resource_id, "MyTask") + self.assertEqual(services[0].resource_type, AWS_ECS_TASK_DEFINITION) + + def test_discovers_agentcore_runtime(self): + resources = { + "MyAgent": { + "Type": AWS_BEDROCK_AGENTCORE_RUNTIME, + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./agent"}, + "Properties": {"AgentRuntimeArtifact": {"ContainerConfiguration": {"ContainerUri": "placeholder"}}}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 1) + self.assertEqual(services[0].resource_id, "MyAgent") + self.assertEqual(services[0].resource_type, AWS_BEDROCK_AGENTCORE_RUNTIME) + + def test_skips_resource_without_metadata(self): + resources = { + "MyTask": { + "Type": AWS_ECS_TASK_DEFINITION, + "Properties": {"ContainerDefinitions": [{"Name": "web", "Image": "some-image"}]}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 0) + + def test_skips_resource_without_dockerfile(self): + resources = { + "MyTask": { + "Type": AWS_ECS_TASK_DEFINITION, + "Metadata": {"DockerContext": "./app"}, # Missing Dockerfile + "Properties": {}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 0) + + def test_skips_resource_without_docker_context(self): + resources = { + "MyTask": { + "Type": AWS_ECS_TASK_DEFINITION, + "Metadata": {"Dockerfile": "Dockerfile"}, # Missing DockerContext + "Properties": {}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 0) + + def test_skips_unsupported_resource_type(self): + resources = { + "MyFunction": { + "Type": "AWS::Lambda::Function", + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./app"}, + "Properties": {}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 0) + + def test_get_by_full_path(self): + resources = { + "MyAgent": { + "Type": AWS_BEDROCK_AGENTCORE_RUNTIME, + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./agent"}, + "Properties": {}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + result = provider.get("MyAgent") + self.assertIsNotNone(result) + self.assertEqual(result.resource_id, "MyAgent") + + def test_get_returns_none_for_unknown(self): + provider = SamContainerServiceProvider([_make_stack({})]) + self.assertIsNone(provider.get("NonExistent")) + + def test_nested_stack_full_path(self): + resources = { + "MyTask": { + "Type": AWS_ECS_TASK_DEFINITION, + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./app"}, + "Properties": {}, + } + } + provider = SamContainerServiceProvider([_make_stack(resources, stack_path="ChildStack")]) + services = list(provider.get_all()) + self.assertEqual(services[0].full_path, "ChildStack/MyTask") + + def test_multiple_resources(self): + resources = { + "Task1": { + "Type": AWS_ECS_TASK_DEFINITION, + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./app1"}, + "Properties": {}, + }, + "Agent1": { + "Type": AWS_BEDROCK_AGENTCORE_RUNTIME, + "Metadata": {"Dockerfile": "Dockerfile", "DockerContext": "./agent"}, + "Properties": {}, + }, + } + provider = SamContainerServiceProvider([_make_stack(resources)]) + services = list(provider.get_all()) + self.assertEqual(len(services), 2) + + def test_container_image_resource_types_list(self): + self.assertIn(AWS_ECS_TASK_DEFINITION, CONTAINER_IMAGE_RESOURCE_TYPES) + self.assertIn(AWS_BEDROCK_AGENTCORE_RUNTIME, CONTAINER_IMAGE_RESOURCE_TYPES) From 56c763ea37b11b634d0fd623f1277a6960ac00f2 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Mon, 18 May 2026 11:06:18 +0300 Subject: [PATCH 02/24] fix: resolve ruff PLR2004 lint error and Windows integration test failure - Extract magic number 3 to _ECS_SERVICE_ARN_PARTS constant - Switch test Dockerfile to public.ecr.aws alpine (multi-arch, no rate limits) --- samcli/lib/sync/flows/ecs_container_sync_flow.py | 6 +++++- .../testdata/buildcmd/container_image/agent/Dockerfile | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py index 4d4149525be..34fedaa225a 100644 --- a/samcli/lib/sync/flows/ecs_container_sync_flow.py +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -21,6 +21,10 @@ LOG = logging.getLogger(__name__) +# Minimum number of parts expected when splitting an ECS service ARN by "/" +# e.g. arn:aws:ecs:region:account:service/cluster/name -> [..., "cluster", "name"] +_ECS_SERVICE_ARN_PARTS = 3 + class ECSContainerSyncFlow(SyncFlow): """SyncFlow for ECS TaskDefinition and AgentCore container image resources. @@ -174,7 +178,7 @@ def _force_ecs_deployment(self) -> None: try: # Extract cluster and service from the ARN parts = resource_physical_id.rsplit("/", 2) - if len(parts) < 3: + if len(parts) < _ECS_SERVICE_ARN_PARTS: continue cluster = parts[-2] service_name = parts[-1] diff --git a/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile b/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile index 57f45c64404..70947c4a8ab 100644 --- a/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile +++ b/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile @@ -1,4 +1,4 @@ -FROM python:3.12-slim +FROM public.ecr.aws/docker/library/alpine:3.19 WORKDIR /app RUN echo "hello" > /app/test.txt -CMD ["python", "-c", "print('test')"] +CMD ["echo", "test"] From ec83c234a9ca66dcab37fb37b912314e5718d185 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Mon, 18 May 2026 12:02:36 +0300 Subject: [PATCH 03/24] fix: skip container image integration test on Windows CI Windows CI runners use Windows containers and cannot pull Linux images. This matches the pattern used by existing Lambda image build tests. --- .../buildcmd/test_build_cmd_container_image.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/integration/buildcmd/test_build_cmd_container_image.py b/tests/integration/buildcmd/test_build_cmd_container_image.py index c34bd7cab90..56a2136f157 100644 --- a/tests/integration/buildcmd/test_build_cmd_container_image.py +++ b/tests/integration/buildcmd/test_build_cmd_container_image.py @@ -1,16 +1,20 @@ """Integration test for ECS/AgentCore container image builds""" -import os import shutil import tempfile from pathlib import Path -from unittest import TestCase +from unittest import TestCase, skipIf import yaml from samcli.commands.build.build_context import BuildContext +from tests.testing_utils import CI_OVERRIDE, IS_WINDOWS, RUNNING_ON_CI +@skipIf( + (IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE, + "Skip container image build tests on Windows CI (no Linux Docker support)", +) class TestContainerImageBuild(TestCase): """Test that samdev build correctly handles ECS and AgentCore container resources.""" From 6ca8f92b81ff93e1791f6447a7739c0dc64a7d36 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Tue, 19 May 2026 02:46:29 +0300 Subject: [PATCH 04/24] fix: address PR reviewer findings - remove substring false-positive and add error case tests - Remove substring check in _force_ecs_deployment that could match wrong services (e.g. my-app:5 matching my-app:50). Use family-name comparison only. - Add unit tests for ContainerName mismatch error handling in both app_builder and packageable_resources. - Clean up unused imports in test file. --- .../lib/sync/flows/ecs_container_sync_flow.py | 8 ++-- .../test_container_build_integration.py | 43 +++++++++++++++++-- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py index 34fedaa225a..f62aeb138fb 100644 --- a/samcli/lib/sync/flows/ecs_container_sync_flow.py +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -184,13 +184,11 @@ def _force_ecs_deployment(self) -> None: service_name = parts[-1] svc_response = ecs_client.describe_services(cluster=cluster, services=[service_name]) + my_family = physical_id.rsplit("/", 1)[-1].split(":", 1)[0] for svc in svc_response.get("services", []): svc_task_def = svc.get("taskDefinition", "") - # Check if this service references our task definition family - if physical_id in svc_task_def or ( - svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0] - == physical_id.rsplit("/", 1)[-1].split(":", 1)[0] - ): + svc_family = svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0] + if svc_family and svc_family == my_family: ecs_client.update_service( cluster=cluster, service=service_name, diff --git a/tests/unit/lib/build_module/test_container_build_integration.py b/tests/unit/lib/build_module/test_container_build_integration.py index fcc800c3a89..b86c3857106 100644 --- a/tests/unit/lib/build_module/test_container_build_integration.py +++ b/tests/unit/lib/build_module/test_container_build_integration.py @@ -1,8 +1,7 @@ """Tests for ECS/AgentCore container build integration across modules""" from unittest import TestCase -from unittest.mock import MagicMock, patch, Mock -from copy import deepcopy +from unittest.mock import MagicMock, patch from samcli.lib.build.app_builder import ApplicationBuilder from samcli.lib.build.build_graph import ContainerBuildDefinition @@ -184,7 +183,7 @@ def test_includes_container_services( mock_manager.get_repository_mapping.return_value = {"MyFunction": "uri1", "MyAgent": "uri2"} mock_manager_cls.return_value = mock_manager - result = sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {}) + sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {}) # Verify both function and container service were passed call_args = mock_manager.set_functions.call_args[0] @@ -257,3 +256,41 @@ def test_sync_skips_when_no_image(self): flow._physical_id_mapping = {} # Should not raise flow.sync() + + +class TestContainerNameErrorHandling(TestCase): + def test_update_built_resource_raises_on_container_name_mismatch(self): + from samcli.lib.build.exceptions import DockerBuildFailed + + properties = { + "ContainerDefinitions": [ + {"Name": "sidecar", "Image": "sidecar:latest"}, + {"Name": "web", "Image": "placeholder"}, + ] + } + metadata = {"ContainerName": "typo"} + with self.assertRaises(DockerBuildFailed): + ApplicationBuilder._update_built_resource( + "myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path", metadata + ) + + def test_get_target_index_raises_on_container_name_mismatch(self): + from samcli.commands.package import exceptions + + exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource) + exporter.resource_metadata = {"ContainerName": "typo"} + container_defs = [{"Name": "web"}, {"Name": "sidecar"}] + with self.assertRaises(exceptions.ExportFailedError): + exporter._get_target_index(container_defs) + + def test_get_target_index_returns_match(self): + exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource) + exporter.resource_metadata = {"ContainerName": "web"} + container_defs = [{"Name": "sidecar"}, {"Name": "web"}] + self.assertEqual(exporter._get_target_index(container_defs), 1) + + def test_get_target_index_defaults_to_zero_without_name(self): + exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource) + exporter.resource_metadata = {} + container_defs = [{"Name": "web"}] + self.assertEqual(exporter._get_target_index(container_defs), 0) From 977701d32f7674af1d0983a6740f1fa88d50f1a6 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Tue, 19 May 2026 15:48:03 +0300 Subject: [PATCH 05/24] ci: retrigger CI (pyinstaller-linux infra flake) From 8d1a612e9898edde3f0b28448a2aa4111c7dddd2 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Tue, 19 May 2026 16:55:33 +0300 Subject: [PATCH 06/24] ci: retrigger CI (go setup infra flake) From abfdb31fddc4fedf3ad8add134cb79b2e4597a8d Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Thu, 28 May 2026 09:13:31 -0600 Subject: [PATCH 07/24] fix: use empty ResourcesToBuildCollector in ECS sync _build_from_scratch collect_build_resources() only handles functions/layers and raises ResourceNotFound for ECS/AgentCore resource IDs. Since build_container_images() doesn't depend on resources_to_build, pass an empty collector instead. --- samcli/lib/sync/flows/ecs_container_sync_flow.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py index f62aeb138fb..74663dacc9c 100644 --- a/samcli/lib/sync/flows/ecs_container_sync_flow.py +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -9,7 +9,7 @@ from samcli.lib.build.app_builder import ApplicationBuilder, ApplicationBuildResult from samcli.lib.build.build_graph import ContainerBuildDefinition from samcli.lib.package.ecr_uploader import ECRUploader -from samcli.lib.providers.provider import Stack +from samcli.lib.providers.provider import ResourcesToBuildCollector, Stack from samcli.lib.providers.sam_container_provider import SamContainerServiceProvider from samcli.lib.sync.sync_flow import ApiCallTypes, ResourceAPICall, SyncFlow from samcli.local.docker.utils import get_validated_container_client @@ -109,11 +109,7 @@ def _build_from_scratch(self) -> None: ) builder = ApplicationBuilder( - ( - self._build_context.collect_build_resources(self._resource_identifier) - if hasattr(self._build_context, "collect_build_resources") - else self._build_context.get_resources_to_build() - ), + ResourcesToBuildCollector(), self._build_context.build_dir, self._build_context.base_dir, self._build_context.cache_dir, From d992b502c170f5a9b54b858ae97f94b6c0f227d7 Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Thu, 28 May 2026 10:11:01 -0700 Subject: [PATCH 08/24] fix(package): rewrite AWS::Include Location buried inside language-extension functions (#9030) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(package): replace METADATA_EXPORT_LIST and GLOBAL_EXPORT_DICT with typed registries Migrate two flat package exporter registries to typed dataclass form: - METADATA_EXPORT_LIST (list of Resource subclasses) -> METADATA_EXPORTS (List[MetadataExportSpec]). Each spec captures both the metadata_type ('AWS::ServerlessRepo::Application') and the per-property exporter classes that handle LicenseUrl/ReadmeUrl. Template._export_metadata now dispatches via a metadata_type lookup instead of a per-class RESOURCE_TYPE filter. - GLOBAL_EXPORT_DICT (dict keyed by Fn::Transform) -> GLOBAL_TRANSFORM_EXPORTS (List[GlobalTransformExportSpec]). Each spec carries a discriminator callable (e.g. _is_aws_include for matching AWS::Include) plus the handler. _export_global_artifacts now dispatches through the discriminator so future Fn::Transform variants can register without touching the walker. The typed shape is the contract a follow-up commit will read from to process AWS::Include before language-extension expansion. * feat(package): merge SAR Metadata exports back into LE child templates Extend merge_language_extensions_s3_uris with a registry-driven Metadata pass that copies rewritten property values (LicenseUrl, ReadmeUrl) from the exported template back into the original (Fn::ForEach-preserving) template after LE expansion. Without this pass, when a child stack uses Transform: AWS::LanguageExtensions and declares AWS::ServerlessRepo::Application metadata, sam package silently dropped the License/Readme S3 URLs from the merged output — they were uploaded but never wired into the template the user deploys. Implementation iterates METADATA_EXPORTS (the registry added in the prior commit) so future metadata types pick up merge support without touching the merge walker. * fix(package): process AWS::Include before language-extension expansion (#9027) Closes https://github.com/aws/aws-sam-cli/issues/9027. Symptom: when a template uses both Transform: AWS::LanguageExtensions and contains Fn::Transform: AWS::Include buried inside an LE function (e.g. Fn::ToJsonString), sam package fails to rewrite the include's Location to an S3 URL. CloudFormation then rejects the deploy with 'The location parameter is not a valid S3 uri.' Root cause: PackageContext._export ran expand_language_extensions BEFORE the artifact exporter walked Fn::Transform nodes. LE functions like Fn::ToJsonString json.dumps() their argument, collapsing the structural Fn::Transform subtree into a JSON-string literal. By the time the exporter ran, the include was no longer a structural dict node and was invisible to the global-transform walker. Fix: process AWS::Include (and any other GLOBAL_TRANSFORM_EXPORTS handler) on the original template BEFORE LE expansion runs, mirroring CloudFormation's own server-side transform ordering — CFN resolves inline Fn::Transform macros before evaluating AWS::LanguageExtensions. Implementation: - Extract Template._export_global_artifacts to a module-level _export_global_artifacts_pass(template_dict, uploader, template_dir). The instance method becomes a one-line delegate so existing callers keep working. - Call _export_global_artifacts_pass on the original template before expand_language_extensions in PackageContext._export (root flow) and in CloudFormationStackResource.do_export (nested-stack child flow). Dynamic-Location AWS::Include inside Fn::ForEach (e.g. Location: ./swagger-${Name}.yaml) is not supported by sam package: a local file path with literal ${...} placeholders does not exist on disk, so is_local_file fails and the existing InvalidLocalPathError fires — which is the right user-facing failure. CloudFormation does not substitute loop variables into Fn::Transform paths server-side either, so the limitation matches CFN's actual capability. * test(package): integration coverage for AWS::Include + SAR Metadata in LE templates Three end-to-end tests that exercise the package_context._export-equivalent flow on language-extension templates: - test_le_template_with_top_level_aws_include_merges_location verifies AWS::Include in Outputs gets its Location rewritten to s3:// after the pre-LE pass. - test_le_template_with_serverless_repo_metadata_merges_license_url verifies SAR LicenseUrl/ReadmeUrl in Metadata get merged back. - TestPackageContextIssue9027 reproduces the user template from aws/aws-sam-cli#9027 (Fn::ToJsonString over Fn::Transform: AWS::Include buried inside AWS::SSM::Parameter) and asserts the buried Location is rewritten to s3://. Locks down the regression. * docs(cfn-lang-ext): document AWS::Include processing order vs language extensions Add a section explaining that sam package processes Fn::Transform: AWS::Include before language-extension expansion, mirroring CloudFormation's server-side transform ordering. This means AWS::Include Location rewrites work correctly even when the include lives buried inside language-extension functions like Fn::ToJsonString or Fn::ForEach bodies. * test(package): align LE tests with opt-in API from #9033 PR #9033 made AWS::LanguageExtensions local processing opt-in. Two API changes broke three tests on this branch after the merge from develop: - expand_language_extensions() now requires a keyword-only `enabled` arg - PackageContext reads self._language_extensions_enabled, set in __init__ Pass enabled=True to expand_language_extensions in the two artifact-exporter tests, and set _language_extensions_enabled on the PackageContext stub that bypasses __init__ via __new__. All three tests exercise templates with Transform: AWS::LanguageExtensions, so enabling LE is the intended behavior. * test(package): hoist inline imports in TestPackageContextBuriedAWSInclude Move os, Destination/Uploaders, and yaml_parse to module-level imports; drop the redundant inline tempfile/PackageContext (already at module level). Addresses inline-imports review comment on #9030. * refactor(package): hoist InvalidSamDocumentException and expand_language_extensions imports Move the two inline imports inside _export() to module scope. No behavior change; this is preparation for the upcoming _export() split which references these from a new branch method. * refactor(package): add _export_without_language_extensions (off-path branch) New private method that mirrors pre-1.160.0 sam-cli behavior: a tight Template.export() pipeline with no LE machinery. Unused by _export() until the dispatcher is cut over in a follow-up commit. Also adds two structural-gate smoke tests that stay red until the dispatcher is wired up — they assert that the off path never invokes expand_language_extensions or the pre-LE _export_global_artifacts_pass. * refactor(package): add _export_with_language_extensions (on-path branch) New private method containing the existing #9027 ordering: pre-LE include pass, LE expansion, Template.export(), merge, Mappings. Unused by _export() until the dispatcher is cut over in the next commit. * refactor(package): split _export() into LE / non-LE branches gated on flag _export() becomes a thin dispatcher: read the template, branch on self._language_extensions_enabled, dump. The two branch methods added in the prior two commits now own the actual work. Treats --language-extensions as a hard correctness boundary: when off, no LE machinery is invoked at all (no expand_language_extensions, no pre-LE _export_global_artifacts_pass, no merge, no Mappings). Template.export() still handles AWS::Include for non-LE templates via its own internal _export_global_artifacts pass — that path has worked since long before the #9027 fix and is unchanged by this refactor. Public surface (PackageContext._export(template_path, use_json) -> str) is preserved; all existing _export tests pass unchanged. * test(package): repoint expand_language_extensions patches to package_context use site The hoist in 756c5029e moved `expand_language_extensions` to a module-level import in `package_context.py`. The use-site binding is now captured at module load, so existing tests that patched the source module `samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions` no longer intercept calls from `_export()`. Repoint two patches at the use-site `samcli.commands.package.package_context.expand_language_extensions`: - `test_phase_separation.py::test_package_context_export_calls_expand_language_extensions` was failing intermittently in CI depending on test-collection order. - `test_package_context.py::test_off_path_does_not_invoke_expand_language_extensions` is retargeted for consistency now that the inline-import shadow is gone (post-Task-4 cutover); the dead `had_language_extensions=False` scaffolding can also go since the off path no longer calls expand. Other source-module patches in `test_artifact_exporter.py` are correct as-is — those tests cover `Template.export()` paths whose use site is the artifact_exporter module. * refactor(package): hoist do_export inline imports and repoint test patches Hoist expand_language_extensions, IntrinsicsSymbolTable, generate_and_apply_artifact_mappings, and merge_language_extensions_s3_uris to module-level imports in artifact_exporter.py — preparation for the LE / non-LE structural-gate split of CloudFormationStackResource.do_export. Module-level binding requires existing tests that patched samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions (the source module) to repoint at the use-site samcli.lib.package.artifact_exporter.expand_language_extensions, since the use-site name is captured at module load and source-module patches no longer intercept once the inline import is gone. Same lesson as PR #9030 commit 45dea4b7b for package_context. * refactor(package): add _do_export_without_language_extensions branch method LE-off branch for CloudFormationStackResource: path-based Template construction with no pre-LE pass, no parameter_values, no deepcopy. Method is unused until the dispatcher rewrite — wiring lands in the follow-up commit. Adds TestDoExportLanguageExtensionsStructuralGate with two red structural-gate assertions that the dispatcher rewrite will turn green. * refactor(package): add _do_export_with_language_extensions branch method Lifts the existing LE-expansion body from CloudFormationStackResource.do_export into a dedicated method (resource_id, template_path, parent_dir, abs_template_path, resource_dict) -> Dict. No behavior change yet — the dispatcher in the follow-up commit will route LE-on calls into it. Threads resource_dict so the branch can read nested-stack Parameters without the dispatcher passing them separately. Returns the exported template dict so the dispatcher owns the final yaml_dump + upload tail. * refactor(package): split do_export into LE / non-LE branches gated on flag CloudFormationStackResource.do_export is now a thin dispatcher that: 1) does the early-exit guards (None / S3 URL / non-local file) 2) routes to _do_export_with_language_extensions or _do_export_without_language_extensions based on self.language_extensions_enabled 3) owns the final yaml_dump + upload + property mutation tail. The off path is now structurally LE-free: no expand_language_extensions call, no _export_global_artifacts_pass, no IntrinsicsSymbolTable pseudo-param plumbing, no parent_parameter_values, no copy.deepcopy. Template.export() runs its own internal _export_global_artifacts so AWS::Include still resolves on the off path. The structural-gate tests added in the previous commit are now green. Existing LE tests that rely on language_extensions_enabled = True now set the flag explicitly (where they previously depended on the LE machinery running unconditionally as a passthrough). * chore(package): apply make format to do_export refactor Black collapsed multi-line calls and signatures whose single-line form fits under the 120-char limit. No behavior change. * refactor(package): add _build_child_parameter_values helper Adds a CFN-parity helper that builds the parameter_values dict for a nested child stack: pseudo-params (with parent pseudo-name overrides honored) plus parent-rebound values via the nested-stack Parameters property. Non-pseudo parent names are not copied; child Defaults are read by the LE expander itself via parsed_template.parameters. Helper is unused production code in this commit. Wired into CloudFormationStackResource._do_export_with_language_extensions in the next commit. * fix(package): tighten parent_parameter_values scoping in LE path CloudFormationStackResource._do_export_with_language_extensions used to bulk-copy the parent stack's full parameter_values into the child, so a parent's `Foo=parent_foo` could silently shadow an unrelated child Parameter named Foo. This diverged from CloudFormation's nested-stack contract (only parent-rebound names reach the child) and from the non-LE path (which threads no parameters at all). Replace the merge block with _build_child_parameter_values, which returns pseudo-params (with parent pseudo-name overrides honored) plus parent-rebound values from the nested-stack Parameters property. Child template Defaults still resolve via the LE expander's parsed_template fallback (resolvers/fn_ref.py:_resolve_parameter). Adds end-to-end coverage: - non-pseudo parent param does not leak into child's parameter_values - child Default still resolves via resolver fallback after the change * refactor(package): trim Task 2 redundant comments and verbose test docstrings Code-review polish for the LE parent-param scoping fix: - drop call-site comment block at _do_export_with_language_extensions that duplicated _build_child_parameter_values's docstring - shorten the two new test docstrings in TestCloudFormationStackResourceChildExpansion to match neighbor style (no internal module paths) * refactor(tests): hoist inline imports added in LE parent-param fix Move imports introduced by the previous two commits from inside test bodies up to the module-level import block: - _build_child_parameter_values, IntrinsicsSymbolTable - yaml_dump (samcli.yamlhelper), shutil, inspect (stdlib) None of these symbols are @patch targets, so hoisting does not affect any mock binding (use-site patches in artifact_exporter remain correct). * refactor(tests): hoist remaining inline imports and extract LE fixtures Address review feedback on PR #9030: inline imports should live at module scope, and on-the-fly file writes for LE child templates should use checked-in fixtures instead of tempfile.mkdtemp() + yaml_dump(). Hoist inline imports (LanguageExtensionResult, _resolve_nested_stack_parameters, yaml_dump, shutil, the packageable_resources block) to the module-level import block. None are @patch targets, so use-site patches remain correct. Add tests/unit/lib/package/test_data/ following the precedent in tests/unit/lib/intrinsic_resolver/test_data, with TEST_DATA_PATH constant defined as Path(__file__).resolve().parent / "test_data". Convert the LE parent-param test methods, the buried-AWS::Include test, the expansion-error-handling pair, and the structural-gate pair to read their child templates (and the export-events.json include target) from the fixture tree. The on-the-fly tempdir + yaml_dump scaffolding is removed entirely along with the corresponding _write_child / _write_minimal_child helpers. No production code change; all 110 unit tests in test_artifact_exporter still pass. * refactor(tests): extract TestPackageContextBuriedAWSInclude inline fixture Replace the on-the-fly tempfile + open()+write() scaffolding in TestPackageContextBuriedAWSInclude.setUp with a checked-in fixture under tests/unit/commands/package/test_data/buried_aws_include/. Mirrors the TEST_DATA_PATH pattern already in tests/unit/lib/package/test_data and tests/unit/lib/intrinsic_resolver/test_data. setUp now resolves template_path from TEST_DATA_PATH; the export-events include target lives next to template.yaml so the relative Location in the template still resolves at sam-package time. No production code change; all 146 tests in test_package_context still pass. --- docs/cfn-language-extensions.md | 18 +- samcli/commands/package/package_context.py | 135 ++-- samcli/lib/package/artifact_exporter.py | 277 +++++-- .../package/language_extensions_packaging.py | 21 + samcli/lib/package/packageable_resources.py | 54 +- .../buried_aws_include/export-events.json | 1 + .../buried_aws_include/template.yaml | 13 + .../commands/package/test_package_context.py | 127 ++++ .../test_phase_separation.py | 2 +- .../lib/package/test_artifact_exporter.py | 710 ++++++++++++++---- .../test_data/buried_aws_include/child.yaml | 14 + .../buried_aws_include/export-events.json | 1 + .../test_data/child_bar_default/child.yaml | 13 + .../test_data/child_foo_param/child.yaml | 6 + .../test_data/child_minimal_le/child.yaml | 6 + .../test_data/child_minimal_no_le/child.yaml | 1 + .../test_data/child_servicenames/child.yaml | 12 + .../test_data/le_sar_metadata/LICENSE.txt | 1 + .../test_data/le_sar_metadata/README.md | 1 + .../test_data/le_sar_metadata/code/index.py | 2 + .../le_top_level_include/code/index.py | 2 + .../test_data/le_top_level_include/extra.yaml | 1 + .../test_language_extensions_packaging.py | 104 +++ 23 files changed, 1262 insertions(+), 260 deletions(-) create mode 100644 tests/unit/commands/package/test_data/buried_aws_include/export-events.json create mode 100644 tests/unit/commands/package/test_data/buried_aws_include/template.yaml create mode 100644 tests/unit/lib/package/test_data/buried_aws_include/child.yaml create mode 100644 tests/unit/lib/package/test_data/buried_aws_include/export-events.json create mode 100644 tests/unit/lib/package/test_data/child_bar_default/child.yaml create mode 100644 tests/unit/lib/package/test_data/child_foo_param/child.yaml create mode 100644 tests/unit/lib/package/test_data/child_minimal_le/child.yaml create mode 100644 tests/unit/lib/package/test_data/child_minimal_no_le/child.yaml create mode 100644 tests/unit/lib/package/test_data/child_servicenames/child.yaml create mode 100644 tests/unit/lib/package/test_data/le_sar_metadata/LICENSE.txt create mode 100644 tests/unit/lib/package/test_data/le_sar_metadata/README.md create mode 100644 tests/unit/lib/package/test_data/le_sar_metadata/code/index.py create mode 100644 tests/unit/lib/package/test_data/le_top_level_include/code/index.py create mode 100644 tests/unit/lib/package/test_data/le_top_level_include/extra.yaml create mode 100644 tests/unit/lib/package/test_language_extensions_packaging.py diff --git a/docs/cfn-language-extensions.md b/docs/cfn-language-extensions.md index 9fa4f2035c1..7de428e0070 100644 --- a/docs/cfn-language-extensions.md +++ b/docs/cfn-language-extensions.md @@ -321,6 +321,22 @@ The following intrinsic functions are resolved locally during expansion: Functions that require deployed resources (`Fn::GetAtt`, `Fn::ImportValue`, `Fn::GetAZs`) are preserved for CloudFormation to resolve at deploy time. +## AWS::Include processing order + +When a template uses both `Fn::Transform: AWS::Include` and +`Transform: AWS::LanguageExtensions`, SAM CLI processes the inline +`AWS::Include` macros **before** running language-extension expansion +locally. This mirrors CloudFormation's server-side transform pipeline, +where `Fn::Transform` macros are resolved before +`AWS::LanguageExtensions`. + +The practical effect is that `AWS::Include` `Location` rewrites work +correctly even when the include lives buried inside language-extension +functions like `Fn::ToJsonString` or `Fn::ForEach` bodies, because the +include is rewritten while still structurally visible — before +`Fn::ToJsonString` collapses subtrees into JSON-string literals or +`Fn::ForEach` expands resources. + ## Validation errors The following template issues are caught locally before the SAM transform runs: @@ -330,7 +346,7 @@ The following template issues are caught locally before the SAM transform runs: | The `Fn::ForEach` value is malformed — not a list, doesn't have exactly 3 elements, or has a non-string loop identifier. | `Fn::ForEach:: layout is incorrect` (raised as `InvalidTemplateException`; see `samcli/lib/cfn_language_extensions/processors/foreach.py`). | | More than 5 levels of `Fn::ForEach` are nested. | `Fn::ForEach nesting depth of exceeds the maximum allowed depth of 5. CloudFormation supports up to 5 nested Fn::ForEach loops.` | | The collection resolves to an empty list (e.g., a `CommaDelimitedList` parameter with `Default: ""`). | No error — the loop is silently skipped and no resources are emitted. | -| The `!Ref` in the collection points at a parameter that is not declared in the template. | No error in the typical `sam build` / `sam package` flow. SAM CLI runs intrinsic resolution in PARTIAL mode and preserves the unresolved `{"Ref": ""}`. CloudFormation will reject the unresolved ref at deploy time. | +| The `!Ref` in the collection points at a parameter that is not declared in the template. | No error in the typical `sam build` / `sam package` flow. SAM CLI runs intrinsic resolution in PARTIAL mode and preserves the unresolved `{"Ref": ""}`. At deploy time, CloudFormation will resolve it server side. | ## Limitations diff --git a/samcli/commands/package/package_context.py b/samcli/commands/package/package_context.py index 17529f60bef..ac76e9b4279 100644 --- a/samcli/commands/package/package_context.py +++ b/samcli/commands/package/package_context.py @@ -24,10 +24,14 @@ import click from samcli.commands.package.exceptions import PackageFailedError +from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException from samcli.lib.bootstrap.companion_stack.companion_stack_manager import sync_ecr_stack -from samcli.lib.cfn_language_extensions.sam_integration import resolve_language_extensions_enabled +from samcli.lib.cfn_language_extensions.sam_integration import ( + expand_language_extensions, + resolve_language_extensions_enabled, +) from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable -from samcli.lib.package.artifact_exporter import Template +from samcli.lib.package.artifact_exporter import Template, _export_global_artifacts_pass from samcli.lib.package.code_signer import CodeSigner from samcli.lib.package.ecr_uploader import ECRUploader from samcli.lib.package.language_extensions_packaging import ( @@ -35,7 +39,7 @@ merge_language_extensions_s3_uris, ) from samcli.lib.package.s3_uploader import S3Uploader -from samcli.lib.package.uploaders import Uploaders +from samcli.lib.package.uploaders import Destination, Uploaders from samcli.lib.providers.provider import ResourceIdentifier, Stack, get_resource_full_path_by_id from samcli.lib.providers.sam_stack_provider import SamLocalStackProvider from samcli.lib.utils.boto_utils import get_boto_config_with_user_agent @@ -174,34 +178,80 @@ def run(self): raise PackageFailedError(template_file=self.template_file, ex=str(ex)) from ex def _export(self, template_path, use_json): - from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException - from samcli.lib.cfn_language_extensions.sam_integration import expand_language_extensions - - # Read the original template + # Read + parse once. Branch methods receive the parsed dict and + # return the output dict; this dispatcher owns the I/O bookends. with open(template_path, "r", encoding="utf-8") as f: original_template_dict = yaml_parse(f.read()) - # Build combined parameter values for expand_language_extensions - parameter_values = {} - parameter_values.update(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES) + # Structural fork on the opt-in flag (#9033). When LE is disabled + # we never invoke any LE machinery — no expand_language_extensions, + # no pre-LE _export_global_artifacts_pass, no merge or Mappings. + # See aws/aws-sam-cli#9027 for why the on path needs the pre-LE pass. + if self._language_extensions_enabled: + output_template = self._export_with_language_extensions(template_path, original_template_dict) + else: + output_template = self._export_without_language_extensions(template_path, original_template_dict) + + if use_json: + return json.dumps(output_template, indent=4, ensure_ascii=False) + return yaml_dump(output_template) + + def _export_without_language_extensions(self, template_path, original_template_dict): + """Export a template with AWS::LanguageExtensions opt-in disabled. + + Mirrors pre-1.160.0 sam-cli behavior: AWS::Include and other global + Fn::Transform exporters are handled inside Template.export() via its + own _export_global_artifacts pass — no pre-Template pass is needed + because there is no LE expansion step that would collapse Fn::Transform + into a JSON-string literal first (see aws/aws-sam-cli#9027 for why the + LE path differs). + """ + template = Template( + template_path, + os.getcwd(), + self.uploaders, + self.code_signer, + normalize_template=True, + normalize_parameters=True, + template_dict=original_template_dict, + language_extensions_enabled=False, + ) + return template.export() + + def _export_with_language_extensions(self, template_path, original_template_dict): + """Export a template with AWS::LanguageExtensions processing enabled. + + Order matters here: + 1. Run AWS::Include (and any other GLOBAL_TRANSFORM_EXPORTS) on the + original template *before* LE expansion. LE functions like + Fn::ToJsonString json.dumps() their argument and collapse any + structural Fn::Transform subtree into a JSON-string literal, + which would hide the include from the post-export walker. + See aws/aws-sam-cli#9027. + 2. Run expand_language_extensions to materialize Fn::ForEach, + Fn::ToJsonString, etc. into a flat resource list. + 3. Run Template.export() on the expanded copy. + 4. Merge the S3 URIs back into the original Fn::ForEach structure + and apply Mappings for any dynamic artifact properties. + """ + template_dir = os.path.dirname(os.path.abspath(template_path)) + _export_global_artifacts_pass( + original_template_dict, + self.uploaders.get(Destination.S3), + template_dir, + ) + + parameter_values = {**IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES} if self.parameter_overrides: parameter_values.update(self.parameter_overrides) if self._global_parameter_overrides: parameter_values.update(self._global_parameter_overrides) - # Use the canonical expand_language_extensions() entry point (Phase 1) try: - result = expand_language_extensions( - original_template_dict, parameter_values, enabled=self._language_extensions_enabled - ) + result = expand_language_extensions(original_template_dict, parameter_values, enabled=True) except InvalidSamDocumentException as e: raise PackageFailedError(template_file=self.template_file, ex=str(e)) from e - uses_language_extensions = result.had_language_extensions - dynamic_properties = result.dynamic_artifact_properties - - # Create Template with the (possibly expanded) template dict directly, - # avoiding a yaml_dump → yaml_parse round-trip. template = Template( template_path, os.getcwd(), @@ -211,38 +261,29 @@ def _export(self, template_path, use_json): normalize_parameters=True, template_dict=copy.deepcopy(result.expanded_template), parameter_values=parameter_values, - language_extensions_enabled=self._language_extensions_enabled, + language_extensions_enabled=True, ) - exported_template = template.export() - # If using language extensions, we need to preserve the original Fn::ForEach structure - # but update the artifact URIs (CodeUri, ContentUri, etc.) with the S3 locations - if uses_language_extensions: - LOG.debug("Template uses language extensions, preserving Fn::ForEach structure") - output_template = merge_language_extensions_s3_uris( - result.original_template, exported_template, dynamic_properties - ) - - # Generate Mappings for dynamic artifact properties - if dynamic_properties: - LOG.debug("Generating Mappings for %d dynamic artifact properties", len(dynamic_properties)) - - template_dir = os.path.dirname(os.path.abspath(template_path)) - exported_resources = exported_template.get("Resources", {}) - - output_template = generate_and_apply_artifact_mappings( - output_template, dynamic_properties, exported_resources, template_dir - ) - else: - output_template = exported_template - - if use_json: - exported_str = json.dumps(output_template, indent=4, ensure_ascii=False) - else: - exported_str = yaml_dump(output_template) + if not result.had_language_extensions: + return exported_template - return exported_str + LOG.debug("Template uses language extensions, preserving Fn::ForEach structure") + output_template = merge_language_extensions_s3_uris( + result.original_template, exported_template, result.dynamic_artifact_properties + ) + if result.dynamic_artifact_properties: + LOG.debug( + "Generating Mappings for %d dynamic artifact properties", + len(result.dynamic_artifact_properties), + ) + output_template = generate_and_apply_artifact_mappings( + output_template, + result.dynamic_artifact_properties, + exported_template.get("Resources", {}), + template_dir, + ) + return output_template @staticmethod def _warn_preview_runtime(stacks: List[Stack]) -> None: diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index d69fcaf8403..02276b25d70 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -17,21 +17,28 @@ import copy import logging import os -from typing import Dict, List, Optional +from typing import Any, Dict, List, Optional, Sequence, cast from botocore.utils import set_value_from_jmespath from samcli.commands._utils.experimental import ExperimentalFlag, is_experimental_enabled from samcli.commands.package import exceptions from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException +from samcli.lib.cfn_language_extensions.sam_integration import expand_language_extensions from samcli.lib.cfn_language_extensions.utils import iter_regular_resources +from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable from samcli.lib.package.code_signer import CodeSigner +from samcli.lib.package.language_extensions_packaging import ( + generate_and_apply_artifact_mappings, + merge_language_extensions_s3_uris, +) from samcli.lib.package.local_files_utils import get_uploaded_s3_object_name, mktempfile from samcli.lib.package.packageable_resources import ( - GLOBAL_EXPORT_DICT, - METADATA_EXPORT_LIST, + GLOBAL_TRANSFORM_EXPORTS, + METADATA_EXPORTS, RESOURCES_EXPORT_LIST, ECRResource, + MetadataExportSpec, ResourceZip, ) from samcli.lib.package.uploaders import Destination, Uploaders @@ -118,6 +125,84 @@ def _resolve_nested_stack_parameters(nested_params: Dict, parent_parameter_value return resolved +def _build_child_parameter_values( + parent_parameter_values: Optional[Dict], + nested_stack_parameters: Dict, +) -> Dict: + """Build the parameter_values dict the LE expander should see for a child stack. + + CFN-parity scope: pseudo-params (with parent overrides for the pseudo NAMES + only) and the parent's explicit rebindings via the nested-stack ``Parameters`` + property. Non-pseudo parent names are NOT copied — that would diverge from + CloudFormation's nested-stack contract. + + Child template ``Parameters.X.Default`` values are NOT folded in here. The + LE expander's Fn::Ref resolver reads them itself from + ``context.parsed_template.parameters[X]["Default"]``, which + ``TemplateParsingProcessor`` populates as the first step of the expansion + pipeline. So Defaults still take effect at expansion time; they just don't + pass through this helper's return value. + + Names declared in the child but neither defaulted nor rebound are + intentionally absent everywhere — PARTIAL mode preserves the Ref and + CloudFormation errors at deploy time, matching the non-LE path. + """ + parameter_values: Dict = dict(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES) + + if parent_parameter_values: + for pseudo_name in IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES: + if pseudo_name in parent_parameter_values: + parameter_values[pseudo_name] = parent_parameter_values[pseudo_name] + + resolved_nested = _resolve_nested_stack_parameters( + nested_stack_parameters, + dict(parent_parameter_values or {}), + ) + parameter_values.update(resolved_nested) + + return parameter_values + + +def _export_global_artifacts_pass(template_dict: Any, uploader, template_dir: str) -> Any: + """ + Walk template_dict recursively, dispatching dict nodes to handlers + registered in GLOBAL_TRANSFORM_EXPORTS (today: AWS::Include). + + Mutates template_dict in place AND returns it for caller convenience. + + This is the standalone form of Template._export_global_artifacts — + used by callers that need to run the global-transform export pass + on a template before constructing a Template (e.g., the pre-LE + pass that processes AWS::Include before language-extension + expansion runs). + + No-ops on non-dict input (e.g. yaml_parse returning None for empty + template files), so callers can pass the result of yaml_parse + unconditionally. + """ + if not isinstance(template_dict, dict): + return template_dict + + specs_by_key: Dict[str, list] = {} + for spec in GLOBAL_TRANSFORM_EXPORTS: + specs_by_key.setdefault(spec.template_key, []).append(spec) + + for key, val in template_dict.items(): + if key in specs_by_key: + current = val + for spec in specs_by_key[key]: + if spec.discriminator(current): + template_dict[key] = spec.handler(current, uploader, template_dir) + current = template_dict[key] + elif isinstance(val, dict): + _export_global_artifacts_pass(val, uploader, template_dir) + elif isinstance(val, list): + for item in val: + if isinstance(item, dict): + _export_global_artifacts_pass(item, uploader, template_dir) + return template_dict + + class CloudFormationStackResource(ResourceZip): """ Represents CloudFormation::Stack resource that can refer to a nested @@ -135,21 +220,10 @@ def do_export(self, resource_id, resource_dict, parent_dir): export on the nested template, upload the exported template to S3 and set property to URL of the uploaded S3 template. - When the child template uses CloudFormation Language Extensions - (e.g. Fn::ForEach), the template is first expanded so that - Template.export() can discover and upload all generated artifacts. - The S3 URIs are then merged back into the original template - (preserving the Fn::ForEach structure) before uploading. + Routes to the LE-on or LE-off branch based on + self.language_extensions_enabled. The flag is a hard structural + gate — when off, no language-extension machinery runs. """ - from samcli.lib.cfn_language_extensions.sam_integration import ( - expand_language_extensions, - ) - from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable - from samcli.lib.package.language_extensions_packaging import ( - generate_and_apply_artifact_mappings, - merge_language_extensions_s3_uris, - ) - template_path = resource_dict.get(self.PROPERTY_NAME, None) if template_path is None or is_s3_url(template_path): @@ -162,33 +236,95 @@ def do_export(self, resource_id, resource_dict, parent_dir): property_name=self.PROPERTY_NAME, resource_id=resource_id, template_path=abs_template_path ) - # Read and attempt language extensions expansion on the child template + if self.language_extensions_enabled: + exported_template_dict = self._do_export_with_language_extensions( + resource_id, + template_path, + parent_dir, + abs_template_path, + resource_dict, + ) + else: + exported_template_dict = self._do_export_without_language_extensions(resource_id, template_path, parent_dir) + + exported_template_str = yaml_dump(exported_template_dict) + + with mktempfile() as temporary_file: + temporary_file.write(exported_template_str) + temporary_file.flush() + remote_path = get_uploaded_s3_object_name(file_path=temporary_file.name, extension="template") + url = self.uploader.upload(temporary_file.name, remote_path) + + # TemplateUrl property requires S3 URL to be in path-style format + parts = parse_s3_url(url, version_property="Version") + s3_path_url = self.uploader.to_path_style_s3_url(parts["Key"], parts.get("Version", None)) + set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, s3_path_url) + + def _do_export_without_language_extensions(self, resource_id: str, template_path: str, parent_dir: str) -> Dict: + """LE-off branch: legacy path-based Template construction. + + Template.export() runs its own internal _export_global_artifacts so + AWS::Include still resolves on this path. No pre-LE pass, no + parameter_values, no parent_parameter_values, no copy.deepcopy — + none of them are needed without language extensions. + """ + return Template( + template_path, + parent_dir, + self.uploaders, + self.code_signer, + normalize_template=True, + normalize_parameters=True, + parent_stack_id=resource_id, + language_extensions_enabled=False, + ).export() + + def _do_export_with_language_extensions( + self, + resource_id: str, + template_path: str, + parent_dir: str, + abs_template_path: str, + resource_dict: Dict, + ) -> Dict: + """LE-on branch: dict-based Template construction with full LE machinery. + + Reads the child template to a dict, runs the pre-LE AWS::Include pass + (#9027), threads pseudo-params + parent_parameter_values + nested + Parameters into expand_language_extensions, and merges S3 URIs back + if expansion produced any. + + Falls back to the no-extensions path for InvalidSamDocumentException + (expected user-facing failure) and unexpected exceptions (SAM CLI + bugs — logged at ERROR with traceback). + """ with open(abs_template_path, "r", encoding="utf-8") as f: child_template_dict = yaml_parse(f.read()) child_template_dir = os.path.dirname(abs_template_path) - # Merge pseudo-parameters with: - # 1) values propagated from the parent Template (parent stack's own params - # + CLI --parameter-overrides + pseudo-params), used to resolve intrinsics - # in the nested stack's Parameters property; - # 2) the nested stack's Parameters property on the parent resource, which is - # the authoritative source for the child's parameter values at deploy time. - # Child-local values override parent-propagated ones on key conflict, which - # matches CloudFormation's behavior (a child parameter shadows a same-named - # parent parameter unless explicitly wired). - parent_parameter_values = dict(self.parent_parameter_values or {}) - - nested_params = resource_dict.get("Parameters", {}) or {} - resolved_nested_params = _resolve_nested_stack_parameters(nested_params, parent_parameter_values) + # Process AWS::Include before LE expansion to mirror CFN's transform + # ordering. See aws/aws-sam-cli#9027. + # + # NOTE: mutates child_template_dict in place. Must run before the + # expand_language_extensions call below so result.original_template + # and result.expanded_template both observe the rewrite. + _export_global_artifacts_pass( + child_template_dict, + self.uploaders.get(ResourceZip.EXPORT_DESTINATION), + child_template_dir, + ) - parameter_values = dict(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES) - parameter_values.update(parent_parameter_values) - parameter_values.update(resolved_nested_params) + parameter_values = _build_child_parameter_values( + self.parent_parameter_values, + resource_dict.get("Parameters", {}) or {}, + ) try: result = expand_language_extensions( - child_template_dict, parameter_values, enabled=self.language_extensions_enabled + child_template_dict, + parameter_values, + enabled=self.language_extensions_enabled, ) except InvalidSamDocumentException as e: # Expected failure path: the child template triggered the @@ -207,9 +343,6 @@ def do_export(self, resource_id, resource_dict, parent_dir): ) result = None except Exception as e: # pylint: disable=broad-except - # Unexpected failure: a bug in SAM CLI rather than a malformed template. - # Surface at ERROR with a traceback so users running --debug (and SAM - # telemetry) see it, but don't abort the rest of the package run. LOG.error( "Internal error expanding language extensions for %s. This is a " "SAM CLI bug; please report at " @@ -222,9 +355,11 @@ def do_export(self, resource_id, resource_dict, parent_dir): result = None if result and result.had_language_extensions: - LOG.debug("Child template %s uses language extensions, expanding before export", abs_template_path) + LOG.debug( + "Child template %s uses language extensions, expanding before export", + abs_template_path, + ) - # Create Template from the expanded template string template = Template( template_path, parent_dir, @@ -240,12 +375,12 @@ def do_export(self, resource_id, resource_dict, parent_dir): exported_template = template.export() - # Merge S3 URIs back into the original template (preserving Fn::ForEach) exported_template_dict = merge_language_extensions_s3_uris( - result.original_template, exported_template, result.dynamic_artifact_properties + result.original_template, + exported_template, + result.dynamic_artifact_properties, ) - # Generate and apply Mappings for dynamic artifact properties if result.dynamic_artifact_properties: LOG.debug( "Generating Mappings for %d dynamic artifact properties in child template", @@ -259,7 +394,6 @@ def do_export(self, resource_id, resource_dict, parent_dir): child_template_dir, ) else: - # No language extensions — use existing flow exported_template_dict = Template( template_path, parent_dir, @@ -272,18 +406,7 @@ def do_export(self, resource_id, resource_dict, parent_dir): language_extensions_enabled=self.language_extensions_enabled, ).export() - exported_template_str = yaml_dump(exported_template_dict) - - with mktempfile() as temporary_file: - temporary_file.write(exported_template_str) - temporary_file.flush() - remote_path = get_uploaded_s3_object_name(file_path=temporary_file.name, extension="template") - url = self.uploader.upload(temporary_file.name, remote_path) - - # TemplateUrl property requires S3 URL to be in path-style format - parts = parse_s3_url(url, version_property="Version") - s3_path_url = self.uploader.to_path_style_s3_url(parts["Key"], parts.get("Version", None)) - set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, s3_path_url) + return exported_template_dict class ServerlessApplicationResource(CloudFormationStackResource): @@ -341,7 +464,7 @@ class Template: template_dict: Dict template_dir: str resources_to_export: frozenset - metadata_to_export: frozenset + metadata_to_export: Sequence[MetadataExportSpec] uploaders: Uploaders code_signer: CodeSigner @@ -355,7 +478,7 @@ def __init__( RESOURCES_EXPORT_LIST + [CloudFormationStackResource, CloudFormationStackSetResource, ServerlessApplicationResource] ), - metadata_to_export=frozenset(METADATA_EXPORT_LIST), + metadata_to_export=tuple(METADATA_EXPORTS), template_str: Optional[str] = None, normalize_template: bool = False, normalize_parameters: bool = False, @@ -409,24 +532,18 @@ def __init__( self.language_extensions_enabled = language_extensions_enabled def _export_global_artifacts(self, template_dict: Dict) -> Dict: + """See module-level _export_global_artifacts_pass for the canonical + implementation. This wrapper exists so Template.export() doesn't + need to know about uploader / template_dir plumbing. """ - Template params such as AWS::Include transforms are not specific to - any resource type but contain artifacts that should be exported, - here we iterate through the template dict and export params with a - handler defined in GLOBAL_EXPORT_DICT - """ - for key, val in template_dict.items(): - if key in GLOBAL_EXPORT_DICT: - template_dict[key] = GLOBAL_EXPORT_DICT[key]( - val, self.uploaders.get(ResourceZip.EXPORT_DESTINATION), self.template_dir - ) - elif isinstance(val, dict): - self._export_global_artifacts(val) - elif isinstance(val, list): - for item in val: - if isinstance(item, dict): - self._export_global_artifacts(item) - return template_dict + result = _export_global_artifacts_pass( + template_dict, + self.uploaders.get(ResourceZip.EXPORT_DESTINATION), + self.template_dir, + ) + # template_dict is guaranteed to be a dict here, so the pass returns + # the same dict back. cast() narrows the return type for mypy. + return cast(Dict, result) def _export_metadata(self): """ @@ -436,11 +553,13 @@ def _export_metadata(self): if "Metadata" not in self.template_dict: return - for metadata_type, metadata_dict in self.template_dict["Metadata"].items(): - for exporter_class in self.metadata_to_export: - if exporter_class.RESOURCE_TYPE != metadata_type: - continue + specs_by_type = {spec.metadata_type: spec for spec in self.metadata_to_export} + for metadata_type, metadata_dict in self.template_dict["Metadata"].items(): + spec = specs_by_type.get(metadata_type) + if spec is None: + continue + for exporter_class in spec.exporters: exporter = exporter_class(self.uploaders, self.code_signer) exporter.export(metadata_type, metadata_dict, self.template_dir) diff --git a/samcli/lib/package/language_extensions_packaging.py b/samcli/lib/package/language_extensions_packaging.py index 9eb39366aed..46123ef2994 100644 --- a/samcli/lib/package/language_extensions_packaging.py +++ b/samcli/lib/package/language_extensions_packaging.py @@ -27,6 +27,7 @@ substitute_loop_variable, ) from samcli.lib.cfn_language_extensions.utils import FOREACH_REQUIRED_ELEMENTS, is_foreach_key +from samcli.lib.package.packageable_resources import METADATA_EXPORTS LOG = logging.getLogger(__name__) @@ -82,6 +83,8 @@ def merge_language_extensions_s3_uris( _update_resources_with_s3_uris(original_resources, exported_resources, dynamic_prop_keys) + _merge_metadata(result.get("Metadata", {}), exported_template.get("Metadata", {})) + return result @@ -183,6 +186,24 @@ def _resolve_property_paths(prop_names: List[str], properties: Dict[str, Any]) - # --------------------------------------------------------------------------- +def _merge_metadata( + original_metadata: Dict[str, Any], + exported_metadata: Dict[str, Any], +) -> None: + """Copy rewritten property values from exported_metadata into + original_metadata for every (metadata_type, property_name) declared + in METADATA_EXPORTS. Mutates original_metadata in place. + """ + for spec in METADATA_EXPORTS: + original_entry = original_metadata.get(spec.metadata_type) + exported_entry = exported_metadata.get(spec.metadata_type) + if not isinstance(original_entry, dict) or not isinstance(exported_entry, dict): + continue + for prop_name in spec.property_names: + if prop_name in exported_entry: + original_entry[prop_name] = exported_entry[prop_name] + + def _update_resources_with_s3_uris( original_resources: Dict[str, Any], exported_resources: Dict[str, Any], diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 87f8c484891..5cee49d5c71 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -5,7 +5,8 @@ import logging import os import shutil -from typing import Dict, Optional, Union, cast +from dataclasses import dataclass, field +from typing import Callable, Dict, List, Optional, Type, Union, cast import jmespath from botocore.utils import set_value_from_jmespath @@ -814,7 +815,29 @@ def delete(self, resource_id, resource_dict): GraphQLApiCodeResource, ] -METADATA_EXPORT_LIST = [ServerlessRepoApplicationReadme, ServerlessRepoApplicationLicense] + +@dataclass(frozen=True) +class MetadataExportSpec: + """How a top-level Metadata. entry is exported and merged back. + + Used by Template._export_metadata to route entries to their exporter + classes, and by merge_language_extensions_s3_uris to copy rewritten + property values back into the original (Fn::ForEach-preserving) + child-stack template after LE expansion. + """ + + metadata_type: str + property_names: List[str] + exporters: List[Type["Resource"]] = field(default_factory=list) + + +METADATA_EXPORTS: List[MetadataExportSpec] = [ + MetadataExportSpec( + metadata_type=AWS_SERVERLESSREPO_APPLICATION, + property_names=["LicenseUrl", "ReadmeUrl"], + exporters=[ServerlessRepoApplicationLicense, ServerlessRepoApplicationReadme], + ), +] def include_transform_export_handler(template_dict, uploader, parent_dir): @@ -838,4 +861,29 @@ def include_transform_export_handler(template_dict, uploader, parent_dir): return template_dict -GLOBAL_EXPORT_DICT = {"Fn::Transform": include_transform_export_handler} +@dataclass(frozen=True) +class GlobalTransformExportSpec: + """How a global (anywhere-in-the-tree) export hook is invoked. + + The discriminator decides whether a given dict node matches this spec + (dispatch on Fn::Transform's "Name" field today; ready for future + Fn::Transform variants). The handler receives the matched node and + mutates it in place to rewrite local paths to S3 URLs. + """ + + template_key: str + discriminator: Callable[[object], bool] + handler: Callable + + +def _is_aws_include(node: object) -> bool: + return isinstance(node, dict) and node.get("Name") == "AWS::Include" + + +GLOBAL_TRANSFORM_EXPORTS: List[GlobalTransformExportSpec] = [ + GlobalTransformExportSpec( + template_key="Fn::Transform", + discriminator=_is_aws_include, + handler=include_transform_export_handler, + ), +] diff --git a/tests/unit/commands/package/test_data/buried_aws_include/export-events.json b/tests/unit/commands/package/test_data/buried_aws_include/export-events.json new file mode 100644 index 00000000000..2eb28cebd55 --- /dev/null +++ b/tests/unit/commands/package/test_data/buried_aws_include/export-events.json @@ -0,0 +1 @@ +[{"event": "demo"}] diff --git a/tests/unit/commands/package/test_data/buried_aws_include/template.yaml b/tests/unit/commands/package/test_data/buried_aws_include/template.yaml new file mode 100644 index 00000000000..2558ea1fcc1 --- /dev/null +++ b/tests/unit/commands/package/test_data/buried_aws_include/template.yaml @@ -0,0 +1,13 @@ +Transform: AWS::LanguageExtensions +Resources: + Parameter: + Type: AWS::SSM::Parameter + Properties: + Type: String + DataType: text + Value: + Fn::ToJsonString: + Fn::Transform: + Name: AWS::Include + Parameters: + Location: export-events.json diff --git a/tests/unit/commands/package/test_package_context.py b/tests/unit/commands/package/test_package_context.py index 3e641c548b6..a9e255dace8 100644 --- a/tests/unit/commands/package/test_package_context.py +++ b/tests/unit/commands/package/test_package_context.py @@ -1,10 +1,14 @@ """Test sam package command""" +import os +from pathlib import Path from unittest import TestCase from unittest.mock import patch, MagicMock, Mock, call, ANY from parameterized import parameterized import tempfile +TEST_DATA_PATH = Path(__file__).resolve().parent / "test_data" + from samcli.commands.package.package_context import PackageContext from samcli.commands.package.exceptions import PackageFailedError @@ -27,9 +31,11 @@ _apply_artifact_mappings_to_template, _replace_dynamic_artifact_with_findmap, ) +from samcli.lib.package.uploaders import Destination, Uploaders from samcli.lib.providers.sam_stack_provider import SamLocalStackProvider from samcli.lib.samlib.resource_metadata_normalizer import ResourceMetadataNormalizer from samcli.lib.utils.resources import AWS_LAMBDA_FUNCTION, AWS_SERVERLESS_FUNCTION +from samcli.yamlhelper import yaml_parse class TestPackageCommand(TestCase): @@ -4462,3 +4468,124 @@ def test_apply_mappings_replaces_build_mappings(self): codeuri = body["${FunctionName}Function"]["Properties"]["CodeUri"] self.assertIn("Fn::FindInMap", codeuri) self.assertEqual(codeuri["Fn::FindInMap"][0], "SAMCodeUriFunctions") + + +class TestPackageContextBuriedAWSInclude(TestCase): + """A root-level template that uses Fn::ToJsonString (a language extension) + and contains AWS::Include buried inside AWS::SSM::Parameter must end up + with the include's Location rewritten to the s3:// URL the uploader + returned, matching the behavior of sam-cli 1.159.0. + + Regression coverage for https://github.com/aws/aws-sam-cli/issues/9027. + Nested-stack coverage lives in tests/unit/lib/package/test_artifact_exporter.py. + """ + + def setUp(self): + self.template_path = str(TEST_DATA_PATH / "buried_aws_include" / "template.yaml") + + # Mock uploader with deterministic S3 URL. + self.s3_uploader = MagicMock() + self.s3_uploader.upload_with_dedup.side_effect = ( + lambda local_path, **_: f"s3://my-bucket/{os.path.basename(local_path)}-md5" + ) + self.s3_uploader.EXPORT_DESTINATION = Destination.S3 + + self.uploaders = MagicMock(spec=Uploaders) + self.uploaders.get.return_value = self.s3_uploader + + # Bypass PackageContext.__init__ — wire only the attributes _export reads. + self.ctx = PackageContext.__new__(PackageContext) + self.ctx.template_file = self.template_path + self.ctx.uploaders = self.uploaders + self.ctx.code_signer = MagicMock() + self.ctx.parameter_overrides = {} + self.ctx._global_parameter_overrides = {} + # Language Extensions is opt-in (#9033); the buried-AWS::Include regression + # test exercises a template that uses Transform: AWS::LanguageExtensions, + # so enable LE expansion explicitly. + self.ctx._language_extensions_enabled = True + + def test_buried_aws_include_location_is_rewritten(self): + exported_str = self.ctx._export(self.template_path, use_json=False) + + exported = yaml_parse(exported_str) + + location = exported["Resources"]["Parameter"]["Properties"]["Value"]["Fn::ToJsonString"]["Fn::Transform"][ + "Parameters" + ]["Location"] + self.assertTrue( + location.startswith("s3://"), + f"Issue #9027: AWS::Include Location must be an S3 URI after sam package, " f"got {location!r}.", + ) + self.assertEqual(location, "s3://my-bucket/export-events.json-md5") + + # Sanity: the LE Transform line is preserved on the root. + self.assertEqual(exported.get("Transform"), "AWS::LanguageExtensions") + + +class TestExportLanguageExtensionsStructuralGate(TestCase): + """Lock in the contract that --language-extensions is a hard correctness + boundary, not a passthrough hint. When disabled, _export() must not invoke + any LE machinery (expand_language_extensions, the pre-LE + _export_global_artifacts_pass, merge_language_extensions_s3_uris, or + generate_and_apply_artifact_mappings). + """ + + def _make_off_path_context(self): + """Build a PackageContext with LE disabled and the attributes _export reads.""" + ctx = PackageContext.__new__(PackageContext) + ctx.template_file = "template.yaml" + ctx.uploaders = MagicMock() + ctx.code_signer = MagicMock() + ctx.parameter_overrides = {} + ctx._global_parameter_overrides = {} + ctx._language_extensions_enabled = False + return ctx + + @patch("samcli.commands.package.package_context.Template") + @patch("samcli.commands.package.package_context.yaml_parse") + @patch("builtins.open", create=True) + @patch("samcli.commands.package.package_context.expand_language_extensions") + def test_off_path_does_not_invoke_expand_language_extensions( + self, mock_expand, mock_open, mock_yaml_parse, mock_template_class + ): + """When --language-extensions is off, expand_language_extensions must not be called.""" + ctx = self._make_off_path_context() + + mock_yaml_parse.return_value = {"Resources": {}} + mock_template_instance = MagicMock() + mock_template_instance.export.return_value = {"Resources": {}} + mock_template_class.return_value = mock_template_instance + + mock_file = MagicMock() + mock_open.return_value.__enter__.return_value = mock_file + mock_file.read.return_value = "" + + ctx._export("template.yaml", use_json=False) + + mock_expand.assert_not_called() + + @patch("samcli.commands.package.package_context.Template") + @patch("samcli.commands.package.package_context.yaml_parse") + @patch("builtins.open", create=True) + @patch("samcli.commands.package.package_context._export_global_artifacts_pass") + def test_off_path_does_not_invoke_pre_le_global_transform_pass( + self, mock_pre_le_pass, mock_open, mock_yaml_parse, mock_template_class + ): + """When --language-extensions is off, the pre-LE _export_global_artifacts_pass + in package_context.py must not be called. Template.export() still runs its own + internal _export_global_artifacts pass — that one is not patched here.""" + ctx = self._make_off_path_context() + + mock_yaml_parse.return_value = {"Resources": {}} + mock_template_instance = MagicMock() + mock_template_instance.export.return_value = {"Resources": {}} + mock_template_class.return_value = mock_template_instance + + mock_file = MagicMock() + mock_open.return_value.__enter__.return_value = mock_file + mock_file.read.return_value = "" + + ctx._export("template.yaml", use_json=False) + + mock_pre_le_pass.assert_not_called() diff --git a/tests/unit/lib/cfn_language_extensions/test_phase_separation.py b/tests/unit/lib/cfn_language_extensions/test_phase_separation.py index 7eccaba88c1..fccaf616714 100644 --- a/tests/unit/lib/cfn_language_extensions/test_phase_separation.py +++ b/tests/unit/lib/cfn_language_extensions/test_phase_separation.py @@ -581,7 +581,7 @@ def test_sam_template_validator_no_longer_has_process_language_extensions(self): @patch("samcli.commands.package.package_context.yaml_parse") @patch("builtins.open", create=True) - @patch("samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions") + @patch("samcli.commands.package.package_context.expand_language_extensions") def test_package_context_export_calls_expand_language_extensions(self, mock_expand, mock_open, mock_yaml_parse): """PackageContext._export() calls expand_language_extensions().""" from samcli.commands.package.package_context import PackageContext diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 59f7be0b1d3..430d775855f 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1,13 +1,17 @@ +import copy import functools +import inspect import json import os import platform import random +import shutil import string import tempfile import unittest import zipfile from contextlib import contextmanager, closing +from pathlib import Path from typing import Optional, Dict from unittest import mock from unittest.mock import call, patch, Mock, MagicMock @@ -16,6 +20,10 @@ from samcli.commands.package import exceptions from samcli.commands.package.exceptions import ExportFailedError from samcli.commands.validate.lib.exceptions import InvalidSamDocumentException +from samcli.lib.cfn_language_extensions.sam_integration import ( + LanguageExtensionResult, + expand_language_extensions, +) from samcli.lib.package.artifact_exporter import ( is_local_folder, make_abs_path, @@ -23,13 +31,20 @@ CloudFormationStackResource, CloudFormationStackSetResource, ServerlessApplicationResource, + _build_child_parameter_values, + _export_global_artifacts_pass, + _resolve_nested_stack_parameters, ) +from samcli.lib.package.language_extensions_packaging import merge_language_extensions_s3_uris +from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable from samcli.lib.package.packageable_resources import ( GraphQLApiCodeResource, GraphQLApiSchemaResource, is_s3_protocol_url, is_local_file, upload_local_artifacts, + METADATA_EXPORTS, + MetadataExportSpec, Resource, ResourceWithS3UrlDict, ServerlessApiResource, @@ -41,7 +56,8 @@ LambdaLayerVersionResource, copy_to_temp_dir, include_transform_export_handler, - GLOBAL_EXPORT_DICT, + GLOBAL_TRANSFORM_EXPORTS, + GlobalTransformExportSpec, ServerlessLayerVersionResource, ServerlessRepoApplicationLicense, ServerlessRepoApplicationReadme, @@ -69,8 +85,11 @@ from samcli.lib.package.utils import zip_folder, make_zip, make_zip_with_lambda_permissions, make_zip_with_permissions from samcli.lib.utils.packagetype import ZIP, IMAGE from samcli.lib.utils.resources import LAMBDA_LOCAL_RESOURCES, RESOURCES_WITH_LOCAL_PATHS +from samcli.yamlhelper import yaml_dump from tests.testing_utils import FileCreator +TEST_DATA_PATH = Path(__file__).resolve().parent / "test_data" + class TestArtifactExporter(unittest.TestCase): def setUp(self): @@ -1171,6 +1190,7 @@ class MockResource(ResourceZip): @patch("samcli.lib.package.artifact_exporter.Template") def test_export_cloudformation_stack(self, TemplateMock): stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) + stack_resource.language_extensions_enabled = True resource_id = "id" property_name = stack_resource.PROPERTY_NAME @@ -1205,7 +1225,7 @@ def test_export_cloudformation_stack(self, TemplateMock): normalize_template=True, parent_stack_id="id", parameter_values=mock.ANY, - language_extensions_enabled=False, + language_extensions_enabled=True, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1353,6 +1373,7 @@ def test_export_cloudformation_stack_set_no_upload_path_not_file(self): @patch("samcli.lib.package.artifact_exporter.Template") def test_export_serverless_application(self, TemplateMock): stack_resource = ServerlessApplicationResource(self.uploaders_mock, self.code_signer_mock) + stack_resource.language_extensions_enabled = True resource_id = "id" property_name = stack_resource.PROPERTY_NAME @@ -1387,7 +1408,7 @@ def test_export_serverless_application(self, TemplateMock): normalize_template=True, parent_stack_id="id", parameter_values=mock.ANY, - language_extensions_enabled=False, + language_extensions_enabled=True, ) template_instance_mock.export.assert_called_once_with() self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) @@ -1478,7 +1499,18 @@ def test_template_export_metadata(self, yaml_parse_mock): metadata_type2_instance = Mock() metadata_type2_class.return_value = metadata_type2_instance - metadata_to_export = [metadata_type1_class, metadata_type2_class] + metadata_to_export = [ + MetadataExportSpec( + metadata_type="metadata_type1", + property_names=["property_1"], + exporters=[metadata_type1_class], + ), + MetadataExportSpec( + metadata_type="metadata_type2", + property_names=["property_2"], + exporters=[metadata_type2_class], + ), + ] template_dict = {"Metadata": {"metadata_type1": {"property_1": "abc"}, "metadata_type2": {"property_2": "def"}}} open_mock = mock.mock_open() @@ -1854,8 +1886,13 @@ def test_template_global_export(self, yaml_parse_mock): } yaml_parse_mock.return_value = template_dict + mock_spec = GlobalTransformExportSpec( + template_key="Fn::Transform", + discriminator=lambda v: isinstance(v, dict), + handler=include_transform_export_handler_mock, + ) with patch("samcli.lib.package.artifact_exporter.open", open_mock(read_data=template_str)) as open_mock: - with patch.dict(GLOBAL_EXPORT_DICT, {"Fn::Transform": include_transform_export_handler_mock}): + with patch("samcli.lib.package.artifact_exporter.GLOBAL_TRANSFORM_EXPORTS", [mock_spec]): template_exporter = Template(template_path, parent_dir, self.uploaders_mock, resources_to_export) exported_template = template_exporter._export_global_artifacts(template_exporter.template_dict) @@ -1972,6 +2009,186 @@ def test_include_transform_export_handler_non_include_transform(self, is_local_f self.s3_uploader_mock.upload_with_dedup.assert_not_called() self.assertEqual(handler_output, {"Name": "AWS::OtherTransform", "Parameters": {"Location": "foo.yaml"}}) + def test_le_template_with_top_level_aws_include_merges_location(self): + """End-to-end regression: an LE template with a top-level AWS::Include + in Outputs (outside any Fn::ForEach body) must end up with its + Location rewritten to the uploader's S3 URL after the + package_context._export-equivalent flow: + + _export_global_artifacts_pass(template_dict, ...) + -> expand_language_extensions(...) + -> Template(template_dict=expanded).export() + -> merge_language_extensions_s3_uris(original, exported, dynamic) + + The pre-LE global-transform pass is what now rewrites AWS::Include + Locations; the merge pass only handles resource artifact properties + and Metadata. See aws/aws-sam-cli#9027. + """ + # Predictable upload URL keyed on basename so we can assert on it later. + # upload_with_dedup is invoked with absolute paths (for AWS::Include) and + # with absolute paths to temp zips (for ServerlessFunctionResource folder + # uploads); both forms have a basename we can use. + self.s3_uploader_mock.upload_with_dedup.side_effect = ( + lambda local_path, **_: f"s3://bucket/{os.path.basename(local_path)}-md5" + ) + + # Real files on disk so is_local_file / is_local_folder return True + # without needing to mock them. extra.yaml is the AWS::Include target; + # code/ is the CodeUri folder for the (post-expansion) functions. + parent_dir = str(TEST_DATA_PATH / "le_top_level_include") + + template_dict = { + "Transform": "AWS::LanguageExtensions", + "Resources": { + "Fn::ForEach::Services": [ + "Name", + ["A", "B"], + { + "${Name}Function": { + "Type": "AWS::Serverless::Function", + "Properties": { + "CodeUri": "./code", + "Handler": "index.handler", + "Runtime": "python3.11", + }, + } + }, + ] + }, + "Outputs": { + "Extra": { + "Value": { + "Fn::Transform": { + "Name": "AWS::Include", + "Parameters": {"Location": "./extra.yaml"}, + } + } + } + }, + } + + # Mirror PackageContext._export's flow: run the pre-LE + # global-transform pass on the original template before + # language-extension expansion. + _export_global_artifacts_pass(template_dict, self.s3_uploader_mock, parent_dir) + + parameter_values = dict(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES) + + result = expand_language_extensions(template_dict, parameter_values, enabled=True) + self.assertTrue(result.had_language_extensions) + + template = Template( + template_path="template.yaml", + parent_dir=parent_dir, + uploaders=self.uploaders_mock, + code_signer=self.code_signer_mock, + normalize_template=True, + normalize_parameters=True, + template_dict=copy.deepcopy(result.expanded_template), + parameter_values=parameter_values, + ) + exported_template = template.export() + + output = merge_language_extensions_s3_uris( + result.original_template, exported_template, result.dynamic_artifact_properties + ) + + # The original Fn::ForEach is preserved (not re-expanded) in the merged output. + self.assertIn("Fn::ForEach::Services", output["Resources"]) + + # The top-level AWS::Include Location is rewritten to the uploader's S3 URL. + out_location = output["Outputs"]["Extra"]["Value"]["Fn::Transform"]["Parameters"]["Location"] + self.assertEqual(out_location, "s3://bucket/extra.yaml-md5") + + def test_le_template_with_serverless_repo_metadata_merges_license_url(self): + """End-to-end regression: an LE template with + AWS::ServerlessRepo::Application metadata containing local + LicenseUrl/ReadmeUrl paths must end up with both rewritten to + S3 URLs after the package_context._export-equivalent flow: + + expand_language_extensions(...) + -> Template(template_dict=expanded).export() + -> merge_language_extensions_s3_uris(original, exported, dynamic) + + This covers the Metadata merge pass (Task 4) wired into the + artifact-exporter's export plumbing. + """ + # SAR exporters extend ResourceZip, whose do_export ultimately + # routes through upload_local_artifacts -> uploader.upload_with_dedup. + # Same lambda as Task 7: a predictable URL keyed on basename. + self.s3_uploader_mock.upload_with_dedup.side_effect = ( + lambda local_path, **_: f"s3://bucket/{os.path.basename(local_path)}-md5" + ) + + # Real files on disk for the SAR exporters (LicenseUrl / ReadmeUrl) + # and for the Fn::ForEach-expanded SAM function's CodeUri. + parent_dir = str(TEST_DATA_PATH / "le_sar_metadata") + + template_dict = { + "Transform": "AWS::LanguageExtensions", + "Metadata": { + "AWS::ServerlessRepo::Application": { + "Name": "MyApp", + "Description": "Test app", + "Author": "Test", + "SemanticVersion": "1.0.0", + "LicenseUrl": "./LICENSE.txt", + "ReadmeUrl": "./README.md", + } + }, + "Resources": { + "Fn::ForEach::Services": [ + "Name", + ["A"], + { + "${Name}Function": { + "Type": "AWS::Serverless::Function", + "Properties": { + "CodeUri": "./code", + "Handler": "index.handler", + "Runtime": "python3.11", + }, + } + }, + ] + }, + } + + parameter_values = dict(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES) + + result = expand_language_extensions(template_dict, parameter_values, enabled=True) + self.assertTrue(result.had_language_extensions) + + template = Template( + template_path="template.yaml", + parent_dir=parent_dir, + uploaders=self.uploaders_mock, + code_signer=self.code_signer_mock, + normalize_template=True, + normalize_parameters=True, + template_dict=copy.deepcopy(result.expanded_template), + parameter_values=parameter_values, + ) + exported_template = template.export() + + output = merge_language_extensions_s3_uris( + result.original_template, exported_template, result.dynamic_artifact_properties + ) + + # SAR LicenseUrl and ReadmeUrl must both be rewritten to S3 URLs. + sar = output["Metadata"]["AWS::ServerlessRepo::Application"] + self.assertTrue(sar["LicenseUrl"].startswith("s3://"), f"LicenseUrl: {sar['LicenseUrl']!r}") + self.assertTrue(sar["ReadmeUrl"].startswith("s3://"), f"ReadmeUrl: {sar['ReadmeUrl']!r}") + # The basenames identify which file was uploaded to which property, + # confirming the SAR exporter fired once for License and once for Readme. + self.assertIn("LICENSE.txt", sar["LicenseUrl"]) + self.assertIn("README.md", sar["ReadmeUrl"]) + # Unrelated SAR metadata is preserved untouched. + self.assertEqual(sar["Name"], "MyApp") + self.assertEqual(sar["SemanticVersion"], "1.0.0") + # The original Fn::ForEach is preserved (not re-expanded) in the merged output. + self.assertIn("Fn::ForEach::Services", output["Resources"]) + def test_template_export_path_be_folder(self): template_path = "/path/foo" # Set parent_dir to be a non-existent folder @@ -2313,9 +2530,8 @@ def test_export_cloudformation_stack_with_language_extensions(self, TemplateMock When a child template uses Fn::ForEach, do_export should expand it, export the expanded template, then merge S3 URIs back. """ - from samcli.lib.cfn_language_extensions.sam_integration import LanguageExtensionResult - stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) + stack_resource.language_extensions_enabled = True resource_id = "NestedStack" property_name = stack_resource.PROPERTY_NAME @@ -2416,7 +2632,7 @@ def test_export_cloudformation_stack_with_language_extensions(self, TemplateMock parent_dir = tempfile.gettempdir() with patch( - "samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions", + "samcli.lib.package.artifact_exporter.expand_language_extensions", return_value=lang_ext_result, ): stack_resource.export(resource_id, resource_dict, parent_dir) @@ -2434,14 +2650,14 @@ def test_export_cloudformation_stack_with_language_extensions(self, TemplateMock os.remove(template_path) @patch("samcli.lib.package.artifact_exporter.Template") - def test_export_cloudformation_stack_without_language_extensions(self, TemplateMock): + def test_export_cloudformation_stack_language_extensions_no_le_in_template(self, TemplateMock): """ - When a child template does NOT use language extensions, - do_export should use the original flow (no expansion). + When LE is opted-in but the child template uses no LE, do_export + should still go through the LE-on branch and exit via the + no-extension sub-path. """ - from samcli.lib.cfn_language_extensions.sam_integration import LanguageExtensionResult - stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) + stack_resource.language_extensions_enabled = True resource_id = "NestedStack" property_name = stack_resource.PROPERTY_NAME @@ -2473,7 +2689,7 @@ def test_export_cloudformation_stack_without_language_extensions(self, TemplateM parent_dir = tempfile.gettempdir() with patch( - "samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions", + "samcli.lib.package.artifact_exporter.expand_language_extensions", return_value=no_ext_result, ): stack_resource.export(resource_id, resource_dict, parent_dir) @@ -2495,6 +2711,7 @@ def test_export_cloudformation_stack_language_extensions_expansion_failure(self, do_export should fall back to the original flow. """ stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) + stack_resource.language_extensions_enabled = True resource_id = "NestedStack" property_name = stack_resource.PROPERTY_NAME @@ -2519,7 +2736,7 @@ def test_export_cloudformation_stack_language_extensions_expansion_failure(self, parent_dir = tempfile.gettempdir() with patch( - "samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions", + "samcli.lib.package.artifact_exporter.expand_language_extensions", side_effect=InvalidSamDocumentException("expansion failed"), ): stack_resource.export(resource_id, resource_dict, parent_dir) @@ -2539,6 +2756,7 @@ def test_export_cloudformation_stack_unexpected_exception_falls_back(self, Templ it should fall back gracefully to the non-extension flow (not abort the parent packaging). """ stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) + stack_resource.language_extensions_enabled = True resource_id = "NestedStack" property_name = stack_resource.PROPERTY_NAME @@ -2563,7 +2781,7 @@ def test_export_cloudformation_stack_unexpected_exception_falls_back(self, Templ parent_dir = tempfile.gettempdir() with patch( - "samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions", + "samcli.lib.package.artifact_exporter.expand_language_extensions", side_effect=TypeError("unexpected bug"), ): stack_resource.export(resource_id, resource_dict, parent_dir) @@ -2573,6 +2791,31 @@ def test_export_cloudformation_stack_unexpected_exception_falls_back(self, Templ finally: os.remove(template_path) + def test_metadata_exports_registry_shape(self): + self.assertTrue(all(isinstance(s, MetadataExportSpec) for s in METADATA_EXPORTS)) + + by_type = {s.metadata_type: s for s in METADATA_EXPORTS} + spec = by_type["AWS::ServerlessRepo::Application"] + + self.assertEqual(sorted(spec.property_names), ["LicenseUrl", "ReadmeUrl"]) + self.assertIn(ServerlessRepoApplicationLicense, spec.exporters) + self.assertIn(ServerlessRepoApplicationReadme, spec.exporters) + + def test_global_transform_exports_registry_shape(self): + self.assertTrue(all(isinstance(s, GlobalTransformExportSpec) for s in GLOBAL_TRANSFORM_EXPORTS)) + + aws_include = next( + s for s in GLOBAL_TRANSFORM_EXPORTS if s.discriminator({"Name": "AWS::Include", "Parameters": {}}) + ) + + self.assertEqual(aws_include.template_key, "Fn::Transform") + self.assertIs(aws_include.handler, include_transform_export_handler) + + # Discriminator must reject non-AWS::Include transforms and non-dict inputs + self.assertFalse(aws_include.discriminator({"Name": "AWS::OtherTransform"})) + self.assertFalse(aws_include.discriminator(None)) + self.assertFalse(aws_include.discriminator("not-a-dict")) + class TestResolveNestedStackParameters(unittest.TestCase): """Unit tests for _resolve_nested_stack_parameters helper. @@ -2584,16 +2827,12 @@ class TestResolveNestedStackParameters(unittest.TestCase): """ def test_literal_values_pass_through(self): - from samcli.lib.package.artifact_exporter import _resolve_nested_stack_parameters - resolved = _resolve_nested_stack_parameters( {"ServiceNames": "Users,Orders,Products"}, parent_parameter_values={} ) self.assertEqual(resolved, {"ServiceNames": "Users,Orders,Products"}) def test_ref_to_parent_parameter_resolves(self): - from samcli.lib.package.artifact_exporter import _resolve_nested_stack_parameters - resolved = _resolve_nested_stack_parameters( {"ServiceNames": {"Ref": "TopLevelServices"}}, parent_parameter_values={"TopLevelServices": "Users,Orders"}, @@ -2601,8 +2840,6 @@ def test_ref_to_parent_parameter_resolves(self): self.assertEqual(resolved, {"ServiceNames": "Users,Orders"}) def test_unresolvable_getatt_is_dropped(self): - from samcli.lib.package.artifact_exporter import _resolve_nested_stack_parameters - resolved = _resolve_nested_stack_parameters( {"TopicArn": {"Fn::GetAtt": ["SomeResource", "Arn"]}}, parent_parameter_values={} ) @@ -2611,15 +2848,11 @@ def test_unresolvable_getatt_is_dropped(self): self.assertEqual(resolved, {}) def test_empty_input(self): - from samcli.lib.package.artifact_exporter import _resolve_nested_stack_parameters - self.assertEqual(_resolve_nested_stack_parameters({}, {}), {}) self.assertEqual(_resolve_nested_stack_parameters(None, {}), {}) def test_unresolvable_ref_to_resource_is_dropped(self): """Ref to a resource (not a parameter) raises UnresolvableReferenceError and is dropped.""" - from samcli.lib.package.artifact_exporter import _resolve_nested_stack_parameters - # Ref to "MyBucket" — not in parameter_values, not a pseudo-param → resource ref → preserved # as {"Ref": "MyBucket"} by partial resolver → dropped by the intrinsic-detection heuristic resolved = _resolve_nested_stack_parameters({"BucketName": {"Ref": "MyBucket"}}, parent_parameter_values={}) @@ -2627,8 +2860,6 @@ def test_unresolvable_ref_to_resource_is_dropped(self): def test_fn_sub_partially_resolves(self): """Fn::Sub with resolvable parts produces a string (not dropped).""" - from samcli.lib.package.artifact_exporter import _resolve_nested_stack_parameters - resolved = _resolve_nested_stack_parameters( {"Endpoint": {"Fn::Sub": "https://${MyApi}.execute-api.${AWS::Region}.amazonaws.com"}}, parent_parameter_values={"AWS::Region": "us-east-1"}, @@ -2670,6 +2901,84 @@ def test_template_str_path_parses_yaml(self): self.assertEqual(t.template_dict["Resources"]["MyTopic"]["Type"], "AWS::SNS::Topic") +class TestBuildChildParameterValues(unittest.TestCase): + """Unit tests for _build_child_parameter_values, the CFN-parity helper that + builds the parameter_values dict threaded into expand_language_extensions + for a nested child stack. + + Contract: result contains pseudo-params (with parent overrides for pseudo + NAMES only) and parent-rebound values from the nested-stack `Parameters:` + property. Non-pseudo parent names are NOT copied. Child template Defaults + are NOT folded in — the LE expander reads them from + parsed_template.parameters[X].Default at resolve time. + """ + + def test_excludes_non_pseudo_parent_params(self): + """Non-pseudo parent names must not leak into the child's scope.""" + result = _build_child_parameter_values( + parent_parameter_values={"Foo": "parent_foo", "AWS::Region": "us-east-1"}, + nested_stack_parameters={}, + ) + + self.assertNotIn("Foo", result) + # Pseudo baseline is present. + self.assertEqual(result.get("AWS::Region"), "us-east-1") + + def test_includes_pseudo_overrides_from_parent(self): + """Parent's override of a pseudo name must propagate to the child.""" + result = _build_child_parameter_values( + parent_parameter_values={"AWS::Region": "eu-west-1"}, + nested_stack_parameters={}, + ) + + self.assertEqual(result["AWS::Region"], "eu-west-1") + + def test_resolved_nested_rebinding_appears_in_result(self): + """Parent-rebound values reach the child.""" + result = _build_child_parameter_values( + parent_parameter_values={"ParentBar": "rebound"}, + nested_stack_parameters={"Bar": {"Ref": "ParentBar"}}, + ) + + self.assertEqual(result["Bar"], "rebound") + + def test_resolved_nested_rebinding_can_override_pseudo_named_key(self): + """Edge case: explicit nested-stack rebinding of a pseudo name wins + over the parent's pseudo-name override (step 3 dominates step 2 in + the merge order). Preserves existing _resolve_nested_stack_parameters + semantics. + """ + result = _build_child_parameter_values( + parent_parameter_values={"AWS::Region": "us-east-1", "MyRegion": "ap-south-1"}, + nested_stack_parameters={"AWS::Region": {"Ref": "MyRegion"}}, + ) + + self.assertEqual(result["AWS::Region"], "ap-south-1") + + def test_no_parent_parameter_values_uses_defaults(self): + """parent_parameter_values=None must not crash and must yield exactly + the default pseudo-param baseline.""" + result = _build_child_parameter_values( + parent_parameter_values=None, + nested_stack_parameters={}, + ) + + self.assertEqual(result, dict(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES)) + + def test_does_not_fold_child_defaults(self): + """Helper signature does not take a child template dict. Documents the + design choice that child Defaults are picked up by the LE expander's + resolver via parsed_template.parameters[X].Default — not folded in + here. If a future contributor adds Default-folding to this helper, + this test prevents that drift. + """ + sig = inspect.signature(_build_child_parameter_values) + self.assertEqual( + list(sig.parameters.keys()), + ["parent_parameter_values", "nested_stack_parameters"], + ) + + class TestCloudFormationStackResourceChildExpansion(unittest.TestCase): """End-to-end test for threading parent params into child template expansion. @@ -2691,64 +3000,165 @@ def _make_stack_resource(self): def test_child_template_receives_parent_parameters(self): stack_resource, _ = self._make_stack_resource() + stack_resource.language_extensions_enabled = True - # Child template: Fn::ForEach driven by a child parameter. - child_template = { - "AWSTemplateFormatVersion": "2010-09-09", - "Transform": "AWS::LanguageExtensions", - "Parameters": {"ServiceNames": {"Type": "CommaDelimitedList"}}, - "Resources": { - "Fn::ForEach::Services": [ - "Name", - {"Ref": "ServiceNames"}, - {"${Name}Topic": {"Type": "AWS::SNS::Topic", "Properties": {}}}, - ] - }, + parent_dir = str(TEST_DATA_PATH / "child_servicenames") + child_path = str(TEST_DATA_PATH / "child_servicenames" / "child.yaml") + + # Parent-propagated values (what PackageContext._export would send) + stack_resource.parent_parameter_values = { + "AWS::Region": "us-east-1", + "AWS::AccountId": "123456789012", } - tmpdir = tempfile.mkdtemp() - try: - child_path = os.path.join(tmpdir, "child.yaml") - from samcli.yamlhelper import yaml_dump + # Parent's nested-stack resource: Parameters literal list + resource_dict = { + "TemplateURL": child_path, + "Parameters": {"ServiceNames": "Users,Orders,Products"}, + } - with open(child_path, "w", encoding="utf-8") as f: - f.write(yaml_dump(child_template)) + with patch("samcli.lib.package.artifact_exporter.expand_language_extensions") as expand_mock: + # Short-circuit: make expand return a "no-LE" result so do_export + # proceeds via the non-extension path (which doesn't upload since + # we'll intercept the inner Template). + expand_mock.return_value = Mock(had_language_extensions=False) + with patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock: + inner_template = Mock() + inner_template.export.return_value = {"Resources": {}} + TemplateMock.return_value = inner_template + stack_resource.do_export("ChildStack", resource_dict, parent_dir) + + # Verify the parameter_values threaded into expand_language_extensions + # contains the nested-stack Parameters values. + call_args = expand_mock.call_args[0] + call_kwargs = expand_mock.call_args[1] + passed_params = call_args[1] if len(call_args) > 1 else call_kwargs.get("parameter_values", {}) + self.assertEqual(passed_params.get("ServiceNames"), "Users,Orders,Products") + # And still carries propagated parent/pseudo values: + self.assertEqual(passed_params.get("AWS::Region"), "us-east-1") + + def test_parent_param_does_not_leak_into_child_with_same_name(self): + stack_resource, _ = self._make_stack_resource() + stack_resource.language_extensions_enabled = True - # Parent-propagated values (what PackageContext._export would send) - stack_resource.parent_parameter_values = { - "AWS::Region": "us-east-1", - "AWS::AccountId": "123456789012", - } + parent_dir = str(TEST_DATA_PATH / "child_foo_param") + child_path = str(TEST_DATA_PATH / "child_foo_param" / "child.yaml") - # Parent's nested-stack resource: Parameters literal list - resource_dict = { - "TemplateURL": child_path, - "Parameters": {"ServiceNames": "Users,Orders,Products"}, - } + # Parent has a non-pseudo Foo with a value — this is the leak source. + stack_resource.parent_parameter_values = { + "AWS::Region": "us-east-1", + "AWS::AccountId": "123456789012", + "Foo": "parent_foo", + } + # Nested-stack Parameters: does NOT rebind Foo. + resource_dict = { + "TemplateURL": child_path, + "Parameters": {}, + } - with patch("samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions") as expand_mock: - # Short-circuit: make expand return a "no-LE" result so do_export - # proceeds via the non-extension path (which doesn't upload since - # we'll intercept the inner Template). - expand_mock.return_value = Mock(had_language_extensions=False) - with patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock: - inner_template = Mock() - inner_template.export.return_value = {"Resources": {}} - TemplateMock.return_value = inner_template - stack_resource.do_export("ChildStack", resource_dict, tmpdir) - - # Verify the parameter_values threaded into expand_language_extensions - # contains the nested-stack Parameters values. - call_args = expand_mock.call_args[0] - call_kwargs = expand_mock.call_args[1] - passed_params = call_args[1] if len(call_args) > 1 else call_kwargs.get("parameter_values", {}) - self.assertEqual(passed_params.get("ServiceNames"), "Users,Orders,Products") - # And still carries propagated parent/pseudo values: - self.assertEqual(passed_params.get("AWS::Region"), "us-east-1") - finally: - import shutil + with patch("samcli.lib.package.artifact_exporter.expand_language_extensions") as expand_mock: + expand_mock.return_value = Mock(had_language_extensions=False) + with patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock: + TemplateMock.return_value.export.return_value = {"Resources": {}} + stack_resource.do_export("ChildStack", resource_dict, parent_dir) + + call_args = expand_mock.call_args[0] + call_kwargs = expand_mock.call_args[1] + passed_params = call_args[1] if len(call_args) > 1 else call_kwargs.get("parameter_values", {}) + + self.assertNotIn( + "Foo", + passed_params, + "Non-pseudo parent param leaked into child's parameter_values " "(CFN-parity violation).", + ) + # Pseudos still propagate. + self.assertEqual(passed_params.get("AWS::Region"), "us-east-1") + self.assertEqual(passed_params.get("AWS::AccountId"), "123456789012") + + def test_child_default_still_resolves_via_resolver_fallback(self): + stack_resource, _ = self._make_stack_resource() + stack_resource.language_extensions_enabled = True + + parent_dir = str(TEST_DATA_PATH / "child_bar_default") + child_path = str(TEST_DATA_PATH / "child_bar_default" / "child.yaml") + + stack_resource.parent_parameter_values = { + "AWS::Region": "us-east-1", + "AWS::AccountId": "123456789012", + } + resource_dict = { + "TemplateURL": child_path, + "Parameters": {}, + } + + captured = {} + + def capture_template_dict(*args, **kwargs): + captured["template_dict"] = kwargs.get("template_dict") + inner = Mock() + inner.export.return_value = {"Resources": {}} + return inner + + with patch( + "samcli.lib.package.artifact_exporter.Template", + side_effect=capture_template_dict, + ): + stack_resource.do_export("ChildStack", resource_dict, parent_dir) + + expanded = captured.get("template_dict") or {} + resources = expanded.get("Resources", {}) + # Default "bar_default" should have been substituted into the + # ForEach loop, producing a "bar_defaultTopic" resource. + self.assertIn( + "bar_defaultTopic", + resources, + "Child Default did not resolve via resolver fallback. " "Expanded Resources: %r" % (resources,), + ) + + +class TestCloudFormationStackResourceBuriedAWSInclude(unittest.TestCase): + """Nested-stack child template containing AWS::Include buried inside an + LE function (e.g. Fn::ToJsonString) must have the include's Location + rewritten by the pre-LE global-transform pass in + CloudFormationStackResource.do_export, before LE expansion would have + collapsed the structural Fn::Transform into a JSON-string literal. + + Regression coverage for https://github.com/aws/aws-sam-cli/issues/9027. + Root-flow coverage lives in tests/unit/commands/package/ + test_package_context.py. + """ + + def test_buried_aws_include_in_le_child_is_rewritten(self): + uploaders_mock = Mock() + s3_uploader_mock = Mock() + s3_uploader_mock.upload_with_dedup.side_effect = ( + lambda local_path, **_: f"s3://bucket/{os.path.basename(local_path)}-md5" + ) + # Template upload (the child template itself) — give it a deterministic URL. + s3_uploader_mock.upload.return_value = "s3://bucket/child-template-md5" + s3_uploader_mock.to_path_style_s3_url.return_value = "https://s3.amazonaws.com/bucket/child-template-md5" + uploaders_mock.get.return_value = s3_uploader_mock + + code_signer_mock = Mock() + code_signer_mock.should_sign_package.return_value = False + + stack_resource = CloudFormationStackResource(uploaders_mock, code_signer_mock) + stack_resource.language_extensions_enabled = True + + # Fixture mirrors the issue #9027 reporter's shape: Fn::ToJsonString + # over Fn::Transform: AWS::Include, buried inside + # AWS::SSM::Parameter.Properties.Value, under + # Transform: AWS::LanguageExtensions so the LE branch is taken. + parent_dir = str(TEST_DATA_PATH / "buried_aws_include") + child_path = str(TEST_DATA_PATH / "buried_aws_include" / "child.yaml") + include_path = str(TEST_DATA_PATH / "buried_aws_include" / "export-events.json") - shutil.rmtree(tmpdir, ignore_errors=True) + resource_dict = {"TemplateURL": child_path} + stack_resource.do_export("ChildStack", resource_dict, parent_dir) + + # Walk every upload_with_dedup call to confirm the include was uploaded. + uploaded_files = [call_args[0][0] for call_args in s3_uploader_mock.upload_with_dedup.call_args_list] + self.assertIn(include_path, uploaded_files) class TestCloudFormationStackResourceExpansionErrorHandling(unittest.TestCase): @@ -2771,64 +3181,43 @@ def _make_stack_resource(self): code_signer_mock.should_sign_package.return_value = False return CloudFormationStackResource(uploaders_mock, code_signer_mock) - def _write_child(self, tmpdir): - child = { - "AWSTemplateFormatVersion": "2010-09-09", - "Transform": "AWS::LanguageExtensions", - "Resources": {"Dummy": {"Type": "AWS::SNS::Topic", "Properties": {}}}, - } - from samcli.yamlhelper import yaml_dump - - path = os.path.join(tmpdir, "child.yaml") - with open(path, "w", encoding="utf-8") as f: - f.write(yaml_dump(child)) - return path - def test_invalid_sam_document_exception_logs_warning_and_falls_back(self): stack_resource = self._make_stack_resource() - tmpdir = tempfile.mkdtemp() - try: - child_path = self._write_child(tmpdir) - with ( - patch("samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions") as expand_mock, - patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock, - self.assertLogs("samcli.lib.package.artifact_exporter", level="WARNING") as log_ctx, - ): - expand_mock.side_effect = InvalidSamDocumentException("bad template") - TemplateMock.return_value.export.return_value = {"Resources": {}} - stack_resource.do_export("ChildStack", {"TemplateURL": child_path}, tmpdir) - - # Warning emitted (not error) and fallback path taken. - self.assertTrue(any("Language extensions expansion failed" in m for m in log_ctx.output)) - TemplateMock.assert_called_once() # non-extension fallback constructor - finally: - import shutil + stack_resource.language_extensions_enabled = True + parent_dir = str(TEST_DATA_PATH / "child_minimal_le") + child_path = str(TEST_DATA_PATH / "child_minimal_le" / "child.yaml") + with ( + patch("samcli.lib.package.artifact_exporter.expand_language_extensions") as expand_mock, + patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock, + self.assertLogs("samcli.lib.package.artifact_exporter", level="WARNING") as log_ctx, + ): + expand_mock.side_effect = InvalidSamDocumentException("bad template") + TemplateMock.return_value.export.return_value = {"Resources": {}} + stack_resource.do_export("ChildStack", {"TemplateURL": child_path}, parent_dir) - shutil.rmtree(tmpdir, ignore_errors=True) + # Warning emitted (not error) and fallback path taken. + self.assertTrue(any("Language extensions expansion failed" in m for m in log_ctx.output)) + TemplateMock.assert_called_once() # non-extension fallback constructor def test_unexpected_exception_logs_error_and_falls_back(self): stack_resource = self._make_stack_resource() - tmpdir = tempfile.mkdtemp() - try: - child_path = self._write_child(tmpdir) - with ( - patch("samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions") as expand_mock, - patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock, - self.assertLogs("samcli.lib.package.artifact_exporter", level="ERROR") as log_ctx, - ): - expand_mock.side_effect = AttributeError("unexpected bug") - TemplateMock.return_value.export.return_value = {"Resources": {}} - stack_resource.do_export("ChildStack", {"TemplateURL": child_path}, tmpdir) - - # ERROR-level log containing the bug-report pointer; fallback still taken. - joined = "\n".join(log_ctx.output) - self.assertIn("Internal error expanding language extensions", joined) - self.assertIn("github.com/aws/aws-sam-cli/issues", joined) - TemplateMock.assert_called_once() - finally: - import shutil + stack_resource.language_extensions_enabled = True + parent_dir = str(TEST_DATA_PATH / "child_minimal_le") + child_path = str(TEST_DATA_PATH / "child_minimal_le" / "child.yaml") + with ( + patch("samcli.lib.package.artifact_exporter.expand_language_extensions") as expand_mock, + patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock, + self.assertLogs("samcli.lib.package.artifact_exporter", level="ERROR") as log_ctx, + ): + expand_mock.side_effect = AttributeError("unexpected bug") + TemplateMock.return_value.export.return_value = {"Resources": {}} + stack_resource.do_export("ChildStack", {"TemplateURL": child_path}, parent_dir) - shutil.rmtree(tmpdir, ignore_errors=True) + # ERROR-level log containing the bug-report pointer; fallback still taken. + joined = "\n".join(log_ctx.output) + self.assertIn("Internal error expanding language extensions", joined) + self.assertIn("github.com/aws/aws-sam-cli/issues", joined) + TemplateMock.assert_called_once() class TestTemplateLanguageExtensionsKwarg(unittest.TestCase): @@ -2865,3 +3254,66 @@ def test_default_is_false(self): template_dict={"Resources": {}}, ) self.assertFalse(template.language_extensions_enabled) + + +class TestDoExportLanguageExtensionsStructuralGate(unittest.TestCase): + """Structural-gate guarantees for CloudFormationStackResource.do_export(). + + When language_extensions_enabled is False (the default), do_export must not + invoke any LE machinery: no expand_language_extensions call, no pre-LE + AWS::Include pass, no pseudo-param plumbing. The flag is a hard correctness + boundary, not a passthrough. + + Patch targets are use-site (samcli.lib.package.artifact_exporter.*) per the + lesson from PR #9030's 45dea4b7b — hoisting an import freezes the use-site + binding, so source-module patches stop intercepting and tests fail only + under CI's deterministic collection order. + """ + + def _make_stack_resource(self): + uploaders_mock = Mock() + uploaders_mock.get.return_value = Mock() + uploaders_mock.get.return_value.upload.return_value = "s3://bucket/key" + uploaders_mock.get.return_value.to_path_style_s3_url.return_value = "http://s3.amazonaws.com/bucket/key" + code_signer_mock = Mock() + code_signer_mock.should_sign_package.return_value = False + return CloudFormationStackResource(uploaders_mock, code_signer_mock) + + def test_off_path_does_not_invoke_expand_language_extensions(self): + """LE-off: dispatcher must NOT call expand_language_extensions.""" + stack_resource = self._make_stack_resource() + # Default state: language_extensions_enabled is False. + self.assertFalse(stack_resource.language_extensions_enabled) + + parent_dir = str(TEST_DATA_PATH / "child_minimal_no_le") + child_path = str(TEST_DATA_PATH / "child_minimal_no_le" / "child.yaml") + with ( + patch("samcli.lib.package.artifact_exporter.expand_language_extensions") as mock_expand, + patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock, + ): + TemplateMock.return_value.export.return_value = {"Resources": {}} + stack_resource.do_export("ChildStack", {"TemplateURL": child_path}, parent_dir) + + mock_expand.assert_not_called() + + def test_off_path_does_not_invoke_pre_le_global_transform_pass(self): + """LE-off: dispatcher / off branch must NOT call _export_global_artifacts_pass. + + Note: Template.export() runs its own internal _export_global_artifacts_pass, + but here Template is patched, so any call to the patched + _export_global_artifacts_pass would be from the do_export call site + (which we want to NOT happen on the off path). + """ + stack_resource = self._make_stack_resource() + self.assertFalse(stack_resource.language_extensions_enabled) + + parent_dir = str(TEST_DATA_PATH / "child_minimal_no_le") + child_path = str(TEST_DATA_PATH / "child_minimal_no_le" / "child.yaml") + with ( + patch("samcli.lib.package.artifact_exporter._export_global_artifacts_pass") as mock_pre_pass, + patch("samcli.lib.package.artifact_exporter.Template") as TemplateMock, + ): + TemplateMock.return_value.export.return_value = {"Resources": {}} + stack_resource.do_export("ChildStack", {"TemplateURL": child_path}, parent_dir) + + mock_pre_pass.assert_not_called() diff --git a/tests/unit/lib/package/test_data/buried_aws_include/child.yaml b/tests/unit/lib/package/test_data/buried_aws_include/child.yaml new file mode 100644 index 00000000000..15baadfa922 --- /dev/null +++ b/tests/unit/lib/package/test_data/buried_aws_include/child.yaml @@ -0,0 +1,14 @@ +AWSTemplateFormatVersion: '2010-09-09' +Transform: AWS::LanguageExtensions +Resources: + Parameter: + Type: AWS::SSM::Parameter + Properties: + Type: String + DataType: text + Value: + Fn::ToJsonString: + Fn::Transform: + Name: AWS::Include + Parameters: + Location: export-events.json diff --git a/tests/unit/lib/package/test_data/buried_aws_include/export-events.json b/tests/unit/lib/package/test_data/buried_aws_include/export-events.json new file mode 100644 index 00000000000..2eb28cebd55 --- /dev/null +++ b/tests/unit/lib/package/test_data/buried_aws_include/export-events.json @@ -0,0 +1 @@ +[{"event": "demo"}] diff --git a/tests/unit/lib/package/test_data/child_bar_default/child.yaml b/tests/unit/lib/package/test_data/child_bar_default/child.yaml new file mode 100644 index 00000000000..41d293bfd16 --- /dev/null +++ b/tests/unit/lib/package/test_data/child_bar_default/child.yaml @@ -0,0 +1,13 @@ +AWSTemplateFormatVersion: '2010-09-09' +Transform: AWS::LanguageExtensions +Parameters: + Bar: + Type: CommaDelimitedList + Default: bar_default +Resources: + Fn::ForEach::Vals: + - Item + - Ref: Bar + - ${Item}Topic: + Type: AWS::SNS::Topic + Properties: {} diff --git a/tests/unit/lib/package/test_data/child_foo_param/child.yaml b/tests/unit/lib/package/test_data/child_foo_param/child.yaml new file mode 100644 index 00000000000..1a8072857f9 --- /dev/null +++ b/tests/unit/lib/package/test_data/child_foo_param/child.yaml @@ -0,0 +1,6 @@ +AWSTemplateFormatVersion: '2010-09-09' +Transform: AWS::LanguageExtensions +Parameters: + Foo: + Type: String +Resources: {} diff --git a/tests/unit/lib/package/test_data/child_minimal_le/child.yaml b/tests/unit/lib/package/test_data/child_minimal_le/child.yaml new file mode 100644 index 00000000000..ba8410e2845 --- /dev/null +++ b/tests/unit/lib/package/test_data/child_minimal_le/child.yaml @@ -0,0 +1,6 @@ +AWSTemplateFormatVersion: '2010-09-09' +Transform: AWS::LanguageExtensions +Resources: + Dummy: + Type: AWS::SNS::Topic + Properties: {} diff --git a/tests/unit/lib/package/test_data/child_minimal_no_le/child.yaml b/tests/unit/lib/package/test_data/child_minimal_no_le/child.yaml new file mode 100644 index 00000000000..ccdf30b80eb --- /dev/null +++ b/tests/unit/lib/package/test_data/child_minimal_no_le/child.yaml @@ -0,0 +1 @@ +Resources: {} diff --git a/tests/unit/lib/package/test_data/child_servicenames/child.yaml b/tests/unit/lib/package/test_data/child_servicenames/child.yaml new file mode 100644 index 00000000000..19f3d153707 --- /dev/null +++ b/tests/unit/lib/package/test_data/child_servicenames/child.yaml @@ -0,0 +1,12 @@ +AWSTemplateFormatVersion: '2010-09-09' +Transform: AWS::LanguageExtensions +Parameters: + ServiceNames: + Type: CommaDelimitedList +Resources: + Fn::ForEach::Services: + - Name + - Ref: ServiceNames + - ${Name}Topic: + Type: AWS::SNS::Topic + Properties: {} diff --git a/tests/unit/lib/package/test_data/le_sar_metadata/LICENSE.txt b/tests/unit/lib/package/test_data/le_sar_metadata/LICENSE.txt new file mode 100644 index 00000000000..a22a2da24d1 --- /dev/null +++ b/tests/unit/lib/package/test_data/le_sar_metadata/LICENSE.txt @@ -0,0 +1 @@ +MIT diff --git a/tests/unit/lib/package/test_data/le_sar_metadata/README.md b/tests/unit/lib/package/test_data/le_sar_metadata/README.md new file mode 100644 index 00000000000..85d89a44082 --- /dev/null +++ b/tests/unit/lib/package/test_data/le_sar_metadata/README.md @@ -0,0 +1 @@ +# MyApp diff --git a/tests/unit/lib/package/test_data/le_sar_metadata/code/index.py b/tests/unit/lib/package/test_data/le_sar_metadata/code/index.py new file mode 100644 index 00000000000..5ff82cf6f4a --- /dev/null +++ b/tests/unit/lib/package/test_data/le_sar_metadata/code/index.py @@ -0,0 +1,2 @@ +def handler(event, context): + return event diff --git a/tests/unit/lib/package/test_data/le_top_level_include/code/index.py b/tests/unit/lib/package/test_data/le_top_level_include/code/index.py new file mode 100644 index 00000000000..5ff82cf6f4a --- /dev/null +++ b/tests/unit/lib/package/test_data/le_top_level_include/code/index.py @@ -0,0 +1,2 @@ +def handler(event, context): + return event diff --git a/tests/unit/lib/package/test_data/le_top_level_include/extra.yaml b/tests/unit/lib/package/test_data/le_top_level_include/extra.yaml new file mode 100644 index 00000000000..9f11beba7ed --- /dev/null +++ b/tests/unit/lib/package/test_data/le_top_level_include/extra.yaml @@ -0,0 +1 @@ +# stub include target diff --git a/tests/unit/lib/package/test_language_extensions_packaging.py b/tests/unit/lib/package/test_language_extensions_packaging.py new file mode 100644 index 00000000000..7f01fc31518 --- /dev/null +++ b/tests/unit/lib/package/test_language_extensions_packaging.py @@ -0,0 +1,104 @@ +"""Unit tests for samcli.lib.package.language_extensions_packaging. + +Resource-property merge tests live in test_artifact_exporter.py; this file +focuses on the Metadata merge pass added for the registry-driven merge. +""" + +from unittest import TestCase + +from samcli.lib.package.language_extensions_packaging import merge_language_extensions_s3_uris + + +class TestMergeMetadata(TestCase): + def test_serverless_repo_license_url_is_merged(self): + original = { + "Transform": "AWS::LanguageExtensions", + "Metadata": { + "AWS::ServerlessRepo::Application": { + "Name": "MyApp", + "LicenseUrl": "./LICENSE.txt", + "ReadmeUrl": "./README.md", + } + }, + "Resources": {}, + } + exported = { + "Metadata": { + "AWS::ServerlessRepo::Application": { + "Name": "MyApp", + "LicenseUrl": "s3://bucket/license-md5", + "ReadmeUrl": "s3://bucket/readme-md5", + } + }, + "Resources": {}, + } + + result = merge_language_extensions_s3_uris(original, exported) + + sar = result["Metadata"]["AWS::ServerlessRepo::Application"] + self.assertEqual(sar["LicenseUrl"], "s3://bucket/license-md5") + self.assertEqual(sar["ReadmeUrl"], "s3://bucket/readme-md5") + self.assertEqual(sar["Name"], "MyApp") # unrelated keys preserved + + def test_metadata_without_serverless_repo_is_unchanged(self): + original = { + "Metadata": {"OtherKey": {"Foo": "./bar"}}, + "Resources": {}, + } + exported = { + "Metadata": {"OtherKey": {"Foo": "./bar"}}, + "Resources": {}, + } + + result = merge_language_extensions_s3_uris(original, exported) + + self.assertEqual(result["Metadata"], {"OtherKey": {"Foo": "./bar"}}) + + def test_missing_metadata_section_in_either_template_is_safe(self): + # No Metadata in original + result = merge_language_extensions_s3_uris( + {"Resources": {}}, + {"Metadata": {"AWS::ServerlessRepo::Application": {"LicenseUrl": "s3://x"}}, "Resources": {}}, + ) + self.assertNotIn("Metadata", result) + + # No Metadata in exported + original = { + "Metadata": {"AWS::ServerlessRepo::Application": {"LicenseUrl": "./LICENSE"}}, + "Resources": {}, + } + result = merge_language_extensions_s3_uris(original, {"Resources": {}}) + # Original retained, since exporter never wrote anything + self.assertEqual(result["Metadata"]["AWS::ServerlessRepo::Application"]["LicenseUrl"], "./LICENSE") + + def test_partial_serverless_repo_export_preserves_unwritten_properties(self): + """If the exported template has only LicenseUrl written (no ReadmeUrl), + the original's ReadmeUrl must be left untouched. Guards the + `if prop_name in exported_entry:` check in _merge_metadata. + """ + original = { + "Metadata": { + "AWS::ServerlessRepo::Application": { + "Name": "MyApp", + "LicenseUrl": "./LICENSE.txt", + "ReadmeUrl": "./README.md", + } + }, + "Resources": {}, + } + exported = { + "Metadata": { + "AWS::ServerlessRepo::Application": { + "Name": "MyApp", + "LicenseUrl": "s3://bucket/license-md5", + # ReadmeUrl deliberately absent + } + }, + "Resources": {}, + } + + result = merge_language_extensions_s3_uris(original, exported) + + sar = result["Metadata"]["AWS::ServerlessRepo::Application"] + self.assertEqual(sar["LicenseUrl"], "s3://bucket/license-md5") + self.assertEqual(sar["ReadmeUrl"], "./README.md") # untouched From 3714cf81c73d1fd76c8d56814817cefa2d8eff65 Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Fri, 29 May 2026 10:20:15 -0700 Subject: [PATCH 09/24] refactor: Replace custom copytree() with stdlib shutil.copytree (#8934) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace hand-rolled copytree implementations with shutil.copytree(..., symlinks=True, dirs_exist_ok=True) in both osutils.py and copy_terraform_built_artifacts.py. The custom implementations were originally needed for Python 3.7 compatibility (dirs_exist_ok was added in 3.8), but SAM CLI now requires Python 3.10+, making them unnecessary. Using the stdlib version with symlinks=True correctly preserves both file and directory symlinks without following them. Changes: - osutils.copytree: replaced ~40-line manual implementation with single shutil.copytree call; removed unused errno import; replaced os.remove try/except with Path.unlink(missing_ok=True) in remove() - Terraform copy_terraform_built_artifacts.copytree: same replacement - Updated unit tests to match new implementation - Added integration-style tests verifying file and directory symlink preservation on disk - Audited os.walk(followlinks=True) in package/utils.py — intentional and correct for zip packaging (zip files need dereferenced content) This is a code quality / defense-in-depth improvement, not a security fix. We do not believe this warrants a new CVE assignment. --- .../copy_terraform_built_artifacts.py | 17 +-- samcli/lib/utils/osutils.py | 62 +++-------- tests/unit/lib/utils/test_osutils.py | 102 ++++++++++-------- 3 files changed, 74 insertions(+), 107 deletions(-) diff --git a/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py b/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py index add87f60eb2..b6f6062e066 100644 --- a/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py +++ b/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py @@ -195,20 +195,11 @@ def search(self, data): def copytree(src, dst): - """Modified copytree method - Note: before python3.8 there is no `dir_exists_ok` argument, therefore - this function explicitly creates one if it does not exist. + """Copy a directory tree from src to dst, merging into dst if it already exists. + + Symbolic links are preserved as symbolic links in the destination. """ - if not os.path.exists(dst): - os.makedirs(dst) - for item in os.listdir(src): - src_item = os.path.join(src, item) - dst_item = os.path.join(dst, item) - if os.path.isdir(src_item): - # recursively call itself. - copytree(src_item, dst_item) - else: - shutil.copy2(src_item, dst_item) + shutil.copytree(src, dst, symlinks=True, dirs_exist_ok=True) def cli_exit(): diff --git a/samcli/lib/utils/osutils.py b/samcli/lib/utils/osutils.py index 4887de2eea2..74cf67199b1 100644 --- a/samcli/lib/utils/osutils.py +++ b/samcli/lib/utils/osutils.py @@ -2,7 +2,6 @@ Common OS utilities """ -import errno import io import logging import os @@ -117,7 +116,7 @@ def stderr() -> io.TextIOWrapper: def remove(path): if path: try: - os.remove(path) + Path(path).unlink(missing_ok=True) except OSError: pass @@ -134,60 +133,27 @@ def tempfile_platform_independent(): remove(_tempfile.name) -# NOTE: Py3.8 or higher has a ``dir_exist_ok=True`` parameter to provide this functionality. -# This method can be removed if we stop supporting Py37 def copytree(source, destination, ignore=None): """ - Similar to shutil.copytree except that it removes the limitation that the destination directory should - be present. + Recursively copy a directory tree from source to destination, allowing the destination + to already exist. Symbolic links in the source tree are preserved as symbolic links + in the destination. + + Delegates to ``shutil.copytree`` with ``dirs_exist_ok=True`` and ``symlinks=True``. + :type source: str :param source: - Path to the source folder to copy + Path to the source directory to copy :type destination: str :param destination: - Path to destination folder - :type ignore: function + Path to the destination directory. Will be created if it does not exist; + existing files will be overwritten by corresponding files from source. + :type ignore: callable, optional :param ignore: - A function that returns a set of file names to ignore, given a list of available file names. Similar to the - ``ignore`` property of ``shutils.copytree`` method + A callable that receives the directory being visited and a list of its contents, + and returns a set of names to ignore. See ``shutil.ignore_patterns`` for a helper. """ - - if not os.path.exists(destination): - os.makedirs(destination) - - try: - # Let's try to copy the directory metadata from source to destination - shutil.copystat(source, destination) - except OSError as ex: - # Can't copy file access times in Windows - LOG.debug("Unable to copy file access times from %s to %s", source, destination, exc_info=ex) - - names = os.listdir(source) - if ignore is not None: - ignored_names = ignore(source, names) - else: - ignored_names = set() - - for name in names: - # Skip ignored names - if name in ignored_names: - continue - - new_source = os.path.join(source, name) - new_destination = os.path.join(destination, name) - - if os.path.isdir(new_source): - copytree(new_source, new_destination, ignore=ignore) - else: - try: - shutil.copy2(new_source, new_destination, follow_symlinks=False) - except OSError as e: - if e.errno != errno.EINVAL: - raise e - - # Symlinks do not get copied for Windows using shutil.copy2, which is why - # they are handled separately here. - create_symlink_or_copy(new_source, new_destination) + shutil.copytree(source, destination, symlinks=True, ignore=ignore, dirs_exist_ok=True) def convert_files_to_unix_line_endings(path: str, target_files: Optional[List[str]] = None) -> None: diff --git a/tests/unit/lib/utils/test_osutils.py b/tests/unit/lib/utils/test_osutils.py index 3d7fe3c47a3..7fdcb2d71ab 100644 --- a/tests/unit/lib/utils/test_osutils.py +++ b/tests/unit/lib/utils/test_osutils.py @@ -3,7 +3,9 @@ """ import os +import shutil import sys +import tempfile from unittest import TestCase from unittest.mock import patch, Mock @@ -91,66 +93,74 @@ def test_must_delete_if_path_exist(self, patched_rmtree, patched_path): class Test_copytree(TestCase): - @patch("samcli.lib.utils.osutils.Path") - @patch("samcli.lib.utils.osutils.os") - @patch("samcli.lib.utils.osutils.shutil.copy2") - def test_must_copytree(self, patched_copy2, patched_os, patched_path): + @patch("samcli.lib.utils.osutils.shutil.copytree") + def test_must_copytree(self, patched_copytree): source_path = "mock-source/path" destination_path = "mock-destination/path" - mock_path_obj = Mock() - patched_path.exists.return_value = True - patched_os.path.return_value = mock_path_obj - patched_os.path.join.side_effect = [source_path, destination_path] - patched_os.path.isdir.return_value = False - patched_os.listdir.return_value = ["mock-source-file1"] osutils.copytree(source_path, destination_path) - patched_os.path.join.assert_called() - patched_copy2.assert_called_with(source_path, destination_path, follow_symlinks=False) + patched_copytree.assert_called_once_with( + source_path, destination_path, symlinks=True, ignore=None, dirs_exist_ok=True + ) - @patch("samcli.lib.utils.osutils.Path") - @patch("samcli.lib.utils.osutils.os") - @patch("samcli.lib.utils.osutils.shutil.copy2") - def test_copytree_throws_oserror_path_exists(self, patched_copy2, patched_os, patched_path): + @patch("samcli.lib.utils.osutils.shutil.copytree") + def test_copytree_with_ignore(self, patched_copytree): source_path = "mock-source/path" destination_path = "mock-destination/path" - mock_path_obj = Mock() - patched_path.exists.return_value = True - patched_os.path.return_value = mock_path_obj - patched_copy2.side_effect = OSError("mock-os-error") + mock_ignore = Mock() + + osutils.copytree(source_path, destination_path, ignore=mock_ignore) + + patched_copytree.assert_called_once_with( + source_path, destination_path, symlinks=True, ignore=mock_ignore, dirs_exist_ok=True + ) + + @patch("samcli.lib.utils.osutils.shutil.copytree") + def test_copytree_throws_oserror(self, patched_copytree): + patched_copytree.side_effect = OSError("mock-os-error") - patched_os.path.join.side_effect = [source_path, destination_path] - patched_os.path.isdir.return_value = False - patched_os.listdir.return_value = ["mock-source-file1"] with self.assertRaises(OSError): - osutils.copytree(source_path, destination_path) + osutils.copytree("mock-source/path", "mock-destination/path") - patched_os.path.join.assert_called() - patched_copy2.assert_called_with(source_path, destination_path, follow_symlinks=False) - @patch("samcli.lib.utils.osutils.create_symlink_or_copy") - @patch("samcli.lib.utils.osutils.Path") - @patch("samcli.lib.utils.osutils.os") - @patch("samcli.lib.utils.osutils.shutil.copy2") - def test_copytree_symlink_copy_error_handling( - self, patched_copy2, patched_os, patched_path, patched_create_symlink_or_copy - ): - source_path = "mock-source/path" - destination_path = "mock-destination/path" - mock_path_obj = Mock() - patched_path.exists.return_value = True - patched_os.path.return_value = mock_path_obj - patched_copy2.side_effect = OSError(22, "mock-os-error") +class Test_copytree_symlinks(TestCase): + """Integration-style tests verifying symlinks=True preserves symlinks on disk.""" - patched_os.path.join.side_effect = [source_path, destination_path] - patched_os.path.isdir.return_value = False - patched_os.listdir.return_value = ["mock-source-file1"] - osutils.copytree(source_path, destination_path) + def setUp(self): + self.src = tempfile.mkdtemp() + self.dst = tempfile.mkdtemp() + # clean dst so copytree creates it + shutil.rmtree(self.dst) + + def tearDown(self): + shutil.rmtree(self.src, ignore_errors=True) + shutil.rmtree(self.dst, ignore_errors=True) + + def test_preserves_file_symlink(self): + real_file = os.path.join(self.src, "real.txt") + with open(real_file, "w") as f: + f.write("content") + os.symlink("real.txt", os.path.join(self.src, "link.txt")) + + osutils.copytree(self.src, self.dst) + + link = os.path.join(self.dst, "link.txt") + self.assertTrue(os.path.islink(link)) + self.assertEqual(os.readlink(link), "real.txt") + + def test_preserves_directory_symlink(self): + subdir = os.path.join(self.src, "subdir") + os.makedirs(subdir) + with open(os.path.join(subdir, "file.txt"), "w") as f: + f.write("content") + os.symlink("subdir", os.path.join(self.src, "link_dir")) + + osutils.copytree(self.src, self.dst) - patched_os.path.join.assert_called() - patched_copy2.assert_called_with(source_path, destination_path, follow_symlinks=False) - patched_create_symlink_or_copy.assert_called_with(source_path, destination_path) + link = os.path.join(self.dst, "link_dir") + self.assertTrue(os.path.islink(link)) + self.assertEqual(os.readlink(link), "subdir") class Test_create_symlink_or_copy(TestCase): From e2044fea7e5d030f29cad874d5fcfd3b10a7a75c Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Fri, 29 May 2026 10:44:02 -0700 Subject: [PATCH 10/24] chore: bump version to 1.161.1 (#9051) --- samcli/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/__init__.py b/samcli/__init__.py index 8280958891b..caa430809d9 100644 --- a/samcli/__init__.py +++ b/samcli/__init__.py @@ -2,4 +2,4 @@ SAM CLI version """ -__version__ = "1.161.0" +__version__ = "1.161.1" From 65c12d1ed7a55c7172766b462f1fbc5541b52c9b Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Mon, 1 Jun 2026 16:02:37 -0700 Subject: [PATCH 11/24] ci: unpin durable functions emulator image tag (#9054) --- .github/workflows/build.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bfae5e813fe..ccc2279c906 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -197,7 +197,6 @@ jobs: run: pytest -vv ${{ matrix.tests_config.params }} env: FORCE_RUN_DOCKER_TEST: ${{ matrix.tests_config.name == 'durable-functions' && '1' || '' }} - DURABLE_EXECUTIONS_EMULATOR_IMAGE_TAG: ${{ matrix.tests_config.name == 'durable-functions' && 'v1.1.1' || '' }} smoke-and-functional-tests: name: ${{ matrix.tests_config.name }} / ${{ matrix.tests_config.os }} / ${{ matrix.python }} From bb2a26fe9f38ccbb92b8d2d21c135fa4f06a7d46 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Fri, 15 May 2026 10:15:11 +0300 Subject: [PATCH 12/24] feat: add ECS and AgentCore container build/package/deploy support Extends SAM CLI to build, package, and deploy container images for AWS::ECS::TaskDefinition and AWS::BedrockAgentCore::Runtime resources using the same Metadata convention as Lambda Image functions. Fixes #8933 --- samcli/lib/package/artifact_exporter.py | 1 + .../lib/sync/flows/ecs_container_sync_flow.py | 190 +++++++++++++----- .../test_build_cmd_container_image.py | 8 +- .../buildcmd/container_image/agent/Dockerfile | 4 +- .../test_container_build_integration.py | 43 +--- 5 files changed, 144 insertions(+), 102 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 02276b25d70..6968465bc5b 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -620,6 +620,7 @@ def export(self) -> Dict: exporter.parent_parameter_values = self.parameter_values exporter.resource_metadata = resource.get("Metadata") exporter.language_extensions_enabled = self.language_extensions_enabled + exporter.resource_metadata = resource.get("Metadata") exporter.export(full_path, resource_dict, self.template_dir) return self.template_dict diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py index 74663dacc9c..f73853f9b0a 100644 --- a/samcli/lib/sync/flows/ecs_container_sync_flow.py +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -21,15 +21,21 @@ LOG = logging.getLogger(__name__) -# Minimum number of parts expected when splitting an ECS service ARN by "/" +# ECS service ARN format: arn:aws:ecs:::service// +_ECS_SERVICE_ARN_PREFIX = "arn:aws:ecs:" +_ECS_SERVICE_ARN_SERVICE_SEGMENT = ":service/" + +# Minimum number of slash-separated parts after splitting a service ARN # e.g. arn:aws:ecs:region:account:service/cluster/name -> [..., "cluster", "name"] _ECS_SERVICE_ARN_PARTS = 3 class ECSContainerSyncFlow(SyncFlow): - """SyncFlow for ECS TaskDefinition and AgentCore container image resources. + """SyncFlow for ECS TaskDefinition container image resources. - Builds the container image, pushes to ECR, and triggers an ECS service update. + Builds the container image, pushes to ECR, registers a new TaskDefinition + revision with the updated image URI, then updates any associated ECS services + to that revision. """ _resource_identifier: str @@ -131,7 +137,6 @@ def sync(self) -> None: LOG.debug("%sSkipping sync. Image name is None.", self.log_prefix) return - # Get ECR repo from deploy context ecr_repo = self._deploy_context.image_repository if ( not ecr_repo @@ -142,68 +147,145 @@ def sync(self) -> None: if not ecr_repo: LOG.warning( - "%sNo ECR repository configured for %s. " "Use --image-repository or --image-repositories.", + "%sNo ECR repository configured for %s. Use --image-repository or --image-repositories.", self.log_prefix, self._resource_identifier, ) return - # Push image to ECR ecr_uploader = ECRUploader(self._get_docker_client(), self._get_ecr_client(), ecr_repo, None) - ecr_uploader.upload(self._image_name, self._resource_identifier) + image_uri = ecr_uploader.upload(self._image_name, self._resource_identifier) - # Force new deployment of ECS service if one is associated - self._force_ecs_deployment() + new_task_def_arn = self._register_updated_task_definition(image_uri) + if new_task_def_arn: + self._update_services_to_task_definition(new_task_def_arn) - def _force_ecs_deployment(self) -> None: - """Force new deployment for ECS services in the stack that use this task definition.""" + def _register_updated_task_definition(self, image_uri: str) -> Optional[str]: + """Describe the current task definition, swap the image URI, and register a new revision.""" physical_id = self._physical_id_mapping.get(self._resource_identifier) if not physical_id: - LOG.debug("%sNo physical ID found for %s, skipping ECS update", self.log_prefix, self._resource_identifier) - return + LOG.debug( + "%sNo physical ID for %s; cannot register new task definition revision.", + self.log_prefix, + self._resource_identifier, + ) + return None + + ecs_client = self._get_ecs_client() + try: + response = ecs_client.describe_task_definition(taskDefinition=physical_id, include=["TAGS"]) + except ClientError: + LOG.warning( + "%sFailed to describe task definition %s", self.log_prefix, physical_id, exc_info=True + ) + return None + + task_def = response.get("taskDefinition", {}) + tags = response.get("tags", []) + + # Fields that are output-only and must be stripped before re-registering + _READONLY_FIELDS = { + "taskDefinitionArn", + "revision", + "status", + "requiresAttributes", + "compatibilities", + "registeredAt", + "registeredBy", + } + register_input = {k: v for k, v in task_def.items() if k not in _READONLY_FIELDS} + + # Update the image in the matching container definition + container_name = self._get_container_name() + container_defs = register_input.get("containerDefinitions", []) + updated = False + for cd in container_defs: + if container_name is None or cd.get("name") == container_name: + cd["image"] = image_uri + updated = True + if container_name is not None: + break + + if not updated: + LOG.warning( + "%sContainerName '%s' not found in task definition; skipping registration.", + self.log_prefix, + container_name, + ) + return None + + if tags: + register_input["tags"] = tags try: - ecs_client = self._get_ecs_client() - - # Find ECS services in the same stack by looking at physical_id_mapping - # ECS Service physical IDs are ARNs like arn:aws:ecs:region:account:service/cluster/name - for resource_id, resource_physical_id in self._physical_id_mapping.items(): - if not resource_physical_id or "service/" not in str(resource_physical_id): - continue - # Check if this service uses our task definition - try: - # Extract cluster and service from the ARN - parts = resource_physical_id.rsplit("/", 2) - if len(parts) < _ECS_SERVICE_ARN_PARTS: - continue - cluster = parts[-2] - service_name = parts[-1] - - svc_response = ecs_client.describe_services(cluster=cluster, services=[service_name]) - my_family = physical_id.rsplit("/", 1)[-1].split(":", 1)[0] - for svc in svc_response.get("services", []): - svc_task_def = svc.get("taskDefinition", "") - svc_family = svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0] - if svc_family and svc_family == my_family: - ecs_client.update_service( - cluster=cluster, - service=service_name, - forceNewDeployment=True, - ) - LOG.info( - "%sForced new deployment for service %s", - self.log_prefix, - service_name, - ) - except ClientError: - LOG.warning( - "%sFailed to update service %s", - self.log_prefix, - resource_id, - exc_info=True, - ) + reg_response = ecs_client.register_task_definition(**register_input) except ClientError: - LOG.warning("%sFailed to force ECS deployment", self.log_prefix, exc_info=True) + LOG.warning("%sFailed to register new task definition revision", self.log_prefix, exc_info=True) + return None + + new_arn = reg_response.get("taskDefinition", {}).get("taskDefinitionArn") + LOG.info("%sRegistered new task definition revision: %s", self.log_prefix, new_arn) + return new_arn + + def _get_container_name(self) -> Optional[str]: + """Return ContainerName from resource metadata, if set.""" + if not self._stacks: + return None + provider = SamContainerServiceProvider(self._stacks) + service = provider.get(self._resource_identifier) + if service and isinstance(service.metadata, dict): + return service.metadata.get("ContainerName") + return None + + def _update_services_to_task_definition(self, task_def_arn: str) -> None: + """Update ECS services that reference this task definition family to the new revision.""" + physical_id = self._physical_id_mapping.get(self._resource_identifier) + if not physical_id: + return + + ecs_client = self._get_ecs_client() + my_family = physical_id.rsplit("/", 1)[-1].split(":", 1)[0] + + for resource_id, resource_physical_id in self._physical_id_mapping.items(): + if not resource_physical_id: + continue + # Only consider ECS service ARNs; skip everything else (incl. App Runner, VPC Lattice, etc.) + if not ( + str(resource_physical_id).startswith(_ECS_SERVICE_ARN_PREFIX) + and _ECS_SERVICE_ARN_SERVICE_SEGMENT in str(resource_physical_id) + ): + continue + + parts = resource_physical_id.rsplit("/", 2) + if len(parts) < _ECS_SERVICE_ARN_PARTS: + continue + cluster = parts[-2] + service_name = parts[-1] + + try: + svc_response = ecs_client.describe_services(cluster=cluster, services=[service_name]) + for svc in svc_response.get("services", []): + svc_task_def = svc.get("taskDefinition", "") + svc_family = svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0] + if svc_family and svc_family == my_family: + ecs_client.update_service( + cluster=cluster, + service=service_name, + taskDefinition=task_def_arn, + ) + LOG.info( + "%sUpdated service %s to task definition %s", + self.log_prefix, + service_name, + task_def_arn, + ) + except ClientError: + LOG.warning( + "%sFailed to update service %s", + self.log_prefix, + resource_id, + exc_info=True, + ) def gather_dependencies(self) -> List[SyncFlow]: return [] @@ -212,7 +294,7 @@ def _get_resource_api_calls(self) -> List[ResourceAPICall]: return [ ResourceAPICall( self._resource_identifier, - [ApiCallTypes.UPDATE_FUNCTION_CODE], + [ApiCallTypes.UPDATE_CONTAINER_IMAGE], ) ] diff --git a/tests/integration/buildcmd/test_build_cmd_container_image.py b/tests/integration/buildcmd/test_build_cmd_container_image.py index 56a2136f157..c34bd7cab90 100644 --- a/tests/integration/buildcmd/test_build_cmd_container_image.py +++ b/tests/integration/buildcmd/test_build_cmd_container_image.py @@ -1,20 +1,16 @@ """Integration test for ECS/AgentCore container image builds""" +import os import shutil import tempfile from pathlib import Path -from unittest import TestCase, skipIf +from unittest import TestCase import yaml from samcli.commands.build.build_context import BuildContext -from tests.testing_utils import CI_OVERRIDE, IS_WINDOWS, RUNNING_ON_CI -@skipIf( - (IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE, - "Skip container image build tests on Windows CI (no Linux Docker support)", -) class TestContainerImageBuild(TestCase): """Test that samdev build correctly handles ECS and AgentCore container resources.""" diff --git a/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile b/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile index 70947c4a8ab..57f45c64404 100644 --- a/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile +++ b/tests/integration/testdata/buildcmd/container_image/agent/Dockerfile @@ -1,4 +1,4 @@ -FROM public.ecr.aws/docker/library/alpine:3.19 +FROM python:3.12-slim WORKDIR /app RUN echo "hello" > /app/test.txt -CMD ["echo", "test"] +CMD ["python", "-c", "print('test')"] diff --git a/tests/unit/lib/build_module/test_container_build_integration.py b/tests/unit/lib/build_module/test_container_build_integration.py index b86c3857106..fcc800c3a89 100644 --- a/tests/unit/lib/build_module/test_container_build_integration.py +++ b/tests/unit/lib/build_module/test_container_build_integration.py @@ -1,7 +1,8 @@ """Tests for ECS/AgentCore container build integration across modules""" from unittest import TestCase -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch, Mock +from copy import deepcopy from samcli.lib.build.app_builder import ApplicationBuilder from samcli.lib.build.build_graph import ContainerBuildDefinition @@ -183,7 +184,7 @@ def test_includes_container_services( mock_manager.get_repository_mapping.return_value = {"MyFunction": "uri1", "MyAgent": "uri2"} mock_manager_cls.return_value = mock_manager - sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {}) + result = sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {}) # Verify both function and container service were passed call_args = mock_manager.set_functions.call_args[0] @@ -256,41 +257,3 @@ def test_sync_skips_when_no_image(self): flow._physical_id_mapping = {} # Should not raise flow.sync() - - -class TestContainerNameErrorHandling(TestCase): - def test_update_built_resource_raises_on_container_name_mismatch(self): - from samcli.lib.build.exceptions import DockerBuildFailed - - properties = { - "ContainerDefinitions": [ - {"Name": "sidecar", "Image": "sidecar:latest"}, - {"Name": "web", "Image": "placeholder"}, - ] - } - metadata = {"ContainerName": "typo"} - with self.assertRaises(DockerBuildFailed): - ApplicationBuilder._update_built_resource( - "myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path", metadata - ) - - def test_get_target_index_raises_on_container_name_mismatch(self): - from samcli.commands.package import exceptions - - exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource) - exporter.resource_metadata = {"ContainerName": "typo"} - container_defs = [{"Name": "web"}, {"Name": "sidecar"}] - with self.assertRaises(exceptions.ExportFailedError): - exporter._get_target_index(container_defs) - - def test_get_target_index_returns_match(self): - exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource) - exporter.resource_metadata = {"ContainerName": "web"} - container_defs = [{"Name": "sidecar"}, {"Name": "web"}] - self.assertEqual(exporter._get_target_index(container_defs), 1) - - def test_get_target_index_defaults_to_zero_without_name(self): - exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource) - exporter.resource_metadata = {} - container_defs = [{"Name": "web"}] - self.assertEqual(exporter._get_target_index(container_defs), 0) From 0ba929d96a6afcd417f8ff9d1948cb1ee26a83ab Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Tue, 19 May 2026 15:48:03 +0300 Subject: [PATCH 13/24] ci: retrigger CI (pyinstaller-linux infra flake) From 2915d438360d8e9f353c51bc1831f89104145b6d Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Tue, 19 May 2026 16:55:33 +0300 Subject: [PATCH 14/24] ci: retrigger CI (go setup infra flake) From 854065ba3b83e33b991c391df9a49a3f844954a5 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Sat, 6 Jun 2026 12:33:42 -0600 Subject: [PATCH 15/24] fix: address bnusunny PR review findings for ECS/AgentCore container sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - sam sync now captures the ECR URI returned by ECRUploader.upload and registers a new TaskDefinition revision with the updated image, then updates matching ECS services to that revision (fixes silent no-op) - Tighten _force_ecs_deployment → _update_services_to_task_definition to require both the arn:aws:ecs: prefix and :service/ segment, preventing false matches on App Runner / VPC Lattice / AppMesh ARNs - Split AgentCore out of ECSContainerSyncFlow: GENERATOR_MAPPING now routes AWS::BedrockAgentCore::Runtime to _create_agentcore_flow which logs a clear 'not yet supported' warning and returns None (UpdateAgentRuntime not yet implemented) - Add AWS::ECS::TaskDefinition to SyncCodeResources._accepted_resources so sam sync --code-resource AWS::ECS::TaskDefinition is accepted by Click - Add ApiCallTypes.UPDATE_CONTAINER_IMAGE enum value; use it in ECSContainerSyncFlow._get_resource_api_calls instead of UPDATE_FUNCTION_CODE - Fix multi-container TaskDefinition safety: raise DockerBuildFailed when >1 ContainerDefinitions and no Metadata.ContainerName (app_builder.py) - Fix AgentCore _update_built_resource: replace setdefault chain (which corrupts intrinsics) with direct dict assignment - Fix isinstance(metadata, dict) guards in _update_built_resource, _has_container_build_metadata, and ECSTaskDefinitionImageResource - Update ECSTaskDefinitionImageResource docstring and add LOG.warning for multi-container fallback (packageable_resources.py) - Remove dead BuildGraph.CONTAINER_BUILD_DEFINITIONS constant and get/put_container_build_definition methods (zero callers, not serialized in _read/_write) - Add test_ecs_container_sync_flow.py with 17 unit tests covering the new sync path, ARN filtering, error handling, and tag propagation Co-Authored-By: Claude Sonnet 4.6 --- samcli/lib/build/app_builder.py | 13 +- samcli/lib/build/build_graph.py | 17 - samcli/lib/package/packageable_resources.py | 15 +- .../lib/providers/sam_container_provider.py | 2 +- samcli/lib/sync/sync_flow.py | 1 + samcli/lib/sync/sync_flow_factory.py | 16 +- .../flows/test_ecs_container_sync_flow.py | 306 ++++++++++++++++++ tests/unit/lib/sync/test_sync_flow.py | 4 +- tests/unit/lib/sync/test_sync_flow_factory.py | 2 + 9 files changed, 348 insertions(+), 28 deletions(-) create mode 100644 tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index 158d6569c2a..bf8b94baae1 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -420,7 +420,7 @@ def _update_built_resource( if resource_type == AWS_ECS_TASK_DEFINITION: container_defs = resource_properties.get("ContainerDefinitions", []) if container_defs: - target_name = metadata.get("ContainerName") if metadata else None + target_name = metadata.get("ContainerName") if isinstance(metadata, dict) else None if target_name: for container_def in container_defs: if container_def.get("Name") == target_name: @@ -431,12 +431,17 @@ def _update_built_resource( f"Metadata.ContainerName '{target_name}' does not match any " f"container in ContainerDefinitions" ) + elif len(container_defs) > 1: + raise DockerBuildFailed( + f"TaskDefinition has {len(container_defs)} containers but Metadata.ContainerName is not set. " + f"Add 'ContainerName: ' to the resource Metadata to specify which container to build." + ) else: container_defs[0]["Image"] = path if resource_type == AWS_BEDROCK_AGENTCORE_RUNTIME: - artifact = resource_properties.setdefault("AgentRuntimeArtifact", {}) - container_config = artifact.setdefault("ContainerConfiguration", {}) - container_config["ContainerUri"] = path + resource_properties["AgentRuntimeArtifact"] = { + "ContainerConfiguration": {"ContainerUri": path} + } def _build_lambda_image(self, function_name: str, metadata: Dict, architecture: str) -> str: """ diff --git a/samcli/lib/build/build_graph.py b/samcli/lib/build/build_graph.py index e2199e79902..48625a184b3 100644 --- a/samcli/lib/build/build_graph.py +++ b/samcli/lib/build/build_graph.py @@ -200,14 +200,12 @@ class BuildGraph: # global table build definitions key FUNCTION_BUILD_DEFINITIONS = "function_build_definitions" LAYER_BUILD_DEFINITIONS = "layer_build_definitions" - CONTAINER_BUILD_DEFINITIONS = "container_build_definitions" def __init__(self, build_dir: str) -> None: # put build.toml file inside .aws-sam folder self._filepath = Path(build_dir).parent.joinpath(DEFAULT_BUILD_GRAPH_FILE_NAME) self._function_build_definitions: List["FunctionBuildDefinition"] = [] self._layer_build_definitions: List["LayerBuildDefinition"] = [] - self._container_build_definitions: List["ContainerBuildDefinition"] = [] self._atomic_read() def get_function_build_definitions(self) -> Tuple["FunctionBuildDefinition", ...]: @@ -216,21 +214,6 @@ def get_function_build_definitions(self) -> Tuple["FunctionBuildDefinition", ... def get_layer_build_definitions(self) -> Tuple["LayerBuildDefinition", ...]: return tuple(self._layer_build_definitions) - def get_container_build_definitions(self) -> Tuple["ContainerBuildDefinition", ...]: - return tuple(self._container_build_definitions) - - def put_container_build_definition(self, container_build_definition: "ContainerBuildDefinition") -> None: - """ - Puts the newly read container build definition into existing build graph. - Each container build definition is unique per resource identifier. - """ - if container_build_definition not in self._container_build_definitions: - LOG.debug( - "Adding container build definition: %s", - container_build_definition, - ) - self._container_build_definitions.append(container_build_definition) - def get_function_build_definition_with_full_path( self, function_full_path: str ) -> Optional["FunctionBuildDefinition"]: diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 5cee49d5c71..5c68d961d67 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -694,7 +694,8 @@ def export(self, resource_id: str, resource_dict: Optional[Dict], parent_dir: st class ECSTaskDefinitionImageResource(ResourceImage): """ Represents an ECS TaskDefinition with a container image that needs to be pushed to ECR. - The image reference is at ContainerDefinitions[0].Image. + For single-container TaskDefinitions the first container is used by default; for + multi-container TaskDefinitions Metadata.ContainerName must be set. """ RESOURCE_TYPE = AWS_ECS_TASK_DEFINITION @@ -703,8 +704,8 @@ class ECSTaskDefinitionImageResource(ResourceImage): def _get_target_index(self, container_defs): """Find the target container index using ContainerName from Metadata.""" - metadata = getattr(self, "resource_metadata", None) or {} - target_name = metadata.get("ContainerName") + metadata = getattr(self, "resource_metadata", None) + target_name = metadata.get("ContainerName") if isinstance(metadata, dict) else None if target_name: for i, cd in enumerate(container_defs): if cd.get("Name") == target_name: @@ -714,9 +715,15 @@ def _get_target_index(self, container_defs): property_name=self.PROPERTY_NAME, property_value=target_name, ex=ValueError( - f"Metadata.ContainerName '{target_name}' does not match any " f"container in ContainerDefinitions" + f"Metadata.ContainerName '{target_name}' does not match any container in ContainerDefinitions" ), ) + if len(container_defs) > 1: + LOG.warning( + "TaskDefinition has multiple containers but Metadata.ContainerName is not set; " + "packaging the first container. Add 'ContainerName: ' to the resource Metadata " + "to avoid ambiguity." + ) return 0 def export(self, resource_id, resource_dict, parent_dir): diff --git a/samcli/lib/providers/sam_container_provider.py b/samcli/lib/providers/sam_container_provider.py index db9689a4104..70e576d6484 100644 --- a/samcli/lib/providers/sam_container_provider.py +++ b/samcli/lib/providers/sam_container_provider.py @@ -81,7 +81,7 @@ def _extract_container_services(self) -> None: @staticmethod def _has_container_build_metadata(metadata: Optional[Dict]) -> bool: """Check if metadata contains the required fields for a container image build.""" - if not metadata: + if not isinstance(metadata, dict): return False return bool(metadata.get("Dockerfile") and metadata.get("DockerContext")) diff --git a/samcli/lib/sync/sync_flow.py b/samcli/lib/sync/sync_flow.py index 1f5fec38ade..a64cdee8725 100644 --- a/samcli/lib/sync/sync_flow.py +++ b/samcli/lib/sync/sync_flow.py @@ -43,6 +43,7 @@ class ApiCallTypes(Enum): UPDATE_FUNCTION_CONFIGURATION = "UpdateFunctionConfiguration" UPDATE_FUNCTION_CODE = "UpdateFunctionCode" PUBLISH_VERSION = "PublishVersion" + UPDATE_CONTAINER_IMAGE = "UpdateContainerImage" class ResourceAPICall(NamedTuple): diff --git a/samcli/lib/sync/sync_flow_factory.py b/samcli/lib/sync/sync_flow_factory.py index 0fb33e22cd8..4a20829e8e5 100644 --- a/samcli/lib/sync/sync_flow_factory.py +++ b/samcli/lib/sync/sync_flow_factory.py @@ -75,6 +75,7 @@ class SyncCodeResources: AWS_APIGATEWAY_V2_API, AWS_SERVERLESS_STATEMACHINE, AWS_STEPFUNCTIONS_STATEMACHINE, + AWS_ECS_TASK_DEFINITION, ] @classmethod @@ -356,6 +357,19 @@ def _create_ecs_container_flow( application_build_result, ) + def _create_agentcore_flow( + self, + resource_identifier: ResourceIdentifier, + application_build_result: Optional[ApplicationBuildResult], + ) -> Optional[SyncFlow]: + # AgentCore sync requires UpdateAgentRuntime, which is not yet implemented. + LOG.warning( + "sam sync is not yet supported for AWS::BedrockAgentCore::Runtime resource '%s'. " + "Use sam deploy to update AgentCore resources.", + str(resource_identifier), + ) + return None + GeneratorFunction = Callable[ ["SyncFlowFactory", ResourceIdentifier, Optional[ApplicationBuildResult]], Optional[SyncFlow] ] @@ -371,7 +385,7 @@ def _create_ecs_container_flow( AWS_SERVERLESS_STATEMACHINE: _create_stepfunctions_flow, AWS_STEPFUNCTIONS_STATEMACHINE: _create_stepfunctions_flow, AWS_ECS_TASK_DEFINITION: _create_ecs_container_flow, - AWS_BEDROCK_AGENTCORE_RUNTIME: _create_ecs_container_flow, + AWS_BEDROCK_AGENTCORE_RUNTIME: _create_agentcore_flow, } # SyncFlow mapping between resource type and creation function diff --git a/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py b/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py new file mode 100644 index 00000000000..9858af1fbcf --- /dev/null +++ b/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py @@ -0,0 +1,306 @@ +from unittest import TestCase +from unittest.mock import MagicMock, patch, call + +from botocore.exceptions import ClientError + +from samcli.lib.sync.flows.ecs_container_sync_flow import ECSContainerSyncFlow +from samcli.lib.sync.sync_flow import ApiCallTypes + + +def _make_client_error(code="SomeError"): + err = ClientError({"Error": {"Code": code, "Message": "msg"}}, "op") + return err + + +def _make_flow(image_repository="123456789012.dkr.ecr.us-east-1.amazonaws.com/myrepo", stacks=None): + build_context = MagicMock() + deploy_context = MagicMock() + deploy_context.image_repository = image_repository + deploy_context.image_repositories = None + flow = ECSContainerSyncFlow( + "MyTask", + build_context=build_context, + deploy_context=deploy_context, + sync_context=MagicMock(), + physical_id_mapping={"MyTask": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5"}, + stacks=stacks or [], + application_build_result=None, + ) + flow._image_name = "myapp:latest" + return flow + + +class TestECSContainerSyncFlowApiCallTypes(TestCase): + def test_get_resource_api_calls_uses_update_container_image(self): + flow = _make_flow() + calls = flow._get_resource_api_calls() + self.assertEqual(1, len(calls)) + self.assertIn(ApiCallTypes.UPDATE_CONTAINER_IMAGE, calls[0].api_calls) + + def test_equality_keys(self): + flow = _make_flow() + self.assertEqual("MyTask", flow._equality_keys()) + + def test_sync_state_identifier(self): + flow = _make_flow() + self.assertEqual("MyTask", flow.sync_state_identifier) + + +class TestECSContainerSyncFlowSync(TestCase): + def _make_ecs_client(self, container_name="app", image_uri="old-image:1"): + ecs_client = MagicMock() + task_def = { + "taskDefinitionArn": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5", + "family": "mytask", + "revision": 5, + "status": "ACTIVE", + "containerDefinitions": [{"name": container_name, "image": image_uri}], + "requiresAttributes": [], + "compatibilities": ["EC2"], + } + ecs_client.describe_task_definition.return_value = {"taskDefinition": task_def, "tags": []} + ecs_client.register_task_definition.return_value = { + "taskDefinition": {"taskDefinitionArn": "arn:aws:ecs:us-east-1:123:task-definition/mytask:6"} + } + ecs_client.describe_services.return_value = { + "services": [{"taskDefinition": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5"}] + } + return ecs_client + + def test_sync_registers_new_task_definition_and_updates_service(self): + flow = _make_flow() + # Include a service ARN so _update_services_to_task_definition can match it + flow._physical_id_mapping = { + "MyTask": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5", + "MyService": "arn:aws:ecs:us-east-1:123:service/mycluster/mysvc", + } + ecr_client = MagicMock() + docker_client = MagicMock() + ecs_client = self._make_ecs_client() + + flow._ecr_client = ecr_client + flow._docker_client = docker_client + flow._ecs_client = ecs_client + + with patch( + "samcli.lib.sync.flows.ecs_container_sync_flow.ECRUploader" + ) as MockUploader, patch.object(flow, "_get_container_name", return_value=None): + mock_uploader_instance = MagicMock() + mock_uploader_instance.upload.return_value = ( + "123456789012.dkr.ecr.us-east-1.amazonaws.com/myrepo:newtag" + ) + MockUploader.return_value = mock_uploader_instance + + flow.sync() + + ecs_client.describe_task_definition.assert_called_once_with( + taskDefinition="arn:aws:ecs:us-east-1:123:task-definition/mytask:5", include=["TAGS"] + ) + # register_task_definition should be called with updated image + reg_call_kwargs = ecs_client.register_task_definition.call_args[1] + self.assertEqual( + "123456789012.dkr.ecr.us-east-1.amazonaws.com/myrepo:newtag", + reg_call_kwargs["containerDefinitions"][0]["image"], + ) + # update_service should target the new revision + ecs_client.update_service.assert_called_once() + update_kwargs = ecs_client.update_service.call_args[1] + self.assertEqual("arn:aws:ecs:us-east-1:123:task-definition/mytask:6", update_kwargs["taskDefinition"]) + + def test_sync_skips_when_no_image_name(self): + flow = _make_flow() + flow._image_name = None + ecs_client = MagicMock() + flow._ecs_client = ecs_client + + flow.sync() + + ecs_client.describe_task_definition.assert_not_called() + + def test_sync_skips_when_no_ecr_repo(self): + flow = _make_flow(image_repository=None) + flow._image_name = "myapp:latest" + ecs_client = MagicMock() + flow._ecs_client = ecs_client + + flow.sync() + + ecs_client.describe_task_definition.assert_not_called() + + def test_register_returns_none_when_describe_fails(self): + flow = _make_flow() + ecs_client = MagicMock() + ecs_client.describe_task_definition.side_effect = _make_client_error("AccessDenied") + flow._ecs_client = ecs_client + + result = flow._register_updated_task_definition("new-image:uri") + self.assertIsNone(result) + + def test_register_returns_none_when_no_physical_id(self): + flow = _make_flow() + flow._physical_id_mapping = {} + result = flow._register_updated_task_definition("new-image:uri") + self.assertIsNone(result) + + def test_update_services_skips_non_ecs_arns(self): + flow = _make_flow() + flow._physical_id_mapping = { + "MyTask": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5", + # App Runner ARN — must NOT trigger describe_services + "AppRunnerSvc": "arn:aws:apprunner:us-east-1:123:service/mysvc/abc123", + } + ecs_client = MagicMock() + flow._ecs_client = ecs_client + + flow._update_services_to_task_definition("arn:aws:ecs:us-east-1:123:task-definition/mytask:6") + + ecs_client.describe_services.assert_not_called() + + def test_update_services_updates_matching_service(self): + flow = _make_flow() + flow._physical_id_mapping = { + "MyTask": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5", + "MyService": "arn:aws:ecs:us-east-1:123:service/mycluster/mysvc", + } + ecs_client = MagicMock() + ecs_client.describe_services.return_value = { + "services": [{"taskDefinition": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5"}] + } + flow._ecs_client = ecs_client + + new_arn = "arn:aws:ecs:us-east-1:123:task-definition/mytask:6" + flow._update_services_to_task_definition(new_arn) + + ecs_client.update_service.assert_called_once_with( + cluster="mycluster", service="mysvc", taskDefinition=new_arn + ) + + def test_update_services_skips_mismatched_family(self): + flow = _make_flow() + flow._physical_id_mapping = { + "MyTask": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5", + "OtherService": "arn:aws:ecs:us-east-1:123:service/mycluster/othersvc", + } + ecs_client = MagicMock() + ecs_client.describe_services.return_value = { + "services": [{"taskDefinition": "arn:aws:ecs:us-east-1:123:task-definition/othertask:3"}] + } + flow._ecs_client = ecs_client + + flow._update_services_to_task_definition("arn:aws:ecs:us-east-1:123:task-definition/mytask:6") + + ecs_client.update_service.assert_not_called() + + def test_update_services_handles_describe_client_error(self): + flow = _make_flow() + flow._physical_id_mapping = { + "MyTask": "arn:aws:ecs:us-east-1:123:task-definition/mytask:5", + "MyService": "arn:aws:ecs:us-east-1:123:service/mycluster/mysvc", + } + ecs_client = MagicMock() + ecs_client.describe_services.side_effect = _make_client_error("AccessDenied") + flow._ecs_client = ecs_client + + # Should not raise + flow._update_services_to_task_definition("arn:aws:ecs:us-east-1:123:task-definition/mytask:6") + ecs_client.update_service.assert_not_called() + + def test_register_strips_readonly_fields(self): + flow = _make_flow() + ecs_client = self._make_ecs_client() + flow._ecs_client = ecs_client + + with patch.object(flow, "_get_container_name", return_value=None): + flow._register_updated_task_definition("new-image:uri") + + reg_kwargs = ecs_client.register_task_definition.call_args[1] + for field in ("taskDefinitionArn", "revision", "status", "requiresAttributes", "compatibilities"): + self.assertNotIn(field, reg_kwargs, f"readonly field '{field}' should be stripped") + + def test_register_tags_included_when_present(self): + flow = _make_flow() + ecs_client = self._make_ecs_client() + ecs_client.describe_task_definition.return_value = { + "taskDefinition": { + "family": "mytask", + "containerDefinitions": [{"name": "app", "image": "old"}], + }, + "tags": [{"key": "env", "value": "prod"}], + } + flow._ecs_client = ecs_client + + with patch.object(flow, "_get_container_name", return_value=None): + flow._register_updated_task_definition("new-image:uri") + + reg_kwargs = ecs_client.register_task_definition.call_args[1] + self.assertEqual([{"key": "env", "value": "prod"}], reg_kwargs.get("tags")) + + def test_register_updates_named_container(self): + flow = _make_flow() + ecs_client = MagicMock() + ecs_client.describe_task_definition.return_value = { + "taskDefinition": { + "family": "mytask", + "containerDefinitions": [ + {"name": "sidecar", "image": "sidecar:old"}, + {"name": "app", "image": "app:old"}, + ], + }, + "tags": [], + } + ecs_client.register_task_definition.return_value = { + "taskDefinition": {"taskDefinitionArn": "arn:aws:ecs:us-east-1:123:task-definition/mytask:6"} + } + flow._ecs_client = ecs_client + + with patch.object(flow, "_get_container_name", return_value="app"): + flow._register_updated_task_definition("new-image:uri") + + reg_kwargs = ecs_client.register_task_definition.call_args[1] + containers = {cd["name"]: cd["image"] for cd in reg_kwargs["containerDefinitions"]} + self.assertEqual("new-image:uri", containers["app"]) + self.assertEqual("sidecar:old", containers["sidecar"]) + + def test_register_returns_none_when_named_container_missing(self): + flow = _make_flow() + ecs_client = MagicMock() + ecs_client.describe_task_definition.return_value = { + "taskDefinition": { + "family": "mytask", + "containerDefinitions": [{"name": "other", "image": "img"}], + }, + "tags": [], + } + flow._ecs_client = ecs_client + + with patch.object(flow, "_get_container_name", return_value="app"): + result = flow._register_updated_task_definition("new-image:uri") + + self.assertIsNone(result) + ecs_client.register_task_definition.assert_not_called() + + +class TestECSContainerSyncFlowGatherResources(TestCase): + def test_gather_resources_uses_prebuilt_artifacts(self): + build_result = MagicMock() + build_result.artifacts = {"MyTask": "prebuilt-image:tag"} + + docker_client = MagicMock() + img = MagicMock() + img.attrs = {"Id": "sha256:abc"} + docker_client.images.get.return_value = img + + flow = ECSContainerSyncFlow( + "MyTask", + build_context=MagicMock(), + deploy_context=MagicMock(), + sync_context=MagicMock(), + physical_id_mapping={}, + stacks=[], + application_build_result=build_result, + ) + flow._docker_client = docker_client + flow.gather_resources() + + self.assertEqual("prebuilt-image:tag", flow._image_name) + self.assertEqual("sha256:abc", flow._local_sha) diff --git a/tests/unit/lib/sync/test_sync_flow.py b/tests/unit/lib/sync/test_sync_flow.py index f4a4a6f85bc..2ea2ae207af 100644 --- a/tests/unit/lib/sync/test_sync_flow.py +++ b/tests/unit/lib/sync/test_sync_flow.py @@ -299,7 +299,7 @@ def test_compare_local(self, patched_session, patched_sync_state_identifier): def test_api_call_types(self): """Test cases for ApiCallTypes enum values""" # Test enum members are unique - self.assertEqual(len(set(ApiCallTypes)), 4) + self.assertEqual(len(set(ApiCallTypes)), 5) # Test enum members exist self.assertTrue(hasattr(ApiCallTypes, "BUILD")) @@ -310,3 +310,5 @@ def test_api_call_types(self): self.assertEqual(ApiCallTypes.UPDATE_FUNCTION_CODE.value, "UpdateFunctionCode") self.assertTrue(hasattr(ApiCallTypes, "PUBLISH_VERSION")) self.assertEqual(ApiCallTypes.PUBLISH_VERSION.value, "PublishVersion") + self.assertTrue(hasattr(ApiCallTypes, "UPDATE_CONTAINER_IMAGE")) + self.assertEqual(ApiCallTypes.UPDATE_CONTAINER_IMAGE.value, "UpdateContainerImage") diff --git a/tests/unit/lib/sync/test_sync_flow_factory.py b/tests/unit/lib/sync/test_sync_flow_factory.py index 92d30c8a09a..98e930be95e 100644 --- a/tests/unit/lib/sync/test_sync_flow_factory.py +++ b/tests/unit/lib/sync/test_sync_flow_factory.py @@ -19,6 +19,7 @@ AWS_APIGATEWAY_V2_API, AWS_SERVERLESS_STATEMACHINE, AWS_STEPFUNCTIONS_STATEMACHINE, + AWS_ECS_TASK_DEFINITION, ) @@ -275,5 +276,6 @@ def test_values(self): AWS_APIGATEWAY_V2_API, AWS_SERVERLESS_STATEMACHINE, AWS_STEPFUNCTIONS_STATEMACHINE, + AWS_ECS_TASK_DEFINITION, ] self.assertEqual(expected, output) From 99822c6c5eefde50a12e861cab46a5a96fb42540 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Sat, 6 Jun 2026 16:15:25 -0600 Subject: [PATCH 16/24] fix: raise ExportFailedError in _get_target_index for multi-container TaskDef Mirror the build path: when len(ContainerDefinitions) > 1 and Metadata.ContainerName is unset, sam package now raises ExportFailedError instead of silently packaging the first container. Prevents the wrong-container overwrite in standalone sam package invocations. Co-Authored-By: Claude Sonnet 4.6 --- samcli/lib/package/packageable_resources.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 5c68d961d67..2c82959be4f 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -719,10 +719,14 @@ def _get_target_index(self, container_defs): ), ) if len(container_defs) > 1: - LOG.warning( - "TaskDefinition has multiple containers but Metadata.ContainerName is not set; " - "packaging the first container. Add 'ContainerName: ' to the resource Metadata " - "to avoid ambiguity." + raise exceptions.ExportFailedError( + resource_id="", + property_name=self.PROPERTY_NAME, + property_value="", + ex=ValueError( + f"TaskDefinition has {len(container_defs)} containers but Metadata.ContainerName is not set. " + f"Add 'ContainerName: ' to the resource Metadata to specify which container to package." + ), ) return 0 From 79965b425a3e77b773d23961c8367ad5c86cadc1 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Sat, 6 Jun 2026 16:31:49 -0600 Subject: [PATCH 17/24] style: black formatting fixes for ecs_container_sync_flow and app_builder Co-Authored-By: Claude Sonnet 4.6 --- samcli/lib/build/app_builder.py | 4 +--- samcli/lib/sync/flows/ecs_container_sync_flow.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index bf8b94baae1..35d0ef1f50c 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -439,9 +439,7 @@ def _update_built_resource( else: container_defs[0]["Image"] = path if resource_type == AWS_BEDROCK_AGENTCORE_RUNTIME: - resource_properties["AgentRuntimeArtifact"] = { - "ContainerConfiguration": {"ContainerUri": path} - } + resource_properties["AgentRuntimeArtifact"] = {"ContainerConfiguration": {"ContainerUri": path}} def _build_lambda_image(self, function_name: str, metadata: Dict, architecture: str) -> str: """ diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py index f73853f9b0a..bff8bec03af 100644 --- a/samcli/lib/sync/flows/ecs_container_sync_flow.py +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -175,9 +175,7 @@ def _register_updated_task_definition(self, image_uri: str) -> Optional[str]: try: response = ecs_client.describe_task_definition(taskDefinition=physical_id, include=["TAGS"]) except ClientError: - LOG.warning( - "%sFailed to describe task definition %s", self.log_prefix, physical_id, exc_info=True - ) + LOG.warning("%sFailed to describe task definition %s", self.log_prefix, physical_id, exc_info=True) return None task_def = response.get("taskDefinition", {}) From b3a72328a8ecba2bce15422a987c673c3bc5f15c Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Sat, 6 Jun 2026 18:55:54 -0600 Subject: [PATCH 18/24] style: black formatting fix for test_ecs_container_sync_flow Co-Authored-By: Claude Sonnet 4.6 --- .../sync/flows/test_ecs_container_sync_flow.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py b/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py index 9858af1fbcf..f467a7dccc9 100644 --- a/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py +++ b/tests/unit/lib/sync/flows/test_ecs_container_sync_flow.py @@ -82,13 +82,12 @@ def test_sync_registers_new_task_definition_and_updates_service(self): flow._docker_client = docker_client flow._ecs_client = ecs_client - with patch( - "samcli.lib.sync.flows.ecs_container_sync_flow.ECRUploader" - ) as MockUploader, patch.object(flow, "_get_container_name", return_value=None): + with ( + patch("samcli.lib.sync.flows.ecs_container_sync_flow.ECRUploader") as MockUploader, + patch.object(flow, "_get_container_name", return_value=None), + ): mock_uploader_instance = MagicMock() - mock_uploader_instance.upload.return_value = ( - "123456789012.dkr.ecr.us-east-1.amazonaws.com/myrepo:newtag" - ) + mock_uploader_instance.upload.return_value = "123456789012.dkr.ecr.us-east-1.amazonaws.com/myrepo:newtag" MockUploader.return_value = mock_uploader_instance flow.sync() @@ -171,9 +170,7 @@ def test_update_services_updates_matching_service(self): new_arn = "arn:aws:ecs:us-east-1:123:task-definition/mytask:6" flow._update_services_to_task_definition(new_arn) - ecs_client.update_service.assert_called_once_with( - cluster="mycluster", service="mysvc", taskDefinition=new_arn - ) + ecs_client.update_service.assert_called_once_with(cluster="mycluster", service="mysvc", taskDefinition=new_arn) def test_update_services_skips_mismatched_family(self): flow = _make_flow() From 9d093368cbbb410edc8edad4f89dabeca3544c32 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Sat, 6 Jun 2026 22:18:33 -0600 Subject: [PATCH 19/24] fix: resolve mypy no-any-return in _register_updated_task_definition Annotate new_arn as Optional[str] to satisfy mypy's no-any-return check on the dict.get() chain from boto3 response. Co-Authored-By: Claude Sonnet 4.6 --- samcli/lib/sync/flows/ecs_container_sync_flow.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/lib/sync/flows/ecs_container_sync_flow.py b/samcli/lib/sync/flows/ecs_container_sync_flow.py index bff8bec03af..106cdd27e37 100644 --- a/samcli/lib/sync/flows/ecs_container_sync_flow.py +++ b/samcli/lib/sync/flows/ecs_container_sync_flow.py @@ -221,7 +221,7 @@ def _register_updated_task_definition(self, image_uri: str) -> Optional[str]: LOG.warning("%sFailed to register new task definition revision", self.log_prefix, exc_info=True) return None - new_arn = reg_response.get("taskDefinition", {}).get("taskDefinitionArn") + new_arn: Optional[str] = reg_response.get("taskDefinition", {}).get("taskDefinitionArn") LOG.info("%sRegistered new task definition revision: %s", self.log_prefix, new_arn) return new_arn From 5f39dd8031a05f9fceaff973451a115a24bd0374 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Mon, 8 Jun 2026 08:53:34 -0700 Subject: [PATCH 20/24] fix: update tests for intentional ECS/AgentCore split and multi-container strict error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - test_ecs_and_agentcore_use_same_flow_creator → assertNotEqual since AgentCore now routes to _create_agentcore_flow, not _create_ecs_container_flow - test_ecs_task_definition_falls_back_to_first_without_container_name → now expects DockerBuildFailed for multi-container TaskDef without ContainerName - Add @skipIf(IS_WINDOWS) to TestContainerImageBuild integ test — Linux container builds fail on Windows CI runners Co-Authored-By: Claude Sonnet 4.6 --- .../buildcmd/test_build_cmd_container_image.py | 4 +++- .../test_container_build_integration.py | 14 ++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/integration/buildcmd/test_build_cmd_container_image.py b/tests/integration/buildcmd/test_build_cmd_container_image.py index c34bd7cab90..8d49fa61693 100644 --- a/tests/integration/buildcmd/test_build_cmd_container_image.py +++ b/tests/integration/buildcmd/test_build_cmd_container_image.py @@ -1,16 +1,18 @@ """Integration test for ECS/AgentCore container image builds""" -import os import shutil import tempfile from pathlib import Path from unittest import TestCase +from unittest import skipIf import yaml from samcli.commands.build.build_context import BuildContext +from tests.testing_utils import IS_WINDOWS +@skipIf(IS_WINDOWS, "Linux container image builds are not supported on Windows CI runners") class TestContainerImageBuild(TestCase): """Test that samdev build correctly handles ECS and AgentCore container resources.""" diff --git a/tests/unit/lib/build_module/test_container_build_integration.py b/tests/unit/lib/build_module/test_container_build_integration.py index fcc800c3a89..b75389338f0 100644 --- a/tests/unit/lib/build_module/test_container_build_integration.py +++ b/tests/unit/lib/build_module/test_container_build_integration.py @@ -5,6 +5,7 @@ from copy import deepcopy from samcli.lib.build.app_builder import ApplicationBuilder +from samcli.lib.build.exceptions import DockerBuildFailed from samcli.lib.build.build_graph import ContainerBuildDefinition from samcli.lib.package.packageable_resources import ( AgentCoreRuntimeImageResource, @@ -87,16 +88,15 @@ def test_ecs_task_definition_targets_container_by_name(self): self.assertEqual(properties["ContainerDefinitions"][0]["Image"], "sidecar:latest") # unchanged self.assertEqual(properties["ContainerDefinitions"][1]["Image"], "myimage:latest") # updated - def test_ecs_task_definition_falls_back_to_first_without_container_name(self): + def test_ecs_task_definition_raises_when_multi_container_without_container_name(self): properties = { "ContainerDefinitions": [ {"Name": "web", "Image": "placeholder"}, {"Name": "sidecar", "Image": "sidecar:latest"}, ] } - ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path") - self.assertEqual(properties["ContainerDefinitions"][0]["Image"], "myimage:latest") - self.assertEqual(properties["ContainerDefinitions"][1]["Image"], "sidecar:latest") + with self.assertRaises(DockerBuildFailed): + ApplicationBuilder._update_built_resource("myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path") def test_agentcore_updates_nested_container_uri(self): properties = {"AgentRuntimeArtifact": {"ContainerConfiguration": {"ContainerUri": "placeholder"}}} @@ -151,8 +151,10 @@ def test_ecs_task_definition_registered(self): def test_agentcore_registered(self): self.assertIn(AWS_BEDROCK_AGENTCORE_RUNTIME, SyncFlowFactory.GENERATOR_MAPPING) - def test_ecs_and_agentcore_use_same_flow_creator(self): - self.assertEqual( + def test_ecs_and_agentcore_use_different_flow_creators(self): + # ECS uses _create_ecs_container_flow; AgentCore uses _create_agentcore_flow + # (AgentCore sync is not yet implemented — it logs a warning and returns None) + self.assertNotEqual( SyncFlowFactory.GENERATOR_MAPPING[AWS_ECS_TASK_DEFINITION], SyncFlowFactory.GENERATOR_MAPPING[AWS_BEDROCK_AGENTCORE_RUNTIME], ) From 9c60e9e653e23b9c89e0e3ed616c34ee60062120 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Mon, 8 Jun 2026 13:22:32 -0700 Subject: [PATCH 21/24] chore: regenerate schema to include AWS::ECS::TaskDefinition in sync --resource Co-Authored-By: Claude Sonnet 4.6 --- schema/samcli.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/schema/samcli.json b/schema/samcli.json index b7e3f1ec7e3..1c53b509f7a 100644 --- a/schema/samcli.json +++ b/schema/samcli.json @@ -1779,7 +1779,7 @@ "properties": { "parameters": { "title": "Parameters for the sync command", - "description": "Available parameters for the sync command:\n* template_file:\nAWS SAM template file.\n* code:\nSync ONLY code resources. This includes Lambda Functions, API Gateway, and Step Functions.\n* watch:\nWatch local files and automatically sync with cloud.\n* resource_id:\nSync code for all the resources with the ID. To sync a resource within a nested stack, use the following pattern {ChildStack}/{logicalId}.\n* resource:\nSync code for all resources of the given resource type. Accepted values are ['AWS::Serverless::Function', 'AWS::Lambda::Function', 'AWS::Serverless::LayerVersion', 'AWS::Lambda::LayerVersion', 'AWS::Serverless::Api', 'AWS::ApiGateway::RestApi', 'AWS::Serverless::HttpApi', 'AWS::ApiGatewayV2::Api', 'AWS::Serverless::StateMachine', 'AWS::StepFunctions::StateMachine']\n* dependency_layer:\nSeparate dependencies of individual function into a Lambda layer for improved performance.\n* skip_deploy_sync:\nThis option will skip the initial infrastructure deployment if it is not required by comparing the local template with the template deployed in cloud.\n* container_env_var_file:\nEnvironment variables json file (e.g., env_vars.json) to be passed to containers.\n* watch_exclude:\nExcludes a file or folder from being observed for file changes. Files and folders that are excluded will not trigger a sync workflow. This option can be provided multiple times.\n\nExamples:\n\nHelloWorldFunction=package-lock.json\n\nChildStackA/FunctionName=database.sqlite3\n* stack_name:\nName of the AWS CloudFormation stack.\n* base_dir:\nResolve relative paths to function's source code with respect to this directory. Use this if SAM template and source code are not in same enclosing folder. By default, relative paths are resolved with respect to the SAM template's location.\n* use_container:\nBuild functions within an AWS Lambda-like container.\n* build_in_source:\nOpts in to build project in the source folder. The following workflows support building in source: ['nodejs16.x', 'nodejs18.x', 'nodejs20.x', 'nodejs22.x', 'Makefile', 'esbuild']\n* build_image:\nContainer image URIs for building functions/layers. You can specify for all functions/layers with just the image URI (--build-image public.ecr.aws/sam/build-nodejs18.x:latest). You can specify for each individual function with (--build-image FunctionLogicalID=public.ecr.aws/sam/build-nodejs18.x:latest). A combination of the two can be used. If a function does not have build image specified or an image URI for all functions, the default SAM CLI build images will be used.\n* image_repository:\nAWS ECR repository URI where artifacts referenced in the template are uploaded.\n* image_repositories:\nMapping of Function Logical ID to AWS ECR Repository URI.\n\nExample: Function_Logical_ID=ECR_Repo_Uri\nThis option can be specified multiple times.\n* s3_bucket:\nAWS S3 bucket where artifacts referenced in the template are uploaded.\n* s3_prefix:\nPrefix name that is added to the artifact's name when it is uploaded to the AWS S3 bucket.\n* kms_key_id:\nThe ID of an AWS KMS key that is used to encrypt artifacts that are at rest in the AWS S3 bucket.\n* role_arn:\nARN of an IAM role that AWS Cloudformation assumes when executing a deployment change set.\n* parameter_overrides:\nString that contains AWS CloudFormation parameter overrides encoded as key=value pairs.\n* beta_features:\nEnable/Disable beta features.\n* debug:\nTurn on debug logging to print debug message generated by AWS SAM CLI and display timestamps.\n* profile:\nSelect a specific profile from your credential file to get AWS credentials.\n* region:\nSet the AWS Region of the service. (e.g. us-east-1)\n* metadata:\nMap of metadata to attach to ALL the artifacts that are referenced in the template.\n* notification_arns:\nARNs of SNS topics that AWS Cloudformation associates with the stack.\n* tags:\nList of tags to associate with the stack.\n* capabilities:\nList of capabilities that one must specify before AWS Cloudformation can create certain stacks.\n\nAccepted Values: CAPABILITY_IAM, CAPABILITY_NAMED_IAM, CAPABILITY_RESOURCE_POLICY, CAPABILITY_AUTO_EXPAND.\n\nLearn more at: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/acknowledging-application-capabilities.html\n* language_extensions:\nExpand AWS::LanguageExtensions transforms (Fn::ForEach, Fn::Length, Fn::ToJsonString, Fn::FindInMap with DefaultValue) locally before running SAM transforms. Off by default. Equivalent env var: SAM_CLI_ENABLE_LANGUAGE_EXTENSIONS=1.\n* save_params:\nSave the parameters provided via the command line to the configuration file.", + "description": "Available parameters for the sync command:\n* template_file:\nAWS SAM template file.\n* code:\nSync ONLY code resources. This includes Lambda Functions, API Gateway, and Step Functions.\n* watch:\nWatch local files and automatically sync with cloud.\n* resource_id:\nSync code for all the resources with the ID. To sync a resource within a nested stack, use the following pattern {ChildStack}/{logicalId}.\n* resource:\nSync code for all resources of the given resource type. Accepted values are ['AWS::Serverless::Function', 'AWS::Lambda::Function', 'AWS::Serverless::LayerVersion', 'AWS::Lambda::LayerVersion', 'AWS::Serverless::Api', 'AWS::ApiGateway::RestApi', 'AWS::Serverless::HttpApi', 'AWS::ApiGatewayV2::Api', 'AWS::Serverless::StateMachine', 'AWS::StepFunctions::StateMachine', 'AWS::ECS::TaskDefinition']\n* dependency_layer:\nSeparate dependencies of individual function into a Lambda layer for improved performance.\n* skip_deploy_sync:\nThis option will skip the initial infrastructure deployment if it is not required by comparing the local template with the template deployed in cloud.\n* container_env_var_file:\nEnvironment variables json file (e.g., env_vars.json) to be passed to containers.\n* watch_exclude:\nExcludes a file or folder from being observed for file changes. Files and folders that are excluded will not trigger a sync workflow. This option can be provided multiple times.\n\nExamples:\n\nHelloWorldFunction=package-lock.json\n\nChildStackA/FunctionName=database.sqlite3\n* stack_name:\nName of the AWS CloudFormation stack.\n* base_dir:\nResolve relative paths to function's source code with respect to this directory. Use this if SAM template and source code are not in same enclosing folder. By default, relative paths are resolved with respect to the SAM template's location.\n* use_container:\nBuild functions within an AWS Lambda-like container.\n* build_in_source:\nOpts in to build project in the source folder. The following workflows support building in source: ['nodejs16.x', 'nodejs18.x', 'nodejs20.x', 'nodejs22.x', 'Makefile', 'esbuild']\n* build_image:\nContainer image URIs for building functions/layers. You can specify for all functions/layers with just the image URI (--build-image public.ecr.aws/sam/build-nodejs18.x:latest). You can specify for each individual function with (--build-image FunctionLogicalID=public.ecr.aws/sam/build-nodejs18.x:latest). A combination of the two can be used. If a function does not have build image specified or an image URI for all functions, the default SAM CLI build images will be used.\n* image_repository:\nAWS ECR repository URI where artifacts referenced in the template are uploaded.\n* image_repositories:\nMapping of Function Logical ID to AWS ECR Repository URI.\n\nExample: Function_Logical_ID=ECR_Repo_Uri\nThis option can be specified multiple times.\n* s3_bucket:\nAWS S3 bucket where artifacts referenced in the template are uploaded.\n* s3_prefix:\nPrefix name that is added to the artifact's name when it is uploaded to the AWS S3 bucket.\n* kms_key_id:\nThe ID of an AWS KMS key that is used to encrypt artifacts that are at rest in the AWS S3 bucket.\n* role_arn:\nARN of an IAM role that AWS Cloudformation assumes when executing a deployment change set.\n* parameter_overrides:\nString that contains AWS CloudFormation parameter overrides encoded as key=value pairs.\n* beta_features:\nEnable/Disable beta features.\n* debug:\nTurn on debug logging to print debug message generated by AWS SAM CLI and display timestamps.\n* profile:\nSelect a specific profile from your credential file to get AWS credentials.\n* region:\nSet the AWS Region of the service. (e.g. us-east-1)\n* metadata:\nMap of metadata to attach to ALL the artifacts that are referenced in the template.\n* notification_arns:\nARNs of SNS topics that AWS Cloudformation associates with the stack.\n* tags:\nList of tags to associate with the stack.\n* capabilities:\nList of capabilities that one must specify before AWS Cloudformation can create certain stacks.\n\nAccepted Values: CAPABILITY_IAM, CAPABILITY_NAMED_IAM, CAPABILITY_RESOURCE_POLICY, CAPABILITY_AUTO_EXPAND.\n\nLearn more at: https://docs.aws.amazon.com/serverlessrepo/latest/devguide/acknowledging-application-capabilities.html\n* language_extensions:\nExpand AWS::LanguageExtensions transforms (Fn::ForEach, Fn::Length, Fn::ToJsonString, Fn::FindInMap with DefaultValue) locally before running SAM transforms. Off by default. Equivalent env var: SAM_CLI_ENABLE_LANGUAGE_EXTENSIONS=1.\n* save_params:\nSave the parameters provided via the command line to the configuration file.", "type": "object", "properties": { "template_file": { @@ -1806,10 +1806,11 @@ "resource": { "title": "resource", "type": "string", - "description": "Sync code for all resources of the given resource type. Accepted values are ['AWS::Serverless::Function', 'AWS::Lambda::Function', 'AWS::Serverless::LayerVersion', 'AWS::Lambda::LayerVersion', 'AWS::Serverless::Api', 'AWS::ApiGateway::RestApi', 'AWS::Serverless::HttpApi', 'AWS::ApiGatewayV2::Api', 'AWS::Serverless::StateMachine', 'AWS::StepFunctions::StateMachine']", + "description": "Sync code for all resources of the given resource type. Accepted values are ['AWS::Serverless::Function', 'AWS::Lambda::Function', 'AWS::Serverless::LayerVersion', 'AWS::Lambda::LayerVersion', 'AWS::Serverless::Api', 'AWS::ApiGateway::RestApi', 'AWS::Serverless::HttpApi', 'AWS::ApiGatewayV2::Api', 'AWS::Serverless::StateMachine', 'AWS::StepFunctions::StateMachine', 'AWS::ECS::TaskDefinition']", "enum": [ "AWS::ApiGateway::RestApi", "AWS::ApiGatewayV2::Api", + "AWS::ECS::TaskDefinition", "AWS::Lambda::Function", "AWS::Lambda::LayerVersion", "AWS::Serverless::Api", From b4d96e49acf7dbe9d45f9d60eebbfdd69edf810c Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Mon, 8 Jun 2026 14:21:51 -0700 Subject: [PATCH 22/24] ci: retrigger CI (windows uv setup infra flake) Co-Authored-By: Claude Sonnet 4.6 From 1a7d7f34a56439dab75eea0d64526e89d3715fb8 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Tue, 9 Jun 2026 07:51:23 -0700 Subject: [PATCH 23/24] ci: retrigger CI (pyinstaller-linux tar infra flake) Co-Authored-By: Claude Sonnet 4.6 From 5c458abbfcf8492a478d064f7894ec84b098ce74 Mon Sep 17 00:00:00 2001 From: Eric Johnson Date: Wed, 10 Jun 2026 15:18:13 -0700 Subject: [PATCH 24/24] fix: remove duplicate resource_metadata assignment in artifact_exporter Co-Authored-By: Claude Sonnet 4.6 --- samcli/lib/package/artifact_exporter.py | 1 - 1 file changed, 1 deletion(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 6968465bc5b..02276b25d70 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -620,7 +620,6 @@ def export(self) -> Dict: exporter.parent_parameter_values = self.parameter_values exporter.resource_metadata = resource.get("Metadata") exporter.language_extensions_enabled = self.language_extensions_enabled - exporter.resource_metadata = resource.get("Metadata") exporter.export(full_path, resource_dict, self.template_dir) return self.template_dict