Skip to content

Commit 5a5b2e4

Browse files
majst01majst01
authored
Remove hardcoded /32 bitmask for some switch entities (#555)
1 parent d5e5fcf commit 5a5b2e4

File tree

2 files changed

+80
-36
lines changed

2 files changed

+80
-36
lines changed

‎cmd/metal-api/internal/service/switch-service.go‎

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"log/slog"
77
"net/http"
8+
"net/netip"
89
"sort"
910
"strconv"
1011
"strings"
@@ -776,31 +777,38 @@ func makeSwitchResponse(s *metal.Switch, ds *datastore.RethinkStore) (*v1.Switch
776777
return nil, err
777778
}
778779

779-
nics := makeSwitchNics(s, ips, machines)
780+
nics, err := makeSwitchNics(s, ips, machines)
781+
if err != nil {
782+
return nil, err
783+
}
780784
cons := makeSwitchCons(s)
781785

782786
return v1.NewSwitchResponse(s, ss, p, nics, cons), nil
783787
}
784788

785-
func makeBGPFilterFirewall(m metal.Machine) v1.BGPFilter {
789+
func makeBGPFilterFirewall(m metal.Machine) (v1.BGPFilter, error) {
786790
vnis := []string{}
787791
cidrs := []string{}
788792

789793
for _, net := range m.Allocation.MachineNetworks {
790794
if net.Underlay {
791795
for _, ip := range net.IPs {
792-
cidrs = append(cidrs, fmt.Sprintf("%s/32", ip))
796+
ipwithMask, err := ipWithMask(ip)
797+
if err != nil {
798+
return v1.BGPFilter{}, err
799+
}
800+
cidrs = append(cidrs, ipwithMask)
793801
}
794802
} else {
795803
vnis = append(vnis, fmt.Sprintf("%d", net.Vrf))
796804
// filter for "project" addresses / cidrs is not possible since EVPN Type-5 routes can not be filtered by prefixes
797805
}
798806
}
799807

800-
return v1.NewBGPFilter(vnis, cidrs)
808+
return v1.NewBGPFilter(vnis, cidrs), nil
801809
}
802810

803-
func makeBGPFilterMachine(m metal.Machine, ips metal.IPsMap) v1.BGPFilter {
811+
func makeBGPFilterMachine(m metal.Machine, ips metal.IPsMap) (v1.BGPFilter, error) {
804812
vnis := []string{}
805813
cidrs := []string{}
806814

@@ -828,29 +836,44 @@ func makeBGPFilterMachine(m metal.Machine, ips metal.IPsMap) v1.BGPFilter {
828836
continue
829837
}
830838
// Allow all other ip addresses allocated for the project.
831-
cidrs = append(cidrs, fmt.Sprintf("%s/32", i.IPAddress))
839+
ipwithMask, err := ipWithMask(i.IPAddress)
840+
if err != nil {
841+
return v1.BGPFilter{}, err
842+
}
843+
cidrs = append(cidrs, ipwithMask)
832844
}
833845

834-
return v1.NewBGPFilter(vnis, cidrs)
846+
return v1.NewBGPFilter(vnis, cidrs), nil
835847
}
836848

837-
func makeBGPFilter(m metal.Machine, vrf string, ips metal.IPsMap) v1.BGPFilter {
838-
var filter v1.BGPFilter
849+
func ipWithMask(ip string) (string, error) {
850+
parsed, err := netip.ParseAddr(ip)
851+
if err != nil {
852+
return "", err
853+
}
854+
return fmt.Sprintf("%s/%d", ip, parsed.BitLen()), nil
855+
}
856+
857+
func makeBGPFilter(m metal.Machine, vrf string, ips metal.IPsMap) (v1.BGPFilter, error) {
858+
var (
859+
filter v1.BGPFilter
860+
err error
861+
)
839862

840863
if m.IsFirewall() {
841864
// vrf "default" means: the firewall was successfully allocated and the switch port configured
842865
// otherwise the port is still not configured yet (pxe-setup) and a BGPFilter would break the install routine
843866
if vrf == "default" {
844-
filter = makeBGPFilterFirewall(m)
867+
filter, err = makeBGPFilterFirewall(m)
845868
}
846869
} else {
847-
filter = makeBGPFilterMachine(m, ips)
870+
filter, err = makeBGPFilterMachine(m, ips)
848871
}
849872

850-
return filter
873+
return filter, err
851874
}
852875

853-
func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines) v1.SwitchNics {
876+
func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines) (v1.SwitchNics, error) {
854877
machinesByID := map[string]*metal.Machine{}
855878
for i, m := range machines {
856879
machinesByID[m.ID] = &machines[i]
@@ -871,7 +894,10 @@ func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines)
871894
m := machinesBySwp[n.Name]
872895
var filter *v1.BGPFilter
873896
if m != nil && m.Allocation != nil {
874-
f := makeBGPFilter(*m, n.Vrf, ips)
897+
f, err := makeBGPFilter(*m, n.Vrf, ips)
898+
if err != nil {
899+
return nil, err
900+
}
875901
filter = &f
876902
}
877903
nic := v1.SwitchNic{
@@ -896,7 +922,7 @@ func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines)
896922
return nics[i].Name < nics[j].Name
897923
})
898924

