Merge pull request #160 from michaelryanmcneill/ROSA-786

openshift-merge-bot[bot] · web-flow · commit 1ca111af1627 · 2026-07-02T14:42:13.000Z
ROSAENG-57757 | fix: apply trust_policy_external_id to support role trust policy
diff --git a/modules/account-iam-resources/README.md b/modules/account-iam-resources/README.md
@@ -56,7 +56,6 @@ No modules.
 | [random_string.default_random](https://registry.terraform.io/providers/hashicorp/random/latest/docs/resources/string) | resource |
 | [time_sleep.account_iam_resources_wait](https://registry.terraform.io/providers/hashicorp/time/latest/docs/resources/sleep) | resource |
 | [aws_caller_identity.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) | data source |
-| [aws_iam_policy_document.custom_trust_policy](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
 | [aws_partition.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/partition) | data source |
 | [rhcs_hcp_policies.all_policies](https://registry.terraform.io/providers/terraform-redhat/rhcs/latest/docs/data-sources/hcp_policies) | data source |
 | [rhcs_info.current](https://registry.terraform.io/providers/terraform-redhat/rhcs/latest/docs/data-sources/info) | data source |
diff --git a/modules/account-iam-resources/main.tf b/modules/account-iam-resources/main.tf
@@ -3,14 +3,17 @@
 
 locals {
   path = coalesce(var.path, "/")
+  trust_policy_external_id = (
+    var.trust_policy_external_id != null && var.trust_policy_external_id != ""
+  ) ? var.trust_policy_external_id : null
   account_roles_properties = [
     {
       role_name            = "HCP-ROSA-Installer"
       role_type            = "installer"
       policy_details       = "arn:${data.aws_partition.current.partition}:iam::aws:policy/service-role/ROSAInstallerPolicy"
       principal_type       = "AWS"
       principal_identifier = "arn:${data.aws_partition.current.partition}:iam::${data.rhcs_info.current.ocm_aws_account_id}:role/RH-Managed-OpenShift-Installer"
-      external_id          = var.trust_policy_external_id
+      external_id          = local.trust_policy_external_id
     },
     {
       role_name      = "HCP-ROSA-Support"
@@ -19,7 +22,7 @@ locals {
       principal_type = "AWS"
       // This is a SRE RH Support role which is used to assume this support role
       principal_identifier = data.rhcs_hcp_policies.all_policies.account_role_policies["sts_support_rh_sre_role"]
-      external_id          = null
+      external_id          = local.trust_policy_external_id
     },
     {
       role_name            = "HCP-ROSA-Worker"
@@ -45,6 +48,32 @@ locals {
   vpce_role_name          = local.vpce_splits[length(local.vpce_splits) - 1]
   vpce_policy_name        = substr("${local.vpce_role_name}-assume-role", 0, 64)
   policy_arn_base         = "arn:aws:iam::${data.aws_caller_identity.current.account_id}:policy"
+  # jsonencode instead of aws_iam_policy_document: trades structural validation for plan-time testability via terraform test.
+  custom_trust_policy_json = [
+    for idx in range(local.account_roles_count) : jsonencode({
+      Version = "2012-10-17"
+      Statement = [
+        merge(
+          {
+            Effect = "Allow"
+            Action = "sts:AssumeRole"
+            Principal = local.account_roles_properties[idx].principal_type == "AWS" ? {
+              AWS = local.account_roles_properties[idx].principal_identifier
+              } : {
+              Service = local.account_roles_properties[idx].principal_identifier
+            }
+          },
+          local.account_roles_properties[idx].external_id != null ? {
+            Condition = {
+              StringEquals = {
+                "sts:ExternalId" = local.account_roles_properties[idx].external_id
+              }
+            }
+          } : {}
+        )
+      ]
+    })
+  ]
 }
 
 data "aws_caller_identity" "current" {}
@@ -55,35 +84,12 @@ data "aws_partition" "current" {}
 
 data "rhcs_info" "current" {}
 
-data "aws_iam_policy_document" "custom_trust_policy" {
-  count = local.account_roles_count
-
-  statement {
-    effect  = "Allow"
-    actions = ["sts:AssumeRole"]
-    principals {
-      type        = local.account_roles_properties[count.index].principal_type
-      identifiers = [local.account_roles_properties[count.index].principal_identifier]
-    }
-
-    dynamic "condition" {
-      # Only set this condition if "trust_policy_external_id" is set
-      for_each = (local.account_roles_properties[count.index].external_id != null) ? [1] : []
-      content {
-        test     = "StringEquals"
-        variable = "sts:ExternalId"
-        values   = [local.account_roles_properties[count.index].external_id]
-      }
-    }
-  }
-}
-
 resource "aws_iam_role" "account_role" {
   count                = local.account_roles_count
   name                 = substr("${local.account_role_prefix_valid}-${local.account_roles_properties[count.index].role_name}-Role", 0, 64)
   permissions_boundary = var.permissions_boundary
   path                 = local.path
-  assume_role_policy   = data.aws_iam_policy_document.custom_trust_policy[count.index].json
+  assume_role_policy   = local.custom_trust_policy_json[count.index]
 
   tags = merge(var.tags, {
     red-hat-managed       = true
@@ -165,6 +171,6 @@ resource "time_sleep" "account_iam_resources_wait" {
     account_role_prefix           = local.account_role_prefix_valid
     path                          = local.path
     shared_vpc_policy_attachments = local.route53_shared_role_arn != "" && local.vpce_shared_role_arn != "" ? jsonencode([aws_iam_role_policy_attachment.route53_policy_installer_account_role_attachment[0].policy_arn, aws_iam_role_policy_attachment.vpce_policy_installer_account_role_attachment[0].policy_arn]) : jsonencode([])
-    trust_policy_external_id      = var.trust_policy_external_id
+    trust_policy_external_id      = local.trust_policy_external_id
   }
 }
diff --git a/modules/account-iam-resources/outputs.tf b/modules/account-iam-resources/outputs.tf
@@ -25,3 +25,4 @@ output "trust_policy_external_id" {
   value       = time_sleep.account_iam_resources_wait.triggers["trust_policy_external_id"]
   description = "External ID for trust policy condition in account roles"
 }
+
diff --git a/modules/account-iam-resources/tests/trust_policy_external_id.tftest.hcl b/modules/account-iam-resources/tests/trust_policy_external_id.tftest.hcl
@@ -0,0 +1,208 @@
+// Copyright Red Hat
+// SPDX-License-Identifier: Apache-2.0
+
+mock_provider "aws" {
+  alias = "default"
+
+  mock_data "aws_partition" {
+    defaults = {
+      partition = "aws"
+    }
+  }
+
+  mock_data "aws_caller_identity" {
+    defaults = {
+      account_id = "123456789012"
+    }
+  }
+
+  mock_resource "aws_iam_role" {
+    defaults = {
+      arn  = "arn:aws:iam::123456789012:role/mock"
+      id   = "mock-role-id"
+      name = "mock-role"
+    }
+  }
+
+  mock_resource "aws_iam_role_policy_attachment" {
+    defaults = {
+      id = "mock-attachment-id"
+    }
+  }
+}
+
+mock_provider "rhcs" {
+  alias = "default"
+
+  mock_data "rhcs_hcp_policies" {
+    defaults = {
+      account_role_policies = {
+        sts_support_rh_sre_role = "arn:aws:iam::999999999999:role/RH-SRE-Support"
+      }
+    }
+  }
+
+  mock_data "rhcs_info" {
+    defaults = {
+      ocm_aws_account_id = "999999999999"
+    }
+  }
+}
+
+mock_provider "time" {
+  alias = "default"
+
+  mock_resource "time_sleep" {
+    defaults = {
+      id = "mock-sleep"
+    }
+  }
+}
+
+mock_provider "random" {
+  alias = "default"
+}
+
+variables {
+  account_role_prefix = "tf-test-acc"
+}
+
+run "null_external_id_omits_condition" {
+  command = plan
+
+  providers = {
+    aws    = aws.default
+    rhcs   = rhcs.default
+    time   = time.default
+    random = random.default
+  }
+
+  variables {
+    trust_policy_external_id = null
+  }
+
+  assert {
+    condition     = !strcontains(aws_iam_role.account_role[0].assume_role_policy, "sts:ExternalId")
+    error_message = "Installer trust policy must not include sts:ExternalId when trust_policy_external_id is null."
+  }
+
+  assert {
+    condition     = !strcontains(aws_iam_role.account_role[1].assume_role_policy, "sts:ExternalId")
+    error_message = "Support trust policy must not include sts:ExternalId when trust_policy_external_id is null."
+  }
+}
+
+run "empty_external_id_rejected" {
+  command = plan
+
+  providers = {
+    aws    = aws.default
+    rhcs   = rhcs.default
+    time   = time.default
+    random = random.default
+  }
+
+  variables {
+    trust_policy_external_id = ""
+  }
+
+  expect_failures = [
+    var.trust_policy_external_id,
+  ]
+}
+
+run "whitespace_external_id_rejected" {
+  command = plan
+
+  providers = {
+    aws    = aws.default
+    rhcs   = rhcs.default
+    time   = time.default
+    random = random.default
+  }
+
+  variables {
+    trust_policy_external_id = "   "
+  }
+
+  expect_failures = [
+    var.trust_policy_external_id,
+  ]
+}
+
+run "non_empty_external_id_adds_condition_to_installer_and_support" {
+  command = plan
+
+  providers = {
+    aws    = aws.default
+    rhcs   = rhcs.default
+    time   = time.default
+    random = random.default
+  }
+
+  variables {
+    trust_policy_external_id = "test-external-id-12345"
+  }
+
+  assert {
+    condition = strcontains(
+      aws_iam_role.account_role[0].assume_role_policy,
+      "test-external-id-12345",
+    )
+    error_message = "Installer trust policy must include the configured external ID."
+  }
+
+  assert {
+    condition = strcontains(
+      aws_iam_role.account_role[1].assume_role_policy,
+      "test-external-id-12345",
+    )
+    error_message = "Support trust policy must include the configured external ID."
+  }
+
+  assert {
+    condition = strcontains(
+      aws_iam_role.account_role[0].assume_role_policy,
+      "sts:ExternalId",
+    )
+    error_message = "Installer trust policy must include sts:ExternalId condition key."
+  }
+
+  assert {
+    condition = strcontains(
+      aws_iam_role.account_role[1].assume_role_policy,
+      "sts:ExternalId",
+    )
+    error_message = "Support trust policy must include sts:ExternalId condition key."
+  }
+}
+
+run "worker_never_includes_external_id" {
+  command = plan
+
+  providers = {
+    aws    = aws.default
+    rhcs   = rhcs.default
+    time   = time.default
+    random = random.default
+  }
+
+  variables {
+    trust_policy_external_id = "test-external-id-12345"
+  }
+
+  assert {
+    condition     = !strcontains(aws_iam_role.account_role[2].assume_role_policy, "sts:ExternalId")
+    error_message = "Worker trust policy must never include sts:ExternalId."
+  }
+
+  assert {
+    condition     = strcontains(aws_iam_role.account_role[2].assume_role_policy, "ec2.amazonaws.com")
+    error_message = "Worker role trust policy must trust ec2.amazonaws.com."
+  }
+
+  assert {
+    condition     = length(aws_iam_role.account_role) == 3
+    error_message = "account_iam_resources must create Installer, Support, and Worker roles."
+  }
+}
diff --git a/modules/account-iam-resources/variables.tf b/modules/account-iam-resources/variables.tf
@@ -44,4 +44,9 @@ variable "trust_policy_external_id" {
   type        = string
   default     = null
   description = "External ID for trust policy condition in account roles"
+
+  validation {
+    condition     = var.trust_policy_external_id == null ? true : length(trimspace(var.trust_policy_external_id)) > 0
+    error_message = "trust_policy_external_id must be null or a non-empty, non-whitespace string"
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -25,3 +25,4 @@ output "trust_policy_external_id" {`
`25`	`25`	`value = time_sleep.account_iam_resources_wait.triggers["trust_policy_external_id"]`
`26`	`26`	`description = "External ID for trust policy condition in account roles"`
`27`	`27`	`}`
	`28`	`+`