fix: support PSC cluster cleanup in break-glass #929
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RaphaelBut 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
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR adds hint messages for hive shard access failures in the access command, and extends the cluster cleanup command to support Private Service Connect (PSC) clusters alongside PrivateLink, introducing a ChangesHive access hints and PSC cleanup support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant cleanupCmd as cleanupAccessOptions
participant HiveOCM as Hive OCM Connection
participant HiveClient
participant KubeClient
User->>cleanupCmd: run cleanup (--hive-ocm-url, --cluster-id)
cleanupCmd->>cleanupCmd: detect PSC or PrivateLink cluster
alt hive-ocm-url provided
cleanupCmd->>HiveOCM: resolve hive shard connection
HiveOCM-->>cleanupCmd: backplane cluster-admin elevation
cleanupCmd->>HiveClient: create Hive client
else no hive-ocm-url
cleanupCmd->>KubeClient: derive client with impersonation
end
cleanupCmd->>HiveClient: list jump pods
cleanupCmd->>HiveClient: delete jump pods
cleanupCmd->>HiveClient: poll pod termination
HiveClient-->>cleanupCmd: termination status
cleanupCmd-->>User: cleanup result
Related PRs: None identified. Suggested labels: enhancement, tests Suggested reviewers: None identified. 🐰 A rabbit hops through hive and shard, 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
cmd/cluster/access/cleanup.go (2)
146-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
hiveOCM.Close()error is discarded.The primary connection at Line 116 wraps
conn.Close()incmdutil.CheckErr, but this deferred close silently drops its error. Consider handling it consistently (e.g.defer func(){ cmdutil.CheckErr(hiveOCM.Close()) }()).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 `@cmd/cluster/access/cleanup.go` at line 146, The deferred close in cleanup logic discards the error returned by hiveOCM.Close(), unlike the earlier connection close handled with cmdutil.CheckErr. Update the defer in the cleanup flow to call hiveOCM.Close() through an inline deferred function and pass its error to cmdutil.CheckErr, keeping the handling consistent with the existing close behavior in the same command path.Source: Path instructions
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fmt.Sprintfwith no format arguments.Both calls pass a constant string with no verbs/args;
staticcheckflags this (S1039) and it may fail CI lint. Use the string literal directly.♻️ Proposed change
- hiveClient, err = k8s.NewAsBackplaneClusterAdminWithConn(hive.ID(), kclient.Options{Scheme: scheme.Scheme}, hiveOCM, c.reason, fmt.Sprintf("Elevation required to clean break-glass on PrivateLink/PSC Clusters")) + hiveClient, err = k8s.NewAsBackplaneClusterAdminWithConn(hive.ID(), kclient.Options{Scheme: scheme.Scheme}, hiveOCM, c.reason, "Elevation required to clean break-glass on PrivateLink/PSC Clusters")- c.kubeCli.Impersonate("backplane-cluster-admin", c.reason, fmt.Sprintf("Elevation required to clean break-glass on PrivateLink/PSC Clusters")) + c.kubeCli.Impersonate("backplane-cluster-admin", c.reason, "Elevation required to clean break-glass on PrivateLink/PSC Clusters")Also applies to: 158-158
🤖 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/cluster/access/cleanup.go` at line 153, The calls in the cleanup flow are using fmt.Sprintf with a constant string and no formatting arguments, which triggers the lint issue. In cleanup.go, update the k8s.NewAsBackplaneClusterAdminWithConn calls in the cleanup path to pass the string literal directly instead of wrapping it in fmt.Sprintf, keeping the existing hive.ID(), kclient.Options, hiveOCM, and c.reason arguments unchanged.cmd/cluster/access/cleanup_test.go (2)
140-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPSC test doesn't exercise the new PSC-detection logic.
dropPrivateLinkAccessonly usescluster.ID(); the PSC predicatecluster.GCP().PrivateServiceConnect().ServiceAttachmentSubnet() != ""lives inRun()(cleanup.go Line 124), which this test never calls. As written,TestCleanupAccessOptions_dropPscAccessis functionally identical to the PrivateLink test and gives no coverage of the actual detection change. Consider a focused table test asserting the predicate against both PSC and non-PSC cluster objects.🤖 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/cluster/access/cleanup_test.go` around lines 140 - 234, The PSC cleanup test currently bypasses the new detection path and only exercises dropPrivateLinkAccess, so it does not verify the cluster.GCP().PrivateServiceConnect().ServiceAttachmentSubnet() check used by Run(). Update TestCleanupAccessOptions_dropPscAccess to target the PSC predicate directly or through Run(), and add table coverage for both PSC and non-PSC cluster objects using generatePscClusterObjectForTesting and a non-PSC equivalent so the new branching logic is actually validated.
246-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty-URL case is a no-op assertion.
The
"Empty (flag omitted)"row never reachesValidateAndResolveOcmUrlbecause of theif tt.hiveOcmUrl != ""guard at Line 251, so it asserts nothing. To meaningfully cover the empty path (which mirrorscleanupCmdComplete's skip behavior), drive the test throughcleanupCmdCompleteor assert the skip explicitly.🤖 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/cluster/access/cleanup_test.go` around lines 246 - 259, The "Empty (flag omitted)" test case in cleanup_test.go is not exercising any behavior because the tt.hiveOcmUrl guard skips the ValidateAndResolveOcmUrl call entirely. Update the test around cleanupCmdComplete and the ValidateAndResolveOcmUrl path so the empty-URL case is asserted explicitly, matching the skip behavior when the flag is omitted. Use the cleanupCmdComplete test flow (or an equivalent direct assertion) to verify that an empty hive-ocm-url results in no resolution attempt and no error.
🤖 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 `@cmd/cluster/access/cleanup_test.go`:
- Around line 140-234: The PSC cleanup test currently bypasses the new detection
path and only exercises dropPrivateLinkAccess, so it does not verify the
cluster.GCP().PrivateServiceConnect().ServiceAttachmentSubnet() check used by
Run(). Update TestCleanupAccessOptions_dropPscAccess to target the PSC predicate
directly or through Run(), and add table coverage for both PSC and non-PSC
cluster objects using generatePscClusterObjectForTesting and a non-PSC
equivalent so the new branching logic is actually validated.
- Around line 246-259: The "Empty (flag omitted)" test case in cleanup_test.go
is not exercising any behavior because the tt.hiveOcmUrl guard skips the
ValidateAndResolveOcmUrl call entirely. Update the test around
cleanupCmdComplete and the ValidateAndResolveOcmUrl path so the empty-URL case
is asserted explicitly, matching the skip behavior when the flag is omitted. Use
the cleanupCmdComplete test flow (or an equivalent direct assertion) to verify
that an empty hive-ocm-url results in no resolution attempt and no error.
In `@cmd/cluster/access/cleanup.go`:
- Line 146: The deferred close in cleanup logic discards the error returned by
hiveOCM.Close(), unlike the earlier connection close handled with
cmdutil.CheckErr. Update the defer in the cleanup flow to call hiveOCM.Close()
through an inline deferred function and pass its error to cmdutil.CheckErr,
keeping the handling consistent with the existing close behavior in the same
command path.
- Line 153: The calls in the cleanup flow are using fmt.Sprintf with a constant
string and no formatting arguments, which triggers the lint issue. In
cleanup.go, update the k8s.NewAsBackplaneClusterAdminWithConn calls in the
cleanup path to pass the string literal directly instead of wrapping it in
fmt.Sprintf, keeping the existing hive.ID(), kclient.Options, hiveOCM, and
c.reason arguments unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5f34771-4960-4549-9cdb-9f27b911e5b9
📒 Files selected for processing (3)
cmd/cluster/access/access.gocmd/cluster/access/cleanup.gocmd/cluster/access/cleanup_test.go
…l flag break-glass cleanup was not detecting GCP Private Service Connect (PSC) clusters, causing it to fall through to the local KUBECONFIG path instead of cleaning up jump pods on hive. Also adds --hive-ocm-url flag to the cleanup subcommand so it can connect to the correct hive shard when a non-default OCM environment was used during break-glass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
RaphaelBut
force-pushed
the
fix-rosaeng-16043
branch
from
4c1cffe to
3376ec8
Compare
|
@RaphaelBut: 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. |
| if cluster.AWS().PrivateLink() { | ||
| return c.dropPrivateLinkAccess(cluster) | ||
| isPscCluster := cluster.GCP().PrivateServiceConnect().ServiceAttachmentSubnet() != "" | ||
| if cluster.AWS().PrivateLink() || isPscCluster { |
There was a problem hiding this comment.
this was the missing part to do the cleanup
the rest of the PR adds the multi env hive-ocm-url support to test/make this possible in stage
1 participant
break-glass cleanup was not detecting GCP Private Service Connect (PSC) clusters, causing it to fall through to the local KUBECONFIG path instead of cleaning up jump pods on hive.
Also adds --hive-ocm-url flag to the cleanup subcommand so it can connect to the correct hive shard when a non-default OCM environment was used during break-glass. ( needed for testing in stage or if we would actually want to breakglass in stage at some point )
https://redhat.atlassian.net/browse/ROSAENG-16042
Summary by CodeRabbit
New Features
Bug Fixes
Tests