899-
return nics
925+
return nics, nil
900926
}
901927

902928
func makeSwitchCons(s *metal.Switch) []v1.SwitchConnection {
@@ -990,7 +1016,10 @@ func makeSwitchResponseList(ss metal.Switches, ds *datastore.RethinkStore) ([]*v
9901016
p = &partitionEntity
9911017
}
9921018

993-
nics := makeSwitchNics(&sw, ips, m)
1019+
nics, err := makeSwitchNics(&sw, ips, m)
1020+
if err != nil {
1021+
return nil, err
1022+
}
9941023
cons := makeSwitchCons(&sw)
9951024
ss, err := ds.GetSwitchStatus(sw.ID)
9961025
if err != nil && !metal.IsNotFound(err) {

‎cmd/metal-api/internal/service/switch-service_test.go‎

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/google/go-cmp/cmp"
1616
"github.com/stretchr/testify/assert"
1717
"github.com/stretchr/testify/require"
18-
"gopkg.in/rethinkdb/rethinkdb-go.v6"
1918
r "gopkg.in/rethinkdb/rethinkdb-go.v6"
2019

2120
"github.com/metal-stack/metal-api/cmd/metal-api/internal/datastore"
@@ -351,11 +350,20 @@ func TestMakeBGPFilterFirewall(t *testing.T) {
351350
IPs: []string{"212.89.42.1", "212.89.42.2"},
352351
Vrf: 104009,
353352
},
353+
{
354+
IPs: []string{"2001::", "fe80::"},
355+
Vrf: 104011,
356+
},
357+
{
358+
IPs: []string{"2002::", "fe81::"},
359+
Underlay: true,
360+
Vrf: 104012,
361+
},
354362
},
355363
},
356364
},
357365
},
358-
want: v1.NewBGPFilter([]string{"104009", "104010"}, []string{"10.0.0.1/32", "10.0.0.2/32"}),
366+
want: v1.NewBGPFilter([]string{"104009", "104010", "104011"}, []string{"10.0.0.1/32", "10.0.0.2/32", "2002::/128", "fe81::/128"}),
359367
},
360368
{
361369
name: "no underlay firewall networks",
@@ -395,7 +403,7 @@ func TestMakeBGPFilterFirewall(t *testing.T) {
395403
for i := range tests {
396404
tt := tests[i]
397405
t.Run(tt.name, func(t *testing.T) {
398-
got := makeBGPFilterFirewall(tt.args.machine)
406+
got, _ := makeBGPFilterFirewall(tt.args.machine)
399407
if !reflect.DeepEqual(got, tt.want) {
400408
t.Errorf("makeBGPFilterFirewall() = %v, want %v", got, tt.want)
401409
}
@@ -429,6 +437,9 @@ func TestMakeBGPFilterMachine(t *testing.T) {
429437
metal.IP{
430438
IPAddress: "10.1.0.1",
431439
},
440+
metal.IP{
441+
IPAddress: "2001::1",
442+
},
432443
}},
433444
machine: metal.Machine{
434445
Allocation: &metal.MachineAllocation{
@@ -449,11 +460,15 @@ func TestMakeBGPFilterMachine(t *testing.T) {
449460
IPs: []string{"212.89.42.2", "212.89.42.1"},
450461
Vrf: 104009,
451462
},
463+
{
464+
IPs: []string{"2001::"},
465+
Vrf: 104010,
466+
},
452467
},
453468
},
454469
},
455470
},
456-
want: v1.NewBGPFilter([]string{}, []string{"10.1.0.0/22", "10.2.0.0/22", "100.127.1.1/32", "212.89.42.1/32", "212.89.42.2/32"}),
471+
want: v1.NewBGPFilter([]string{}, []string{"10.1.0.0/22", "10.2.0.0/22", "100.127.1.1/32", "2001::1/128", "212.89.42.1/32", "212.89.42.2/32"}),
457472
},
458473
{
459474
name: "allow only allocated ips",
@@ -481,7 +496,7 @@ func TestMakeBGPFilterMachine(t *testing.T) {
481496
for i := range tests {
482497
tt := tests[i]
483498
t.Run(tt.name, func(t *testing.T) {
484-
got := makeBGPFilterMachine(tt.args.machine, tt.args.ipsMap)
499+
got, _ := makeBGPFilterMachine(tt.args.machine, tt.args.ipsMap)
485500
if !reflect.DeepEqual(got, tt.want) {
486501
t.Errorf("makeBGPFilterMachine() = %v, want %v", got, tt.want)
487502
}
@@ -588,7 +603,7 @@ func TestMakeSwitchNics(t *testing.T) {
588603
for i := range tests {
589604
tt := tests[i]
590605
t.Run(tt.name, func(t *testing.T) {
591-
got := makeSwitchNics(tt.args.s, tt.args.ips, tt.args.machines)
606+
got, _ := makeSwitchNics(tt.args.s, tt.args.ips, tt.args.machines)
592607
if !reflect.DeepEqual(got, tt.want) {
593608
t.Errorf("makeSwitchNics() = %v, want %v", got, tt.want)
594609
}
@@ -1425,23 +1440,23 @@ func TestToggleSwitchNicWithoutMachine(t *testing.T) {
14251440
func Test_SwitchDelete(t *testing.T) {
14261441
tests := []struct {
14271442
name string
1428-
mockFn func(mock *rethinkdb.Mock)
1443+
mockFn func(mock *r.Mock)
14291444
want *v1.SwitchResponse
14301445
wantErr error
14311446
wantStatus int
14321447
force bool
14331448
}{
14341449
{
14351450
name: "delete switch",
1436-
mockFn: func(mock *rethinkdb.Mock) {
1437-
mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
1451+
mockFn: func(mock *r.Mock) {
1452+
mock.On(r.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
14381453
Base: metal.Base{
14391454
ID: "switch-1",
14401455
},
14411456
}, nil)
1442-
mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
1443-
mock.On(rethinkdb.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil)
1444-
mock.On(rethinkdb.DB("mockdb").Table("ip")).Return(nil, nil)
1457+
mock.On(r.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
1458+
mock.On(r.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil)
1459+
mock.On(r.DB("mockdb").Table("ip")).Return(nil, nil)
14451460
},
14461461
want: &v1.SwitchResponse{
14471462
Common: v1.Common{
@@ -1460,16 +1475,16 @@ func Test_SwitchDelete(t *testing.T) {
14601475
},
14611476
{
14621477
name: "delete switch does not work due to machine connections",
1463-
mockFn: func(mock *rethinkdb.Mock) {
1464-
mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
1478+
mockFn: func(mock *r.Mock) {
1479+
mock.On(r.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
14651480
Base: metal.Base{
14661481
ID: "switch-1",
14671482
},
14681483
MachineConnections: metal.ConnectionMap{
14691484
"port-a": metal.Connections{},
14701485
},
14711486
}, nil)
1472-
mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
1487+
mock.On(r.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
14731488
},
14741489
wantErr: &httperrors.HTTPErrorResponse{
14751490
StatusCode: http.StatusBadRequest,
@@ -1479,18 +1494,18 @@ func Test_SwitchDelete(t *testing.T) {
14791494
},
14801495
{
14811496
name: "delete switch with force",
1482-
mockFn: func(mock *rethinkdb.Mock) {
1483-
mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
1497+
mockFn: func(mock *r.Mock) {
1498+
mock.On(r.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{
14841499
Base: metal.Base{
14851500
ID: "switch-1",
14861501
},
14871502
MachineConnections: metal.ConnectionMap{
14881503
"port-a": metal.Connections{},
14891504
},
14901505
}, nil)
1491-
mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
1492-
mock.On(rethinkdb.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil)
1493-
mock.On(rethinkdb.DB("mockdb").Table("ip")).Return(nil, nil)
1506+
mock.On(r.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil)
1507+
mock.On(r.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil)
1508+
mock.On(r.DB("mockdb").Table("ip")).Return(nil, nil)
14941509
},
14951510
force: true,
14961511
want: &v1.SwitchResponse{

0 commit comments

Comments
 (0)