diff --git a/Tiltfile b/Tiltfile index f3662dbc2..7ded83347 100644 --- a/Tiltfile +++ b/Tiltfile @@ -136,6 +136,7 @@ k8s_yaml('./config/samples/v1alpha1_bgppeer.yaml') k8s_resource(new_name='peer-spine1', objects=['leaf1-spine1:bgppeer'], resource_deps=['bgp', 'lo0'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) k8s_resource(new_name='peer-spine2', objects=['leaf1-spine2:bgppeer'], resource_deps=['bgp', 'lo0'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) k8s_resource(new_name='peer-spine1-filtered', objects=['leaf1-spine1-filtered:bgppeer'], resource_deps=['bgp', 'lo0', 'bgp-import-policy'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) +k8s_resource(new_name='peer-spine1-unnumbered', objects=['leaf1-spine1-unnumbered:bgppeer'], resource_deps=['bgp', 'eth1-4'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) k8s_yaml('./config/samples/v1alpha1_ospf.yaml') k8s_resource(new_name='ospf-underlay', objects=['underlay:ospf'], resource_deps=['lo0', 'lo1', 'eth1-1', 'eth1-2'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) diff --git a/api/core/v1alpha1/bgp_peer_types.go b/api/core/v1alpha1/bgp_peer_types.go index ba54aa75e..e2498368a 100644 --- a/api/core/v1alpha1/bgp_peer_types.go +++ b/api/core/v1alpha1/bgp_peer_types.go @@ -13,6 +13,11 @@ import ( ) // BGPPeerSpec defines the desired state of BGPPeer +// +kubebuilder:validation:XValidation:rule="has(self.address) != has(self.interfaceRef)", message="exactly one of address or interfaceRef must be specified" +// +kubebuilder:validation:XValidation:rule="!has(self.interfaceRef) || !has(self.localAddress)", message="localAddress must not be specified for interface-based peers" +// +kubebuilder:validation:XValidation:rule="type(self.asNumber) != string || self.asNumber != 'external' || has(self.interfaceRef)", message="asNumber external requires interfaceRef" +// +kubebuilder:validation:XValidation:rule="(!has(self.address) && !has(oldSelf.address)) || (has(self.address) && has(oldSelf.address) && self.address == oldSelf.address)",message="Address is immutable" +// +kubebuilder:validation:XValidation:rule="(!has(self.interfaceRef) && !has(oldSelf.interfaceRef)) || (has(self.interfaceRef) && has(oldSelf.interfaceRef) && self.interfaceRef == oldSelf.interfaceRef)",message="InterfaceRef is immutable" type BGPPeerSpec struct { // DeviceName is the name of the Device this object belongs to. The Device object must exist in the same namespace. // Immutable. @@ -27,7 +32,9 @@ type BGPPeerSpec struct { // BgpRef is a reference to the BGP instance this peer belongs to. // The BGP object must exist in the same namespace. + // Immutable. // +required + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="BgpRef is immutable" BgpRef LocalObjectReference `json:"bgpRef"` // AdminState indicates whether this BGP peer is administratively up or down. @@ -37,12 +44,26 @@ type BGPPeerSpec struct { AdminState AdminState `json:"adminState,omitempty"` // Address is the IPv4 address of the BGP peer. - // +required + // Mutually exclusive with InterfaceRef: exactly one of both must be specified. + // Immutable. + // +optional // +kubebuilder:validation:Format=ipv4 - Address string `json:"address"` + Address string `json:"address,omitempty"` + + // InterfaceRef is a reference to an Interface resource over which an unnumbered + // (interface-based) BGP session is established. The peers discover each other over + // their IPv6 link-local addresses, so the link needs no addressing of its own. + // The referenced Interface must belong to the same Device, exist in the same namespace, + // and be configured for link-local operation (spec.ipv6.useLinkLocalOnly). + // Mutually exclusive with Address: exactly one of both must be specified. + // Immutable. + // +optional + InterfaceRef *LocalObjectReference `json:"interfaceRef,omitempty"` // ASNumber is the autonomous system number (ASN) of the BGP peer. // Supports both plain format (1-4294967295) and dotted notation (0-65535.0-65535) as per RFC 5396. + // The special value "external" configures a dynamic AS number, accepting any AS number + // that differs from the local one. It is only valid together with InterfaceRef. // +required ASNumber intstr.IntOrString `json:"asNumber"` @@ -66,6 +87,16 @@ type BGPPeerSpec struct { LocalAS *LocalAS `json:"localAS,omitempty"` } +// BGPPeerASNumberExternal is the value of BGPPeerSpec.ASNumber that requests a dynamic +// AS number for the peer. The session is established with any AS number that differs from +// the local one, which is the common setup for unnumbered eBGP peerings. +const BGPPeerASNumberExternal = "external" + +// IsExternalASNumber reports whether the peer is configured with a dynamic AS number. +func (s *BGPPeerSpec) IsExternalASNumber() bool { + return s.ASNumber.Type == intstr.String && s.ASNumber.StrVal == BGPPeerASNumberExternal +} + // LocalAS defines the local AS configuration and how it factors in BGP announcements. type LocalAS struct { // ASNumber specifies a local AS number to present in BGP sessions with this peer. @@ -178,6 +209,12 @@ type BGPPeerStatus struct { // +patchMergeKey=afiSafi AddressFamilies []AddressFamilyStatus `json:"addressFamilies,omitempty"` + // PeerInterface is the device-level name of the interface an unnumbered peer is + // configured over. It is recorded so that the peer can still be removed from the + // device after the referenced Interface has been deleted. + // +optional + PeerInterface string `json:"peerInterface,omitempty"` + // ObservedGeneration reflects the .metadata.generation that was last processed by the controller. // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` @@ -254,6 +291,7 @@ const ( // +kubebuilder:resource:singular=bgppeer // +kubebuilder:resource:shortName=peer;bgpneighbor // +kubebuilder:printcolumn:name="Peer Address",type=string,JSONPath=`.spec.address` +// +kubebuilder:printcolumn:name="Peer Interface",type=string,JSONPath=`.spec.interfaceRef.name` // +kubebuilder:printcolumn:name="Device",type=string,JSONPath=`.spec.deviceRef.name` // +kubebuilder:printcolumn:name="Admin State",type=string,JSONPath=`.spec.adminState` // +kubebuilder:printcolumn:name="AS Number",type=string,JSONPath=`.spec.asNumber` diff --git a/api/core/v1alpha1/zz_generated.deepcopy.go b/api/core/v1alpha1/zz_generated.deepcopy.go index 7ceabafe5..446abc4af 100644 --- a/api/core/v1alpha1/zz_generated.deepcopy.go +++ b/api/core/v1alpha1/zz_generated.deepcopy.go @@ -833,6 +833,11 @@ func (in *BGPPeerSpec) DeepCopyInto(out *BGPPeerSpec) { **out = **in } out.BgpRef = in.BgpRef + if in.InterfaceRef != nil { + in, out := &in.InterfaceRef, &out.InterfaceRef + *out = new(LocalObjectReference) + **out = **in + } out.ASNumber = in.ASNumber if in.LocalAddress != nil { in, out := &in.LocalAddress, &out.LocalAddress diff --git a/charts/network-operator/templates/crd/bgppeers.networking.metal.ironcore.dev.yaml b/charts/network-operator/templates/crd/bgppeers.networking.metal.ironcore.dev.yaml index 6515b97bd..cbfd5d5ec 100644 --- a/charts/network-operator/templates/crd/bgppeers.networking.metal.ironcore.dev.yaml +++ b/charts/network-operator/templates/crd/bgppeers.networking.metal.ironcore.dev.yaml @@ -24,6 +24,9 @@ spec: - jsonPath: .spec.address name: Peer Address type: string + - jsonPath: .spec.interfaceRef.name + name: Peer Interface + type: string - jsonPath: .spec.deviceRef.name name: Device type: string @@ -91,7 +94,10 @@ spec: More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status properties: address: - description: Address is the IPv4 address of the BGP peer. + description: |- + Address is the IPv4 address of the BGP peer. + Mutually exclusive with InterfaceRef: exactly one of both must be specified. + Immutable. format: ipv4 type: string addressFamilies: @@ -290,11 +296,14 @@ spec: description: |- ASNumber is the autonomous system number (ASN) of the BGP peer. Supports both plain format (1-4294967295) and dotted notation (0-65535.0-65535) as per RFC 5396. + The special value "external" configures a dynamic AS number, accepting any AS number + that differs from the local one. It is only valid together with InterfaceRef. x-kubernetes-int-or-string: true bgpRef: description: |- BgpRef is a reference to the BGP instance this peer belongs to. The BGP object must exist in the same namespace. + Immutable. properties: name: description: |- @@ -307,6 +316,9 @@ spec: - name type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: BgpRef is immutable + rule: self == oldSelf description: description: |- Description is an optional human-readable description for this BGP peer. @@ -331,6 +343,27 @@ spec: x-kubernetes-validations: - message: DeviceRef is immutable rule: self == oldSelf + interfaceRef: + description: |- + InterfaceRef is a reference to an Interface resource over which an unnumbered + (interface-based) BGP session is established. The peers discover each other over + their IPv6 link-local addresses, so the link needs no addressing of its own. + The referenced Interface must belong to the same Device, exist in the same namespace, + and be configured for link-local operation (spec.ipv6.useLinkLocalOnly). + Mutually exclusive with Address: exactly one of both must be specified. + Immutable. + properties: + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + maxLength: 63 + minLength: 1 + type: string + required: + - name + type: object + x-kubernetes-map-type: atomic localAS: description: LocalAS configures the local AS number and how it factors into BGP announcements for this peer. @@ -415,11 +448,24 @@ spec: type: object x-kubernetes-map-type: atomic required: - - address - asNumber - bgpRef - deviceRef type: object + x-kubernetes-validations: + - message: exactly one of address or interfaceRef must be specified + rule: has(self.address) != has(self.interfaceRef) + - message: localAddress must not be specified for interface-based peers + rule: '!has(self.interfaceRef) || !has(self.localAddress)' + - message: asNumber external requires interfaceRef + rule: type(self.asNumber) != string || self.asNumber != 'external' || + has(self.interfaceRef) + - message: Address is immutable + rule: (!has(self.address) && !has(oldSelf.address)) || (has(self.address) + && has(oldSelf.address) && self.address == oldSelf.address) + - message: InterfaceRef is immutable + rule: (!has(self.interfaceRef) && !has(oldSelf.interfaceRef)) || (has(self.interfaceRef) + && has(oldSelf.interfaceRef) && self.interfaceRef == oldSelf.interfaceRef) status: description: |- Status of the resource. This is set and updated automatically. @@ -541,6 +587,12 @@ spec: that was last processed by the controller. format: int64 type: integer + peerInterface: + description: |- + PeerInterface is the device-level name of the interface an unnumbered peer is + configured over. It is recorded so that the peer can still be removed from the + device after the referenced Interface has been deleted. + type: string sessionState: description: SessionState is the current operational state of the BGP session. diff --git a/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml b/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml index d5d54d272..3f0d24658 100644 --- a/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml +++ b/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml @@ -21,6 +21,9 @@ spec: - jsonPath: .spec.address name: Peer Address type: string + - jsonPath: .spec.interfaceRef.name + name: Peer Interface + type: string - jsonPath: .spec.deviceRef.name name: Device type: string @@ -88,7 +91,10 @@ spec: More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status properties: address: - description: Address is the IPv4 address of the BGP peer. + description: |- + Address is the IPv4 address of the BGP peer. + Mutually exclusive with InterfaceRef: exactly one of both must be specified. + Immutable. format: ipv4 type: string addressFamilies: @@ -287,11 +293,14 @@ spec: description: |- ASNumber is the autonomous system number (ASN) of the BGP peer. Supports both plain format (1-4294967295) and dotted notation (0-65535.0-65535) as per RFC 5396. + The special value "external" configures a dynamic AS number, accepting any AS number + that differs from the local one. It is only valid together with InterfaceRef. x-kubernetes-int-or-string: true bgpRef: description: |- BgpRef is a reference to the BGP instance this peer belongs to. The BGP object must exist in the same namespace. + Immutable. properties: name: description: |- @@ -304,6 +313,9 @@ spec: - name type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: BgpRef is immutable + rule: self == oldSelf description: description: |- Description is an optional human-readable description for this BGP peer. @@ -328,6 +340,27 @@ spec: x-kubernetes-validations: - message: DeviceRef is immutable rule: self == oldSelf + interfaceRef: + description: |- + InterfaceRef is a reference to an Interface resource over which an unnumbered + (interface-based) BGP session is established. The peers discover each other over + their IPv6 link-local addresses, so the link needs no addressing of its own. + The referenced Interface must belong to the same Device, exist in the same namespace, + and be configured for link-local operation (spec.ipv6.useLinkLocalOnly). + Mutually exclusive with Address: exactly one of both must be specified. + Immutable. + properties: + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + maxLength: 63 + minLength: 1 + type: string + required: + - name + type: object + x-kubernetes-map-type: atomic localAS: description: LocalAS configures the local AS number and how it factors into BGP announcements for this peer. @@ -412,11 +445,24 @@ spec: type: object x-kubernetes-map-type: atomic required: - - address - asNumber - bgpRef - deviceRef type: object + x-kubernetes-validations: + - message: exactly one of address or interfaceRef must be specified + rule: has(self.address) != has(self.interfaceRef) + - message: localAddress must not be specified for interface-based peers + rule: '!has(self.interfaceRef) || !has(self.localAddress)' + - message: asNumber external requires interfaceRef + rule: type(self.asNumber) != string || self.asNumber != 'external' || + has(self.interfaceRef) + - message: Address is immutable + rule: (!has(self.address) && !has(oldSelf.address)) || (has(self.address) + && has(oldSelf.address) && self.address == oldSelf.address) + - message: InterfaceRef is immutable + rule: (!has(self.interfaceRef) && !has(oldSelf.interfaceRef)) || (has(self.interfaceRef) + && has(oldSelf.interfaceRef) && self.interfaceRef == oldSelf.interfaceRef) status: description: |- Status of the resource. This is set and updated automatically. @@ -538,6 +584,12 @@ spec: that was last processed by the controller. format: int64 type: integer + peerInterface: + description: |- + PeerInterface is the device-level name of the interface an unnumbered peer is + configured over. It is recorded so that the peer can still be removed from the + device after the referenced Interface has been deleted. + type: string sessionState: description: SessionState is the current operational state of the BGP session. diff --git a/config/samples/v1alpha1_bgppeer.yaml b/config/samples/v1alpha1_bgppeer.yaml index 98c4625e2..14c04891b 100644 --- a/config/samples/v1alpha1_bgppeer.yaml +++ b/config/samples/v1alpha1_bgppeer.yaml @@ -66,3 +66,25 @@ spec: name: bgp-import-policy outboundRoutingPolicyRef: name: bgp-import-policy +--- +apiVersion: networking.metal.ironcore.dev/v1alpha1 +kind: BGPPeer +metadata: + labels: + app.kubernetes.io/name: network-operator + app.kubernetes.io/managed-by: kustomize + name: leaf1-spine1-unnumbered +spec: + deviceRef: + name: leaf1 + bgpRef: + name: bgp + # Unnumbered peering: the session runs over the interface's IPv6 link-local + # address, and "external" accepts any AS number that differs from the local one. + interfaceRef: + name: eth1-4 + asNumber: external + description: Unnumbered eBGP to spine1 + addressFamilies: + ipv4Unicast: + enabled: true diff --git a/docs/api-reference/index.md b/docs/api-reference/index.md index f89990a8f..04d6622a5 100644 --- a/docs/api-reference/index.md +++ b/docs/api-reference/index.md @@ -1053,10 +1053,11 @@ _Appears in:_ | --- | --- | --- | --- | | `deviceRef` _[LocalObjectReference](#localobjectreference)_ | DeviceName is the name of the Device this object belongs to. The Device object must exist in the same namespace.
Immutable. | | Required: \{\}
| | `providerConfigRef` _[TypedLocalObjectReference](#typedlocalobjectreference)_ | ProviderConfigRef is a reference to a resource holding the provider-specific configuration of this interface.
This reference is used to link the BGPPeer to its provider-specific configuration. | | Optional: \{\}
| -| `bgpRef` _[LocalObjectReference](#localobjectreference)_ | BgpRef is a reference to the BGP instance this peer belongs to.
The BGP object must exist in the same namespace. | | Required: \{\}
| +| `bgpRef` _[LocalObjectReference](#localobjectreference)_ | BgpRef is a reference to the BGP instance this peer belongs to.
The BGP object must exist in the same namespace.
Immutable. | | Required: \{\}
| | `adminState` _[AdminState](#adminstate)_ | AdminState indicates whether this BGP peer is administratively up or down.
When Down, the BGP session with this peer is administratively shut down. | Up | Enum: [Up Down]
Optional: \{\}
| -| `address` _string_ | Address is the IPv4 address of the BGP peer. | | Format: ipv4
Required: \{\}
| -| `asNumber` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#intorstring-intstr-util)_ | ASNumber is the autonomous system number (ASN) of the BGP peer.
Supports both plain format (1-4294967295) and dotted notation (0-65535.0-65535) as per RFC 5396. | | Required: \{\}
| +| `address` _string_ | Address is the IPv4 address of the BGP peer.
Mutually exclusive with InterfaceRef: exactly one of both must be specified.
Immutable. | | Format: ipv4
Optional: \{\}
| +| `interfaceRef` _[LocalObjectReference](#localobjectreference)_ | InterfaceRef is a reference to an Interface resource over which an unnumbered
(interface-based) BGP session is established. The peers discover each other over
their IPv6 link-local addresses, so the link needs no addressing of its own.
The referenced Interface must belong to the same Device, exist in the same namespace,
and be configured for link-local operation (spec.ipv6.useLinkLocalOnly).
Mutually exclusive with Address: exactly one of both must be specified.
Immutable. | | Optional: \{\}
| +| `asNumber` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#intorstring-intstr-util)_ | ASNumber is the autonomous system number (ASN) of the BGP peer.
Supports both plain format (1-4294967295) and dotted notation (0-65535.0-65535) as per RFC 5396.
The special value "external" configures a dynamic AS number, accepting any AS number
that differs from the local one. It is only valid together with InterfaceRef. | | Required: \{\}
| | `description` _string_ | Description is an optional human-readable description for this BGP peer.
This field is used for documentation purposes and may be displayed in management interfaces. | | Optional: \{\}
| | `localAddress` _[BGPPeerLocalAddress](#bgppeerlocaladdress)_ | LocalAddress specifies the local address configuration for the BGP session with this peer.
This determines the source address/interface for BGP packets sent to this peer. | | Optional: \{\}
| | `addressFamilies` _[BGPPeerAddressFamilies](#bgppeeraddressfamilies)_ | AddressFamilies configures address family specific settings for this BGP peer.
Controls which address families are enabled and their specific configuration. | | Optional: \{\}
| @@ -1080,6 +1081,7 @@ _Appears in:_ | `lastEstablishedTime` _[Time](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#time-v1-meta)_ | LastEstablishedTime is the timestamp when the BGP session last transitioned to the ESTABLISHED state.
A frequently changing timestamp indicates session instability (flapping). | | Optional: \{\}
| | `advertisedPrefixesSummary` _string_ | AdvertisedPrefixesSummary provides a human-readable summary of advertised prefixes
across all address families (e.g., "10 (IPv4Unicast), 5 (IPv6Unicast)").
This field is computed by the controller from the AddressFamilies field. | | Optional: \{\}
| | `addressFamilies` _[AddressFamilyStatus](#addressfamilystatus) array_ | AddressFamilies contains per-address-family statistics for this peer.
Only address families that are enabled and negotiated with the peer are included. | | Optional: \{\}
| +| `peerInterface` _string_ | PeerInterface is the device-level name of the interface an unnumbered peer is
configured over. It is recorded so that the peer can still be removed from the
device after the referenced Interface has been deleted. | | Optional: \{\}
| | `observedGeneration` _integer_ | ObservedGeneration reflects the .metadata.generation that was last processed by the controller. | | Optional: \{\}
| | `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#condition-v1-meta) array_ | The conditions are a list of status objects that describe the state of the BGP. | | Optional: \{\}
| diff --git a/hack/provider/main.go b/hack/provider/main.go index 097e48c4e..ce554666c 100644 --- a/hack/provider/main.go +++ b/hack/provider/main.go @@ -503,6 +503,22 @@ func performCreate(ctx context.Context, prov provider.Provider, obj client.Objec sourceInterface = iface.Spec.Name } + peerInterface := "" + if res.Spec.InterfaceRef != nil && res.Spec.InterfaceRef.Name != "" { + if len(refStore) == 0 { + return errors.New("bgppeer resource references peer interface but no reference files provided (use --ref-files)") + } + obj := refStore.Get(res.Spec.InterfaceRef.Name, res.Namespace) + if obj == nil { + return fmt.Errorf("referenced peer interface %s not found in reference files", res.Spec.InterfaceRef.Name) + } + iface, ok := obj.(*v1alpha1.Interface) + if !ok { + return fmt.Errorf("referenced resource %s is not an Interface", res.Spec.InterfaceRef.Name) + } + peerInterface = iface.Spec.Name + } + var cfg *provider.ProviderConfig if res.Spec.ProviderConfigRef != nil { var err error @@ -515,6 +531,7 @@ func performCreate(ctx context.Context, prov provider.Provider, obj client.Objec return bpp.EnsureBGPPeer(ctx, &provider.EnsureBGPPeerRequest{ BGPPeer: res, SourceInterface: sourceInterface, + PeerInterface: peerInterface, ProviderConfig: cfg, }) @@ -1109,8 +1126,25 @@ func performDelete(ctx context.Context, prov provider.Provider, obj client.Objec if !ok { return errors.New("provider does not implement BGPPeerProvider") } + + // An unnumbered peer is identified by its interface, so it has to be + // resolved for the deletion as well. + peerInterface := "" + if resource.Spec.InterfaceRef != nil && resource.Spec.InterfaceRef.Name != "" { + obj := refStore.Get(resource.Spec.InterfaceRef.Name, resource.Namespace) + if obj == nil { + return fmt.Errorf("referenced peer interface %s not found in reference files", resource.Spec.InterfaceRef.Name) + } + iface, ok := obj.(*v1alpha1.Interface) + if !ok { + return fmt.Errorf("referenced resource %s is not an Interface", resource.Spec.InterfaceRef.Name) + } + peerInterface = iface.Spec.Name + } + return bpp.DeleteBGPPeer(ctx, &provider.DeleteBGPPeerRequest{ - BGPPeer: resource, + BGPPeer: resource, + PeerInterface: peerInterface, }) case *v1alpha1.Certificate: diff --git a/internal/controller/core/bgp_peer_controller.go b/internal/controller/core/bgp_peer_controller.go index ff7014c56..a786fa5e7 100644 --- a/internal/controller/core/bgp_peer_controller.go +++ b/internal/controller/core/bgp_peer_controller.go @@ -47,6 +47,10 @@ const bgpPeerBGPRefIndexKey = ".spec.bgpRef.name" // referenced by BGPPeer address families. const bgpPeerRoutingPolicyRefIndexKey = ".spec.addressFamilies.routingPolicyRefs" +// bgpPeerInterfaceRefIndexKey is the field index key for all Interface names referenced by +// a BGPPeer, both as unnumbered peer interface and as local (source) address. +const bgpPeerInterfaceRefIndexKey = ".spec.interfaceRefs" + // BGPPeerReconciler reconciles a BGPPeer object type BGPPeerReconciler struct { client.Client @@ -276,6 +280,20 @@ func (r *BGPPeerReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return err } + if err := mgr.GetFieldIndexer().IndexField(ctx, &v1alpha1.BGPPeer{}, bgpPeerInterfaceRefIndexKey, func(obj client.Object) []string { + o := obj.(*v1alpha1.BGPPeer) + var names []string + if o.Spec.InterfaceRef != nil { + names = append(names, o.Spec.InterfaceRef.Name) + } + if o.Spec.LocalAddress != nil { + names = append(names, o.Spec.LocalAddress.InterfaceRef.Name) + } + return names + }); err != nil { + return err + } + bldr := ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.BGPPeer{}). Named("bgppeer"). @@ -353,6 +371,20 @@ func (r *BGPPeerReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag }, }), ). + // Watches enqueues BGPPeers when a referenced Interface is created or deleted. + // Only triggers on create and delete events since interface names are immutable. + Watches( + &v1alpha1.Interface{}, + handler.EnqueueRequestsFromMapFunc(r.interfaceToBGPPeers), + builder.WithPredicates(predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + return false + }, + GenericFunc: func(e event.GenericEvent) bool { + return false + }, + }), + ). Complete(r) } @@ -418,30 +450,23 @@ func (r *BGPPeerReconciler) reconcile(ctx context.Context, s *bgpPeerScope) (ret var sourceInterface string if addr := s.BGPPeer.Spec.LocalAddress; addr != nil { - intf := new(v1alpha1.Interface) - if err := r.Get(ctx, client.ObjectKey{Name: addr.InterfaceRef.Name, Namespace: s.BGPPeer.Namespace}, intf); err != nil { - if apierrors.IsNotFound(err) { - conditions.Set(s.BGPPeer, metav1.Condition{ - Type: v1alpha1.ConfiguredCondition, - Status: metav1.ConditionFalse, - Reason: v1alpha1.InterfaceNotFoundReason, - Message: fmt.Sprintf("source interface %q not found", addr.InterfaceRef.Name), - }) - return reconcile.TerminalError(fmt.Errorf("source interface %q not found", addr.InterfaceRef.Name)) - } - return fmt.Errorf("failed to get source interface %q: %w", addr.InterfaceRef.Name, err) + intf, err := r.reconcileInterfaceRef(ctx, s, addr.InterfaceRef.Name, "source interface") + if err != nil { + return err } + sourceInterface = intf.Spec.Name + } - if intf.Spec.DeviceRef.Name != s.Device.Name { - conditions.Set(s.BGPPeer, metav1.Condition{ - Type: v1alpha1.ConfiguredCondition, - Status: metav1.ConditionFalse, - Reason: v1alpha1.CrossDeviceReferenceReason, - Message: fmt.Sprintf("source interface %q does not belong to device %q", intf.Name, s.Device.Name), - }) - return reconcile.TerminalError(fmt.Errorf("source interface %q does not belong to device %q", intf.Name, s.Device.Name)) + var peerInterface string + if ref := s.BGPPeer.Spec.InterfaceRef; ref != nil { + intf, err := r.reconcileInterfaceRef(ctx, s, ref.Name, "peer interface") + if err != nil { + return err } - sourceInterface = intf.Spec.Name + peerInterface = intf.Spec.Name + // Recorded even if configuring the device fails below, so that a partially + // applied configuration can still be cleaned up once the Interface is gone. + s.BGPPeer.Status.PeerInterface = peerInterface } if s.BGPPeer.Spec.LocalAS != nil && s.BGPPeer.Spec.ASNumber.String() == bgp.Spec.ASNumber.String() { @@ -468,6 +493,7 @@ func (r *BGPPeerReconciler) reconcile(ctx context.Context, s *bgpPeerScope) (ret BGPPeer: s.BGPPeer, ProviderConfig: s.ProviderConfig, SourceInterface: sourceInterface, + PeerInterface: peerInterface, BGP: bgp, VRF: vrf, InboundRoutingPolicies: inbound, @@ -484,6 +510,7 @@ func (r *BGPPeerReconciler) reconcile(ctx context.Context, s *bgpPeerScope) (ret status, err := s.Provider.GetPeerStatus(ctx, &provider.BGPPeerStatusRequest{ BGPPeer: s.BGPPeer, ProviderConfig: s.ProviderConfig, + PeerInterface: peerInterface, VRF: vrf, }) if err != nil { @@ -559,6 +586,28 @@ func (r *BGPPeerReconciler) finalize(ctx context.Context, s *bgpPeerScope) (rete } } + // The interface is the identity of an unnumbered peer on the device. Prefer the name + // recorded during reconciliation, as the Interface may already have been deleted. + var peerInterface string + if ref := s.BGPPeer.Spec.InterfaceRef; ref != nil { + peerInterface = s.BGPPeer.Status.PeerInterface + if peerInterface == "" { + intf := new(v1alpha1.Interface) + if err := r.Get(ctx, types.NamespacedName{ + Name: ref.Name, + Namespace: s.BGPPeer.Namespace, + }, intf); err != nil { + // Without the recorded name and the Interface the peer cannot be + // identified on the device, so we can only proceed with deletion. + return client.IgnoreNotFound(err) + } + if intf.Spec.DeviceRef.Name != s.Device.Name { + return reconcile.TerminalError(fmt.Errorf("interface %s belongs to different device", ref.Name)) + } + peerInterface = intf.Spec.Name + } + } + if err := s.Provider.Connect(ctx, s.Connection); err != nil { return fmt.Errorf("failed to connect to provider: %w", err) } @@ -569,6 +618,7 @@ func (r *BGPPeerReconciler) finalize(ctx context.Context, s *bgpPeerScope) (rete }() return s.Provider.DeleteBGPPeer(ctx, &provider.DeleteBGPPeerRequest{ + PeerInterface: peerInterface, BGPPeer: s.BGPPeer, ProviderConfig: s.ProviderConfig, BGP: bgp, @@ -576,6 +626,37 @@ func (r *BGPPeerReconciler) finalize(ctx context.Context, s *bgpPeerScope) (rete }) } +// reconcileInterfaceRef resolves an Interface referenced by the BGPPeer and validates that +// it belongs to the same Device. The description is used in conditions and errors to tell +// the source interface and the unnumbered peer interface apart. +func (r *BGPPeerReconciler) reconcileInterfaceRef(ctx context.Context, s *bgpPeerScope, name, description string) (*v1alpha1.Interface, error) { + intf := new(v1alpha1.Interface) + if err := r.Get(ctx, client.ObjectKey{Name: name, Namespace: s.BGPPeer.Namespace}, intf); err != nil { + if apierrors.IsNotFound(err) { + conditions.Set(s.BGPPeer, metav1.Condition{ + Type: v1alpha1.ConfiguredCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.InterfaceNotFoundReason, + Message: fmt.Sprintf("%s %q not found", description, name), + }) + return nil, reconcile.TerminalError(fmt.Errorf("%s %q not found", description, name)) + } + return nil, fmt.Errorf("failed to get %s %q: %w", description, name, err) + } + + if intf.Spec.DeviceRef.Name != s.Device.Name { + conditions.Set(s.BGPPeer, metav1.Condition{ + Type: v1alpha1.ConfiguredCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.CrossDeviceReferenceReason, + Message: fmt.Sprintf("%s %q does not belong to device %q", description, intf.Name, s.Device.Name), + }) + return nil, reconcile.TerminalError(fmt.Errorf("%s %q does not belong to device %q", description, intf.Name, s.Device.Name)) + } + + return intf, nil +} + // reconcileBGP resolves the referenced BGP instance. // Sets ConfiguredCondition and returns a terminal error when the BGP is not found // or belongs to a different device. @@ -771,6 +852,32 @@ func (r *BGPPeerReconciler) bgpPeersForProviderConfig(ctx context.Context, obj c // bgpToBGPPeers is a [handler.MapFunc] to be used to enqueue requests for reconciliation // for BGPPeers when a BGP resource is created, deleted or updated on the same device. +func (r *BGPPeerReconciler) interfaceToBGPPeers(ctx context.Context, obj client.Object) []ctrl.Request { + intf, ok := obj.(*v1alpha1.Interface) + if !ok { + panic(fmt.Sprintf("Expected an Interface but got a %T", obj)) + } + + log := ctrl.LoggerFrom(ctx, "Interface", klog.KObj(intf)) + + list := new(v1alpha1.BGPPeerList) + if err := r.List( + ctx, list, + client.InNamespace(intf.Namespace), + client.MatchingFields{bgpPeerInterfaceRefIndexKey: intf.Name}, + ); err != nil { + log.Error(err, "Failed to list BGPPeers") + return nil + } + + requests := make([]ctrl.Request, 0, len(list.Items)) + for i := range list.Items { + log.V(2).Info("Enqueuing BGPPeer for reconciliation", "BGPPeer", klog.KObj(&list.Items[i])) + requests = append(requests, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) + } + return requests +} + func (r *BGPPeerReconciler) bgpToBGPPeers(ctx context.Context, obj client.Object) []ctrl.Request { bgp, ok := obj.(*v1alpha1.BGP) if !ok { diff --git a/internal/controller/core/bgp_peer_controller_test.go b/internal/controller/core/bgp_peer_controller_test.go index fa89c92b0..b9fa90367 100644 --- a/internal/controller/core/bgp_peer_controller_test.go +++ b/internal/controller/core/bgp_peer_controller_test.go @@ -6,6 +6,7 @@ package core import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -281,6 +282,164 @@ var _ = Describe("BGPPeer Controller", func() { }).Should(Succeed()) }) + It("Should handle peer interface reference to non-existing Interface", func() { + By("Creating a BGP resource for the Device") + bgp := &v1alpha1.BGP{ + GenerateName: "test-bgp-", + Namespace: metav1.NamespaceDefault, + Spec: v1alpha1.BGPSpec{ + DeviceRef: v1alpha1.LocalObjectReference{Name: device.Name}, + ASNumber: intstr.FromInt(65000), + RouterID: "10.0.0.1", + }, + } + Expect(k8sClient.Create(ctx, bgp)).To(Succeed()) + + Eventually(func(g Gomega) { + b := &v1alpha1.BGP{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bgp), b)).To(Succeed()) + g.Expect(conditions.IsReady(b)).To(BeTrue()) + }).Should(Succeed()) + + By("Creating an unnumbered BGPPeer pointing to a non-existent Interface") + bgppeer := &v1alpha1.BGPPeer{ + GenerateName: "test-bgppeer-", + Namespace: metav1.NamespaceDefault, + Spec: v1alpha1.BGPPeerSpec{ + DeviceRef: v1alpha1.LocalObjectReference{Name: device.Name}, + BgpRef: v1alpha1.LocalObjectReference{Name: bgp.Name}, + InterfaceRef: &v1alpha1.LocalObjectReference{Name: "non-existing-interface"}, + ASNumber: intstr.FromString(v1alpha1.BGPPeerASNumberExternal), + }, + } + Expect(k8sClient.Create(ctx, bgppeer)).To(Succeed()) + + By("Verifying the controller sets Interface not found status") + Eventually(func(g Gomega) { + resource := &v1alpha1.BGPPeer{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bgppeer), resource)).To(Succeed()) + g.Expect(resource.Status.Conditions).To(HaveLen(4)) + g.Expect(conditions.IsConfigured(resource)).To(BeFalse()) + g.Expect(resource.Status.Conditions[1].Type).To(Equal(v1alpha1.ConfiguredCondition)) + g.Expect(resource.Status.Conditions[1].Reason).To(Equal(v1alpha1.InterfaceNotFoundReason)) + g.Expect(resource.Status.Conditions[1].Message).To(ContainSubstring("peer interface")) + }).Should(Succeed()) + }) + + It("Should remove an unnumbered BGP peer from the provider after its Interface was deleted", func() { + By("Creating a BGP resource for the Device") + bgp := &v1alpha1.BGP{ + GenerateName: "test-bgppeer-bgp-", + Namespace: metav1.NamespaceDefault, + Spec: v1alpha1.BGPSpec{ + DeviceRef: v1alpha1.LocalObjectReference{Name: device.Name}, + ASNumber: intstr.FromInt(65000), + RouterID: "10.0.0.10", + }, + } + Expect(k8sClient.Create(ctx, bgp)).To(Succeed()) + + Eventually(func(g Gomega) { + b := &v1alpha1.BGP{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bgp), b)).To(Succeed()) + g.Expect(conditions.IsReady(b)).To(BeTrue()) + }).Should(Succeed()) + + By("Creating a link-local-only Interface resource") + intf := &v1alpha1.Interface{ + GenerateName: "test-bgppeer-intf-", + Namespace: metav1.NamespaceDefault, + Spec: v1alpha1.InterfaceSpec{ + DeviceRef: v1alpha1.LocalObjectReference{Name: device.Name}, + Name: "Ethernet1/1", + AdminState: v1alpha1.AdminStateUp, + Type: v1alpha1.InterfaceTypePhysical, + IPv6: &v1alpha1.InterfaceIPv6{UseLinkLocalOnly: true}, + }, + } + Expect(k8sClient.Create(ctx, intf)).To(Succeed()) + + By("Creating an unnumbered BGPPeer over the Interface") + bgppeer := &v1alpha1.BGPPeer{ + GenerateName: "test-bgppeer-", + Namespace: metav1.NamespaceDefault, + Spec: v1alpha1.BGPPeerSpec{ + DeviceRef: v1alpha1.LocalObjectReference{Name: device.Name}, + BgpRef: v1alpha1.LocalObjectReference{Name: bgp.Name}, + InterfaceRef: &v1alpha1.LocalObjectReference{Name: intf.Name}, + ASNumber: intstr.FromString(v1alpha1.BGPPeerASNumberExternal), + }, + } + Expect(k8sClient.Create(ctx, bgppeer)).To(Succeed()) + + By("Verifying the peer is configured and its interface is recorded") + Eventually(func(g Gomega) { + resource := &v1alpha1.BGPPeer{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bgppeer), resource)).To(Succeed()) + g.Expect(resource.Status.PeerInterface).To(Equal("Ethernet1/1")) + g.Expect(testProvider.BGPPeers.Has("Ethernet1/1")).To(BeTrue()) + }).Should(Succeed()) + + By("Deleting the Interface before the BGPPeer") + Expect(k8sClient.Delete(ctx, intf)).To(Succeed()) + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(intf), &v1alpha1.Interface{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }).Should(Succeed()) + + By("Deleting the BGPPeer") + Expect(k8sClient.Delete(ctx, bgppeer)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(testProvider.BGPPeers.Has("Ethernet1/1")).To(BeFalse(), "Provider should not have the unnumbered BGP peer configured") + }).Should(Succeed()) + }) + + It("Should reject changes to the peer identity", func() { + By("Creating a BGPPeer resource") + bgppeer := &v1alpha1.BGPPeer{ + GenerateName: "test-bgppeer-", + Namespace: metav1.NamespaceDefault, + Spec: v1alpha1.BGPPeerSpec{ + DeviceRef: v1alpha1.LocalObjectReference{Name: device.Name}, + BgpRef: v1alpha1.LocalObjectReference{Name: "bgp"}, + Address: host, + ASNumber: intstr.FromInt(65000), + }, + } + Expect(k8sClient.Create(ctx, bgppeer)).To(Succeed()) + + for _, tc := range []struct { + mutate func(*v1alpha1.BGPPeer) + message string + }{ + { + mutate: func(p *v1alpha1.BGPPeer) { p.Spec.Address = "10.0.0.2" }, + message: "Address is immutable", + }, + { + mutate: func(p *v1alpha1.BGPPeer) { + p.Spec.Address = "" + p.Spec.InterfaceRef = &v1alpha1.LocalObjectReference{Name: "eth1-1"} + }, + message: "InterfaceRef is immutable", + }, + { + mutate: func(p *v1alpha1.BGPPeer) { p.Spec.BgpRef.Name = "other-bgp" }, + message: "BgpRef is immutable", + }, + } { + By("Attempting an update that must be rejected with: " + tc.message) + Eventually(func(g Gomega) { + resource := &v1alpha1.BGPPeer{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(bgppeer), resource)).To(Succeed()) + tc.mutate(resource) + err := k8sClient.Update(ctx, resource) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.message)) + }).Should(Succeed()) + } + }) + It("Should reject local address reference to Interface on different device", func() { By("Creating a BGP resource for the Device") bgp := &v1alpha1.BGP{ diff --git a/internal/controller/core/suite_test.go b/internal/controller/core/suite_test.go index 48ca12a49..e786b2da2 100644 --- a/internal/controller/core/suite_test.go +++ b/internal/controller/core/suite_test.go @@ -4,6 +4,7 @@ package core import ( + "cmp" "context" "errors" "fmt" @@ -793,14 +794,14 @@ func (p *Provider) DeleteBGP(context.Context, *provider.DeleteBGPRequest) error func (p *Provider) EnsureBGPPeer(_ context.Context, req *provider.EnsureBGPPeerRequest) error { p.Lock() defer p.Unlock() - p.BGPPeers.Insert(req.BGPPeer.Spec.Address) + p.BGPPeers.Insert(cmp.Or(req.PeerInterface, req.BGPPeer.Spec.Address)) return nil } func (p *Provider) DeleteBGPPeer(_ context.Context, req *provider.DeleteBGPPeerRequest) error { p.Lock() defer p.Unlock() - p.BGPPeers.Delete(req.BGPPeer.Spec.Address) + p.BGPPeers.Delete(cmp.Or(req.PeerInterface, req.BGPPeer.Spec.Address)) return nil } diff --git a/internal/provider/cisco/iosxr/provider.go b/internal/provider/cisco/iosxr/provider.go index 7a7b733d5..353350455 100644 --- a/internal/provider/cisco/iosxr/provider.go +++ b/internal/provider/cisco/iosxr/provider.go @@ -11,6 +11,7 @@ import ( "time" "github.com/ironcore-dev/network-operator/api/core/v1alpha1" + "github.com/ironcore-dev/network-operator/internal/apistatus" "github.com/ironcore-dev/network-operator/internal/deviceutil" "github.com/ironcore-dev/network-operator/internal/provider" "github.com/ironcore-dev/network-operator/internal/transport/gnmiext" @@ -444,6 +445,13 @@ func (p *Provider) DeleteBGP(context.Context, *provider.DeleteBGPRequest) error } func (p *Provider) EnsureBGPPeer(ctx context.Context, req *provider.EnsureBGPPeerRequest) error { + if req.BGPPeer.Spec.InterfaceRef != nil { + return apistatus.NewUnsupportedFieldError(apistatus.FieldViolation{ + Field: "spec.interfaceRef", + Description: "iosxr provider does not support unnumbered BGP peering", + }) + } + // Ensure that the BGP instance exists and is configured on the "default" domain bgp := new(BGP) bgp.InstanceName = BGPDefaultInstance @@ -540,6 +548,11 @@ func (p *Provider) EnsureBGPPeer(ctx context.Context, req *provider.EnsureBGPPee } func (p *Provider) DeleteBGPPeer(ctx context.Context, req *provider.DeleteBGPPeerRequest) error { + // Unnumbered peers are rejected by EnsureBGPPeer, so there is nothing to delete. + if req.BGPPeer.Spec.InterfaceRef != nil { + return nil + } + // Fetch the default BGP instance id bgp := new(BGP) bgp.InstanceName = BGPDefaultInstance diff --git a/internal/provider/cisco/iosxr/provider_test.go b/internal/provider/cisco/iosxr/provider_test.go index 8d2840c8b..4bc9d1d9a 100644 --- a/internal/provider/cisco/iosxr/provider_test.go +++ b/internal/provider/cisco/iosxr/provider_test.go @@ -263,3 +263,20 @@ func Test_NewMTU(t *testing.T) { }) } } + +func Test_DeleteBGPPeer_Unnumbered(t *testing.T) { + // The mock has no functions set, so any device call panics. + p := &Provider{client: &gnmiext.ClientMock{}} + + err := p.DeleteBGPPeer(t.Context(), &provider.DeleteBGPPeerRequest{ + BGPPeer: &v1alpha1.BGPPeer{ + Spec: v1alpha1.BGPPeerSpec{ + InterfaceRef: &v1alpha1.LocalObjectReference{Name: "eth1-1"}, + }, + }, + PeerInterface: "HundredGigE0/0/0/1", + }) + if err != nil { + t.Fatalf("DeleteBGPPeer() error = %v", err) + } +} diff --git a/internal/provider/cisco/nxos/bgp.go b/internal/provider/cisco/nxos/bgp.go index 92996735e..f872eaca8 100644 --- a/internal/provider/cisco/nxos/bgp.go +++ b/internal/provider/cisco/nxos/bgp.go @@ -19,6 +19,8 @@ var ( _ gnmiext.DataElement = (*BGPDom)(nil) _ gnmiext.DataElement = (*BGPDomItems)(nil) _ gnmiext.DataElement = (*BGPPeerGroup)(nil) + _ gnmiext.DataElement = (*BGPPeerIf)(nil) + _ gnmiext.DataElement = (*BGPPeerIfOperItems)(nil) ) // ownershipMarkerPrefix is used to build per-VRF peer template names written @@ -194,22 +196,51 @@ func (af *BGPDomAfItem) SetMultipath(m *v1alpha1.BGPMultipath) error { } type BGPPeer struct { - VRFName string `json:"-"` - Addr string `json:"addr"` - AdminSt AdminSt `json:"adminSt"` - Asn string `json:"asn"` - AsnType PeerAsnType `json:"asnType"` - Name string `json:"name,omitempty"` - SrcIf string `json:"srcIf,omitempty"` - LocalAsnItems struct { - AsnPropagate AsnPropagate `json:"asnPropagate"` - LocalAsn string `json:"localAsn"` - } `json:"localasn-items,omitzero"` - AfItems struct { + VRFName string `json:"-"` + Addr string `json:"addr"` + AdminSt AdminSt `json:"adminSt"` + // Asn is empty for peers with a dynamic AS number, which is indicated by AsnType. + // The device reports it as an empty string in that case, so the zero value matches. + Asn string `json:"asn,omitempty"` + AsnType PeerAsnType `json:"asnType"` + Name string `json:"name,omitempty"` + SrcIf string `json:"srcIf,omitempty"` + LocalAsnItems BGPPeerLocalAsn `json:"localasn-items,omitzero"` + AfItems struct { + PeerAfList gnmiext.List[AddressFamily, *BGPPeerAfItem] `json:"PeerAf-list,omitzero"` + } `json:"af-items,omitzero"` +} + +// BGPPeerLocalAsn is the local AS number a peer sees instead of the AS number of the +// BGP instance, and how both AS numbers factor into the announcements towards the peer. +type BGPPeerLocalAsn struct { + AsnPropagate AsnPropagate `json:"asnPropagate"` + LocalAsn string `json:"localAsn"` +} + +// BGPPeerIf is an unnumbered (interface-based) BGP peer. The session is established over +// the IPv6 link-local address the peer advertises on the interface, so the peer has no +// address of its own and, unlike [BGPPeer], no source interface. +type BGPPeerIf struct { + VRFName string `json:"-"` + ID string `json:"id"` + AdminSt AdminSt `json:"adminSt"` + // Asn is empty for peers with a dynamic AS number, which is indicated by AsnType. + Asn string `json:"asn,omitempty"` + AsnType PeerAsnType `json:"asnType"` + Name string `json:"name,omitempty"` + LocalAsnItems BGPPeerLocalAsn `json:"localasn-items,omitzero"` + AfItems struct { PeerAfList gnmiext.List[AddressFamily, *BGPPeerAfItem] `json:"PeerAf-list,omitzero"` } `json:"af-items,omitzero"` } +func (*BGPPeerIf) IsListItem() {} + +func (p *BGPPeerIf) XPath() string { + return "System/bgp-items/inst-items/dom-items/Dom-list[name=" + p.VRFName + "]/peerif-items/PeerIf-list[id=" + p.ID + "]" +} + type AsnPropagate string const ( @@ -275,6 +306,19 @@ func (p *BGPPeerOperItems) XPath() string { return "System/bgp-items/inst-items/dom-items/Dom-list[name=" + p.VRFName + "]/peer-items/Peer-list[addr=" + p.Addr + "]/ent-items/PeerEntry-list[addr=" + p.Addr + "]" } +// BGPPeerIfOperItems holds the peer entries of an unnumbered BGP peer. The entries are +// keyed by the link-local address of the peer, which is only learned at runtime, so the +// whole container is retrieved instead of a single entry. +type BGPPeerIfOperItems struct { + VRFName string `json:"-"` + ID string `json:"-"` + PeerEntryList []*BGPPeerOperItems `json:"PeerEntry-list,omitempty"` +} + +func (p *BGPPeerIfOperItems) XPath() string { + return "System/bgp-items/inst-items/dom-items/Dom-list[name=" + p.VRFName + "]/peerif-items/PeerIf-list[id=" + p.ID + "]/ent-items" +} + type BGPPeerAfOperItems struct { AcceptedPaths uint32 `json:"acceptedPaths"` PfxSent string `json:"pfxSent"` diff --git a/internal/provider/cisco/nxos/bgp_test.go b/internal/provider/cisco/nxos/bgp_test.go index 41482dcff..3987930ce 100644 --- a/internal/provider/cisco/nxos/bgp_test.go +++ b/internal/provider/cisco/nxos/bgp_test.go @@ -42,6 +42,32 @@ func init() { }) Register("bgp_peer", bgpPeer) + // Unnumbered peer with a dynamic AS number ("remote-as external"). The device + // reports asn as an empty string in that case, so it is omitted from the payload. + bgpPeerIf := &BGPPeerIf{ + VRFName: DefaultVRFName, + ID: "eth1/1", + AdminSt: AdminStEnabled, + AsnType: PeerAsnTypeExternal, + Name: "Unnumbered peering with spine", + } + bgpPeerIf.AfItems.PeerAfList.Set(&BGPPeerAfItem{ + SendComExt: AdminStDisabled, + SendComStd: AdminStDisabled, + Type: AddressFamilyIPv4Unicast, + }) + Register("bgp_peer_if", bgpPeerIf) + + // Unnumbered peer with an explicit AS number ("remote-as 65020"). + bgpPeerIfAsn := &BGPPeerIf{ + VRFName: DefaultVRFName, + ID: "eth1/2", + AdminSt: AdminStEnabled, + Asn: "65020", + AsnType: PeerAsnTypeNone, + } + Register("bgp_peer_if_asn", bgpPeerIfAsn) + bgwPeer := &MultisitePeer{Addr: "1.1.1.1", PeerType: BorderGatewayPeerTypeFabricExternal} Register("bgw_peer", bgwPeer) diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index 451898e88..b75737935 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -792,16 +792,52 @@ func (p *Provider) EnsureBGPPeer(ctx context.Context, req *provider.EnsureBGPPee return apistatus.NewFailedPreconditionError(fmt.Sprintf("bgp peer: BGP instance %q must be configured on the device before peers can be realized: %v", bgp.Name, err)) } + adminSt := AdminStEnabled + if req.BGPPeer.Spec.AdminState == v1alpha1.AdminStateDown { + adminSt = AdminStDisabled + } + + // A peer with a dynamic AS number carries no AS number of its own. + asn, asnType := req.BGPPeer.Spec.ASNumber.String(), PeerAsnTypeNone + if req.BGPPeer.Spec.IsExternalASNumber() { + asn, asnType = "", PeerAsnTypeExternal + } + + localAsn, err := bgpPeerLocalAsn(req) + if err != nil { + return err + } + + // Unnumbered peers are identified by the interface they are reachable over instead + // of by an address, and are configured under a separate list on the device. + if req.PeerInterface != "" { + id, err := ShortName(req.PeerInterface) + if err != nil { + return fmt.Errorf("bgp peer: invalid peer interface name %q: %w", req.PeerInterface, err) + } + + pe := new(BGPPeerIf) + pe.VRFName = bgp.Name + pe.ID = id + pe.AdminSt = adminSt + pe.Asn = asn + pe.AsnType = asnType + pe.Name = req.BGPPeer.Spec.Description + pe.LocalAsnItems = localAsn + pe.AfItems.PeerAfList = bgpPeerAfItems(req) + + return p.client.Update(ctx, pe) + } + pe := new(BGPPeer) pe.VRFName = bgp.Name pe.Addr = req.BGPPeer.Spec.Address - pe.AdminSt = AdminStEnabled - if req.BGPPeer.Spec.AdminState == v1alpha1.AdminStateDown { - pe.AdminSt = AdminStDisabled - } - pe.Asn = req.BGPPeer.Spec.ASNumber.String() - pe.AsnType = PeerAsnTypeNone + pe.AdminSt = adminSt + pe.Asn = asn + pe.AsnType = asnType pe.Name = req.BGPPeer.Spec.Description + pe.LocalAsnItems = localAsn + pe.AfItems.PeerAfList = bgpPeerAfItems(req) if req.SourceInterface != "" { srcIf, err := ShortName(req.SourceInterface) @@ -811,76 +847,101 @@ func (p *Provider) EnsureBGPPeer(ctx context.Context, req *provider.EnsureBGPPee pe.SrcIf = srcIf } - if req.BGPPeer.Spec.LocalAS != nil { - if req.BGPPeer.Spec.LocalAS.ASNumber.String() == req.BGP.Spec.ASNumber.String() { - return apistatus.NewInvalidArgumentError(apistatus.FieldViolation{ - Field: "spec.localAS", - Description: "local-as cannot be configured on iBGP peers", - }) - } + return p.client.Update(ctx, pe) +} - pe.LocalAsnItems.LocalAsn = req.BGPPeer.Spec.LocalAS.ASNumber.String() +// bgpPeerLocalAsn builds the local AS configuration shared by both peer kinds. +func bgpPeerLocalAsn(req *provider.EnsureBGPPeerRequest) (items BGPPeerLocalAsn, err error) { + if req.BGPPeer.Spec.LocalAS == nil { + return items, nil + } - prependLocalAS := req.BGPPeer.Spec.LocalAS.PrependLocalAS == nil || *req.BGPPeer.Spec.LocalAS.PrependLocalAS - prependGlobalAS := req.BGPPeer.Spec.LocalAS.PrependGlobalAS == nil || *req.BGPPeer.Spec.LocalAS.PrependGlobalAS + if req.BGPPeer.Spec.LocalAS.ASNumber.String() == req.BGP.Spec.ASNumber.String() { + return items, apistatus.NewInvalidArgumentError(apistatus.FieldViolation{ + Field: "spec.localAS", + Description: "local-as cannot be configured on iBGP peers", + }) + } - switch { - case !prependLocalAS && prependGlobalAS: - pe.LocalAsnItems.AsnPropagate = AsnPropagateNoPrep - case !prependLocalAS && !prependGlobalAS: - pe.LocalAsnItems.AsnPropagate = AsnPropagateReplaceAs - case prependLocalAS && !prependGlobalAS: - return apistatus.NewInvalidArgumentError(apistatus.FieldViolation{ - Field: "spec.localAS.prependGlobalAS", - Description: "prependGlobalAS=false (replace-as mode) requires prependLocalAS=false (no-prepend on inbound)", - }) - default: - pe.LocalAsnItems.AsnPropagate = AsnPropagateNone - } + items.LocalAsn = req.BGPPeer.Spec.LocalAS.ASNumber.String() + + prependLocalAS := req.BGPPeer.Spec.LocalAS.PrependLocalAS == nil || *req.BGPPeer.Spec.LocalAS.PrependLocalAS + prependGlobalAS := req.BGPPeer.Spec.LocalAS.PrependGlobalAS == nil || *req.BGPPeer.Spec.LocalAS.PrependGlobalAS + + switch { + case !prependLocalAS && prependGlobalAS: + items.AsnPropagate = AsnPropagateNoPrep + case !prependLocalAS && !prependGlobalAS: + items.AsnPropagate = AsnPropagateReplaceAs + case prependLocalAS && !prependGlobalAS: + return items, apistatus.NewInvalidArgumentError(apistatus.FieldViolation{ + Field: "spec.localAS.prependGlobalAS", + Description: "prependGlobalAS=false (replace-as mode) requires prependLocalAS=false (no-prepend on inbound)", + }) + default: + items.AsnPropagate = AsnPropagateNone } - if req.BGPPeer.Spec.AddressFamilies != nil { - for t, af := range map[AddressFamily]*v1alpha1.BGPPeerAddressFamily{ - AddressFamilyIPv4Unicast: req.BGPPeer.Spec.AddressFamilies.Ipv4Unicast, - AddressFamilyIPv6Unicast: req.BGPPeer.Spec.AddressFamilies.Ipv6Unicast, - AddressFamilyL2EVPN: req.BGPPeer.Spec.AddressFamilies.L2vpnEvpn, - } { - if af == nil || !af.Enabled { - continue - } - item := new(BGPPeerAfItem) - item.Type = t - item.SendComStd = AdminStDisabled - if af.SendCommunity == v1alpha1.BGPCommunityTypeStandard || af.SendCommunity == v1alpha1.BGPCommunityTypeBoth { - item.SendComStd = AdminStEnabled - } - item.SendComExt = AdminStDisabled - if af.SendCommunity == v1alpha1.BGPCommunityTypeExtended || af.SendCommunity == v1alpha1.BGPCommunityTypeBoth { - item.SendComExt = AdminStEnabled - } - if af.RouteReflectorClient { - item.Ctrl = NewOption(RouteReflectorClient) - } - afType := t.ToAddressFamilyType() - if name, ok := req.InboundRoutingPolicies[afType]; ok { - item.RtCtrlPItems.RtCtrlPList.Set(&BGPPeerAfRtCtrlP{Direction: RtCtrlDirectionIn, RtMap: name}) - } - if name, ok := req.OutboundRoutingPolicies[afType]; ok { - item.RtCtrlPItems.RtCtrlPList.Set(&BGPPeerAfRtCtrlP{Direction: RtCtrlDirectionOut, RtMap: name}) - } - pe.AfItems.PeerAfList.Set(item) + return items, nil +} + +// bgpPeerAfItems builds the per-address-family configuration shared by both peer kinds. +func bgpPeerAfItems(req *provider.EnsureBGPPeerRequest) gnmiext.List[AddressFamily, *BGPPeerAfItem] { + var list gnmiext.List[AddressFamily, *BGPPeerAfItem] + if req.BGPPeer.Spec.AddressFamilies == nil { + return list + } + + for t, af := range map[AddressFamily]*v1alpha1.BGPPeerAddressFamily{ + AddressFamilyIPv4Unicast: req.BGPPeer.Spec.AddressFamilies.Ipv4Unicast, + AddressFamilyIPv6Unicast: req.BGPPeer.Spec.AddressFamilies.Ipv6Unicast, + AddressFamilyL2EVPN: req.BGPPeer.Spec.AddressFamilies.L2vpnEvpn, + } { + if af == nil || !af.Enabled { + continue + } + item := new(BGPPeerAfItem) + item.Type = t + item.SendComStd = AdminStDisabled + if af.SendCommunity == v1alpha1.BGPCommunityTypeStandard || af.SendCommunity == v1alpha1.BGPCommunityTypeBoth { + item.SendComStd = AdminStEnabled + } + item.SendComExt = AdminStDisabled + if af.SendCommunity == v1alpha1.BGPCommunityTypeExtended || af.SendCommunity == v1alpha1.BGPCommunityTypeBoth { + item.SendComExt = AdminStEnabled + } + if af.RouteReflectorClient { + item.Ctrl = NewOption(RouteReflectorClient) } + afType := t.ToAddressFamilyType() + if name, ok := req.InboundRoutingPolicies[afType]; ok { + item.RtCtrlPItems.RtCtrlPList.Set(&BGPPeerAfRtCtrlP{Direction: RtCtrlDirectionIn, RtMap: name}) + } + if name, ok := req.OutboundRoutingPolicies[afType]; ok { + item.RtCtrlPItems.RtCtrlPList.Set(&BGPPeerAfRtCtrlP{Direction: RtCtrlDirectionOut, RtMap: name}) + } + list.Set(item) } - return p.client.Update(ctx, pe) + return list } func (p *Provider) DeleteBGPPeer(ctx context.Context, req *provider.DeleteBGPPeerRequest) error { - b := new(BGPPeer) - b.VRFName = DefaultVRFName + vrfName := DefaultVRFName if req.VRF != nil { - b.VRFName = req.VRF.Spec.Name + vrfName = req.VRF.Spec.Name } + + if req.PeerInterface != "" { + id, err := ShortName(req.PeerInterface) + if err != nil { + return fmt.Errorf("bgp peer: invalid peer interface name %q: %w", req.PeerInterface, err) + } + return p.client.Delete(ctx, &BGPPeerIf{VRFName: vrfName, ID: id}) + } + + b := new(BGPPeer) + b.VRFName = vrfName b.Addr = req.BGPPeer.Spec.Address return p.client.Delete(ctx, b) } @@ -892,7 +953,23 @@ func (p *Provider) GetPeerStatus(ctx context.Context, req *provider.BGPPeerStatu ps.VRFName = req.VRF.Spec.Name } ps.Addr = req.BGPPeer.Spec.Address - if err := p.client.GetState(ctx, ps); err != nil && !errors.Is(err, gnmiext.ErrNil) { + + // An unnumbered peer has no address of its own: its entry is keyed by the link-local + // address learned at runtime, so the whole entry container is retrieved instead. + if req.PeerInterface != "" { + id, err := ShortName(req.PeerInterface) + if err != nil { + return provider.BGPPeerStatus{}, fmt.Errorf("bgp peer status: invalid peer interface name %q: %w", req.PeerInterface, err) + } + ents := &BGPPeerIfOperItems{VRFName: ps.VRFName, ID: id} + if err := p.client.GetState(ctx, ents); err != nil && !errors.Is(err, gnmiext.ErrNil) { + return provider.BGPPeerStatus{}, err + } + if len(ents.PeerEntryList) == 0 { + return provider.BGPPeerStatus{SessionState: v1alpha1.BGPPeerSessionStateIdle}, nil + } + ps = ents.PeerEntryList[0] + } else if err := p.client.GetState(ctx, ps); err != nil && !errors.Is(err, gnmiext.ErrNil) { return provider.BGPPeerStatus{}, err } @@ -1659,12 +1736,16 @@ func (p *Provider) EnsureInterface(ctx context.Context, req *provider.EnsureInte sb.Patch(stp) } - // Add the address items last, as they depend on the interface being created first. + // The address items are replaced rather than merged, so that addresses removed + // from the spec are also removed from the device. They depend on the interface + // being created and routed first, but a gNMI Set processes replace operations + // before update operations, so they are sent in a separate, later Set. + ab := new(gnmiext.SetBuilder).Limit(maxSetOperations) if addr != nil { - sb.Patch(addr) + ab.Update(addr) } if ipv6Addr != nil { - sb.Patch(ipv6Addr) + ab.Update(ipv6Addr) } switch { @@ -1736,7 +1817,10 @@ func (p *Provider) EnsureInterface(ctx context.Context, req *provider.EnsureInte } } - return p.Do(ctx, sb) + if err := p.Do(ctx, sb); err != nil { + return err + } + return p.Do(ctx, ab) } func (p *Provider) DeleteInterface(ctx context.Context, req *provider.InterfaceRequest) error { diff --git a/internal/provider/cisco/nxos/testdata/bgp_peer_if.json b/internal/provider/cisco/nxos/testdata/bgp_peer_if.json new file mode 100644 index 000000000..76e50f92c --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/bgp_peer_if.json @@ -0,0 +1,33 @@ +{ + "bgp-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "peerif-items": { + "PeerIf-list": [ + { + "id": "eth1/1", + "adminSt": "enabled", + "asnType": "external", + "name": "Unnumbered peering with spine", + "af-items": { + "PeerAf-list": [ + { + "ctrl": "DME_UNSET_PROPERTY_MARKER", + "sendComExt": "disabled", + "sendComStd": "disabled", + "type": "ipv4-ucast" + } + ] + } + } + ] + } + } + ] + } + } + } +} diff --git a/internal/provider/cisco/nxos/testdata/bgp_peer_if.json.txt b/internal/provider/cisco/nxos/testdata/bgp_peer_if.json.txt new file mode 100644 index 000000000..09883f63e --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/bgp_peer_if.json.txt @@ -0,0 +1,5 @@ +router bgp 65000 + neighbor Ethernet1/1 + description Unnumbered peering with spine + remote-as external + address-family ipv4 unicast diff --git a/internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json b/internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json new file mode 100644 index 000000000..fcba03ef7 --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json @@ -0,0 +1,23 @@ +{ + "bgp-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "peerif-items": { + "PeerIf-list": [ + { + "id": "eth1/2", + "adminSt": "enabled", + "asn": "65020", + "asnType": "none" + } + ] + } + } + ] + } + } + } +} diff --git a/internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json.txt b/internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json.txt new file mode 100644 index 000000000..8d14de31f --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json.txt @@ -0,0 +1,3 @@ +router bgp 65000 + neighbor Ethernet1/2 + remote-as 65020 diff --git a/internal/provider/openconfig/bgp_test.go b/internal/provider/openconfig/bgp_test.go index 6b5e7bb74..7e85e7c45 100644 --- a/internal/provider/openconfig/bgp_test.go +++ b/internal/provider/openconfig/bgp_test.go @@ -7,6 +7,10 @@ import ( "testing" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/ironcore-dev/network-operator/api/core/v1alpha1" + "github.com/ironcore-dev/network-operator/internal/provider" + "github.com/ironcore-dev/network-operator/internal/transport/gnmiext" ) func TestAsnToUint32(t *testing.T) { @@ -32,3 +36,20 @@ func TestAsnToUint32(t *testing.T) { }) } } + +func TestDeleteBGPPeer_Unnumbered(t *testing.T) { + // The mock has no functions set, so any device call panics. + p := newProviderWithClient(&gnmiext.ClientMock{}) + + err := p.DeleteBGPPeer(t.Context(), &provider.DeleteBGPPeerRequest{ + BGPPeer: &v1alpha1.BGPPeer{ + Spec: v1alpha1.BGPPeerSpec{ + InterfaceRef: &v1alpha1.LocalObjectReference{Name: "eth1-1"}, + }, + }, + PeerInterface: "ethernet-1/1", + }) + if err != nil { + t.Fatalf("DeleteBGPPeer() error = %v", err) + } +} diff --git a/internal/provider/openconfig/bgppeer.go b/internal/provider/openconfig/bgppeer.go index 79505dbd7..78ece95fb 100644 --- a/internal/provider/openconfig/bgppeer.go +++ b/internal/provider/openconfig/bgppeer.go @@ -24,6 +24,13 @@ const bgpPeerGroupName = "NETOP-DEFAULT" func (p *Provider) EnsureBGPPeer(ctx context.Context, req *provider.EnsureBGPPeerRequest) error { spec := req.BGPPeer.Spec + if spec.InterfaceRef != nil { + return apistatus.NewUnsupportedFieldError(apistatus.FieldViolation{ + Field: "spec.interfaceRef", + Description: "openconfig provider does not support unnumbered BGP peering on SRLinux", + }) + } + peerAS, err := asnToUint32(spec.ASNumber) if err != nil { return err @@ -143,6 +150,11 @@ func (p *Provider) EnsureBGPPeer(ctx context.Context, req *provider.EnsureBGPPee } func (p *Provider) DeleteBGPPeer(ctx context.Context, req *provider.DeleteBGPPeerRequest) error { + // Unnumbered peers are rejected by EnsureBGPPeer, so there is nothing to delete. + if req.BGPPeer.Spec.InterfaceRef != nil { + return nil + } + ni := DefaultNetworkInstance if req.VRF != nil { ni = req.VRF.Spec.Name diff --git a/internal/provider/provider.go b/internal/provider/provider.go index 64a11b3cd..db7033791 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -530,6 +530,9 @@ type EnsureBGPPeerRequest struct { BGPPeer *v1alpha1.BGPPeer ProviderConfig *ProviderConfig SourceInterface string + // PeerInterface is the device-level name of the interface an unnumbered + // (interface-based) peer is reachable over. Empty for peers with an address. + PeerInterface string // BGP is the resolved BGP instance referenced by BGPPeer.Spec.BgpRef. BGP *v1alpha1.BGP // VRF is the resolved VRF referenced by BGP.Spec.VrfRef. @@ -546,6 +549,9 @@ type EnsureBGPPeerRequest struct { type DeleteBGPPeerRequest struct { BGPPeer *v1alpha1.BGPPeer ProviderConfig *ProviderConfig + // PeerInterface is the device-level name of the interface an unnumbered + // (interface-based) peer is reachable over. Empty for peers with an address. + PeerInterface string // BGP is the resolved BGP instance referenced by BGPPeer.Spec.BgpRef. BGP *v1alpha1.BGP // VRF is the resolved VRF referenced by BGP.Spec.VrfRef. @@ -556,6 +562,9 @@ type DeleteBGPPeerRequest struct { type BGPPeerStatusRequest struct { BGPPeer *v1alpha1.BGPPeer ProviderConfig *ProviderConfig + // PeerInterface is the device-level name of the interface an unnumbered + // (interface-based) peer is reachable over. Empty for peers with an address. + PeerInterface string // VRF is the resolved VRF referenced by the BGP instance of this peer. // When nil, the provider shall use the default VRF. VRF *v1alpha1.VRF diff --git a/internal/webhook/core/v1alpha1/bgppeer_webhook.go b/internal/webhook/core/v1alpha1/bgppeer_webhook.go index 93576b747..09ed071df 100644 --- a/internal/webhook/core/v1alpha1/bgppeer_webhook.go +++ b/internal/webhook/core/v1alpha1/bgppeer_webhook.go @@ -5,6 +5,8 @@ package v1alpha1 import ( "context" + "errors" + "fmt" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -49,7 +51,21 @@ func (v *BGPPeerCustomValidator) ValidateDelete(_ context.Context, _ *v1alpha1.B } func validateBGPPeer(bgppeer v1alpha1.BGPPeerSpec) error { - if err := validateASNumber(bgppeer.ASNumber); err != nil { + if (bgppeer.Address == "") == (bgppeer.InterfaceRef == nil) { + return errors.New("exactly one of address or interfaceRef must be specified") + } + + if bgppeer.InterfaceRef != nil && bgppeer.LocalAddress != nil { + return errors.New("localAddress must not be specified for interface-based peers") + } + + // A peer with a dynamic AS number accepts any AS number that differs from the local + // one, which is only meaningful for unnumbered, interface-based peers. + if bgppeer.IsExternalASNumber() { + if bgppeer.InterfaceRef == nil { + return fmt.Errorf("AS number %q requires interfaceRef", v1alpha1.BGPPeerASNumberExternal) + } + } else if err := validateASNumber(bgppeer.ASNumber); err != nil { return err } diff --git a/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go b/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go index 8240eb37a..9a01057b8 100644 --- a/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go +++ b/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go @@ -36,6 +36,61 @@ var _ = Describe("BGPPeer Webhook", func() { Expect(obj).NotTo(BeNil(), "Expected obj to be initialized") }) + Context("When creating an unnumbered BGPPeer", func() { + It("Should admit an interface-based peer with a dynamic AS number", func() { + obj.Spec.Address = "" + obj.Spec.InterfaceRef = &v1alpha1.LocalObjectReference{Name: "eth1-1"} + obj.Spec.ASNumber = intstr.FromString(v1alpha1.BGPPeerASNumberExternal) + + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("Should admit an interface-based peer with an explicit AS number", func() { + obj.Spec.Address = "" + obj.Spec.InterfaceRef = &v1alpha1.LocalObjectReference{Name: "eth1-1"} + obj.Spec.ASNumber = intstr.FromInt32(65020) + + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("Should deny both address and interfaceRef", func() { + obj.Spec.InterfaceRef = &v1alpha1.LocalObjectReference{Name: "eth1-1"} + obj.Spec.ASNumber = intstr.FromInt32(65001) + + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(MatchError(ContainSubstring("exactly one of address or interfaceRef"))) + }) + + It("Should deny neither address nor interfaceRef", func() { + obj.Spec.Address = "" + obj.Spec.ASNumber = intstr.FromInt32(65001) + + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(MatchError(ContainSubstring("exactly one of address or interfaceRef"))) + }) + + It("Should deny localAddress on an interface-based peer", func() { + obj.Spec.Address = "" + obj.Spec.InterfaceRef = &v1alpha1.LocalObjectReference{Name: "eth1-1"} + obj.Spec.ASNumber = intstr.FromString(v1alpha1.BGPPeerASNumberExternal) + obj.Spec.LocalAddress = &v1alpha1.BGPPeerLocalAddress{ + InterfaceRef: v1alpha1.LocalObjectReference{Name: "lo0"}, + } + + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(MatchError(ContainSubstring("localAddress must not be specified"))) + }) + + It("Should deny a dynamic AS number without interfaceRef", func() { + obj.Spec.ASNumber = intstr.FromString(v1alpha1.BGPPeerASNumberExternal) + + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(MatchError(ContainSubstring("requires interfaceRef"))) + }) + }) + Context("When creating BGPPeer under Validating Webhook", func() { It("Should admit creation with valid integer AS number", func() { obj.Spec.ASNumber = intstr.FromInt32(65001) diff --git a/test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/bgp_peer_unnumbered.txtar b/test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/bgp_peer_unnumbered.txtar new file mode 100644 index 000000000..4cf5314f9 --- /dev/null +++ b/test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/bgp_peer_unnumbered.txtar @@ -0,0 +1,206 @@ +-- interfaces/uplink -- +apiVersion: networking.metal.ironcore.dev/v1alpha1 +kind: Interface +metadata: + name: uplink + namespace: default +spec: + deviceRef: + name: device + name: eth1/1 + adminState: Up + type: Physical + ipv6: + useLinkLocalOnly: true + +-- bgps/fabric-bgp -- +apiVersion: networking.metal.ironcore.dev/v1alpha1 +kind: BGP +metadata: + name: fabric-bgp + namespace: default +spec: + deviceRef: + name: device + asNumber: 65010 + routerId: "10.0.0.1" + +-- bgppeers/spine1 -- +apiVersion: networking.metal.ironcore.dev/v1alpha1 +kind: BGPPeer +metadata: + name: spine1 + namespace: default +spec: + deviceRef: + name: device + bgpRef: + name: fabric-bgp + interfaceRef: + name: uplink + asNumber: external + description: "Unnumbered peering with spine1" + addressFamilies: + ipv4Unicast: + enabled: true + +-- state/preload -- +{ + "System": { + "procsys-items": { + "bootTime": "1700000000" + } + } +} + +-- state/expect -- + +{ + "System": { + "procsys-items": { + "bootTime": "1700000000" + }, + "intf-items": { + "phys-items": { + "PhysIf-list": [ + { + "accessVlan": "unknown", + "adminSt": "up", + "descr": "DME_UNSET_PROPERTY_MARKER", + "FECMode": "auto", + "id": "eth1/1", + "layer": "Layer3", + "mtu": 1500, + "medium": "broadcast", + "mode": "access", + "nativeVlan": "unknown", + "userCfgdFlags": "admin_layer,admin_state", + "rtvrfMbr-items": { + "tDn": "/System/inst-items/Inst-list[name='default']" + }, + "physExtd-items": { + "bufferBoost": "enable" + } + } + ] + } + }, + "ipv6-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "useLinkLocalAddr": "enabled" + } + ] + } + } + ] + } + } + }, + "fm-items": { + "bgp-items": { + "adminSt": "enabled" + }, + "evpn-items": { + "adminSt": "enabled" + } + }, + "bgp-items": { + "inst-items": { + "adminSt": "enabled", + "asn": "65010", + "dom-items": { + "Dom-list": [ + { + "name": "default", + "rtrId": "10.0.0.1", + "rtrIdAuto": "disabled", + "peerif-items": { + "PeerIf-list": [ + { + "id": "eth1/1", + "adminSt": "enabled", + "asnType": "external", + "name": "Unnumbered peering with spine1", + "af-items": { + "PeerAf-list": [ + { + "ctrl": "DME_UNSET_PROPERTY_MARKER", + "sendComExt": "disabled", + "sendComStd": "disabled", + "type": "ipv4-ucast" + } + ] + } + } + ] + } + } + ] + } + } + } + } +} + +-- state/delete -- + +{ + "System": { + "procsys-items": { + "bootTime": "1700000000" + }, + "intf-items": { + "phys-items": { + "PhysIf-list": [ + { + "accessVlan": "vlan-1", + "descr": "DME_UNSET_PROPERTY_MARKER", + "FECMode": "auto", + "id": "eth1/1", + "layer": "Layer2", + "mtu": 1500, + "medium": "broadcast", + "mode": "access", + "nativeVlan": "vlan-1", + "userCfgdFlags": "", + "physExtd-items": { + "bufferBoost": "enable" + }, + "trunkVlans": "1-4094" + } + ] + } + }, + "ipv6-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [] + } + } + ] + } + } + }, + "fm-items": { + "bgp-items": { + "adminSt": "enabled" + }, + "evpn-items": { + "adminSt": "enabled" + } + }, + "bgp-items": {} + } +}