Accept protocol = ICMP on ClusterNetworkPolicies

[jsalatiel]

Describe the problem/challenge you have
I would like to allow ping as a ClusterNetworkPolicy.
Unfortunatelly ICMP is not supported yet.

Describe the solution you'd like
Accept protocol: ICMP on ClusterNetworkPolicy and maybe icmpType to describe the ICMP code allowed.

The current behaviour is wrong as explained here #3262.

[jsalatiel]

I ask this because even if k8s default netpol does not support it, calico GlobalNetworkPolicies do support it. And that helps a lot debugging connectivity.

[Dyanngg]

@GraysonWu Could you share some insights / do some scope investigation? Thanks

[jsalatiel]

@GraysonWu could you please share your insights on this? Is this change hard to implement? ( No developer here )

[GraysonWu]

Sorry for the late response. Somehow I missed this message.
From my understanding, it won't be hard to implement on our data path. OVS already has ICMP matching support.
Most work would be from the API perspective. Let me take another look and get a doc to discuss the implementation detail of this issue.

[jsalatiel]

@GraysonWu will that doc be publicly available?😅

[GraysonWu]

@jsalatiel After a few round discussions offline we got some ideas and here is the doc: https://docs.google.com/document/d/1TnAhJh-8-5XEU-wYetL5zQu_aVsXgItPWudpTmoPnaI/edit?usp=sharing
Feel free to leave a comment.

[jsalatiel]

Hi @GraysonWu , I've read the documentation. Just perfect!
Thanks!

[jsalatiel]

@GraysonWu In which release can icmp support be expected then? 😅

[GraysonWu]

I'd like to say in 1.7

[GraysonWu]

Had another round of discussion in the community meeting. We decided not to remove ports field. Users are familiar with ports field, and the usage of ICMP is much smaller than other protocols. If users want to use ICMP or they prefer the new struct, they could choose to use Protocols as an advanced feature. Otherwise, they could keep using ports. Also, in this way, we don’t need to bump up the API version.

type Rule struct {
   Action    *RuleAction
   Ports []NetworkPolicyPort
   Protocols []Protocol
   ...
}

type Protocol struct {
   TCP *L4Protocol
   UDP *L4Protocol
   SCTP *L4Protocol
   ICMP *ICMPProtocol
}

type L4Protocol struct {
   Port *intstr.IntOrString
   EndPort *int32
}

type ICMPProtocol struct {
   ICMPType *int32
   ICMPCode *int32
}

[jsalatiel]

@GraysonWu Understood, tks! Probably not having to bump up the api version is a good thing.
What exactly would be the syntax to allow ECHO REQUESTS ( ping ) then ? if you can add an example it would be great.

[GraysonWu]

@jsalatiel NP. This is an example.

ingress:
- action: allow
  protocols:
  - icmp:   // matching Ping request
       icmpType: 8
       icmpCode: 0

[jsalatiel]

Thanks!, @GraysonWu
Still targeted for 1.7 release?

[GraysonWu]

Thanks!, @GraysonWu Still targeted for 1.7 release?

Yes.

[jsalatiel]

@GraysonWu when it gets merged to master I will deploy the latest image in my test environment, do several tests, and let you know if I see any problems.