Looks good overall. I've added some suggestions for code style/linters and there is a potential issue where a timestamp will remain unset/zero valued in the caller.
Diff comments: > diff --git a/src/maasagent/cmd/netmon/main.go > b/src/maasagent/cmd/netmon/main.go > index e83655f..7b60e83 100644 > --- a/src/maasagent/cmd/netmon/main.go > +++ b/src/maasagent/cmd/netmon/main.go > @@ -1,9 +1,83 @@ > 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" > + "os" > + "os/signal" > + "syscall" > + > + "github.com/rs/zerolog" > + "github.com/rs/zerolog/log" > + "golang.org/x/sync/errgroup" > + > "launchpad.net/maas/maas/src/maasagent/internal/netmon" > ) > > +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().Msg("Please provide an interface to monitor") > + return 2 > + } > + iface := os.Args[1] For readability, we tend add new lines after each stanza, block, or logical step. For example, here we would add a new line after the closing brace of the if statement. We've written a small shell script to enforce this here: https://github.com/lxc/lxd/blob/49b9c78cc50ab66178c32799c44ebe517e75b7ee/test/lint/newline-after-block.sh We use many more linters for LXD. Primarily this is managed via golangci-lint (https://github.com/golangci/golangci-lint) but there are a few more custom ones. This is defined in our Makefile here: https://github.com/lxc/lxd/blob/49b9c78cc50ab66178c32799c44ebe517e75b7ee/Makefile#L251-L271 > + > + ctx, cancel := context.WithCancel(context.Background()) > + > + sigC := make(chan os.Signal, 2) > + signal.Notify(sigC, syscall.SIGTERM, syscall.SIGINT) > + > + 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/netmon/service.go > b/src/maasagent/internal/netmon/service.go > index 14ebdc6..11be56a 100644 > --- a/src/maasagent/internal/netmon/service.go > +++ b/src/maasagent/internal/netmon/service.go > @@ -1,3 +1,255 @@ > 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/ethernet" > +) > + > +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" Depending on how and where these events are used, it can be useful to implement a golang "enum" as a type alias of a uint8. These can still be json marshaled/unmarshaled by implementing json.Marshaler/json.Unmarshaler for the type. An example of this can be found at https://github.com/canonical/lxd-cloud/blob/4c2acf6c99449a0f9354f2437b681985de40fe1c/api/types/cell_state.go > +) > + > +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 *ethernet.ARPPacket, vid *uint16, > timestamp time.Time) (res []Result) { > + if timestamp.IsZero() { > + timestamp = time.Now() Here the value of `timestamp` is set to the current time, but that timestamp argument is passed by value. So the `time.Time` variable that was passed in by the caller is still zero (which may be intentional but to me is a bit confusing). Instead you could pass in `timestamp *time.Time` and change the above to: ```go if timestamp == nil || 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 == ethernet.OpReply { > + discoveredBindings = append(discoveredBindings, Binding{ > + IP: pkt.TgtIPAddr, > + MAC: pkt.TgtHwdAddr, > + VID: vid, > + Time: timestamp, > + }) > + } > + > + for _, discoveredBinding := range discoveredBindings { > + key := fmt.Sprintf("%d_%s", vidLabel, > discoveredBinding.IP.String()) > + binding, ok := s.bindings[key] > + if !ok { > + 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, > + }) > + continue > + } > + if bytes.Compare(binding.MAC, discoveredBinding.MAC) != 0 { > + s.bindings[key] = discoveredBinding > + res = append(res, Result{ > + 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, > + }) > + } > + } > + > + return res > +} > + > +func isValidARPPacket(pkt *ethernet.ARPPacket) bool { > + if pkt.HardwareType != ethernet.HardwareTypeEthernet && > pkt.HardwareType != ethernet.HardwareTypeExpEth { > + return false > + } > + if pkt.ProtocolType != ethernet.ProtocolTypeIPv4 && pkt.ProtocolType != > ethernet.ProtocolTypeARP { > + 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 := ðernet.EthernetFrame{} > + err := eth.UnmarshalBinary(pkt.B) > + if err != nil { > + return nil, err > + } > + > + if eth.EthernetType != ethernet.EthernetTypeVLAN && eth.EthernetType != > ethernet.EthernetTypeARP { > + log.Debug().Msg("skipping non-ARP packet") > + return nil, nil > + } > + > + var vid *uint16 > + if eth.EthernetType == ethernet.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) { > + 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, ethernet.ErrMalformedARPPacket) || errors.Is(err, > ethernet.ErrMalformedVLAN) || errors.Is(err, ethernet.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() > + err = hndlr.SetBPFFilter("ether proto arp") > + if err != nil { > + return err > + } > + 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 > + } > + } > + } > +} -- 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