Skip to content
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

Address sets for nftables and OVN #1728

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Address sets for nftables and OVN #1728

wants to merge 24 commits into from

Conversation

irhndt
Copy link
Contributor

@irhndt irhndt commented Mar 5, 2025

Closes #1450
This pull request add address sets support for incus, more precisely it allows the use of named sets for nftables and address sets in OVN.
This is my first 'real' contribution and first go project. You may want to check important logic parts in nftables and ovn drivers.

Notes:

  • I wasn't able to get patch test work in github action but worked on my local env
  • I did not add ovn tests in main shell test suite because I did not see any test related to ovn in scripts (perhaps did not look well) but I could add them quickly
  • I made a lot of 'bad' commits because I wasn't able to run test in my local env and did a lot of back and forth with github actions
  • Support for OpenFGA has been added, filtering too
  • Surely missed or forgot something and would be happy to enhance the feature !

@irhndt irhndt requested a review from stgraber as a code owner March 5, 2025 21:17
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Mar 5, 2025
@irhndt irhndt changed the title Address set for nftables and OVN Address sets for nftables and OVN Mar 5, 2025
@irhndt
Copy link
Contributor Author

irhndt commented Mar 11, 2025

Hi, is there anything I can do to help process the PR?

@stgraber
Copy link
Member

Nope, it's next on my list to review, just taking a bit as it's a sizable PR :)

@stgraber
Copy link
Member

I spent a couple of hours on cleaning up the branch, mostly rebasing everything on current main and re-slicing the code in our usual commit chunks. I also did a few minor code style tweaks here and there, but nothing major.

I did notice a few major changes that I'll want to make to this branch, but I'll need to do that later, maybe tomorrow evening, maybe Thursday. It's mostly around the database, changing the name of the tables to line up with our usual pattern, using the DB generator instead of manual SQL functions and a few other similar tweaks.

Anyway, I also need to do an actual review of the logic and manual testing of all of this.
Today was really a superficial scan through things as I put all the changes into commits.

@stgraber
Copy link
Member

Don't worry about the test failures, it's expected at this stage :)

@irhndt
Copy link
Contributor Author

irhndt commented Mar 12, 2025

Good to know, looking forward to learn from your modifications !

@stgraber
Copy link
Member

Haven't forgotten about this but have had to deal with some priority bugs.

@stgraber
Copy link
Member

Been making some progress on this one, but working locally so haven't pushed anything yet.

I've renamed all the ExternalIDs stuff to Config for consistency, renamed the tables to our usual pattern too and put the DB generator in place to generate all the DB access code.

I'm now replacing all the DB function calls with the equivalent for the generated code.

After that's done, I'll want to make sure we can get the tests behaving again before I actually review the content of the commits.

@irhndt
Copy link
Contributor Author

irhndt commented Mar 19, 2025

Yes, at first glance I used ExternalIDs while reading OVN man pages however after I have seen that ACL for example had the same field under config... Forgot to change to comply but I should have done it.

Concerning DB function I want to see how you do that as I don't get exactly what is the 'equivalent'.

When I took a look at failed tests last week I saw that we have failure on the PATCH request test. I was able to get it to work on my computer but once I ran it in github action it wouldn't, so as everything else was ok in github actions I commented it out. So we may want to check other tests are behaving before tackling the patch issue.

Thanks for the heads up!

@stgraber
Copy link
Member

Just pushing an intermediate state. I've moved everything to the DB generator, code builds, static analysis looks clean, so I want to get an idea of what I broke before I clean things up and review the rest.

@irhndt
Copy link
Contributor Author

irhndt commented Mar 21, 2025

HI, I took a look at the error:

Error: Failed to fetch from "network_address_set_config" table: Failed to fetch from "network_address_set_config" table: no such table: networks_addresss_sets_config

I think the issue lies in the GetConfig function in internal/server/db/cluster/config.mapper.go, I will take a look later if you dont do it in the meantime. I am off for most of the week-end.

@stgraber
Copy link
Member

HI, I took a look at the error:

Error: Failed to fetch from "network_address_set_config" table: Failed to fetch from "network_address_set_config" table: no such table: networks_addresss_sets_config

I think the issue lies in the GetConfig function in internal/server/db/cluster/config.mapper.go, I will take a look later if you dont do it in the meantime. I am off for most of the week-end.

Yeah, I know, but this is generated code. You can't change that file as it will get overwritten.

I reached out to @masnax about the database generator and what we need to do to make it behave given the name of the tables involved.

@stgraber stgraber force-pushed the main branch 7 times, most recently from d493e64 to b850a6f Compare March 22, 2025 02:04
irhndt added 4 commits March 22, 2025 00:12
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
irhndt added 6 commits March 22, 2025 00:12
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
irhndt and others added 14 commits March 22, 2025 00:40
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

Implement network address sets
2 participants