Merge branch 'main' into fix/s3-grant-imported-bucket-race-condition

Author: weklund

CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### **Changed**
 
 - fixed `sagemaker-templates` Model Deploy seed code S3 permission race condition where `grant_read_write()` on an imported bucket created a `DefaultPolicy` with no CloudFormation dependency from the SageMaker Model, causing intermittent `s3:GetObject` access denied errors
+- consolidated redundant `DevStage`/`PreProdStage`/`ProdStage` classes into a single `DeployStage` in `sagemaker-templates` model deploy seed code, fixing redundant CF stack names (e.g. `dev-dev-endpoint` → `dev-{project}-endpoint`) and adding project uniqueness to prevent cross-project collisions
 
 ## v3.2.3
 

modules/sagemaker/sagemaker-templates/templates/model_deploy/seed_code/deploy_app/deploy_app/deploy_endpoint_stack.py

@@ -79,6 +79,8 @@ def __init__(
         self,
         scope: constructs.Construct,
         id: str,
+        *,
+        stage_name: str,
         vpc_id: str,
         subnet_ids: List[str],
         security_group_ids: List[str],
@@ -199,7 +201,7 @@ def __init__(
         latest_approved_model_package = get_approved_package()
 
         # Sagemaker Model
-        model_name = f"-{id}-{timestamp}"
+        model_name = f"-{stage_name}-{timestamp}"
         model_name = MODEL_PACKAGE_GROUP_NAME[: MAX_NAME_LENGTH - len(model_name)] + model_name
 
         vpc_config = None
@@ -227,7 +229,7 @@ def __init__(
         )
 
         # Sagemaker Endpoint Config
-        endpoint_config_name = f"-{id}-ec-{timestamp}"
+        endpoint_config_name = f"-{stage_name}-ec-{timestamp}"
         endpoint_config_name = (
             MODEL_PACKAGE_GROUP_NAME[: MAX_NAME_LENGTH - len(endpoint_config_name)] + endpoint_config_name
         )
@@ -311,7 +313,7 @@ def __init__(
         endpoint_config.add_dependency(model)
 
         # Sagemaker Endpoint
-        endpoint_name = f"-{id}-endpoint"
+        endpoint_name = f"-{stage_name}-ep"
         endpoint_name = MODEL_PACKAGE_GROUP_NAME[: MAX_NAME_LENGTH - len(endpoint_name)] + endpoint_name
 
         endpoint = sagemaker.CfnEndpoint(

modules/sagemaker/sagemaker-templates/templates/model_deploy/seed_code/deploy_app/deploy_app/pipeline_stack.py

@@ -44,42 +44,27 @@
 }
 
 
-class DevStage(cdk.Stage):
-    def __init__(self, scope: Construct, construct_id: str, **kwargs: Any) -> None:
-        super().__init__(scope, construct_id, **kwargs)
-
-        DeployEndpointStack(
-            self,
-            "dev-endpoint",
-            vpc_id=constants.DEV_VPC_ID,
-            subnet_ids=constants.DEV_SUBNET_IDS,
-            security_group_ids=constants.DEV_SECURITY_GROUP_IDS,
-        )
-
-
-class PreProdStage(cdk.Stage):
-    def __init__(self, scope: Construct, construct_id: str, **kwargs: Any) -> None:
-        super().__init__(scope, construct_id, **kwargs)
-
-        DeployEndpointStack(
-            self,
-            "preprod-endpoint",
-            vpc_id=constants.PRE_PROD_VPC_ID,
-            subnet_ids=constants.PRE_PROD_SUBNET_IDS,
-            security_group_ids=constants.PRE_PROD_SECURITY_GROUP_IDS,
-        )
-
-
-class ProdStage(cdk.Stage):
-    def __init__(self, scope: Construct, construct_id: str, **kwargs: Any) -> None:
+class DeployStage(cdk.Stage):
+    def __init__(
+        self,
+        scope: Construct,
+        construct_id: str,
+        *,
+        stage_name: str,
+        vpc_id: str,
+        subnet_ids: list[str],
+        security_group_ids: list[str],
+        **kwargs: Any,
+    ) -> None:
         super().__init__(scope, construct_id, **kwargs)
 
         DeployEndpointStack(
             self,
-            "prod-endpoint",
-            vpc_id=constants.PROD_VPC_ID,
-            subnet_ids=constants.PROD_SUBNET_IDS,
-            security_group_ids=constants.PROD_SECURITY_GROUP_IDS,
+            f"{constants.PROJECT_NAME}-endpoint",
+            stage_name=stage_name,
+            vpc_id=vpc_id,
+            subnet_ids=subnet_ids,
+            security_group_ids=security_group_ids,
         )
 
 
@@ -188,17 +173,25 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: Any) -> None:
         )
 
         pipeline.add_stage(
-            DevStage(
+            DeployStage(
                 self,
                 "dev",
+                stage_name="dev",
+                vpc_id=constants.DEV_VPC_ID,
+                subnet_ids=constants.DEV_SUBNET_IDS,
+                security_group_ids=constants.DEV_SECURITY_GROUP_IDS,
                 env=cdk.Environment(account=constants.DEV_ACCOUNT_ID, region=constants.DEV_REGION),
             )
         )
 
         pipeline.add_stage(
-            PreProdStage(
+            DeployStage(
                 self,
                 "preprod",
+                stage_name="preprod",
+                vpc_id=constants.PRE_PROD_VPC_ID,
+                subnet_ids=constants.PRE_PROD_SUBNET_IDS,
+                security_group_ids=constants.PRE_PROD_SECURITY_GROUP_IDS,
                 env=cdk.Environment(account=constants.PRE_PROD_ACCOUNT_ID, region=constants.PRE_PROD_REGION),
             ),
             pre=[ManualApprovalStep("ApprovePreProd", comment="Approve deployment to Pre-Production")]
@@ -207,9 +200,13 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs: Any) -> None:
         )
 
         pipeline.add_stage(
-            ProdStage(
+            DeployStage(
                 self,
                 "prod",
+                stage_name="prod",
+                vpc_id=constants.PROD_VPC_ID,
+                subnet_ids=constants.PROD_SUBNET_IDS,
+                security_group_ids=constants.PROD_SECURITY_GROUP_IDS,
                 env=cdk.Environment(account=constants.PROD_ACCOUNT_ID, region=constants.PROD_REGION),
             ),
             pre=[ManualApprovalStep("ApproveProd", comment="Approve deployment to Production")]

modules/sagemaker/sagemaker-templates/templates/model_deploy/seed_code/deploy_app/tests/test_synth_s3_permissions.py

@@ -1,8 +1,8 @@
-"""Verify that cdk synth produces the expected S3 permission statements.
+"""Verify that cdk synth produces correct SageMaker resource names.
 
 This test mocks the SageMaker API call (get_approved_package) and all
 required environment variables, then synthesizes the DeployEndpointStack
-and asserts the ManagedPolicy contains the correct S3 actions.
+and asserts that stage_name drives unique SageMaker resource names.
 """
 
 import json
@@ -53,7 +53,7 @@
 from deploy_app.deploy_endpoint_stack import DeployEndpointStack  # noqa: E402
 
 
-def _synth_deploy_endpoint_stack() -> assertions.Template:
+def _synth_deploy_endpoint_stack(stage_name: str = "dev") -> assertions.Template:
     """Synthesize a standalone DeployEndpointStack and return its Template."""
     # Reset the mock so each test gets a fresh call count
     _fake_get_approved.reset_mock()
@@ -67,7 +67,8 @@ def _synth_deploy_endpoint_stack() -> assertions.Template:
     )
     stack = DeployEndpointStack(
         stage,
-        "test-endpoint",
+        "test-project-endpoint",
+        stage_name=stage_name,
         vpc_id="vpc-abc123",
         subnet_ids=["subnet-aaa"],
         security_group_ids=["sg-aaa"],
@@ -76,95 +77,25 @@ def _synth_deploy_endpoint_stack() -> assertions.Template:
     return assertions.Template.from_stack(stack)
 
 
-def test_managed_policy_has_s3_read_write_actions():
-    """The ModelExecutionPolicy must contain S3 read/write actions on the model bucket."""
-    template = _synth_deploy_endpoint_stack()
-
-    template.has_resource_properties(
-        "AWS::IAM::ManagedPolicy",
-        assertions.Match.object_like(
-            {
-                "PolicyDocument": {
-                    "Statement": assertions.Match.array_with(
-                        [
-                            assertions.Match.object_like(
-                                {
-                                    "Action": assertions.Match.array_with(
-                                        [
-                                            "s3:GetObject*",
-                                            "s3:GetBucket*",
-                                            "s3:List*",
-                                            "s3:PutObject",
-                                        ]
-                                    ),
-                                    "Effect": "Allow",
-                                    "Resource": [
-                                        "arn:aws:s3:::test-model-bucket",
-                                        "arn:aws:s3:::test-model-bucket/*",
-                                    ],
-                                }
-                            ),
-                        ]
-                    ),
-                }
-            }
-        ),
-    )
+def _get_endpoint_name(template: assertions.Template) -> str:
+    """Extract the SageMaker endpoint name from a synthesized template."""
+    endpoints = template.find_resources("AWS::SageMaker::Endpoint")
+    assert len(endpoints) == 1, f"Expected 1 endpoint, found {len(endpoints)}"
+    props = next(iter(endpoints.values()))["Properties"]
+    return props["EndpointName"]
 
 
-def test_managed_policy_has_data_capture_write_actions():
-    """The ModelExecutionPolicy must contain S3 write actions for data capture."""
-    template = _synth_deploy_endpoint_stack()
+def test_stage_name_produces_unique_endpoint_names():
+    """Different stage_name values must produce different SageMaker endpoint names."""
+    dev_template = _synth_deploy_endpoint_stack(stage_name="dev")
+    prod_template = _synth_deploy_endpoint_stack(stage_name="prod")
 
-    template.has_resource_properties(
-        "AWS::IAM::ManagedPolicy",
-        assertions.Match.object_like(
-            {
-                "PolicyDocument": {
-                    "Statement": assertions.Match.array_with(
-                        [
-                            assertions.Match.object_like(
-                                {
-                                    "Action": assertions.Match.array_with(
-                                        [
-                                            "s3:PutObject",
-                                        ]
-                                    ),
-                                    "Effect": "Allow",
-                                    "Resource": "arn:aws:s3:::test-model-bucket/endpoint-data-capture/*",
-                                }
-                            ),
-                        ]
-                    ),
-                }
-            }
-        ),
-    )
-
-
-def test_no_separate_default_policy():
-    """There must be NO DefaultPolicy (inline AWS::IAM::Policy) for S3 permissions.
-
-    S3 permissions must be in the ManagedPolicy, not a separate inline policy,
-    to preserve the CloudFormation dependency chain.
-    """
-    template = _synth_deploy_endpoint_stack()
+    dev_ep = _get_endpoint_name(dev_template)
+    prod_ep = _get_endpoint_name(prod_template)
 
-    # Count all IAM::Policy resources -- there should be none with S3 actions
-    resources = template.find_resources("AWS::IAM::Policy")
-    for logical_id, resource in resources.items():
-        policy_doc = resource.get("Properties", {}).get("PolicyDocument", {})
-        statements = policy_doc.get("Statement", [])
-        for stmt in statements:
-            actions = stmt.get("Action", [])
-            if isinstance(actions, str):
-                actions = [actions]
-            s3_actions = [a for a in actions if a.startswith("s3:")]
-            assert not s3_actions, (
-                f"Found S3 actions {s3_actions} in inline IAM::Policy '{logical_id}'. "
-                f"S3 permissions must be in the ManagedPolicy to ensure correct "
-                f"CloudFormation dependency ordering."
-            )
+    assert dev_ep != prod_ep, f"dev and prod endpoint names should differ, both are '{dev_ep}'"
+    assert "-dev-" in dev_ep, f"Expected '-dev-' in endpoint name, got '{dev_ep}'"
+    assert "-prod-" in prod_ep, f"Expected '-prod-' in endpoint name, got '{prod_ep}'"
 
 
 def test_role_references_managed_policy():