CORENET-6813: Add config override for allow-icmp-network-policy#2920
CORENET-6813: Add config override for allow-icmp-network-policy#2920arkadeepsen wants to merge 1 commit intoopenshift:masterfrom
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. |
WalkthroughAdds support for an allow-icmp-network-policy override: template flag injection, parsing and validation in ovn_kubernetes rendering, and unit tests covering present, 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v0.8.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.38.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17231 characters] ... ired in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20250922181213-ec3ebc5fd46b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.34.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.34.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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 |
|
[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 |
|
/retest-required |
1 similar comment
|
/retest-required |
openshift-merge-robot
added
the
needs-rebase
The allow-icmp-network-policy flag can be set to true when starting ovnkube-controller to allow ICMP/ICMPv6 traffic when using network policy.
arkadeepsen
force-pushed
the
allow-icmp-netpol
branch
from
aac499f to
f7a5fff
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-merge-robot
removed
the
needs-rebase
|
@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.
🧹 Nitpick comments (2)
pkg/network/ovn_kubernetes_test.go (1)
4360-4381: Make these assertions less formatting-sensitive.These subtests depend on an exact multiline shell fragment, including indentation and the current empty-value branch shape. That will break on harmless template cleanup. Matching the flag assignment substring, like the nearby
openflow-probetest, would keep this focused on behavior.🤖 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 4360 - 4381, The test assertions are too format-sensitive; update the three subtests in pkg/network/ovn_kubernetes_test.go that call renderWithOverrides and assert on a multi-line, indented shell fragment to instead assert only on the relevant flag assignment substring. Locate the checks using ContainSubstring that reference the allow_icmp_network_policy block and replace them with asserts that match the assignment (e.g., look for allow_icmp_network_policy_flag and the --allow-icmp-network-policy= token or the full assignment including "true" for the true override) so the test verifies presence and value of the flag without relying on exact whitespace or multi-line shape.pkg/network/ovn_kubernetes.go (1)
192-202: Consider extracting config-override parsing into a helper.This follows the
openflow-probeflow almost line-for-line. A small helper for trim/validate/warn/set would keep future overrides consistent and make this section easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/network/ovn_kubernetes.go` around lines 192 - 202, Extract the repeated trim/validate/warn/set logic into a small helper (e.g., SetBoolOverrideFromConfigOverrides or parseBoolOverride) that accepts the ConfigOverrides map, the override key ("allow-icmp-network-policy"), the target data map (data.Data) and target field name ("AllowICMPNetworkPolicy"); inside the helper trim the raw string, return early on empty, attempt strconv.ParseBool and on error call klog.Warningf with the same message, otherwise set the validated value into data.Data; then replace the inline block in the OVNKubernetesConfig handling with a single call to that helper to keep behavior identical and reusable (use the same klog and strconv imports/symbols).
🤖 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 4360-4381: The test assertions are too format-sensitive; update
the three subtests in pkg/network/ovn_kubernetes_test.go that call
renderWithOverrides and assert on a multi-line, indented shell fragment to
instead assert only on the relevant flag assignment substring. Locate the checks
using ContainSubstring that reference the allow_icmp_network_policy block and
replace them with asserts that match the assignment (e.g., look for
allow_icmp_network_policy_flag and the --allow-icmp-network-policy= token or the
full assignment including "true" for the true override) so the test verifies
presence and value of the flag without relying on exact whitespace or multi-line
shape.
In `@pkg/network/ovn_kubernetes.go`:
- Around line 192-202: Extract the repeated trim/validate/warn/set logic into a
small helper (e.g., SetBoolOverrideFromConfigOverrides or parseBoolOverride)
that accepts the ConfigOverrides map, the override key
("allow-icmp-network-policy"), the target data map (data.Data) and target field
name ("AllowICMPNetworkPolicy"); inside the helper trim the raw string, return
early on empty, attempt strconv.ParseBool and on error call klog.Warningf with
the same message, otherwise set the validated value into data.Data; then replace
the inline block in the OVNKubernetesConfig handling with a single call to that
helper to keep behavior identical and reusable (use the same klog and strconv
imports/symbols).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6cd5637-c9d9-4820-bfba-a13e9347d4b1
📒 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
|
Pre merge testing done with network policy for ICMP and ICMPv6. |
|
/retest-required |
|
@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. |
5 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.Summary by CodeRabbit
New Features
Refactor
Tests