[SREP-3696] Run the egress verifier in pod mode on HCP clusters#858
Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom Mar 9, 2026
Merged
[SREP-3696] Run the egress verifier in pod mode on HCP clusters#858openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
Conversation
WalkthroughAdds runtime validation for Pod mode by introducing validatePodModeCompatibility(), forces Pod mode for hypershift-enabled clusters in Run, and removes setup-time mutual exclusivity between Pod mode and certain cloud-provider flags; errors are returned when incompatible flags are set with Pod mode. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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
🧹 Nitpick comments (2)
cmd/network/verification_pod_mode_test.go (2)
437-444: MisleadingerrorMsgfield in test case.The
errorMsgis set to"--ca-cert"butwantErrorisfalse. SinceerrorMsgis only checked whenwantErroristrue, this is dead code and could confuse future readers.🔧 Suggested fix
{ name: "non_pod_mode", ev: &EgressVerification{ PodMode: false, }, wantError: false, - errorMsg: "--ca-cert", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/network/verification_pod_mode_test.go` around lines 437 - 444, The test case named "non_pod_mode" in the table of tests for EgressVerification sets wantError=false but still provides an errorMsg of "--ca-cert", which is dead/confusing; remove the errorMsg value (or set it to an empty string) for that test case (the struct literal with name "non_pod_mode" and ev: &EgressVerification{PodMode: false}) so only failing cases include errorMsg and readers aren’t misled.
430-531: Missing test case for pod mode happy path.The test cases cover various conflicting flag scenarios, but there's no test case verifying that pod mode enabled with no conflicting flags returns no error. Adding this would ensure the happy path is explicitly covered.
🧪 Suggested test case to add
tests := []struct { name string ev *EgressVerification wantError bool errorMsg string }{ + { + name: "pod_mode_no_conflicts", + ev: &EgressVerification{ + PodMode: true, + }, + wantError: false, + }, { name: "non_pod_mode",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/network/verification_pod_mode_test.go` around lines 430 - 531, Add a test case to TestValidatePodModeCompatibility that verifies the pod-mode happy path: create an entry named like "pod_mode_happy_path" with ev set to &EgressVerification{PodMode: true} (no CaCert, SubnetIds, SecurityGroupId, AllSubnets, CpuArchName, GcpProjectID, VpcName) and wantError: false, then the existing loop will call ev.validatePodModeCompatibility() and assert no error; this uses the existing EgressVerification struct and validatePodModeCompatibility method so it fits into the current test table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/network/verification.go`:
- Around line 239-242: The code dereferences e.cluster without checking for nil
when running without --cluster-id, causing a panic in the block that reads
e.cluster.Hypershift().Enabled(); fix by guarding that access — only call
e.cluster.Hypershift().Enabled() (and run the e.log.Info/PodMode assignment)
when e.cluster is non-nil (or ensure fetchCluster() sets e.cluster); update the
conditional around PodMode (the block that references e.cluster, Hypershift(),
Enabled(), e.log.Info and PodMode) to perform a nil-check on e.cluster before
calling its methods so the command works when no cluster-id is provided.
---
Nitpick comments:
In `@cmd/network/verification_pod_mode_test.go`:
- Around line 437-444: The test case named "non_pod_mode" in the table of tests
for EgressVerification sets wantError=false but still provides an errorMsg of
"--ca-cert", which is dead/confusing; remove the errorMsg value (or set it to an
empty string) for that test case (the struct literal with name "non_pod_mode"
and ev: &EgressVerification{PodMode: false}) so only failing cases include
errorMsg and readers aren’t misled.
- Around line 430-531: Add a test case to TestValidatePodModeCompatibility that
verifies the pod-mode happy path: create an entry named like
"pod_mode_happy_path" with ev set to &EgressVerification{PodMode: true} (no
CaCert, SubnetIds, SecurityGroupId, AllSubnets, CpuArchName, GcpProjectID,
VpcName) and wantError: false, then the existing loop will call
ev.validatePodModeCompatibility() and assert no error; this uses the existing
EgressVerification struct and validatePodModeCompatibility method so it fits
into the current test table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b5143d3-40b0-4c28-b29a-101c30331eb7
📒 Files selected for processing (2)
cmd/network/verification.gocmd/network/verification_pod_mode_test.go
Contributor
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
gvnnn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/network/verification.go (1)
239-242:⚠️ Potential issue | 🔴 CriticalStill need the nil guard before HCP detection.
e.clusteris still nil on the supported no---cluster-idpath, soe.cluster.Hypershift().Enabled()can panic before the new pod-mode compatibility validation runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/network/verification.go` around lines 239 - 242, The code checks e.cluster.Hypershift().Enabled() without verifying e.cluster is non-nil which can panic on the no---cluster-id path; update the block that sets e.PodMode (the if that currently reads if !e.PodMode && e.cluster.Hypershift().Enabled()) to first guard that e.cluster != nil (and only then call e.cluster.Hypershift().Enabled()), keeping the log and assignment to e.PodMode as-is; ensure the nil-check is performed before invoking Hypershift() or Enabled() to avoid nil dereference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/network/verification.go`:
- Around line 811-813: The code incorrectly marks "--cacert" as incompatible by
appending it to conflicts when e.CaCert is set; remove that classification so
the CA bundle is allowed in pod-mode. Specifically, delete (or stop executing)
the if block that checks e.CaCert and appends "--cacert" to the conflicts slice
(the e.CaCert check and conflicts = append(conflicts, "--cacert") line) so that
defaultValidateEgressInput() can continue to load the CA bundle into the proxy
config for pod-mode runs.
- Around line 827-829: The check using e.CpuArchName only detects the final
value and cannot tell if the user explicitly passed --cpu-arch; replace that
pattern with a Cobra Flag.Changed check: lookup the "cpu-arch" flag on
validateEgressCmd (e.g., validateEgressCmd.Flags().Lookup("cpu-arch")), ensure
the flag is non-nil and flag.Changed is true, and only then append "--cpu-arch"
to conflicts instead of relying on e.CpuArchName; this mirrors how other flags
in this function detect explicit usage.
---
Duplicate comments:
In `@cmd/network/verification.go`:
- Around line 239-242: The code checks e.cluster.Hypershift().Enabled() without
verifying e.cluster is non-nil which can panic on the no---cluster-id path;
update the block that sets e.PodMode (the if that currently reads if !e.PodMode
&& e.cluster.Hypershift().Enabled()) to first guard that e.cluster != nil (and
only then call e.cluster.Hypershift().Enabled()), keeping the log and assignment
to e.PodMode as-is; ensure the nil-check is performed before invoking
Hypershift() or Enabled() to avoid nil dereference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9ad0983-f3cb-42b9-aa06-1bfdf65524b0
📒 Files selected for processing (2)
cmd/network/verification.gocmd/network/verification_pod_mode_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/network/verification_pod_mode_test.go
Contributor
Author
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bergmannf, gvnnn 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
|
@gvnnn: 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.
Enforce pod mode if the cluster is HCP, regardless of whether
--pod-modewas provided on the command line.Also verify if any of the conflicting command line flags are provided after flag parsing to ensure we're comparing against the current run mode.
In the future this change might be moved to a dedicated code path, independent of input/flag validation.
Summary by CodeRabbit
New Features
Bug Fixes
Tests