fix(cpo): diff SG permissions before AuthorizeSecurityGroupIngress

[sdminonne]

Summary

This is a follow-up to #8768, which fixed the main source of CloudTrail spam by adding diffPermissions to the awsprivatelink controller's reconcileAWSEndpointSecurityGroup (the path running every 5 minutes on port 443).

This PR addresses the remaining blind-upsert path in createAWSDefaultSecurityGroup:

• Moves permission-diff logic (diffPermissions, isPermissionPresent, equalIPRanges) from the awsprivatelink controller to support/awsutil/sg.go as shared utilities
• Extends the comparison to also handle UserIdGroupPair rules (used by DefaultWorkerSGIngressRules) via a new equalUserIdGroupPairs helper
• Applies the diff check in createAWSDefaultSecurityGroup so the operator only calls AuthorizeSecurityGroupIngress for genuinely missing rules, instead of blindly upserting and relying on InvalidPermission.Duplicate errors

Closes: OCPBUGS-93738

Test plan

• New unit tests in support/awsutil/sg_test.go covering IpRanges, UserIdGroupPairs, mixed, and partial-match scenarios
• Existing TestDiffPermissions in awsprivatelink controller updated to use the shared function
• go test ./support/awsutil/... passes
• go test ./control-plane-operator/controllers/awsprivatelink/... passes
• go test ./control-plane-operator/controllers/hostedcontrolplane/... passes

🤖 Generated with Claude Code

[openshift-merge-bot]

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

[coderabbitai]

📝 Walkthrough

Walkthrough

A new exported DiffPermissions function and its helpers (isPermissionPresent, equalIPRanges, equalUserIdGroupPairs) are added to support/awsutil/sg.go. This function computes the subset of required EC2 IpPermission entries not present in the actual set. The awsprivatelink controller's local diffPermissions, isPermissionPresent, and equalIPRanges helpers are removed and replaced with calls to the shared function. The hostedcontrolplane controller's createAWSDefaultSecurityGroup is updated to diff current vs desired permissions before authorizing, skipping the AWS call when no permissions are missing. Tests are updated accordingly.

Sequence Diagram(s)

No sequence diagram generated — the change is a refactor extracting shared utility logic without introducing new component interactions.

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly reflects the main change: diffing security group permissions before calling AuthorizeSecurityGroupIngress.
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	PASS: The changed test files contain no Ginkgo titles, and the new table-driven names are static/descriptive with no run-to-run dynamic data.
Test Structure And Quality	✅ Passed	Changed tests are table-driven unit tests using t.Run and gomock; no Ginkgo/cluster waits/cleanup concerns, and assertions include clear failure messages in sg_test.go.
Topology-Aware Scheduling Compatibility	✅ Passed	The PR only changes AWS security-group permission diffing and ingress rule authorization; no pod scheduling, replicas, affinity, or node selectors were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; touched tests are plain unit tests, so the IPv4/disconnected-network check is not applicable.
No-Weak-Crypto	✅ Passed	Touched code only adds AWS security-group diffing; diff scan found no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons.
Container-Privileges	✅ Passed	No touched file adds privileged, hostPID/hostNetwork/hostIPC, SYS_ADMIN, runAsUser:0, or allowPrivilegeEscalation:true settings.
No-Sensitive-Data-In-Logs	✅ Passed	No new logging exposes secrets or PII; changed logs only include SG/VPC IDs, infra IDs, and resource names.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sdminonne
Once this PR has been reviewed and has the lgtm label, please assign cblecker 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

[coderabbitai]

🧹 Nitpick comments (1)

support/awsutil/sg_test.go (1)

140-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assertions don't verify range/pair contents.

The comparison loop only checks FromPort/ToPort/IpProtocol. Cases like "different group ID" pass solely on length+port, never asserting the returned UserIdGroupPairs/IpRanges content. Consider asserting the full entry (or using reflect.DeepEqual/Gomega Equal) so a regression in equalUserIdGroupPairs/equalIPRanges field comparison is caught.

