ROSAENG-57757 | fix: apply trust_policy_external_id to support role trust policy by michaelryanmcneill · Pull Request #160 · terraform-redhat/terraform-rhcs-rosa-hcp

michaelryanmcneill · 2026-06-11T01:29:50Z

PR Summary

Fixes modules/account-iam-resources so trust_policy_external_id is injected into the support role trust policy (not just installer) and empty strings are treated as unset.

Detailed Description of the Issue

The HCP account-iam-resources module already exposed trust_policy_external_id, but only the installer role received the sts:ExternalId trust policy condition. The support role had external_id = null, so cluster create validation in the RHCS provider (which requires the external ID in both installer and support policies) would fail even when users configured the module correctly.

This change aligns HCP module behavior with ROSA product expectations and with the new Classic module implementation.

Related Issues and PRs

Jira: ROSAENG-57757
Related PR(s):
- terraform-provider-rhcs: sts.trust_policy_external_id on cluster resources and create-time validation
- terraform-rhcs-rosa-classic: new trust_policy_external_id support in account-iam-resources

Type of Change

Previous Behavior

trust_policy_external_id applied to installer role trust policy only.
Support role trust policy never received sts:ExternalId when the variable was set.
Empty string passed through as-is (could produce invalid or unintended conditions).

Behavior After This Change

local.trust_policy_external_id normalizes null and "" to unset.
Installer and support roles use local.trust_policy_external_id for trust policy conditions.
Worker role unchanged (no external ID).
time_sleep triggers use normalized local value.

Example wiring

module "account_iam_resources" {
  source  = "terraform-redhat/rosa-hcp/rhcs//modules/account-iam-resources"
  version = ">=1.6.3"

  account_role_prefix      = var.account_role_prefix
  trust_policy_external_id = var.trust_policy_external_id
}

resource "rhcs_cluster_rosa_hcp" "example" {
  sts = {
    trust_policy_external_id = var.trust_policy_external_id
    # role_arn, support_role_arn, ...
  }
}

How to Test (Step-by-Step)

Preconditions

AWS credentials for target account
RHCS provider configured
Terraform >= 1.0

Test Steps

Format module:

cd modules/account-iam-resources
terraform fmt

Apply module with trust_policy_external_id = "test-external-id-123".
In AWS IAM, verify both HCP-ROSA-Installer and HCP-ROSA-Support role trust policies include sts:ExternalId with the same value.
Verify HCP-ROSA-Worker role trust policy has no external ID condition.
Re-apply with trust_policy_external_id = null; confirm conditions are removed on new role creation (or validate on fresh prefix).

Expected Results

Installer and support trust policies match for external ID when variable is set.
RHCS provider create-time validation passes when cluster uses the same value.
Worker role unaffected.

Proof of the Fix

Screenshots: IAM trust policies for installer and support showing matching sts:ExternalId
Videos: N/A
Logs/CLI output: terraform apply output
Other artifacts: N/A

Breaking Changes

No breaking changes
Yes, this PR introduces a breaking change (describe impact and migration plan below)

Breaking Change Details / Migration Plan

N/A for new deployments.

Note for existing deployments: clusters created with external ID on installer only may need support role trust policy updated (re-apply module or manual IAM update) before RHCS provider validation will pass when sts.trust_policy_external_id is set on the cluster.

Developer Verification Checklist

Summary by CodeRabbit

Release Notes

Bug Fixes
- Normalized trust_policy_external_id so empty/unset values behave as null, omitting the sts:ExternalId condition for installer/support roles while keeping worker free of sts:ExternalId.
New Features
- Added an output that exposes the rendered trust-policy JSON used for account roles.
Validation & Tests
- Added input validation to reject empty/whitespace external IDs.
- Expanded tests to verify omit/include behavior across roles and expected failure cases.
Documentation
- Updated module docs to reflect the new trust-policy output.

coderabbitai · 2026-06-11T01:29:58Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: a858606e-0088-4589-b9ef-794699e91c5a

📥 Commits

Reviewing files that changed from the base of the PR and between d94f4c7 and 3b2c533.

📒 Files selected for processing (5)

modules/account-iam-resources/README.md
modules/account-iam-resources/main.tf
modules/account-iam-resources/outputs.tf
modules/account-iam-resources/tests/trust_policy_external_id.tftest.hcl
modules/account-iam-resources/variables.tf

💤 Files with no reviewable changes (1)

