Add unnumbered BGP peering for frrk8s backend #2605
Conversation
karampok
force-pushed
the
unnumbered-frrk8s
branch
7 times, most recently
from
2609f31 to
e5b79ad
Compare
karampok
force-pushed
the
unnumbered-frrk8s
branch
5 times, most recently
from
eaa72e2 to
0591413
Compare
karampok
force-pushed
the
unnumbered-frrk8s
branch
from
0591413 to
be27981
Compare
There was a problem hiding this comment.
are these changes related to unnumbered? if not, can you put it in a different commit?
There was a problem hiding this comment.
yes it is a refactor on host:port (split out the port) that forced to do the change in native.go
internal/bgp/native/native.go
Outdated
| @@ -100,7 +100,8 @@ func (sm *sessionManager) SetEventCallback(func(interface{})) {} | |||
|
|
|||
| // run tries to stay connected to the peer, and pumps route updates to it. | |||
| func (s *session) run() { | |||
| defer stats.DeleteSession(s.PeerAddress) | |||
| label := fmt.Sprintf("%s:%d", s.PeerAddress, s.PeerPort) | |||
There was a problem hiding this comment.
nit: label->peerLabel, have this string returned from a function (peerLabelFor)
There was a problem hiding this comment.
Done I think, at lease I add a method (so I suppose the For in the name looks not great)
internal/config/config_test.go
Outdated
| }, | ||
| }, | ||
| { | ||
| desc: "unnumbered peer with address without interface ok", |
There was a problem hiding this comment.
this seems like the previous one (interface only), also I guess this scenario can be removed as it should be all over the tests by now
There was a problem hiding this comment.
Do you mean this is the normal way, so it does not add value?
| @@ -226,14 +225,20 @@ func (c *bgpController) syncPeers(l log.Logger) error { | |||
| } else if p.session == nil && shouldRun { | |||
| // Session doesn't exist, but should be running. Create | |||
| // it. | |||
| level.Info(l).Log("event", "peerAdded", "peer", p.cfg.Addr, "msg", "peer configured, starting BGP session") | |||
There was a problem hiding this comment.
I think we need to change the p.cfg.Addr in the rest of the places (+wrap this id in a function)
internal/bgp/frr/frr.go
Outdated
| host, port, err := net.SplitHostPort(s.PeerAddress) | ||
| if err != nil { | ||
| return nil, err | ||
| if s.PeerPort == 0 { |
There was a problem hiding this comment.
can we block this sooner when the config is parsed?
internal/bgp/frrk8s/frrk8s.go
Outdated
| func sessionName(s session) string { | ||
| baseName := fmt.Sprintf("%d@%s-%d@%s", s.PeerASN, s.PeerAddress, s.MyASN, s.SourceAddress) | ||
| f := func(ASN uint32, dynamicASN string) string { |
There was a problem hiding this comment.
can we just use the contents of this function to set asn? this is a bit confusing, especially since it's used once
internal/bgp/frrk8s/frrk8s.go
Outdated
| if !reflect.DeepEqual(s.PasswordRef, corev1.SecretReference{}) && s.Password != "" { | ||
| return fmt.Errorf("invalid session with password and secret set: %s", sessionName(*s)) | ||
| } | ||
|
|
||
| if s.PeerPort == 0 { |
There was a problem hiding this comment.
you mean validate eariler?
I think we should change api validation
// Port to dial when establishing the session.
// +optional
// +kubebuilder:validation:Minimum=0
// +kubebuilder:validation:Maximum=16384
// +kubebuilder:default:=179
Port uint16 `json:"peerPort,omitempty"`
and remove validation,
There was a problem hiding this comment.
yeah I mean block it from ever getting to this situation (blocking the bad config earlier, not calling the session code with port 0).
e2etest/bgptests/dump.go
Outdated
| "go.universe.tf/e2etest/pkg/metallb" | ||
| clientset "k8s.io/client-go/kubernetes" | ||
| ) | ||
|
|
||
| func dumpBGPPeers(basePath, testName string, containers []*frrcontainer.FRR) { |
There was a problem hiding this comment.
why do we need this function (what's the difference between this and dumpBGPInfo)?
There was a problem hiding this comment.
The dupBGPPeers is doing the same as bumpBGPInfo but is not dumping the standard/hardcoded FRRContainers and is not dumping the speakerPods.
There was a problem hiding this comment.
so when do we want to use this and not the other?
e2etest/bgptests/unnummbered.go
Outdated
| ) | ||
|
|
||
| var FRRImage string | ||
| var _ = ginkgo.Describe("IPV4 FRRK8S-MODE Unnumbered BGP", func() { |
There was a problem hiding this comment.
I am working towards on all stacks
e2etest/bgptests/unnummbered.go
Outdated
| // peerLLA, err := netdev.LinkLocalAddressForDevice(externalP2PContainer, iface) | ||
| // Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Kind of, might be needed for the "learning route" part == if the external router adv routes I should check the local route for that.
(keep working on that)
There was a problem hiding this comment.
Given I configure using the BGPPeer CR with Interface, adding the route learning is not possible (I think)
I clean up all related code.
|
|
||
| ginkgo.By(fmt.Sprintf("validating the p2p peer %s received the routes from node", externalP2PContainer.Name)) | ||
| validatePrefixViaFor(externalP2PContainer, iface, nodeLLA, prefixSend) | ||
|
|
There was a problem hiding this comment.
is it possible to check that the external client can reach the service?
There was a problem hiding this comment.
I could investigate but why do you think it worth? I mean having the route on the peer is all what metallb does, if works or not is not something that depends on our changes. Is not trivial because I will need to modify the routes in the host to return traffic in the right secondary interface
There was a problem hiding this comment.
fair enough, we can keep it this way
e2etest/bgptests/unnummbered.go
Outdated
| ToAdvertiseV4: []string{"1.1.1.0/24"}, | ||
| ToAdvertiseV6: []string{"1111::/64"}, |
There was a problem hiding this comment.
Was supposed to be part of the route learning but removed.
e2etest/bgptests/unnummbered.go
Outdated
| return err | ||
| } | ||
| for _, n := range neighbors { | ||
| if n.BGPNeighborAddr == nodeLLA && n.Connected { //&& n.BFDInfo.Status == "Up" { |
There was a problem hiding this comment.
i willl add it, we have the e2e in frr-k8s doing BFD
e2etest/bgptests/unnummbered.go
Outdated
| ingressIP := jigservice.GetIngressPoint(&i) | ||
| err = config.ValidateIPInRange(resources.Pools, ingressIP) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| } |
There was a problem hiding this comment.
let's verify that the service got the prefixSend (it makes the test clearer)
There was a problem hiding this comment.
Given that prefixSend includes mask /32, /128, doing that check the result does not look cleaner.
IMO we can let that as is because the real check is following. I take another look once I have the per stack test
There was a problem hiding this comment.
Added the check we can keep that if you like it more (or revert)
karampok
force-pushed
the
unnumbered-frrk8s
branch
from
b0b0cf7 to
b479b21
Compare
The plan is to proceed next to support that in the frr backend AFAIK, not sure worth spliting the validation. |
karampok
force-pushed
the
unnumbered-frrk8s
branch
3 times, most recently
from
6ea524d to
29aa697
Compare
karampok
force-pushed
the
unnumbered-frrk8s
branch
4 times, most recently
from
a617ae6 to
4ef8d5b
Compare
e2etest/bgptests/dump.go
Outdated
| "go.universe.tf/e2etest/pkg/metallb" | ||
| clientset "k8s.io/client-go/kubernetes" | ||
| ) | ||
|
|
||
| func dumpBGPPeers(basePath, testName string, containers []*frrcontainer.FRR) { |
There was a problem hiding this comment.
so when do we want to use this and not the other?
| @@ -54,6 +54,9 @@ func DiscardFRROnly(c ClusterResources) error { | |||
| if p.Spec.DynamicASN != "" { | |||
| return fmt.Errorf("peer %s has dynamicASN set on native bgp mode", p.Spec.Address) | |||
| } | |||
| if p.Spec.Interface != "" { | |||
There was a problem hiding this comment.
can you please add a case for this in TestValidate?
speaker/bgp_controller.go
Outdated
| @@ -60,6 +59,15 @@ type peer struct { | |||
| session bgp.Session | |||
| } | |||
|
|
|||
| func (p peer) id() string { | |||
| ret := "" | |||
There was a problem hiding this comment.
can this be ret := p.cfg.Iface, if the addr is not nil then ret = p.cfg.Addr.String() and return ret?
speaker/bgp_controller.go
Outdated
| if p.cfg.Addr != nil { | ||
| peerAddr = p.cfg.Addr.String() | ||
| } | ||
| level.Info(l).Log("event", "peerAdded", "peer", peerAddr+p.cfg.Iface, "msg", "peer configured, starting BGP session") |
There was a problem hiding this comment.
move this back up (using the id func)
internal/bgp/native/native.go
Outdated
| @@ -98,9 +98,14 @@ func (sm *sessionManager) SyncExtraInfo(extras string) error { | |||
|
|
|||
| func (sm *sessionManager) SetEventCallback(func(interface{})) {} | |||
|
|
|||
| func (s *session) PeerLabelFor() string { | |||
There was a problem hiding this comment.
yeah the "for" can be removed (in response to your comment 😅)
internal/bgp/frrk8s/frrk8s.go
Outdated
| if v := s.DynamicASN; v != "" { | ||
| asn = v | ||
| } |
There was a problem hiding this comment.
nit: if s.DynamicASN != "" and asn = s.DynamicASN looks cleaner
There was a problem hiding this comment.
(not sure) but done
internal/bgp/frrk8s/frrk8s.go
Outdated
| } | ||
|
|
||
| peer := s.PeerAddress | ||
| if v := s.PeerInterface; v != "" { |
karampok
force-pushed
the
unnumbered-frrk8s
branch
3 times, most recently
from
57ddd7b to
d0b0af2
Compare
Signed-off-by: karampok <karampok@gmail.com>
- Add 'interface' field to BGPPeer CRD that specify network interface for unnumbered peering - Make 'peerAddress' and 'interface' mutually exclusive Signed-off-by: karampok <karampok@gmail.com>
karampok
force-pushed
the
unnumbered-frrk8s
branch
from
d0b0af2 to
91c71e9
Compare
/kind feature
What this PR does / why we need it:
Special notes for your reviewer:
Release note: