From fea40a33e671c9f3fea17c9c636a28f056ad9dbf Mon Sep 17 00:00:00 2001 From: Oliver Frommel Date: Thu, 17 Sep 2026 14:33:41 +0200 Subject: [PATCH 1/3] Support unnumbered interface-based BGP peering Allow a BGPPeer to name an Interface instead of a peer address, so an eBGP session can run over the peers' IPv6 link-local addresses and the transit link needs no addressing of its own. The referenced Interface must set spec.ipv6.useLinkLocalOnly. spec.address becomes optional and is mutually exclusive with the new spec.interfaceRef; spec.localAddress is meaningless for such a peer. Both rules are enforced by CEL and by the webhook. A peer's identity, its address, interfaceRef or bgpRef, is immutable, as the finalizer only knows the current identity and would leave the previous peer behind on the device. spec.asNumber accepts the sentinel "external" for dynamic AS discovery, which only applies to interface-based peers. On NX-OS the peer maps to a PeerIf object under peerif-items, keyed by the interface name. The device reports an empty asn with asnType external, which the omitempty payload matches, so reconciliation stays idempotent. The device-level interface name is recorded in status.peerInterface, so the finalizer can still remove the peer after its Interface was deleted. The openconfig and iosxr providers reject interfaceRef as unsupported and skip the deletion of such peers as they were never configured on the device. The BGPPeer controller now watches Interfaces through a field index covering both interface references, so a peer converges as soon as its Interface appears instead of waiting for the periodic requeue. The NX-OS interface address items are replaced instead of merged, so addresses removed from the spec, such as global addresses when switching to link-local only, are removed from the device. As a gNMI Set applies replace before update operations, they are sent in a separate Set after the interface itself has been created. Signed-off-by: Oliver Frommel --- Tiltfile | 1 + api/core/v1alpha1/bgp_peer_types.go | 42 +++- api/core/v1alpha1/zz_generated.deepcopy.go | 5 + ...gppeers.networking.metal.ironcore.dev.yaml | 56 ++++- ...etworking.metal.ironcore.dev_bgppeers.yaml | 56 ++++- config/samples/v1alpha1_bgppeer.yaml | 22 ++ docs/api-reference/index.md | 8 +- hack/provider/main.go | 36 ++- .../controller/core/bgp_peer_controller.go | 149 ++++++++++-- .../core/bgp_peer_controller_test.go | 159 +++++++++++++ internal/controller/core/suite_test.go | 5 +- internal/provider/cisco/iosxr/provider.go | 13 ++ .../provider/cisco/iosxr/provider_test.go | 17 ++ internal/provider/cisco/nxos/bgp.go | 68 +++++- internal/provider/cisco/nxos/bgp_test.go | 26 +++ internal/provider/cisco/nxos/provider.go | 220 ++++++++++++------ .../cisco/nxos/testdata/bgp_peer_if.json | 33 +++ .../cisco/nxos/testdata/bgp_peer_if.json.txt | 5 + .../cisco/nxos/testdata/bgp_peer_if_asn.json | 23 ++ .../nxos/testdata/bgp_peer_if_asn.json.txt | 3 + internal/provider/openconfig/bgp_test.go | 21 ++ internal/provider/openconfig/bgppeer.go | 12 + internal/provider/provider.go | 9 + .../webhook/core/v1alpha1/bgppeer_webhook.go | 18 +- .../core/v1alpha1/bgppeer_webhook_test.go | 55 +++++ .../bgp_peer_unnumbered.txtar | 206 ++++++++++++++++ 26 files changed, 1154 insertions(+), 114 deletions(-) create mode 100644 internal/provider/cisco/nxos/testdata/bgp_peer_if.json create mode 100644 internal/provider/cisco/nxos/testdata/bgp_peer_if.json.txt create mode 100644 internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json create mode 100644 internal/provider/cisco/nxos/testdata/bgp_peer_if_asn.json.txt create mode 100644 test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/bgp_peer_unnumbered.txtar 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": {} + } +} From 78c94363061fe8fd825d29e60ecf2f07cdef3ef0 Mon Sep 17 00:00:00 2001 From: Oliver Frommel Date: Thu, 17 Sep 2026 17:48:20 +0200 Subject: [PATCH 2/3] feat: set suppress-ra an ra-interval --- Tiltfile | 1 + .../nx/v1alpha1/interfaceconfig_types.go | 32 +++ .../nx/v1alpha1/zz_generated.deepcopy.go | 25 +++ ...x.cisco.networking.metal.ironcore.dev.yaml | 31 +++ ...g.metal.ironcore.dev_interfaceconfigs.yaml | 31 +++ .../cisco/nx/v1alpha1_interfaceconfig.yaml | 15 ++ config/samples/v1alpha1_interface.yaml | 6 + docs/api-reference/index.md | 18 ++ hack/provider/main.go | 13 +- internal/provider/cisco/nxos/intf.go | 62 ++++++ internal/provider/cisco/nxos/intf_test.go | 54 +++++ internal/provider/cisco/nxos/provider.go | 28 +++ .../nxos/testdata/nd_if_ra_interval.json | 23 +++ .../nxos/testdata/nd_if_ra_interval.json.txt | 3 + .../nxos/testdata/nd_if_suppress_ra.json | 23 +++ .../nxos/testdata/nd_if_suppress_ra.json.txt | 3 + .../bgp_peer_unnumbered.txtar | 58 ++++++ .../interface_physical_ipv6_nd.txtar | 193 ++++++++++++++++++ 18 files changed, 618 insertions(+), 1 deletion(-) create mode 100644 internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json create mode 100644 internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json.txt create mode 100644 internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json create mode 100644 internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json.txt create mode 100644 test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/interface_physical_ipv6_nd.txtar diff --git a/Tiltfile b/Tiltfile index 7ded83347..09537d511 100644 --- a/Tiltfile +++ b/Tiltfile @@ -159,6 +159,7 @@ k8s_resource(new_name='vpcdomain', objects=['leaf1-vpcdomain:vpcdomain', 'leaf1- k8s_yaml('./config/samples/cisco/nx/v1alpha1_interfaceconfig.yaml') k8s_resource(new_name='spanning-tree-network', objects=['spanning-tree-network:interfaceconfig'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) k8s_resource(new_name='lacp-vpc', objects=['lacp-vpc:interfaceconfig'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) +k8s_resource(new_name='unnumbered-uplink', objects=['unnumbered-uplink:interfaceconfig'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples']) k8s_yaml('./config/samples/v1alpha1_nve.yaml') k8s_resource(new_name='nve1', objects=['nve1:networkvirtualizationedge'], trigger_mode=TRIGGER_MODE_MANUAL, resource_deps=['lo0', 'lo1'], auto_init=False, labels=['samples']) diff --git a/api/cisco/nx/v1alpha1/interfaceconfig_types.go b/api/cisco/nx/v1alpha1/interfaceconfig_types.go index 9fdb6f360..56740ad34 100644 --- a/api/cisco/nx/v1alpha1/interfaceconfig_types.go +++ b/api/cisco/nx/v1alpha1/interfaceconfig_types.go @@ -30,6 +30,38 @@ type InterfaceConfigSpec struct { // EVPNMultihoming defines EVPN ESI multihoming settings for the interface. // +optional EVPNMultihoming *EVPNMultihoming `json:"evpnMultihoming,omitempty"` + + // IPv6 defines IPv6 settings for the interface that have no equivalent in + // the core Interface API. Neighbor Discovery settings are only managed while + // this is set: other ND settings on the device are left untouched, and + // removing it leaves the last applied settings on the device. + // +optional + IPv6 *InterfaceConfigIPv6 `json:"ipv6,omitempty"` +} + +// InterfaceConfigIPv6 defines IPv6 settings for an interface. +type InterfaceConfigIPv6 struct { + // SuppressRouterAdvertisement stops the interface from sending IPv6 Router + // Advertisements. NX-OS sends them by default. Neighbours that discover + // each other over their link-local addresses, such as unnumbered BGP peers, + // depend on them, so this must stay disabled for such interfaces. + // Only applied to interfaces that carry IPv6 configuration. + // Maps to CLI command: ipv6 nd suppress-ra + // +required + SuppressRouterAdvertisement bool `json:"suppressRouterAdvertisement"` + + // RouterAdvertisementInterval is the maximum interval between periodic + // IPv6 Router Advertisements, between 4s and 30m in whole seconds. The + // minimum interval is derived from it the way NX-OS does for the CLI + // command: a third of this value, but no less than 3s. + // Unnumbered BGP peers only discover each other once an advertisement is + // received, so a short interval speeds up session establishment. + // If not specified, the NX-OS default of 600s applies. + // Maps to CLI command: ipv6 nd ra-interval + // +optional + // +kubebuilder:validation:Type=string + // +kubebuilder:validation:Pattern="^([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$" + RouterAdvertisementInterval *metav1.Duration `json:"routerAdvertisementInterval,omitempty"` } // SpanningTree defines the spanning tree configuration for an interface. diff --git a/api/cisco/nx/v1alpha1/zz_generated.deepcopy.go b/api/cisco/nx/v1alpha1/zz_generated.deepcopy.go index 1475968f6..2b0634717 100644 --- a/api/cisco/nx/v1alpha1/zz_generated.deepcopy.go +++ b/api/cisco/nx/v1alpha1/zz_generated.deepcopy.go @@ -507,6 +507,26 @@ func (in *InterfaceConfig) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *InterfaceConfigIPv6) DeepCopyInto(out *InterfaceConfigIPv6) { + *out = *in + if in.RouterAdvertisementInterval != nil { + in, out := &in.RouterAdvertisementInterval, &out.RouterAdvertisementInterval + *out = new(v1.Duration) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InterfaceConfigIPv6. +func (in *InterfaceConfigIPv6) DeepCopy() *InterfaceConfigIPv6 { + if in == nil { + return nil + } + out := new(InterfaceConfigIPv6) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *InterfaceConfigLACP) DeepCopyInto(out *InterfaceConfigLACP) { *out = *in @@ -587,6 +607,11 @@ func (in *InterfaceConfigSpec) DeepCopyInto(out *InterfaceConfigSpec) { *out = new(EVPNMultihoming) **out = **in } + if in.IPv6 != nil { + in, out := &in.IPv6, &out.IPv6 + *out = new(InterfaceConfigIPv6) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InterfaceConfigSpec. diff --git a/charts/network-operator/templates/crd/interfaceconfigs.nx.cisco.networking.metal.ironcore.dev.yaml b/charts/network-operator/templates/crd/interfaceconfigs.nx.cisco.networking.metal.ironcore.dev.yaml index e90717721..14c64d537 100644 --- a/charts/network-operator/templates/crd/interfaceconfigs.nx.cisco.networking.metal.ironcore.dev.yaml +++ b/charts/network-operator/templates/crd/interfaceconfigs.nx.cisco.networking.metal.ironcore.dev.yaml @@ -73,6 +73,37 @@ spec: required: - coreTracking type: object + ipv6: + description: |- + IPv6 defines IPv6 settings for the interface that have no equivalent in + the core Interface API. Neighbor Discovery settings are only managed while + this is set: other ND settings on the device are left untouched, and + removing it leaves the last applied settings on the device. + properties: + routerAdvertisementInterval: + description: |- + RouterAdvertisementInterval is the maximum interval between periodic + IPv6 Router Advertisements, between 4s and 30m in whole seconds. The + minimum interval is derived from it the way NX-OS does for the CLI + command: a third of this value, but no less than 3s. + Unnumbered BGP peers only discover each other once an advertisement is + received, so a short interval speeds up session establishment. + If not specified, the NX-OS default of 600s applies. + Maps to CLI command: ipv6 nd ra-interval + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + suppressRouterAdvertisement: + description: |- + SuppressRouterAdvertisement stops the interface from sending IPv6 Router + Advertisements. NX-OS sends them by default. Neighbours that discover + each other over their link-local addresses, such as unnumbered BGP peers, + depend on them, so this must stay disabled for such interfaces. + Only applied to interfaces that carry IPv6 configuration. + Maps to CLI command: ipv6 nd suppress-ra + type: boolean + required: + - suppressRouterAdvertisement + type: object lacp: description: LACP defines LACP options for PortChannel (Aggregate) interfaces. diff --git a/config/crd/bases/nx.cisco.networking.metal.ironcore.dev_interfaceconfigs.yaml b/config/crd/bases/nx.cisco.networking.metal.ironcore.dev_interfaceconfigs.yaml index a9d14bd2a..127cd84d2 100644 --- a/config/crd/bases/nx.cisco.networking.metal.ironcore.dev_interfaceconfigs.yaml +++ b/config/crd/bases/nx.cisco.networking.metal.ironcore.dev_interfaceconfigs.yaml @@ -70,6 +70,37 @@ spec: required: - coreTracking type: object + ipv6: + description: |- + IPv6 defines IPv6 settings for the interface that have no equivalent in + the core Interface API. Neighbor Discovery settings are only managed while + this is set: other ND settings on the device are left untouched, and + removing it leaves the last applied settings on the device. + properties: + routerAdvertisementInterval: + description: |- + RouterAdvertisementInterval is the maximum interval between periodic + IPv6 Router Advertisements, between 4s and 30m in whole seconds. The + minimum interval is derived from it the way NX-OS does for the CLI + command: a third of this value, but no less than 3s. + Unnumbered BGP peers only discover each other once an advertisement is + received, so a short interval speeds up session establishment. + If not specified, the NX-OS default of 600s applies. + Maps to CLI command: ipv6 nd ra-interval + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + suppressRouterAdvertisement: + description: |- + SuppressRouterAdvertisement stops the interface from sending IPv6 Router + Advertisements. NX-OS sends them by default. Neighbours that discover + each other over their link-local addresses, such as unnumbered BGP peers, + depend on them, so this must stay disabled for such interfaces. + Only applied to interfaces that carry IPv6 configuration. + Maps to CLI command: ipv6 nd suppress-ra + type: boolean + required: + - suppressRouterAdvertisement + type: object lacp: description: LACP defines LACP options for PortChannel (Aggregate) interfaces. diff --git a/config/samples/cisco/nx/v1alpha1_interfaceconfig.yaml b/config/samples/cisco/nx/v1alpha1_interfaceconfig.yaml index a47a3a82a..cb358f0e5 100644 --- a/config/samples/cisco/nx/v1alpha1_interfaceconfig.yaml +++ b/config/samples/cisco/nx/v1alpha1_interfaceconfig.yaml @@ -31,3 +31,18 @@ metadata: spec: evpnMultihoming: coreTracking: true +--- +apiVersion: nx.cisco.networking.metal.ironcore.dev/v1alpha1 +kind: InterfaceConfig +metadata: + labels: + app.kubernetes.io/name: network-operator + app.kubernetes.io/managed-by: kustomize + name: unnumbered-uplink +spec: + # Peers discover each other through Router Advertisements sent to their + # link-local addresses, so advertisements must not be suppressed. A short + # interval lets the session come up within seconds rather than minutes. + ipv6: + suppressRouterAdvertisement: false + routerAdvertisementInterval: 4s diff --git a/config/samples/v1alpha1_interface.yaml b/config/samples/v1alpha1_interface.yaml index e6fb8bf26..a5494084c 100644 --- a/config/samples/v1alpha1_interface.yaml +++ b/config/samples/v1alpha1_interface.yaml @@ -276,6 +276,12 @@ spec: adminState: Up type: Physical mtu: 9216 + # Router Advertisement settings have no equivalent in the core API, so they + # come from the NX-OS specific InterfaceConfig resource. + providerConfigRef: + apiVersion: nx.cisco.networking.metal.ironcore.dev/v1alpha1 + kind: InterfaceConfig + name: unnumbered-uplink # Use only the automatically generated IPv6 link-local address, so the link # needs no addressing of its own. Mutually exclusive with ipv6.addresses. ipv6: diff --git a/docs/api-reference/index.md b/docs/api-reference/index.md index 04d6622a5..d976226b2 100644 --- a/docs/api-reference/index.md +++ b/docs/api-reference/index.md @@ -4933,6 +4933,23 @@ InterfaceConfig is the Schema for the interfaceconfigs API | `spec` _[InterfaceConfigSpec](#interfaceconfigspec)_ | Specification of the desired state of the resource.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status | | Required: \{\}
| +#### InterfaceConfigIPv6 + + + +InterfaceConfigIPv6 defines IPv6 settings for an interface. + + + +_Appears in:_ +- [InterfaceConfigSpec](#interfaceconfigspec) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `suppressRouterAdvertisement` _boolean_ | SuppressRouterAdvertisement stops the interface from sending IPv6 Router
Advertisements. NX-OS sends them by default. Neighbours that discover
each other over their link-local addresses, such as unnumbered BGP peers,
depend on them, so this must stay disabled for such interfaces.
Only applied to interfaces that carry IPv6 configuration.
Maps to CLI command: ipv6 nd suppress-ra | | Required: \{\}
| +| `routerAdvertisementInterval` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#duration-v1-meta)_ | RouterAdvertisementInterval is the maximum interval between periodic
IPv6 Router Advertisements, between 4s and 30m in whole seconds. The
minimum interval is derived from it the way NX-OS does for the CLI
command: a third of this value, but no less than 3s.
Unnumbered BGP peers only discover each other once an advertisement is
received, so a short interval speeds up session establishment.
If not specified, the NX-OS default of 600s applies.
Maps to CLI command: ipv6 nd ra-interval | | Pattern: `^([0-9]+(\.[0-9]+)?(ns\|us\|µs\|ms\|s\|m\|h))+$`
Type: string
Optional: \{\}
| + + #### InterfaceConfigLACP @@ -4967,6 +4984,7 @@ _Appears in:_ | `bufferBoost` _[BufferBoost](#bufferboost)_ | BufferBoost defines the buffer boost configuration for the interface.
Buffer boost increases the shared buffer space allocation for the interface. | | Optional: \{\}
| | `lacp` _[InterfaceConfigLACP](#interfaceconfiglacp)_ | LACP defines LACP options for PortChannel (Aggregate) interfaces. | | Optional: \{\}
| | `evpnMultihoming` _[EVPNMultihoming](#evpnmultihoming)_ | EVPNMultihoming defines EVPN ESI multihoming settings for the interface. | | Optional: \{\}
| +| `ipv6` _[InterfaceConfigIPv6](#interfaceconfigipv6)_ | IPv6 defines IPv6 settings for the interface that have no equivalent in
the core Interface API. Neighbor Discovery settings are only managed while
this is set: other ND settings on the device are left untouched, and
removing it leaves the last applied settings on the device. | | Optional: \{\}
| #### KeepAlive diff --git a/hack/provider/main.go b/hack/provider/main.go index ce554666c..c8ededc5a 100644 --- a/hack/provider/main.go +++ b/hack/provider/main.go @@ -17,6 +17,7 @@ import ( "syscall" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/client-go/kubernetes/scheme" @@ -151,7 +152,17 @@ func loadAndUnmarshalResource(path string) (runtime.Object, error) { obj, _, err := decoder.Decode(json, nil, nil) if err != nil { - return nil, fmt.Errorf("failed to decode resource: %w", err) + if !runtime.IsNotRegisteredError(err) { + return nil, fmt.Errorf("failed to decode resource: %w", err) + } + // Provider-specific configs live in their own API groups and are read + // as unstructured objects by the providers, so decode them as such + // instead of registering every group in the scheme. + u := &unstructured.Unstructured{} + if err := u.UnmarshalJSON(json); err != nil { + return nil, fmt.Errorf("failed to decode resource: %w", err) + } + return u, nil } return obj, nil diff --git a/internal/provider/cisco/nxos/intf.go b/internal/provider/cisco/nxos/intf.go index e152e9519..8c8b49076 100644 --- a/internal/provider/cisco/nxos/intf.go +++ b/internal/provider/cisco/nxos/intf.go @@ -37,6 +37,7 @@ var ( _ gnmiext.DataElement = (*EncapRoutedInterface)(nil) _ gnmiext.DataElement = (*EncapRoutedInterfaceOperItems)(nil) _ gnmiext.DataElement = (*AddrItem)(nil) + _ gnmiext.DataElement = (*NDIf)(nil) _ gnmiext.DataElement = (*FabricFwdIf)(nil) ) @@ -439,6 +440,67 @@ func (a *AddrItem) XPath() string { return "System/ipv4-items/inst-items/dom-items/Dom-list[name=" + a.Vrf + "]/if-items/If-list[id=" + a.ID + "]" } +// NDIf represents the IPv6 Neighbor Discovery configuration of an interface. +// The device only keeps this object for as long as the interface is routed, +// so it needs no explicit cleanup when the interface is deleted. +type NDIf struct { + ID string `json:"id"` + Ctrl string `json:"ctrl"` + RaIntvl int64 `json:"raIntvl"` + RaIntvlMin int64 `json:"raIntvlMin"` + + // Vrf is the VRF Domain in which the interface is a member. + // This field is not serialized to JSON and is only used internally to + // determine the correct XPath. + Vrf string `json:"-"` +} + +func (*NDIf) IsListItem() {} + +func (n *NDIf) XPath() string { + return "System/nd-items/inst-items/dom-items/Dom-list[name=" + n.Vrf + "]/if-items/If-list[id=" + n.ID + "]" +} + +const ( + // NDCtrlDefault is the NX-OS default for ndIf.ctrl: ICMPv6 redirects are + // sent and Router Advertisements are not suppressed. + NDCtrlDefault = "redirects" + // NDCtrlSuppressRA is the ndIf.ctrl flag that stops Router Advertisements + // from being sent ("ipv6 nd suppress-ra"). + NDCtrlSuppressRA = "suppress-ra" + + // NDRAIntervalDefault and NDRAIntervalMinDefault are the NX-OS defaults, + // in seconds, for the maximum and minimum Router Advertisement interval. + NDRAIntervalDefault = 600 + NDRAIntervalMinDefault = 200 +) + +// SetSuppressRA adds or removes the suppress-ra flag and keeps all other flags, +// such as redirects or managed-cfg, which the operator does not manage. The +// device reports the flags sorted, so they are kept sorted to compare equal. +func (n *NDIf) SetSuppressRA(suppress bool) { + flags := slices.DeleteFunc(strings.Split(n.Ctrl, ","), func(f string) bool { + return f == "" || f == NDCtrlSuppressRA + }) + if suppress { + flags = append(flags, NDCtrlSuppressRA) + } + slices.Sort(flags) + n.Ctrl = strings.Join(flags, ",") +} + +// SetRAInterval sets the maximum Router Advertisement interval and derives the +// minimum interval the way "ipv6 nd ra-interval" does on the device: a third of +// the maximum, or the maximum itself where a third would drop below the smallest +// allowed minimum of 3 seconds. +func (n *NDIf) SetRAInterval(seconds int64) { + n.RaIntvl = seconds + n.RaIntvlMin = seconds / 3 + if n.RaIntvlMin < 3 { + n.RaIntvlMin = seconds + } +} + type IntfAddr struct { Addr string `json:"addr"` Pref int `json:"pref"` diff --git a/internal/provider/cisco/nxos/intf_test.go b/internal/provider/cisco/nxos/intf_test.go index dcfdf0506..ed749e4bf 100644 --- a/internal/provider/cisco/nxos/intf_test.go +++ b/internal/provider/cisco/nxos/intf_test.go @@ -145,6 +145,14 @@ func init() { // "ipv6 address use-link-local-only", as required for unnumbered peering. Register("intf_lladdr6", &AddrItem{ID: "eth1/1", Vrf: DefaultVRFName, Is6: true, UseLinkLocalAddr: AdminStEnabled}) + // "ipv6 nd suppress-ra". + Register("nd_if_suppress_ra", &NDIf{ID: "eth1/1", Vrf: DefaultVRFName, Ctrl: "redirects,suppress-ra", RaIntvl: NDRAIntervalDefault, RaIntvlMin: NDRAIntervalMinDefault}) + + // "ipv6 nd ra-interval 4", which NX-OS expands to "ipv6 nd ra-interval 4 min 4". + ndRA := &NDIf{ID: "eth1/1", Vrf: DefaultVRFName, Ctrl: NDCtrlDefault} + ndRA.SetRAInterval(4) + Register("nd_if_ra_interval", ndRA) + pc := &PortChannel{ AccessVlan: DefaultVLAN, AdminSt: AdminStUp, @@ -246,3 +254,49 @@ func init() { icmp := &ICMPIf{ID: "eth1/1", Ctrl: "port-unreachable"} Register("rdr", icmp) } + +func TestNDIfSetRAInterval(t *testing.T) { + // Expected minimums as reported by "show running-config" on NX-OS 10.3(9) + // after "ipv6 nd ra-interval ". + tests := []struct { + seconds, wantMin int64 + }{ + {seconds: 4, wantMin: 4}, + {seconds: 8, wantMin: 8}, + {seconds: 9, wantMin: 3}, + {seconds: 20, wantMin: 6}, + {seconds: 600, wantMin: 200}, + {seconds: 1800, wantMin: 600}, + } + for _, test := range tests { + nd := new(NDIf) + nd.SetRAInterval(test.seconds) + if nd.RaIntvl != test.seconds || nd.RaIntvlMin != test.wantMin { + t.Errorf("SetRAInterval(%d) = %d min %d, want %d min %d", test.seconds, nd.RaIntvl, nd.RaIntvlMin, test.seconds, test.wantMin) + } + } +} + +func TestNDIfSetSuppressRA(t *testing.T) { + // Flag lists as reported by NX-OS 10.3(9), which sorts them. + tests := []struct { + ctrl string + suppress bool + want string + }{ + {ctrl: "redirects", suppress: true, want: "redirects,suppress-ra"}, + {ctrl: "redirects,suppress-ra", suppress: true, want: "redirects,suppress-ra"}, + {ctrl: "redirects,suppress-ra", suppress: false, want: "redirects"}, + {ctrl: "managed-cfg,other-cfg", suppress: true, want: "managed-cfg,other-cfg,suppress-ra"}, + {ctrl: "managed-cfg,other-cfg,redirects,suppress-ra", suppress: false, want: "managed-cfg,other-cfg,redirects"}, + {ctrl: "", suppress: true, want: "suppress-ra"}, + {ctrl: "suppress-ra", suppress: false, want: ""}, + } + for _, test := range tests { + nd := &NDIf{Ctrl: test.ctrl} + nd.SetSuppressRA(test.suppress) + if nd.Ctrl != test.want { + t.Errorf("SetSuppressRA(%t) on %q = %q, want %q", test.suppress, test.ctrl, nd.Ctrl, test.want) + } + } +} diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index b75737935..c08252ccf 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -1748,6 +1748,34 @@ func (p *Provider) EnsureInterface(ctx context.Context, req *provider.EnsureInte ab.Update(ipv6Addr) } + // Neighbor Discovery is only managed where the provider config asks for it, + // so that ND settings configured on the device by other means stay untouched. + if ipv6Addr != nil && cfg.Spec.IPv6 != nil { + nd := new(NDIf) + nd.ID = name + nd.Vrf = vrf + if err := p.client.GetConfig(ctx, nd); err != nil { + if !errors.Is(err, gnmiext.ErrNil) { + return err + } + // The object does not exist before IPv6 is enabled on the interface. + nd.Ctrl = NDCtrlDefault + } + nd.SetSuppressRA(cfg.Spec.IPv6.SuppressRouterAdvertisement) + nd.RaIntvl = NDRAIntervalDefault + nd.RaIntvlMin = NDRAIntervalMinDefault + if d := cfg.Spec.IPv6.RouterAdvertisementInterval; d != nil { + if d.Duration%time.Second != 0 || d.Duration < 4*time.Second || d.Duration > 30*time.Minute { + return apistatus.NewInvalidArgumentError(apistatus.FieldViolation{ + Field: "spec.ipv6.routerAdvertisementInterval", + Description: fmt.Sprintf("router advertisement interval %s must be between 4s and 30m in whole seconds", d.Duration), + }) + } + nd.SetRAInterval(int64(d.Duration / time.Second)) + } + ab.Patch(nd) + } + switch { case req.Interface.Spec.BFD != nil && req.Interface.Spec.BFD.Enabled: f := new(Feature) diff --git a/internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json b/internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json new file mode 100644 index 000000000..6f9a72d73 --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json @@ -0,0 +1,23 @@ +{ + "nd-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "ctrl": "redirects", + "raIntvl": 4, + "raIntvlMin": 4 + } + ] + } + } + ] + } + } + } +} diff --git a/internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json.txt b/internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json.txt new file mode 100644 index 000000000..e19a1bce5 --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/nd_if_ra_interval.json.txt @@ -0,0 +1,3 @@ +interface Ethernet1/1 + no switchport + ipv6 nd ra-interval 4 min 4 diff --git a/internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json b/internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json new file mode 100644 index 000000000..17479f6f8 --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json @@ -0,0 +1,23 @@ +{ + "nd-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "ctrl": "redirects,suppress-ra", + "raIntvl": 600, + "raIntvlMin": 200 + } + ] + } + } + ] + } + } + } +} diff --git a/internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json.txt b/internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json.txt new file mode 100644 index 000000000..ce1695549 --- /dev/null +++ b/internal/provider/cisco/nxos/testdata/nd_if_suppress_ra.json.txt @@ -0,0 +1,3 @@ +interface Ethernet1/1 + no switchport + ipv6 nd suppress-ra 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 index 4cf5314f9..909c1ae7a 100644 --- 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 @@ -10,9 +10,24 @@ spec: name: eth1/1 adminState: Up type: Physical + providerConfigRef: + apiVersion: nx.cisco.networking.metal.ironcore.dev/v1alpha1 + kind: InterfaceConfig + name: uplink-nxconfig ipv6: useLinkLocalOnly: true +-- interfaceconfigs/uplink-nxconfig -- +apiVersion: nx.cisco.networking.metal.ironcore.dev/v1alpha1 +kind: InterfaceConfig +metadata: + name: uplink-nxconfig + namespace: default +spec: + ipv6: + suppressRouterAdvertisement: false + routerAdvertisementInterval: 4s + -- bgps/fabric-bgp -- apiVersion: networking.metal.ironcore.dev/v1alpha1 kind: BGP @@ -104,6 +119,27 @@ spec: } } }, + "nd-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "ctrl": "redirects", + "raIntvl": 4, + "raIntvlMin": 4 + } + ] + } + } + ] + } + } + }, "fm-items": { "bgp-items": { "adminSt": "enabled" @@ -193,6 +229,27 @@ spec: } } }, + "nd-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "ctrl": "redirects", + "raIntvl": 4, + "raIntvlMin": 4 + } + ] + } + } + ] + } + } + }, "fm-items": { "bgp-items": { "adminSt": "enabled" @@ -204,3 +261,4 @@ spec: "bgp-items": {} } } + diff --git a/test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/interface_physical_ipv6_nd.txtar b/test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/interface_physical_ipv6_nd.txtar new file mode 100644 index 000000000..400f7bb58 --- /dev/null +++ b/test/gnmi/testdata/nx.cisco.networking.metal.ironcore.dev/interface_physical_ipv6_nd.txtar @@ -0,0 +1,193 @@ +-- interfaceconfigs/eth1-1-nxconfig -- +apiVersion: nx.cisco.networking.metal.ironcore.dev/v1alpha1 +kind: InterfaceConfig +metadata: + name: eth1-1-nxconfig + namespace: default +spec: + ipv6: + suppressRouterAdvertisement: true + +-- interfaces/eth1-1 -- +apiVersion: networking.metal.ironcore.dev/v1alpha1 +kind: Interface +metadata: + name: eth1-1 + namespace: default +spec: + deviceRef: + name: device + name: eth1/1 + description: Leaf1 to Host1 + adminState: Up + type: Physical + mtu: 9216 + providerConfigRef: + apiVersion: nx.cisco.networking.metal.ironcore.dev/v1alpha1 + kind: InterfaceConfig + name: eth1-1-nxconfig + ipv6: + addresses: + - 2001:db8:1::1/64 + +-- 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": "Leaf1 to Host1", + "FECMode": "auto", + "id": "eth1/1", + "layer": "Layer3", + "mtu": 9216, + "medium": "broadcast", + "mode": "access", + "nativeVlan": "unknown", + "userCfgdFlags": "admin_layer,admin_mtu,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": "disabled", + "addr-items": { + "Addr-list": [ + { + "addr": "2001:db8:1::1/64", + "pref": 0, + "tag": 0, + "type": "primary" + } + ] + } + } + ] + } + } + ] + } + } + }, + "nd-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "ctrl": "redirects,suppress-ra", + "raIntvl": 600, + "raIntvlMin": 200 + } + ] + } + } + ] + } + } + } + } +} + +-- 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": [] + } + } + ] + } + } + }, + "nd-items": { + "inst-items": { + "dom-items": { + "Dom-list": [ + { + "name": "default", + "if-items": { + "If-list": [ + { + "id": "eth1/1", + "ctrl": "redirects,suppress-ra", + "raIntvl": 600, + "raIntvlMin": 200 + } + ] + } + } + ] + } + } + } + } +} + From 7bb0b5db85f60b8a098c6094b38fe3a729bbbf08 Mon Sep 17 00:00:00 2001 From: IvoGoman Date: Thu, 17 Sep 2026 14:51:30 +0200 Subject: [PATCH 3/3] Add handling of firmware upgrades for nxos (#536) * Add handling of firmware upgrades for nxos - adds new DeviceMaintenance key "firmware-upgrade" - pauses reconciliation while upgrade is running - adds DeviceMaintenanceFirmwareTargetAnnotation to specify the target firmware version nxos provider impletementation - ensure enough space, extends NX-API timeout during upgrade. - image is copied & verified. - show incompatibility-all nxos & show install all impact nxos are run to check software & hw - after install & reload the device is checked for version. When boot image matches the target image the upgrade is considered done. Signed-off-by: Ivo Gosemann * incoporate PR review Signed-off-by: Ivo Gosemann * use moq for nxos api client Signed-off-by: Ivo Gosemann --------- Signed-off-by: Ivo Gosemann --- .typos.toml | 1 + api/core/v1alpha1/groupversion_info.go | 12 + internal/controller/core/device_controller.go | 94 ++++- internal/controller/core/suite_test.go | 15 + internal/paused/paused.go | 6 + internal/provider/cisco/nxos/firmware.go | 326 ++++++++++++++++++ internal/provider/cisco/nxos/firmware_test.go | 279 +++++++++++++++ internal/provider/cisco/nxos/provider.go | 2 +- internal/provider/cisco/nxos/system.go | 7 + internal/provider/provider.go | 17 + internal/transport/nxapi/client_mock.go | 151 ++++++++ .../transport/nxapi/client_mock_helper.go | 27 ++ internal/transport/nxapi/nxapi.go | 57 ++- internal/transport/nxapi/nxapi_test.go | 3 +- 14 files changed, 984 insertions(+), 13 deletions(-) create mode 100644 internal/provider/cisco/nxos/firmware.go create mode 100644 internal/provider/cisco/nxos/firmware_test.go create mode 100644 internal/transport/nxapi/client_mock.go create mode 100644 internal/transport/nxapi/client_mock_helper.go diff --git a/.typos.toml b/.typos.toml index d734ae8fe..428c8754b 100644 --- a/.typos.toml +++ b/.typos.toml @@ -9,6 +9,7 @@ extend-ignore-re = [ [default.extend-words] ser = "ser" otu = "otu" +ISSU = "ISSU" # Typo in name used by Cisco NX-OS for a configurable property. # See: https://pubhub.devnetcloud.com/media/dme-docs-10-4-3/docs/System/snmp%3ACommSecP/#configurable-properties acess = "acess" diff --git a/api/core/v1alpha1/groupversion_info.go b/api/core/v1alpha1/groupversion_info.go index 230050b22..c2c6e192c 100644 --- a/api/core/v1alpha1/groupversion_info.go +++ b/api/core/v1alpha1/groupversion_info.go @@ -76,6 +76,11 @@ const VRFLabel = "networking.metal.ironcore.dev/vrf-name" // to trigger certain disruptive operations, such as reboots or firmware upgrades. const DeviceMaintenanceAnnotation = "networking.metal.ironcore.dev/maintenance" +// DeviceMaintenanceFirmwareTargetAnnotation specifies the target firmware image for a firmware upgrade. +// It also includes the MD5 checksum of the firmware image if available. +// The value format is {"url": "", "md5": ""} +const DeviceMaintenanceFirmwareTargetAnnotation = "networking.metal.ironcore.dev/maintenance-target-firmware" + // PhysicalInterfaceNeighborLabel identifies the peer Interface resource on the other end of a physical link. // The value must be the name of another Interface resource in the same namespace. // This label is only valid for interfaces of type Physical. @@ -113,6 +118,10 @@ const ( // spec.provisioning is defined. // The annotation is always consumed once the device reaches Running. DeviceMaintenanceSkipProvisioning = "skip-provisioning" + // DeviceMaintenanceFirmwareUpgrade triggers a firmware upgrade on the device. The provider initiates + // the upgrade workflow, which will apply the new firmware and reboot the device as necessary. + // The target firmware image is specified by the DeviceMaintenanceFirmwareTargetAnnotation. + DeviceMaintenanceFirmwareUpgrade = "firmware-upgrade" ) // Condition types that are used across different objects. @@ -267,6 +276,9 @@ const ( const ( // MaintenanceFailedReason indicates that a requested maintenance operation (e.g., reboot or factory reset) failed. MaintenanceFailedReason = "MaintenanceFailed" + // MaintenanceInProgressReason indicates that a long-running maintenance + // operation (e.g., firmware upgrade) is still in progress and will be retried. + MaintenanceInProgressReason = "MaintenanceInProgress" ) // Reasons that are specific to [RoutingPolicy] objects. diff --git a/internal/controller/core/device_controller.go b/internal/controller/core/device_controller.go index 5374193e7..55a5afb95 100644 --- a/internal/controller/core/device_controller.go +++ b/internal/controller/core/device_controller.go @@ -6,9 +6,11 @@ package core import ( "cmp" "context" + "encoding/json" "errors" "fmt" "math/rand/v2" + "net/url" "regexp" "slices" "strings" @@ -246,6 +248,12 @@ func (r *DeviceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ c } if err := r.reconcileMaintenance(ctx, obj, conn); err != nil { + // ErrMaintenanceInProgress signals that a long-running maintenance step was + // issued and the operation must be resumed on a subsequent reconcile, so + // it must requeue rather than terminate. + if errors.Is(err, provider.ErrMaintenanceInProgress) { + return ctrl.Result{RequeueAfter: Jitter(r.HeartbeatInterval)}, nil + } return ctrl.Result{}, reconcile.TerminalError(err) } @@ -471,7 +479,8 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph case v1alpha1.DeviceMaintenanceReboot, v1alpha1.DeviceMaintenanceFactoryReset, - v1alpha1.DeviceMaintenanceReprovision: + v1alpha1.DeviceMaintenanceReprovision, + v1alpha1.DeviceMaintenanceFirmwareUpgrade: prov, err := provider.LoadProvider[provider.DeviceProvider](obj.Spec.Provider) if err != nil { @@ -479,7 +488,8 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph } if err := prov.Connect(ctx, conn); err != nil { - return fmt.Errorf("failed to connect to device: %w", err) + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceFailed", "Maintenance", "Failed to connect to device for maintenance operation: %v", err) + return provider.ErrMaintenanceInProgress } defer prov.Disconnect(ctx, conn) //nolint:errcheck @@ -545,6 +555,39 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph return fmt.Errorf("failed to prepare device for reprovisioning: %w", err) } obj.Status.Phase = v1alpha1.DevicePhasePending + + case v1alpha1.DeviceMaintenanceFirmwareUpgrade: + mp, ok := prov.(provider.MaintenanceProvider) + if !ok { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support firmware upgrade operation: %s", action) + return nil + } + targetFirmware, err := r.getTargetFirmware(obj) + if err != nil { + return err + } + + err = mp.UpgradeFirmware(ctx, conn, targetFirmware) + if errors.Is(err, provider.ErrMaintenanceInProgress) { + conditions.Set(obj, metav1.Condition{ + Type: v1alpha1.ReadyCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.MaintenanceInProgressReason, + Message: "Firmware upgrade in progress", + }) + r.Recorder.Eventf(obj, nil, "Normal", "FirmwareUpgradeInProgress", "Maintenance", "Device firmware upgrade is in progress") + return err + } + if err != nil { + conditions.Set(obj, metav1.Condition{ + Type: v1alpha1.ReadyCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.MaintenanceFailedReason, + Message: fmt.Sprintf("Failed to upgrade firmware: %v", err), + }) + r.Recorder.Eventf(obj, nil, "Warning", "FirmwareUpgradeFailed", "Maintenance", "Device firmware upgrade has failed: %v", err) + return fmt.Errorf("failed to upgrade firmware: %w", err) + } } default: @@ -558,6 +601,53 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph return nil } +// getTargetFirmware retrieves the target firmware information from the device's annotations +func (r *DeviceReconciler) getTargetFirmware(obj *v1alpha1.Device) (provider.TargetFirmware, error) { + targetFirmwareJSON, ok := obj.Annotations[v1alpha1.DeviceMaintenanceFirmwareTargetAnnotation] + if !ok { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceMissingFirmwareTarget", "Maintenance", "Firmware upgrade requested but no target firmware specified") + return provider.TargetFirmware{}, errors.New("firmware upgrade requested but no target firmware specified") + } + + var targetFirmware provider.TargetFirmware + if err := json.Unmarshal([]byte(targetFirmwareJSON), &targetFirmware); err != nil { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: %v", err) + return provider.TargetFirmware{}, fmt.Errorf("failed to parse firmware target: %w", err) + } + + // Validate the URL to ensure it is a well-formed absolute HTTP(S) URL and reject + // characters commonly used for shell/control injection if this value is later passed on. + parsedURL, err := url.ParseRequestURI(targetFirmware.URL) + if err != nil { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: url is invalid: %v", err) + return provider.TargetFirmware{}, fmt.Errorf("invalid firmware target: url is invalid: %w", err) + } + if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: url scheme must be http or https") + return provider.TargetFirmware{}, errors.New("invalid firmware target: url scheme must be http or https") + } + if parsedURL.Host == "" { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: url host is required") + return provider.TargetFirmware{}, errors.New("invalid firmware target: url host is required") + } + if parsedURL.User != nil { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: url must not contain user info") + return provider.TargetFirmware{}, errors.New("invalid firmware target: url must not contain user info") + } + if strings.ContainsAny(targetFirmware.URL, "\r\n\t`$\\<>|;&()") { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: url contains forbidden characters") + return provider.TargetFirmware{}, errors.New("invalid firmware target: url contains forbidden characters") + } + + // Validate the MD5 checksum if provided. It must be alphanumeric. + if targetFirmware.MD5 != "" && !regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString(targetFirmware.MD5) { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceInvalidFirmwareTarget", "Maintenance", "Invalid firmware target specified: md5 must be alphanumeric") + return provider.TargetFirmware{}, errors.New("invalid firmware target: md5 must be alphanumeric") + } + + return targetFirmware, nil +} + // secretToDevices is a [handler.MapFunc] to be used to enqueue requests for reconciliation // for a Device to update when one of its referenced Secrets gets updated. func (r *DeviceReconciler) secretToDevices(ctx context.Context, obj client.Object) []ctrl.Request { diff --git a/internal/controller/core/suite_test.go b/internal/controller/core/suite_test.go index e786b2da2..57c9576b4 100644 --- a/internal/controller/core/suite_test.go +++ b/internal/controller/core/suite_test.go @@ -430,6 +430,7 @@ type Provider struct { sync.Mutex ConnectError error // if non-nil, Connect returns this error + UpgradeError error // if non-nil, UpgradeFirmware returns this error LastRebootTime time.Time Ports sets.Set[string] @@ -552,6 +553,20 @@ func (p *Provider) FactoryReset(ctx context.Context, conn *deviceutil.Connection return nil } +func (p *Provider) UpgradeFirmware(ctx context.Context, conn *deviceutil.Connection, target provider.TargetFirmware) error { + p.Lock() + defer p.Unlock() + return p.UpgradeError +} + +// SetUpgradeError sets the error that UpgradeFirmware returns on subsequent +// calls. Pass nil to clear it. +func (p *Provider) SetUpgradeError(err error) { + p.Lock() + defer p.Unlock() + p.UpgradeError = err +} + func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) (reterr error) { return nil } diff --git a/internal/paused/paused.go b/internal/paused/paused.go index 9e6362168..9ed3722c7 100644 --- a/internal/paused/paused.go +++ b/internal/paused/paused.go @@ -103,6 +103,12 @@ func computeCondition(device *v1alpha1.Device, obj Object) metav1.Condition { condition.Message = "Device is not reachable: " + cond.Message return condition } + if _, ok := device.GetAnnotations()[v1alpha1.DeviceMaintenanceAnnotation]; ok { + condition.Status = metav1.ConditionTrue + condition.Reason = v1alpha1.PausedReason + condition.Message = "Device is in maintenance" + return condition + } } } diff --git a/internal/provider/cisco/nxos/firmware.go b/internal/provider/cisco/nxos/firmware.go new file mode 100644 index 000000000..267e9047d --- /dev/null +++ b/internal/provider/cisco/nxos/firmware.go @@ -0,0 +1,326 @@ +// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package nxos + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "path" + "regexp" + "strconv" + "strings" + "time" + + "github.com/go-logr/logr" + + "github.com/ironcore-dev/network-operator/internal/deviceutil" + "github.com/ironcore-dev/network-operator/internal/provider" + "github.com/ironcore-dev/network-operator/internal/transport/nxapi" +) + +const ( + // upgradeTimeout is the NX-API client timeout for long-running firmware + // commands (copy and install), which block synchronously for minutes. + upgradeTimeout = 20 * time.Minute + // defaultSessionTimeout is the default NX-API session timeout in seconds. + defaultSessionTimeout = 300 + // firmwareSessionTimeout is an increased timeout for long-running firmware operations (copy and install) to avoid NX-API session expiration. + firmwareSessionTimeout = 1200 +) + +func (p *Provider) UpgradeFirmware(ctx context.Context, _ *deviceutil.Connection, target provider.TargetFirmware) error { + logger := logr.FromContextOrDiscard(ctx) + + if upgraded, err := p.isUpgraded(ctx, target); err != nil || upgraded { + return err + } + + // The copy and install commands block for several minutes, so they run on a + // clone of the NX-API client that only differs in its longer timeout. + nxapiUpgrade, err := p.nxapi.Clone(nxapi.WithTimeout(upgradeTimeout)) + if err != nil { + return fmt.Errorf("failed to create long-timeout nxapi client: %w", err) + } + + // Disable POAP and extend NX-API session timeouts before long-running commands. + if _, err := p.nxapi.Do(ctx, nxapi.NewRequest( + "no boot poap enable", + fmt.Sprintf("system server session cmd-timeout %d", firmwareSessionTimeout), + ).WithRollback(nxapi.Stop)); err != nil { + return fmt.Errorf("nxos firmware: prepare upgrade: %w", err) + } + + targetFileName := path.Base(target.URL) + + if err := p.ensureFirmwareImage(ctx, nxapiUpgrade, target, targetFileName); err != nil { + return err + } + if err := p.checkCompatibility(ctx, nxapiUpgrade, targetFileName); err != nil { + return err + } + if err := p.doUpgrade(ctx, nxapiUpgrade, targetFileName); err != nil { + return err + } + + logger.V(1).Info("Reloading device to boot new firmware") + // Reload is a separate request; connection drop is expected. + if _, err := p.nxapi.Do(ctx, nxapi.NewRequest("reload")); err != nil && !nxapi.IsTransportError(err) { + return fmt.Errorf("nxos firmware: reload failed: %w", err) + } + return provider.ErrMaintenanceInProgress +} + +// isUpgraded checks whether the device is already running the target firmware. +// If the device is running the target firmware, it also resets the NX-API session timeout to the default. +func (p *Provider) isUpgraded(ctx context.Context, target provider.TargetFirmware) (bool, error) { + logger := logr.FromContextOrDiscard(ctx) + + // A transport-level failure here means the device is still unreachable + // (typically mid-reload from a prior step), which is a normal in-progress + // state rather than a failure, so signal the caller to requeue. + bootImage := new(BootImage) + if err := p.client.GetState(ctx, bootImage); err != nil { + return false, fmt.Errorf("nxos firmware: failed to read running version: %w", err) + } + + targetFileName := path.Base(target.URL) + if path.Base(string(*bootImage)) == targetFileName { + logger.V(1).Info("Device already running target firmware", "filename", targetFileName) + if _, err := p.nxapi.Do(ctx, nxapi.NewRequest( + fmt.Sprintf("system server session cmd-timeout %d", defaultSessionTimeout), + ).WithRollback(nxapi.Stop)); err != nil { + return false, fmt.Errorf("nxos firmware: reset session timeout: %w", err) + } + return true, nil + } + return false, nil +} + +// ensureFirmwareImage ensures a valid firmware image is present on bootflash +func (p *Provider) ensureFirmwareImage(ctx context.Context, c nxapi.Client, target provider.TargetFirmware, targetFileName string) error { + logger := logr.FromContextOrDiscard(ctx) + + sum, err := p.fileMD5(ctx, targetFileName) + if err != nil { + return fmt.Errorf("nxos firmware: check existing image: %w", err) + } + + if sum != "" && target.MD5 != "" && !strings.EqualFold(sum, target.MD5) { + logger.V(1).Info("Stale image on bootflash, deleting", "file", targetFileName) + if _, err := p.nxapi.Do(ctx, nxapi.NewRequest("delete bootflash:///"+targetFileName+" no-prompt")); err != nil { + return fmt.Errorf("nxos firmware: delete stale image: %w", err) + } + sum = "" // force re-copy + } + + if sum == "" { + size, err := remoteImageSize(ctx, target.URL) + if err != nil { + return err + } + dir, err := p.ListDirectory(ctx, "bootflash:") + if err != nil { + return fmt.Errorf("nxos firmware: dir bootflash: failed: %w", err) + } + if size > dir.Bytesfree { + // TODO: Check if more than the current target and the current running image are present on the bootflash and delete them to free up space. + return fmt.Errorf("nxos firmware: image (%d bytes) does not fit in bootflash free space (%d bytes)", size, dir.Bytesfree) + } + + // The NX-OS `copy https://...` command unconditionally prompts + // "Enter username:", which NX-API cannot answer. Depending on the + // endpoint a dummy username results in 403 and http is not supported. + // Downloading via `run bash wget` avoids the prompt entirely; bootflash + // is mounted at /bootflash inside the bash shell. The management VRF is + // a Linux netns, so wget must run inside it to reach the image server. + dest := "/bootflash/" + targetFileName + logger.V(1).Info("Copying firmware image to bootflash", "file", targetFileName) + if _, err := c.Do(ctx, nxapi.NewRequest( + "feature bash-shell", + //nolint:dupword // NX-OS requires `run bash bash -c` here. + `run bash bash -c 'ip netns exec management wget --no-verbose --output-document="$1" "$2"' -- `+strconv.Quote(dest)+` `+ + strconv.Quote(target.URL), + ).WithRollback(nxapi.Stop)); err != nil { + return fmt.Errorf("nxos firmware: copy image: %w", err) + } + + sum, err = p.fileMD5(ctx, targetFileName) + if err != nil { + return fmt.Errorf("nxos firmware: verify md5 after copy: %w", err) + } + } + + switch { + case sum == "": + return errors.New("nxos firmware: unexpected missing MD5 checksum") + case target.MD5 == "": + return nil // no checksum provided, so we cannot verify the image + case !strings.EqualFold(sum, target.MD5): + return fmt.Errorf("nxos firmware: md5 mismatch after copy: got %s want %s", sum, target.MD5) + default: + logger.V(1).Info("Firmware image copied and verified") + return nil + } +} + +// checkCompatibility runs the software compatibility and install impact checks +// and logs their output. +func (p *Provider) checkCompatibility(ctx context.Context, c nxapi.Client, targetFileName string) error { + logger := logr.FromContextOrDiscard(ctx) + compatRes, err := c.Do(ctx, nxapi.NewRequest( + "show incompatibility-all nxos bootflash:"+targetFileName, + ).WithMethod(nxapi.MethodCLIASCII)) + if err != nil { + return fmt.Errorf("nxos firmware: compatibility check failed: %w", err) + } + logCLIResult(logger, compatRes, "Software compatibility check result") + + impactRes, err := c.Do(ctx, nxapi.NewRequest( + "show install all impact nxos bootflash:"+targetFileName, + ).WithMethod(nxapi.MethodCLIASCII)) + if err != nil { + return fmt.Errorf("nxos firmware: install impact check failed: %w", err) + } + logCLIResult(logger, impactRes, "Install impact check result") + return nil +} + +// doUpgrade saves the running config, installs the firmware without +// reload. +func (p *Provider) doUpgrade(ctx context.Context, c nxapi.Client, targetFileName string) error { + logger := logr.FromContextOrDiscard(ctx) + if _, err := p.nxapi.Do(ctx, nxapi.NewRequest( + "copy running-config startup-config", + ).WithRollback(nxapi.Stop)); err != nil { + return fmt.Errorf("nxos firmware: save config: %w", err) + } + + // install all with no-reload keeps the connection up and returns the real result. + logger.V(1).Info("Installing firmware (no-reload)", "file", targetFileName) + installRes, err := c.Do(ctx, nxapi.NewRequest( + "install all nxos bootflash:"+targetFileName+" no-reload", + ).WithMethod(nxapi.MethodCLIASCII).WithRollback(nxapi.Stop)) + if err != nil { + return fmt.Errorf("nxos firmware: install failed: %w", err) + } + logCLIResult(logger, installRes, "Install result") + return nil +} + +// fileMD5 returns the md5 checksum of a file on bootflash, or an empty string +// if the file does not exist. A "file not found" RPC error from the device is +// treated as absence, not a failure, so callers can proceed to copy the image. +func (p *Provider) fileMD5(ctx context.Context, filename string) (string, error) { + res, err := p.nxapi.Do(ctx, nxapi.NewRequest("show file bootflash:"+filename+" md5sum")) + if err != nil { + if isFileNotFound(err) { + return "", nil + } + return "", err + } + if len(res) == 0 { + return "", nil + } + var result struct { + MD5Sum string `json:"file_content_md5sum"` + } + if err := json.Unmarshal(res[0], &result); err != nil { + return "", fmt.Errorf("nxos firmware: failed to decode md5sum response: %w", err) + } + return strings.TrimSpace(result.MD5Sum), nil +} + +// isFileNotFound reports whether err is an NX-API RPC error indicating that a +// referenced file does not exist on the device, so callers can distinguish a +// missing image (an expected pre-copy state) from a genuine failure. +func isFileNotFound(err error) bool { + var rpcErr *nxapi.RPCError + if !errors.As(err, &rpcErr) { + return false + } + msg := strings.ToLower(rpcErr.Error()) + return strings.Contains(msg, "no such file") || strings.Contains(msg, "not found") +} + +// remoteImageSize issues an HTTP HEAD to the firmware URL and returns its +// Content-Length in bytes. +func remoteImageSize(ctx context.Context, rawURL string) (int64, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodHead, rawURL, nil) + if err != nil { + return 0, fmt.Errorf("nxos firmware: build HEAD request: %w", err) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return 0, fmt.Errorf("nxos firmware: HEAD %s: %w", rawURL, err) + } + defer resp.Body.Close() + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return 0, fmt.Errorf("nxos firmware: HEAD %s returned status %d", rawURL, resp.StatusCode) + } + if resp.ContentLength < 0 { + return 0, fmt.Errorf("nxos firmware: HEAD %s did not return a Content-Length", rawURL) + } + return resp.ContentLength, nil +} + +// configSessionActive reports whether a configuration session is currently +// open on the device (which would block ISSU). +func (p *Provider) configSessionActive(ctx context.Context) (bool, error) { + res, err := p.nxapi.Do(ctx, nxapi.NewRequest("show configuration session summary")) + if err != nil { + return false, err + } + if len(res) == 0 { + return false, nil + } + var body struct { + Table struct { + Row json.RawMessage `json:"ROW_session"` + } `json:"TABLE_session"` + } + if err := json.Unmarshal(res[0], &body); err != nil { + return false, nil //nolint:nilerr // unmarshal failure means no session table => no sessions + } + return len(body.Table.Row) > 0, nil +} + +// progressBarRe matches NX-OS CLI progress bar segments like +// "[#### ] 25%" that pollute cli_ascii output. +var progressBarRe = regexp.MustCompile(`\[[#\s]*\]\s*\d+%`) + +// statusMarkerRe matches a trailing " -- SUCCESS" style status marker left on a +// line after its progress bars are stripped, capturing the marker word. The +// marker must be an uppercase word so table separator lines ending in a run of +// dashes (e.g. "------ ------") are not mistaken for markers. +var statusMarkerRe = regexp.MustCompile(`(?m)\s*--\s*([A-Z]+)\s*$`) + +// cleanCLIOutput tidies raw cli_ascii command output for logging by stripping +// progress bar segments, moving trailing status markers (e.g. "-- SUCCESS") +// onto their own line without the leading "--", and dropping blank lines. +func cleanCLIOutput(s string) string { + s = progressBarRe.ReplaceAllString(s, "") + s = statusMarkerRe.ReplaceAllString(s, "\n$1") + var kept []string + for line := range strings.SplitSeq(s, "\n") { + if strings.TrimSpace(line) == "" { + continue + } + kept = append(kept, strings.TrimRight(line, " \t")) + } + return strings.Join(kept, "\n") +} + +// logCLIResult logs the cli_ascii output from an NX-API response, if present. +func logCLIResult(logger logr.Logger, res []json.RawMessage, msg string) { + if len(res) == 0 { + return + } + var output string + if err := json.Unmarshal(res[0], &output); err == nil { + logger.V(1).Info(msg, "output", cleanCLIOutput(output)) + } +} diff --git a/internal/provider/cisco/nxos/firmware_test.go b/internal/provider/cisco/nxos/firmware_test.go new file mode 100644 index 000000000..991fe66e0 --- /dev/null +++ b/internal/provider/cisco/nxos/firmware_test.go @@ -0,0 +1,279 @@ +// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package nxos + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/ironcore-dev/network-operator/internal/provider" + "github.com/ironcore-dev/network-operator/internal/transport/gnmiext" + "github.com/ironcore-dev/network-operator/internal/transport/nxapi" +) + +func TestCleanCLIOutput(t *testing.T) { + in := "Installer will perform compatibility check first. Please wait. \nInstaller will exit before reload\nInstaller is forced disruptive\n\nVerifying image bootflash:/nxos64-cs.10.6.3.F.bin for boot variable \"nxos\".\n[# ] 0%[####################] 100% -- SUCCESS\n\nVerifying EPLD/FPGA image //bootflash/nxos64-cs.10.6.3.F.bin.\n[# ] 0%[####################] 100% -- SUCCESS\n\nVerifying image type.\n[# ] 0%[####################] 100% -- SUCCESS\n\nPreparing \"nxos\" version info using image bootflash:/nxos64-cs.10.6.3.F.bin.\n[# ] 0%[####################] 100% -- SUCCESS\n\nPreparing \"bios\" version info using image bootflash:/nxos64-cs.10.6.3.F.bin.\n[# ] 0%[####################] 100% -- SUCCESS\n\nPerforming module support checks.\n[####################] 100% -- SUCCESS\n\nNotifying services about system upgrade.\n[####################] 100% -- SUCCESS\n\n\n\nCompatibility check is done:\nModule bootable Impact Install-type Reason\n------ -------- -------------- ------------ ------\n 1 yes disruptive reset default upgrade is not hitless\n 27 yes disruptive reset default upgrade is not hitless\n\n\n\nImages will be upgraded according to following table:\nModule Image Running-Version(pri:alt) New-Version Upg-Required\n------ ---------- ---------------------------------------- -------------------- ------------\n 1 lcn9k 10.6(2) 10.6(3) yes\n 27 nxos 10.6(2) 10.6(3) yes\n 27 bios v05.53(01/22/2025):v05.47(04/28/2022) v05.53(01/22/2025) no\n\n\nFPGA microcode will be upgraded according to following table:\nModule Type EPLD Running-Version Flashed-Version* New-Version Upg-Required\n------ ---- ------------- --------------- ---------------- ----------- ------------\n 27 SUP MI FPGA 0x5 0x5 0x5 No\n 27 SUP IO FPGA 0x17 0x17 0x18 Yes\n* If Running-Version and Flashed-Version are different it implies that the system has not yet been reloaded for the new version to take effect\n\nEPLD Upgrade may result in multiple modules going offline.\n\nAdditional info for this installation:\n--------------------------------------\n\nOption \"no-reload\" has been used - it is necessary reload device after installation without saving config.\nSaving config before can result incorrect startup config load after reload with new version of NXOS.\n\nService \"vpc\" in vdc 1: Vpc is enabled, Please make sure both Vpc peer switches have same boot mode using 'show boot mode' and proceed \n\n\n\n\n\nInstall is in progress, please wait.\n[# ] 0%\nSetting boot variables.\n[####################] 100% -- SUCCESS\n\nPerforming configuration copy.\n[# ] 0%[# ] 0%[###### ] 25%[########### ] 50%[################ ] 75%[####################] 100%\nPerforming configuration copy.\n[####################] 100% -- SUCCESS\n\nModule 1: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\n[# ] 0%\nModule 1: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\n[####################] 100% -- SUCCESS\n\nModule 27: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\n[# ] 0%\nModule 27: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\n[####################] 100% -- SUCCESS\n\nEPLD/FPGA upgrade can take upto 4 mins\n[# ] 0%\nPerforming EPLD/FPGA upgrade .\n[####################] 100% -- SUCCESS\n\n\n" + + want := "Installer will perform compatibility check first. Please wait.\nInstaller will exit before reload\nInstaller is forced disruptive\nVerifying image bootflash:/nxos64-cs.10.6.3.F.bin for boot variable \"nxos\".\nSUCCESS\nVerifying EPLD/FPGA image //bootflash/nxos64-cs.10.6.3.F.bin.\nSUCCESS\nVerifying image type.\nSUCCESS\nPreparing \"nxos\" version info using image bootflash:/nxos64-cs.10.6.3.F.bin.\nSUCCESS\nPreparing \"bios\" version info using image bootflash:/nxos64-cs.10.6.3.F.bin.\nSUCCESS\nPerforming module support checks.\nSUCCESS\nNotifying services about system upgrade.\nSUCCESS\nCompatibility check is done:\nModule bootable Impact Install-type Reason\n------ -------- -------------- ------------ ------\n 1 yes disruptive reset default upgrade is not hitless\n 27 yes disruptive reset default upgrade is not hitless\nImages will be upgraded according to following table:\nModule Image Running-Version(pri:alt) New-Version Upg-Required\n------ ---------- ---------------------------------------- -------------------- ------------\n 1 lcn9k 10.6(2) 10.6(3) yes\n 27 nxos 10.6(2) 10.6(3) yes\n 27 bios v05.53(01/22/2025):v05.47(04/28/2022) v05.53(01/22/2025) no\nFPGA microcode will be upgraded according to following table:\nModule Type EPLD Running-Version Flashed-Version* New-Version Upg-Required\n------ ---- ------------- --------------- ---------------- ----------- ------------\n 27 SUP MI FPGA 0x5 0x5 0x5 No\n 27 SUP IO FPGA 0x17 0x17 0x18 Yes\n* If Running-Version and Flashed-Version are different it implies that the system has not yet been reloaded for the new version to take effect\nEPLD Upgrade may result in multiple modules going offline.\nAdditional info for this installation:\n--------------------------------------\nOption \"no-reload\" has been used - it is necessary reload device after installation without saving config.\nSaving config before can result incorrect startup config load after reload with new version of NXOS.\nService \"vpc\" in vdc 1: Vpc is enabled, Please make sure both Vpc peer switches have same boot mode using 'show boot mode' and proceed\nInstall is in progress, please wait.\nSetting boot variables.\nSUCCESS\nPerforming configuration copy.\nPerforming configuration copy.\nSUCCESS\nModule 1: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\nModule 1: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\nSUCCESS\nModule 27: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\nModule 27: Refreshing compact flash and upgrading bios/loader/bootrom.\nWarning: please do not remove or power off the module at this time.\nSUCCESS\nEPLD/FPGA upgrade can take upto 4 mins\nPerforming EPLD/FPGA upgrade .\nSUCCESS" + + got := cleanCLIOutput(in) + if got != want { + t.Errorf("cleanCLIOutput mismatch:\ngot:\n%s\n\nwant:\n%s", got, want) + } +} + +// mockGNMI returns a gnmiext.ClientMock that reports the given running version +// and boot image on GetState and a canned hostname on GetConfig. +func mockGNMI(version, bootImage string) *gnmiext.ClientMock { + return &gnmiext.ClientMock{ + GetStateFunc: func(_ context.Context, elems ...gnmiext.DataElement) error { + for _, e := range elems { + switch v := e.(type) { + case *FirmwareVersion: + *v = FirmwareVersion(version) + case *BootImage: + *v = BootImage(bootImage) + } + } + return nil + }, + GetConfigFunc: func(_ context.Context, elems ...gnmiext.DataElement) error { + for _, e := range elems { + if h, ok := e.(*Hostname); ok { + *h = Hostname("test-switch") + } + } + return nil + }, + } +} + +func TestUpgradeFirmwareAlreadyOnTarget(t *testing.T) { + client := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + return []json.RawMessage{json.RawMessage(`null`)}, nil + }) + + p := &Provider{client: mockGNMI("", "bootflash://nxos64-cs.10.6.3.F.bin"), nxapi: client} + target := provider.TargetFirmware{ + URL: "https://repo.example/nxos64-cs.10.6.3.F.bin", + MD5: "48c0db0a564c442f123eba8724ef352f", + } + if err := p.UpgradeFirmware(t.Context(), nil, target); err != nil { + t.Fatalf("expected nil (already upgraded), got %v", err) + } +} + +func TestListDirectoryBytesfree(t *testing.T) { + client := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + if got := r.Commands()[0]; got != "dir bootflash:" { + t.Errorf("cmd = %q, want 'dir bootflash:'", got) + } + return []json.RawMessage{json.RawMessage(`{"bytesfree":3664789504}`)}, nil + }) + p := &Provider{nxapi: client} + dir, err := p.ListDirectory(t.Context(), "bootflash:") + if err != nil { + t.Fatalf("ListDirectory error: %v", err) + } + if dir.Bytesfree != 3664789504 { + t.Errorf("dir.Bytesfree = %d, want 3664789504", dir.Bytesfree) + } +} + +func TestFileMD5(t *testing.T) { + client := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + want := "show file bootflash:nxos64-cs.10.6.3.F.bin md5sum" + if got := r.Commands()[0]; got != want { + t.Errorf("cmd = %q, want %q", got, want) + } + return []json.RawMessage{json.RawMessage(`{"file_content_md5sum":"48c0db0a564c442f123eba8724ef352f\n"}`)}, nil + }) + p := &Provider{nxapi: client} + got, err := p.fileMD5(t.Context(), "nxos64-cs.10.6.3.F.bin") + if err != nil { + t.Fatalf("fileMD5 error: %v", err) + } + if got != "48c0db0a564c442f123eba8724ef352f" { + t.Errorf("fileMD5 = %q", got) + } +} + +func TestFileMD5NotFound(t *testing.T) { + p := &Provider{nxapi: nxapi.MockErrorClient(1, "No such file or directory")} + got, err := p.fileMD5(t.Context(), "nxos64-cs.10.6.3.F.bin") + if err != nil { + t.Fatalf("fileMD5 error: %v", err) + } + if got != "" { + t.Errorf("fileMD5 = %q, want empty string for missing file", got) + } +} + +func TestFileMD5RealError(t *testing.T) { + p := &Provider{nxapi: nxapi.MockErrorClient(500, "internal device error")} + if _, err := p.fileMD5(t.Context(), "nxos64-cs.10.6.3.F.bin"); err == nil { + t.Fatal("expected error for non-not-found RPC failure, got nil") + } +} + +func TestConfigSessionActive(t *testing.T) { + client1 := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + if len(r.Commands()) == 1 && r.Commands()[0] == "show configuration session summary" { + return []json.RawMessage{json.RawMessage(`{"TABLE_session":{"ROW_session":[{"session":"s1"}]}}`)}, nil + } + return nil, errors.New("unexpected command(s)") + }) + p := &Provider{nxapi: client1} + active, err := p.configSessionActive(t.Context()) + if err != nil { + t.Fatalf("configSessionActive error: %v", err) + } + if !active { + t.Error("expected active session, got false") + } +} + +func TestRemoteImageSize(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodHead { + t.Errorf("method = %s, want HEAD", r.Method) + } + w.Header().Set("Content-Length", "3005853696") + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + got, err := remoteImageSize(t.Context(), srv.URL+"/nxos64-cs.10.6.3.F.bin") + if err != nil { + t.Fatalf("remoteImageSize error: %v", err) + } + if got != 3005853696 { + t.Errorf("remoteImageSize = %d, want 3005853696", got) + } +} + +func TestUpgradeFirmwareCopyStep(t *testing.T) { + // Device on old version, image absent -> preflight + copy issued -> in progress. + var got []string + copied := false + client := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + cmds := r.Commands() + got = append(got, cmds...) + msgs := make([]json.RawMessage, len(cmds)) + for i, c := range cmds { + switch { + case c == "show file bootflash:nxos64-cs.10.6.3.F.bin md5sum": + if copied { + msgs[i] = json.RawMessage(`{"file_content_md5sum":"48c0db0a564c442f123eba8724ef352f"}`) // present after copy + } else { + msgs[i] = json.RawMessage(`{"file_content_md5sum":""}`) // absent before copy + } + case c == "dir bootflash:": + msgs[i] = json.RawMessage(`{"bytesfree":6000000000}`) + case strings.HasPrefix(c, "run bash"): + copied = true + msgs[i] = json.RawMessage(`""`) + default: + msgs[i] = json.RawMessage(`""`) + } + } + return msgs, nil + }) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", "3005853696") + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + p := &Provider{ + client: mockGNMI("", "bootflash://nxos64-cs.10.6.2.F.bin"), + nxapi: client, + } + target := provider.TargetFirmware{ + URL: srv.URL + "/nxos64-cs.10.6.3.F.bin", + MD5: "48c0db0a564c442f123eba8724ef352f", + } + err := p.UpgradeFirmware(t.Context(), nil, target) + if !errors.Is(err, provider.ErrMaintenanceInProgress) { + t.Fatalf("expected ErrUpgradeInProgress, got %v", err) + } + joined := strings.Join(got, "|") + if !strings.Contains(joined, "run bash") { + t.Errorf("wget copy command not issued; got %v", got) + } +} + +func TestUpgradeFirmwareInsufficientSpace(t *testing.T) { + client := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + cmds := r.Commands() + msgs := make([]json.RawMessage, len(cmds)) + for i, c := range cmds { + if c == "dir bootflash:" { + msgs[i] = json.RawMessage(`{"bytesfree":"1000"}`) + } else { + msgs[i] = json.RawMessage(`null`) + } + } + return msgs, nil + }) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", "3005853696") + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + p := &Provider{client: mockGNMI("10.6(2)", ""), nxapi: client} + target := provider.TargetFirmware{URL: srv.URL + "/nxos64-cs.10.6.3.F.bin", MD5: "abc"} + err := p.UpgradeFirmware(t.Context(), nil, target) + if err == nil || errors.Is(err, provider.ErrMaintenanceInProgress) { + t.Fatalf("expected hard error for insufficient space, got %v", err) + } +} + +func TestUpgradeFirmwareInstallAndReload(t *testing.T) { + // Image present with matching md5 -> impact + save + install(no-reload) + reload. + var got []string + client := nxapi.NewClientMock(func(_ context.Context, r nxapi.Request) ([]json.RawMessage, error) { + cmds := r.Commands() + got = append(got, cmds...) + // The reload request drops the connection, surfacing as a transport + // error that UpgradeFirmware treats as expected (device going down). + if len(cmds) == 1 && cmds[0] == "reload" { + return nil, io.EOF + } + msgs := make([]json.RawMessage, len(cmds)) + for i, c := range cmds { + if c == "show file bootflash:nxos64-cs.10.6.3.F.bin md5sum" { + msgs[i] = json.RawMessage(`{"file_content_md5sum":"48c0db0a564c442f123eba8724ef352f"}`) + } else { + msgs[i] = json.RawMessage(`null`) + } + } + return msgs, nil + }) + + p := &Provider{client: mockGNMI("10.6(2)", ""), nxapi: client} + target := provider.TargetFirmware{URL: "https://repo.example/nxos64-cs.10.6.3.F.bin", MD5: "48c0db0a564c442f123eba8724ef352f"} + err := p.UpgradeFirmware(t.Context(), nil, target) + if !errors.Is(err, provider.ErrMaintenanceInProgress) { + t.Fatalf("expected ErrUpgradeInProgress after reload, got %v", err) + } + joined := strings.Join(got, "|") + for _, want := range []string{ + "show install all impact nxos bootflash:nxos64-cs.10.6.3.F.bin", + "copy running-config startup-config", + "install all nxos bootflash:nxos64-cs.10.6.3.F.bin no-reload", + "reload", + } { + if !strings.Contains(joined, want) { + t.Errorf("missing command %q; got %v", want, got) + } + } +} diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index c08252ccf..97c81c03a 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -81,7 +81,7 @@ const maxSetOperations = 20 type Provider struct { conn *grpc.ClientConn client gnmiext.Client - nxapi *nxapi.Client + nxapi nxapi.Client } // timeout is the default timeout for all HTTP/gRPC requests made by the provider. diff --git a/internal/provider/cisco/nxos/system.go b/internal/provider/cisco/nxos/system.go index bff6d5a7e..a3153a6a9 100644 --- a/internal/provider/cisco/nxos/system.go +++ b/internal/provider/cisco/nxos/system.go @@ -68,6 +68,13 @@ func (*FirmwareVersion) XPath() string { return "System/showversion-items/nxosVersion" } +// BootImage is the boot image filename of the device, e.g. "bootflash://nxos.10.4.3.bin". +type BootImage string + +func (*BootImage) XPath() string { + return "System/showversion-items/nxosImageFile" +} + type BootTime UnixTime func (*BootTime) XPath() string { diff --git a/internal/provider/provider.go b/internal/provider/provider.go index db7033791..072436fc7 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -5,6 +5,7 @@ package provider import ( "context" "crypto/tls" + "errors" "fmt" "maps" "net/netip" @@ -47,6 +48,9 @@ type MaintenanceProvider interface { Reboot(context.Context, *deviceutil.Connection) error // FactoryReset performs a factory reset of the device. FactoryReset(context.Context, *deviceutil.Connection) error + // UpgradeFirmware initiates a firmware upgrade on the device. + // The provider is responsible for applying the new firmware and rebooting the device as necessary. + UpgradeFirmware(context.Context, *deviceutil.Connection, TargetFirmware) error } // ProvisioningProvider is the interface for the realization of the provisioning-related operations over different providers. @@ -73,6 +77,19 @@ type DevicePort struct { Transceiver string } +// TargetFirmware represents the firmware image to be applied to the device, including its URL and optional checksum. +type TargetFirmware struct { + // URL is the URL of the firmware image to be applied to the device. + URL string `json:"url"` + // MD5 is the MD5 checksum of the firmware image, if available. + MD5 string `json:"md5,omitempty"` +} + +// ErrMaintenanceInProgress is returned by MaintenanceProvider when it fails to fully complete +// a maintenance operation. The controller treats this as a signal to +// requeue and re-invoke rather than a hard failure. +var ErrMaintenanceInProgress = errors.New("provider: maintenance in progress") + type DeviceInfo struct { // Hostname is the hostname of the device. Hostname string diff --git a/internal/transport/nxapi/client_mock.go b/internal/transport/nxapi/client_mock.go new file mode 100644 index 000000000..693541a46 --- /dev/null +++ b/internal/transport/nxapi/client_mock.go @@ -0,0 +1,151 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package nxapi + +import ( + "context" + "encoding/json" + "sync" +) + +// Ensure, that ClientMock does implement Client. +// If this is not the case, regenerate this file with moq. +var _ Client = &ClientMock{} + +// ClientMock is a mock implementation of Client. +// +// func TestSomethingThatUsesClient(t *testing.T) { +// +// // make and configure a mocked Client +// mockedClient := &ClientMock{ +// CloneFunc: func(options ...Option) (Client, error) { +// panic("mock out the Clone method") +// }, +// DoFunc: func(contextMoqParam context.Context, request Request) ([]json.RawMessage, error) { +// panic("mock out the Do method") +// }, +// } +// +// // use mockedClient in code that requires Client +// // and then make assertions. +// +// } +type ClientMock struct { + // CloneFunc mocks the Clone method. + CloneFunc func(options ...Option) (Client, error) + + // DoFunc mocks the Do method. + DoFunc func(contextMoqParam context.Context, request Request) ([]json.RawMessage, error) + + // calls tracks calls to the methods. + calls struct { + // Clone holds details about calls to the Clone method. + Clone []struct { + // Options is the options argument value. + Options []Option + } + // Do holds details about calls to the Do method. + Do []struct { + // ContextMoqParam is the contextMoqParam argument value. + ContextMoqParam context.Context + // Request is the request argument value. + Request Request + } + } + lockClone sync.RWMutex + lockDo sync.RWMutex +} + +// Clone calls CloneFunc. +func (mock *ClientMock) Clone(options ...Option) (Client, error) { + if mock.CloneFunc == nil { + panic("ClientMock.CloneFunc: method is nil but Client.Clone was just called") + } + callInfo := struct { + Options []Option + }{ + Options: options, + } + mock.lockClone.Lock() + mock.calls.Clone = append(mock.calls.Clone, callInfo) + mock.lockClone.Unlock() + return mock.CloneFunc(options...) +} + +// CloneCalls gets all the calls that were made to Clone. +// Check the length with: +// +// len(mockedClient.CloneCalls()) +func (mock *ClientMock) CloneCalls() []struct { + Options []Option +} { + var calls []struct { + Options []Option + } + mock.lockClone.RLock() + calls = mock.calls.Clone + mock.lockClone.RUnlock() + return calls +} + +// ResetCloneCalls reset all the calls that were made to Clone. +func (mock *ClientMock) ResetCloneCalls() { + mock.lockClone.Lock() + mock.calls.Clone = nil + mock.lockClone.Unlock() +} + +// Do calls DoFunc. +func (mock *ClientMock) Do(contextMoqParam context.Context, request Request) ([]json.RawMessage, error) { + if mock.DoFunc == nil { + panic("ClientMock.DoFunc: method is nil but Client.Do was just called") + } + callInfo := struct { + ContextMoqParam context.Context + Request Request + }{ + ContextMoqParam: contextMoqParam, + Request: request, + } + mock.lockDo.Lock() + mock.calls.Do = append(mock.calls.Do, callInfo) + mock.lockDo.Unlock() + return mock.DoFunc(contextMoqParam, request) +} + +// DoCalls gets all the calls that were made to Do. +// Check the length with: +// +// len(mockedClient.DoCalls()) +func (mock *ClientMock) DoCalls() []struct { + ContextMoqParam context.Context + Request Request +} { + var calls []struct { + ContextMoqParam context.Context + Request Request + } + mock.lockDo.RLock() + calls = mock.calls.Do + mock.lockDo.RUnlock() + return calls +} + +// ResetDoCalls reset all the calls that were made to Do. +func (mock *ClientMock) ResetDoCalls() { + mock.lockDo.Lock() + mock.calls.Do = nil + mock.lockDo.Unlock() +} + +// ResetCalls reset all the calls that were made to all mocked methods. +func (mock *ClientMock) ResetCalls() { + mock.lockClone.Lock() + mock.calls.Clone = nil + mock.lockClone.Unlock() + + mock.lockDo.Lock() + mock.calls.Do = nil + mock.lockDo.Unlock() +} diff --git a/internal/transport/nxapi/client_mock_helper.go b/internal/transport/nxapi/client_mock_helper.go new file mode 100644 index 000000000..65eaa17dd --- /dev/null +++ b/internal/transport/nxapi/client_mock_helper.go @@ -0,0 +1,27 @@ +// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package nxapi + +import ( + "context" + "encoding/json" +) + +// NewClientMock returns a [nxapi.ClientMock] whose Do method is implemented by the given function. +func NewClientMock(doFunc func(context.Context, Request) ([]json.RawMessage, error)) *ClientMock { + client := &ClientMock{ + DoFunc: doFunc, + } + client.CloneFunc = func(...Option) (Client, error) { return client, nil } + return client +} + +// MockErrorClient returns an [nxapi.ClientMock] whose Do always fails with a single +// [nxapi.RPCError] carrying the given code and message, so tests can exercise +// how helpers react to device-side command failures. +func MockErrorClient(code int, message string) *ClientMock { + return NewClientMock(func(_ context.Context, _ Request) ([]json.RawMessage, error) { + return nil, RPCErrors{{Code: code, Message: message}} + }) +} diff --git a/internal/transport/nxapi/nxapi.go b/internal/transport/nxapi/nxapi.go index 4ca458b80..0afbe6f28 100644 --- a/internal/transport/nxapi/nxapi.go +++ b/internal/transport/nxapi/nxapi.go @@ -30,22 +30,35 @@ func (f RoundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } +//go:generate go tool moq -with-resets -out nxapi_mock.go . Client + // Client sends JSON-RPC requests to a Cisco NX-OS device via NX-API. +type Client interface { + // Do sends a Request to the device and returns one [json.RawMessage] per + // command, in the same order as the request. + Do(context.Context, Request) ([]json.RawMessage, error) + // Clone returns a copy of the client with the given options applied. + Clone(...Option) (Client, error) +} + +// client sends JSON-RPC requests to a Cisco NX-OS device via NX-API. // Use [NewClient] to construct one. -type Client struct { +type client struct { client *http.Client url url.URL target string } -// Option configures a [Client]. -type Option func(*Client) error +var _ Client = &client{} + +// Option configures a [client]. +type Option func(*client) error // WithPort overrides the port in the connection address. // This is useful when NX-API is reachable on a different // port (e.g. 8443) than the default (80/443). func WithPort(port string) Option { - return func(c *Client) error { + return func(c *client) error { host := c.url.Host if h, _, err := net.SplitHostPort(host); err == nil { host = h @@ -58,7 +71,7 @@ func WithPort(port string) Option { // WithTimeout sets the HTTP client timeout. // The default is 0 (no timeout). func WithTimeout(d time.Duration) Option { - return func(c *Client) error { + return func(c *client) error { c.client.Timeout = d return nil } @@ -66,7 +79,7 @@ func WithTimeout(d time.Duration) Option { // WithTarget sets the device target label used in metrics. func WithTarget(target string) Option { - return func(c *Client) error { + return func(c *client) error { c.target = target return nil } @@ -74,7 +87,7 @@ func WithTarget(target string) Option { // NewClient creates a new [Client] for the given connection. // If the connection has a TLS configuration set, HTTPS is used; otherwise HTTP. -func NewClient(conn *deviceutil.Connection, opts ...Option) (*Client, error) { +func NewClient(conn *deviceutil.Connection, opts ...Option) (Client, error) { proto := "http" if conn.TLS != nil { proto = "https" @@ -83,7 +96,7 @@ func NewClient(conn *deviceutil.Connection, opts ...Option) (*Client, error) { if conn.TLS != nil { transport.TLSClientConfig = conn.TLS } - c := &Client{ + c := &client{ client: &http.Client{ Transport: RoundTripFunc(func(r *http.Request) (*http.Response, error) { r.Header.Set("Content-Type", "application/json-rpc") @@ -107,11 +120,28 @@ func NewClient(conn *deviceutil.Connection, opts ...Option) (*Client, error) { return c, nil } +// Clone returns a copy of the client with the given options applied, leaving +// the original untouched. The copy keeps the resolved endpoint URL and shares +// the underlying HTTP transport, so it stays reachable at the same address and +// reuses pooled connections. Use it to derive a client that differs only in +// request behaviour, e.g. a longer timeout for long-running commands. +func (c *client) Clone(opts ...Option) (Client, error) { + clone := *c + httpClient := *c.client + clone.client = &httpClient + for _, opt := range opts { + if err := opt(&clone); err != nil { + return nil, err + } + } + return &clone, nil +} + // Do sends a Request to the device and returns one [json.RawMessage] per // command, in the same order as the request. If any command fails, Do returns // an [RPCErrors] containing one [RPCError] per failed command; transport and // HTTP errors are returned directly. -func (c *Client) Do(ctx context.Context, r Request) ([]json.RawMessage, error) { +func (c *client) Do(ctx context.Context, r Request) ([]json.RawMessage, error) { b, err := r.Encode() if err != nil { return nil, fmt.Errorf("nxapi: failed to encode request: %w", err) @@ -203,6 +233,15 @@ func NewRequest(cmds ...string) Request { return r } +// Commands returns the CLI command strings in the request, in order. +func (r Request) Commands() []string { + cmds := make([]string, len(r)) + for i := range r { + cmds[i] = r[i].Params.Cmd + } + return cmds +} + // Method is the NX-API command type. type Method string diff --git a/internal/transport/nxapi/nxapi_test.go b/internal/transport/nxapi/nxapi_test.go index 525f5e85a..f181a0c68 100644 --- a/internal/transport/nxapi/nxapi_test.go +++ b/internal/transport/nxapi/nxapi_test.go @@ -41,10 +41,11 @@ func TestUri(t *testing.T) { } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - c, err := NewClient(test.conn) + got, err := NewClient(test.conn) if err != nil { t.Fatalf("unexpected error: %v", err) } + c := got.(*client) if c.url.Scheme != test.wantProto { t.Errorf("scheme = %q, want %q", c.url.Scheme, test.wantProto) }