SPLAT-2452: Add SetSecurityGroups IAM permission to master nodes for BYO SG support for AWS NLBs

[mfbonfigli]

Context

As part of the new Bring Your Own Security Groups (BYO SG) for AWS Network Load Balancers (NLBs) feature currently under review in upstream, it is required to add a new elasticloadbalancing:SetSecurityGroups permission to the role used by AWS CCM to interact with AWS APIs.

The elasticloadbalancing:SetSecurityGroups permission is required to enable AWS CCM to change the security groups associated with Network Load Balancers without deleting and recreating the NLB, which is not viable.

Without this permission, the following operations are not possible:

• Changing BYO security groups
• Transition from managed to BYO Security Group
• Transition from BYO Security Group to managed

The new permission is similar to elasticloadbalancing:ApplySecurityGroupsToLoadBalancer which is already present and required to edit BYO Security Groups associated with Classic Load Balancers (CLBs).

The upstream kOps Kubernetes infrastructure provisioning tool has been already updated to add the required permission: kubernetes/kops#18211

References:

• Upstream PR to add the BYO SG feature (currently pending review): feat: Add BYO security group support for NLB kubernetes/cloud-provider-aws#1379
• Upstream PR to kOps to add to the Kubernetes cluster install tool support for the new permission (already merged): Support NLBSecurityGroupMode for AWS Cloud Controller Manager kubernetes/kops#18211

Summary by CodeRabbit

• Bug Fixes
  • Restored missing AWS load balancer security group management permission for cluster master nodes, resolving failures when updating or assigning security groups to load balancers and improving reliability of cluster load balancer operations.

[openshift-ci-robot]

@mfbonfigli: This pull request references SPLAT-2452 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Do Not Merge / Work In Progress

This PR adds the elasticloadbalancing:SetSecurityGroups IAM permission to master nodes, which is required for the BYO Security Groups feature for AWS Network Load Balancers on AWS CCM.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 129c3355-2f53-4dc3-ba47-8402afb2f1dc

📥 Commits

Reviewing files that changed from the base of the PR and between f0c7b3e and 1a7476b.

📒 Files selected for processing (1)

• upi/aws/cloudformation/03_cluster_security.yaml

✅ Files skipped from review due to trivial changes (1)

• upi/aws/cloudformation/03_cluster_security.yaml

---

Walkthrough

Updated the inline IAM policy for the AWS cluster API master role to add the ELB permission elasticloadbalancing:SetSecurityGroups. No other logic, control flow, resources, or role selection were modified.

Changes

Cohort / File(s)	Summary
IAM Policy — Go pkg/infrastructure/aws/clusterapi/iam.go	Added elasticloadbalancing:SetSecurityGroups to the master role's inline IAM policy Action list.
IAM Policy — CloudFormation upi/aws/cloudformation/03_cluster_security.yaml	Added elasticloadbalancing:SetSecurityGroups to the MasterIamRole policy document's ELB-related actions.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 12

✅ Passed checks (12 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding the SetSecurityGroups IAM permission to master nodes for AWS NLB support.
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.
Stable And Deterministic Test Names	✅ Passed	The PR modifies infrastructure and IAM configuration files only, not test files. No Ginkgo test declarations are present in the modified code.
Test Structure And Quality	✅ Passed	The custom check for "Test Structure and Quality" is not applicable to this PR as it contains no Ginkgo test code changes.
Microshift Test Compatibility	✅ Passed	PR does not add any new Ginkgo e2e tests, only modifies IAM policy configurations. Custom check applies only when new tests are introduced.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	This pull request does not add any Ginkgo e2e tests. The changes are purely infrastructure configuration files: one Go source file and one CloudFormation template that add the elasticloadbalancing:SetSecurityGroups action to existing IAM policy documents.
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies only IAM policy documents for AWS master node roles by adding elasticloadbalancing:SetSecurityGroups permission. No Kubernetes manifests, scheduling constraints, or topology-related configurations present.
Ote Binary Stdout Contract	✅ Passed	The OTE Binary Stdout Contract check is not applicable to this PR; changes are only in infrastructure configuration files with no stdout-writing operations.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests found in this PR. Changes are limited to infrastructure configuration files adding IAM permissions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.}

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

[mtulio]

