fix(core,cloudformation): unknown X-Amz-Target -> UnknownOperationException; UpdateStack GetAtt merge (bug-hunt) (#2287)

Author: vieiralucas

crates/fakecloud-cloudformation/src/service.rs

@@ -30,6 +30,33 @@ use crate::xml_responses;
 /// supplement the eagerly-captured `ProvisionResult::attributes` with
 /// live-state lookups via `ResourceProvisioner::get_att`. Resources not
 /// listed here just use whatever the create handler captured.
+/// Build the full GetAtt attribute set for a resource: start from `base` (the
+/// create-time attributes, so values like `CreationTime` survive an update),
+/// overlay the resource's own attributes (the handler's fresh values), then
+/// overlay any well-known attribute the provisioner can still resolve from live
+/// state. Used by both create and update so a 2nd deploy's GetAtt/Outputs don't
+/// collapse to the update handler's subset (and resolve to literal
+/// `"Logical.Attr"`).
+fn merged_resource_attributes(
+    provisioner: &ResourceProvisioner,
+    resource: &StackResource,
+    base: std::collections::BTreeMap<String, String>,
+) -> std::collections::BTreeMap<String, String> {
+    let mut attr_map = base;
+    for (k, v) in &resource.attributes {
+        attr_map.insert(k.clone(), v.clone());
+    }
+    for attr in well_known_attributes_for(&resource.resource_type) {
+        if attr_map.contains_key(*attr) {
+            continue;
+        }
+        if let Some(v) = provisioner.get_att(resource, attr) {
+            attr_map.insert((*attr).to_string(), v);
+        }
+    }
+    attr_map
+}
+
 fn well_known_attributes_for(resource_type: &str) -> &'static [&'static str] {
     match resource_type {
         "AWS::S3::Bucket" => &[
@@ -290,15 +317,11 @@ pub(crate) fn provision_stack_resources(
                     // overlay anything the provisioner can resolve from
                     // live state (e.g. attributes that depend on side-effects
                     // recorded after the create handler returned).
-                    let mut attr_map = stack_resource.attributes.clone();
-                    for attr in well_known_attributes_for(&stack_resource.resource_type) {
-                        if attr_map.contains_key(*attr) {
-                            continue;
-                        }
-                        if let Some(v) = provisioner.get_att(&stack_resource, attr) {
-                            attr_map.insert((*attr).to_string(), v);
-                        }
-                    }
+                    let attr_map = merged_resource_attributes(
+                        provisioner,
+                        &stack_resource,
+                        std::collections::BTreeMap::new(),
+                    );
                     attributes.insert(stack_resource.logical_id.clone(), attr_map.clone());
                     // Persist the live-resolved attributes onto the stored
                     // resource so later readers — notably resolve_template_outputs,
@@ -2778,7 +2801,7 @@ pub(crate) fn apply_resource_updates(
                 .cloned();
             if let Some(existing) = existing {
                 match provisioner.update_resource(&existing, &resolved_def) {
-                    Ok(Some(updated)) => {
+                    Ok(Some(mut updated)) => {
                         changes.push(ResourceChange {
                             action: ResourceChangeAction::Update,
                             logical_id: updated.logical_id.clone(),
@@ -2787,7 +2810,18 @@ pub(crate) fn apply_resource_updates(
                         });
                         physical_ids
                             .insert(updated.logical_id.clone(), updated.physical_id.clone());
-                        attributes.insert(updated.logical_id.clone(), updated.attributes.clone());
+                        // Merge onto the create-time attribute set + well-known
+                        // GetAtt overlay so a 2nd deploy doesn't collapse Outputs
+                        // to the update handler's subset (dropping e.g.
+                        // CreationTime and making GetAtt resolve to literal
+                        // "Logical.Attr").
+                        let merged = merged_resource_attributes(
+                            provisioner,
+                            &updated,
+                            existing.attributes.clone(),
+                        );
+                        attributes.insert(updated.logical_id.clone(), merged.clone());
+                        updated.attributes = merged;
                         if let Some(slot) = stack
                             .resources
                             .iter_mut()

crates/fakecloud-core/src/dispatch.rs

@@ -86,6 +86,24 @@ pub async fn dispatch(
         match protocol::detect_service(&parts.headers, &query_params, &body_bytes) {
             Some(d) => d,
             None => {
+                // A request carrying X-Amz-Target is unambiguously an awsJson
+                // call whose operation we couldn't map to a known service. AWS
+                // answers these with UnknownOperationException; routing them to
+                // the apigateway catch-all below would 404 with a misleading
+                // "Stage not found" instead.
+                if let Some(target) = parts
+                    .headers
+                    .get("x-amz-target")
+                    .and_then(|v| v.to_str().ok())
+                {
+                    return build_error_response(
+                        StatusCode::BAD_REQUEST,
+                        "UnknownOperationException",
+                        &format!("The operation {target} is not recognized."),
+                        &request_id,
+                        AwsProtocol::Json,
+                    );
+                }
                 // OPTIONS requests (CORS preflight) don't carry Authorization headers.
                 // Route them to S3 since S3 is the only REST service that handles CORS.
                 // Note: API Gateway CORS preflight is not fully supported in this emulator

crates/fakecloud-e2e/tests/cloudformation_pipes.rs

@@ -126,3 +126,112 @@ async fn cfn_provisions_and_runs_pipe() {
     let gone = pipes.describe_pipe().name("cfn-pipe").send().await;
     assert!(gone.is_err(), "stack delete should remove the pipe");
 }
+
+// The pipe update handler returns only a subset of attributes (Arn, state,
+// LastModifiedTime) — not the create-time CreationTime. UpdateStack must merge
+// the update onto the create-time attribute set so a GetAtt CreationTime output
+// keeps resolving to the real timestamp instead of the literal "Pipe.CreationTime".
+const TEMPLATE_CREATIONTIME_V1: &str = r#"{
+  "Resources": {
+    "SrcQ": { "Type": "AWS::SQS::Queue", "Properties": { "QueueName": "cfn-pipe-ct-src" } },
+    "TgtQ": { "Type": "AWS::SQS::Queue", "Properties": { "QueueName": "cfn-pipe-ct-tgt" } },
+    "Pipe": {
+      "Type": "AWS::Pipes::Pipe",
+      "Properties": {
+        "Name": "cfn-pipe-ct",
+        "DesiredState": "RUNNING",
+        "RoleArn": "arn:aws:iam::000000000000:role/pipe-role",
+        "Source": { "Fn::GetAtt": ["SrcQ", "Arn"] },
+        "Target": { "Fn::GetAtt": ["TgtQ", "Arn"] }
+      }
+    }
+  },
+  "Outputs": {
+    "PipeCreatedAt": { "Value": { "Fn::GetAtt": ["Pipe", "CreationTime"] } }
+  }
+}"#;
+
+const TEMPLATE_CREATIONTIME_V2: &str = r#"{
+  "Resources": {
+    "SrcQ": { "Type": "AWS::SQS::Queue", "Properties": { "QueueName": "cfn-pipe-ct-src" } },
+    "TgtQ": { "Type": "AWS::SQS::Queue", "Properties": { "QueueName": "cfn-pipe-ct-tgt" } },
+    "Pipe": {
+      "Type": "AWS::Pipes::Pipe",
+      "Properties": {
+        "Name": "cfn-pipe-ct",
+        "DesiredState": "STOPPED",
+        "RoleArn": "arn:aws:iam::000000000000:role/pipe-role",
+        "Source": { "Fn::GetAtt": ["SrcQ", "Arn"] },
+        "Target": { "Fn::GetAtt": ["TgtQ", "Arn"] }
+      }
+    }
+  },
+  "Outputs": {
+    "PipeCreatedAt": { "Value": { "Fn::GetAtt": ["Pipe", "CreationTime"] } }
+  }
+}"#;
+
+fn output_value(
+    desc: &aws_sdk_cloudformation::operation::describe_stacks::DescribeStacksOutput,
+    key: &str,
+) -> String {
+    desc.stacks()[0]
+        .outputs()
+        .iter()
+        .find(|o| o.output_key() == Some(key))
+        .and_then(|o| o.output_value())
+        .unwrap_or_default()
+        .to_string()
+}
+
+#[tokio::test]
+async fn cfn_update_preserves_create_time_getatt_output() {
+    let s = TestServer::start().await;
+    let cfn = s.cloudformation_client().await;
+
+    cfn.create_stack()
+        .stack_name("pipe-ct-stack")
+        .template_body(TEMPLATE_CREATIONTIME_V1)
+        .send()
+        .await
+        .expect("create_stack");
+
+    let after_create = cfn
+        .describe_stacks()
+        .stack_name("pipe-ct-stack")
+        .send()
+        .await
+        .unwrap();
+    let created_at = output_value(&after_create, "PipeCreatedAt");
+    assert!(
+        created_at.parse::<i64>().is_ok(),
+        "CreationTime output should be a timestamp on create, got {created_at:?}"
+    );
+
+    cfn.update_stack()
+        .stack_name("pipe-ct-stack")
+        .template_body(TEMPLATE_CREATIONTIME_V2)
+        .send()
+        .await
+        .expect("update_stack");
+
+    let after_update = cfn
+        .describe_stacks()
+        .stack_name("pipe-ct-stack")
+        .send()
+        .await
+        .unwrap();
+    let updated_output = output_value(&after_update, "PipeCreatedAt");
+    assert_ne!(
+        updated_output, "Pipe.CreationTime",
+        "GetAtt collapsed to the literal placeholder after update"
+    );
+    assert!(
+        updated_output.parse::<i64>().is_ok(),
+        "CreationTime output must survive the update as a timestamp, got {updated_output:?}"
+    );
+    assert_eq!(
+        updated_output, created_at,
+        "CreationTime must be stable across the update"
+    );
+}

crates/fakecloud-e2e/tests/dispatch_unknown_target.rs

@@ -0,0 +1,36 @@
+//! Dispatch compatibility: a request carrying an unrecognized `X-Amz-Target`
+//! header is an awsJson call whose operation we can't map to a known service.
+//! AWS answers these with `UnknownOperationException`; fakecloud must not route
+//! them to the apigateway catch-all (which 404s with a misleading "Stage not
+//! found").
+
+mod helpers;
+
+use helpers::TestServer;
+
+#[tokio::test]
+async fn unknown_x_amz_target_returns_unknown_operation_exception() {
+    let server = TestServer::start().await;
+    let http = reqwest::Client::new();
+
+    let resp = http
+        .post(server.endpoint())
+        .header("content-type", "application/x-amz-json-1.1")
+        .header("x-amz-target", "NotARealService_20990101.DoSomething")
+        .body("{}")
+        .send()
+        .await
+        .expect("request sent");
+
+    assert_eq!(resp.status(), 400, "unknown target must be a 400");
+    let body = resp.text().await.unwrap();
+    assert!(
+        body.contains("UnknownOperationException"),
+        "expected UnknownOperationException, got: {body}"
+    );
+    // And crucially not the misleading apigateway catch-all.
+    assert!(
+        !body.contains("Stage") && !body.contains("NotFoundException"),
+        "must not fall through to the apigateway catch-all: {body}"
+    );
+}