SPLAT-2741: Add SetSecurityGroups perms for AWS CCM for BYO SG on AWS NLB by mfbonfigli · Pull Request #8401 · openshift/hypershift

mfbonfigli · 2026-05-04T09:10:02Z

What this PR does / why we need it:

This PR adds the elasticloadbalancing:SetSecurityGroups IAM permission which is required to support the upcoming AWS CCM feature that adds support for BYO Security Groups (SG) on AWS Network Load Balancers (See kubernetes/cloud-provider-aws#1379).

The permission is required by AWS CCM to be able to swap byo security groups and move from managed to BYO SGs and from BYO to managed without deleting and recreating the load balancer. This permission is the Network Load Balancer equivalent of the already present ApplySecurityGroupsToLoadBalancer which however only applies to Classic Load Balancers.

References / related work:

PR to add support for BYO SG on NLBs to AWS CCM: feat: Add BYO security group support for NLB kubernetes/cloud-provider-aws#1379
PR to add SetSecurityGroups IAM permission on Openshift Installer: SPLAT-2452: Add SetSecurityGroups IAM permission to master nodes for BYO SG support for AWS NLBs installer#10512
PR to add SetSecurityGroups IAM permission to ROSA policy: SPLAT-2721: Add SetSecurityGroups IAM permission managed-cluster-config#2727

This work is part of OCPSTRAT-1553.

Which issue(s) this PR fixes:

Fixes #SPLAT-2741

Special notes for your reviewer:

Checklist:

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

Summary by CodeRabbit

New Features
- Enabled SetSecurityGroups for AWS Elastic Load Balancing, including API support and delegate-generation filtering to handle the operation appropriately.
Documentation
- Updated AWS IAM policy examples to include the SetSecurityGroups permission for controller components.

openshift-merge-bot · 2026-05-04T09:10:06Z

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

openshift-ci · 2026-05-04T09:10:07Z

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

openshift-ci-robot · 2026-05-04T09:10:08Z

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

Details

In response to this:

What this PR does / why we need it:

This PR adds the elasticloadbalancing:SetSecurityGroups IAM permission which is required to support the upcoming AWS CCM feature that adds support for BYO Security Groups (SG) on AWS Network Load Balancers (See kubernetes/cloud-provider-aws#1379).

The permission is required by AWS CCM to be able to swap byo security groups and move from managed to BYO SGs and from BYO to managed without deleting and recreating the load balancer. This permission is the Network Load Balancer equivalent of the already present ApplySecurityGroupsToLoadBalancer which however only applies to Classic Load Balancers.

References / related work:

PR to add support for BYO SG on NLBs to AWS CCM: feat: Add BYO security group support for NLB kubernetes/cloud-provider-aws#1379

PR to add SetSecurityGroups IAM permission on Openshift Installer: SPLAT-2452: Add SetSecurityGroups IAM permission to master nodes for BYO SG support for AWS NLBs installer#10512

PR to add SetSecurityGroups IAM permission to ROSA policy: SPLAT-2721: Add SetSecurityGroups IAM permission managed-cluster-config#2727

This work is part of OCPSTRAT-1553.

Which issue(s) this PR fixes:

Fixes #SPLAT-2741

Special notes for your reviewer:

Checklist:

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

coderabbitai · 2026-05-04T09:10:20Z

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: 9f831823-9aac-4c46-8404-8ec7bb149d26

📥 Commits

Reviewing files that changed from the base of the PR and between 726e8f4 and 1fb77b2.

⛔ Files ignored due to path filters (2)

cmd/infra/aws/delegating_client.go is excluded by !cmd/infra/aws/delegating_client.go
docs/content/reference/aggregated-docs.md is excluded by !docs/content/reference/aggregated-docs.md

📒 Files selected for processing (4)

cmd/infra/aws/delegatingclientgenerator/main.go
cmd/infra/aws/iam.go
docs/content/reference/infrastructure/aws.md
support/awsapi/elasticloadbalancingv2.go

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

docs/content/reference/infrastructure/aws.md

📝 Walkthrough

Walkthrough

This change adds support for the AWS Elastic Load Balancing SetSecurityGroups operation. It updates the ELBV2 API surface by adding SetSecurityGroups to the ELBV2API interface, adds the elasticloadbalancing:SetSecurityGroups IAM permission to kube-controller-manager policies (including ROSA-managed inline path), and excludes the SetSecurityGroups operation from delegating-client generation remapping. Documentation showing the required permission was also updated.

Sequence Diagram(s)

sequenceDiagram
    participant KCM as "Kube Controller Manager"
    participant SDK as "ELBV2 SDK Client"
    participant ELB as "AWS ELBv2 API"
    participant IAM as "AWS IAM"

    KCM->>SDK: SetSecurityGroups(ctx, input)
    SDK->>ELB: Call SetSecurityGroups API
    ELB->>IAM: Check elasticloadbalancing:SetSecurityGroups permission
    IAM-->>ELB: Allow
    ELB-->>SDK: SetSecurityGroups response
    SDK-->>KCM: Return result

🚥 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 PR title clearly describes the main change: adding SetSecurityGroups IAM permissions for AWS CCM to support bring-your-own security groups on AWS NLB, which aligns with all file modifications shown in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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	No test files were modified; only production code and documentation files changed in this PR.
Test Structure And Quality	✅ Passed	PR contains standard Go tests using testing.T framework, not Ginkgo-style tests. The custom check targets Ginkgo-specific patterns (Describe, It, BeforeEach, etc.) which are absent, making the check inapplicable.
Microshift Test Compatibility	✅ Passed	This PR does not add any new Ginkgo e2e tests. The changes are limited to infrastructure code files and documentation with no Ginkgo test patterns or imports.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added in this PR. Changes only modify infrastructure configuration, IAM permissions, documentation, and API interfaces.
Topology-Aware Scheduling Compatibility	✅ Passed	This pull request does not introduce topology-aware scheduling constraints; changes consist of infrastructure configuration only.
Ote Binary Stdout Contract	✅ Passed	The PR only modifies the data structure in the adjustAPIs function by adding 'SetSecurityGroups' to the apiRemovals set for 'elasticloadbalancing'. This is a purely data-driven change with no impact on stdout behavior.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	This pull request does not add any new Ginkgo e2e tests. All four modified files are production code and documentation without Ginkgo test patterns.

_{✏️ 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

_{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.}

openshift-ci · 2026-05-04T09:10:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mfbonfigli
Once this PR has been reviewed and has the lgtm label, please assign bryan-cox for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2026-05-04T09:14:37Z

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.22%. Comparing base (4d34484) to head (1fb77b2).
⚠️ Report is 443 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/infra/aws/iam.go	0.00%	3 Missing ⚠️
cmd/infra/aws/delegating_client.go	0.00%	2 Missing ⚠️
cmd/infra/aws/delegatingclientgenerator/main.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8401      +/-   ##
==========================================
- Coverage   37.22%   37.22%   -0.01%     
==========================================
  Files         750      750              
  Lines       91789    91794       +5     
==========================================
  Hits        34168    34168              
- Misses      54981    54986       +5     
  Partials     2640     2640

Files with missing lines	Coverage Δ
cmd/infra/aws/delegatingclientgenerator/main.go	`0.00% <0.00%> (ø)`
cmd/infra/aws/delegating_client.go	`0.00% <0.00%> (ø)`
cmd/infra/aws/iam.go	`28.95% <0.00%> (-0.08%)`	⬇️

Flag	Coverage Δ
cmd-support	`32.06% <0.00%> (-0.01%)`	⬇️
cpo-hostedcontrolplane	`36.45% <ø> (ø)`
cpo-other	`37.73% <ø> (ø)`
hypershift-operator	`47.84% <ø> (ø)`
other	`27.77% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cmd/infra/aws/iam.go`:
- Around line 217-219: The ROSA path still builds ccmPolicyStatement with only
DescribeTargetGroupAttributes and ModifyTargetGroupAttributes, so update the
code that constructs ccmPolicyStatement in the UseROSAManagedPolicies branch to
include the same ELB action(s) added to kubeControllerPolicy (at minimum
"elasticloadbalancing:SetSecurityGroups") so ROSA clusters receive that
permission; locate the ccmPolicyStatement construction and add the missing
action string to the statement array.

🪄 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: bac410c6-3cbe-4d92-b8b0-9eca9dea34a6

📥 Commits

Reviewing files that changed from the base of the PR and between 29053b7 and 726e8f4.

⛔ Files ignored due to path filters (2)

cmd/infra/aws/delegating_client.go is excluded by !cmd/infra/aws/delegating_client.go
docs/content/reference/aggregated-docs.md is excluded by !docs/content/reference/aggregated-docs.md

📒 Files selected for processing (4)

cmd/infra/aws/delegatingclientgenerator/main.go
cmd/infra/aws/iam.go
docs/content/reference/infrastructure/aws.md
support/awsapi/elasticloadbalancingv2.go

…n NLB Adds SetSecurityGroups IAM permission required to support the new AWS CCM feature that adds support for BYO Security Groups on AWS Network Load Balancers.

openshift-ci · 2026-05-11T16:27:15Z

@mfbonfigli: all tests passed!

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.

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

Stale PRs are closed after 21d of inactivity.

If this PR is still relevant, comment to refresh it or remove the stale label.
Mark the PR as fresh by commenting /remove-lifecycle stale.

If this PR is safe to close now please do so with /close.

/lifecycle stale

hypershift-jira-solve-ci · 2026-06-11T11:39:56Z

Test Failure Analysis Complete

Job Information

Prow Job: codecov/patch
Build ID: Check Run 80787557852
PR: #8401 — SPLAT-2741: Add SetSecurityGroups perms for AWS CCM for BYO SG on AWS NLB
Target: Patch coverage gate on openshift/hypershift

Test Failure Analysis

Error

0.00% of diff hit (target 37.22%)
Patch coverage is 0% with 6 lines in your changes missing coverage.

Summary

The codecov/patch check failed because none of the 6 executable lines added by this PR are covered by unit tests, yielding 0% patch coverage against the project target of 37.22%. The PR adds the SetSecurityGroups AWS ELBv2 API permission and delegating client method across three Go files that have little to no pre-existing test coverage. This is a code coverage gap in pre-existing code, not a product bug or CI infrastructure failure. The codecov/patch check is informational and non-blocking for merge.

Root Cause

The failure is caused by pre-existing test coverage debt in the modified files. The PR adds a single new AWS ELBv2 API method (SetSecurityGroups) and its corresponding IAM permission string across three files, none of which have meaningful test coverage:

cmd/infra/aws/delegating_client.go (2 uncovered lines) — New SetSecurityGroups delegating method. This file has 0% test coverage — no tests exist for any of the ~50+ delegating client wrapper methods. The new method follows the exact same one-line delegation pattern as all existing methods (e.g., RegisterTargets, ModifyTargetGroupAttributes).
cmd/infra/aws/iam.go (3 uncovered lines) — Adds "elasticloadbalancing:SetSecurityGroups" to IAM policy JSON strings in two locations (the shared CCM policy and the ROSA/STS inline policy). This file has only ~29% existing coverage; the IAM policy construction functions are not exercised by tests.
cmd/infra/aws/delegatingclientgenerator/main.go (1 uncovered line) — Adds "SetSecurityGroups" to the API exclusion list in the code generator. This file has 0% coverage — it is a build-time code generation tool with no unit tests.

The remaining changed files are correctly excluded from coverage by .codecov.yml:

support/awsapi/elasticloadbalancingv2.go — interface-only, excluded via support/awsapi/*.go
docs/content/reference/*.md — excluded via docs/** and **/*.md patterns

The codecov/patch gate compares patch coverage (0%) against the project-level target (37.22%). Since 0% < 37.22%, the check fails. This is inherited coverage debt, not a deficiency of this specific PR.

Recommendations

This failure is non-blocking. The codecov/patch check is not a required status check for merging into main. The PR can merge without this check passing.
No action required for this PR. The changes are mechanical — adding a single IAM permission string and a one-line delegating wrapper following an established pattern identical to dozens of existing methods. Writing unit tests for these specific lines would provide negligible value.
Long-term coverage improvement (optional): If the team wants to address the underlying debt:
- delegating_client.go could benefit from table-driven tests verifying each delegation method routes to the correct underlying client — this would cover all ~50 methods at once.
- iam.go policy construction functions could be tested by parsing the generated JSON and asserting specific permission entries are present.
- delegatingclientgenerator/main.go is a build tool unlikely to benefit from unit tests.

Evidence

Evidence	Detail
Check conclusion	`failure` — 0.00% patch coverage vs 37.22% target
Uncovered lines	6 total: 3 in `iam.go`, 2 in `delegating_client.go`, 1 in `delegatingclientgenerator/main.go`
`delegating_client.go` existing coverage	0% — no tests for any delegating client method
`delegatingclientgenerator/main.go` existing coverage	0% — build-time tool, no unit tests
`iam.go` existing coverage	~29% — IAM policy JSON construction untested
Codecov ignore patterns	`support/awsapi/.go`, `docs/`, `/.md` correctly exclude interface & doc changes
Project coverage impact	Negligible: 37.22% → 37.22% (-0.01%)
`codecov/project` check	✅ Passed — project-level coverage is unaffected
Codecov report	app.codecov.io/gh/openshift/hypershift/pull/8401

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2026

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 4, 2026

openshift-ci Bot added the do-not-merge/needs-area label May 4, 2026

openshift-ci Bot added the area/platform/aws PR/issue for AWS (AWSPlatform) platform label May 4, 2026

coderabbitai Bot reviewed May 4, 2026

View reviewed changes

Comment thread cmd/infra/aws/iam.go

feat(aws): add SetSecurityGroups permission for BYO security groups o…

1fb77b2

…n NLB Adds SetSecurityGroups IAM permission required to support the new AWS CCM feature that adds support for BYO Security Groups on AWS Network Load Balancers.

mfbonfigli force-pushed the SPLAT-2741_update-hypershift-iam-perm-for-byo-sg-nlb branch from 726e8f4 to 1fb77b2 Compare May 4, 2026 10:34

mfbonfigli marked this pull request as ready for review May 4, 2026 11:45

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2026

openshift-ci Bot requested review from bryan-cox and devguyio May 4, 2026 11:45

mfbonfigli mentioned this pull request May 5, 2026

SPLAT-2746: Add SetSecurityGroups permission to OCP documentation openshift/openshift-docs#111222

Draft

1 task

openshift-ci Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 11, 2026

Conversation

mfbonfigli commented May 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist:

Summary by CodeRabbit

Uh oh!

openshift-merge-bot Bot commented May 4, 2026

Uh oh!

openshift-ci Bot commented May 4, 2026

Uh oh!

openshift-ci-robot commented May 4, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist:

Uh oh!

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

Uh oh!

Walkthrough

Sequence Diagram(s)

Uh oh!

openshift-ci Bot commented May 4, 2026

Uh oh!

codecov Bot commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci Bot commented May 11, 2026

Uh oh!

openshift-ci Bot commented Jun 11, 2026

Uh oh!

hypershift-jira-solve-ci Bot commented Jun 11, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Failure Analysis Complete

Job Information

Test Failure Analysis

Error

Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mfbonfigli commented May 4, 2026 •

edited by coderabbitai Bot

Loading

openshift-ci-robot commented May 4, 2026 •

edited by openshift-ci Bot

Loading

coderabbitai Bot commented May 4, 2026 •

edited

Loading

codecov Bot commented May 4, 2026 •

edited

Loading

hypershift-jira-solve-ci Bot commented Jun 11, 2026 •

edited by openshift-ci Bot

Loading