OCPBUGS-82443: fix(cpo): deduplicate VPC endpoint subnets by AZ - #8651
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-important
|
@reedcort: This pull request references Jira Issue OCPBUGS-82443, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces subnet-to-availability-zone (AZ) tracking for AWS VPC endpoints. The feature adds an Sequence Diagram(s)sequenceDiagram
participant Controller
participant deduplicateSubnetsByAZ
participant AZCache
participant EC2DescribeSubnets
participant ReconciliationFlow
Controller->>deduplicateSubnetsByAZ: Call with SubnetIDs + CachedAZs
deduplicateSubnetsByAZ->>AZCache: Check cached AZ mappings
alt Missing AZ mappings
deduplicateSubnetsByAZ->>EC2DescribeSubnets: Query missing subnets
EC2DescribeSubnets-->>deduplicateSubnetsByAZ: Return AZ data
else All AZs cached
deduplicateSubnetsByAZ->>deduplicateSubnetsByAZ: Use cache only
end
deduplicateSubnetsByAZ->>deduplicateSubnetsByAZ: Group by AZ, select lexicographic first
deduplicateSubnetsByAZ->>deduplicateSubnetsByAZ: Prune stale cache entries
deduplicateSubnetsByAZ-->>Controller: Return DeduplicatedIDs + UpdatedAZMap
Controller->>ReconciliationFlow: Update Spec.SubnetIDs
Controller->>ReconciliationFlow: Update Status.EndpointSubnetAZs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/infra/aws/iam.go (1)
679-680: Note the graceful degradation behavior for ROSA clusters.The
ec2:DescribeSubnetspermission has been added to three CPO-managed policies. However, ROSA clusters using the AWS-managedROSAControlPlaneOperatorPolicywill not receive this permission until AWS updates their managed policy. For these clusters, the controller will fall back to the original behavior (proceed with duplicate subnets, AWS rejects withDuplicateSubnetsInSameZone). This graceful degradation is intentional and preserves existing behavior until the AWS-managed policy is updated.Also applies to: 706-707, 781-782
🤖 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 `@cmd/infra/aws/iam.go` around lines 679 - 680, Add a clear in-code comment where the EC2 permissions list includes "ec2:DescribeSubnets" that explains ROSA clusters using the AWS-managed ROSAControlPlaneOperatorPolicy will not immediately receive this new permission until AWS updates their managed policy, and that the controller intentionally falls back to the original behavior (proceeding with duplicate subnets and allowing AWS to reject with DuplicateSubnetsInSameZone) to preserve existing behavior; place the same explanatory comment near the other two occurrences where "ec2:DescribeSubnets" was added (the blocks corresponding to the other CPO-managed policies) so future maintainers see the graceful-degradation note.
🤖 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 `@api/hypershift/v1beta1/endpointservice_types_test.go`:
- Around line 87-95: The test is missing a check for SecurityGroupID when
round-tripping the N-1 struct; in the verification block that compares
roundTripped to tt.nMinus1Result (alongside EndpointServiceName, EndpointID, and
EndpointSubnetAZs), add an assertion that roundTripped.SecurityGroupID equals
tt.nMinus1Result.SecurityGroupID to ensure
awsEndpointServiceStatusNMinus1.SecurityGroupID is preserved during
deserialization into the current struct; place this check next to the existing
EndpointServiceName/EndpointID comparisons and use the same t.Errorf style for
mismatch reporting.
In `@api/hypershift/v1beta1/endpointservice_types.go`:
- Around line 95-100: Remove the restrictive validation on the EndpointSubnetAZs
field by deleting the +kubebuilder:validation:MinProperties=1 tag on the
EndpointSubnetAZs map in endpointservice_types.go; this allows the cache to
become empty (nil-equivalent) after pruning in deduplicateSubnetsByAZ
(awsprivatelink_controller.go) without failing validation, and adjust any
related comments if needed to reflect that an empty map is valid.
---
Nitpick comments:
In `@cmd/infra/aws/iam.go`:
- Around line 679-680: Add a clear in-code comment where the EC2 permissions
list includes "ec2:DescribeSubnets" that explains ROSA clusters using the
AWS-managed ROSAControlPlaneOperatorPolicy will not immediately receive this new
permission until AWS updates their managed policy, and that the controller
intentionally falls back to the original behavior (proceeding with duplicate
subnets and allowing AWS to reject with DuplicateSubnetsInSameZone) to preserve
existing behavior; place the same explanatory comment near the other two
occurrences where "ec2:DescribeSubnets" was added (the blocks corresponding to
the other CPO-managed policies) so future maintainers see the
graceful-degradation note.
🪄 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: acf96909-54f8-477d-a418-510c4ac9c45b
⛔ Files ignored due to path filters (5)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/awsendpointservices.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/awsendpointservices.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/endpointservice_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (5)
api/hypershift/v1beta1/endpointservice_types.goapi/hypershift/v1beta1/endpointservice_types_test.gocmd/infra/aws/iam.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8651 +/- ##
==========================================
+ Coverage 40.69% 41.47% +0.78%
==========================================
Files 755 756 +1
Lines 93373 93721 +348
==========================================
+ Hits 37994 38867 +873
+ Misses 52646 52133 -513
+ Partials 2733 2721 -12
... and 47 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| SecurityGroupID string `json:"securityGroupID,omitempty"` | ||
|
|
||
| // endpointSubnetAZs maps subnet IDs used by the VPC endpoint to their | ||
| // availability zones. Used to avoid redundant DescribeSubnets calls. |
There was a problem hiding this comment.
It makes more sense to define why we actually need this, something like "We need to keep track of which Availability Zones we have endpoints in, as AWS only supports 1 per AZ."
| "ec2:DescribeSecurityGroups", | ||
| "ec2:DescribeVpcs" | ||
| "ec2:DescribeVpcs", | ||
| "ec2:DescribeSubnets" |
There was a problem hiding this comment.
Can we confirm we need this for the sharedVPCPolicyBinding? I am not sure, just curious
There was a problem hiding this comment.
Yes — in SharedVPC mode the CPO assumes the sharedVPCEndpointRole for all EC2 operations.
| return | ||
| } | ||
|
|
||
| func deduplicateSubnetsByAZ(ctx context.Context, ec2Client awsapi.EC2API, subnetIDs []string, cachedAZs map[string]string) ([]string, map[string]string, error) { |
There was a problem hiding this comment.
There is a lot going on here, would be good to add a comment laying out the business logic a bit for future readers.
| if len(subnetIDs) <= 1 { | ||
| return subnetIDs, cachedAZs, nil | ||
| } |
There was a problem hiding this comment.
If a cluster has 2 subnets, then goes down to 1, the cachedAZs would include the subnet that was removed, right? I can't think of a clean way to prevent that, but as soon as they add a second subnet, the dedupe logic would prune it from the cache. Generally, that's fine, but wanted to note it.
There was a problem hiding this comment.
Yeah good callout — the <= 1 short-circuit skips pruning so the old entry lingers. Since it's just unused cache data in Status it does no actual harm. We could have it prune on every reconcile if we really don't want to have stale data in the status
There was a problem hiding this comment.
No worries, we want to keep the reconcile fast and no-op as much as possible, so I think given how unlikely it is to go from >1 subnet down to 1, it's fine.
There was a problem hiding this comment.
I could foresee a future where customers start using additional subnets as a "testing ground", where they have a static cluster and then add a new machine pool and subnet for a day, then destroy it later. Would this cause an issue?
There was a problem hiding this comment.
With the new in-memory cache approach no. When the NodePool is deleted, the hypershift-operator removes that subnet from AWSEndpointService.Spec.SubnetIDs. The dedup function only looks at subnets in subnetIDs — the stale cache entry for the removed subnet is never read. The only effect is a few bytes of orphaned cache entries in memory, which clear on CPO restart.
dcdaf62 to
1a398ba
Compare
openshift-ci
Bot
added
the
area/testing
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 `@test/e2e/privatelink_subnet_dedup_test.go`:
- Around line 99-124: The cleanup loop currently creates waitCtx from the test's
ctx and breaks on any List error, so it can exit prematurely; change waitCtx to
be derived from context.Background() (e.g.,
context.WithTimeout(context.Background(), 3*time.Minute)) so test cancellation
doesn't abort cleanup, keep the defer cancel, and replace the unconditional
break on mgtClient.List error with retry logic: log the error and continue the
loop (or short backoff) so transient API errors don't stop waiting for
AWSEndpointServiceList items to be reconciled; keep using
AWSEndpointServiceList, mgtClient.List, waitCtx and newSubnetID to locate the
right objects and preserve the timeout check via waitCtx.Err().
🪄 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: f4b7c384-2a40-4def-aa4a-12975eaa1e7c
⛔ Files ignored due to path filters (5)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/awsendpointservices.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/awsendpointservices.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/endpointservice_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (7)
api/hypershift/v1beta1/endpointservice_types.goapi/hypershift/v1beta1/endpointservice_types_test.gocmd/infra/aws/iam.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.gotest/e2e/create_cluster_test.gotest/e2e/privatelink_subnet_dedup_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- api/hypershift/v1beta1/endpointservice_types.go
- cmd/infra/aws/iam.go
- control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go
- control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
- api/hypershift/v1beta1/endpointservice_types_test.go
…SubnetsInSameZone When a HCP cluster has multiple NodePools with subnets in the same AWS availability zone, the CPO's VPC endpoint reconciliation fails with DuplicateSubnetsInSameZone because AWS allows at most one subnet per AZ per endpoint. Add a deduplicateSubnetsByAZ method on the reconciler that calls DescribeSubnets to resolve AZ membership, groups subnets by AZ, and picks one per AZ (lexicographically first for determinism). The subnet-to-AZ mapping is cached in an in-memory map on the reconciler to avoid redundant AWS API calls across reconcile loops. On DescribeSubnets failure the controller gracefully degrades by proceeding with the original subnet list, preserving existing behavior. Also adds ec2:DescribeSubnets to the three CPO IAM policies that lacked it. The ROSA-managed ROSAControlPlaneOperatorPolicy requires a separate update with AWS (tracked in ROSAENG-57993). Signed-off-by: Cortney Reed <creed@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-bug
|
@reedcort: This pull request references Jira Issue OCPBUGS-82443, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
Scheduling tests matching the |
|
/assign @sjenning |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 3 test failures are unrelated to the PR changes and represent pre-existing flaky test behavior. The PR modifies Root CauseFailure 1 & 2 — Failure 3 — Failure 4 — No relation to PR #8651: The PR changes Recommendations
Evidence
|
|
/test e2e-aws |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, reedcort 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 |
openshift-ci
Bot
added
the
approved
Test EvidenceTested on a ROSA HCP staging cluster with two subnets in the same AZ ( AWSEndpointService spec shows both subnets: Before — graceful degradation without
|
|
/verified by @reedcort — tested on ROSA HCP staging cluster with two subnets in same AZ |
openshift-ci-robot
added
the
verified
|
@reedcort: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@reedcort: all tests passed! 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. |
|
@reedcort: Jira Issue Verification Checks: Jira Issue OCPBUGS-82443 Jira Issue OCPBUGS-82443 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick release-4.22 |
|
@reedcort: #8651 failed to apply on top of branch "release-4.20": 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. |
|
@reedcort: #8651 failed to apply on top of branch "release-4.21": 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. |
|
@reedcort: #8651 failed to apply on top of branch "release-4.22": 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. |
|
Fix included in release 5.0.0-0.nightly-2026-06-12-141614 |
10 participants
What this PR does / why we need it:
When a HCP cluster has multiple NodePools with subnets in the same AWS availability zone, the CPO's VPC endpoint
reconciliation fails indefinitely with
DuplicateSubnetsInSameZone. AWS VPC endpoints allow at most one subnetper AZ, but
listSubnetIDs()only deduplicates by subnet ID, not by AZ.This PR adds AZ-aware subnet deduplication in the CPO's
ensureVPCEndpointpath:deduplicateSubnetsByAZ()— callsDescribeSubnetsto resolve AZ membership, groups subnets by AZ, andpicks one per AZ (lexicographically first for determinism)
sync.RWMutextoavoid redundant
DescribeSubnetscalls on subsequent reconciles. The cache is rebuilt on CPO restart.DescribeSubnetsfails (e.g. missing IAM permission), the controller proceedswith the original subnet list, preserving existing behavior
ec2:DescribeSubnetsto the three CPO policies iniam.go. The ROSA-managedROSAControlPlaneOperatorPolicyrequires a separate update with AWS (tracked in ROSAENG-57993)Files changed
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.godeduplicateSubnetsByAZmethod, in-memory cache on reconciler, integrate inensureVPCEndpointcontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.gocmd/infra/aws/iam.goec2:DescribeSubnetsto 3 IAM policiesWhich issue(s) this PR fixes:
Fixes OCPBUGS-82443
Related: ROSAENG-57993 (ROSA managed policy update)
Special notes for your reviewer:
DescribeSubnetscall for ~2-3 subnets)sharedVPCEndpointRolefor all EC2 operations, so that role also needsec2:DescribeSubnetsROSAControlPlaneOperatorPolicywon't get the fix until AWS addsec2:DescribeSubnetsto that policy — the graceful degradation ensures no regressionChecklist: