[ROSAENG-1070] feat: Support backplane elevation for network verifier pod mode#891
Merged
openshift-merge-bot[bot] merged 2 commits intoMay 15, 2026
Merged
Conversation
WalkthroughAdds an elevation "reason" to pod-mode egress verification: ChangesPod-Mode Elevation Reason
Sequence DiagramsequenceDiagram
participant User as Client
participant CLI
participant Val as Validator
participant K8s as pkg/k8s
participant Back as BackplaneAuth
participant Cluster
Client->>CLI: run osdctl network verify-egress --pod-mode --cluster-id [--reason]
CLI->>Val: build EgressVerification, validate inputs
Val-->>CLI: validation OK / error (requires --reason)
CLI->>K8s: request REST config (cluster-id, elevationReasons?)
alt Reason provided
K8s->>Back: load backplane config & request rest config as "backplane-cluster-admin" (with reason)
Back-->>K8s: elevated rest.Config
else No reason
K8s->>Back: load backplane config & request standard backplane REST config
Back-->>K8s: non-elevated rest.Config
end
K8s->>Cluster: use rest.Config to operate against cluster
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/network/verification.go (1)
255-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
--reasonafter HCP auto-switches to pod mode.
validateInput()runs beforeLine 255can force pod mode for HCP. That allowsLine 677to be skipped, and execution can proceed with non-elevated backplane creds in pod mode.💡 Proposed fix
@@ if !e.PodMode && platform == cloud.AWSHCP { e.log.Info(ctx, "Cluster is HCP - forcing pod mode.") e.PodMode = true } + + if e.PodMode && e.ClusterId != "" && e.KubeConfig == "" && strings.TrimSpace(e.Reason) == "" { + log.Fatalf("pod mode with --cluster-id requires --reason flag for elevation (write operations need backplane-cluster-admin). Example: --reason 'PD-12345' or --reason 'OHSS-67890'") + }Also applies to: 676-679
🤖 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/network/verification.go` around lines 255 - 263, After auto-switching to pod mode for HCP in the block that sets e.PodMode = true when platform == cloud.AWSHCP, ensure the presence of the --reason flag (or non-empty e.Reason) is enforced before proceeding: either re-run the higher-level validation (call validateInput()) or add an explicit check that e.Reason (the CLI --reason flag) is set and return a fatal error if it is missing; update the logic near the e.PodMode assignment so validatePodModeCompatibility() cannot be reached without confirming --reason, referencing e.PodMode, platform/cloud.AWSHCP, validateInput(), and validatePodModeCompatibility() to locate where to insert the check.
🧹 Nitpick comments (1)
cmd/network/verification_test.go (1)
913-934: ⚡ Quick winAlign
expectErrorvalues with the actual assertion flow.These new cases set
expectError: false, but the test body still expectserr != nilfor non-default-kubeconfig paths. This makes table semantics misleading.💡 Proposed fix
{ name: "priority_2_backplane_credentials_with_elevation", @@ - expectError: false, + expectError: true, expectedResult: "backplane credentials with elevation should be used", }, { name: "priority_2_backplane_credentials_without_elevation", @@ - expectError: false, + expectError: true, expectedResult: "backplane credentials without elevation should be used", },🤖 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/network/verification_test.go` around lines 913 - 934, The table-driven tests for the two cases "priority_2_backplane_credentials_with_elevation" and "priority_2_backplane_credentials_without_elevation" have expectError set to false while the test body asserts an error for non-default kubeconfig paths; update the table to reflect the actual assertion flow by setting expectError: true for these EgressVerification entries (or alternatively change the test body to assert no error), referencing the test case names and the EgressVerification struct fields (ClusterId, KubeConfig, log) so the expectation matches the err check in the test runner.
🤖 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 `@cmd/network/verification.go`:
- Around line 729-737: The logs currently announcing credential selection are
emitted before checking the result of k8s.NewRestConfigAsBackplaneClusterAdmin /
k8s.NewRestConfig, which can mislead when restConfig creation fails; update the
block that sets restConfig, err (using e.Reason, e.ClusterId) so that you first
call the appropriate constructor, check err, and only call e.log.Info(ctx, ...)
after err == nil to confirm successful credential creation—ensure any logging on
error uses e.log.Error or similar and includes err details.
---
Outside diff comments:
In `@cmd/network/verification.go`:
- Around line 255-263: After auto-switching to pod mode for HCP in the block
that sets e.PodMode = true when platform == cloud.AWSHCP, ensure the presence of
the --reason flag (or non-empty e.Reason) is enforced before proceeding: either
re-run the higher-level validation (call validateInput()) or add an explicit
check that e.Reason (the CLI --reason flag) is set and return a fatal error if
it is missing; update the logic near the e.PodMode assignment so
validatePodModeCompatibility() cannot be reached without confirming --reason,
referencing e.PodMode, platform/cloud.AWSHCP, validateInput(), and
validatePodModeCompatibility() to locate where to insert the check.
---
Nitpick comments:
In `@cmd/network/verification_test.go`:
- Around line 913-934: The table-driven tests for the two cases
"priority_2_backplane_credentials_with_elevation" and
"priority_2_backplane_credentials_without_elevation" have expectError set to
false while the test body asserts an error for non-default kubeconfig paths;
update the table to reflect the actual assertion flow by setting expectError:
true for these EgressVerification entries (or alternatively change the test body
to assert no error), referencing the test case names and the EgressVerification
struct fields (ClusterId, KubeConfig, log) so the expectation matches the err
check in the test runner.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81518ba9-9394-4dca-9ab7-3bb9783816bd
📒 Files selected for processing (4)
cmd/network/verification.gocmd/network/verification_pod_mode_test.gocmd/network/verification_test.gopkg/k8s/client.go
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao, joshbranham 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
Contributor
Contributor
|
@feichashao: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Assisted by Claude Code. Reviewed and validated by Human.
What this PR changes?
This PR adds backplane elevation support for running network verifier in pod mode:
--reasonflag in a similar way like other osdctl commands. If--reasonis provided, it will run as backplane-cluster-admin.pkg/k8s/client.goto get the kubeconfig with impersonation.Why?
We have change the backplane RBAC recently, SREP by default has read-only permissions. Users will see the below error when running in pod mode without elevation:
Validation
After the change in this PR, the osdctl can create the job successfully:
Notes to reviewer: There's a watch issue that may lead to network-verifier hangs watching for the job finish. That is a separated issue and I raised a separated issue to address it openshift/osd-network-verifier#347 .
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests