API docs: clarify ICMP related. by fasaxc · Pull Request #364 · kubernetes-sigs/network-policy-api

fasaxc · 2026-03-31T13:06:01Z

Clarify that ICMP related packets (such as TOOBIG) are included when accepting traffic. Tighten up action descriptions and improve formatting in various places.

PR generated with assistance of AI (Claude).

netlify · 2026-03-31T13:06:56Z

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name	Link
🔨 Latest commit	`cadb73e`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-network-policy-api/deploys/69e88e5bdde88f00086dff9c
😎 Deploy Preview	https://deploy-preview-364--kubernetes-sigs-network-policy-api.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

danwinship

Should mention that the Networks peer selects both IP/IPv4 and ICMP/ICMPv6 traffic too.

danwinship · 2026-03-31T21:18:33Z

-	//   the destination. No further ClusterNetworkPolicy or
-	//   NetworkPolicy rules will be processed.
+	// - Accept: Accepts the selected traffic, and ensures that response
+	//   traffic including "related" ICMP traffic is also allowed. No further


I'd at least put a comma before "including", but maybe "Accepts the selected traffic, including replies to that traffic and related ICMP traffic." (and likewise in the other places with the same text).

danwinship · 2026-03-31T21:21:16Z

-Each CNP should define at least one `Ingress` or `Egress` relevant in-cluster traffic flow 
-along with the associated Action that should occur. In each `gress` rule the user 
+Each CNP should define at least one `Ingress` or `Egress` relevant in-cluster traffic flow
+along with the associated Action that should occur. In each `gress` rule the user


I think there's an issue about getting rid of the non-word "gress" everywhere, so let's start here. You can just drop it entirely and say "each rule".

danwinship · 2026-03-31T21:23:26Z

-along with the associated Action that should occur. In each `gress` rule the user 
+Each CNP should define at least one `Ingress` or `Egress` relevant in-cluster traffic flow
+along with the associated Action that should occur. In each `gress` rule the user
 should AT THE MINIMUM define an `Action`, and at least one `ClusterNetworkPolicyPeer`.


Suggested change

should AT THE MINIMUM define an `Action`, and at least one `ClusterNetworkPolicyPeer`.

should *at the minimum* define an `Action`, and at least one peer (`To` or `From` entry).

danwinship · 2026-03-31T21:23:56Z

+along with the associated Action that should occur. In each `gress` rule the user
 should AT THE MINIMUM define an `Action`, and at least one `ClusterNetworkPolicyPeer`.
-Optionally the user may also define select `Ports` to filter traffic on and also 
+Optionally the user may also define select `Protocols` to filter traffic on and also


Suggested change

Optionally the user may also define select `Protocols` to filter traffic on and also

Optionally the user may also select `Protocols` to filter traffic on and also

danwinship · 2026-03-31T21:24:20Z

 should AT THE MINIMUM define an `Action`, and at least one `ClusterNetworkPolicyPeer`.
-Optionally the user may also define select `Ports` to filter traffic on and also 
+Optionally the user may also define select `Protocols` to filter traffic on and also
 a name for each rule to make management and reporting easier for Admins.


Suggested change

a name for each rule to make management and reporting easier for Admins.

a `Name` for each rule to make management and reporting easier for Admins.

linux-foundation-easycla · 2026-04-02T09:54:07Z

The committers listed above are authorized under a signed CLA.

✅ login: fasaxc / name: Shaun Crampton (cadb73e)

tssurya

can't tell why the CI is red https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_network-policy-api/364/pull-network-policy-api-verify/2039649007680098304
very weird "Test started last Thursday at 12:19 PM failed after 49h42m18s."

tssurya · 2026-04-05T11:59:45Z

-	// - Accept: Accepts the selected traffic, allowing it into
-	//   the destination. No further ClusterNetworkPolicy or
-	//   NetworkPolicy rules will be processed.
+	// - Accept: Accepts the selected traffic, including replies to that


We spoke a bit about this during AMS KubeCon,
but a note for future that since we have made this explicit we do need to consider this while designing the ICMP protocol match whether ICMP protocol match will also allow related or if its purely restricted to specific independent types and codes.
just highlighting it for history (no action required from this comment)

There's some trickiness here, but I would hope that conntrack deals with it all for us.

If you allow ICMP pings, you would also have implicitly be allowing the replies to that ping, just like in the IP case.

tssurya · 2026-04-05T12:01:15Z

-	// ClusterNetworkPolicyRuleActionAccept indicates that
-	// matching traffic will be accepted and no further policy
-	// evaluation will be done. This is a final decision.
+	// ClusterNetworkPolicyRuleActionAccept stops further rule processing of


hmm interesting..I am trying to find the nuance/motivation behind the change here and for the other actions..
so ACCEPT in ingress rules being hit doesn't mean the policy stops being evaluated but we still evaluate the egress rules?
OR
is there something between policy v/s rules?