modules/account-iam-resources/README.md

🚧 Files skipped from review as they are similar to previous changes (4)

modules/account-iam-resources/variables.tf
modules/account-iam-resources/outputs.tf
modules/account-iam-resources/tests/trust_policy_external_id.tftest.hcl
modules/account-iam-resources/main.tf

Walkthrough

The account-iam-resources module now normalizes trust_policy_external_id, generates trust policy JSON conditionally including sts:ExternalId, updates role consumption and outputs, and adds validation plus plan coverage for null, empty, whitespace, and non-empty values.

Changes

Trust policy external ID normalization and conditional ExternalId inclusion

Layer / File(s)	Summary
Input validation and normalization `modules/account-iam-resources/variables.tf`, `modules/account-iam-resources/main.tf`	`trust_policy_external_id` now rejects empty or whitespace-only strings while allowing `null`. A normalized local value replaces raw variable use in the installer and support role `external_id` fields.
Trust policy JSON generation with conditional ExternalId `modules/account-iam-resources/main.tf`	Per-role assume-role policy JSON is now built with `jsonencode` and `merge`, adding the `sts:ExternalId` condition only when a role’s `external_id` is set.
Resource consumption and trigger update `modules/account-iam-resources/main.tf`	`aws_iam_role.account_role` consumes the generated trust policy JSON directly, and the wait resource trigger uses the normalized external ID local.
Output exposure and documentation `modules/account-iam-resources/outputs.tf`, `modules/account-iam-resources/README.md`	A new output exposes the rendered trust policy JSON, and the autogenerated module docs remove the deprecated data source entry.
Comprehensive test coverage `modules/account-iam-resources/tests/trust_policy_external_id.tftest.hcl`	New Terraform tests cover null, empty, whitespace, and non-empty external ID cases, plus worker-role trust policy assertions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: willkutler, BraeTroutman

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Pr Checklist Claims Vs Evidence (Generic)	⚠️ Warning	The fix claim is supported, but the checked 'No breaking changes' claim is contradicted by new validation that rejects whitespace-only external IDs.	Treat this as a breaking validation change, or uncheck 'No breaking changes' if that restriction is intended.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: applying trust_policy_external_id to the support role trust policy.
Description check	✅ Passed	The description follows the template well and includes the required summary, issue context, links, testing, results, and change notes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-06-11T01:30:01Z

Hi @michaelryanmcneill. Thanks for your PR.

I'm waiting for a terraform-redhat member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

michaelryanmcneill · 2026-06-11T01:31:43Z

/hold

michaelryanmcneill · 2026-06-11T01:38:31Z

Working on additional manual testing before submitting for review.

michaelryanmcneill · 2026-06-11T13:00:00Z

@coderabbitai review

coderabbitai · 2026-06-11T13:00:08Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

olucasfreitas · 2026-06-11T13:14:37Z

/ok-to-test

olucasfreitas · 2026-06-11T13:16:22Z

/ok-to-test

michaelryanmcneill · 2026-06-11T13:27:00Z

/retest

michaelryanmcneill · 2026-06-11T19:04:10Z

Test results:

==> Verifying IAM trust policies (scenario=none, prefix=r786-hcp-none)
==> Roles: installer=r786-hcp-none-HCP-ROSA-Installer-Role, support=r786-hcp-none-HCP-ROSA-Support-Role, worker=r786-hcp-none-HCP-ROSA-Worker-Role
PASS: Installer has no sts:ExternalId
PASS: Support has no sts:ExternalId
PASS: Worker has no sts:ExternalId
PASS: Worker trusts ec2.amazonaws.com
PASS: Module output trust_policy_external_id unset

Installer trust policy:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::710019948333:role/RH-Managed-OpenShift-Installer"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}
Support trust policy:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::710019948333:role/RH-Technical-Support-14540493"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}

==> Verifying IAM trust policies (scenario=set, prefix=r786-hcp-set)
==> Roles: installer=r786-hcp-set-HCP-ROSA-Installer-Role, support=r786-hcp-set-HCP-ROSA-Support-Role, worker=r786-hcp-set-HCP-ROSA-Worker-Role
PASS: Installer has sts:ExternalId=test-external-id-12345
PASS: Support has sts:ExternalId=test-external-id-12345
PASS: Worker has no sts:ExternalId
PASS: Worker trusts ec2.amazonaws.com
PASS: Module output trust_policy_external_id=test-external-id-12345

