sts-sponsors team mailing list archive

[Mark Laing <mp+441702@xxxxxxxxxxxxxxxxxx>]

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 := &ethernet.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.