Review: Approve LGTM with some minor comments
Diff comments: > diff --git a/src/maasagent/cmd/netmon/main.go > b/src/maasagent/cmd/netmon/main.go > index e83655f..c88d9bf 100644 > --- a/src/maasagent/cmd/netmon/main.go > +++ b/src/maasagent/cmd/netmon/main.go > @@ -1,9 +1,84 @@ > package main > > +/* > + Copyright 2023 Canonical Ltd. This software is licensed under the > + GNU Affero General Public License version 3 (see the file LICENSE). > +*/ > + > import ( > + "context" > + "encoding/json" > + "errors" > + "os" > + > + "github.com/rs/zerolog" > + "github.com/rs/zerolog/log" > + "golang.org/x/sync/errgroup" > + > "launchpad.net/maas/maas/src/maasagent/internal/netmon" > ) > > +var ( > + ErrMissingIface = errors.New("Missing interface argument") > +) > + > +func Run() int { > + zerolog.SetGlobalLevel(zerolog.InfoLevel) > + log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr}) > + > + if envLogLevel, ok := os.LookupEnv("LOG_LEVEL"); ok { > + if logLevel, err := zerolog.ParseLevel(envLogLevel); err != nil > { > + log.Warn().Str("LOG_LEVEL", envLogLevel).Msg("Unknown > log level, defaulting to INFO") > + } else { > + zerolog.SetGlobalLevel(logLevel) > + } > + } > + > + if len(os.Args) < 2 { > + log.Error().Err(ErrMissingIface).Msg("Please provide an > interface to monitor") Since ErrMissingIface error is not used anywhere to check the actual error, we can remove that variable and just print Msg("Missing interface argument")? > + return 2 > + } > + iface := os.Args[1] > + > + ctx, cancel := context.WithCancel(context.Background()) > + > + sigC := make(chan os.Signal) > + resultC := make(chan netmon.Result) > + > + g, ctx := errgroup.WithContext(ctx) > + g.SetLimit(2) > + > + svc := netmon.NewService(iface) > + g.Go(func() error { > + return svc.Start(ctx, resultC) > + }) > + g.Go(func() error { > + encoder := json.NewEncoder(os.Stdout) > + for { > + select { > + case <-sigC: > + cancel() > + return nil > + case res, ok := <-resultC: > + if !ok { > + log.Debug().Msg("result channel has > been closed") > + return nil > + } > + err := encoder.Encode(res) > + if err != nil { > + return err > + } > + } > + } > + }) > + log.Info().Msg("Service netmon started") > + if err := g.Wait(); err != nil { > + log.Error().Err(err).Send() > + return 1 > + } > + return 0 > +} > + > func main() { > - netmon.NewService() > + os.Exit(Run()) > } > diff --git a/src/maasagent/internal/arp/ethernet.go > b/src/maasagent/internal/arp/ethernet.go > new file mode 100644 > index 0000000..cb029aa > --- /dev/null > +++ b/src/maasagent/internal/arp/ethernet.go > @@ -0,0 +1,142 @@ > +package arp Maybe we could name it `package ethernet` because it contains not only ARP specific things? > + > +/* > + Copyright 2023 Canonical Ltd. This software is licensed under the > + GNU Affero General Public License version 3 (see the file LICENSE). > +*/ > + > +import ( > + "encoding/binary" > + "errors" > + "io" > + "net" > +) > + > +const ( > + minEthernetLen = 14 > +) > + > +const ( > + // EthernetTypeLLC is a special ethernet type, if found the frame is > truncated > + EthernetTypeLLC uint16 = 0 > + // EthernetTypeIPv4 is the ethernet type for a frame containing an IPv4 > packet > + EthernetTypeIPv4 uint16 = 0x0800 > + // EthernetTypeARP is the ethernet type for a frame containing an ARP > packet > + EthernetTypeARP uint16 = 0x0806 > + // EthernetTypeIPv6 is the ethernet type for a frame containing an IPv6 > packet > + EthernetTypeIPv6 uint16 = 0x86dd > + // EthernetTypeVLAN is the ethernet type for a frame containing a VLAN > tag, > + // the VLAN tag bytes will indicate the actual type of packet the frame > contains > + EthernetTypeVLAN uint16 = 0x8100 > + > + // NonStdLenEthernetTypes is a magic number to find any non-standard > types > + // and mark them as EthernetTypeLLC > + NonStdLenEthernetTypes uint16 = 0x600 > +) > + > +var ( > + // ErrNotVLAN is an error returned when calling > EthernetFrame.ExtractVLAN > + // if the frame is not of type EthernetTypeVLAN > + ErrNotVLAN = errors.New("ethernet frame not of type VLAN") > + // ErrMalformedVLAN is an error returned when parsing a VLAN tag > + // that is malformed > + ErrMalformedVLAN = errors.New("VLAN tag is malformed") > + // ErrMalformedFrame is an error returned when parsing an ethernet frame > + // that is malformed > + ErrMalformedFrame = errors.New("malformed ethernet frame") > +) > + > +// VLAN represents a VLAN tag within an ethernet frame > +type VLAN struct { > + Priority uint8 > + DropEligible bool > + ID uint16 > + EthernetType uint16 > +} > + > +// UnmarshalBinary will take the ethernet frame's payload > +// and extract a VLAN tag if one is present > +func (v *VLAN) UnmarshalBinary(buf []byte) error { > + if len(buf) < 4 { > + return ErrMalformedVLAN > + } > + > + // extract the first 3 bits > + v.Priority = (buf[0] & 0xe0) >> 5 > + // extract the next bit and turn it into a bool > + v.DropEligible = buf[0]&0x10 != 0 > + // extract the next 12 bits for an ID > + v.ID = binary.BigEndian.Uint16(buf[:2]) & 0x0fff > + // last 2 bytes are ethernet type > + v.EthernetType = binary.BigEndian.Uint16(buf[2:]) > + return nil > +} > + > +// EthernetFrame represents an ethernet frame > +type EthernetFrame struct { > + SrcMAC net.HardwareAddr > + DstMAC net.HardwareAddr > + EthernetType uint16 > + Len uint16 > + Payload []byte > +} > + > +// ExtractARPPacket will extract an ARP packet from the ethernet frame's > +// payload > +func (e *EthernetFrame) ExtractARPPacket() (*Packet, error) { > + var buf []byte > + if e.EthernetType == EthernetTypeVLAN { > + buf = e.Payload[4:] > + } else { > + buf = e.Payload > + } > + a := &Packet{} > + err := a.UnmarshalBinary(buf) > + if err != nil { > + return nil, err > + } > + return a, nil > +} > + > +// ExtractVLAN will extract the VLAN tag from the ethernet frame's > +// payload if one is present and return ErrNotVLAN if not > +func (e *EthernetFrame) ExtractVLAN() (*VLAN, error) { > + if e.EthernetType != EthernetTypeVLAN { > + return nil, ErrNotVLAN > + } > + v := &VLAN{} > + err := v.UnmarshalBinary(e.Payload[0:4]) > + if err != nil { > + return nil, err > + } > + return v, nil > +} > + > +// UnmarshalBinary parses ethernet frame bytes into an EthernetFrame > +func (eth *EthernetFrame) UnmarshalBinary(buf []byte) error { > + if len(buf) < minEthernetLen { > + if len(buf) == 0 { > + return io.ErrUnexpectedEOF > + } > + return ErrMalformedFrame > + } > + > + eth.DstMAC = buf[0:6] > + eth.SrcMAC = buf[6:12] > + eth.EthernetType = binary.BigEndian.Uint16(buf[12:14]) > + eth.Payload = buf[14:] > + if eth.EthernetType < NonStdLenEthernetTypes { > + // see IEEE 802.3, non-standard ethernet may contain padding > + // this calculation is used to truncate the payload to the > length > + // specified for that ethernet type > + eth.Len = eth.EthernetType > + eth.EthernetType = EthernetTypeLLC > + cmp := len(eth.Payload) - int(eth.Len) > + if cmp < 0 { > + return ErrMalformedFrame > + } else if cmp > 0 { > + eth.Payload = eth.Payload[:len(eth.Payload)-cmp] > + } > + } > + return nil > +} > diff --git a/src/maasagent/internal/netmon/service.go > b/src/maasagent/internal/netmon/service.go > index 14ebdc6..8f8ef27 100644 > --- a/src/maasagent/internal/netmon/service.go > +++ b/src/maasagent/internal/netmon/service.go > @@ -1,3 +1,251 @@ > package netmon > > -func NewService() {} > +/* > + Copyright 2023 Canonical Ltd. This software is licensed under the > + GNU Affero General Public License version 3 (see the file LICENSE). > +*/ > + > +import ( > + "bytes" > + "context" > + "errors" > + "fmt" > + "net" > + "net/netip" > + "time" > + > + pcap "github.com/packetcap/go-pcap" > + "github.com/rs/zerolog/log" > + > + "launchpad.net/maas/maas/src/maasagent/internal/arp" > +) > + > +const ( > + snapLen int32 = 64 > + timeout time.Duration = -1 > + seenAgainThreshold time.Duration = 600 * time.Second > +) > + > +const ( > + // EventNew is the Event value for a new Result > + EventNew = "NEW" > + // EventRefreshed is the Event value for a Result that is for > + // refreshed ARP values > + EventRefreshed = "REFRESHED" > + // EventMoved is the Event value for a Result where the IP has > + // changed its MAC address > + EventMoved = "MOVED" > +) > + > +var ( > + // ErrEmptyPacket is returned when a packet of 0 bytes has been received > + ErrEmptyPacket = errors.New("received an empty packet") > + // ErrPacketCaptureClosed is returned when the packet capture channel > + // has been closed unexpectedly > + ErrPacketCaptureClosed = errors.New("packet capture channel closed") > +) > + > +// Binding represents the binding between an IP address and MAC address > +type Binding struct { > + // IP is the IP a binding is tracking > + IP netip.Addr > + // MAC is the MAC address the IP is currently bound to > + MAC net.HardwareAddr > + // VID is the associated VLAN ID, if one exists > + VID *uint16 > + // Time is the time the packet creating / updating the binding > + // was observed > + Time time.Time > +} > + > +// Result is the result of observed ARP packets > +type Result struct { > + // IP is the presentation format of an observed IP > + IP string `json:"ip"` > + // MAC is the presentation format of an observed MAC > + MAC string `json:"mac"` > + // Previous MAC is the presentation format of a previous MAC if > + // an EventMoved was observed > + PreviousMAC string `json:"previous_mac,omitempty"` > + // Event is the type of event the Result is > + Event string `json:"event"` > + // Time is the time the packet creating the Result was observed > + Time int64 `json:"time"` > + // VID is the VLAN ID if one exists > + VID *uint16 `json:"vid"` > +} > + > +// Service is responsible for starting packet capture and > +// converting observed ARP packets into discovered Results > +type Service struct { > + iface string > + bindings map[string]Binding > +} > + > +// NewService returns a pointer to a Service. It > +// takes the desired interface to observe's name as an argument > +func NewService(iface string) *Service { > + return &Service{ > + iface: iface, > + bindings: make(map[string]Binding), > + } > +} > + > +func (s *Service) updateBindings(pkt *arp.Packet, vid *uint16, timestamp > time.Time) (res []Result) { > + if timestamp.IsZero() { > + timestamp = time.Now() > + } > + > + var vidLabel int > + if vid != nil { > + vidLabel = int(*vid) > + } > + > + discoveredBindings := []Binding{ > + { > + IP: pkt.SendIPAddr, > + MAC: pkt.SendHwdAddr, > + VID: vid, > + Time: timestamp, > + }, > + } > + if pkt.OpCode == arp.OpReply { > + discoveredBindings = append(discoveredBindings, Binding{ > + IP: pkt.TgtIPAddr, > + MAC: pkt.TgtHwdAddr, > + VID: vid, > + Time: timestamp, > + }) > + } > + > + for _, discoveredBinding := range discoveredBindings { It seems that here we iterate over a known slice length. Can't we use slice initialiser here and have `res []Result` of a known length instead of having a possible slice re-alloc? https://git.launchpad.net/maas/tree/go-style-guide.md#n641 > + key := fmt.Sprintf("%d_%s", vidLabel, > discoveredBinding.IP.String()) > + binding, ok := s.bindings[key] > + if ok { > + if bytes.Compare(binding.MAC, discoveredBinding.MAC) != > 0 { You can use `!bytes.Equal` here > + s.bindings[key] = discoveredBinding It seems that we have this assignment in all three branches. Can we just have it one place? > + res = append(res, Result{ Would it be more readable to have an instance of Result with a common properties being initialised once? ``` r := Result{ IP: discoveredBinding.IP.String(), MAC: discoveredBinding.MAC.String(), Time: discoveredBinding.Time.Unix(), VID: discoveredBinding.VID, } ``` And then in each if-else if-else branch fill in the things that are different? I think it makes it more readable to understand what exactly is the difference. > + IP: > discoveredBinding.IP.String(), > + PreviousMAC: binding.MAC.String(), > + MAC: > discoveredBinding.MAC.String(), > + VID: discoveredBinding.VID, > + Time: > discoveredBinding.Time.Unix(), > + Event: EventMoved, > + }) > + } else if discoveredBinding.Time.Sub(binding.Time) >= > seenAgainThreshold { > + s.bindings[key] = discoveredBinding > + res = append(res, Result{ > + IP: discoveredBinding.IP.String(), > + MAC: discoveredBinding.MAC.String(), > + VID: discoveredBinding.VID, > + Time: discoveredBinding.Time.Unix(), > + Event: EventRefreshed, > + }) > + } > + } else { > + s.bindings[key] = discoveredBinding > + res = append(res, Result{ > + IP: discoveredBinding.IP.String(), > + MAC: discoveredBinding.MAC.String(), > + VID: discoveredBinding.VID, > + Time: discoveredBinding.Time.Unix(), > + Event: EventNew, > + }) > + } > + } > + > + return res > +} > + > +func isValidARPPacket(pkt *arp.Packet) bool { > + if pkt.HardwareType != arp.HardwareTypeEthernet { > + return false > + } > + if pkt.ProtocolType != arp.ProtocolTypeIPv4 { > + return false > + } > + if pkt.HardwareAddrLen != 6 { > + return false > + } > + if pkt.ProtocolAddrLen != 4 { > + return false > + } > + return true > +} > + > +func (s *Service) handlePacket(pkt pcap.Packet) ([]Result, error) { > + if pkt.Error != nil { > + return nil, pkt.Error > + } > + if len(pkt.B) == 0 { > + return nil, ErrEmptyPacket > + } > + eth := &arp.EthernetFrame{} > + err := eth.UnmarshalBinary(pkt.B) > + if err != nil { > + return nil, err > + } > + > + if eth.EthernetType != arp.EthernetTypeVLAN && eth.EthernetType != > arp.EthernetTypeARP { > + log.Debug().Msg("skipping non-ARP packet") > + return nil, nil > + } > + > + var vid *uint16 > + if eth.EthernetType == arp.EthernetTypeVLAN { > + vlan, err := eth.ExtractVLAN() > + if err != nil { > + return nil, err > + } > + vid = &vlan.ID > + } > + > + arpPkt, err := eth.ExtractARPPacket() > + if err != nil { > + return nil, err > + } > + > + if !isValidARPPacket(arpPkt) { Why we want retrieve ARP packet from `eth.ExtractARPPacket()` and check if it is valid later? Why cant we return `error` from `eth.ExtractARPPacket` in case of invalid packet? > + log.Debug().Msg("skipping non-ethernet+IPv4 ARP packet") > + return nil, nil > + } > + return s.updateBindings(arpPkt, vid, pkt.Info.Timestamp), nil > +} > + > +func isRecoverableError(err error) bool { > + return errors.Is(err, arp.ErrMalformedPacket) || errors.Is(err, > arp.ErrMalformedVLAN) || errors.Is(err, arp.ErrMalformedFrame) > +} > + > +// Start will start packet capture and send results to a channel > +func (s *Service) Start(ctx context.Context, resultC chan<- Result) error { > + defer close(resultC) > + > + hndlr, err := pcap.OpenLive(s.iface, snapLen, false, timeout, true) > + if err != nil { > + return err > + } > + defer hndlr.Close() > + pkts := hndlr.Listen() > + for { > + select { > + case <-ctx.Done(): > + return nil > + case pkt, ok := <-pkts: > + if !ok { > + log.Debug().Msg("packet capture has closed") > + return ErrPacketCaptureClosed > + } > + res, err := s.handlePacket(pkt) > + if err != nil { > + if isRecoverableError(err) { > + log.Error().Err(err).Send() > + continue > + } > + return err > + } > + for _, r := range res { > + resultC <- r > + } > + } > + } > +} > diff --git a/src/maasagent/internal/netmon/service_test.go > b/src/maasagent/internal/netmon/service_test.go > new file mode 100644 > index 0000000..158a651 > --- /dev/null > +++ b/src/maasagent/internal/netmon/service_test.go > @@ -0,0 +1,382 @@ > +package netmon > + > +/* > + Copyright 2023 Canonical Ltd. This software is licensed under the > + GNU Affero General Public License version 3 (see the file LICENSE). > +*/ > + > +import ( > + "net" > + "net/netip" > + "testing" > + "time" > + > + "github.com/google/gopacket" > + pcap "github.com/packetcap/go-pcap" > + "github.com/stretchr/testify/assert" > + > + "launchpad.net/maas/maas/src/maasagent/internal/arp" > +) > + > +func uint16Pointer(v uint16) *uint16 { > + return &v > +} > + > +type isValidARPPacketCase struct { > + Name string > + In *arp.Packet > + Out bool > +} > + > +func TestIsValidARPPacket(t *testing.T) { > + table := []isValidARPPacketCase{ I am wondering why you decided to define a type for a testCase here and not use approach defined on the style guide? https://git.launchpad.net/maas/tree/go-style-guide.md#n722 > + { > + Name: "ValidARPPacket", > + In: &arp.Packet{ > + HardwareType: arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + }, > + Out: true, > + }, > + { > + Name: "InvalidHardwareTypeARPPacket", > + In: &arp.Packet{ > + HardwareType: arp.HardwareTypeChaos, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + }, > + Out: false, > + }, > + { > + Name: "InvalidProtocolTypeARPPacket", > + In: &arp.Packet{ > + HardwareType: arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv6, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + }, > + Out: false, > + }, > + { > + Name: "InvalidHardwareAddrLenARPPacket", > + In: &arp.Packet{ > + HardwareType: arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 8, > + ProtocolAddrLen: 4, > + }, > + Out: false, > + }, > + { > + Name: "InvalidProtocolAddrLenARPPacket", > + In: &arp.Packet{ > + HardwareType: arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 16, > + }, > + Out: false, > + }, > + } > + for _, tcase := range table { > + t.Run(tcase.Name, func(tt *testing.T) { > + assert.Equalf(tt, tcase.Out, > isValidARPPacket(tcase.In), "expected the result to be %v", tcase.Out) > + }) > + } > +} > + > +type updateBindingsArgs struct { > + Pkt *arp.Packet > + VID *uint16 > + Time time.Time > +} > + > +type updateBindingsCase struct { > + Name string > + BindingsFixture map[string]Binding > + In updateBindingsArgs > + Out []Result > +} > + > +func TestUpdateBindings(t *testing.T) { > + timestamp := time.Now() > + testIP1 := net.ParseIP("10.0.0.1").To4() > + testIP2 := net.ParseIP("10.0.0.2").To4() > + table := []updateBindingsCase{ > + { > + Name: "NewRequestPacket", > + In: updateBindingsArgs{ > + Pkt: &arp.Packet{ > + HardwareType: > arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + OpCode: arp.OpRequest, > + SendHwdAddr: net.HardwareAddr{0xc0, > 0xff, 0xee, 0x15, 0xc0, 0x01}, > + SendIPAddr: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + TgtIPAddr: > netip.AddrFrom4([4]byte{testIP2[0], testIP2[1], testIP2[2], testIP2[3]}), > + }, > + Time: timestamp, > + }, > + Out: []Result{ > + { > + IP: "10.0.0.1", > + MAC: "c0:ff:ee:15:c0:01", > + Time: timestamp.Unix(), > + Event: EventNew, > + }, > + }, > + }, > + { > + Name: "NewReplyPacket", > + In: updateBindingsArgs{ > + Pkt: &arp.Packet{ > + HardwareType: > arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + OpCode: arp.OpReply, > + SendHwdAddr: net.HardwareAddr{0xc0, > 0xff, 0xee, 0x15, 0xc0, 0x01}, > + SendIPAddr: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + TgtHwdAddr: net.HardwareAddr{0xc0, > 0xff, 0xee, 0x15, 0xc0, 0x1d}, > + TgtIPAddr: > netip.AddrFrom4([4]byte{testIP2[0], testIP2[1], testIP2[2], testIP2[3]}), > + }, > + Time: timestamp, > + }, > + Out: []Result{ > + { > + IP: "10.0.0.1", > + MAC: "c0:ff:ee:15:c0:01", > + Time: timestamp.Unix(), > + Event: EventNew, > + }, > + { > + IP: "10.0.0.2", > + MAC: "c0:ff:ee:15:c0:1d", > + Time: timestamp.Unix(), > + Event: EventNew, > + }, > + }, > + }, > + { > + Name: "NewVLANPacket", > + In: updateBindingsArgs{ > + Pkt: &arp.Packet{ > + HardwareType: > arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + OpCode: arp.OpRequest, > + SendHwdAddr: net.HardwareAddr{0xc0, > 0xff, 0xee, 0x15, 0xc0, 0x01}, > + SendIPAddr: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + TgtIPAddr: > netip.AddrFrom4([4]byte{testIP2[0], testIP2[1], testIP2[2], testIP2[3]}), > + }, > + VID: uint16Pointer(2), > + Time: timestamp, > + }, > + Out: []Result{ > + { > + IP: "10.0.0.1", > + MAC: "c0:ff:ee:15:c0:01", > + Time: timestamp.Unix(), > + VID: uint16Pointer(2), > + Event: EventNew, > + }, > + }, > + }, > + { > + Name: "Refresh", > + BindingsFixture: map[string]Binding{ > + "0_10.0.0.1": Binding{ > + IP: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + MAC: net.HardwareAddr{0xc0, 0xff, > 0xee, 0x15, 0xc0, 0x01}, > + Time: timestamp, > + }, > + }, > + In: updateBindingsArgs{ > + Pkt: &arp.Packet{ > + HardwareType: > arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + OpCode: arp.OpRequest, > + SendHwdAddr: net.HardwareAddr{0xc0, > 0xff, 0xee, 0x15, 0xc0, 0x01}, > + SendIPAddr: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + TgtIPAddr: > netip.AddrFrom4([4]byte{testIP2[0], testIP2[1], testIP2[2], testIP2[3]}), > + }, > + Time: timestamp.Add(seenAgainThreshold + > time.Second), > + }, > + Out: []Result{ > + { > + IP: "10.0.0.1", > + MAC: "c0:ff:ee:15:c0:01", > + Time: timestamp.Add(seenAgainThreshold > + time.Second).Unix(), > + Event: EventRefreshed, > + }, > + }, > + }, > + { > + Name: "Move", > + BindingsFixture: map[string]Binding{ > + "0_10.0.0.1": Binding{ > + IP: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + MAC: net.HardwareAddr{0xc0, 0xff, > 0xee, 0x15, 0xc0, 0x01}, > + Time: timestamp, > + }, > + }, > + In: updateBindingsArgs{ > + Pkt: &arp.Packet{ > + HardwareType: > arp.HardwareTypeEthernet, > + ProtocolType: arp.ProtocolTypeIPv4, > + HardwareAddrLen: 6, > + ProtocolAddrLen: 4, > + OpCode: arp.OpRequest, > + SendHwdAddr: net.HardwareAddr{0xc0, > 0xff, 0xee, 0x15, 0xc0, 0x1d}, > + SendIPAddr: > netip.AddrFrom4([4]byte{testIP1[0], testIP1[1], testIP1[2], testIP1[3]}), > + TgtIPAddr: > netip.AddrFrom4([4]byte{testIP2[0], testIP2[1], testIP2[2], testIP2[3]}), > + }, > + Time: timestamp, > + }, > + Out: []Result{ > + { > + IP: "10.0.0.1", > + MAC: "c0:ff:ee:15:c0:1d", > + Time: timestamp.Unix(), > + Event: EventMoved, > + }, > + }, > + }, > + } > + for _, tcase := range table { > + t.Run(tcase.Name, func(tt *testing.T) { > + svc := NewService("lo") > + if tcase.BindingsFixture != nil { > + svc.bindings = tcase.BindingsFixture > + } > + res := svc.updateBindings(tcase.In.Pkt, tcase.In.VID, > tcase.In.Time) > + for i, expected := range tcase.Out { > + var expectedVID int > + if expected.VID != nil { > + expectedVID = int(*expected.VID) > + } > + assert.Equalf(tt, expected.IP, res[i].IP, > "expected Result at index of %d to have the IP %s", i, expected.IP) > + assert.Equalf(tt, expected.MAC, res[i].MAC, > "expected Result at index of %d to have the MAC %s", i, expected.MAC) > + assert.Equalf(tt, expected.VID, res[i].VID, > "expected Result at index of %d to have the VID %d", i, expectedVID) > + assert.Equalf(tt, expected.Time, res[i].Time, > "expected Result at index of %d to have the Time of %d", i, > int(expected.Time)) > + assert.Equalf(tt, expected.Event, res[i].Event, > "expected Result at index of %d to have the Event of %s", i, expected.Event) > + } > + }) > + } > +} > + > +type handlePacketCase struct { > + Name string > + In pcap.Packet > + Out []Result > + Err error > +} > + > +func TestServiceHandlePacket(t *testing.T) { > + timestamp := time.Now() > + table := []handlePacketCase{ > + { > + Name: "ValidRequestPacket", > + In: pcap.Packet{ > + // generated from tcpdump > + B: []byte{ > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > 0x84, 0x39, 0xc0, 0x0b, 0x22, 0x25, 0x81, 0x00, 0x00, 0x02, > + 0x08, 0x06, 0x00, 0x01, 0x08, 0x00, > 0x06, 0x04, 0x00, 0x01, 0x84, 0x39, 0xc0, 0x0b, 0x22, 0x25, > + 0xc0, 0xa8, 0x0a, 0x1a, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0xc0, 0xa8, 0x0a, 0x19, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + }, > + Info: gopacket.CaptureInfo{ > + Timestamp: timestamp, > + }, > + }, > + Out: []Result{ > + { > + IP: "192.168.10.26", > + MAC: "84:39:c0:0b:22:25", > + VID: uint16Pointer(2), > + Time: timestamp.Unix(), > + Event: EventNew, > + }, > + }, > + }, > + { > + Name: "ValidReplyPacket", > + In: pcap.Packet{ > + B: []byte{ > + 0x24, 0x4b, 0xfe, 0xe1, 0xea, 0x26, > 0x80, 0x61, 0x5f, 0x08, 0xfc, 0x16, 0x08, 0x06, 0x00, 0x01, > + 0x08, 0x00, 0x06, 0x04, 0x00, 0x02, > 0x80, 0x61, 0x5f, 0x08, 0xfc, 0x16, 0xc0, 0xa8, 0x01, 0x6c, > + 0x24, 0x4b, 0xfe, 0xe1, 0xea, 0x26, > 0xc0, 0xa8, 0x01, 0x50, > + }, > + }, > + Out: []Result{ > + { > + IP: "192.168.1.108", > + MAC: "80:61:5f:08:fc:16", > + VID: nil, > + Time: timestamp.Unix(), > + Event: EventNew, > + }, > + { > + IP: "192.168.1.80", > + MAC: "24:4b:fe:e1:ea:26", > + VID: nil, > + Time: timestamp.Unix(), > + Event: EventNew, > + }, > + }, > + }, > + { > + Name: "EmptyPacket", > + Err: ErrEmptyPacket, > + }, > + { > + Name: "MalformedPacket", > + In: pcap.Packet{ > + B: []byte{ > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > 0x84, 0x39, 0xc0, 0x0b, 0x22, > + 0x08, 0x06, 0x00, 0x01, 0x08, 0x06, > 0x04, 0x00, 0x01, 0x84, > + 0xc0, 0xa8, 0x0a, 0x1a, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0xc0, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, > + }, > + }, > + // should return nil, nil > + }, > + { > + Name: "ShortPacket", > + In: pcap.Packet{ > + B: []byte{ > + 0x24, 0x4b, 0xfe, 0xe1, 0xea, 0x26, > 0x80, 0x61, 0x5f, 0x08, 0xfc, 0x16, 0x08, 0x06, 0x00, 0x01, > + 0x08, 0x00, 0x06, 0x04, 0x00, 0x02, > 0x80, 0xfc, 0x16, 0xc0, 0xa8, 0x01, 0x6c, > + 0x24, 0x4b, 0xfe, 0xe1, 0xea, 0x26, > 0xc0, 0xa8, 0x01, 0x50, > + }, > + }, > + Err: arp.ErrMalformedPacket, > + }, > + } > + > + svc := NewService("") > + > + for _, tcase := range table { > + t.Run(tcase.Name, func(tt *testing.T) { > + res, err := svc.handlePacket(tcase.In) > + assert.ErrorIsf(tt, err, tcase.Err, "expected > handlePacket to return an error of: %s", tcase.Err) > + if tcase.Out != nil { > + for i, expected := range tcase.Out { > + assert.Equalf(tt, expected.IP, > res[i].IP, "expected result at index %d to have an IP address of %s", i, > expected.IP) > + assert.Equalf(tt, expected.MAC, > res[i].MAC, "expected result at index %d to have a MAC address of %s", i, > expected.MAC) > + assert.Equalf(tt, expected.VID, > res[i].VID, "expected result at index %d to have a VID of %v", i, > expected.VID) > + assert.Equalf(tt, expected.Time, > res[i].Time, "expected result at index %d to have a Time of %s", i, > expected.Time) > + } > + } else { > + assert.Nil(tt, res) > + } > + }) > + } > +} -- https://code.launchpad.net/~cgrabowski/maas/+git/maas/+merge/441702 Your team MAAS Committers is subscribed to branch maas:master. -- Mailing list: https://launchpad.net/~sts-sponsors Post to : sts-sponsors@lists.launchpad.net Unsubscribe : https://launchpad.net/~sts-sponsors More help : https://help.launchpad.net/ListHelp