Installer trust policy:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::710019948333:role/RH-Managed-OpenShift-Installer"
      },
      "Action": "sts:AssumeRole",
      "Condition": {
        "StringEquals": {
          "sts:ExternalId": "test-external-id-12345"
        }
      }
    }
  ]
}
Support trust policy:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::710019948333:role/RH-Technical-Support-14540493"
      },
      "Action": "sts:AssumeRole",
      "Condition": {
        "StringEquals": {
          "sts:ExternalId": "test-external-id-12345"
        }
      }
    }
  ]
}

==> Verifying IAM trust policies (scenario=empty, prefix=r786-hcp-empty)
==> Roles: installer=r786-hcp-empty-HCP-ROSA-Installer-Role, support=r786-hcp-empty-HCP-ROSA-Support-Role, worker=r786-hcp-empty-HCP-ROSA-Worker-Role
PASS: Installer has no sts:ExternalId
PASS: Support has no sts:ExternalId
PASS: Worker has no sts:ExternalId
PASS: Worker trusts ec2.amazonaws.com
PASS: Module output trust_policy_external_id unset

Installer trust policy:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::710019948333:role/RH-Managed-OpenShift-Installer"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}
Support trust policy:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::710019948333:role/RH-Technical-Support-14540493"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}

coderabbitai · 2026-06-11T19:56:28Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

michaelryanmcneill · 2026-06-11T22:13:35Z

@coderabbitai review

coderabbitai · 2026-06-11T22:13:42Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

michaelryanmcneill · 2026-06-12T15:45:51Z

@coderabbitai review

coderabbitai · 2026-06-12T15:45:59Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

michaelryanmcneill · 2026-06-12T15:55:49Z

As discussed, since the CI jobs are a known issue, this is ready for final review @olucasfreitas.

michaelryanmcneill · 2026-06-12T15:56:47Z

/unhold

michaelryanmcneill · 2026-06-17T18:26:19Z

/retest-required

michaelryanmcneill · 2026-06-17T19:54:23Z

