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
