-
-
Notifications
You must be signed in to change notification settings - Fork 201
feat(sdn): add support for zones, vnets, subnets, validation, and tests #1995
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
base: main
Are you sure you want to change the base?
Conversation
96553b0
to
37885ab
Compare
That's awesome, thanks so much @MacherelR for your contribution! ❤️ It will take time to go over all the code and tests, but I'll try my best to get it reviewed over the next few days. |
27ff85c
to
51815df
Compare
… and tests BREAKING CHANGE: introduces sdn support. Signed-off-by: MacherelR <[email protected]>
Signed-off-by: MacherelR <[email protected]>
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.
just ran make example
, and noticed a few issues.
will try acc tests and manual tests next.
Also worth to mention, the make example
kinda broken in a few other unrelated places as well, I'll fix them in a separate PR
} | ||
|
||
data "proxmox_virtual_environment_sdn_subnet" "subnet_ex" { | ||
subnet = "ZoneEx-100.100.0.0-24" |
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.
this doesn't seem to be working
I've tried also zoneS-10.10.0.0-24
name, but no luck:
╷
│ Error: Subnet not found
│
│ with data.proxmox_virtual_environment_sdn_subnet.subnet_ex,
│ on resource_virtual_environment_sdn.tf line 95, in data "proxmox_virtual_environment_sdn_subnet" "subnet_ex":
│ 95: data "proxmox_virtual_environment_sdn_subnet" "subnet_ex" {
│
│ error reading SDN subnet zoneS-10.10.0.0-24 for Vnet vnetM: the requested resource does not exist
│ received an HTTP 500 response - Reason: sdn subnet 'zoneS-10.10.0.0-24' does not exist
╵
the corresponding resource in the state:
{
"mode": "managed",
"type": "proxmox_virtual_environment_sdn_subnet",
"name": "subnet_simple",
"provider": "provider[\"registry.opentofu.org/bpg/proxmox\"]",
"instances": [
{
"schema_version": 0,
"attributes": {
"canonical_name": "zoneS-10.10.0.0-24",
"dhcp_dns_server": "10.10.0.53",
"dhcp_range": [
{
"end_address": "10.10.0.100",
"start_address": "10.10.0.10"
}
],
"dnszoneprefix": null,
"gateway": "10.10.0.1",
"id": "zoneS-10.10.0.0-24",
"snat": true,
"subnet": "10.10.0.0/24",
"type": "subnet",
"vnet": "vnetM"
},
"sensitive_attributes": [],
"dependencies": [
"proxmox_virtual_environment_sdn_vnet.vnet_simple",
"proxmox_virtual_environment_sdn_zone.zone_simple"
]
}
]
}
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.
@bpg
used to work locally for me but I guess I had some manual steps remains...
Investigating it !
Thing sis, my guess is that it might actually not be poossible to test datasource, as (as mentionned in #817 ) the provider does not apply
configurations and therefore I'm not sure the datasource will be able to extract the data... I'll investigate and let you know but what'd be your opinion on this ?
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.
Also, I have some problems sometimes when running the make example
. I started fresh following your doc (proxmox on a local vm using virt-manager), and then simply defined an api-token and ran make example
and got the following error:
Error: error reading rule 0 : error retrieving firewall rule 0: received an HTTP 500 response - Reason: no such security group 'example-sg'
Not exactly sure where that comes from but that could point something on what you said about some other tests being with errors.
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.
There seems to be some residual resources in your env that conflict with TF state, or vice versa, the sate references some resources that were manually removed.
I tried the current branch in my env, and make examples
worked fine, so I guess there are no permanent issues anymore.
# Data Source: proxmox_virtual_environment_sdn_subnet | ||
|
||
Retrieve details about a specific SDN Subnet in Proxmox VE. | ||
|
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 should probably indicate here if the target resource does not exist in PVE, the datasource will return an error. It's pretty common (albeit a bad practice) to return a datasource with null attributes in this case, which some users may expect.
It may also be worthwhile to mark the attributes as required as well, as I assume they all (or at least most of them) will be present on the ds.
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.
same for the other datasources
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.
See what you mean.
For me it seems clearer to return an error when the target resource doesn't exist as it can indicate the user something is wrong... On the other hand, returning resource will null attribute could allow the rest of the deployment to work correctly, but may lead to errors later on.. What do you think is best ?
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.
For me it seems clearer to return an error when the target resource doesn't exist as it can indicate the user something is wrong...
Yeah, I totally agree with that, and I think we should maintain this approach for all new data sources moving forward.
Signed-off-by: Pavel Boldyrev <[email protected]>
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.
I'm trying to implement vnet_firewall configuration into this PR. It can go into a separate PR, but it would be nice if ID association was cleaned up in this PR to make those implementations easier.
proxmox/cluster/sdn/vnets/client.go
Outdated
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.
It would be nice if the vnet id could be defined within the client similar to vms. I'm trying to implement firewall configuration for this PR, but since ExpandPath
doesnt actually include the id, adding firewall to vnets is made harder.
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.
Since subnets is a child of vnets, wouldnt it make more sense to put subnets inside of vnets and provide a Subnets
interface within vnets?
Hey @rad10 👋🏼 Thanks for your efforts, much appreciated! |
…bpg#1983) Signed-off-by: Marco Attia <[email protected]> Signed-off-by: Pavel Boldyrev <[email protected]> Co-authored-by: Pavel Boldyrev <[email protected]>
* docs: update CONTRIBUTORS.md * docs: update .all-contributorsrc --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
…v7.3.0) (bpg#2010) | datasource | package | from | to | | ---------- | ------------------------------- | ------ | ------ | | go | github.com/brianvoe/gofakeit/v7 | v7.2.1 | v7.3.0 | Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Pavel Boldyrev <[email protected]>
… and tests BREAKING CHANGE: introduces sdn support. Signed-off-by: MacherelR <[email protected]>
Signed-off-by: MacherelR <[email protected]>
Signed-off-by: MacherelR <[email protected]>
Description: "Manages SDN Zones in Proxmox VE.", | ||
Attributes: map[string]schema.Attribute{ | ||
"id": attribute.ResourceID(), | ||
"name": schema.StringAttribute{ |
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.
I started testing SDN pieces in my environment and noticed that we don't do much of validation on the schema level.
@MacherelR I think it will be more efficient if I just contribute improvements into this PR, if you don't mind?
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.
Well, not sure we need a separate attribute name
, as id
is essentially the same.
Unlike other resources where we need to have a compound id of + or smth similar, SDN resources are cluster-wide, so just id
would be enough, i think.
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.
@bpg Sure suit yourself I'd gladly collaborate with you on that PR :)
I'm still going through the code, testing and verifying the format of each resource. There are a few changes I'd like to make, mainly in the resource schemas. But it's hard to manage due to the sheer size of the PR. I already have lots of suggestions, and I'm only halfway through the zone 😅 I think it would be more practical to split this PR into multiple, starting with the zone resource/datasource implementation. @MacherelR what do you think? |
@MacherelR Have you considered creating different resources per zone type? I remember seeing discussion somewhere, but not sure if there was any conclusion on that. It seems like having separate resource with their own validation rules would have a better UX than a giant "generic" zone. WDYT? |
@bpg I think you're right this could help sanitize everything as validation gets pretty heavy when trying everything into one... This would need some good reflexions on how to correctly and "nicely" structure the code with generic parts and type-specific parts but I think that's a good idea :) |
@bpg As a quick followup on this, how would you like to team up ? |
Hey @MacherelR, yes that's a plan. I have a draft PR #2046 for zone resources. In general it is almost done, but I still need to go over attributes validation one more time, adjust optionals vs required, etc. Perhaps add few more tests. |
Contributor's Note
/docs
for any user-facing features or additions./fwprovider/tests
for any new or updated resources / data sources.make example
to verify that the change works as expected.<---
⚠ BREAKING CHANGES
As part of extending the Proxmox Terraform provider, I implemented support for managing Software Defined Networking (SDN) resources. The work included:
✅ Resource Implementation
SDN Zones (proxmox_virtual_environment_sdn_zone)
Created support for multiple zone types (simple, vlan, vxlan, evpn, etc.) with field mapping to the Proxmox SDN API.
SDN Vnets (proxmox_virtual_environment_sdn_vnet)
Enabled VNet creation with parameters tailored to zone types, including VLAN tagging and isolation settings.
SDN Subnets (proxmox_virtual_environment_sdn_subnet)
Supported subnet allocation, DHCP ranges, and SNAT configuration.
🔐 Data Validation
Enforced field requirements based on zone type (e.g., bridge required for VLAN, controller for EVPN).
Implemented schema-level validation and model-based logic to ensure consistency and API compatibility.
🧪 Acceptance Testing
Wrote comprehensive acceptance tests for all three SDN resources.
Verified create/read/update/delete operations and validated correct attribute resolution through the API.
Added corresponding data source support and tests to allow importing and referencing existing SDN resources.
This functionality enables Terraform users to declaratively manage their Proxmox SDN infrastructure, enhancing automation and reproducibility for advanced networking scenarios.
I know sdn is much more and has much more possibilities but it's a pretty big work for which I don't have the time and resources, so here's my contribution, hoping others will follow :)
Open to any request/suggestions on this subject folks :)
--->
Proof of Work
Find below an example terraform script used to provide zones, vnets and subnets, and also potentially retrieve an existing one with datasources:
Community Note
Closes #0000 | Relates #817