Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions samcli/lib/build/app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ def _build_lambda_image(self, function_name: str, metadata: Dict, architecture:
docker_tag = f"{image_name.lower()}:{tag}"
docker_build_target = metadata.get("DockerBuildTarget", None)
docker_build_args = metadata.get("DockerBuildArgs", {})
docker_build_extra_params = metadata.get("DockerBuildExtraParams", None)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INPUT_VALIDATION] DockerBuildExtraParams is read from metadata with no type validation, unlike DockerBuildArgs which has an explicit isinstance(docker_build_args, dict) check. If a user provides a non-list value (e.g., a string like "--ssh default"), cmd.extend(extra_params) in CLIBuildClient would iterate over individual characters, producing broken flags. In SDKBuildClient, the truthiness check would pass and the warning would fire, but the inconsistency would be silently ignored.

Add a type check consistent with the existing DockerBuildArgs validation:

docker_build_extra_params = metadata.get("DockerBuildExtraParams", None)

if docker_build_extra_params is not None and not isinstance(docker_build_extra_params, list):
   raise DockerBuildFailed("DockerBuildExtraParams needs to be a list!")


if not dockerfile or not docker_context:
raise DockerBuildFailed("Docker file or Docker context metadata are missed.")
Expand All @@ -460,6 +461,8 @@ def _build_lambda_image(self, function_name: str, metadata: Dict, architecture:
}
if docker_build_target:
build_args["target"] = cast(str, docker_build_target)
if docker_build_extra_params:
build_args["extra_params"] = docker_build_extra_params

try:
if not self._image_build_client:
Expand Down
3 changes: 3 additions & 0 deletions samcli/lib/samlib/resource_metadata_normalizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@
IMAGE_ASSET_PROPERTY = "Code.ImageUri"
ASSET_DOCKERFILE_PATH_KEY = "aws:asset:dockerfile-path"
ASSET_DOCKERFILE_BUILD_ARGS_KEY = "aws:asset:docker-build-args"
ASSET_DOCKER_BUILD_EXTRA_PARAMS_KEY = "aws:asset:docker-build-extra-params"

SAM_RESOURCE_ID_KEY = "SamResourceId"
SAM_IS_NORMALIZED = "SamNormalized"
SAM_METADATA_DOCKERFILE_KEY = "Dockerfile"
SAM_METADATA_DOCKER_CONTEXT_KEY = "DockerContext"
SAM_METADATA_DOCKER_BUILD_ARGS_KEY = "DockerBuildArgs"
SAM_METADATA_DOCKER_BUILD_EXTRA_PARAMS_KEY = "DockerBuildExtraParams"

ASSET_BUNDLED_METADATA_KEY = "aws:asset:is-bundled"
SAM_METADATA_SKIP_BUILD_KEY = "SkipBuild"
Expand Down Expand Up @@ -189,6 +191,7 @@ def _extract_image_asset_metadata(metadata):
SAM_METADATA_DOCKERFILE_KEY: str(dockerfile_path.as_posix()),
SAM_METADATA_DOCKER_CONTEXT_KEY: str(asset_path),
SAM_METADATA_DOCKER_BUILD_ARGS_KEY: metadata.get(ASSET_DOCKERFILE_BUILD_ARGS_KEY, {}),
SAM_METADATA_DOCKER_BUILD_EXTRA_PARAMS_KEY: metadata.get(ASSET_DOCKER_BUILD_EXTRA_PARAMS_KEY, None),
}

@staticmethod
Expand Down
14 changes: 14 additions & 0 deletions samcli/local/docker/image_build_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def build_image(
platform: Optional[str] = None,
target: Optional[str] = None,
rm: bool = True,
extra_params: Optional[list[str]] = None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] The extra_params parameter is added to both SDKBuildClient.build_image and CLIBuildClient.build_image, but the abstract base class ImageBuildClient.build_image is not updated to include it.

This breaks the interface contract: type checkers (mypy) will flag the subclass signatures as incompatible with the abstract method, and developers reading the base class won't know extra_params exists.

The fix is to add the parameter to the abstract method signature:

@abstractmethod
def build_image(
   self,
   path: str,
   dockerfile: str,
   tag: str,
   buildargs: Optional[Dict[str, str]] = None,
   platform: Optional[str] = None,
   target: Optional[str] = None,
   rm: bool = True,
   extra_params: Optional[list[str]] = None,
) -> Generator[Dict[str, Any], None, None]:

