From 8dec67f03167337d9cd6b8f95921f2ba2c03870a Mon Sep 17 00:00:00 2001 From: Maximilian Wilhelm Date: Thu, 9 Nov 2023 02:38:57 +0100 Subject: [PATCH 1/2] Move common functions in netbox/utils sub-module This is done in preparation for splitting up the NetBox connector and client to support version specific NetBox clients and to support multiple version of NetBox at the same time. Also fix some linter nits. Signed-off-by: Maximilian Wilhelm --- pkg/connector/netbox/netbox.go | 157 +++------------------------- pkg/connector/netbox/netbox_test.go | 3 +- pkg/connector/netbox/utils/utils.go | 140 +++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 145 deletions(-) create mode 100644 pkg/connector/netbox/utils/utils.go diff --git a/pkg/connector/netbox/netbox.go b/pkg/connector/netbox/netbox.go index 1ada723..b4a3db7 100644 --- a/pkg/connector/netbox/netbox.go +++ b/pkg/connector/netbox/netbox.go @@ -9,14 +9,13 @@ package netbox import ( "fmt" - "regexp" - "strconv" "strings" "sync" "sync/atomic" "time" dbModel "github.com/cloudflare/octopus/pkg/connector/netbox/model" + nbUtils "github.com/cloudflare/octopus/pkg/connector/netbox/utils" "github.com/cloudflare/octopus/pkg/model" "github.com/cloudflare/octopus/proto/octopus" @@ -29,10 +28,6 @@ const ( updateInterval = time.Minute * 2 ) -var ( - ifNameTagsRegexp = regexp.MustCompile(`^(?P.+?)\.((?P\d+)\.)?(?P\d+)$`) -) - type NetboxConnector struct { connectorMu sync.RWMutex client NetboxClientI @@ -192,7 +187,7 @@ func (n *NetboxConnector) addDevices(t *model.Topology) error { topoDev.Role = d.DeviceRole.Slug topoDev.DeviceType = d.DeviceType.Slug - md, err := metaDataFromTags(d.Tags) + md, err := nbUtils.GetMetaDataFromTags(d.Tags) if err != nil { return fmt.Errorf("unable to get meta data: %v", err) } @@ -218,7 +213,7 @@ func (n *NetboxConnector) addInterfaces(t *model.Topology) error { t.DevicesByInterfaceID[nbIfa.ID] = d t.Interfaces[nbIfa.ID] = ifa - md, err := metaDataFromTags(nbIfa.Tags) + md, err := nbUtils.GetMetaDataFromTags(nbIfa.Tags) if err != nil { return fmt.Errorf("unable to get meta data: %v", err) } @@ -255,15 +250,15 @@ func (n *NetboxConnector) addInterfaceUnits(t *model.Topology) error { return fmt.Errorf("can not find interface %s:%s", nbIfa.Device.Name, nbIfa.Parent.Name) } - vlanTag, err := parseUnitStr(unitStr) + vlanTag, err := nbUtils.ParseUnitStr(unitStr) if err != nil { - return fmt.Errorf("Unable to convert unit %q (id=%d) (interface %q) to int for %s:%s. Ignoring logical interface.", unitStr, nbIfa.ID, nbIfa.Name, nbIfa.Device.Name, nbIfa.Parent.Name) + return fmt.Errorf("unable to convert unit %q (id=%d) (interface %q) to int for %s:%s. Ignoring logical interface", unitStr, nbIfa.ID, nbIfa.Name, nbIfa.Device.Name, nbIfa.Parent.Name) } u := ifa.AddUnitIfNotExists(vlanTag) t.DevicesByInterfaceID[nbIfa.ID] = d - md, err := metaDataFromTags(nbIfa.Tags) + md, err := nbUtils.GetMetaDataFromTags(nbIfa.Tags) if err != nil { return fmt.Errorf("unable to get meta data: %v", err) } @@ -274,34 +269,6 @@ func (n *NetboxConnector) addInterfaceUnits(t *model.Topology) error { return nil } -func parseUnitStr(unitStr string) (model.VLANTag, error) { - if strings.Contains(unitStr, ".") { - parts := strings.Split(unitStr, ".") - if len(parts) != 2 { - return model.VLANTag{}, fmt.Errorf("invalid unit string %q", unitStr) - } - - outerTag, err := strconv.Atoi(parts[0]) - if err != nil { - return model.VLANTag{}, fmt.Errorf("unable to convert %q to int: %v", parts[0], err) - } - - innerTag, err := strconv.Atoi(parts[1]) - if err != nil { - return model.VLANTag{}, fmt.Errorf("unable to convert %q to int: %v", parts[1], err) - } - - return model.NewVLANTag(uint16(outerTag), uint16(innerTag)), nil - } - - ctag, err := strconv.Atoi(unitStr) - if err != nil { - return model.VLANTag{}, fmt.Errorf("unable to convert %q to int: %v", unitStr, err) - } - - return model.NewVLANTag(0, uint16(ctag)), nil -} - func (n *NetboxConnector) addIPAddresses(t *model.Topology) error { for _, nbIP := range n.ipAddresses { if nbIP.AssignedObjectID == 0 || nbIP.AssignedObjectTypeID != n.client.GetDcimInterfaceTypeID() { @@ -323,18 +290,18 @@ func (n *NetboxConnector) addIPAddresses(t *model.Topology) error { return fmt.Errorf("interface with id %d not found", ifaID) } - _, vt, err := getInterfaceAndVLANTag(dcimIfa.Name) + _, vt, err := nbUtils.GetInterfaceAndVLANTag(dcimIfa.Name) if err != nil { return fmt.Errorf("unable to extract interface name and unit from %q: %v", dcimIfa.Name, err) } - pfx, err := bnet.PrefixFromString(sanitizeIPAddress(nbIP.Address)) + pfx, err := bnet.PrefixFromString(nbUtils.SanitizeIPAddress(nbIP.Address)) if err != nil { return fmt.Errorf("failed to parse IP %q: %v", nbIP.Address, err) } ip := model.NewIP(*pfx) - getCustomFieldData(ip.MetaData, nbIP.CustomFieldData) + nbUtils.GetCustomFieldData(ip.MetaData, nbIP.CustomFieldData) u := ifa.AddUnitIfNotExists(vt) if pfx.Addr().IsIPv4() { @@ -347,18 +314,6 @@ func (n *NetboxConnector) addIPAddresses(t *model.Topology) error { return nil } -func sanitizeIPAddress(addr string) string { - if strings.Contains(addr, "/") { - return addr - } - - if strings.Contains(addr, ".") { - return addr + "/32" - } - - return addr + "/128" -} - func (n *NetboxConnector) addPrefixes(t *model.Topology) error { for _, p := range n.prefixes { pfx, err := bnet.PrefixFromString(p.Prefix) @@ -366,7 +321,7 @@ func (n *NetboxConnector) addPrefixes(t *model.Topology) error { return fmt.Errorf("failed to parse Prefix %q: %v", p.Prefix, err) } - md, err := metaDataFromTags(p.Tags) + md, err := nbUtils.GetMetaDataFromTags(p.Tags) if err != nil { return fmt.Errorf("failed to get Tags for Prefix %q: %v", p.Prefix, err) } @@ -380,36 +335,6 @@ func (n *NetboxConnector) addPrefixes(t *model.Topology) error { return nil } -func metaDataFromTags(tags []string) (*model.MetaData, error) { - ret := model.NewMetaData() - for _, tag := range tags { - parts := strings.Split(tag, "=") - - // Semantic Tag - if len(parts) == 2 { - if _, exists := ret.SemanticTags[parts[0]]; exists { - return nil, fmt.Errorf("Key %q exists already: %q vs. %q", parts[0], ret.SemanticTags[parts[0]], parts[1]) - } - - ret.SemanticTags[parts[0]] = parts[1] - - } else { - // Regular Tag - ret.Tags = append(ret.Tags, tag) - } - } - - return ret, nil -} - -func getCustomFieldData(md *model.MetaData, customFieldData string) { - if customFieldData == "" || customFieldData == "{}" { - return - } - - md.CustomFieldData = customFieldData -} - func (n *NetboxConnector) getCableEnd(terminationType int32, terminationID int64, t *model.Topology) (*model.CableEnd, error) { ce := model.CableEnd{} @@ -483,7 +408,7 @@ func (n *NetboxConnector) getCableEnd(terminationType int32, terminationID int64 default: // consoleport, consoleserverport, powerport, or poweroutlet - return nil, fmt.Errorf("Don't know what to do with cable termination ID %d (type %d)", terminationID, terminationType) + return nil, fmt.Errorf("don't know what to do with cable termination ID %d (type %d)", terminationID, terminationType) } return &ce, nil @@ -528,7 +453,7 @@ func (n *NetboxConnector) addRearPorts(t *model.Topology) error { for _, rp := range n.rearPorts { nbDev := n.devices[rp.DeviceID] if nbDev == nil { - return fmt.Errorf("Device %d not found", rp.DeviceID) + return fmt.Errorf("device %d not found", rp.DeviceID) } d := t.GetDevice(nbDev.Name) @@ -549,7 +474,7 @@ func (n *NetboxConnector) addFrontPorts(t *model.Topology) error { for _, fp := range n.frontPorts { nbDev := n.devices[fp.DeviceID] if nbDev == nil { - return fmt.Errorf("Device %d not found", fp.DeviceID) + return fmt.Errorf("device %d not found", fp.DeviceID) } d := t.GetDevice(nbDev.Name) @@ -573,62 +498,6 @@ func (n *NetboxConnector) addFrontPorts(t *model.Topology) error { return nil } -func getInterfaceAndVLANTag(name string) (ifName string, vt model.VLANTag, err error) { - if isLogicalInterface(name) { - return extractInterfaceAndUnit(name) - } - - return name, model.VLANTag{}, nil -} - -func isLogicalInterface(name string) bool { - return strings.Contains(name, ".") -} - -func extractInterfaceAndUnit(name string) (string, model.VLANTag, error) { - extractedVars := reSubMatchMap(ifNameTagsRegexp, name) - if _, exists := extractedVars["intf_name"]; !exists { - return "", model.VLANTag{}, fmt.Errorf("unable to extract interface name from %q", name) - } - - if _, exists := extractedVars["inner_tag"]; !exists { - return "", model.VLANTag{}, fmt.Errorf("unable to extract inner tag from %q", name) - } - - innerTag, err := strconv.Atoi(extractedVars["inner_tag"]) - if err != nil { - return "", model.VLANTag{}, fmt.Errorf("unable to convert inner tag from string %q to int (%q)", extractedVars["inner_tag"], name) - } - - vt := model.VLANTag{ - InnerTag: uint16(innerTag), - } - - outerTagStr := extractedVars["outer_tag"] - if outerTagStr != "" { - outerTag, err := strconv.Atoi(outerTagStr) - if err != nil { - return "", model.VLANTag{}, fmt.Errorf("unable to convert outer tag from string %q to int (%q)", outerTagStr, name) - } - - vt.OuterTag = uint16(outerTag) - } - - return extractedVars["intf_name"], vt, nil -} - -func reSubMatchMap(r *regexp.Regexp, str string) map[string]string { - match := r.FindStringSubmatch(str) - subMatchMap := make(map[string]string) - for i, name := range r.SubexpNames() { - if i != 0 && name != "" && i <= len(match) { - subMatchMap[name] = match[i] - } - } - - return subMatchMap -} - func (n *NetboxConnector) StartRefreshRoutine() { go n.refreshRoutine() } diff --git a/pkg/connector/netbox/netbox_test.go b/pkg/connector/netbox/netbox_test.go index 2f974d0..53cc4a1 100644 --- a/pkg/connector/netbox/netbox_test.go +++ b/pkg/connector/netbox/netbox_test.go @@ -16,6 +16,7 @@ import ( bnet "github.com/bio-routing/bio-rd/net" dbModel "github.com/cloudflare/octopus/pkg/connector/netbox/model" + nbUtils "github.com/cloudflare/octopus/pkg/connector/netbox/utils" octopuspb "github.com/cloudflare/octopus/proto/octopus" ) @@ -1051,7 +1052,7 @@ func TestExtractInterfaceAndUnit(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ifName, vt, err := extractInterfaceAndUnit(test.input) + ifName, vt, err := nbUtils.ExtractInterfaceAndUnit(test.input) if err != nil { if test.wantFail { return diff --git a/pkg/connector/netbox/utils/utils.go b/pkg/connector/netbox/utils/utils.go new file mode 100644 index 0000000..bb52360 --- /dev/null +++ b/pkg/connector/netbox/utils/utils.go @@ -0,0 +1,140 @@ +package utils + +import ( + "fmt" + "regexp" + "strconv" + "strings" + + "github.com/cloudflare/octopus/pkg/model" +) + +var ( + ifNameTagsRegexp = regexp.MustCompile(`^(?P.+?)\.((?P\d+)\.)?(?P\d+)$`) +) + +func ParseUnitStr(unitStr string) (model.VLANTag, error) { + if strings.Contains(unitStr, ".") { + parts := strings.Split(unitStr, ".") + if len(parts) != 2 { + return model.VLANTag{}, fmt.Errorf("invalid unit string %q", unitStr) + } + + outerTag, err := strconv.Atoi(parts[0]) + if err != nil { + return model.VLANTag{}, fmt.Errorf("unable to convert %q to int: %v", parts[0], err) + } + + innerTag, err := strconv.Atoi(parts[1]) + if err != nil { + return model.VLANTag{}, fmt.Errorf("unable to convert %q to int: %v", parts[1], err) + } + + return model.NewVLANTag(uint16(outerTag), uint16(innerTag)), nil + } + + ctag, err := strconv.Atoi(unitStr) + if err != nil { + return model.VLANTag{}, fmt.Errorf("unable to convert %q to int: %v", unitStr, err) + } + + return model.NewVLANTag(0, uint16(ctag)), nil +} + +func SanitizeIPAddress(addr string) string { + if strings.Contains(addr, "/") { + return addr + } + + if strings.Contains(addr, ".") { + return addr + "/32" + } + + return addr + "/128" +} + +func GetMetaDataFromTags(tags []string) (*model.MetaData, error) { + ret := model.NewMetaData() + for _, tag := range tags { + parts := strings.Split(tag, "=") + + // Semantic Tag + if len(parts) == 2 { + if _, exists := ret.SemanticTags[parts[0]]; exists { + return nil, fmt.Errorf("key %q exists already: %q vs. %q", parts[0], ret.SemanticTags[parts[0]], parts[1]) + } + + ret.SemanticTags[parts[0]] = parts[1] + + } else { + // Regular Tag + ret.Tags = append(ret.Tags, tag) + } + } + + return ret, nil +} + +func GetCustomFieldData(md *model.MetaData, customFieldData string) { + if customFieldData == "" || customFieldData == "{}" { + return + } + + md.CustomFieldData = customFieldData +} + +func GetInterfaceAndVLANTag(name string) (ifName string, vt model.VLANTag, err error) { + if isLogicalInterface(name) { + return ExtractInterfaceAndUnit(name) + } + + return name, model.VLANTag{}, nil +} + +func isLogicalInterface(name string) bool { + return strings.Contains(name, ".") +} + +func ExtractInterfaceAndUnit(name string) (string, model.VLANTag, error) { + extractedVars := reSubMatchMap(ifNameTagsRegexp, name) + if _, exists := extractedVars["intf_name"]; !exists { + return "", model.VLANTag{}, fmt.Errorf("unable to extract interface name from %q", name) + } + + if _, exists := extractedVars["inner_tag"]; !exists { + return "", model.VLANTag{}, fmt.Errorf("unable to extract inner tag from %q", name) + } + + innerTag, err := strconv.Atoi(extractedVars["inner_tag"]) + if err != nil { + return "", model.VLANTag{}, fmt.Errorf("unable to convert inner tag from string %q to int (%q)", extractedVars["inner_tag"], name) + } + + vt := model.VLANTag{ + InnerTag: uint16(innerTag), + } + + outerTagStr := extractedVars["outer_tag"] + if outerTagStr != "" { + outerTag, err := strconv.Atoi(outerTagStr) + if err != nil { + return "", model.VLANTag{}, fmt.Errorf("unable to convert outer tag from string %q to int (%q)", outerTagStr, name) + } + + vt.OuterTag = uint16(outerTag) + } + + return extractedVars["intf_name"], vt, nil +} + +func reSubMatchMap(r *regexp.Regexp, str string) map[string]string { + match := r.FindStringSubmatch(str) + subMatchMap := make(map[string]string) + for i, name := range r.SubexpNames() { + if i != 0 && name != "" && i <= len(match) { + subMatchMap[name] = match[i] + } + } + + return subMatchMap +} From 22c898a2e17de25db15b1a5010b6e0a71062b210 Mon Sep 17 00:00:00 2001 From: Maximilian Wilhelm Date: Thu, 9 Nov 2023 03:25:34 +0100 Subject: [PATCH 2/2] Increase unit test coverage for netbox/utils Signed-off-by: Maximilian Wilhelm --- pkg/connector/netbox/netbox_test.go | 53 ------- pkg/connector/netbox/utils/utils.go | 4 +- pkg/connector/netbox/utils/utils_test.go | 170 +++++++++++++++++++++++ 3 files changed, 172 insertions(+), 55 deletions(-) create mode 100644 pkg/connector/netbox/utils/utils_test.go diff --git a/pkg/connector/netbox/netbox_test.go b/pkg/connector/netbox/netbox_test.go index 53cc4a1..8a8f597 100644 --- a/pkg/connector/netbox/netbox_test.go +++ b/pkg/connector/netbox/netbox_test.go @@ -16,7 +16,6 @@ import ( bnet "github.com/bio-routing/bio-rd/net" dbModel "github.com/cloudflare/octopus/pkg/connector/netbox/model" - nbUtils "github.com/cloudflare/octopus/pkg/connector/netbox/utils" octopuspb "github.com/cloudflare/octopus/proto/octopus" ) @@ -1020,55 +1019,3 @@ func TestEnrichment(t *testing.T) { assert.Equal(t, test.expected, test.t.ToProto(), test.name) } } - -func TestExtractInterfaceAndUnit(t *testing.T) { - tests := []struct { - name string - input string - expectedName string - expectedVLANTag model.VLANTag - wantFail bool - }{ - { - name: "dot1q", - input: "vlan.900", - expectedName: "vlan", - expectedVLANTag: model.NewVLANTag(0, 900), - }, - { - name: "Q-in-Q", - input: "xe-0/0/0.100.200", - expectedName: "xe-0/0/0", - expectedVLANTag: model.NewVLANTag(100, 200), - }, - { - name: "broken", - input: "xe-0/0/0", - expectedName: "xe-0/0/0", - expectedVLANTag: model.NewVLANTag(100, 200), - wantFail: true, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ifName, vt, err := nbUtils.ExtractInterfaceAndUnit(test.input) - if err != nil { - if test.wantFail { - return - } - - t.Errorf("unexpected failure of test %q: %v", test.name, err) - return - } - - if test.wantFail { - t.Errorf("unexpected success of test %q", test.name) - return - } - - assert.Equal(t, test.expectedName, ifName, test.name) - assert.Equal(t, test.expectedVLANTag, vt, test.name) - }) - } -} diff --git a/pkg/connector/netbox/utils/utils.go b/pkg/connector/netbox/utils/utils.go index bb52360..1f25911 100644 --- a/pkg/connector/netbox/utils/utils.go +++ b/pkg/connector/netbox/utils/utils.go @@ -85,7 +85,7 @@ func GetCustomFieldData(md *model.MetaData, customFieldData string) { func GetInterfaceAndVLANTag(name string) (ifName string, vt model.VLANTag, err error) { if isLogicalInterface(name) { - return ExtractInterfaceAndUnit(name) + return extractInterfaceAndUnit(name) } return name, model.VLANTag{}, nil @@ -95,7 +95,7 @@ func isLogicalInterface(name string) bool { return strings.Contains(name, ".") } -func ExtractInterfaceAndUnit(name string) (string, model.VLANTag, error) { +func extractInterfaceAndUnit(name string) (string, model.VLANTag, error) { extractedVars := reSubMatchMap(ifNameTagsRegexp, name) if _, exists := extractedVars["intf_name"]; !exists { return "", model.VLANTag{}, fmt.Errorf("unable to extract interface name from %q", name) diff --git a/pkg/connector/netbox/utils/utils_test.go b/pkg/connector/netbox/utils/utils_test.go new file mode 100644 index 0000000..241b165 --- /dev/null +++ b/pkg/connector/netbox/utils/utils_test.go @@ -0,0 +1,170 @@ +package utils + +import ( + "testing" + + "github.com/cloudflare/octopus/pkg/model" + "github.com/stretchr/testify/assert" +) + +func TestEnrichment(t *testing.T) { + tests := []struct { + name string + input string + wantFail bool + expected model.VLANTag + }{ + /* + * Errors + */ + { + name: "invalid unit string", + input: "a.b.c", + wantFail: true, + expected: model.VLANTag{}, + }, + { + name: "invalid outer tag", + input: "a.1", + wantFail: true, + expected: model.VLANTag{}, + }, + { + name: "invalid inner tag", + input: "1.a", + wantFail: true, + expected: model.VLANTag{}, + }, + { + name: "invalid single tag", + input: "something", + wantFail: true, + expected: model.VLANTag{}, + }, + { + name: "valid single tag", + input: "42", + wantFail: false, + expected: model.NewVLANTag(0, 42), + }, + { + name: "valid double tag", + input: "23.42", + wantFail: false, + expected: model.NewVLANTag(23, 42), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + vt, err := ParseUnitStr(test.input) + if err != nil { + if test.wantFail { + return + } + + t.Errorf("unexpected failure of test %q: %v", test.name, err) + return + } + + if test.wantFail { + t.Errorf("unexpected success of test %q", test.name) + return + } + + assert.Equal(t, test.expected, vt, test.name) + }) + } +} + +func TestSanitizeIPAddress(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "address w/ prefix-length", + input: "192.0.2.42/24", + expected: "192.0.2.42/24", + }, + { + name: "IPv4 address w/o prefix-length", + input: "192.0.2.42", + expected: "192.0.2.42/32", + }, + { + name: "IPv6 address w/o prefix-length", + input: "2001:db8::42", + expected: "2001:db8::42/128", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, SanitizeIPAddress(test.input), test.name) + }) + } +} + +func TestExtractInterfaceAndUnit(t *testing.T) { + tests := []struct { + name string + input string + expectedName string + expectedVLANTag model.VLANTag + wantFail bool + }{ + { + name: "dot1q", + input: "vlan.900", + expectedName: "vlan", + expectedVLANTag: model.NewVLANTag(0, 900), + }, + { + name: "Q-in-Q", + input: "xe-0/0/0.100.200", + expectedName: "xe-0/0/0", + expectedVLANTag: model.NewVLANTag(100, 200), + }, + { + name: "no unit", + input: "xe-0/0/0", + expectedName: "", + expectedVLANTag: model.NewVLANTag(0, 0), + wantFail: true, + }, + { + name: "invalid qot1q", + input: "xe-0/0/0.a", + expectedName: "", + expectedVLANTag: model.NewVLANTag(0, 0), + wantFail: true, + }, + { + name: "invalid qot1q", + input: "xe-0/0/0.0.a", + expectedName: "", + expectedVLANTag: model.NewVLANTag(0, 0), + wantFail: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ifName, vt, err := extractInterfaceAndUnit(test.input) + if err != nil && !test.wantFail { + t.Errorf("unexpected failure of test %q: %v", test.name, err) + return + } + + if err == nil && test.wantFail { + t.Errorf("unexpected success of test %q", test.name) + return + } + + assert.Equal(t, test.expectedName, ifName, test.name) + assert.Equal(t, test.expectedVLANTag, vt, test.name) + }) + } +}