HYPERFLEET-1570 - feat: add policy for OKE load balancer NSGs - #95
openshift-merge-bot[bot] merged 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change adds an optional OCI IAM policy for OKE load balancer NSG management. The module accepts a compartment ID and tags, creates compartment-scoped permissions, and exposes its policy ID. Root Terraform conditionally creates the module and validates the OKE compartment ID. Documentation and example variables describe enablement and the required Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant TerraformConfiguration
participant OkeLbNsgPolicyModule
participant OCIIdentityPolicy
TerraformConfiguration->>OkeLbNsgPolicyModule: enable policy and pass compartment ID
OkeLbNsgPolicyModule->>OCIIdentityPolicy: create NSG management policy
OCIIdentityPolicy-->>OkeLbNsgPolicyModule: return policy ID
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The policy is conditionally usable with validated inputs; only a minor documentation correction remains. 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
| variable "oke_compartment_id" { | ||
| description = <<-EOT | ||
| OCID of the compartment that will contain the OKE cluster and its VCN. | ||
| No default: required once oke_lb_nsg_policy_enabled is true. | ||
| EOT | ||
| type = string | ||
| default = null | ||
|
|
||
| validation { | ||
| condition = !var.oke_lb_nsg_policy_enabled || var.oke_compartment_id != null | ||
| error_message = "oke_compartment_id is required when oke_lb_nsg_policy_enabled is true." | ||
| } |
There was a problem hiding this comment.
The validation only rejects null, so an empty or whitespace-only oke_compartment_id passes when the policy is enabled and fails later during the OCI operation. Consider validating that the value is non-empty after trimming whitespace.
There was a problem hiding this comment.
Fixed — now validates with trimspace, matching the existing pattern.
| `oke_lb_nsg_policy_enabled` (default `false`) creates the IAM policy the OCI | ||
| cloud controller manager needs to create and manage a dedicated **frontend | ||
| NSG** per `LoadBalancer` service, instead of editing the security list | ||
| Terraform owns for node and control-plane traffic — the decision recorded in | ||
| the architecture repo's | ||
| [ADR 0024](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/adrs/0024-oke-load-balancer-security-nsg.md). | ||
|
|
||
| The CCM authenticates for this specific action as the **cluster resource | ||
| principal** (`request.principal.type = 'cluster'`), not via a worker node's | ||
| instance principal — verified live end-to-end against a test OKE cluster on | ||
| 2026-09-21: a `LoadBalancer` service annotated | ||
| `oci.oraclecloud.com/security-rule-management-mode: "NSG"` provisioned | ||
| successfully, the CCM created a frontend NSG with the correct ingress rule, | ||
| and the shared security list was never touched. | ||
|
|
||
| This is not yet wired into this stack: no OKE cluster or VCN exists here yet | ||
| ([HYPERFLEET-1525](https://redhat.atlassian.net/browse/HYPERFLEET-1525) is | ||
| still in Backlog), though the IAM mechanism itself is now proven correct. | ||
| Once HYPERFLEET-1525 lands, set `oke_compartment_id` to the compartment | ||
| holding the OKE cluster's VCN and flip `oke_lb_nsg_policy_enabled` to `true`; | ||
| every `LoadBalancer` service manifest in that cluster must then carry the | ||
| `oci.oraclecloud.com/security-rule-management-mode: "NSG"` annotation to | ||
| actually use the frontend NSG this policy authorizes — the policy alone does | ||
| not annotate anything. | ||
|
|
There was a problem hiding this comment.
This feels a bit too detailed for the README. I'd move the live verification details to the PR/ADR and keep this focused on configuration and usage.
There was a problem hiding this comment.
Updated the README to focus on configuration and usage only — moved the verification narrative out (already in the ADR/PR)
| @@ -0,0 +1,4 @@ | |||
| output "policy_id" { | |||
There was a problem hiding this comment.
This output doesnt bubble up in terraform output. We have an existing pattern for flag gated modules you can follow for this output
There was a problem hiding this comment.
Fixed — added oke_lb_nsg_policy_id to terraform/oci/outputs.tf, mirroring the postgresql_id pattern (try(module.X[0].field, null)).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche 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 |
3ccca2e
into
openshift-hyperfleet:main
4 participants
Summary
oke-lb-nsg-policy) letting the OCI cloud controller manager create and manage a frontend NSG perLoadBalancerservice, per architecture ADR 0024network-security-groupsandvirtual-network-familymanagement to the cluster resource principal (request.principal.type = 'cluster')oke_lb_nsg_policy_enabled = false) — no OKE cluster/VCN exists in this stack yetTest plan
terraform validateclean on the new module and the updated root stackterraform fmtcleannsg-testingOKE cluster inhyperfleet-sandbox(rhelcert tenancy): aLoadBalancerservice annotatedoci.oraclecloud.com/security-rule-management-mode: "NSG"provisioned successfully, the CCM created a frontend NSG with the correct ingress rule, and the shared security list stayed empty before and afterLoadBalancerservice removed the frontend NSG automatically (confirmed via 404 onoci network nsg get); the security list remained unchanged