From 5a5b2e460ff6ff96856b518eb3c45779d6dec89e Mon Sep 17 00:00:00 2001 From: Stefan Majer Date: Mon, 5 Aug 2024 08:59:58 +0200 Subject: [PATCH] Remove hardcoded /32 bitmask for some switch entities (#555) --- .../internal/service/switch-service.go | 61 ++++++++++++++----- .../internal/service/switch-service_test.go | 55 +++++++++++------ 2 files changed, 80 insertions(+), 36 deletions(-) diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go index 092096c1..4e9cd57b 100644 --- a/cmd/metal-api/internal/service/switch-service.go +++ b/cmd/metal-api/internal/service/switch-service.go @@ -5,6 +5,7 @@ import ( "fmt" "log/slog" "net/http" + "net/netip" "sort" "strconv" "strings" @@ -776,20 +777,27 @@ func makeSwitchResponse(s *metal.Switch, ds *datastore.RethinkStore) (*v1.Switch return nil, err } - nics := makeSwitchNics(s, ips, machines) + nics, err := makeSwitchNics(s, ips, machines) + if err != nil { + return nil, err + } cons := makeSwitchCons(s) return v1.NewSwitchResponse(s, ss, p, nics, cons), nil } -func makeBGPFilterFirewall(m metal.Machine) v1.BGPFilter { +func makeBGPFilterFirewall(m metal.Machine) (v1.BGPFilter, error) { vnis := []string{} cidrs := []string{} for _, net := range m.Allocation.MachineNetworks { if net.Underlay { for _, ip := range net.IPs { - cidrs = append(cidrs, fmt.Sprintf("%s/32", ip)) + ipwithMask, err := ipWithMask(ip) + if err != nil { + return v1.BGPFilter{}, err + } + cidrs = append(cidrs, ipwithMask) } } else { vnis = append(vnis, fmt.Sprintf("%d", net.Vrf)) @@ -797,10 +805,10 @@ func makeBGPFilterFirewall(m metal.Machine) v1.BGPFilter { } } - return v1.NewBGPFilter(vnis, cidrs) + return v1.NewBGPFilter(vnis, cidrs), nil } -func makeBGPFilterMachine(m metal.Machine, ips metal.IPsMap) v1.BGPFilter { +func makeBGPFilterMachine(m metal.Machine, ips metal.IPsMap) (v1.BGPFilter, error) { vnis := []string{} cidrs := []string{} @@ -828,29 +836,44 @@ func makeBGPFilterMachine(m metal.Machine, ips metal.IPsMap) v1.BGPFilter { continue } // Allow all other ip addresses allocated for the project. - cidrs = append(cidrs, fmt.Sprintf("%s/32", i.IPAddress)) + ipwithMask, err := ipWithMask(i.IPAddress) + if err != nil { + return v1.BGPFilter{}, err + } + cidrs = append(cidrs, ipwithMask) } - return v1.NewBGPFilter(vnis, cidrs) + return v1.NewBGPFilter(vnis, cidrs), nil } -func makeBGPFilter(m metal.Machine, vrf string, ips metal.IPsMap) v1.BGPFilter { - var filter v1.BGPFilter +func ipWithMask(ip string) (string, error) { + parsed, err := netip.ParseAddr(ip) + if err != nil { + return "", err + } + return fmt.Sprintf("%s/%d", ip, parsed.BitLen()), nil +} + +func makeBGPFilter(m metal.Machine, vrf string, ips metal.IPsMap) (v1.BGPFilter, error) { + var ( + filter v1.BGPFilter + err error + ) if m.IsFirewall() { // vrf "default" means: the firewall was successfully allocated and the switch port configured // otherwise the port is still not configured yet (pxe-setup) and a BGPFilter would break the install routine if vrf == "default" { - filter = makeBGPFilterFirewall(m) + filter, err = makeBGPFilterFirewall(m) } } else { - filter = makeBGPFilterMachine(m, ips) + filter, err = makeBGPFilterMachine(m, ips) } - return filter + return filter, err } -func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines) v1.SwitchNics { +func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines) (v1.SwitchNics, error) { machinesByID := map[string]*metal.Machine{} for i, m := range machines { machinesByID[m.ID] = &machines[i] @@ -871,7 +894,10 @@ func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines) m := machinesBySwp[n.Name] var filter *v1.BGPFilter if m != nil && m.Allocation != nil { - f := makeBGPFilter(*m, n.Vrf, ips) + f, err := makeBGPFilter(*m, n.Vrf, ips) + if err != nil { + return nil, err + } filter = &f } nic := v1.SwitchNic{ @@ -896,7 +922,7 @@ func makeSwitchNics(s *metal.Switch, ips metal.IPsMap, machines metal.Machines) return nics[i].Name < nics[j].Name }) - return nics + return nics, nil } func makeSwitchCons(s *metal.Switch) []v1.SwitchConnection { @@ -990,7 +1016,10 @@ func makeSwitchResponseList(ss metal.Switches, ds *datastore.RethinkStore) ([]*v p = &partitionEntity } - nics := makeSwitchNics(&sw, ips, m) + nics, err := makeSwitchNics(&sw, ips, m) + if err != nil { + return nil, err + } cons := makeSwitchCons(&sw) ss, err := ds.GetSwitchStatus(sw.ID) if err != nil && !metal.IsNotFound(err) { diff --git a/cmd/metal-api/internal/service/switch-service_test.go b/cmd/metal-api/internal/service/switch-service_test.go index 46bbd031..51be9077 100644 --- a/cmd/metal-api/internal/service/switch-service_test.go +++ b/cmd/metal-api/internal/service/switch-service_test.go @@ -15,7 +15,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/rethinkdb/rethinkdb-go.v6" r "gopkg.in/rethinkdb/rethinkdb-go.v6" "github.com/metal-stack/metal-api/cmd/metal-api/internal/datastore" @@ -351,11 +350,20 @@ func TestMakeBGPFilterFirewall(t *testing.T) { IPs: []string{"212.89.42.1", "212.89.42.2"}, Vrf: 104009, }, + { + IPs: []string{"2001::", "fe80::"}, + Vrf: 104011, + }, + { + IPs: []string{"2002::", "fe81::"}, + Underlay: true, + Vrf: 104012, + }, }, }, }, }, - want: v1.NewBGPFilter([]string{"104009", "104010"}, []string{"10.0.0.1/32", "10.0.0.2/32"}), + want: v1.NewBGPFilter([]string{"104009", "104010", "104011"}, []string{"10.0.0.1/32", "10.0.0.2/32", "2002::/128", "fe81::/128"}), }, { name: "no underlay firewall networks", @@ -395,7 +403,7 @@ func TestMakeBGPFilterFirewall(t *testing.T) { for i := range tests { tt := tests[i] t.Run(tt.name, func(t *testing.T) { - got := makeBGPFilterFirewall(tt.args.machine) + got, _ := makeBGPFilterFirewall(tt.args.machine) if !reflect.DeepEqual(got, tt.want) { t.Errorf("makeBGPFilterFirewall() = %v, want %v", got, tt.want) } @@ -429,6 +437,9 @@ func TestMakeBGPFilterMachine(t *testing.T) { metal.IP{ IPAddress: "10.1.0.1", }, + metal.IP{ + IPAddress: "2001::1", + }, }}, machine: metal.Machine{ Allocation: &metal.MachineAllocation{ @@ -449,11 +460,15 @@ func TestMakeBGPFilterMachine(t *testing.T) { IPs: []string{"212.89.42.2", "212.89.42.1"}, Vrf: 104009, }, + { + IPs: []string{"2001::"}, + Vrf: 104010, + }, }, }, }, }, - 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"}), + 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"}), }, { name: "allow only allocated ips", @@ -481,7 +496,7 @@ func TestMakeBGPFilterMachine(t *testing.T) { for i := range tests { tt := tests[i] t.Run(tt.name, func(t *testing.T) { - got := makeBGPFilterMachine(tt.args.machine, tt.args.ipsMap) + got, _ := makeBGPFilterMachine(tt.args.machine, tt.args.ipsMap) if !reflect.DeepEqual(got, tt.want) { t.Errorf("makeBGPFilterMachine() = %v, want %v", got, tt.want) } @@ -588,7 +603,7 @@ func TestMakeSwitchNics(t *testing.T) { for i := range tests { tt := tests[i] t.Run(tt.name, func(t *testing.T) { - got := makeSwitchNics(tt.args.s, tt.args.ips, tt.args.machines) + got, _ := makeSwitchNics(tt.args.s, tt.args.ips, tt.args.machines) if !reflect.DeepEqual(got, tt.want) { t.Errorf("makeSwitchNics() = %v, want %v", got, tt.want) } @@ -1425,7 +1440,7 @@ func TestToggleSwitchNicWithoutMachine(t *testing.T) { func Test_SwitchDelete(t *testing.T) { tests := []struct { name string - mockFn func(mock *rethinkdb.Mock) + mockFn func(mock *r.Mock) want *v1.SwitchResponse wantErr error wantStatus int @@ -1433,15 +1448,15 @@ func Test_SwitchDelete(t *testing.T) { }{ { name: "delete switch", - mockFn: func(mock *rethinkdb.Mock) { - mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{ + mockFn: func(mock *r.Mock) { + mock.On(r.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{ Base: metal.Base{ ID: "switch-1", }, }, nil) - mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil) - mock.On(rethinkdb.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil) - mock.On(rethinkdb.DB("mockdb").Table("ip")).Return(nil, nil) + mock.On(r.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil) + mock.On(r.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil) + mock.On(r.DB("mockdb").Table("ip")).Return(nil, nil) }, want: &v1.SwitchResponse{ Common: v1.Common{ @@ -1460,8 +1475,8 @@ func Test_SwitchDelete(t *testing.T) { }, { name: "delete switch does not work due to machine connections", - mockFn: func(mock *rethinkdb.Mock) { - mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{ + mockFn: func(mock *r.Mock) { + mock.On(r.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{ Base: metal.Base{ ID: "switch-1", }, @@ -1469,7 +1484,7 @@ func Test_SwitchDelete(t *testing.T) { "port-a": metal.Connections{}, }, }, nil) - mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil) + mock.On(r.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil) }, wantErr: &httperrors.HTTPErrorResponse{ StatusCode: http.StatusBadRequest, @@ -1479,8 +1494,8 @@ func Test_SwitchDelete(t *testing.T) { }, { name: "delete switch with force", - mockFn: func(mock *rethinkdb.Mock) { - mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{ + mockFn: func(mock *r.Mock) { + mock.On(r.DB("mockdb").Table("switch").Get("switch-1")).Return(&metal.Switch{ Base: metal.Base{ ID: "switch-1", }, @@ -1488,9 +1503,9 @@ func Test_SwitchDelete(t *testing.T) { "port-a": metal.Connections{}, }, }, nil) - mock.On(rethinkdb.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil) - mock.On(rethinkdb.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil) - mock.On(rethinkdb.DB("mockdb").Table("ip")).Return(nil, nil) + mock.On(r.DB("mockdb").Table("switch").Get("switch-1").Delete()).Return(testdata.EmptyResult, nil) + mock.On(r.DB("mockdb").Table("switchstatus").Get("switch-1")).Return(nil, nil) + mock.On(r.DB("mockdb").Table("ip")).Return(nil, nil) }, force: true, want: &v1.SwitchResponse{