Traffic from pod A to pod B goes through pod A's egress rules and pod B's ingress rules. The change was intended to hint that pod A's "egress accept" may still get blocked by pod B's ingress rules.

TBH I started just trying to make the language more direct. "stops and accepts" instead of "indicates that ..."

tssurya · 2026-04-05T12:07:16Z

-	// - Deny: Drops the selected traffic. No further
-	//   ClusterNetworkPolicy or NetworkPolicy rules will be
-	//   processed.
+	// - Deny: Drops the selected traffic. No further ClusterNetworkPolicy or


I recently learnt that networkpolicies when created mid way do expect to effect existing connections? so for both DENY and PASS it actually does include ICMP related as well for that connection?

For networkingv1.NetworkPolicy there is no official expectation about what happens when you create a NetworkPolicy that matches a pre-existing connection. For CNP we're leaning toward "it breaks existing connections" but we never finished that discussion (#305).

tssurya · 2026-04-05T12:13:49Z

+Each CNP should define at least one `Ingress` or `Egress` relevant in-cluster traffic flow
+along with the associated Action that should occur. In each rule the user
+should *at the minimum* define an `Action`, and at least one peer (`To` or `From` entry).
+Optionally the user may also select `Protocols` to filter traffic on and also


I would remove this from this PR since https://github.com/kubernetes-sigs/network-policy-api/pull/363/changes#diff-a6c5effcce61a6f6fa4724456fa5ef6c7cf5d72306dce662570e1429c6776e3dR124 there is an earlier PR here that is doing that change so I can review that separately..the changes to the first part To and From makes sense
I don't want to leave the other contributor hanging (sweat smiley)

danwinship · 2026-04-06T13:15:50Z

 	//
+	// Networks matches both regular IP traffic and ICMP traffic to/from
+	// the specified CIDRs. For example, a Networks entry of "0.0.0.0/0"
+	// will match both IPv4 and ICMP traffic, while "0::/0" will match


danwinship · 2026-04-20T18:09:55Z

lgtm, but needs a rebase

danwinship · 2026-04-20T18:10:06Z

and can you squash the fixup commits?

fasaxc · 2026-04-21T14:34:19Z

Done

Note that ICMP related packets (such as TOOBIG) are allowed when allowing the main protocol connection. Improve wording in various places.

tssurya

/lgtm

k8s-ci-robot · 2026-04-22T17:28:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fasaxc, tssurya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tssurya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 31, 2026

k8s-ci-robot requested review from danwinship and npinaeva March 31, 2026 13:06

danwinship reviewed Mar 31, 2026

View reviewed changes

fasaxc force-pushed the improve-icmp-docs branch from 4538b54 to d158588 Compare April 2, 2026 10:19

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 2, 2026

tssurya reviewed Apr 5, 2026

View reviewed changes

danwinship reviewed Apr 6, 2026

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2026

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2026

fasaxc force-pushed the improve-icmp-docs branch from 082cea5 to 2a1070f Compare April 20, 2026 14:58

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2026

fasaxc force-pushed the improve-icmp-docs branch from 2a1070f to ee67a7d Compare April 21, 2026 14:17

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2026

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2026

Clarify ICMP handling and improve action wording

cadb73e

Note that ICMP related packets (such as TOOBIG) are allowed when allowing the main protocol connection. Improve wording in various places.

fasaxc force-pushed the improve-icmp-docs branch from ee67a7d to cadb73e Compare April 22, 2026 09:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2026

tssurya approved these changes Apr 22, 2026

View reviewed changes

k8s-ci-robot assigned tssurya Apr 22, 2026

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2026

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2026

k8s-ci-robot merged commit 1c9af17 into kubernetes-sigs:main Apr 22, 2026
10 checks passed

	should AT THE MINIMUM define an `Action`, and at least one `ClusterNetworkPolicyPeer`.
	should at the minimum define an `Action`, and at least one peer (`To` or `From` entry).

	Optionally the user may also define select `Protocols` to filter traffic on and also
	Optionally the user may also select `Protocols` to filter traffic on and also

	a name for each rule to make management and reporting easier for Admins.
	a `Name` for each rule to make management and reporting easier for Admins.

Conversation

fasaxc commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!

Uh oh!

danwinship left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tssurya left a comment

Choose a reason for hiding this comment

Uh oh!

tssurya Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tssurya Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship commented Apr 20, 2026

Uh oh!

danwinship commented Apr 20, 2026

Uh oh!

fasaxc commented Apr 21, 2026

Uh oh!

tssurya left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fasaxc commented Mar 31, 2026 •

edited

Loading

netlify Bot commented Mar 31, 2026 •

edited

Loading

linux-foundation-easycla Bot commented Apr 2, 2026 •

edited

Loading

tssurya Apr 5, 2026 •

edited

Loading

danwinship Apr 6, 2026 •

edited

Loading

tssurya Apr 5, 2026 •

edited

Loading