OCPBUGS-88719: Use AdminPolicyBasedExternalRoute CR for external gateway test#31293
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughRefactors external gateway tests from validating pod-creation success/failure to validating APB policy sync state by polling ovnkube-node logs. Updates utility layer to render AdminPolicyBasedExternalRoute manifests and change pod creation to return full Pod objects. Implements three test scenarios (IPv6, IPv4, Dual Stack) that apply explicit APB policies, create fixed-name test pods, and assert sync outcomes based on cluster IP-family compatibility. ChangesAPB External Gateway Policy Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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: 3
🧹 Nitpick comments (2)
test/extended/networking/util.go (1)
322-336: 💤 Low valueConsider quoting LabelKey in the template to prevent potential injection.
At line 331,
{{ .LabelKey }}is not quoted, while{{ .LabelValue | quote }}is. IfLabelKeywere to come from user input or external sources in the future, this could allow template injection or YAML structure manipulation.While the current usage hardcodes
labelKeyas"test"(line 26 in external_gateway.go), applying the defense-in-depth principle suggests quoting all dynamic values in templates.Proposed defense-in-depth improvement
from: namespaceSelector: matchLabels: - {{ .LabelKey }}: {{ .LabelValue | quote }} + {{ .LabelKey | quote }}: {{ .LabelValue | quote }}Note: YAML keys should remain unquoted for standard label names, so this may require careful testing to ensure label selectors work correctly with quoted keys.
🤖 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/extended/networking/util.go` around lines 322 - 336, The template adminPolicyBasedExternalRouteTemplate uses an unquoted dynamic label key (LabelKey) which can allow template/YAML injection; update the template to quote or escape the LabelKey the same way LabelValue is handled (apply the template's quoting/escaping filter to .LabelKey), run tests to confirm the AdminPolicyBasedExternalRoute's namespaceSelector.matchLabels still works as expected, and validate with the code that constructs labelKey (external_gateway.go) to ensure no functional change.test/extended/networking/external_gateway.go (1)
48-48: 💤 Low valueConsider using a proper context instead of context.TODO().
While
context.TODO()is acceptable for test code, using a proper context with timeout would improve clarity and allow better control over the polling behavior. The context could be created at the test function level and passed through.🤖 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/extended/networking/external_gateway.go` at line 48, The wait.PollUntilContextTimeout call currently uses context.TODO(); replace it with a proper context created at the test level (e.g., ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)) and pass that ctx into wait.PollUntilContextTimeout instead of context.TODO(), and ensure you defer cancel() to release resources; locate the call to wait.PollUntilContextTimeout(...) in external_gateway.go and update callers to use the new ctx so the poll uses the intended timeout and cancellation semantics.
🤖 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/extended/networking/external_gateway.go`:
- Around line 48-69: The DualStack case is unhandled causing timeouts; update
the polling lambda used with wait.PollUntilContextTimeout to treat DualStack
like the IPv6 success path: in the switch on podIPFamily (where DualStack
currently has an empty case) check logs with strings.Contains for
fmt.Sprintf(successLog, apbPolicyName, f.Namespace.Name, podName) and return
true,nil if found (use the same check as the IPv6 branch); do this for both
occurrences of the switch so DualStack clusters detect the success pattern
instead of falling through to a timeout.
- Around line 110-112: Accessing pod.Status.PodIPs before checking the result of
createPod can panic if createPod returned an error; change the order to verify
the error from createPod (the variable err returned by createPod) with
expectNoError(err) or an explicit nil check before touching pod, then only read
pod.Status.PodIPs into podIPs after the success check—look for the
createPod(...) call, the pod variable, pod.Status.PodIPs and the expectNoError
invocation to update.
In `@test/extended/networking/util.go`:
- Around line 530-544: In createPod, the inner closure shadows the outer
retrievedPod by using :=; change the declaration inside the wait.PollImmediate
closure to assign to the outer variable (use = instead of :=) so the
retrievedPod set inside the loop is returned by the function; ensure the closure
still returns the correct (bool, error) values and references
execPod.Namespace/execPod.Name as it currently does.
---
Nitpick comments:
In `@test/extended/networking/external_gateway.go`:
- Line 48: The wait.PollUntilContextTimeout call currently uses context.TODO();
replace it with a proper context created at the test level (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), 2*time.Minute)) and pass that ctx into
wait.PollUntilContextTimeout instead of context.TODO(), and ensure you defer
cancel() to release resources; locate the call to
wait.PollUntilContextTimeout(...) in external_gateway.go and update callers to
use the new ctx so the poll uses the intended timeout and cancellation
semantics.
In `@test/extended/networking/util.go`:
- Around line 322-336: The template adminPolicyBasedExternalRouteTemplate uses
an unquoted dynamic label key (LabelKey) which can allow template/YAML
injection; update the template to quote or escape the LabelKey the same way
LabelValue is handled (apply the template's quoting/escaping filter to
.LabelKey), run tests to confirm the AdminPolicyBasedExternalRoute's
namespaceSelector.matchLabels still works as expected, and validate with the
code that constructs labelKey (external_gateway.go) to ensure no functional
change.
🪄 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: 6b72cb40-a8ea-48aa-bbe0-9feba28cf622
📒 Files selected for processing (2)
test/extended/networking/external_gateway.gotest/extended/networking/util.go
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
dd910f5 to
ded79e4
Compare
|
Scheduling required tests: |
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
ded79e4 to
69561e8
Compare
|
Scheduling required tests: |
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
69561e8 to
dc6898d
Compare
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
/retest-required |
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
dc6898d to
9744ea2
Compare
|
/retest |
|
Scheduling required tests: |
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
9744ea2 to
707c2f4
Compare
|
@coderabbitai resume |
|
✅ Action performedReviews resumed. Review finished.
|
|
Scheduling required tests: |
|
Job Failure Risk Analysis for sha: 707c2f4
|
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
707c2f4 to
d17bb4e
Compare
|
Scheduling required tests: |
|
/test e2e-aws-ovn-fips |
1 similar comment
|
/test e2e-aws-ovn-fips |
arkadeepsen
force-pushed
the
use-apber-for-ex-gw
branch
from
d17bb4e to
e8c8c0f
Compare
|
Scheduling required tests: |
|
/test e2e-aws-ovn-fips |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/ovn-kubernetes#3249 |
|
@arkadeepsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/04da61f0-69f9-11f1-866e-33093e39c8c8-0 |
The following test passed: : [sig-network] external gateway address when using openshift ovn-kubernetes should match the address family of the pod [Suite:openshift/conformance/parallel] |
tssurya
left a comment
tssurya
left a comment
There was a problem hiding this comment.
/lgtm
I didn't review in depth since functionally not much changes, just the API surface is changing, CI is passing so that's great
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, tssurya 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
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@arkadeepsen: 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. |
|
/test e2e-aws-ovn-fips |
|
/retest |
|
/test e2e-aws-ovn-fips |
|
Job Failure Risk Analysis for sha: c389785
|
1 similar comment
|
Job Failure Risk Analysis for sha: c389785
|
|
/test e2e-aws-ovn-fips |
|
Job Failure Risk Analysis for sha: c389785
|
1 similar comment
|
Job Failure Risk Analysis for sha: c389785
|
|
@arkadeepsen: 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. |
|
@arkadeepsen: Jira Issue Verification Checks: Jira Issue OCPBUGS-88719 Jira Issue OCPBUGS-88719 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. |
|
/jira backport release-4.22,release-4.21 |
|
@arkadeepsen: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: 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. |
|
@openshift-ci-robot: new pull request created: #31316 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. |
|
@openshift-ci-robot: new pull request created: #31317 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-19-155631 |
5 participants
This PR changes existing external gateway tests to use AdminPolicyBasedExternalRoute CR instead of annotations.
Summary by CodeRabbit
Tests
Refactor