Skip to content

Address sets for nftables and OVN #1728

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

Merged
merged 24 commits into from
Apr 5, 2025
Merged

Address sets for nftables and OVN #1728

merged 24 commits into from
Apr 5, 2025

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 11 times, most recently from 3e1ee93 to d91fb72 Compare March 23, 2025 00:08
stgraber and others added 24 commits April 4, 2025 15:15
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Isidore Reinhardt <[email protected]>
Signed-off-by: Stéphane Graber <[email protected]>
@stgraber
Copy link
Member

stgraber commented Apr 5, 2025

The OVN behavior seems fine now. Going to quickly recheck the nftables one.

@stgraber
Copy link
Member

stgraber commented Apr 5, 2025

nftables implementation seems to still work as expected here, so I think we're good to go!

@stgraber stgraber merged commit a13b439 into lxc:main Apr 5, 2025
38 checks passed
@stgraber
Copy link
Member

stgraber commented Apr 5, 2025

Thanks for all the work @irhndt !

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request May 10, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [lxc/incus](https://github.com/lxc/incus) | minor | `v6.11.0` -> `v6.12.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>lxc/incus (lxc/incus)</summary>

### [`v6.12.0`](https://github.com/lxc/incus/releases/tag/v6.12.0): Incus 6.12

[Compare Source](lxc/incus@v6.11.0...v6.12.0)

### Announcement

https://discuss.linuxcontainers.org/t/incus-6-12-has-been-released/23556

#### What's Changed

-   doc: Fix missing OCI section by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1855
-   doc: Fix config option reference on LINSTOR driver by [@&#8203;luissimas](https://github.com/luissimas) in lxc/incus#1857
-   Add support for server-side filtering by instance name by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1856
-   incusd/instance/lxc: Fix max gid when in a privileged container by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1859
-   Fix some static analysis nits by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1860
-   README: Fix typo by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1861
-   Docs: correct restriction on `path` option by [@&#8203;gwenya](https://github.com/gwenya) in lxc/incus#1862
-   lxd-to-incus: Fix typo in trigger by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1865
-   incusd/instance/edk2: Limit test to UEFI architectures by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1866
-   doc: Tweak ACME documentation by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1868
-   instances/drivers/qemu: update user parameter for QEMU v9.1+ by [@&#8203;dnegreira](https://github.com/dnegreira) in lxc/incus#1871
-   OCI improvements by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1873
-   Support server-side filtering by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1872
-   Enable filtering with the `all-projects` flag when listing images by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1874
-   Improve migration by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1878
-   incusd/storage: Add missing forwarding on snapshot list by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1882
-   incusd/instance/common: Fix concurrent restarts by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1884
-   Fix all static analysis in client/, shared/ and cmd/incus/ by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1883
-   generate-database: Fix documentation for `ignore` by [@&#8203;breml](https://github.com/breml) in lxc/incus#1885
-   incusd/response: Remove redundant line break in error by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1886
-   RFC 3442 compliance in forknet dhcp client by [@&#8203;gwenya](https://github.com/gwenya) in lxc/incus#1887
-   incus-agent: Retry mounts to avoid kernel races by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1888
-   Address sets for nftables and OVN by [@&#8203;irhndt](https://github.com/irhndt) in lxc/incus#1728
-   incusd/operations: Fix WaitGet on op failure by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1894
-   Update list of compresors by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1892
-   Add snapshot pre-fetching support by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1891
-   incusd/instance/lxc: Use pre-existing PATH when not overridden by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1895
-   incusd/acme: Include CA in generate certificate by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1897
-   Usability improvements to incus-migrate by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1898
-   client/incus: Fix non-constant format strings by [@&#8203;c4t3l](https://github.com/c4t3l) in lxc/incus#1899
-   docs: mDNS setup for cluster HA by [@&#8203;MOZGIII](https://github.com/MOZGIII) in lxc/incus#1896
-   Support filtering storage volumes by a single keyword by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1915
-   incusd/instance/qemu: Clean leftover sockets on startup by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1916
-   incusd: Implement Incus OS API forwarding by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1918
-   Add generated documentation for network bridge by [@&#8203;NathanChase22](https://github.com/NathanChase22) in lxc/incus#1920
-   doc: Use `$USER` instead of YOUR-USERNAME by [@&#8203;bjackman](https://github.com/bjackman) in lxc/incus#1922
-   doc: Ignore link that's blocking Azure by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1924
-   Storage bugfixes by [@&#8203;bensmrs](https://github.com/bensmrs) in lxc/incus#1923
-   incusd/patches: Refresh OpenFGA model for address sets by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1925
-   Add generated documentation for network forwards by [@&#8203;tonyn10](https://github.com/tonyn10) in lxc/incus#1926
-   Add support for configurable logging targets  by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1903
-   Port tpm device documentation to gendoc by [@&#8203;saahirN](https://github.com/saahirN) in lxc/incus#1929
-   Allow basic connectivity under nftables by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1930
-   incusd/storage/zfs: Make CacheVolumeSnapshots failures non-fatal by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1931
-   incusd/instance/lxc: Restrict unprivileged ping to recent kernels by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1934
-   Implement SNAT as part of network forwards by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1935
-   incusd/apparmor/lxc: Allow write access to /proc/sys/user by [@&#8203;zgttotev](https://github.com/zgttotev) in lxc/incus#1937
-   incusd/instance/lxc: Defer calls to the scheduler by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1938
-   shared/archive: Prevent xattr errors from crashing unsquashfs by [@&#8203;zgttotev](https://github.com/zgttotev) in lxc/incus#1939
-   Extend use of ZFS pre-caching by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1941
-   Add common aliases for add/create remove/delete/rm in the CLI by [@&#8203;joecwilson](https://github.com/joecwilson) in lxc/incus#1943
-   feat: support access_token query parameter as JWT fallback by [@&#8203;irtaza9](https://github.com/irtaza9) in lxc/incus#1940
-   Memory hotplug support for VMs by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1945
-   incusd: Remove old routing logic by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1947
-   Fix refresh migrations in cluster and speed up ZFS startup by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1946
-   incusd/devices: Don't require a serial number for USB hotplug by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1949
-   Move tls testing functions to tlstest by [@&#8203;nanjj](https://github.com/nanjj) in lxc/incus#1948
-   Remove Rican7/retry dependency by [@&#8203;nanjj](https://github.com/nanjj) in lxc/incus#1952
-   Port `proxy` device documentation to `gendoc` by [@&#8203;Abdomash](https://github.com/Abdomash) in lxc/incus#1953
-   Port gpu device documentation to gendoc by [@&#8203;kmxtn](https://github.com/kmxtn) in lxc/incus#1954
-   Port nic device documentation to gendoc by [@&#8203;rahafjrw](https://github.com/rahafjrw) in lxc/incus#1956
-   Remove arping dependency by [@&#8203;ahmetfturhan](https://github.com/ahmetfturhan) in lxc/incus#1958
-   Remove gocapability dependency by [@&#8203;nanjj](https://github.com/nanjj) in lxc/incus#1957
-   Infiniband Device Documentation Ported to GenDoc by [@&#8203;AbhinavTiruvee](https://github.com/AbhinavTiruvee) in lxc/incus#1962
-   Replace rebfig/cron/v3 with adhocore/gronx by [@&#8203;nanjj](https://github.com/nanjj) in lxc/incus#1959
-   Update help of `incus storage list` by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1968
-   shared/api/scriptlet: Add yaml struct tags by [@&#8203;breml](https://github.com/breml) in lxc/incus#1973
-   incusd/storage/migration: Check instance size during migration by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1971
-   Logfile for forknet dhcp by [@&#8203;gwenya](https://github.com/gwenya) in lxc/incus#1976
-   Add dhcp static routes via 0.0.0.0 with link scope in forknet by [@&#8203;gwenya](https://github.com/gwenya) in lxc/incus#1977
-   incusd/device/disk: Fix registration of custom volumes by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1980
-   Add server side filtering for `incus profile list` by [@&#8203;Abdomash](https://github.com/Abdomash) in lxc/incus#1982
-   Fix reference passing when yaml unmarshal by [@&#8203;nanjj](https://github.com/nanjj) in lxc/incus#1984
-   Various fixes by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1983
-   scriptlet: Return proper error by [@&#8203;breml](https://github.com/breml) in lxc/incus#1986
-   incusd/instance: Also consider local CPU flags by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1987
-   Cap maximum VM memory to match host memory total by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1988

#### New Contributors

-   [@&#8203;dnegreira](https://github.com/dnegreira) made their first contribution in lxc/incus#1871
-   [@&#8203;c4t3l](https://github.com/c4t3l) made their first contribution in lxc/incus#1899
-   [@&#8203;MOZGIII](https://github.com/MOZGIII) made their first contribution in lxc/incus#1896
-   [@&#8203;NathanChase22](https://github.com/NathanChase22) made their first contribution in lxc/incus#1920
-   [@&#8203;bjackman](https://github.com/bjackman) made their first contribution in lxc/incus#1922
-   [@&#8203;tonyn10](https://github.com/tonyn10) made their first contribution in lxc/incus#1926
-   [@&#8203;saahirN](https://github.com/saahirN) made their first contribution in lxc/incus#1929
-   [@&#8203;zgttotev](https://github.com/zgttotev) made their first contribution in lxc/incus#1937
-   [@&#8203;joecwilson](https://github.com/joecwilson) made their first contribution in lxc/incus#1943
-   [@&#8203;irtaza9](https://github.com/irtaza9) made their first contribution in lxc/incus#1940
-   [@&#8203;Abdomash](https://github.com/Abdomash) made their first contribution in lxc/incus#1953
-   [@&#8203;kmxtn](https://github.com/kmxtn) made their first contribution in lxc/incus#1954
-   [@&#8203;rahafjrw](https://github.com/rahafjrw) made their first contribution in lxc/incus#1956
-   [@&#8203;ahmetfturhan](https://github.com/ahmetfturhan) made their first contribution in lxc/incus#1958
-   [@&#8203;AbhinavTiruvee](https://github.com/AbhinavTiruvee) made their first contribution in lxc/incus#1962

**Full Changelog**: lxc/incus@v6.11.0...v6.12.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC4xMS4yIiwidXBkYXRlZEluVmVyIjoiNDAuMTEuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
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