CORENET-6813: Add config override for allow-icmp-network-policy#2920
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@arkadeepsen: This pull request references CORENET-6854 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds an allow-icmp-network-policy render path: template flag variable and injection, reading/trimming/boolean validation from config overrides in renderOVNKubernetes, and unit tests covering configured, absent, and invalid override cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
|
Upstream PR which implemented the feature: ovn-kubernetes/ovn-kubernetes#5247 /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@arkadeepsen: This pull request references CORENET-6854 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
arkadeepsen
changed the title
|
@arkadeepsen: This pull request references CORENET-6813 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes_test.go (1)
4338-4357: Harden these assertions to avoid whitespace-sensitive failures and add explicitfalsecoverage.At Line 4338 and Line 4354, multiline block matching is fragile to harmless template formatting changes. Prefer targeted token assertions (e.g., flag/value substrings) and add a
falseoverride case to complete boolean-path coverage.✅ Suggested test refinement
t.Run("with allow-icmp-network-policy override", func(t *testing.T) { ovnkubeScriptLib := renderWithOverrides(map[string]string{"allow-icmp-network-policy": "true"}) - g.Expect(ovnkubeScriptLib).To(ContainSubstring(` - if [[ "true" != "" ]]; then - allow_icmp_network_policy_flag="--allow-icmp-network-policy=true" - fi`)) + g.Expect(ovnkubeScriptLib).To(ContainSubstring(`allow_icmp_network_policy_flag="--allow-icmp-network-policy=true"`)) }) + t.Run("with allow-icmp-network-policy=false override", func(t *testing.T) { + ovnkubeScriptLib := renderWithOverrides(map[string]string{"allow-icmp-network-policy": "false"}) + g.Expect(ovnkubeScriptLib).To(ContainSubstring(`allow_icmp_network_policy_flag="--allow-icmp-network-policy=false"`)) + }) + t.Run("without allow-icmp-network-policy override", func(t *testing.T) { ovnkubeScriptLib := renderWithOverrides(nil) - g.Expect(ovnkubeScriptLib).To(ContainSubstring(` - if [[ "" != "" ]]; then - allow_icmp_network_policy_flag="--allow-icmp-network-policy=" - fi`)) + g.Expect(ovnkubeScriptLib).To(ContainSubstring(`if [[ "" != "" ]]; then`)) }) t.Run("with invalid allow-icmp-network-policy override", func(t *testing.T) { ovnkubeScriptLib := renderWithOverrides(map[string]string{"allow-icmp-network-policy": "-60"}) - g.Expect(ovnkubeScriptLib).To(ContainSubstring(` - if [[ "" != "" ]]; then - allow_icmp_network_policy_flag="--allow-icmp-network-policy=" - fi`)) + g.Expect(ovnkubeScriptLib).To(ContainSubstring(`if [[ "" != "" ]]; then`)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/network/ovn_kubernetes_test.go` around lines 4338 - 4357, The tests in ovn_kubernetes_test.go use fragile multiline substring matches for the allow-icmp-network-policy flag; update the three cases that call renderWithOverrides to assert targeted tokens instead (e.g., check for the presence of "--allow-icmp-network-policy=true" when override is "true", check for "--allow-icmp-network-policy=" for empty/invalid overrides) and add an explicit test case using renderWithOverrides(map[string]string{"allow-icmp-network-policy":"false"}) that asserts the flag "--allow-icmp-network-policy=false" appears; locate assertions around the renderWithOverrides calls in the t.Run blocks for "with allow-icmp-network-policy override", "without allow-icmp-network-policy override", and "with invalid allow-icmp-network-policy override" and replace multiline ContainSubstring checks with focused token checks to avoid whitespace sensitivity and to cover the false path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 4338-4357: The tests in ovn_kubernetes_test.go use fragile
multiline substring matches for the allow-icmp-network-policy flag; update the
three cases that call renderWithOverrides to assert targeted tokens instead
(e.g., check for the presence of "--allow-icmp-network-policy=true" when
override is "true", check for "--allow-icmp-network-policy=" for empty/invalid
overrides) and add an explicit test case using
renderWithOverrides(map[string]string{"allow-icmp-network-policy":"false"}) that
asserts the flag "--allow-icmp-network-policy=false" appears; locate assertions
around the renderWithOverrides calls in the t.Run blocks for "with
allow-icmp-network-policy override", "without allow-icmp-network-policy
override", and "with invalid allow-icmp-network-policy override" and replace
multiline ContainSubstring checks with focused token checks to avoid whitespace
sensitivity and to cover the false path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cee2851d-7ad3-4882-8da7-f44c1e3ea1d1
📒 Files selected for processing (3)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlpkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.go
|
we really need to refactor that code... |
openshift-ci
bot
added
lgtm
arkadeepsen
force-pushed
the
allow-icmp-netpol
branch
from
aac499f to
fb4e1f4
Compare
Tried to refactor the code. PLMK whether this needs any other changes or not. |
|
@arkadeepsen: This pull request references CORENET-6813 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 4270-4276: The test case entry for the config override
"allow-icmp-network-policy" currently has checkInvalid set to false so the
bool-validation path isn’t exercised; update that table entry in
pkg/network/ovn_kubernetes_test.go so checkInvalid is true and provide an
invalidValue (e.g., "not-a-bool" or "invalid") for the allow-icmp-network-policy
override to trigger the rejection path in the render/validation logic (look for
the test table row containing configOverrideKey: "allow-icmp-network-policy" and
adjust checkInvalid and invalidValue accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f70ef0c9-0744-4741-8f21-3c127d804816
📒 Files selected for processing (3)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlpkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/network/ovn-kubernetes/common/008-script-lib.yaml
arkadeepsen
force-pushed
the
allow-icmp-netpol
branch
from
fb4e1f4 to
43cba04
Compare
|
@arkadeepsen: This pull request references CORENET-6813 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
Oh, I didn't mean we should refactor it as part of a blocking PR. We can do it later.O (We should probably unparse the YAML into a config struct rather than parsing each piece by hand...) |
arkadeepsen
force-pushed
the
allow-icmp-netpol
branch
from
43cba04 to
aac499f
Compare
Ack. Reverted to the previous commit.
I'll probably create another PR to take care of the refactor. |
|
/lgtm |
|
/retest-required |
1 similar comment
|
/retest-required |
openshift-merge-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
verified
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp |
|
@danwinship: Overrode contexts on behalf of danwinship: ci/prow/e2e-metal-ipi-ovn-dualstack-bgp, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, danwinship 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 |
|
/verified by QE |
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-gcp-ovn |
|
/test e2e-gcp-ovn-upgrade |
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-gcp-ovn-upgrade |
|
/skip |
|
/test e2e-gcp-ovn |
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp |
|
@danwinship: Overrode contexts on behalf of danwinship: ci/prow/e2e-metal-ipi-ovn-dualstack-bgp, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw 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. |
|
/skip |
|
/tide refresh |
|
/test e2e-gcp-ovn |
|
/tide refresh |
|
/skip |
|
/test e2e-hypershift-aks |
|
/test hypershift-e2e-aks |
|
/skip |
|
/tide refresh |
1 similar comment
|
/tide refresh |
|
@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. |
7 participants
This PR adds config override for
allow-icmp-network-policy. Theallow-icmp-network-policyflag can be set to true when starting ovnkube-controller to allow ICMP/ICMPv6 traffic when using network policy.