update lease when device moves to different network (#777)

* update lease when device moves to different network

closes #762

* fix test

* install deps for codeql

* ignore api role config for safe export

.github/workflows/codeql-analysis.yml

@@ -29,6 +29,7 @@ jobs:
           node-version: "20"
           cache: "npm"
           cache-dependency-path: web/package-lock.json
+      - run: make install-deps
       - name: Initialize CodeQL
         uses: github/codeql-action/init@v2
         with:

.github/workflows/test.yml

@@ -26,13 +26,10 @@ jobs:
         run: |
           npm ci
           npm run build
-      - name: Setup test env
-        run: make test-env-start
-      - name: Run tests & coverage
-        run: |
-          make install-deps test
-      - name: Stop test env
-        if: ${{ always() }}
+      - run: make test-env-start
+      - run: make install-deps
+      - run: make test
+      - if: ${{ always() }}
         run: make test-env-stop
       - if: ${{ always() }}
         uses: codecov/codecov-action@v3
@@ -55,10 +52,7 @@ jobs:
         run: |
           npm ci
           npm run build
-      - name: Install deps
-        run: |
-          make install-deps
-      - name: golangci-lint
-        uses: golangci/golangci-lint-action@v3
+      - run: make install-deps
+      - uses: golangci/golangci-lint-action@v3
         with:
           args: --timeout 5000s

pkg/roles/api/api_transport.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	"beryju.io/gravity/pkg/extconfig"
+	instanceTypes "beryju.io/gravity/pkg/instance/types"
 	apiTypes "beryju.io/gravity/pkg/roles/api/types"
 	tsdbTypes "beryju.io/gravity/pkg/roles/tsdb/types"
 	"github.com/swaggest/usecase"
@@ -31,6 +32,8 @@ func (r *Role) ignoredPrefixes() []string {
 		r.i.KV().Key(apiTypes.KeyRole, apiTypes.KeySessions).String(),
 		r.i.KV().Key(apiTypes.KeyRole, apiTypes.KeyTokens).String(),
 		r.i.KV().Key(apiTypes.KeyRole, apiTypes.KeyUsers).String(),
+		// Contains API role config (cookie secret, OIDC config)
+		r.i.KV().Key(instanceTypes.KeyInstance, instanceTypes.KeyRole, apiTypes.KeyRole).String(),
 		// Noisy data we don't need
 		r.i.KV().Key(tsdbTypes.KeyRole, tsdbTypes.KeySystem).String(),
 	}

pkg/roles/api/api_transport_test.go

@@ -29,7 +29,9 @@ func TestExport(t *testing.T) {
 	)
 	assert.NoError(t, err)
 
-	err = role.APIClusterExport().Interact(ctx, struct{}{}, &output)
+	err = role.APIClusterExport().Interact(ctx, api.APIExportInput{
+		Safe: true,
+	}, &output)
 	assert.NoError(t, err)
 	assert.Equal(t, api.APIExportOutput{
 		Entries: []api.APITransportEntry{

pkg/roles/dhcp/dhcp_handler4_request.go

@@ -31,13 +31,3 @@ func (r *Role) HandleDHCPRequest4(req *Request4) *dhcpv4.DHCPv4 {
 	rep.UpdateOption(dhcpv4.OptMessageType(dhcpv4.MessageTypeAck))
 	return rep
 }
-
-func (r *Role) FindLease(req *Request4) *Lease {
-	r.leasesM.RLock()
-	defer r.leasesM.RUnlock()
-	lease, ok := r.leases[r.DeviceIdentifier(req.DHCPv4)]
-	if !ok {
-		return nil
-	}
-	return lease
-}

pkg/roles/dhcp/dhcp_handler4_request_test.go

@@ -1,6 +1,7 @@
 package dhcp_test
 
 import (
+	"net"
 	"testing"
 	"time"
 
@@ -128,3 +129,87 @@ func TestDHCPRequestDNS(t *testing.T) {
 	assert.Equal(t, 86400*time.Second, res.IPAddressLeaseTime(1*time.Second))
 	assert.Equal(t, "test.gravity.beryju.io", res.DomainName())
 }
+
+func TestDHCPRequestDNS_ChangedScope(t *testing.T) {
+	rootInst := instance.New()
+	ctx := tests.Context()
+	inst := rootInst.ForRole("dhcp", ctx)
+	role := dhcp.New(inst)
+	Cleanup()
+
+	tests.PanicIfError(inst.KV().Put(
+		ctx,
+		inst.KV().Key(
+			types.KeyRole,
+			types.KeyScopes,
+			"test",
+		).String(),
+		tests.MustJSON(dhcp.Scope{
+			SubnetCIDR: "10.100.0.0/24",
+			Default:    true,
+			TTL:        86400,
+			DNS: &dhcp.ScopeDNS{
+				Zone:              "test.gravity.beryju.io",
+				AddZoneInHostname: true,
+			},
+			IPAM: map[string]string{
+				"type":        "internal",
+				"range_start": "10.100.0.100",
+				"range_end":   "10.100.0.250",
+			},
+		}),
+	))
+	tests.PanicIfError(inst.KV().Put(
+		ctx,
+		inst.KV().Key(
+			types.KeyRole,
+			types.KeyScopes,
+			"test2",
+		).String(),
+		tests.MustJSON(dhcp.Scope{
+			SubnetCIDR: "10.200.0.0/24",
+			TTL:        86400,
+			DNS: &dhcp.ScopeDNS{
+				Zone:              "test2.gravity.beryju.io",
+				AddZoneInHostname: true,
+			},
+			IPAM: map[string]string{
+				"type":        "internal",
+				"range_start": "10.200.0.100",
+				"range_end":   "10.200.0.250",
+			},
+		}),
+	))
+
+	tests.PanicIfError(role.Start(ctx, []byte(tests.MustJSON(dhcp.RoleConfig{
+		Port: 1067,
+	}))))
+	defer role.Stop()
+
+	// First ensure the lease is created as we expect
+	req, err := dhcpv4.FromBytes(DHCPRequestPayload)
+	assert.NoError(t, err)
+	req4 := role.NewRequest4(req)
+	res := role.Handler4(req4)
+	assert.NotNil(t, res)
+	assert.Equal(t, "10.100.0.100", res.YourIPAddr.String())
+	ones, bits := res.SubnetMask().Size()
+	assert.Equal(t, 24, ones)
+	assert.Equal(t, 32, bits)
+	assert.Equal(t, "44:90:bb:66:32:04", res.ClientHWAddr.String())
+	assert.Equal(t, 86400*time.Second, res.IPAddressLeaseTime(1*time.Second))
+	assert.Equal(t, "test.gravity.beryju.io", res.DomainName())
+
+	// Now we're requesting an IP from another subnet, so the lease should move
+	req.GatewayIPAddr = net.ParseIP("10.200.0.1")
+	req4 = role.NewRequest4(req)
+	res = role.Handler4(req4)
+	assert.NotNil(t, res)
+	assert.Equal(t, "10.200.0.100", res.YourIPAddr.String())
+	ones, bits = res.SubnetMask().Size()
+	assert.Equal(t, 24, ones)
+	assert.Equal(t, 32, bits)
+	assert.Equal(t, "44:90:bb:66:32:04", res.ClientHWAddr.String())
+	assert.Equal(t, 86400*time.Second, res.IPAddressLeaseTime(1*time.Second))
+	assert.Equal(t, "test2.gravity.beryju.io", res.DomainName())
+}

pkg/roles/dhcp/ipam.go

@@ -6,7 +6,11 @@ import (
 )
 
 type IPAM interface {
+	// Return the next free IP address, could be sequential or random
 	NextFreeAddress() *netip.Addr
+	// Check if IP address is in usage (should also check if IP is in specified range and subnet)
+	// Can optionally also check if the IP address is pingable
 	IsIPFree(netip.Addr) bool
+	// Get the subnet mask of the scope
 	GetSubnetMask() net.IPMask
 }

pkg/roles/dhcp/leases.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"math"
 	"net"
+	"net/netip"
 	"strings"
 	"time"
 
@@ -34,11 +35,36 @@ type Lease struct {
 	AddressLeaseTime string `json:"addressLeaseTime,omitempty"`
 	ScopeKey         string `json:"scopeKey"`
 	DNSZone          string `json:"dnsZone,omitempty"`
-	Expiry           int64  `json:"expiry"`
+	// Set to -1 for a reservation
+	Expiry int64 `json:"expiry"`
 
 	etcdKey string
 }
 
+func (r *Role) FindLease(req *Request4) *Lease {
+	r.leasesM.RLock()
+	defer r.leasesM.RUnlock()
+	lease, ok := r.leases[r.DeviceIdentifier(req.DHCPv4)]
+	if !ok {
+		return nil
+	}
+	// Check if the leases's scope matches the expected scope to handle this request
+	expectedScope := r.findScopeForRequest(req)
+	if expectedScope != nil && lease.scope != expectedScope {
+		// We have a specific scope to handle this request but it doesn't match the lease
+		lease.scope = expectedScope
+		lease.setLeaseIP(req)
+		lease.log.Info("Re-assigning address for lease due to changed request scope", zap.String("newIP", lease.Address))
+		go func() {
+			err := lease.Put(req.Context, lease.scope.TTL)
+			if err != nil {
+				r.log.Warn("failed to update lease", zap.Error(err))
+			}
+		}()
+	}
+	return lease
+}
+
 func (r *Role) NewLease(identifier string) *Lease {
 	return &Lease{
 		inst:       r.i,
@@ -48,6 +74,27 @@ func (r *Role) NewLease(identifier string) *Lease {
 	}
 }
 
+func (l *Lease) setLeaseIP(req *Request4) {
+	requestedIP := req.RequestedIPAddress()
+	if requestedIP != nil {
+		req.log.Debug("checking requested IP", zap.String("ip", requestedIP.String()))
+		ip, err := netip.ParseAddr(requestedIP.String())
+		if err != nil {
+			req.log.Warn("failed to parse requested ip", zap.Error(err))
+		} else if l.scope.ipam.IsIPFree(ip) {
+			req.log.Debug("requested IP is free", zap.String("ip", requestedIP.String()))
+			l.Address = requestedIP.String()
+			return
+		}
+	}
+	ip := l.scope.ipam.NextFreeAddress()
+	if ip == nil {
+		return
+	}
+	req.log.Debug("using next free IP from IPAM")
+	l.Address = ip.String()
+}
+
 func (r *Role) leaseFromKV(raw *mvccpb.KeyValue) (*Lease, error) {
 	prefix := r.i.KV().Key(
 		types.KeyRole,

pkg/roles/dhcp/scopes.go

@@ -135,35 +135,13 @@ func (s *Scope) match(peer net.IP) int {
 
 func (s *Scope) createLeaseFor(req *Request4) *Lease {
 	ident := s.role.DeviceIdentifier(req.DHCPv4)
-	lease := &Lease{
-		Identifier: ident,
+	lease := s.role.NewLease(ident)
+	lease.Hostname = req.HostName()
 
-		Hostname: req.HostName(),
-		ScopeKey: s.Name,
-
-		inst:  s.inst,
-		log:   req.log.With(zap.String("lease", ident)),
-		scope: s,
-	}
-	requestedIP := req.RequestedIPAddress()
-	if requestedIP != nil {
-		req.log.Debug("checking requested IP", zap.String("ip", requestedIP.String()))
-		ip, err := netip.ParseAddr(requestedIP.String())
-		if err != nil {
-			req.log.Warn("failed to parse requested ip", zap.Error(err))
-		} else if s.ipam.IsIPFree(ip) {
-			req.log.Debug("requested IP is free", zap.String("ip", requestedIP.String()))
-			lease.Address = requestedIP.String()
-		}
-	}
-	if lease.Address == "" {
-		ip := s.ipam.NextFreeAddress()
-		if ip == nil {
-			return nil
-		}
-		lease.Address = ip.String()
-	}
-	req.log.Info("creating new DHCP lease", zap.String("ip", requestedIP.String()), zap.String("lease", ident))
+	lease.scope = s
+	lease.ScopeKey = s.Name
+	lease.setLeaseIP(req)
+	req.log.Info("creating new DHCP lease", zap.String("ip", lease.Address), zap.String("identifier", ident))
 	return lease
 }