ROSAENG-6808 | feat: add OCM role submodule with required ROSA CLI tags#154
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughAdds an OCM IAM role Terraform module with input validation, conditional IAM policies, role linking, outputs, comprehensive tests, module/docs, and a runnable example with version constraints. ChangesOCM Role Module and Example
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
olucasfreitas
force-pushed
the
ROSAENG-6808
branch
from
e7354d6 to
97cbc80
Compare
openshift-ci
Bot
added
dco-signoff: yes
and removed
dco-signoff: no
needs-rebase
labels
olucasfreitas
force-pushed
the
ROSAENG-6808
branch
2 times, most recently
from
569daf9 to
47c873a
Compare
robpblake
left a comment
robpblake
left a comment
There was a problem hiding this comment.
@olucasfreitas You haven't included the ability for customers to use the --no-console option here. That is a critical requirement
robpblake
left a comment
robpblake
left a comment
There was a problem hiding this comment.
Minor comments @olucasfreitas .
Have you tested that this creates and successfully links the various OCM Role profiles?
olucasfreitas
force-pushed
the
ROSAENG-6808
branch
2 times, most recently
from
6ea1d78 to
ac082ef
Compare
|
This looks good to me, but I'll defer to @amandahla for final sign-off. |
|
/approve The security-check can be ignored, I need to fix this one, sorry for the noise. |
olucasfreitas
force-pushed
the
ROSAENG-6808
branch
from
ac082ef to
a01e04b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/ocm-role/tests/ocm_role.tftest.hcl`:
- Around line 98-165: Add a new test run that exercises the branch where
create_link = false: duplicate the existing "standard_ocm_role_plan" run (or
create a new run name) but set variables { ocm_role_prefix = "ManagedOpenShift"
create_link = false } and add an assert that
length(rhcs_rosa_ocm_role_link.this) == 0 (with an appropriate error_message).
Ensure the new run keeps necessary providers and other asserts unchanged except
remove or adjust the existing assert that expects
rhcs_rosa_ocm_role_link.this[0].id so both link-present and link-absent
scenarios are covered.
🪄 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: 74d78314-cc1b-45fc-ba52-8b33ae1a146f
📒 Files selected for processing (11)
examples/ocm-role/README.mdexamples/ocm-role/main.tfexamples/ocm-role/outputs.tfexamples/ocm-role/variables.tfexamples/ocm-role/versions.tfmodules/ocm-role/README.mdmodules/ocm-role/main.tfmodules/ocm-role/outputs.tfmodules/ocm-role/tests/ocm_role.tftest.hclmodules/ocm-role/variables.tfmodules/ocm-role/versions.tf
✅ Files skipped from review due to trivial changes (3)
- examples/ocm-role/README.md
- examples/ocm-role/versions.tf
- modules/ocm-role/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/ocm-role/main.tf
- examples/ocm-role/outputs.tf
- modules/ocm-role/outputs.tf
- modules/ocm-role/versions.tf
- modules/ocm-role/variables.tf
- modules/ocm-role/main.tf
| # Standard OCM role (default profile) -- plans successfully with correct tags, naming, and link. | ||
| run "standard_ocm_role_plan" { | ||
| command = plan | ||
|
|
||
| providers = { | ||
| rhcs = rhcs.prod | ||
| aws = aws.default | ||
| } | ||
|
|
||
| variables { | ||
| ocm_role_prefix = "ManagedOpenShift" | ||
| } | ||
|
|
||
| assert { | ||
| condition = aws_iam_role.ocm_role.name == "ManagedOpenShift-OCM-Role-orgext123" | ||
| error_message = "Expected OCM role name to follow the ROSA CLI naming pattern." | ||
| } | ||
|
|
||
| assert { | ||
| condition = aws_iam_role.ocm_role.tags["red-hat-managed"] == "true" | ||
| error_message = "Expected red-hat-managed tag to be true." | ||
| } | ||
|
|
||
| assert { | ||
| condition = aws_iam_role.ocm_role.tags["rosa_role_prefix"] == "ManagedOpenShift" | ||
| error_message = "Expected rosa_role_prefix tag to match the prefix variable." | ||
| } | ||
|
|
||
| assert { | ||
| condition = aws_iam_role.ocm_role.tags["rosa_role_type"] == "OCM" | ||
| error_message = "Expected rosa_role_type tag to be OCM." | ||
| } | ||
|
|
||
| assert { | ||
| condition = aws_iam_role.ocm_role.tags["rosa_environment"] == "production" | ||
| error_message = "Expected rosa_environment tag to default to production." | ||
| } | ||
|
|
||
| assert { | ||
| condition = !contains(keys(aws_iam_role.ocm_role.tags), "rosa_admin_role") | ||
| error_message = "Standard role must not have rosa_admin_role tag." | ||
| } | ||
|
|
||
| assert { | ||
| condition = !contains(keys(aws_iam_role.ocm_role.tags), "rosa_no_console_role") | ||
| error_message = "Standard role must not have rosa_no_console_role tag." | ||
| } | ||
|
|
||
| assert { | ||
| condition = length(aws_iam_policy.standard_permission_policy) == 1 | ||
| error_message = "Standard role must create the standard permission policy." | ||
| } | ||
|
|
||
| assert { | ||
| condition = length(aws_iam_policy.ocm_admin_permission_policy) == 0 | ||
| error_message = "Standard role must not create the admin permission policy." | ||
| } | ||
|
|
||
| assert { | ||
| condition = length(aws_iam_policy.ocm_no_console_permission_policy) == 0 | ||
| error_message = "Standard role must not create the no-console permission policy." | ||
| } | ||
|
|
||
| assert { | ||
| condition = rhcs_rosa_ocm_role_link.this[0].id == "ocm-link-id" | ||
| error_message = "Expected the module to create the OCM-side link resource." | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for create_link = false.
The module has a create_link boolean variable that conditionally creates the rhcs_rosa_ocm_role_link resource. The current test suite only covers the default create_link = true case (line 162 asserts the link resource exists). As per coding guidelines, when module behavior branches on boolean variables (e.g., link creation), prefer multiple separated test scenarios to cover both true/false outcomes.
Add a test run that sets create_link = false and asserts that length(rhcs_rosa_ocm_role_link.this) == 0.
🧪 Suggested test case for create_link = false
+# OCM role without creating the OCM-side link.
+run "ocm_role_without_link_plan" {
+ command = plan
+
+ providers = {
+ rhcs = rhcs.prod
+ aws = aws.default
+ }
+
+ variables {
+ ocm_role_prefix = "ManagedOpenShift"
+ create_link = false
+ }
+
+ assert {
+ condition = aws_iam_role.ocm_role.name == "ManagedOpenShift-OCM-Role-orgext123"
+ error_message = "Expected OCM role to be created even when link is not created."
+ }
+
+ assert {
+ condition = length(rhcs_rosa_ocm_role_link.this) == 0
+ error_message = "Expected no OCM link resource when create_link is false."
+ }
+}🤖 Prompt for 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.
In `@modules/ocm-role/tests/ocm_role.tftest.hcl` around lines 98 - 165, Add a new
test run that exercises the branch where create_link = false: duplicate the
existing "standard_ocm_role_plan" run (or create a new run name) but set
variables { ocm_role_prefix = "ManagedOpenShift" create_link = false } and add
an assert that length(rhcs_rosa_ocm_role_link.this) == 0 (with an appropriate
error_message). Ensure the new run keeps necessary providers and other asserts
unchanged except remove or adjust the existing assert that expects
rhcs_rosa_ocm_role_link.this[0].id so both link-present and link-absent
scenarios are covered.
Source: Coding guidelines
|
/approve |
|
@amandahla: Overrode contexts on behalf of amandahla: ci/prow/security-check DetailsIn response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, olucasfreitas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@olucasfreitas: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
Add modules/ocm-role/ that creates the AWS IAM OCM role with the tags the ROSA CLI applies (red-hat-managed, rosa_role_prefix, rosa_role_type, rosa_environment, and conditional rosa_admin_role). Permission policies are read from the rhcs_hcp_policies data source using the policy IDs confirmed in terraform-provider-rhcs PR #1156. The submodule outputs role_arn for composition with the provider's rhcs_rosa_ocm_role_link resource. An example and tests are included. Signed-off-by: lufreita <lufreita@redhat.com>
olucasfreitas
force-pushed
the
ROSAENG-6808
branch
from
a01e04b to
6bc8147
Compare
|
/override ci/prow/rosa-hcp-public |
|
@olucasfreitas: Overrode contexts on behalf of olucasfreitas: ci/prow/rosa-hcp-private, ci/prow/rosa-hcp-public DetailsIn response to this:
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. |
|
/lgtm |
openshift-merge-bot
Bot
merged commit 33c9f18
into
terraform-redhat:main
4 participants
PR Summary
Add
modules/ocm-role/submodule that creates the AWS IAM OCM role with the tags required by the ROSA CLI, and an example showing composition withrhcs_rosa_ocm_role_link.Detailed Description of the Issue
The ROSA CLI tags OCM IAM roles with
red-hat-managed,rosa_role_prefix,rosa_role_type,rosa_environment, and conditionallyrosa_admin_role. The terraform-rhcs-rosa-hcp module had no OCM role creation path, so customers using Terraform-only workflows had to tag the role manually or risk the role not being recognized by ROSA tooling.This PR adds a standalone
modules/ocm-role/submodule that creates the OCM IAM role with the correct tags, builds the trust policy trustingRH-Managed-OpenShift-Installerin the OCM AWS account (withsts:ExternalIdfor the org ID), and attaches the appropriate permission policy (standard or admin) fromdata.rhcs_hcp_policies.ocm_role_policies.The submodule is not wired into root
main.tfbecause the OCM role is org-level (one per AWS account per organization), not cluster-level. Instead, the example atexamples/ocm-role/shows how to compose the submodule withrhcs_rosa_ocm_role_linkfor the full flow.Related Issues and PRs
rhcs_rosa_ocm_role_linkandocm_role_policies)Type of Change
Previous Behavior
The HCP module had no OCM role creation path. Customers had to create and tag the OCM IAM role manually or via the ROSA CLI.
Behavior After This Change
Customers can call
modules/ocm-role/to create a compliant OCM IAM role with all required ROSA CLI-parity tags, then compose withrhcs_rosa_ocm_role_linkto complete the OCM-side link.Tags applied to the role match the ROSA CLI contract:
red-hat-managed=truerosa_role_prefix= user-specified prefixrosa_role_type=ocmrosa_environment= OCM environmentrosa_admin_role=true(only whenadmin = true)How to Test (Step-by-Step)
Preconditions
Terraform >= 1.0 installed. No live credentials needed for the mock-based tests.
Test Steps
cd modules/ocm-role && terraform init -backend=false && terraform testterraform fmt -check -recursive modules/ocm-role/ examples/ocm-role/cd examples/ocm-role && terraform init -backend=false && terraform validatemake terraform-docs && make verify-genExpected Results
All 6 tests pass (standard role, admin role, custom environment, invalid environment, prefix too long, prefix invalid chars). Format check and validate pass. Docs are up to date.
Proof of the Fix
terraform test— 6 passed, 0 failedREADME.mdfor both submodule and exampleBreaking Changes
Breaking Change Details / Migration Plan
N/A
Developer Verification Checklist
rhcsdata sources (rhcs_hcp_policies,rhcs_info) alongside AWS resources. Official docs linked: Understanding OCM role and User role for ROSA.[JIRA-TICKET] | [TYPE]: <MESSAGE>.make verifypasses.make verify-genpasses.make terraform-docs).Risks / Follow-ups
rosa_no_console_role) is deferred until the tag contract is confirmed in the ROSA CLI.rhcs >= 1.7.6(provider withocm_role_policiessupport). Rootversions.tfalready declares this floor.Summary by CodeRabbit
New Features
Documentation
Examples
Tests