Skip to content

Commit fa7bcde

Browse files
aaurenaauren
committed
fix(bgp_policies_test.go): use startBgpServer()
Use startBgpServer() rather than doing things individually, so that we can follow the logic path of how kube-router actually works better. This allows us to use annotations rather than set stuff manually and allows us to test more of the code-path of the NRC. Additionally, this change allows us to actually test some errors better such as, make sure that startBgpServer() actually throws the error we expect when only one part of the prepend ASN annotation is present. Previously, we were not actually testing this code path.
1 parent a5d6560 commit fa7bcde

File tree

1 file changed

+70
-38
lines changed

1 file changed

+70
-38
lines changed

‎pkg/controllers/routing/bgp_policies_test.go

Lines changed: 70 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package routing
22

33
import (
44
"context"
5+
"fmt"
56
"reflect"
67
"testing"
78
"time"
@@ -26,7 +27,8 @@ type PolicyTestCase struct {
2627
allPeerDefinedSet *gobgpapi.DefinedSet
2728
exportPolicyStatements []*gobgpapi.Statement
2829
importPolicyStatements []*gobgpapi.Statement
29-
err error
30+
addPolicyErr error
31+
startBGPServerErr error
3032
}
3133

3234
func Test_AddPolicies(t *testing.T) {
@@ -36,6 +38,8 @@ func Test_AddPolicies(t *testing.T) {
3638
&NetworkRoutingController{
3739
clientset: fake.NewSimpleClientset(),
3840
hostnameOverride: "node-1",
41+
routerID: "10.0.0.0",
42+
bgpPort: 10000,
3943
bgpFullMeshMode: false,
4044
bgpEnableInternal: true,
4145
bgpServer: gobgp.NewBgpServer(),
@@ -121,6 +125,7 @@ func Test_AddPolicies(t *testing.T) {
121125
MatchType: gobgpapi.MatchType_ANY,
122126
Name: "iBGPpeerset",
123127
},
128+
RpkiResult: -1,
124129
},
125130
Actions: &gobgpapi.Actions{
126131
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -139,19 +144,23 @@ func Test_AddPolicies(t *testing.T) {
139144
MatchType: gobgpapi.MatchType_ANY,
140145
Name: "allpeerset",
141146
},
147+
RpkiResult: -1,
142148
},
143149
Actions: &gobgpapi.Actions{
144150
RouteAction: gobgpapi.RouteAction_REJECT,
145151
},
146152
},
147153
},
148154
nil,
155+
nil,
149156
},
150157
{
151158
"has nodes, services with external peers",
152159
&NetworkRoutingController{
153160
clientset: fake.NewSimpleClientset(),
154161
hostnameOverride: "node-1",
162+
routerID: "10.0.0.0",
163+
bgpPort: 10000,
155164
bgpFullMeshMode: false,
156165
bgpEnableInternal: true,
157166
bgpServer: gobgp.NewBgpServer(),
@@ -253,6 +262,7 @@ func Test_AddPolicies(t *testing.T) {
253262
MatchType: gobgpapi.MatchType_ANY,
254263
Name: "iBGPpeerset",
255264
},
265+
RpkiResult: -1,
256266
},
257267
Actions: &gobgpapi.Actions{
258268
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -269,6 +279,7 @@ func Test_AddPolicies(t *testing.T) {
269279
MatchType: gobgpapi.MatchType_ANY,
270280
Name: "externalpeerset",
271281
},
282+
RpkiResult: -1,
272283
},
273284
Actions: &gobgpapi.Actions{
274285
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -287,19 +298,23 @@ func Test_AddPolicies(t *testing.T) {
287298
MatchType: gobgpapi.MatchType_ANY,
288299
Name: "allpeerset",
289300
},
301+
RpkiResult: -1,
290302
},
291303
Actions: &gobgpapi.Actions{
292304
RouteAction: gobgpapi.RouteAction_REJECT,
293305
},
294306
},
295307
},
296308
nil,
309+
nil,
297310
},
298311
{
299312
"has nodes, services with external peers and iBGP disabled",
300313
&NetworkRoutingController{
301314
clientset: fake.NewSimpleClientset(),
302315
hostnameOverride: "node-1",
316+
routerID: "10.0.0.0",
317+
bgpPort: 10000,
303318
bgpFullMeshMode: false,
304319
bgpEnableInternal: false,
305320
bgpServer: gobgp.NewBgpServer(),
@@ -401,6 +416,7 @@ func Test_AddPolicies(t *testing.T) {
401416
MatchType: gobgpapi.MatchType_ANY,
402417
Name: "externalpeerset",
403418
},
419+
RpkiResult: -1,
404420
},
405421
Actions: &gobgpapi.Actions{
406422
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -419,24 +435,25 @@ func Test_AddPolicies(t *testing.T) {
419435
MatchType: gobgpapi.MatchType_ANY,
420436
Name: "allpeerset",
421437
},
438+
RpkiResult: -1,
422439
},
423440
Actions: &gobgpapi.Actions{
424441
RouteAction: gobgpapi.RouteAction_REJECT,
425442
},
426443
},
427444
},
428445
nil,
446+
nil,
429447
},
430448
{
431449
"prepends AS with external peers",
432450
&NetworkRoutingController{
433451
clientset: fake.NewSimpleClientset(),
434452
hostnameOverride: "node-1",
453+
routerID: "10.0.0.0",
454+
bgpPort: 10000,
435455
bgpEnableInternal: true,
436456
bgpFullMeshMode: false,
437-
pathPrepend: true,
438-
pathPrependCount: 5,
439-
pathPrependAS: "65100",
440457
bgpServer: gobgp.NewBgpServer(),
441458
activeNodes: make(map[string]bool),
442459
podCidr: "172.20.0.0/24",
@@ -459,7 +476,9 @@ func Test_AddPolicies(t *testing.T) {
459476
ObjectMeta: metav1.ObjectMeta{
460477
Name: "node-1",
461478
Annotations: map[string]string{
462-
"kube-router.io/node.asn": "100",
479+
"kube-router.io/node.asn": "100",
480+
"kube-router.io/path-prepend.as": "65100",
481+
"kube-router.io/path-prepend.repeat-n": "5",
463482
},
464483
},
465484
Status: v1core.NodeStatus{
@@ -536,6 +555,7 @@ func Test_AddPolicies(t *testing.T) {
536555
MatchType: gobgpapi.MatchType_ANY,
537556
Name: "iBGPpeerset",
538557
},
558+
RpkiResult: -1,
539559
},
540560
Actions: &gobgpapi.Actions{
541561
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -552,6 +572,7 @@ func Test_AddPolicies(t *testing.T) {
552572
MatchType: gobgpapi.MatchType_ANY,
553573
Name: "externalpeerset",
554574
},
575+
RpkiResult: -1,
555576
},
556577
Actions: &gobgpapi.Actions{
557578
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -574,23 +595,25 @@ func Test_AddPolicies(t *testing.T) {
574595
MatchType: gobgpapi.MatchType_ANY,
575596
Name: "allpeerset",
576597
},
598+
RpkiResult: -1,
577599
},
578600
Actions: &gobgpapi.Actions{
579601
RouteAction: gobgpapi.RouteAction_REJECT,
580602
},
581603
},
582604
},
583605
nil,
606+
nil,
584607
},
585608
{
586609
"only prepends AS when both node annotations are present",
587610
&NetworkRoutingController{
588611
clientset: fake.NewSimpleClientset(),
589612
hostnameOverride: "node-1",
613+
routerID: "10.0.0.0",
614+
bgpPort: 10000,
590615
bgpEnableInternal: true,
591616
bgpFullMeshMode: false,
592-
pathPrepend: false,
593-
pathPrependAS: "65100",
594617
bgpServer: gobgp.NewBgpServer(),
595618
activeNodes: make(map[string]bool),
596619
podCidr: "172.20.0.0/24",
@@ -613,7 +636,8 @@ func Test_AddPolicies(t *testing.T) {
613636
ObjectMeta: metav1.ObjectMeta{
614637
Name: "node-1",
615638
Annotations: map[string]string{
616-
"kube-router.io/node.asn": "100",
639+
"kube-router.io/node.asn": "100",
640+
"kube-router.io/path-prepend.as": "65100",
617641
},
618642
},
619643
Status: v1core.NodeStatus{
@@ -690,6 +714,7 @@ func Test_AddPolicies(t *testing.T) {
690714
MatchType: gobgpapi.MatchType_ANY,
691715
Name: "iBGPpeerset",
692716
},
717+
RpkiResult: -1,
693718
},
694719
Actions: &gobgpapi.Actions{
695720
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -706,6 +731,7 @@ func Test_AddPolicies(t *testing.T) {
706731
MatchType: gobgpapi.MatchType_ANY,
707732
Name: "externalpeerset",
708733
},
734+
RpkiResult: -1,
709735
},
710736
Actions: &gobgpapi.Actions{
711737
RouteAction: gobgpapi.RouteAction_ACCEPT,
@@ -728,44 +754,46 @@ func Test_AddPolicies(t *testing.T) {
728754
MatchType: gobgpapi.MatchType_ANY,
729755
Name: "allpeerset",
730756
},
757+
RpkiResult: -1,
731758
},
732759
Actions: &gobgpapi.Actions{
733760
RouteAction: gobgpapi.RouteAction_REJECT,
734761
},
735762
},
736763
},
737764
nil,
765+
fmt.Errorf("both %s and %s must be set", pathPrependASNAnnotation, pathPrependRepeatNAnnotation),
738766
},
739767
}
740768