Thanks for submitting PR, I think it makes sense as CCM uses CP instance profile.

PTAL to the detected policies by SREP-3643 and if it would be required too here, looks like some are not included to the master profile as well.

Also for follow up:

• We need to make sure OCP docs is updated with new set of permissions (follow up card from your Epic)
• review/update the UPI assets:

  installer/upi/aws/cloudformation/03_cluster_security.yaml

  Line 489 in 0bd82bc

  MasterIamRole:

• I think we need to review the CAPA IAM too:

  installer/pkg/infrastructure/aws/clusterapi/iam.go

  Lines 98 to 99 in 0bd82bc

  	// createIAMRoles creates the roles used by control-plane and compute nodes.
  	func createIAMRoles(ctx context.Context, infraID string, ic *installconfig.InstallConfig) error {

[mfbonfigli]

Thanks for submitting PR, I think it makes sense as CCM uses CP instance profile.

PTAL to the detected policies by SREP-3643 and if it would be required too here, looks like some are not included to the master profile as well.

Also for follow up:

* We need to make sure OCP docs is updated with new set of permissions (follow up card from your Epic)

* review/update the UPI assets: https://github.com/openshift/installer/blob/0bd82bcf655ac45ef86432b5370d6dbf9e9fedcf/upi/aws/cloudformation/03_cluster_security.yaml#L489

* I think we need to review the CAPA IAM too: https://github.com/openshift/installer/blob/0bd82bcf655ac45ef86432b5370d6dbf9e9fedcf/pkg/infrastructure/aws/clusterapi/iam.go#L98-L99

Thanks for submitting PR, I think it makes sense as CCM uses CP instance profile.

PTAL to the detected policies by SREP-3643 and if it would be required too here, looks like some are not included to the master profile as well.

Also for follow up:

* We need to make sure OCP docs is updated with new set of permissions (follow up card from your Epic)

* review/update the UPI assets: https://github.com/openshift/installer/blob/0bd82bcf655ac45ef86432b5370d6dbf9e9fedcf/upi/aws/cloudformation/03_cluster_security.yaml#L489

* I think we need to review the CAPA IAM too: https://github.com/openshift/installer/blob/0bd82bcf655ac45ef86432b5370d6dbf9e9fedcf/pkg/infrastructure/aws/clusterapi/iam.go#L98-L99

Thanks @mtulio , I updated the PR to include the UPI template. Will work on updating the openshift documentation as a separate task.

[mtulio]

Thanks for addressing the comments.

/lgtm

@mfbonfigli can we trigger e2e-aws workflow with /testwith PRs on downstream?

/hold

Hey Patrick and Thuan, would you mind taking a look? Thanks
FWIW as mentioned in the PR description, this change is required by CCM, which currently is still using the IAM Role/Profile's policy of Control Plane, as this is a core component, and feature is enabled by default when shipped, we can't evaluate conditionals on install-config to group it.

/assign @tthvo @patrickdillon

[mfbonfigli]

/retest

[mfbonfigli]

/test e2e-aws-ovn-heterogeneous

[openshift-ci]

@mfbonfigli: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-heterogeneous	1a7476b	link	false	/test e2e-aws-ovn-heterogeneous

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[tthvo]

/approve

Since the feature is enabled in Default clusters, this change looks good to me 👍 I also cross-referenced with permissions required by CAPA to call ApplySecurityGroupsToLoadBalancer 👇

installer/pkg/asset/installconfig/aws/permissions.go

Line 178 in f043a6d

"elasticloadbalancing:SetSecurityGroups",

As Marco said, we definitely should ensure openshift docs reflect this new permission, especially if the users bring their own IAM roles/profiles to control plane machine pool.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tthvo

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:

• pkg/infrastructure/aws/OWNERS [tthvo]
• upi/aws/OWNERS [tthvo]

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

[tthvo]

@mfbonfigli @mtulio maybe you guys can add the verified label? I'll leave the hold for Patrick to have a look :D

[mfbonfigli]

Thanks for the review @tthvo! will work on a separate PR to update the Openshift Documentation and will coordinate with @mtulio about the label!

[mtulio]

/testwith openshift/installer/main/e2e-aws-ovn openshift/cloud-provider-aws#152 openshift/cluster-cloud-controller-manager-operator#460