🤖 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 `@support/awsutil/sg_test.go` around lines 140 - 150, The test loop in
sg_test.go only compares port/protocol fields and can miss regressions in the
security group rule contents. Update the assertion around the result/tt.expected
comparison in the relevant test to verify the full permission entry, including
UserIdGroupPairs and IpRanges, not just FromPort/ToPort/IpProtocol. Use a
full-struct equality check or explicit deep comparison so changes in
equalUserIdGroupPairs and equalIPRanges are actually validated.

🤖 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.

Nitpick comments:
In `@support/awsutil/sg_test.go`:
- Around line 140-150: The test loop in sg_test.go only compares port/protocol
fields and can miss regressions in the security group rule contents. Update the
assertion around the result/tt.expected comparison in the relevant test to
verify the full permission entry, including UserIdGroupPairs and IpRanges, not
just FromPort/ToPort/IpProtocol. Use a full-struct equality check or explicit
deep comparison so changes in equalUserIdGroupPairs and equalIPRanges are
actually validated.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 91e85069-40da-4cb1-909c-e8e27e118ad3

📥 Commits

Reviewing files that changed from the base of the PR and between bb1af6d and c15b7fc.

📒 Files selected for processing (5)

• control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
• control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go
• control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
• support/awsutil/sg.go
• support/awsutil/sg_test.go

[codecov]

Codecov Report

❌ Patch coverage is 55.76923% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.19%. Comparing base (bb1af6d) to head (c15b7fc).
⚠️ Report is 730 commits behind head on main.

Files with missing lines	Patch %	Lines
...ostedcontrolplane/hostedcontrolplane_controller.go	0.00%	13 Missing ⚠️
support/awsutil/sg.go	76.31%	6 Missing and 3 partials ⚠️
...ollers/awsprivatelink/awsprivatelink_controller.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8869   +/-   ##
=======================================
  Coverage   43.19%   43.19%           
=======================================
  Files         767      767           
  Lines       94914    94931   +17     
=======================================
+ Hits        40998    41007    +9     
- Misses      51052    51059    +7     
- Partials     2864     2865    +1     

Files with missing lines	Coverage Δ
...ollers/awsprivatelink/awsprivatelink_controller.go	40.69% <0.00%> (-0.96%)	⬇️
support/awsutil/sg.go	10.06% <76.31%> (+10.06%)	⬆️
...ostedcontrolplane/hostedcontrolplane_controller.go	45.64% <0.00%> (-0.10%)	⬇️

Flag	Coverage Δ
cmd-support	36.47% <76.31%> (+0.04%)	⬆️
cpo-hostedcontrolplane	45.30% <0.00%> (-0.02%)	⬇️
cpo-other	45.03% <0.00%> (-0.07%)	⬇️
hypershift-operator	53.53% <ø> (ø)
other	31.69% <ø> (ø)

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.

[openshift-ci]

@sdminonne: 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.

[hypershift-jira-solve-ci]

Now I have the complete picture. The gci config requires imports to be ordered in specific groups. Let me verify the exact issue with the import ordering in both files:

The gci config specifies this import order:

1. standard (stdlib)
2. dot
3. prefix(github.com/openshift/hypershift) — local project imports
4. prefix(github.com/openshift) — other openshift imports
5. prefix(github.com/aws) — AWS SDK imports
6. prefix(github.com/Azure) — Azure imports
7. k8s.io imports
8. sigs.k8s.io imports
9. default — everything else

In support/awsutil/sg_test.go, the imports are:

import (
    "testing"
    
    "github.com/aws/aws-sdk-go-v2/aws"
    ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
)

The testing (stdlib) and AWS imports are in separate groups but the gci linter expects blank-line separation matching the configured section order.

In awsprivatelink_controller_test.go, the new import supportawsutil "github.com/openshift/hypershift/support/awsutil" was added to the openshift/hypershift group but the existing github.com/aws imports are in a mixed position.

And the verify job shows that go fmt modified the same two files, confirming a formatting issue.

I now have all the data needed for the report.

Test Failure Analysis Complete

Job Information