/test rosa-hcp-public
/test rosa-hcp-private

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modules/account-iam-resources/variables.tf`:
- Around line 48-50: The validation condition for `trust_policy_external_id`
uses the `||` operator which does not short-circuit in Terraform 1.5.7, causing
`trimspace()` to be evaluated on a null value and fail. Replace the logical OR
approach with a conditional operator that first checks if the variable is null
before attempting to call `trimspace()` on it, ensuring that string functions
are only evaluated when the variable contains an actual string value.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c4ddfd74-5689-42bf-977a-a1086b846ecf

📥 Commits

Reviewing files that changed from the base of the PR and between 08ac68f and 9d751c8.

📒 Files selected for processing (5)

modules/account-iam-resources/README.md
modules/account-iam-resources/main.tf
modules/account-iam-resources/outputs.tf
modules/account-iam-resources/tests/trust_policy_external_id.tftest.hcl
modules/account-iam-resources/variables.tf

olucasfreitas · 2026-06-23T18:27:18Z

/ok-to-test

…rust policy Align support account role with installer when trust_policy_external_id is set and normalize empty strings to unset for external ID injection. Part of ROSA-786. Signed-off-by: michaelryanmcneill <michael@michaelryanmcneill.com>

michaelryanmcneill · 2026-07-02T02:04:28Z

@coderabbitai resume
@coderabbitai review

coderabbitai · 2026-07-02T02:04:42Z

@michaelryanmcneill Resuming automatic reviews and kicking off a fresh review now.

✅ Action performed

Reviews resumed.

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

michaelryanmcneill · 2026-07-02T03:11:39Z

/test rosa-hcp-private

olucasfreitas · 2026-07-02T13:41:19Z

/lgtm
/approve
/override ci/prow/security

openshift-ci · 2026-07-02T13:41:26Z

@olucasfreitas: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

ci/prow/security

Only the following failed contexts/checkruns were expected:

CodeRabbit
ci/prow/images
ci/prow/pre-push-checks
ci/prow/rosa-hcp-private
ci/prow/rosa-hcp-public
ci/prow/security-check
pull-ci-terraform-redhat-terraform-rhcs-rosa-hcp-main-images
pull-ci-terraform-redhat-terraform-rhcs-rosa-hcp-main-pre-push-checks
pull-ci-terraform-redhat-terraform-rhcs-rosa-hcp-main-rosa-hcp-private
pull-ci-terraform-redhat-terraform-rhcs-rosa-hcp-main-rosa-hcp-public
pull-ci-terraform-redhat-terraform-rhcs-rosa-hcp-main-security-check
tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

Details

In response to this:

/lgtm
/approve
/override ci/prow/security

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2026-07-02T13:41:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michaelryanmcneill, olucasfreitas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [olucasfreitas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

olucasfreitas · 2026-07-02T13:41:52Z

/ok-to-test

openshift-ci Bot added the dco-signoff: yes label Jun 11, 2026

openshift-ci Bot requested review from BraeTroutman and willkutler June 11, 2026 01:29

openshift-ci Bot added the needs-ok-to-test label Jun 11, 2026

michaelryanmcneill mentioned this pull request Jun 11, 2026

ROSAENG-57757 | feat: add trust_policy_external_id to account-iam-resources terraform-redhat/terraform-rhcs-rosa-classic#149

Merged

21 tasks

openshift-ci Bot added the do-not-merge/hold label Jun 11, 2026

michaelryanmcneill mentioned this pull request Jun 11, 2026

ROSAENG-57757 | feat(sts): add trust_policy_external_id for ROSA Classic and HCP terraform-redhat/terraform-provider-rhcs#1190

Merged

23 tasks

openshift-ci Bot added ok-to-test and removed needs-ok-to-test labels Jun 11, 2026

openshift-ci Bot removed the do-not-merge/hold label Jun 12, 2026

michaelryanmcneill force-pushed the ROSA-786 branch from a2a5b00 to 08ac68f Compare June 17, 2026 18:26

michaelryanmcneill force-pushed the ROSA-786 branch 2 times, most recently from 9d751c8 to d94f4c7 Compare June 17, 2026 20:16

coderabbitai Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread modules/account-iam-resources/variables.tf

michaelryanmcneill force-pushed the ROSA-786 branch 2 times, most recently from 19f4dce to ba89400 Compare June 22, 2026 12:58

olucasfreitas reviewed Jun 22, 2026

View reviewed changes

Comment thread modules/account-iam-resources/outputs.tf Outdated

Comment thread modules/account-iam-resources/main.tf

michaelryanmcneill force-pushed the ROSA-786 branch 2 times, most recently from f00109a to ffcaf25 Compare June 23, 2026 13:34

michaelryanmcneill force-pushed the ROSA-786 branch from ffcaf25 to ebc382b Compare July 2, 2026 01:56

michaelryanmcneill force-pushed the ROSA-786 branch from ebc382b to 3b2c533 Compare July 2, 2026 01:59

openshift-ci Bot assigned olucasfreitas Jul 2, 2026

openshift-ci Bot added the lgtm label Jul 2, 2026

openshift-ci Bot added the approved label Jul 2, 2026

openshift-merge-bot Bot merged commit 1ca111a into terraform-redhat:main Jul 2, 2026
12 checks passed

Uh oh!

Conversation

michaelryanmcneill commented Jun 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Detailed Description of the Issue

Related Issues and PRs

Type of Change

Previous Behavior

Behavior After This Change

Example wiring

How to Test (Step-by-Step)

Preconditions

Test Steps

Expected Results

Proof of the Fix

Breaking Changes

Breaking Change Details / Migration Plan

Developer Verification Checklist

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

openshift-ci Bot commented Jun 11, 2026

Uh oh!

michaelryanmcneill commented Jun 11, 2026

Uh oh!

michaelryanmcneill commented Jun 11, 2026

Uh oh!

michaelryanmcneill commented Jun 11, 2026

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olucasfreitas commented Jun 11, 2026

Uh oh!

olucasfreitas commented Jun 11, 2026

Uh oh!

michaelryanmcneill commented Jun 11, 2026

Uh oh!

michaelryanmcneill commented Jun 11, 2026

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelryanmcneill commented Jun 11, 2026

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelryanmcneill commented Jun 12, 2026

Uh oh!

coderabbitai Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelryanmcneill commented Jun 12, 2026

Uh oh!

michaelryanmcneill commented Jun 12, 2026

Uh oh!

michaelryanmcneill commented Jun 17, 2026

Uh oh!

michaelryanmcneill commented Jun 17, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olucasfreitas commented Jun 23, 2026

Uh oh!

michaelryanmcneill commented Jul 2, 2026

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

michaelryanmcneill commented Jun 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 12, 2026 •

edited

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading