GCP-859: scope PSC NAT subnet discovery to the management cluster VPC - #8927
Conversation
The previous discoverNATSubnet implementation listed all subnets in the GCP project/region with purpose=PRIVATE_SERVICE_CONNECT and picked the first available one, with no filtering by VPC network. This assumed a single management cluster per GCP project and broke when multiple MCs shared a project, each in its own VPC — GCP rejects a Service Attachment whose NAT subnet and forwarding rule are in different VPCs. Changes: - Rename lookupForwardingRuleName to lookupForwardingRule and return the full *compute.ForwardingRule so callers can access any field without future signature changes. - Gate NAT subnet discovery on the forwarding rule being available; if the ILB is not yet provisioned the controller returns nil and requeues without writing to the spec. - Derive the VPC network URL from the forwarding rule's Network field (authoritative, set by GCP CCM at ILB creation) and pass it to discoverNATSubnet as a server-side filter, eliminating cross-VPC subnet selection. - Guard against a forwarding rule with an empty Network field to surface a clear error instead of silently producing a bad filter. - Handle the partial-write edge case: when ForwardingRuleName is already set but NATSubnet is not, re-fetch the forwarding rule to obtain the network URL and complete subnet discovery. - Add TestNATSubnetFilterFormat to cover the VPC-scoped filter string. - Add e2e test that fetches the forwarding rule and NAT subnet from the GCP API and asserts they share the same VPC network URL. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
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
openshift-ci-robot
added
the
jira/valid-reference
|
@cristianoveiga: This pull request references GCP-859 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 story to target the "5.0.0" version, but no target version was set. 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. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/hypershift-operator
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe GCP Private Service Connect controller now uses a compute-client interface, looks up forwarding rules before filling missing spec fields, and scopes NAT subnet discovery to the forwarding rule’s network. Service-attachment operations now go through the same interface. The PR also adds unit tests for lookup and subnet selection behavior, plus an e2e test that checks PSC spec fields and service-attachment status. 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-v2-gke |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go (1)
154-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ForwardingRuleNameis overwritten unconditionally, unlike the guardedNATSubnetwrite.Line 173 always reassigns
gcpPSC.Spec.ForwardingRuleName = hyperv1.GCPResourceName(rule.Name), even when it was already set (the early-return only skips when both fields are non-empty).NATSubnetis correctly guarded withif gcpPSC.Spec.NATSubnet == "". SincelookupForwardingRulecan non-deterministically pick "first" among multiple IP-matching forwarding rules (line 207-211), an already-correctForwardingRuleNamecould theoretically flip on a later reconcile, causing unnecessary spec churn. Consider guarding the assignment the same way:🔧 Suggested fix
- gcpPSC.Spec.ForwardingRuleName = hyperv1.GCPResourceName(rule.Name) + if gcpPSC.Spec.ForwardingRuleName == "" { + gcpPSC.Spec.ForwardingRuleName = hyperv1.GCPResourceName(rule.Name) + }🤖 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 `@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go` around lines 154 - 184, The spec update in reconcileGCPPrivateServiceConnectSpec always rewrites GCPPrivateServiceConnect.Spec.ForwardingRuleName, which can cause unnecessary churn if lookupForwardingRule returns a different matching rule on a later reconcile. Mirror the NATSubnet guard by only setting ForwardingRuleName when it is empty, while keeping the existing lookupForwardingRule and discoverNATSubnet flow intact. Use the reconcileGCPPrivateServiceConnectSpec and lookupForwardingRule symbols to update the assignment site without changing the early return behavior.
🧹 Nitpick comments (3)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go (1)
294-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest duplicates the filter string instead of exercising the production code path.
TestNATSubnetFilterFormatre-implements thefmt.Sprintffilter construction inline (line 310) rather than callingdiscoverNATSubnet's actual filter-building logic. If the real filter format in the controller changes, this test won't catch the regression — it will still pass because it only compares itself against itself. This mirrors the pre-existingTestIPAddressFilterFormatpattern, but extracting the filter construction into a small shared helper function (called by both the controller and the test) would give real regression protection.🤖 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 `@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go` around lines 294 - 316, The NAT subnet filter test is duplicating the filter construction instead of verifying the controller’s real behavior. Move the filter-building logic used by discoverNATSubnet into a shared helper (similar to the existing IP address filter pattern), then update TestNATSubnetFilterFormat to call that helper and compare its output against the expected string. This keeps the test exercising the production code path and makes the filter format change-safe.hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go (1)
243-279: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winN+1 GCP API calls:
isSubnetInUsere-lists all Service Attachments for every candidate subnet.The loop calls
r.isSubnetInUse(ctx, subnet.Name)per subnet, and each call performs a freshServiceAttachments.Listfor the whole region (line 217-241, unchanged). With several PSC subnets in a shared VPC, this multiplies redundant GCP API calls unnecessarily. Consider listing Service Attachments once before the loop and building a used-subnet set for O(1) membership checks.🤖 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 `@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go` around lines 243 - 279, The loop in discoverNATSubnet is causing an N+1 pattern because isSubnetInUse re-lists all Service Attachments for each candidate subnet. Refactor discoverNATSubnet and/or isSubnetInUse so ServiceAttachments.List is called once up front, build a set of used subnet names from the result, and then use that set inside the subnet iteration for O(1) checks. Keep the existing logging and selection behavior in GCPPrivateServiceConnectReconciler intact while removing the repeated per-subnet API calls.test/e2e/v2/tests/hosted_cluster_psc_test.go (1)
44-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid adherence to v2 e2e conventions; consider validating all PSC resources, not just the first.
The test correctly uses
tc.Context, guards withNotTo(BeEmpty())before indexing, nil-checkshc.Spec.Platform.GCP, and includes diagnostic assertion messages — all per AGENTS.md. One gap: line 61 only validatespscList.Items[0]. If a hosted cluster can have more than oneGCPPrivateServiceConnect(e.g., additional endpoints), the others go unchecked. Consider iterating over all items.Based on path instructions for
test/e2e/v2/**/*.go("Strictly enforce all standards documented in test/e2e/v2/AGENTS.md"), rule 16 requires asserting non-emptiness before per-item loops and using afoundboolean for search loops — worth confirming whether multipleGCPPrivateServiceConnectresources can legitimately exist per hosted cluster to decide if full iteration is warranted here.🤖 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 `@test/e2e/v2/tests/hosted_cluster_psc_test.go` around lines 44 - 93, The test only validates the first GCPPrivateServiceConnect item, so additional PSC resources can be missed. Update the hosted cluster PSC test to iterate over all entries in pscList.Items (instead of indexing pscList.Items[0]) and apply the same ForwardingRuleName/NATSubnet checks and network comparison to each GCPPrivateServiceConnect item; use the existing testCtx, pscList, and GCPPrivateServiceConnect symbols to keep the logic easy to locate.Source: Path instructions
🤖 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.
Outside diff comments:
In
`@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go`:
- Around line 154-184: The spec update in reconcileGCPPrivateServiceConnectSpec
always rewrites GCPPrivateServiceConnect.Spec.ForwardingRuleName, which can
cause unnecessary churn if lookupForwardingRule returns a different matching
rule on a later reconcile. Mirror the NATSubnet guard by only setting
ForwardingRuleName when it is empty, while keeping the existing
lookupForwardingRule and discoverNATSubnet flow intact. Use the
reconcileGCPPrivateServiceConnectSpec and lookupForwardingRule symbols to update
the assignment site without changing the early return behavior.
---
Nitpick comments:
In
`@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go`:
- Around line 294-316: The NAT subnet filter test is duplicating the filter
construction instead of verifying the controller’s real behavior. Move the
filter-building logic used by discoverNATSubnet into a shared helper (similar to
the existing IP address filter pattern), then update TestNATSubnetFilterFormat
to call that helper and compare its output against the expected string. This
keeps the test exercising the production code path and makes the filter format
change-safe.
In
`@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go`:
- Around line 243-279: The loop in discoverNATSubnet is causing an N+1 pattern
because isSubnetInUse re-lists all Service Attachments for each candidate
subnet. Refactor discoverNATSubnet and/or isSubnetInUse so
ServiceAttachments.List is called once up front, build a set of used subnet
names from the result, and then use that set inside the subnet iteration for
O(1) checks. Keep the existing logging and selection behavior in
GCPPrivateServiceConnectReconciler intact while removing the repeated per-subnet
API calls.
In `@test/e2e/v2/tests/hosted_cluster_psc_test.go`:
- Around line 44-93: The test only validates the first GCPPrivateServiceConnect
item, so additional PSC resources can be missed. Update the hosted cluster PSC
test to iterate over all entries in pscList.Items (instead of indexing
pscList.Items[0]) and apply the same ForwardingRuleName/NATSubnet checks and
network comparison to each GCPPrivateServiceConnect item; use the existing
testCtx, pscList, and GCPPrivateServiceConnect symbols to keep the logic easy to
locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c3f2f008-5b4d-44f7-a2cd-83c4faef8f0d
📒 Files selected for processing (3)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.gotest/e2e/v2/tests/hosted_cluster_psc_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8927 +/- ##
==========================================
+ Coverage 43.34% 43.52% +0.18%
==========================================
Files 771 771
Lines 95534 95749 +215
==========================================
+ Hits 41408 41678 +270
+ Misses 51242 51182 -60
- Partials 2884 2889 +5
... and 6 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:
|
|
/test e2e-v2-gke |
- Guard ForwardingRuleName write to avoid unnecessary spec churn when the field is already set (mirrors the existing NATSubnet guard). - Extract filter construction into buildNATSubnetFilter helper so TestNATSubnetFilterFormat exercises the production code path. - Add comment clarifying there is exactly one GCPPrivateServiceConnect per hosted cluster. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
478cdd9 to
3189891
Compare
|
/test e2e-v2-gke |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/v2/tests/hosted_cluster_psc_test.go (1)
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused GCP platform-spec nil-check.
hc.Spec.Platform.GCPis asserted non-nil but never read afterward in the test body. This looks like a leftover from the previous version of the test that needed GCP project/region fields to build a direct GCP API client (now removed per this PR's shift to condition-based validation). Consider dropping the assertion if the field is truly unused, or keep it only if a future addition will reference it.♻️ Proposed cleanup
testCtx := getTestCtx() hc := testCtx.GetHostedCluster() - Expect(hc.Spec.Platform.GCP).NotTo(BeNil(), - "GCP platform spec must be set for GCP HostedCluster %s/%s", hc.Namespace, hc.Name) // Find the GCPPrivateServiceConnect CR in the control plane namespace.🤖 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 `@test/e2e/v2/tests/hosted_cluster_psc_test.go` around lines 44 - 48, The test in hosted_cluster_psc_test.go has an unused nil-check on hc.Spec.Platform.GCP inside the GCPServiceAttachmentAvailable condition case. Remove the redundant Expect(hc.Spec.Platform.GCP).NotTo(BeNil()) assertion from the test body unless a later step in the same It block actually uses the GCP platform fields; keep the rest of the condition-based validation unchanged.
🤖 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 `@test/e2e/v2/tests/hosted_cluster_psc_test.go`:
- Around line 44-48: The test in hosted_cluster_psc_test.go has an unused
nil-check on hc.Spec.Platform.GCP inside the GCPServiceAttachmentAvailable
condition case. Remove the redundant Expect(hc.Spec.Platform.GCP).NotTo(BeNil())
assertion from the test body unless a later step in the same It block actually
uses the GCP platform fields; keep the rest of the condition-based validation
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e32cf1b0-6785-46d9-bd48-c2e859820ee7
📒 Files selected for processing (3)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.gotest/e2e/v2/tests/hosted_cluster_psc_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go
- hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go
The hc.Spec.Platform.GCP nil check was left over from the original test design that called the GCP Compute API using project and region fields. Now that the test validates VPC correctness via the GCPServiceAttachmentAvailable condition instead, no GCP platform fields are accessed and the check is unreachable dead code. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go (1)
315-319: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn subnet-usage check failures instead of skipping them.
If
ListServiceAttachmentsfails, continuing can mask the real GCP API error and end with a misleading “no available subnet” result. Fail fast with subnet context so reconciliation retries the actual dependency failure.Proposed fix
inUse, err := r.isSubnetInUse(ctx, subnet.Name) if err != nil { - log.Error(err, "Failed to check subnet usage", "subnet", subnet.Name) - continue + return "", fmt.Errorf("failed to check subnet usage for %q: %w", subnet.Name, err) }As per path instructions, “Never ignore error returns.”
🤖 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 `@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go` around lines 315 - 319, The subnet-usage check in the private service connect reconciliation currently logs and continues on isSubnetInUse failures, which can hide the real GCP API error. Update the error handling in the privateserviceconnect controller so that ListServiceAttachments/isSubnetInUse failures are returned from the reconcile path with subnet context instead of skipping that subnet, letting the controller retry the actual dependency failure rather than reaching a misleading “no available subnet” result.Source: Path instructions
🧹 Nitpick comments (2)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go (2)
44-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCapture and assert the Compute API filters in the fake.
The PR’s core contract is VPC-scoped subnet discovery, but the fake drops the
filterargument, sodiscoverNATSubnetcould stop passingnetworkURLtoListSubnetworkswithout these tests failing.Proposed test-fake extension
type fakeComputeClient struct { forwardingRules []*compute.ForwardingRule forwardingRulesErr error + forwardingRulesFilter string subnetworks []*compute.Subnetwork subnetworksErr error + subnetworksFilter string serviceAttachments []*compute.ServiceAttachment serviceAttachmentsErr error @@ -func (f *fakeComputeClient) ListForwardingRules(_ context.Context, _, _, _ string) ([]*compute.ForwardingRule, error) { +func (f *fakeComputeClient) ListForwardingRules(_ context.Context, _, _, filter string) ([]*compute.ForwardingRule, error) { + f.forwardingRulesFilter = filter return f.forwardingRules, f.forwardingRulesErr } -func (f *fakeComputeClient) ListSubnetworks(_ context.Context, _, _, _ string) ([]*compute.Subnetwork, error) { +func (f *fakeComputeClient) ListSubnetworks(_ context.Context, _, _, filter string) ([]*compute.Subnetwork, error) { + f.subnetworksFilter = filter return f.subnetworks, f.subnetworksErr }Then assert
subnetworksFilter == buildNATSubnetFilter(networkURL)in the NAT subnet tests.🤖 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 `@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go` around lines 44 - 49, The fake Compute client methods are ignoring the filter argument, so the NAT subnet tests are not verifying that discoverNATSubnet passes the expected network-scoped filter. Update the fakeComputeClient implementation to capture the filter passed into ListSubnetworks (and keep it available for assertions), then extend the NAT subnet tests to assert that the recorded subnetworks filter matches buildNATSubnetFilter(networkURL). Use discoverNATSubnet, fakeComputeClient, and ListSubnetworks as the key symbols when wiring the assertion.
383-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse “When ... it should ...” descriptions for the new test cases.
These new unit tests use deterministic names, but they don’t follow the repository’s required test-case description format. Consider table-driven subtests with
t.Run("When ... it should ...", ...)for the lookup/reconcile/discovery cases.As per coding guidelines,
**/*_test.go: “Always use "When ... it should ..." format for describing test cases when creating unit tests.”🤖 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 `@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go` around lines 383 - 542, The new unit tests in the lookupForwardingRule, reconcileGCPPrivateServiceConnectSpec, and discoverNATSubnet coverage do not follow the required “When ... it should ...” test-case description format. Refactor these deterministic cases into table-driven subtests with t.Run names like “When ... it should ...” so the scenarios are clearly described and consistent with the repository’s *_test.go guidelines, while keeping the same assertions and coverage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go`:
- Around line 315-319: The subnet-usage check in the private service connect
reconciliation currently logs and continues on isSubnetInUse failures, which can
hide the real GCP API error. Update the error handling in the
privateserviceconnect controller so that ListServiceAttachments/isSubnetInUse
failures are returned from the reconcile path with subnet context instead of
skipping that subnet, letting the controller retry the actual dependency failure
rather than reaching a misleading “no available subnet” result.
---
Nitpick comments:
In
`@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go`:
- Around line 44-49: The fake Compute client methods are ignoring the filter
argument, so the NAT subnet tests are not verifying that discoverNATSubnet
passes the expected network-scoped filter. Update the fakeComputeClient
implementation to capture the filter passed into ListSubnetworks (and keep it
available for assertions), then extend the NAT subnet tests to assert that the
recorded subnetworks filter matches buildNATSubnetFilter(networkURL). Use
discoverNATSubnet, fakeComputeClient, and ListSubnetworks as the key symbols
when wiring the assertion.
- Around line 383-542: The new unit tests in the lookupForwardingRule,
reconcileGCPPrivateServiceConnectSpec, and discoverNATSubnet coverage do not
follow the required “When ... it should ...” test-case description format.
Refactor these deterministic cases into table-driven subtests with t.Run names
like “When ... it should ...” so the scenarios are clearly described and
consistent with the repository’s *_test.go guidelines, while keeping the same
assertions and coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 82047cb6-80e2-41cd-8eb6-747dcb496b6e
📒 Files selected for processing (3)
hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.gotest/e2e/v2/tests/hosted_cluster_psc_test.go
💤 Files with no reviewable changes (1)
- test/e2e/v2/tests/hosted_cluster_psc_test.go
cdae895 to
1cacd37
Compare
…ests Extracts a ComputeClient interface over the GCP Compute API calls used by the PSC controller (following the Azure PrivateLinkServicesAPI pattern). A computeServiceAdapter bridges the GCP SDK's chained-call surface to the interface. Unit tests inject a fakeGCPComputeClient to cover the new reconciliation paths without requiring GCP credentials: - lookupForwardingRule: API error, no results, single result, multiple results (uses first) - reconcileGCPPrivateServiceConnectSpec: both fields set (early return), lookup error, ILB not yet provisioned, empty Network field, happy path - discoverNATSubnet: API error, no subnets, subnet in-use skip, all subnets in use Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
1cacd37 to
2576bcd
Compare
|
/test e2e-v2-gke |
…ed subnet discovery Add capturedSubnetFilter to fakeComputeClient so that TestDiscoverNATSubnet_* tests can assert the VPC-scoped filter reaches the ListSubnetworks call site. Without this, deleting the filter argument from discoverNATSubnet would not be caught by any unit test. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| } | ||
| } | ||
|
|
||
| // --- reconcileGCPPrivateServiceConnectSpec --- |
There was a problem hiding this comment.
Nit: the partial-write edge case (ForwardingRuleName set, NATSubnet empty) that the code comments on L211-213 call out is worth covering here. Something like newGCPPSC("original-rule", "") with a fake returning a different rule name would verify that the existing name is preserved and discoverNATSubnet still gets the correct network URL.
There was a problem hiding this comment.
Addressed - added a new test case to cover this.
| return subnet.Name, nil | ||
| } | ||
| // Find the first available PSC subnet in the MC's VPC not already in use by another Service Attachment. | ||
| for _, subnet := range subnets { |
There was a problem hiding this comment.
Pre-existing, not introduced by this PR: isSubnetInUse makes a fresh ListServiceAttachments call per candidate subnet. In environments with several PSC subnets, fetching the list once before the loop and building a set of in-use names would avoid N redundant API calls. Fine as a follow-up.
There was a problem hiding this comment.
Created https://redhat.atlassian.net/browse/GCP-883 as a follow up.
Cover the case where ForwardingRuleName is already set but NATSubnet is empty (e.g. transient discoverNATSubnet failure on a prior reconcile). Asserts the existing name is preserved and the VPC-scoped subnet filter is still derived from the forwarding rule's Network field. Also capture the ListSubnetworks filter argument in fakeComputeClient so all discoverNATSubnet tests can assert the VPC filter reaches the call site. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
| log.V(1).Info("Subnet already in use, trying next", "subnet", subnet.Name) | ||
| } | ||
|
|
||
| return "", fmt.Errorf("no available PRIVATE_SERVICE_CONNECT subnet found in region %s", r.Region) |
There was a problem hiding this comment.
Suggestion: when every isSubnetInUse call errors out (e.g. transient GCP IAM / API failure), this message reports "no available subnet" — which would send operators looking at subnet provisioning rather than API connectivity. Consider tracking error count through the loop and including it here, e.g. "no available PRIVATE_SERVICE_CONNECT subnet found in region %s (failed to check %d of %d candidates due to API errors)".
…fail When all isSubnetInUse calls fail due to transient GCP API/IAM errors, the previous error reported "no available subnet" — pointing operators at subnet provisioning rather than API connectivity. Now includes the count of failed candidates to distinguish the two failure modes. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
@CodeRabbit resume |
✅ Action performedReviews resumed. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, cristianoveiga 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 Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 17
... and 12 more failed tests |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required |
|
/test e2e-azure-v2-self-managed |
|
/retest-required |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe single test failure is a transient management cluster kube-apiserver connectivity timeout, completely unrelated to the PR changes. The test attempted to connect to the self-managed management cluster's API server at Root CauseThe root cause is a transient network-level connectivity interruption to the self-managed management cluster's kube-apiserver ( Timeline of events:
The 30-second timeout window (18:45:15 → 18:45:45) aligns with a brief TCP-level connectivity disruption. This is a known pattern in Azure self-managed clusters where the kube-apiserver (on non-standard port 7443) can experience brief periods of unreachability during initial client connection establishment, particularly when multiple test groups concurrently create their first connections to the API server. This failure is NOT caused by the PR changes. PR #8927 modifies only 3 files, all GCP-specific:
The failing job is an Azure self-managed test — there is zero code path overlap. Recommendations
Evidence
|
|
Changes solely in GCP package. Azure confirmed infrastructure flake /verified later by @cristianoveiga |
|
@cblecker: Only users can be targets for the 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. |
|
@cblecker: Overrode contexts on behalf of cblecker: ci/prow/e2e-azure-v2-self-managed 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. |
|
/verified later @cristianoveiga |
openshift-ci-robot
added
verified-later
verified
|
@cblecker: This PR has been marked to be verified later 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. |
|
@cristianoveiga: 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. |
4 participants
Summary
Fixes PSC NAT subnet discovery to scope subnet lookup to the management cluster's VPC, preventing cross-VPC selection when multiple management clusters share a GCP project.
Root cause: `discoverNATSubnet()` listed all PSC-purpose subnets in a project/region with no VPC filter. GCP rejects a Service Attachment whose NAT subnet and forwarding rule are in different VPCs, causing PSC setup to fail in multi-cluster-per-project deployments.
Changes:
Fixes: https://redhat.atlassian.net/browse/GCP-859
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests