Add support for L2VPN - #607
Conversation
|
Hi @flynn-nrg, thanks for opening a PR. If a l2vpnclaim CR is created with the "identifier" field, is it equivalent to creating a l2vpn CR? |
It follows the same pattern by the other claims like IpAddressClaim, so it will compute a restoration hash first and check if the matching object already exists. If not and there is a identifier it will be used directly to create a child L2VPN CR. |
Mirrors the existing IpAddress/IpAddressClaim envtest + gomock pattern: wires L2VPNReconciler/L2VPNClaimReconciler into the test manager with dedicated MockVpnAPI instances per reconciler, and covers reservation, update, restoration-hash mismatch, reserve failure, range allocation, restore-by-hash, explicit identifier, and lease-lock contention.
bruelea
left a comment
bruelea
left a comment
There was a problem hiding this comment.
Currently, different CR's can point to the same l2vpn in NetBox which could be fixed by changing the list option used to get the l2vpn for a CR from NetBox.
| // Field is immutable, required | ||
| //+kubebuilder:validation:Required | ||
| //+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'name' is immutable" | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
could metadata.Name be used instead?
|
|
||
| func (c *NetboxCompositeClient) getL2VPN(ctx context.Context, l2vpn *models.L2VPN) (*v4client.PaginatedL2VPNList, error) { | ||
| req := c.clientV4.VpnAPI.VpnL2vpnsList(ctx). | ||
| Name([]string{l2vpn.Name}) |
There was a problem hiding this comment.
If the l2vpn are filtered by name, different CR's can point to the same l2vpn in NetBox. What are the unique values the l2vpns could be filterd by?
There was a problem hiding this comment.
Have changed that part to have the adapter expose the identifier, which is now the filter criterion and prevents two CRs with a shared or duplicate name from colliding: 13997fb
| // tryLockOnRange serializes concurrent range-based claims against the same | ||
| // identifier range. Returns a nil locker and nil error when the claim uses an | ||
| // explicit identifier instead of a range, since there's no shared pool to lock. | ||
| func (r *L2VPNClaimReconciler) tryLockOnRange(ctx context.Context, o *netboxv1.L2VPNClaim) (ll *leaselocker.LeaseLocker, cleanup context.CancelFunc, res ctrl.Result, err error) { |
There was a problem hiding this comment.
The identifier range can be set be the user, so there could be overlaps. If the identifiers are only unique per l2vpn types the LeaseLockName could be the type. If they need to be unique across type, the LeaseLockName could be "l2vpn" to make sure that different claims cannot get the same identifier assigned. Also for a the claims with explicit identifier there can be a lock, to make sure it does not overlap with a l2vpn which was claimed by a l2vpnclaim with a range.
There was a problem hiding this comment.
Reworked the code so that a single lease lock name and avoid this scenario: e0e8c27
* The adapter exposes the identifier filter. * getL2VPN now filters by identifier so two CRs can no longer collide on a shared or duplicate name. * Update mocks and tests.
* replaced `convertL2VPNRangeToLeaseLockName` (keyed by type+range) with a single fixed constant `l2vpnIdentifierLockName` * `tryLockOnRange` → `tryLockL2VPNIdentifier` * `getLeaseLockerNSNandOwner` updated the same way, so the L2VPN controller's own re-lock (held while actually reserving the VNI in NetBox) stays consistent with the claim controller's lock, for both explicit and range-based owners. * Added `describeL2VPNClaimIdentifier` helper for log/event message text * Test updates
2 participants
Add L2VPN and L2VPNClaim support
Adds L2VPN (Layer 2 VPN) management to the NetBox Operator, following the same claim/resource pattern already used for IP addresses, prefixes, IP ranges and ASNs.
New CRDs
L2VPN— represents a single L2VPN in NetBox (name, type, identifier/VNI, tenant, comments, description, custom fields, preserveInNetbox).L2VPNClaim— claims an L2VPN identifier (VXLAN VNI) either as an exactidentifieror from anidentifierRangeStart/identifierRangeEndrange, and creates an ownedL2VPNCR.identifierand the range fields are mutually exclusive (CEL validation) and immutable once set.Controllers
L2VPNReconciler— reserves/updates the L2VPN in NetBox (ReserveOrUpdateL2VPN), finalizer-based cleanup (l2vpn.netbox.dev/finalizer), lease-locks the parent identifier range while the L2VPN is not yet Ready, reports conditions and events.L2VPNClaimReconciler— restores a previously assigned identifier by hash before falling back to the explicitidentifieror a new range-based allocation, syncs mutable fields down to the ownedL2VPN, lease-locks per range to avoid races between concurrent claims (l2vpnclaim.netbox.dev/finalizer).NetBox client
New methods on
NetboxCompositeClient:ReserveOrUpdateL2VPN,DeleteL2VPN,RestoreExistingL2VPNByHash,GetAvailableL2VPNIdentifierByClaim(scans existing L2VPNs, since NetBox has no dedicated "available identifiers" endpoint for L2VPN). NewL2VPN/L2VPNClaimmodels and regenerated mocks for theVpnL2vpns*API surface. ThenetboxOperatorRestorationHashcustom field is extended to covervpn.l2vpnin the kind data-load job.Tests & docs
pkg/netbox/api/l2vpn_test.go,l2vpn_claim_test.go) and for the controllers (internal/controller/l2vpn_controller_test.go,l2vpnclaim_controller_test.go), covering reservation, update, restoration-hash mismatch, reserve failure, range allocation, restore-by-hash, and lease-lock contention.tests/e2e/l2vpn: explicit/range apply-update, range restore, range-exhausted, invalid tenant/custom-field, and owner-reference cases.config/samples/.L2VPN/L2VPNClaimmodel and usage.Behaviour notes
preserveInNetbox: truekeeps the L2VPN in NetBox after CR deletion, enabling later reclaim.name,type,identifierandtenantimmutability enforced via CEL validation rules on bothL2VPNandL2VPNClaim.This was done with the help of Claude. I haven't seen anything in the docs about it so I assume it's fine.