EFW: Add support for secondary nodeIPs #3960
Conversation
tssurya
requested review from
trozet,
dcbw,
girishmg and
jcaamano
as code owners
| libovsdbutil.GetACLName(dbIDs), | ||
| nbdb.ACLDirectionToLport, | ||
| t.EgressFirewallStartPriority, | ||
| fmt.Sprintf("(ip4.dst == %s) && ip4.src == $%s", nodeIP, asHash), | ||
| fmt.Sprintf("(ip4.dst == %s || ip4.dst == %s || ip6.dst == %s) && (ip4.src == $%s || ip6.src == $%s)", |
There was a problem hiding this comment.
lol feels weird having v4 and v6 in the same match, the || is fine, but its combined by an && ?
tssurya
force-pushed
the
efw-add-2ndary-nodeIPs
branch
3 times, most recently
from
181de90
to
0570a1e
Compare
|
Changes unknown |
tssurya
force-pushed
the
efw-add-2ndary-nodeIPs
branch
2 times, most recently
from
42d482d
to
eac4170
Compare
test/e2e/egress_firewall.go
Outdated
| @@ -290,6 +305,37 @@ spec: | |||
|
|
|||
| } | |||
|
|
|||
| ginkgo.By("Selecting additional IP addresses for serverNode (networking routing to secondaryIP address on other nodes is harder to achieve)") | |||
There was a problem hiding this comment.
I dont understand what "networking routing to secondaryIP address on other nodes is harder to achieve" is in relation to. Can you elaborate on what you mean by this?
There was a problem hiding this comment.
good point, :) so I add 172.18.1.13 to ovn-control plane, I add 172.18.1.14 to ovn-worker and 172.18.1.15 to ovn-worker2, these are just IPs and we don't really add routes to the host for routing this. So if src pod.Node == dst Node, same node case it works in other cases we need to add routes to tell the node how to reach the 172.18 subnet which is a pain.
There was a problem hiding this comment.
so i basically only did pod on nodeA => secondaryIPs on nodeA test
I didn't do pod on nodeA => secondaryIPs on nodeB and nodeC test
If you insist I can change this to try to include that
There was a problem hiding this comment.
no its fine, just didnt realize until later in the test case that the entire e2e suit seems setup to sending to something on the same node.
Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
This commit changes efw.to.nodeSelector to apply on all the nodeIPs present in the host-cidrs annotation and not just primary nodeIPs. Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com> Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
tssurya
force-pushed
the
efw-add-2ndary-nodeIPs
branch
from
eac4170
to
64f3bcd
Compare
This commit edits existing e2e's to accommodate testing secondaryIPs on nodes Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
tssurya
force-pushed
the
efw-add-2ndary-nodeIPs
branch
from
64f3bcd
to
249e662
Compare
|
Fix included in accepted release 4.16.0-0.nightly-2024-02-17-013806 |
- What this PR does and why is it needed
This commit changes efw.to.nodeSelector
to apply on all the nodeIPs present in the
host-cidrs annotation and not just primary
nodeIPs.
- Special notes for reviewers
filed during openshift/enhancements#1388 (comment)
- How to verify it
e2e's and ut's added
- Description for the changelog
ensure all nodeIPs are considered for nodeselector when using egress firewall