741769
for _, testcase := range testcases {
742770
t.Run(testcase.name, func(t *testing.T) {
743-
go testcase.nrc.bgpServer.Serve()
744-
global := &gobgpapi.Global{
745-
As: 1,
746-
RouterId: "10.0.0.0",
747-
ListenPort: 10000,
748-
}
749-
err := testcase.nrc.bgpServer.StartBgp(context.Background(), &gobgpapi.StartBgpRequest{Global: global})
750-
if err != nil {
751-
t.Fatalf("failed to start BGP server: %v", err)
752-
}
753-
defer func() {
754-
if err := testcase.nrc.bgpServer.StopBgp(context.Background(), &gobgpapi.StopBgpRequest{}); err != nil {
755-
t.Fatalf("failed to stop BGP server : %s", err)
756-
}
757-
}()
758-
759771
startInformersForRoutes(testcase.nrc, testcase.nrc.clientset)
760772

761-
if err = createNodes(testcase.nrc.clientset, testcase.existingNodes); err != nil {
773+
if err := createNodes(testcase.nrc.clientset, testcase.existingNodes); err != nil {
762774
t.Errorf("failed to create existing nodes: %v", err)
763775
}
764776

765-
if err = createServices(testcase.nrc.clientset, testcase.existingServices); err != nil {
777+
if err := createServices(testcase.nrc.clientset, testcase.existingServices); err != nil {
766778
t.Errorf("failed to create existing nodes: %v", err)
767779
}
768780

781+
err := testcase.nrc.startBgpServer(false)
782+
if !reflect.DeepEqual(err, testcase.startBGPServerErr) {
783+
t.Logf("expected err when invoking startBGPServer(): %v", testcase.startBGPServerErr)
784+
t.Logf("actual err from startBGPServer() received: %v", err)
785+
t.Error("unexpected error")
786+
}
787+
// If the server was not expected to start we should stop here as the rest of the tests are unimportant
788+
if testcase.startBGPServerErr != nil {
789+
return
790+
}
791+
defer func() {
792+
if err := testcase.nrc.bgpServer.StopBgp(context.Background(), &gobgpapi.StopBgpRequest{}); err != nil {
793+
t.Fatalf("failed to stop BGP server : %s", err)
794+
}
795+
}()
796+
769797
// ClusterIPs and ExternalIPs
770798
waitForListerWithTimeout(testcase.nrc.svcLister, time.Second*10, t)
771799

@@ -777,21 +805,25 @@ func Test_AddPolicies(t *testing.T) {
777805
nodeInformer := informerFactory.Core().V1().Nodes().Informer()
778806
testcase.nrc.nodeLister = nodeInformer.GetIndexer()
779807
err = testcase.nrc.AddPolicies()
780-
if !reflect.DeepEqual(err, testcase.err) {
781-
t.Logf("expected err %v", testcase.err)
782-
t.Logf("actual err %v", err)
808+
if !reflect.DeepEqual(err, testcase.addPolicyErr) {
809+
t.Logf("expected err when invoking AddPolicies(): %v", testcase.addPolicyErr)
810+
t.Logf("actual err from AddPolicies() received: %v", err)
783811
t.Error("unexpected error")
784812
}
813+
// If we expect AddPolicies() to fail we should stop here as there is no point in further evaluating policies
814+
if testcase.addPolicyErr != nil {
815+
return
816+
}
785817

786-
err = testcase.nrc.bgpServer.ListDefinedSet(context.Background(), &gobgpapi.ListDefinedSetRequest{
787-
DefinedType: gobgpapi.DefinedType_PREFIX,
788-
Name: "podcidrdefinedset"}, func(podDefinedSet *gobgpapi.DefinedSet) {
789-
if !reflect.DeepEqual(podDefinedSet, testcase.podDefinedSet) {
790-
t.Logf("expected pod defined set: %+v", testcase.podDefinedSet)
791-
t.Logf("actual pod defined set: %+v", podDefinedSet)
792-
t.Error("unexpected pod defined set")
793-
}
794-
})
818+
err = testcase.nrc.bgpServer.ListDefinedSet(context.Background(),
819+
&gobgpapi.ListDefinedSetRequest{DefinedType: gobgpapi.DefinedType_PREFIX, Name: "podcidrdefinedset"},
820+
func(podDefinedSet *gobgpapi.DefinedSet) {
821+
if !reflect.DeepEqual(podDefinedSet, testcase.podDefinedSet) {
822+
t.Logf("expected pod defined set: %+v", testcase.podDefinedSet)
823+
t.Logf("actual pod defined set: %+v", podDefinedSet)
824+
t.Error("unexpected pod defined set")
825+
}
826+
})
795827
if err != nil {
796828
t.Fatalf("error validating defined sets: %v", err)
797829
}

0 commit comments

Comments
 (0)