diff --git a/dhcp/lease.go b/dhcp/lease.go index d8f6c70..f10e2cd 100644 --- a/dhcp/lease.go +++ b/dhcp/lease.go @@ -24,9 +24,7 @@ type leaseEntry struct { Expiry string `json:"expiry"` } -// evictedLease describes a lease entry displaced by add(). Same-MAC -// rebind leaves the old IP's route + pool slot orphaned; other-MAC -// conflict is reported for logging. +// evictedLease is a lease displaced by add(): a same-MAC rebind orphans the old IP's route and pool slot, an other-MAC conflict is log-only. type evictedLease struct { MAC string IP net.IP @@ -54,12 +52,10 @@ func (s *leaseStore) add(mac net.HardwareAddr, ip net.IP, duration time.Duration newIP := ip.To4() var evicted []evictedLease - // Same MAC, different IP — surface the old IP so the caller can clean it up. if prev, ok := s.leases[key]; ok && !prev.IP.Equal(newIP) { evicted = append(evicted, evictedLease{MAC: key, IP: prev.IP}) } - // Other MAC, same IP — drop the conflicting active lease. for k, l := range s.leases { if l.IP.Equal(newIP) && k != key && now.Before(l.Expiry) { delete(s.leases, k) diff --git a/dhcp/server.go b/dhcp/server.go index 86f7343..0837532 100644 --- a/dhcp/server.go +++ b/dhcp/server.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "net" - "sync" "time" "github.com/insomniacslk/dhcp/dhcpv4" @@ -45,8 +44,6 @@ type Server struct { offers *pendingOffers linkIndex int // cached kernel interface index for route operations - mu sync.Mutex - stopped bool } // New creates a DHCP server. IPs are the allocatable pool (excluding gateway). @@ -97,9 +94,6 @@ func (s *Server) Run(ctx context.Context) error { select { case <-ctx.Done(): - s.mu.Lock() - s.stopped = true - s.mu.Unlock() _ = srv.Close() if err := s.leases.save(); err != nil { logger.Error(ctx, err, "persist leases on shutdown") @@ -107,12 +101,6 @@ func (s *Server) Run(ctx context.Context) error { logger.Info(ctx, "DHCP server stopped") return nil case err := <-errCh: - s.mu.Lock() - stopped := s.stopped - s.mu.Unlock() - if stopped { - return nil - } return fmt.Errorf("DHCP server: %w", err) } } diff --git a/platform/gke/provision.go b/platform/gke/provision.go index f8f51be..8dddeb9 100644 --- a/platform/gke/provision.go +++ b/platform/gke/provision.go @@ -1,6 +1,7 @@ package gke import ( + "cmp" "context" "fmt" "strings" @@ -13,10 +14,7 @@ import ( func (g *GKE) ProvisionNetwork(ctx context.Context, cfg *platform.Config) (*platform.NetworkResult, error) { logger := log.WithFunc("gke.ProvisionNetwork") - primaryNIC := cfg.PrimaryNIC - if primaryNIC == "" { - primaryNIC = platform.DefaultNIC(g.Name()) - } + primaryNIC := cmp.Or(cfg.PrimaryNIC, platform.DefaultNIC(g.Name())) instance, zone, project, subnet, err := fetchMetadata(ctx) if err != nil { diff --git a/platform/ip.go b/platform/ip.go index 221f160..aa00a6c 100644 --- a/platform/ip.go +++ b/platform/ip.go @@ -85,6 +85,10 @@ func SubnetIPs(cidr, gateway string, count int) ([]string, error) { return nil, fmt.Errorf("gateway %s collides with network/broadcast of %s", gateway, cidr) } + if count < 0 { + return nil, fmt.Errorf("pool size %d is negative", count) + } + ips := make([]string, 0, count) for addr := prefix.Masked().Addr().Next(); prefix.Contains(addr) && len(ips) < count; addr = addr.Next() { if addr == gwAddr || addr == bcast { diff --git a/platform/ip_test.go b/platform/ip_test.go index e2db9df..1f6747c 100644 --- a/platform/ip_test.go +++ b/platform/ip_test.go @@ -136,6 +136,14 @@ func TestSubnetIPs_CountZero(t *testing.T) { } } +func TestSubnetIPs_NegativeCountIsError(t *testing.T) { + t.Parallel() + + if _, err := SubnetIPs("10.0.0.0/24", "10.0.0.1", -1); err == nil { + t.Fatal("expected error for negative count") + } +} + func TestFirstHostIP(t *testing.T) { t.Parallel() diff --git a/platform/volcengine/eni.go b/platform/volcengine/eni.go index 369dcd6..6f28063 100644 --- a/platform/volcengine/eni.go +++ b/platform/volcengine/eni.go @@ -13,6 +13,7 @@ import ( const ( createPropagationDelay = 2 * time.Second attachPropagationDelay = 4 * time.Second + orphanDeleteTimeout = 15 * time.Second ) type networkInterface struct { @@ -26,11 +27,19 @@ type networkInterface struct { } `json:"PrivateIpSets"` } -func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, count int) ([]string, error) { - logger := log.WithFunc("volcengine.createAndAttachENIs") - eniIDs := make([]string, 0, count) +func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, count int) ([]networkInterface, error) { + logger := log.WithFunc("volcengine.ensureENIs") - for i := 1; i <= count; i++ { + existing, err := listENIs(ctx, instanceID) + if err != nil { + return nil, fmt.Errorf("list existing ENIs: %w", err) + } + result := reusableENIs(existing, count) + if len(result) > 0 { + logger.Infof(ctx, "reusing %d existing ENI(s)", len(result)) + } + + for i := len(result) + 1; i <= count; i++ { out, err := veRun( ctx, "vpc", "CreateNetworkInterface", "--SubnetId", subnetID, @@ -38,7 +47,7 @@ func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix "--NetworkInterfaceName", fmt.Sprintf("%s-eni-%d", prefix, i), ) if err != nil { - return nil, fmt.Errorf("create ENI %d: %w", i, err) + return result, fmt.Errorf("create ENI %d: %w", i, err) } var resp struct { Result struct { @@ -46,12 +55,17 @@ func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix } `json:"Result"` } if unmarshalErr := json.Unmarshal(out, &resp); unmarshalErr != nil { - return nil, fmt.Errorf("parse create ENI %d response: %w", i, unmarshalErr) + return result, fmt.Errorf("parse create ENI %d response: %w", i, unmarshalErr) } eniID := resp.Result.NetworkInterfaceID if err := sleepCtx(ctx, createPropagationDelay); err != nil { - return eniIDs, err + delCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), orphanDeleteTimeout) + if _, delErr := veRun(delCtx, "vpc", "DeleteNetworkInterface", "--NetworkInterfaceId", eniID); delErr != nil { + logger.Warnf(ctx, "delete orphan ENI %s: %v", eniID, delErr) + } + cancel() + return result, err } _, attachErr := veRun( @@ -72,13 +86,26 @@ func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix } if err := sleepCtx(ctx, attachPropagationDelay); err != nil { - return eniIDs, err + return result, err } - eniIDs = append(eniIDs, eniID) + result = append(result, networkInterface{NetworkInterfaceID: eniID}) logger.Infof(ctx, "created and attached ENI %s (%d/%d)", eniID, i, count) } - return eniIDs, nil + return result, nil +} + +func reusableENIs(enis []networkInterface, count int) []networkInterface { + var reusable []networkInterface + for _, e := range enis { + if e.Type != eniTypePrimary { + reusable = append(reusable, e) + } + } + if len(reusable) > count { + reusable = reusable[:count] + } + return reusable } func assignSecondaryIPs(ctx context.Context, eniID string, count int) ([]string, error) { diff --git a/platform/volcengine/eni_test.go b/platform/volcengine/eni_test.go new file mode 100644 index 0000000..d3e51da --- /dev/null +++ b/platform/volcengine/eni_test.go @@ -0,0 +1,100 @@ +package volcengine + +import ( + "encoding/json" + "testing" +) + +const ( + eniList1Primary7Secondary = `{ + "Result": { + "NetworkInterfaceSets": [ + {"NetworkInterfaceId": "eni-primary", "Type": "primary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": true, "PrivateIpAddress": "10.0.0.1"}]}}, + {"NetworkInterfaceId": "eni-1", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.2"}]}}, + {"NetworkInterfaceId": "eni-2", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.3"}]}}, + {"NetworkInterfaceId": "eni-3", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.4"}]}}, + {"NetworkInterfaceId": "eni-4", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.5"}]}}, + {"NetworkInterfaceId": "eni-5", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.6"}]}}, + {"NetworkInterfaceId": "eni-6", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.7"}]}}, + {"NetworkInterfaceId": "eni-7", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.8"}]}} + ] + } +}` + + eniList1Primary3Secondary = `{ + "Result": { + "NetworkInterfaceSets": [ + {"NetworkInterfaceId": "eni-primary", "Type": "primary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": true, "PrivateIpAddress": "10.0.0.1"}]}}, + {"NetworkInterfaceId": "eni-1", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.2"}]}}, + {"NetworkInterfaceId": "eni-2", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.3"}]}}, + {"NetworkInterfaceId": "eni-3", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.4"}]}} + ] + } +}` +) + +func TestReusableENIs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + fixture string + count int + want int + }{ + {"exact match runs zero shortfall", eniList1Primary7Secondary, 7, 7}, + {"fewer than count returns all", eniList1Primary3Secondary, 7, 3}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + enis := unmarshalENIList(t, tt.fixture) + if got := len(reusableENIs(enis, tt.count)); got != tt.want { + t.Errorf("got %d reusable ENIs, want %d", got, tt.want) + } + }) + } +} + +func TestIPShortfall_FullENIIsZero(t *testing.T) { + t.Parallel() + + eni := newENIWithSecondaryIPs("eni-full", ipsPerENI) + + var existing int + for _, pip := range eni.PrivateIPSets.PrivateIPSet { + if !pip.Primary { + existing++ + } + } + if shortfall := ipsPerENI - existing; shortfall != 0 { + t.Errorf("got shortfall %d, want 0", shortfall) + } +} + +func unmarshalENIList(t *testing.T, fixture string) []networkInterface { + t.Helper() + + var resp struct { + Result struct { + NetworkInterfaceSets []networkInterface `json:"NetworkInterfaceSets"` + } `json:"Result"` + } + if err := json.Unmarshal([]byte(fixture), &resp); err != nil { + t.Fatalf("unmarshal fixture: %v", err) + } + return resp.Result.NetworkInterfaceSets +} + +func newENIWithSecondaryIPs(id string, n int) networkInterface { + eni := networkInterface{NetworkInterfaceID: id, Type: "secondary"} + for range n { + eni.PrivateIPSets.PrivateIPSet = append(eni.PrivateIPSets.PrivateIPSet, struct { + Primary bool `json:"Primary"` + PrivateIPAddress string `json:"PrivateIpAddress"` + }{PrivateIPAddress: "10.0.1.1"}) + } + return eni +} diff --git a/platform/volcengine/provision.go b/platform/volcengine/provision.go index 59de891..e3977f5 100644 --- a/platform/volcengine/provision.go +++ b/platform/volcengine/provision.go @@ -1,6 +1,7 @@ package volcengine import ( + "cmp" "context" "fmt" @@ -12,10 +13,7 @@ import ( func (v *Volcengine) ProvisionNetwork(ctx context.Context, cfg *platform.Config) (*platform.NetworkResult, error) { logger := log.WithFunc("volcengine.ProvisionNetwork") - primaryNIC := cfg.PrimaryNIC - if primaryNIC == "" { - primaryNIC = platform.DefaultNIC(v.Name()) - } + primaryNIC := cmp.Or(cfg.PrimaryNIC, platform.DefaultNIC(v.Name())) vpcID, err := fetchMeta(ctx, "/vpc-id") if err != nil { @@ -39,24 +37,34 @@ func (v *Volcengine) ProvisionNetwork(ctx context.Context, cfg *platform.Config) } logger.Infof(ctx, "instance id: %s", instanceID) - eniIDs, err := createAndAttachENIs(ctx, subnetID, sgID, instanceID, cfg.NodeName, enisPerNode) + enis, err := ensureENIs(ctx, subnetID, sgID, instanceID, cfg.NodeName, enisPerNode) if err != nil { - return nil, fmt.Errorf("create/attach ENIs: %w", err) + return nil, fmt.Errorf("ensure ENIs: %w", err) } - logger.Infof(ctx, "attached %d ENIs", len(eniIDs)) + logger.Infof(ctx, "%d ENIs ready", len(enis)) var allIPs []string - for _, eniID := range eniIDs { - ips, assignErr := assignSecondaryIPs(ctx, eniID, ipsPerENI) - if assignErr != nil { - // One ENI's failure is tolerable; the len(allIPs)==0 guard below aborts only if every ENI failed. - logger.Warnf(ctx, "assign secondary IPs to %s: %v", eniID, assignErr) - continue + for _, eni := range enis { + var existing []string + for _, pip := range eni.PrivateIPSets.PrivateIPSet { + if !pip.Primary { + existing = append(existing, pip.PrivateIPAddress) + } + } + allIPs = append(allIPs, existing...) + + if shortfall := ipsPerENI - len(existing); shortfall > 0 { + ips, assignErr := assignSecondaryIPs(ctx, eni.NetworkInterfaceID, shortfall) + if assignErr != nil { + // One ENI's failure is tolerable; the len(allIPs)==0 guard below aborts only if every ENI failed. + logger.Warnf(ctx, "assign secondary IPs to %s: %v", eni.NetworkInterfaceID, assignErr) + continue + } + allIPs = append(allIPs, ips...) } - allIPs = append(allIPs, ips...) } if len(allIPs) == 0 { - return nil, fmt.Errorf("no secondary IPs assigned across %d ENIs", len(eniIDs)) + return nil, fmt.Errorf("no secondary IPs assigned across %d ENIs", len(enis)) } logger.Infof(ctx, "assigned %d secondary IPs", len(allIPs)) diff --git a/pool/pool_test.go b/pool/pool_test.go index c9e7764..4129df9 100644 --- a/pool/pool_test.go +++ b/pool/pool_test.go @@ -74,7 +74,7 @@ func TestState_SaveAtomicTmpIgnored(t *testing.T) { } tmp := filepath.Join(dir, poolFileName+".tmp") - if err := os.WriteFile(tmp, []byte("{not json"), 0o644); err != nil { + if err := os.WriteFile(tmp, []byte("{not json"), filePerm); err != nil { t.Fatalf("write fake tmp: %v", err) }