diff --git a/api/core/v1alpha1/bgp_peer_types.go b/api/core/v1alpha1/bgp_peer_types.go index c0c14110..8a3cf914 100644 --- a/api/core/v1alpha1/bgp_peer_types.go +++ b/api/core/v1alpha1/bgp_peer_types.go @@ -13,6 +13,7 @@ import ( ) // BGPPeerSpec defines the desired state of BGPPeer +// +kubebuilder:validation:XValidation:rule="!(has(self.localAS) && has(self.localASNumber))",message="localAS and localASNumber are mutually exclusive" type BGPPeerSpec struct { // DeviceName is the name of the Device this object belongs to. The Device object must exist in the same namespace. // Immutable. @@ -61,6 +62,13 @@ type BGPPeerSpec struct { // +optional AddressFamilies *BGPPeerAddressFamilies `json:"addressFamilies,omitempty"` + // LocalASNumber specifies a local AS number to present to the BGP peer. + // When set, it is equivalent to LocalAS with ASNumber set and PrependLocalAS/PrependGlobalAS defaults. + // + // Deprecated: Use LocalAS.ASNumber instead. This field will be removed in a future release. + // +optional + LocalASNumber *intstr.IntOrString `json:"localASNumber,omitempty"` + // LocalAS configures the local AS number and how it factors into BGP announcements for this peer. // +optional LocalAS *LocalAS `json:"localAS,omitempty"` diff --git a/api/core/v1alpha1/zz_generated.deepcopy.go b/api/core/v1alpha1/zz_generated.deepcopy.go index 5bcc315e..1d6104fe 100644 --- a/api/core/v1alpha1/zz_generated.deepcopy.go +++ b/api/core/v1alpha1/zz_generated.deepcopy.go @@ -561,6 +561,11 @@ func (in *BGPPeerSpec) DeepCopyInto(out *BGPPeerSpec) { *out = new(BGPPeerAddressFamilies) (*in).DeepCopyInto(*out) } + if in.LocalASNumber != nil { + in, out := &in.LocalASNumber, &out.LocalASNumber + *out = new(intstr.IntOrString) + **out = **in + } if in.LocalAS != nil { in, out := &in.LocalAS, &out.LocalAS *out = new(LocalAS) 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 ff32caaf..12b91173 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 @@ -355,6 +355,16 @@ spec: required: - asNumber type: object + localASNumber: + anyOf: + - type: integer + - type: string + description: |- + LocalASNumber specifies a local AS number to present to the BGP peer. + When set, it is equivalent to LocalAS with ASNumber set and PrependLocalAS/PrependGlobalAS defaults. + + Deprecated: Use LocalAS.ASNumber instead. This field will be removed in a future release. + x-kubernetes-int-or-string: true localAddress: description: |- LocalAddress specifies the local address configuration for the BGP session with this peer. @@ -420,6 +430,9 @@ spec: - bgpRef - deviceRef type: object + x-kubernetes-validations: + - message: localAS and localASNumber are mutually exclusive + rule: '!(has(self.localAS) && has(self.localASNumber))' status: description: |- Status of the resource. This is set and updated automatically. diff --git a/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml b/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml index 379cb55d..bf81da8c 100644 --- a/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml +++ b/config/crd/bases/networking.metal.ironcore.dev_bgppeers.yaml @@ -352,6 +352,16 @@ spec: required: - asNumber type: object + localASNumber: + anyOf: + - type: integer + - type: string + description: |- + LocalASNumber specifies a local AS number to present to the BGP peer. + When set, it is equivalent to LocalAS with ASNumber set and PrependLocalAS/PrependGlobalAS defaults. + + Deprecated: Use LocalAS.ASNumber instead. This field will be removed in a future release. + x-kubernetes-int-or-string: true localAddress: description: |- LocalAddress specifies the local address configuration for the BGP session with this peer. @@ -417,6 +427,9 @@ spec: - bgpRef - deviceRef type: object + x-kubernetes-validations: + - message: localAS and localASNumber are mutually exclusive + rule: '!(has(self.localAS) && has(self.localASNumber))' status: description: |- Status of the resource. This is set and updated automatically. diff --git a/docs/api-reference/index.md b/docs/api-reference/index.md index ea6a3e39..cae61154 100644 --- a/docs/api-reference/index.md +++ b/docs/api-reference/index.md @@ -491,6 +491,7 @@ _Appears in:_ | `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: \{\}
| +| `localASNumber` _[IntOrString](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.35/#intorstring-intstr-util)_ | LocalASNumber specifies a local AS number to present to the BGP peer.
When set, it is equivalent to LocalAS with ASNumber set and PrependLocalAS/PrependGlobalAS defaults.
Deprecated: Use LocalAS.ASNumber instead. This field will be removed in a future release. | | Optional: \{\}
| | `localAS` _[LocalAS](#localas)_ | LocalAS configures the local AS number and how it factors into BGP announcements for this peer. | | Optional: \{\}
| diff --git a/internal/controller/core/bgp_peer_controller.go b/internal/controller/core/bgp_peer_controller.go index 72ec7ee4..08b565b4 100644 --- a/internal/controller/core/bgp_peer_controller.go +++ b/internal/controller/core/bgp_peer_controller.go @@ -443,6 +443,16 @@ func (r *BGPPeerReconciler) reconcile(ctx context.Context, s *bgpPeerScope) (ret sourceInterface = intf.Spec.Name } + // TODO: remove use of deprecated LocalASNumber field in a future release. + //nolint:staticcheck // handling deprecated field for backward compatibility + if s.BGPPeer.Spec.LocalASNumber != nil { + s.BGPPeer.Spec.LocalAS = &v1alpha1.LocalAS{ + ASNumber: *s.BGPPeer.Spec.LocalASNumber, //nolint:staticcheck + PrependLocalAS: new(bool), + PrependGlobalAS: new(bool), + } + } + if s.BGPPeer.Spec.LocalAS != nil && s.BGPPeer.Spec.ASNumber.String() == bgp.Spec.ASNumber.String() { conditions.Set(s.BGPPeer, metav1.Condition{ Type: v1alpha1.ConfiguredCondition, diff --git a/internal/webhook/core/v1alpha1/bgppeer_webhook.go b/internal/webhook/core/v1alpha1/bgppeer_webhook.go index c45056a0..1004e221 100644 --- a/internal/webhook/core/v1alpha1/bgppeer_webhook.go +++ b/internal/webhook/core/v1alpha1/bgppeer_webhook.go @@ -35,14 +35,23 @@ var _ admission.Validator[*v1alpha1.BGPPeer] = &BGPPeerCustomValidator{} func (v *BGPPeerCustomValidator) ValidateCreate(_ context.Context, bgppeer *v1alpha1.BGPPeer) (admission.Warnings, error) { bgppeerlog.Info("Validation for BGPPeer upon creation", "name", bgppeer.GetName()) - return nil, validateBGPPeer(bgppeer.Spec) + var warnings admission.Warnings + if bgppeer.Spec.LocalASNumber != nil { //nolint:staticcheck + warnings = append(warnings, "spec.localASNumber is deprecated; use spec.localAS.asNumber instead") + } + + return warnings, validateBGPPeer(bgppeer.Spec) } // ValidateUpdate implements admission.Validator so a webhook will be registered for the type BGPPeer. func (v *BGPPeerCustomValidator) ValidateUpdate(_ context.Context, _, bgppeer *v1alpha1.BGPPeer) (admission.Warnings, error) { bgppeerlog.Info("Validation for BGPPeer upon update", "name", bgppeer.GetName()) - return nil, validateBGPPeer(bgppeer.Spec) + var warnings admission.Warnings + if bgppeer.Spec.LocalASNumber != nil { //nolint:staticcheck + warnings = append(warnings, "spec.localASNumber is deprecated; use spec.localAS.asNumber instead") + } + return warnings, validateBGPPeer(bgppeer.Spec) } // ValidateDelete implements admission.Validator so a webhook will be registered for the type BGPPeer. @@ -60,5 +69,12 @@ func validateBGPPeer(bgppeer v1alpha1.BGPPeerSpec) error { return err } } + // TODO: Remove when LocalASNumber is removed from the spec. + if bgppeer.LocalASNumber != nil { //nolint:staticcheck + if err := validateASNumber(*bgppeer.LocalASNumber); err != nil { //nolint:staticcheck + return err + } + } + return nil } diff --git a/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go b/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go index 5de5af5b..80704d45 100644 --- a/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go +++ b/internal/webhook/core/v1alpha1/bgppeer_webhook_test.go @@ -137,6 +137,149 @@ var _ = Describe("BGPPeer Webhook", func() { }) }) + Context("When creating BGPPeer with deprecated LocalASNumber", func() { + BeforeEach(func() { + // ASNumber is required for BGPPeer validation + obj.Spec.ASNumber = intstr.FromInt32(65001) + }) + + It("Should return deprecation warning when LocalASNumber is set", func() { + localAS := intstr.FromInt32(65001) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + Expect(warnings[0]).To(ContainSubstring("deprecated")) + }) + + It("Should not return warning when LocalASNumber is not set", func() { + obj.Spec.LocalASNumber = nil //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + }) + + It("Should admit creation with valid integer LocalASNumber and return warning", func() { + localAS := intstr.FromInt32(65001) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit creation with valid string LocalASNumber in plain format and return warning", func() { + localAS := intstr.FromString("4294967295") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit creation with valid string LocalASNumber in dotted notation and return warning", func() { + localAS := intstr.FromString("65000.100") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit creation with minimum valid integer LocalASNumber and return warning", func() { + localAS := intstr.FromInt32(1) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit creation with maximum valid integer LocalASNumber and return warning", func() { + localAS := intstr.FromInt32(2147483647) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should deny creation with zero LocalASNumber", func() { + localAS := intstr.FromInt32(0) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with negative LocalASNumber", func() { + localAS := intstr.FromInt32(-1) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with string LocalASNumber exceeding uint32 max", func() { + localAS := intstr.FromString("4294967296") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with invalid string LocalASNumber", func() { + localAS := intstr.FromString("invalid") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with invalid dotted notation LocalASNumber - too many parts", func() { + localAS := intstr.FromString("1.2.3") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with invalid dotted notation LocalASNumber - high part out of range", func() { + localAS := intstr.FromString("65536.100") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with invalid dotted notation LocalASNumber - low part out of range", func() { + localAS := intstr.FromString("100.65536") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny creation with invalid dotted notation LocalASNumber - high part is zero", func() { + localAS := intstr.FromString("0.100") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should admit creation with dotted notation LocalASNumber - low part is zero and return warning", func() { + localAS := intstr.FromString("100.0") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit creation with dotted notation LocalASNumber at boundary values and return warning", func() { + localAS := intstr.FromString("65535.65535") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit creation with dotted notation LocalASNumber at minimum valid values and return warning", func() { + localAS := intstr.FromString("1.0") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + }) + Context("When updating BGPPeer under Validating Webhook", func() { It("Should admit update with valid AS number", func() { oldObj.Spec.ASNumber = intstr.FromInt32(65001) @@ -167,6 +310,141 @@ var _ = Describe("BGPPeer Webhook", func() { }) }) + Context("When updating BGPPeer with deprecated LocalASNumber", func() { + It("Should return deprecation warning when LocalASNumber is set", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromInt32(65100) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + Expect(warnings[0]).To(ContainSubstring("deprecated")) + }) + + It("Should admit update with valid integer LocalASNumber and return warning", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromInt32(65100) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit update with valid string LocalASNumber in plain format and return warning", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("4294967295") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit update with valid string LocalASNumber in dotted notation and return warning", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("65000.100") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should deny update with zero LocalASNumber", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromInt32(0) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with negative LocalASNumber", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromInt32(-1) + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with string LocalASNumber exceeding uint32 max", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("4294967296") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with invalid string LocalASNumber", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("invalid") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with invalid dotted notation LocalASNumber - too many parts", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("1.2.3") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with invalid dotted notation LocalASNumber - high part out of range", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("65536.100") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with invalid dotted notation LocalASNumber - low part out of range", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("100.65536") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should deny update with invalid dotted notation LocalASNumber - high part is zero", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("0.100") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("Should admit update with dotted notation LocalASNumber - low part is zero and return warning", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("100.0") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + + It("Should admit update with dotted notation LocalASNumber at boundary values and return warning", func() { + oldObj.Spec.ASNumber = intstr.FromInt32(65001) + obj.Spec.ASNumber = intstr.FromInt32(65001) + localAS := intstr.FromString("65535.65535") + obj.Spec.LocalASNumber = &localAS //nolint:staticcheck + warnings, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(HaveLen(1)) + }) + }) + Context("When deleting BGPPeer under Validating Webhook", func() { It("Should admit deletion", func() { _, err := validator.ValidateDelete(ctx, obj)