• Prow Job: verify / Verify (GitHub Actions)
• Build ID: 28435717448 / Job 84261051542
• PR: #8869 — fix(cpo): diff SG permissions before AuthorizeSecurityGroupIngress
• Branch: OCPBUGS-93738 → main

---

• Prow Job: lint / Lint (GitHub Actions)
• Build ID: 28435717419 / Job 84261051183

Test Failure Analysis

Error

# Verify job (28435717448):
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go: needs update
support/awsutil/sg_test.go: needs update
Process completed with exit code 1.

# Lint job (28435717419):
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go:1330:1: File is not properly formatted (gci)
support/awsutil/sg_test.go:21:1: File is not properly formatted (gci)
2 issues: gci: 2
make: *** [Makefile:121: lint] Error 1

Summary

Both CI jobs failed because two files modified in the PR — control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go and support/awsutil/sg_test.go — were not properly formatted before being pushed. The Verify job failed because make fmt (go fmt ./...) reformatted both files, and the subsequent git diff --exit-code dirty-tree check detected uncommitted changes. The Lint job failed because the gci linter (Go Consecutive Imports) detected that the import blocks in both files do not follow the project's required import section ordering. These are the same root cause: the PR author did not run make fmt and make lint (or make verify) before pushing.

Root Cause

The PR moves diffPermissions, isPermissionPresent, and equalIPRanges from awsprivatelink_controller.go to support/awsutil/sg.go and introduces a new test file support/awsutil/sg_test.go. Both modified files have import blocks that violate the project's gci import ordering rules.

1. support/awsutil/sg_test.go (new file, line 21):
The import block groups testing (stdlib) and github.com/aws/... imports, but does not follow the project's .golangci.yml gci section ordering which requires: standard → prefix(github.com/openshift/hypershift) → prefix(github.com/aws) → default, each separated by blank lines with proper grouping.

2. control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go (line 1330):
The PR added supportawsutil "github.com/openshift/hypershift/support/awsutil" to the import block alongside existing github.com/openshift/hypershift and github.com/aws imports. The relative ordering between the openshift/hypershift, openshift, and aws import sections does not match the gci custom-order configuration.

The project's .golangci.yml enforces a strict custom import order with 9 sections: standard → dot → prefix(github.com/openshift/hypershift) → prefix(github.com/openshift) → prefix(github.com/aws) → prefix(github.com/Azure) → k8s.io → sigs.k8s.io → default.

Both the go fmt formatting and gci import ordering checks fail, causing both CI jobs to fail on the same two files.

Recommendations

1. Run make fmt locally to auto-fix go fmt formatting issues:

   make fmt

2. Run make lint locally to verify gci import ordering is correct (or use the auto-fix):

   # Check for lint issues
   make lint
   # Or auto-fix gci import ordering directly
   golangci-lint run --fix --enable-only gci

3. Push the reformatted files — both control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go and support/awsutil/sg_test.go will need updated commits.

4. Pre-push workflow: Consider running make verify before pushing to catch both formatting and lint issues locally. This runs make fmt, make vet, make lint, and the dirty-tree check in sequence.

Evidence

Evidence	Detail
Verify job go fmt output	go fmt ./... reformatted awsprivatelink_controller.go and sg_test.go (lines 162-163 of job log)
Verify job dirty-tree check	git diff --exit-code HEAD failed; both files reported as "needs update" (lines 182-184 of job log)
Lint job gci error 1	control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go:1330:1: File is not properly formatted (gci)
Lint job gci error 2	support/awsutil/sg_test.go:21:1: File is not properly formatted (gci)
gci config (.golangci.yml)	Custom order: standard → dot → prefix(github.com/openshift/hypershift) → prefix(github.com/openshift) → prefix(github.com/aws) → prefix(github.com/Azure) → k8s.io → sigs.k8s.io → default
PR diff — sg_test.go imports	Imports testing (stdlib) and github.com/aws/... without matching required section grouping
PR diff — awsprivatelink_controller_test.go imports	Added supportawsutil "github.com/openshift/hypershift/support/awsutil" into a mixed import block with github.com/aws imports
Verify job URL	Job 84261051542
Lint job URL	Job 84261051183

---

[openshift-ci]

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