Add support for VRRP and IGMP in host fw#39872
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
37cd550 to
6b88379
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6b88379 to
4c67cf7
Compare
7fed78d to
6c0b3ba
Compare
768049e to
e85b4fa
Compare
2612e11 to
8e5e17a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
8e5e17a to
c69a751
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
@squeed I've pushed a new version that extends the existing toPorts field in the policy spec to allow/deny these protocols. I've added more context to the PR description. PTAL! |
c69a751 to
39cd758
Compare
This comment was marked as resolved.
This comment was marked as resolved.
39cd758 to
3d98df1
Compare
|
@ti-mo @squeed @gentoo-root PTAL Changes pushed in the latest revision:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
|
/test |
dylandreimerink
left a comment
There was a problem hiding this comment.
Changes look good for my codeowners
qmonnet
left a comment
There was a problem hiding this comment.
I don't have the time to look at the changes in detail and only covered doc changes, sorry - Your changes look good, but Casey has an open PR that cleans up the docs and you may want to adjust accordingly, see below.
pchaigno
left a comment
There was a problem hiding this comment.
Thanks for the neat pull request! Very easy to review 🙏
I have a couple questions/concerns below.
Pass traffic with IP protocols like VRRP and IGMP in conntrack datapath. The traffic will be allowed/denied per user-defined policies. It's guarded behind a configuration flag to avoid allowing this traffic by default, especially for users who may have previously applied allow-all policies. Signed-off-by: Aditi Ghag <[email protected]>
This is a preparatory commit to add helper functions for adding L4 ingress policy entries to match on protocol bits. Signed-off-by: Aditi Ghag <[email protected]>
Signed-off-by: Aditi Ghag <[email protected]>
This commit extends the network policy spec to allow/deny protocols such as VRRP and IGMP that don't have transport ports. Note that if protocol is set to ANY and port is omitted, the policy may permit any *L4* protocols (TCP, UDP, and SCTP). Signed-off-by: Aditi Ghag <[email protected]>
Extend policy support to include protocols such as VRRP and IGMP, which do not use transport-layer ports. For these protocols, the port field in toPorts rules will be either omitted or explicitly set to 0. Policy matching logic treats the port field as a wildcard in such cases, and matches on the protocol specified in the policy key. To enable this, relax port validation to allow toPorts rules without a port field when applicable. Signed-off-by: Aditi Ghag <[email protected]>
Signed-off-by: Aditi Ghag <[email protected]>
|
/test |
pchaigno
left a comment
There was a problem hiding this comment.
Thanks for the pointers to previous discussions! LGTM!
I think my latest comments can be addressed as follow ups since they're more about documentation.
|
|
|
@aditighag We already cut the release candidate for v1.18, so typically we would not backport new features into the release at this time, especially something that changes API behavior. This has risk to break behavior and users are not likely to pick up on that at the last minute prior to release. Has there been some discussion about the backport proposal? |
This PR adds support for VRRP and IGMP protocols in host firewall.
Commit highlights
toPortsfield is extended to support these protocols. As the protocols don't have transport ports, the existing validation check around empty ports is relaxed. Users can explicitly whitelist/deny these protocols using thetoPortsrules, and these rules can be combined with the existing L3 rules.DROP_CT_UNKNOWN_PROTOerror. This is gated behind a config flag so that it won't break existing allow-all policies. The decision to allow/deny such traffic is delegated to user-defined policies. Also, the existing behavior of allowing reply traffic based on conntrack state is preserved for VRRP and IGMP protocols as well.Additional notes
IPProtocolsthat would allow users to specify such IP protocols without transport ports using their IANA assigned numbers. This field was restricted to CCNP only using CEL rules. However, based on discussions with @jrajahalme and @squeed, we decided to reuse the existingtoPortsfield instead as the protocols don't need header fields matching similar to ICMP, and so the current use cases can be satisfied without introducing the new field. Note that the policy engine already supports policy matching with wildcard port field.Results from manual testing:
Policy verdict log: flow 0x0 local EP ID 559, remote ID world, proto 112, egress, action deny, auth: disabled, match none, 172.19.0.4 -> 224.0.0.18toPortsrule to explicitly allow VRRP. Traffic is allowed.Policy verdict log: flow 0x0 local EP ID 559, remote ID world, proto 112, egress, action deny, auth: disabled, match Proto-Only, 172.19.0.4 -> 224.0.0.18Policy verdict log: flow 0x0 local EP ID 559, remote ID 16777217, proto 112, egress, action deny, auth: disabled, match L3-Only, 172.19.0.4 -> 224.0.0.18Follow-up: E2E CI test and Hubble proto extensions
Fixes: #31502
Fixes: #18347