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

Implement network address sets #1450

Open
stgraber opened this issue Dec 2, 2024 · 5 comments
Open

Implement network address sets #1450

stgraber opened this issue Dec 2, 2024 · 5 comments
Assignees
Labels
API Changes to the REST API Documentation Documentation needs updating Feature New feature, not a bug
Milestone

Comments

@stgraber
Copy link
Member

stgraber commented Dec 2, 2024

One current missing piece of our networking puzzle is support for address sets.
That is, a named set of address and/or subnets which can be used as source/destination selector within ACLs.

There is a native OVN object for this feature, so it would make sense to initially implement this for OVN networking.

Address sets should be scoped the same way as ACLs as they'll be very closely linked. This means address sets should be part of a project when features.networks=true

The expected changes should roughly be:

  • Allocate API extension
  • New database table (schema change)
  • New database functions (using the generator)
  • Add REST API structs for network address sets
  • Add OVN NB bindings for managing address sets
  • Add REST API to manipulate address sets
  • Add integration with OVN ACLs
  • Implement CLI command
  • Refresh translations
  • Add tests
@stgraber stgraber added Documentation Documentation needs updating Feature New feature, not a bug API Changes to the REST API labels Dec 2, 2024
@stgraber stgraber added this to the later milestone Dec 2, 2024
@irhndt
Copy link
Contributor

irhndt commented Dec 9, 2024

Hello, I’m giving this a try, but I’m new to both Go and Incus development. I’m approaching this as an enthusiast and treating it as a learning exercise so you may want someone that is already proficient. I already have a few questions.

In internal/server/network/ovn/ovn_nb_actions.go, I noticed some bindings for address sets. Am I correct in assuming that the ones we need to implement are in driver_ovn.go?

Additionally, I’m currently exploring where to implement the REST API and believe I’ve identified the relevant structures for API definitions. To summarize, I’m focusing on implementing:

OVN NB bindings for managing address sets
A REST API to manipulate address sets

Any guidance would be appreciated.

@stgraber
Copy link
Member Author

Oh, I completely forgot that we were already making use of address sets internally for the @internal and @external selectors in our ACLs, so that indeed means that the work in ovn_nb_actions.go shouldn't be necessary as those functions already exist.

@stgraber
Copy link
Member Author

@irhndt I assigned this issue to you so we don't end up with someone else working on this in parallel. Let me know if you have any questions as you start digging into this one!

@irhndt
Copy link
Contributor

irhndt commented Dec 11, 2024

@stgraber Thank you for assigning me this, I'll do my best.

I understand a bit better the code structure but still it's a huge and complex project <3.
I came with the following roadmap to help me focus and organize. However I still have some doubts on the right way to implement things and mostly where. I already worked on a few other files not mentioned in the roadmap such as internal/version/api.go or internal/lifecycle/network_address_set.go, mainly by grepping acl to find related files and assessing if I need a similar pattern for address sets.

I hope to begin tackle the main logic in the next few days, so before I take the big dive I would like to know if there is some precision for the phase 3 of my roadmap I should know before starting. Also if you spot some obvious mistakes or missing parts from previous phases let me know.

  1. Database Schema:

    • Defined address_sets table in the fresh schema.
    • Added updateFromV75 migration function.
    • Updated the updates map with version 76.
    • Incremented SchemaVersion to 76.
    • Ran make update-schema to regenerate schema.go.
  2. Database Layer:

    • Created the AddressSet struct.
    • Implemented CRUD operations in address_sets.go.
  3. API and Logic Layer:

    • Defined API structures in shared/api/network_address_set.go.
    • Implemented handlers in internal/server/network/address_sets/.
    • Implement driver integration in internal/server/network/driver_*
    • Implement firewall integration either by a smart acl integration or direclty in internal/server/firewall looking forward to understand how ACL are used outside of OVN, also looking to understand how firewall backend integrates with OVN own filtering.
  4. OVN Integration:

    • Ensured OVN NBCTL supports required operations.
  5. CLI Integration:

    • Implemented CLI commands in cmd/incus/address-set.go.
    • Provided help texts and usage examples.
  6. Translations:

    • Updated translation files with address set-related messages.
    • Ensured translations are available in all supported languages.
  7. Testing:

    • Wrote unit tests for database operations.
    • Wrote tests for API interactions.
    • Wrote tests for CLI commands.
    • Verified OVN integration through testing.
  8. Documentation:
    Think about doc/network-addr-sets.md

    • Updated API documentation in doc/reference and doc/manpages.
    • Created user guides in doc/howto.
  9. Review and Validation:

    • Conducted code reviews for all new additions.
    • Performed security audits on the new feature.

@stgraber
Copy link
Member Author

In 2), note that we also generate as much of the database access functions as possible.
This isn't implemented for every object so depending on what you used as an example you may have missed it.

network_integrations.go would be a good place to look for another network related feature which does have code generated database functions.

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 Feature New feature, not a bug
Development

No branches or pull requests

2 participants