Skip to content

Commit 6c0e70d

Browse files
zhangzujianzhangzujian
committed
test: strengthen BGP speaker route validation
Signed-off-by: Zujian Zhang <zhangzujian.7@gmail.com>
1 parent 14fb0c6 commit 6c0e70d

9 files changed

Lines changed: 250 additions & 102 deletions

File tree

‎.github/workflows/build-x86-image.yaml‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1815,8 +1815,13 @@ jobs:
18151815
E2E_NETWORK_MODE: overlay
18161816
run: make kube-ovn-bgp-speaker-e2e
18171817

1818+
- name: Check kube ovn pod restarts
1819+
id: check-restarts
1820+
if: ${{ success() || (failure() && (steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure')) }}
1821+
run: make check-kube-ovn-pod-restarts
1822+
18181823
- name: Collect BGP diagnostics
1819-
if: failure() && (steps.setup.conclusion == 'failure' || steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure')
1824+
if: failure() && (steps.setup.conclusion == 'failure' || steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure' || steps.check-restarts.conclusion == 'failure')
18201825
run: |
18211826
mkdir -p bgp-speaker-e2e-diagnostics
18221827
docker exec clab-bgp-router vtysh -c "show bgp ipv4 unicast summary json" > bgp-speaker-e2e-diagnostics/frr-summary.json || true
@@ -1830,25 +1835,24 @@ jobs:
18301835
18311836
- name: Upload BGP diagnostics
18321837
uses: actions/upload-artifact@v7
1833-
if: failure() && (steps.setup.conclusion == 'failure' || steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure')
1838+
if: failure() && (steps.setup.conclusion == 'failure' || steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure' || steps.check-restarts.conclusion == 'failure')
18341839
with:
18351840
name: bgp-speaker-e2e-diagnostics
18361841
path: bgp-speaker-e2e-diagnostics.tar.gz
18371842

1838-
- name: Check kube ovn pod restarts
1839-
id: check-restarts
1840-
if: ${{ success() || (failure() && (steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure')) }}
1841-
run: make check-kube-ovn-pod-restarts
1842-
18431843
- name: kubectl ko log
1844-
if: failure() && (steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure' || steps.check-restarts.conclusion == 'failure')
1844+
if: failure() && (steps.setup.conclusion == 'failure' || steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure' || steps.check-restarts.conclusion == 'failure')
18451845
run: |
1846-
make kubectl-ko-log
1847-
mv kubectl-ko-log.tar.gz bgp-speaker-e2e-ko-log.tar.gz
1846+
if make kubectl-ko-log && test -f kubectl-ko-log.tar.gz; then
1847+
mv kubectl-ko-log.tar.gz bgp-speaker-e2e-ko-log.tar.gz
1848+
else
1849+
echo "kubectl ko log was unavailable because the BGP test cluster did not become usable." > bgp-speaker-e2e-ko-log-unavailable.txt
1850+
tar zcf bgp-speaker-e2e-ko-log.tar.gz bgp-speaker-e2e-ko-log-unavailable.txt
1851+
fi
18481852
18491853
- name: Upload kubectl ko log
18501854
uses: actions/upload-artifact@v7
1851-
if: failure() && (steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure' || steps.check-restarts.conclusion == 'failure')
1855+
if: failure() && (steps.setup.conclusion == 'failure' || steps.install.conclusion == 'failure' || steps.e2e.conclusion == 'failure' || steps.check-restarts.conclusion == 'failure')
18521856
with:
18531857
name: bgp-speaker-e2e-ko-log
18541858
path: bgp-speaker-e2e-ko-log.tar.gz

‎makefiles/kind.mk‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -715,8 +715,6 @@ kind-install-bgp-ha: kind-install
715715
-e 's/--neighbor-as=.*/--neighbor-as=65001/' \
716716
-e 's/--cluster-as=.*/--cluster-as=65002/' yamls/speaker.yaml | \
717717
kubectl apply -f -
718-
kubectl -n kube-system patch ds kube-ovn-speaker --type=json \
719-
-p='[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--announce-cluster-ip=true"}]'
720718
kubectl -n kube-system rollout status ds kube-ovn-speaker --timeout 60s
721719
docker exec clab-bgp-router-1 vtysh -c "show ip route bgp"
722720
docker exec clab-bgp-router-2 vtysh -c "show ip route bgp"

‎pkg/speaker/bgp_test.go‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/osrg/gobgp/v4/pkg/packet/bgp"
1212
gobgp "github.com/osrg/gobgp/v4/pkg/server"
1313
"github.com/stretchr/testify/require"
14+
"github.com/vishvananda/netlink"
1415
"k8s.io/utils/set"
1516
)
1617

@@ -33,7 +34,7 @@ func newTestBgpServer(t *testing.T, routerID string) *gobgp.BgpServer {
3334
return server
3435
}
3536

36-
func listTestPrefixes(t *testing.T, server *gobgp.BgpServer, afi api.Family_Afi) map[string][]net.IP {
37+
func listTestPrefixNextHops(t *testing.T, server *gobgp.BgpServer, afi api.Family_Afi) map[string][]net.IP {
3738
t.Helper()
3839

3940
prefixes := map[string][]net.IP{}
@@ -69,15 +70,15 @@ func TestReconcileRoutesAddsAndWithdrawsIPv4Routes(t *testing.T) {
6970
require.NoError(t, controller.reconcileRoutes(prefixMap{
7071
api.Family_AFI_IP: set.New(prefix),
7172
}))
72-
routes := listTestPrefixes(t, controller.config.BgpServer, api.Family_AFI_IP)
73+
routes := listTestPrefixNextHops(t, controller.config.BgpServer, api.Family_AFI_IP)
7374
require.Contains(t, routes, prefix)
7475
require.Len(t, routes[prefix], 1)
7576
require.True(t, net.ParseIP(routerID).Equal(routes[prefix][0]))
7677

7778
require.NoError(t, controller.reconcileRoutes(prefixMap{
7879
api.Family_AFI_IP: set.New[string](),
7980
}))
80-
require.NotContains(t, listTestPrefixes(t, controller.config.BgpServer, api.Family_AFI_IP), prefix)
81+
require.NotContains(t, listTestPrefixNextHops(t, controller.config.BgpServer, api.Family_AFI_IP), prefix)
8182
}
8283

8384
func TestGetPathRequest(t *testing.T) {
@@ -204,12 +205,21 @@ func TestGetNextHopFromPathAttributes(t *testing.T) {
204205
}
205206
}
206207

207-
func TestGetNextHopAttributeUsesCachedLocalAddress(t *testing.T) {
208+
func TestGetNextHopAttributeReusesInitializedNeighborLocalAddress(t *testing.T) {
208209
const neighbor = "192.0.2.1"
209-
localAddress := net.ParseIP("192.0.2.10")
210-
controller := &Controller{config: &Configuration{
211-
NeighborLocalAddresses: map[string]net.IP{neighbor: localAddress},
212-
}}
210+
neighborAddress := net.ParseIP(neighbor)
211+
startupAddress := net.ParseIP("192.0.2.10")
212+
routeSource := startupAddress
213+
config := &Configuration{
214+
NeighborAddresses: []net.IP{neighborAddress},
215+
AllowedSourceAddresses: []net.IP{startupAddress},
216+
}
217+
require.NoError(t, config.initNeighborLocalAddressesWithRouteLookup(func(address net.IP) ([]netlink.Route, error) {
218+
require.True(t, neighborAddress.Equal(address))
219+
return []netlink.Route{{Src: routeSource}}, nil
220+
}))
213221

214-
require.True(t, localAddress.Equal(controller.getNextHopAttribute(net.ParseIP(neighbor))))
222+
routeSource = net.ParseIP("192.0.2.20")
223+
controller := &Controller{config: config}
224+
require.True(t, startupAddress.Equal(controller.getNextHopAttribute(neighborAddress)))
215225
}

‎pkg/speaker/config.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -335,11 +335,11 @@ func (config *Configuration) initKubeClient() error {
335335
}
336336

337337
func (config *Configuration) checkGracefulRestartOptions() error {
338-
if config.GracefulRestartTime > time.Second*4095 || config.GracefulRestartTime <= 0 {
339-
return errors.New("GracefulRestartTime should be less than 4095 seconds or more than 0")
338+
if config.GracefulRestartTime > 4095*time.Second || config.GracefulRestartTime < time.Second {
339+
return errors.New("GracefulRestartTime should be between 1 and 4095 seconds")
340340
}
341-
if config.GracefulRestartDeferralTime > time.Hour*18 || config.GracefulRestartDeferralTime <= 0 {
342-
return errors.New("GracefulRestartDeferralTime should be less than 18 hours or more than 0")
341+
if config.GracefulRestartDeferralTime > 18*time.Hour || config.GracefulRestartDeferralTime < time.Second {
342+
return errors.New("GracefulRestartDeferralTime should be between 1 second and 18 hours")
343343
}
344344

345345
return nil
@@ -554,13 +554,19 @@ func (config *Configuration) watchPeerState() {
554554
}
555555
}
556556

557+
type routeLookupFunc func(net.IP) ([]netlink.Route, error)
558+
557559
func (config *Configuration) initNeighborLocalAddresses() error {
560+
return config.initNeighborLocalAddressesWithRouteLookup(netlink.RouteGet)
561+
}
562+
563+
func (config *Configuration) initNeighborLocalAddressesWithRouteLookup(routeLookup routeLookupFunc) error {
558564
config.NeighborLocalAddresses = make(map[string]net.IP, len(config.NeighborAddresses)+len(config.NeighborIPv6Addresses))
559565

560566
for _, neighbor := range config.NeighborAddresses {
561567
if len(config.AllowedSourceAddresses) != 0 {
562568
klog.Infof("Resolving BGP local address for neighbor %s with allowed IPv4 source addresses %v", neighbor, config.AllowedSourceAddresses)
563-
localAddr, err := config.resolveWhitelistedNeighborLocalAddress(neighbor, config.AllowedSourceAddresses)
569+
localAddr, err := config.resolveWhitelistedNeighborLocalAddress(neighbor, config.AllowedSourceAddresses, routeLookup)
564570
if err != nil {
565571
return err
566572
}
@@ -571,7 +577,7 @@ func (config *Configuration) initNeighborLocalAddresses() error {
571577
for _, neighbor := range config.NeighborIPv6Addresses {
572578
if len(config.AllowedSourceIPv6Addresses) != 0 {
573579
klog.Infof("Resolving BGP local address for neighbor %s with allowed IPv6 source addresses %v", neighbor, config.AllowedSourceIPv6Addresses)
574-
localAddr, err := config.resolveWhitelistedNeighborLocalAddress(neighbor, config.AllowedSourceIPv6Addresses)
580+
localAddr, err := config.resolveWhitelistedNeighborLocalAddress(neighbor, config.AllowedSourceIPv6Addresses, routeLookup)
575581
if err != nil {
576582
return err
577583
}
@@ -598,8 +604,8 @@ func (config *Configuration) getNeighborLocalAddress(neighborAddress net.IP) net
598604
return nil
599605
}
600606

601-
func (config *Configuration) resolveWhitelistedNeighborLocalAddress(neighborAddress net.IP, allowedLocalAddresses []net.IP) (net.IP, error) {
602-
routes, err := netlink.RouteGet(neighborAddress)
607+
func (config *Configuration) resolveWhitelistedNeighborLocalAddress(neighborAddress net.IP, allowedLocalAddresses []net.IP, routeLookup routeLookupFunc) (net.IP, error) {
608+
routes, err := routeLookup(neighborAddress)
603609
if err != nil {
604610
return nil, fmt.Errorf("failed to determine local address for BGP neighbor %s from route lookup: %w", neighborAddress, err)
605611
}

‎pkg/speaker/config_additional_test.go‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,21 @@ func TestCheckGracefulRestartOptions(t *testing.T) {
1616
expectError string
1717
}{
1818
{
19-
name: "valid boundary values",
19+
name: "valid lower boundary values",
20+
restartTime: time.Second,
21+
deferralTime: time.Second,
22+
},
23+
{
24+
name: "valid upper boundary values",
2025
restartTime: 4095 * time.Second,
2126
deferralTime: 18 * time.Hour,
2227
},
28+
{
29+
name: "restart time below one second",
30+
restartTime: time.Second - time.Nanosecond,
31+
deferralTime: time.Minute,
32+
expectError: "GracefulRestartTime",
33+
},
2334
{
2435
name: "zero restart time",
2536
restartTime: 0,
@@ -32,6 +43,12 @@ func TestCheckGracefulRestartOptions(t *testing.T) {
3243
deferralTime: time.Minute,
3344
expectError: "GracefulRestartTime",
3445
},
46+
{
47+
name: "deferral time below one second",
48+
restartTime: time.Minute,
49+
deferralTime: time.Second - time.Nanosecond,
50+
expectError: "GracefulRestartDeferralTime",
51+
},
3552
{
3653
name: "zero deferral time",
3754
restartTime: time.Minute,

‎pkg/speaker/eip.go‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,6 @@ func (c *Controller) syncEIPRoutes() error {
4646

4747
// announceEIPs announce all the prefixes related to EIPs attached to a GW
4848
func (c *Controller) announceEIPs(eips []*v1.IptablesEIP) error {
49-
return c.reconcileRoutes(collectEIPExpectedPrefixes(eips))
50-
}
51-
52-
func collectEIPExpectedPrefixes(eips []*v1.IptablesEIP) prefixMap {
5349
expectedPrefixes := make(prefixMap)
5450
for _, eip := range eips {
5551
// Only announce EIPs marked as "ready" and with the BGP annotation set to true
@@ -66,5 +62,5 @@ func collectEIPExpectedPrefixes(eips []*v1.IptablesEIP) prefixMap {
6662
}
6763
}
6864

69-
return expectedPrefixes
65+
return c.reconcileRoutes(expectedPrefixes)
7066
}

‎pkg/speaker/eip_test.go‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package speaker
22

33
import (
4+
"net"
45
"testing"
56

67
"github.com/osrg/gobgp/v4/api"
@@ -11,7 +12,25 @@ import (
1112
"github.com/kubeovn/kube-ovn/pkg/util"
1213
)
1314

14-
func TestCollectEIPExpectedPrefixes(t *testing.T) {
15+
func TestAnnounceEIPsFiltersAndDeduplicatesPrefixes(t *testing.T) {
16+
const (
17+
routerID = "192.0.2.10"
18+
ipv4Neighbor = "192.0.2.1"
19+
ipv6NextHop = "2001:db8::10"
20+
ipv6Neighbor = "2001:db8::1"
21+
)
22+
23+
controller := &Controller{config: &Configuration{
24+
RouterID: net.ParseIP(routerID),
25+
NeighborAddresses: []net.IP{net.ParseIP(ipv4Neighbor)},
26+
NeighborIPv6Addresses: []net.IP{net.ParseIP(ipv6Neighbor)},
27+
NeighborLocalAddresses: map[string]net.IP{
28+
ipv4Neighbor: net.ParseIP(routerID),
29+
ipv6Neighbor: net.ParseIP(ipv6NextHop),
30+
},
31+
BgpServer: newTestBgpServer(t, routerID),
32+
}}
33+
1534
eips := []*kubeovnv1.IptablesEIP{
1635
{
1736
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{util.BgpAnnotation: "true"}},
@@ -39,9 +58,19 @@ func TestCollectEIPExpectedPrefixes(t *testing.T) {
3958
},
4059
}
4160

42-
prefixes := collectEIPExpectedPrefixes(eips)
43-
require.ElementsMatch(t, []string{"192.0.2.10/32"}, prefixes[api.Family_AFI_IP].UnsortedList())
44-
require.ElementsMatch(t, []string{"2001:db8::10/128"}, prefixes[api.Family_AFI_IP6].UnsortedList())
61+
require.NoError(t, controller.announceEIPs(eips))
62+
ipv4Prefixes := listTestPrefixNextHops(t, controller.config.BgpServer, api.Family_AFI_IP)
63+
require.Len(t, ipv4Prefixes, 1)
64+
require.Len(t, ipv4Prefixes["192.0.2.10/32"], 1)
65+
require.True(t, net.ParseIP(routerID).Equal(ipv4Prefixes["192.0.2.10/32"][0]))
66+
67+
ipv6Prefixes := listTestPrefixNextHops(t, controller.config.BgpServer, api.Family_AFI_IP6)
68+
require.Len(t, ipv6Prefixes, 1)
69+
require.Len(t, ipv6Prefixes["2001:db8::10/128"], 1)
70+
require.True(t, net.ParseIP(ipv6NextHop).Equal(ipv6Prefixes["2001:db8::10/128"][0]))
71+
72+
require.NoError(t, controller.announceEIPs(nil))
73+
require.Empty(t, listTestPrefixNextHops(t, controller.config.BgpServer, api.Family_AFI_IP))
4574
}
4675

4776
func TestSyncEIPRoutesRequiresGatewayName(t *testing.T) {

0 commit comments

Comments
 (0)