Use AdminPolicyBasedExternalRoute CR for external gateway test#31293
Use AdminPolicyBasedExternalRoute CR for external gateway test#31293arkadeepsen wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRefactors external gateway tests to render and apply AdminPolicyBasedExternalRoute manifests, poll OVN kube pod logs for policy sync or errors, and derive IP-family from created Pod.Status.PodIPs. ChangesExternal Gateway Address Test Refactoring
🎯 3 (Moderate) | ⏱️ ~20 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arkadeepsen The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
|
@arkadeepsen: The following tests failed, say
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. |
1 participant
This PR changes existing external gateway tests to use AdminPolicyBasedExternalRoute CR instead of annotations.
Summary by CodeRabbit