The docstring for the abstract method (lines 44-67) should also be updated to document the new parameter.

) -> Generator[Dict[str, Any], None, None]:
"""
Build a container image from a Dockerfile.
Expand All @@ -59,6 +60,8 @@ def build_image(
Build target stage in multi-stage Dockerfile
rm : bool
Remove intermediate containers after build (default: True)
extra_params : list of str, optional
Extra CLI flags (e.g., ["--ssh", "default"]). Only supported by CLIBuildClient

Yields
------
Expand Down Expand Up @@ -119,6 +122,7 @@ def build_image(
platform: Optional[str] = None,
target: Optional[str] = None,
rm: bool = True,
extra_params: Optional[list[str]] = None,
) -> Generator[Dict[str, Any], None, None]:
"""Build image using docker-py SDK"""
build_kwargs = {
Expand All @@ -135,6 +139,12 @@ def build_image(
if target is not None:
build_kwargs["target"] = target

if extra_params:
LOG.warning(
"DockerBuildExtraParams are not supported with the SDK build client and will be ignored. "
"Use --use-buildkit to enable CLI-based builds."
)

_, build_logs = self.container_client.images.build(**build_kwargs)
return build_logs # type: ignore[no-any-return]

Expand All @@ -159,6 +169,7 @@ def build_image(
platform: Optional[str] = None,
target: Optional[str] = None,
rm: bool = True,
extra_params: Optional[list[str]] = None,
) -> Generator[Dict[str, Any], None, None]:
# Make dockerfile path relative to context if not absolute
if not os.path.isabs(dockerfile):
Expand Down Expand Up @@ -187,6 +198,9 @@ def build_image(
if rm:
cmd.append("--rm")

if extra_params:
cmd.extend(extra_params)

cmd.append(path)

LOG.debug(f"Executing build command: {' '.join(cmd)}")
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/lib/build_module/test_app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -1774,6 +1774,24 @@ def test_build_lambda_image_raises_for_non_dict_docker_build_args(self):
with self.assertRaises(DockerBuildFailed):
self.builder._build_lambda_image("Name", metadata, X86_64)

def test_can_build_image_function_with_extra_params(self):
metadata = {
"Dockerfile": "Dockerfile",
"DockerContext": "context",
"DockerTag": "Tag",
"DockerBuildArgs": {"a": "b"},
"DockerBuildExtraParams": ["--ssh", "default"],
}

mock_build_client = Mock()
mock_build_client.build_image.return_value = iter([])
self.builder._image_build_client = mock_build_client

self.builder._build_lambda_image("Name", metadata, X86_64)

call_kwargs = mock_build_client.build_image.call_args[1]
self.assertEqual(call_kwargs["extra_params"], ["--ssh", "default"])

def test_build_lambda_image_uses_latest_tag_when_not_specified(self):
metadata = {
"Dockerfile": "Dockerfile",
Expand Down
5 changes: 5 additions & 0 deletions tests/unit/lib/samlib/test_resource_metadata_normalizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ def test_replace_all_resources_that_contain_metadata(self):

def test_replace_all_resources_that_contain_image_metadata(self):
docker_build_args = {"arg1": "val1", "arg2": "val2"}
docker_build_extra_params = ["--param1", "value1"]
asset_path = pathlib.Path("/path", "to", "asset")
dockerfile_path = pathlib.Path("path", "to", "Dockerfile")
template_data = {
Expand All @@ -97,6 +98,7 @@ def test_replace_all_resources_that_contain_image_metadata(self):
"aws:asset:property": "Code.ImageUri",
"aws:asset:dockerfile-path": dockerfile_path,
"aws:asset:docker-build-args": docker_build_args,
"aws:asset:docker-build-extra-params": docker_build_extra_params,
},
},
}
Expand All @@ -112,6 +114,9 @@ def test_replace_all_resources_that_contain_image_metadata(self):
expected_dockerfile_path = str(pathlib.Path("path", "to", "Dockerfile").as_posix())
self.assertEqual(expected_dockerfile_path, template_data["Resources"]["Function1"]["Metadata"]["Dockerfile"])
self.assertEqual(docker_build_args, template_data["Resources"]["Function1"]["Metadata"]["DockerBuildArgs"])
self.assertEqual(
docker_build_extra_params, template_data["Resources"]["Function1"]["Metadata"]["DockerBuildExtraParams"]
)
self.assertEqual("Function1", template_data["Resources"]["Function1"]["Metadata"]["SamResourceId"])

def test_replace_all_resources_that_contain_image_metadata_dockerfile_extensions(self):
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/local/docker/test_image_build_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ def test_build_image_minimal(self):
rm=True,
)

@patch("samcli.local.docker.image_build_client.LOG")
def test_build_image_extra_params_logs_warning(self, mock_log):
"""Test that extra_params triggers a warning and is not passed to SDK"""
mock_image = Mock()
mock_logs = iter([])

self.mock_container_client.images.build.return_value = (mock_image, mock_logs)
self.client.build_image(**self.base_build_args, extra_params=["--ssh", "default"])
mock_log.warning.assert_called_once()
self.assertIn("DockerBuildExtraParams", mock_log.warning.call_args[0][0])

self.mock_container_client.images.build.assert_called_once_with(**self.base_build_args, rm=True)

def test_is_available_returns_true(self):
"""Test that is_available always returns True for SDK"""
result = SDKBuildClient.is_available("docker")
Expand Down Expand Up @@ -202,6 +215,28 @@ def test_build_image_handles_failure(self, mock_popen):
self.assertIn("Build failed with exit code 1", str(context.exception))
self.assertEqual(context.exception.build_log, [{"stream": "Step 1/5\n"}, {"stream": "Error: build failed\n"}])

@patch("samcli.local.docker.image_build_client.subprocess.Popen")
def test_build_image_docker_with_extra_params(self, mock_popen):
"""Test that extra_params are appended to the docker command"""
mock_process = Mock()
mock_process.stdout = iter(["Done\n"])
mock_process.returncode = 0
mock_popen.return_value = mock_process

list(
self.docker_client.build_image(
**self.base_build_args,
extra_params=["--ssh", "default", "--secret", "id=mysecret,src=secret.txt"],
)
)

actual_cmd = mock_popen.call_args[0][0]
self.assertEqual(actual_cmd[-1], self.base_build_args["path"])
self.assertIn("--ssh", actual_cmd)
self.assertIn("default", actual_cmd)
self.assertIn("--secret", actual_cmd)
self.assertIn("id=mysecret,src=secret.txt", actual_cmd)

@patch("samcli.local.docker.image_build_client.shutil.which")
@patch("samcli.local.docker.image_build_client.subprocess.run")
def test_is_available_docker_success(self, mock_run, mock_which):
Expand Down
Loading