-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ANP: Add Support for Networks as Egress Peers #4235
Conversation
✅ Deploy Preview for subtle-torrone-bb0c84 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c514c0c
to
bc17ec0
Compare
bc17ec0
to
e1fcf87
Compare
e1fcf87
to
1ae725f
Compare
31fad6c
to
6d1bf84
Compare
6d1bf84
to
d0d486b
Compare
there is one flake in one lane: https://github.com/ovn-org/ovn-kubernetes/actions/runs/8521709373/job/23341371557?pr=4235 saving link for investigation.
|
go-controller/pkg/ovn/controller/egressservice/egressservice_zone_service.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/controller/admin_network_policy/admin_network_policy.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/controller/admin_network_policy/admin_network_policy.go
Outdated
Show resolved
Hide resolved
thanks Jaime for the first pass! I am working through the comments, will push tomorrow; I want to finish up nadia's comments #4245 first and get that in before so that I can come back to this. |
@jcaamano : PTAL |
go-controller/pkg/ovn/controller/admin_network_policy/admin_network_policy.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/controller/egressservice/egressservice_zone_service.go
Outdated
Show resolved
Hide resolved
a5d2beb
to
556e9bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm another consideration, do we expect any performance impact in the OVN side of things due to having CIDRs in address sets? I know optimizations happened in relation to CIDRs, but not sure if address sets are covered there.
go-controller/pkg/ovn/controller/admin_network_policy/admin_network_policy.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/controller/admin_network_policy/admin_network_policy.go
Outdated
Show resolved
Hide resolved
This is a great question and it is something I also want to know (already in my upcoming scale test plans). I have been waiting for some scale run results to see how we perform for this cc @rpattath and @lenahorsley (plan is to modify things post that; I need a rough idea on the following items from scale perspective: 1. central controller level driven locking system perf 2. current ACL, AS perf those are the two big things on my mind to try out) meanwhile @dceara / @igsilya / @almusil are any of you aware of this ^ or has performed ovn heater tests for address-sets with CIDR blocks in it? Does it have any perf impact to put a CIDR range into an addressset? so something like:
are the two options we have in ovnk today for doing ACL matches, is there a one is better than the other approach ^ we need to consider? |
556e9bb
to
0f7e7ac
Compare
jeez the re-tagging is not working and its not picking up my upstream fixes (sigh!) which is why the e2es are failing.. I am investigating this. |
We don't have relevant ovn-heater tests yet, unfortunately.
I think option 1, one large address set, will work better. We have this bug: https://issues.redhat.com/browse/FDP-509 and that's partially addressed by @almusil in https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ with the limitation that:
|
the tests are not getting updated and I don't really understand why...
it is still using the old 5353 port and my PR here: kubernetes-sigs/network-policy-api#217 already changed all this. https://github.com/kubernetes-sigs/network-policy-api/releases/tag/v0.1.3 we even re-tagged this. I don't know why our github runners are not picking this up correctly..note that its a connection refused so clearly the install part of daemon sets look good from the manifest; its the test changes that are not getting picked up |
0f7e7ac
to
6409ee9
Compare
maybe its golang/go#42312 not sure.. but clearly the test change from last 2 commits are not getting picked up cc @astoycos maybe you have seen this before? |
go-controller/pkg/ovn/controller/egressservice/egressservice_zone_service.go
Outdated
Show resolved
Hide resolved
e1b717c
to
fce732c
Compare
All right, we had to bump the version to get the test changes since re-tagging doesn't help with go modules. So that's the last new commit coming in. |
Signed-off-by: Surya Seetharaman <[email protected]>
OVN Address-Sets can be a combination of IPs, CIDRs, Ethernet addresses etc as specified in their documentation: " An address set may contain Ethernet, IPv4, or IPv6 addresses with optional bitwise or CIDR masks." _uuid : 642413a7-21c6-42a5-a817-db9d31455716 addresses : ["10.244.0.3", "10.244.0.4", "10.244.0.5", "10.244.1.3", "10.244.2.3", "10.244.3.0/24", "172.19.0.3", "172.19.0.4", "172.19.0.5"] external_ids : {direction=Egress, gress-index="1", ip-family=v4, "k8s.ovn.org/id"="default-network-controller:AdminNetworkPolicy:node-as-egress-peers:Egress:1:v4", "k 8s.ovn.org/name"=node-as-egress-peers, "k8s.ovn.org/owner-controller"=default-network-controller, "k8s.ovn.org/owner-type"=AdminNetworkPolicy} name : a2074424610294966262 However our ovn pkg address-sets were all IP centric using net.IP everywhere. This refactor will help in future commits where we plan to add CIDR peers as well. NOTE: Reviweres please take care to ensure I haven't removed some ParseCIDR/ParseIP validations which could cause some regressions or issues. Updated mockery to 2.40 version and ran mockery --all in the address-set package. Signed-off-by: Surya Seetharaman <[email protected]>
Signed-off-by: Surya Seetharaman <[email protected]>
Signed-off-by: Surya Seetharaman <[email protected]>
Signed-off-by: Surya Seetharaman <[email protected]>
Signed-off-by: Surya Seetharaman <[email protected]>
fce732c
to
e43ae38
Compare
@jcaamano : PTAL I have rebased on master and fixed e2es and I think I addressed your latest comments |
As a general rule, if the address set has to be modified during the expression processing, that will make incremental processing impossible on the ovn-controller side. In simple rules that only contain an address set match on a single field the address set is pretty much taken verbatim, unless it contains exact duplicates. For example, if we have an address set
In this case there is no difference for ovn-controller if the address set contains simple IPs or CIDRs.
As a general rule of thumb for ACLs - don't use complex expressions. More complex the expression, more likely it for ovn-controller to generate unoptimal OpenFlow rule set. See https://bugzilla.redhat.com/show_bug.cgi?id=2180052 for example. And another warning here is that expressions that contain overlapping IPs/CIDRs should be handled very carefully. As shown above, ovn-controller will not try to optimize an address set if it is the only expression for this particular field in the match. However, once you add at least one more condition for the same field, the address set may be optimized and ovn-controller will loose ability to process changes incrementally for this ACL. For example, taking the
After this operation incremental processing of the address set changes is no longer possible. If the set didn't have the CIDR, but we add As I guess that there will be overlap between NP and ANP CIDRs, I'd say that option 2 will likely just break incremental processing. Option 1 is definitely much better in that regard. General advise from conclusions of https://www.openvswitch.org/support/ovscon2023/slides/OVN_expression_parsing_Fighting_inequality.pdf to use priorities and try to split large ACLs into smaller ones applies here. I'd say that safer option may be to even split the |
unrelated failures:
which is a known flake: #4144 https://github.com/ovn-org/ovn-kubernetes/actions/runs/8781279809/job/24094055891?pr=4235 is a known flake I have double checked that its not a bug in code per say, I might need to add retries OR similar to all our other e2e's run these tests twice by default. |
thanks ilya for the insights here, overlapping cidrs/ips in the same address set logically speaking something not possible with this feature as in I can't think of a reason as to why peers will overlap from user stand point of view in the same address-set so we should be good there.
yeah going with option1 for today and we have plans to try out some scale runs with potentially ways users might use this feature, will share some database excerpts from that with ovn team in the future to seek continuous improvements if any. |
func (as *ovnAddressSet) deleteIPs(ips []net.IP) ([]ovsdb.Operation, error) { | ||
if len(ips) == 0 { | ||
// deleteAddresses removes selected addresses from the existing address_set | ||
func (as *ovnAddressSet) deleteAddresses(addresses []string) ([]ovsdb.Operation, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick question: how does this work if we call addAddresses with the same IP multiple times and then deleteAddresses once for the same IP? My understanding is that the address will be removed from the database in the end. How does that work if the same address comes from multiple sources? Or is it not possible to get the same IP from multiple places (e.g. once from ANP and once from a CIDR block) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I ask is primarily that the code is written as if it expects potentially non-unique addresses. That might be a bit misleading for a reader. And might be dangerous if they are in fact non-unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we filter them out - for a given address-set we do not pass in duplicate ips, we ensure they are unique before we send the transact (both during add and delete)
for most of the features address-sets are tied to namespaces and podIPs are unique we are not going to be adding multiple duplicated ips into the address-set where they hold different meanings. So if you delete them from one spot its meant to go away from the database which is the expected behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. I just had an impression from discussions on this PR that you're going to mix IPs from different sources in the same address set and that made me think about possible duplicates.
But if that is not the case and user always sets unique values for the list of CIDRs, that should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, what??? did I edit your comment? I thought I quote replied.. dangit => wait how can I edit your comments... that seems wrong..
sorry @igsilya ! I am unsure how to fix that..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. I just had an impression from discussions on this PR that you're going to mix IPs from different sources in the same address set and that made me think about possible duplicates.
the PR changes an addressset PR from being "ONLY contains IPs" to "contains both IPs and CIDRs" rest of the logic is not changing
But if that is not the case and user always sets unique values for the list of CIDRs, that should be fine.
there is API validation in place to ensure CIDRs for same peer contain unique elements that is a set of cidrs; in addition to that we do the unique filtering on ovnk side before we add transact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. :) The repo owners / org members are able to edit comments from others on github, IIRC. The history is seen in the 'edited by ...' dropdown.
Anyways, thanks for explanation!
depends on #4164 (so ignore the 1st 5 commits here, they are part of 4164, start from commit 6 till 10)- What this PR does and why is it needed
This PR adds support for ANP
network
peers (A networks peer is an array of network cidrs, it is supported only as an egress peer (northbound NOT southbound/ingress;)). The main PR is the last three commits. The first three commits are actually doing all the necessary plumbing and refactoring changes to accommodate that.- Special notes for reviewers
OVN supports including CIDR blocks in address-sets but OVNKubernetes always treated address-sets as only a set of IPs. Currently since ANP has pods, namespaces, nodes, and now networks peers and each address set can have combo of IPs and CIDRs as peers, its time to change the plumbing around address-set machinery to ensure we are able to pass CIDRs as well. However during reviews be careful to ensure I at least do the parseIP and parseCIDR validations everywhere before I pass them as strings into the AddressSet machinery.
Pay attention to commit 3 where I change net.IP to string everywhere to ensure I have not thrown some required validation out the window.
- How to verify it
unit tests and e2e tests have been added
- Description for the changelog
Add support for networks peer