feat: Implement BFD for BGPPeers#438
Draft
sven-rosenzweig wants to merge 1 commit into
Draft
Conversation
sven-rosenzweig
force-pushed
the
feat/bfd_for_bgppeer
branch
from
169ea1e to
3400add
Compare
Signed-off-by: Sven Rosenzweig <sven.rosenzweig@sap.com>
sven-rosenzweig
force-pushed
the
feat/bfd_for_bgppeer
branch
from
3400add to
37e3c78
Compare
Comment on lines
+4
to
+7
| // Package v1alpha1 contains API Schema definitions for the networking.metal.ironcore.dev v1alpha1 API group. | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:object:generate=true | ||
| // +groupName=networking.metal.ironcore.dev |
Contributor
There was a problem hiding this comment.
These comments shouldn't be here. They are only meant to be there once for the entire package, which doc.go already does.
Comment on lines
+68
to
+69
| // BFD defines the Bidirectional Forwarding Detection configuration for the interface. | ||
| // BFD is only applicable for Layer 3 interfaces. |
Contributor
There was a problem hiding this comment.
Interface vs. BGPPeer. Probably just copy+paste.
| MinTxIntvlMs uint32 `json:"minTxMs"` | ||
| } | ||
|
|
||
| func NewBGPNeighborBfd(peer *v1alpha1.BGPPeerSpec) (*BGPNeighborBfd, error) { |
Contributor
There was a problem hiding this comment.
It is weird to only pass the spec here. Either I would pass the whole *v1alpha1.BGPPeer object, or otherwise, just a *v1alpha.BFD (.spec.bfd).
| SrcIf string `json:"srcIf,omitempty"` | ||
|
|
||
| // BFD enablement | ||
| PeerControl string `json:"ctrl"` |
Contributor
There was a problem hiding this comment.
I don't see this field at all in the YANG model. Should we delete it?
|
|
||
| // BFD enablement | ||
| PeerControl string `json:"ctrl"` | ||
| // Bfd Type: none (default), single-hop, multi-hop |
Contributor
There was a problem hiding this comment.
I would create a proper enum for these string values instead of documenting supported values in a comment.
| } | ||
| if peer.BFD.RequiredMinimumReceive != nil { | ||
| ms := peer.BFD.RequiredMinimumReceive.Duration | ||
|
|
Contributor
There was a problem hiding this comment.
Suggested change
weird formatting.
Comment on lines
+418
to
+424
| const ( | ||
| // PeerControlBFD enables BFD on BGP peer | ||
| PeerControlBFD = "bfd" | ||
| // BfdTypeNone relies on default BFD session type selection | ||
| BfdTypeNone = "none" | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
I would make this a proper enum instead of constants:
type PeerControlType string
const (
PeerControlTypeBFD PeerControlType = "bfd"
PeerControlTypeNone PeerControlType = "none"
)
Comment on lines
+542
to
+552
|
|
||
| if req.BGPPeer.Spec.BFD != nil && req.BGPPeer.Spec.BFD.Enabled { | ||
| bfd, err := NewBGPNeighborBfd(&req.BGPPeer.Spec) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| pe.PeerControl = PeerControlBFD | ||
| pe.BfdType = BfdTypeNone | ||
| pe.BfdMultihop = bfd | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Suggested change
| if req.BGPPeer.Spec.BFD != nil && req.BGPPeer.Spec.BFD.Enabled { | |
| bfd, err := NewBGPNeighborBfd(&req.BGPPeer.Spec) | |
| if err != nil { | |
| return err | |
| } | |
| pe.PeerControl = PeerControlBFD | |
| pe.BfdType = BfdTypeNone | |
| pe.BfdMultihop = bfd | |
| } | |
| pe.BfdType = BfdTypeNone | |
| if req.BGPPeer.Spec.BFD != nil && req.BGPPeer.Spec.BFD.Enabled { | |
| bfd, err := NewBGPNeighborBfd(&req.BGPPeer.Spec) | |
| if err != nil { | |
| return err | |
| } | |
| pe.PeerControl = PeerControlBFD | |
| pe.BfdType = "multihop" | |
| pe.BfdMultihop = bfd | |
| } |
we have to set the default values unconditionally, otherwise we risk trigger a diff → update cycle on every reconcile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.