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.

Sign up for GitHub

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

Jump to bottom

Per-project uplink IP quotas #14631

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Per-project uplink IP quotas #14631

wants to merge 16 commits into from

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Dec 10, 2024 •
edited
Loading

Introduces per-network project uplink IP limits, adding a limits.networks.uplink_ips.NETWORK_NAME configuration key to projects.
This config key defines the maximum value of IPs made available on a network named NETWORK_NAME to be assigned as uplink IPs for entities inside a cetain project.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Dec 10, 2024
@hamistao hamistao force-pushed the project_uplink_ip_quotas branch 5 times, most recently from c83d52d to d9df773 Compare December 10, 2024 12:17
minaelee
minaelee reviewed Dec 10, 2024
View reviewed changes
lxd/api_project.go Outdated Show resolved Hide resolved
lxd/network/driver_ovn.go Outdated Show resolved Hide resolved
lxd/network/driver_ovn.go Outdated Show resolved Hide resolved
lxd/network/driver_ovn.go Outdated Show resolved Hide resolved
lxd/network/driver_ovn.go Outdated Show resolved Hide resolved
lxd/network/driver_ovn.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the project_uplink_ip_quotas branch from d9df773 to ad6e23d Compare December 10, 2024 19:42
@hamistao
Copy link
Contributor Author

@minaelee Thanks for the early review, every comment was addressed as you suggested.

@minaelee
Copy link
Contributor

@minaelee Thanks for the early review, every comment was addressed as you suggested.

Thanks! Sorry, I missed that it was marked as Draft. Looks great!

@hamistao
api: Add projects_limits_uplink_ips extension
04b6646 
Signed-off-by: hamistao <[email protected]>
@hamistao
shared/api: Fix comment typo
b8e7cf9 
Signed-off-by: hamistao <[email protected]>
@hamistao
doc: make update-api
48f9843 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network: Create ProjectUplinkAddressThresholdExceeded
0e01a4e 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/api_project: Add projectName argument to projectValidateConfig
45f0e5b 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/networks: Move allowedUplinkNetworks to network_utils
1dcce1b 
Also changes the parameters in two ways:
 - Taking the project config instead of the entire object so we can use it for project config validation.
 - Takes the state to get networks from the database.

Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/networks: Check for project restrictions first
54d78c1 
Just following the pattern of doing the cheaper checks first.

Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/api_project: Create uplinkIPLimitValidator
3f7feb5 
We check for the current uplink IP usage on the validator function for
two reasons:
 - Show a more informative error message in case the provided value is not appropriate.
 - Avoing doing the expensive computation of uplink IP usage unless a config key was provided
   for a valid uplink network.

Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/api_project: Introduce limits.networks.uplink_ips.<network> con…
7627286 
…fig key to projects

Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network/ovn: Create projectUplinkIPQuotaAvailable
8309353 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network/ovn: Create triageExternalAddressUsage
fa9f85d 
Factors out common steps on forward creation and load balancer creation.

Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network/ovn: Use triageExternalAddressUsage
0f45b02 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network/ovn: Check uplink IP limits on triageExternalAddressUsage
0ec6e71 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network/ovn: Refactor validateUplinkNetwork
66cfb8c 
This way we can easily fit an additional check for project uplink IP quotas on the end

Signed-off-by: hamistao <[email protected]>
@hamistao
lxd/network/ovn: Check uplink IP limits on validateUplinkNetwork
05620bc 
Signed-off-by: hamistao <[email protected]>
@hamistao
make update-metadata
cf466fa 
Signed-off-by: hamistao <[email protected]>
@hamistao hamistao force-pushed the project_uplink_ip_quotas branch from ad6e23d to cf466fa Compare December 11, 2024 04:08
@hamistao hamistao marked this pull request as ready for review December 11, 2024 04:28
@hamistao
Copy link
Contributor Author

@tomponline @markylaing Some observations on this:

  • I did not consider bridge network forwards as something that is taking away IPs from another network, as it would be more tricky to check if they are taking IPs from other managed networks and they are exclusive to the default project. If needed we can add this check in the future.
  • Project uplink IP limits are exclusive to projects with features.networks
  • I am not currently distinguishing between ipv6 and ipv4. Maybe I am not 100% clear on the details of the use case of this feature, as that could help decide how to approach the different protocols.
  • Tests are in the works and should be added to lxd CI shortly.

Manual tests are looking fine so far so I am opening this for review, feel free to look whenever you are able.

@tomponline
Copy link
Member

I am not currently distinguishing between ipv6 and ipv4. Maybe I am not 100% clear on the details of the use case of this feature, as that could help decide how to approach the different protocols.

I think we should have separate ipv4 and ipv6 quotas as the routes on the uplinks are defined per protocol.

@tomponline
Copy link
Member

  • I did not consider bridge network forwards as something that is taking away IPs from another network, as it would be more tricky to check if they are taking IPs from other managed networks and they are exclusive to the default project. If needed we can add this check in the future.

I'm not quite following here. We aren't checking if IPs are being taken away from other networks, but rather whether the quota for the project that the IP usage has exceeded the quota.

For any managed networks in the default project they should still be limited by the quota set on the default project (which is likely to be nothing).

Project uplink IP limits are exclusive to projects with features.networks

I think that probably makes sense. The default project has features.networks so that should be fine.

@hamistao
Copy link
Contributor Author

I'm not quite following here. We aren't checking if IPs are being taken away from other networks, but rather whether the quota for the project that the IP usage has exceeded the quota.

Let's make this a topic for our one to one tomorrow so I can better explain.

@hamistao hamistao marked this pull request as draft December 12, 2024 17:18
@hamistao
Copy link
Contributor Author

Going back to driver to implement suggested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minaelee minaelee minaelee left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hamistao @minaelee @tomponline