vlan: don't index based on the vlan ID (#486)

The vlan ID may be repeated in a system as long as the vlan is set
for different parent interfaces. This change makes sure to map based
on the combination of parentID + Vlan (in the form of parentID-Vlan).

google_guest_agent/network/manager/common.go

@@ -108,18 +108,6 @@ func vlanInterfaceParentMap(nics map[int]metadata.VlanInterface, allEthernetInte
 	return vlans, nil
 }
 
-// vlanInterfaceListsIpv6 gets a list of VLAN IDs that support IPv6.
-func vlanInterfaceListsIpv6(nics map[int]VlanInterface) []int {
-	var googleIpv6Interfaces []int
-
-	for _, ni := range nics {
-		if ni.DHCPv6Refresh != "" {
-			googleIpv6Interfaces = append(googleIpv6Interfaces, ni.Vlan)
-		}
-	}
-	return googleIpv6Interfaces
-}
-
 // interfaceListsIpv4Ipv6 gets a list of interface names. The first list is a list of all
 // interfaces, and the second list consists of only interfaces that support IPv6.
 func interfaceListsIpv4Ipv6(nics []metadata.NetworkInterfaces) ([]string, []string) {

google_guest_agent/network/manager/common_test.go

@@ -16,7 +16,6 @@ package manager
 
 import (
 	"fmt"
-	"sort"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/guest-agent/metadata"
@@ -78,22 +77,6 @@ func TestVlanParentInterfaceFailure(t *testing.T) {
 	}
 }
 
-func TestVlanInterfaceListsIpv6(t *testing.T) {
-	nics := map[int]VlanInterface{
-		0: {VlanInterface: metadata.VlanInterface{Vlan: 4, DHCPv6Refresh: "123456"}},
-		1: {VlanInterface: metadata.VlanInterface{Vlan: 5}},
-		2: {VlanInterface: metadata.VlanInterface{Vlan: 6, MTU: 1234}},
-		3: {VlanInterface: metadata.VlanInterface{Vlan: 7, Mac: "acd", ParentInterface: "/parent/0", DHCPv6Refresh: "7890"}},
-	}
-	want := []int{4, 7}
-	got := vlanInterfaceListsIpv6(nics)
-	sort.Ints(got)
-
-	if diff := cmp.Diff(want, got); diff != "" {
-		t.Errorf("vlanInterfaceListsIpv6(%+v) returned unexpected diff (-want,+got)\n%s", nics, diff)
-	}
-}
-
 func TestVlanInterfaceParentMap(t *testing.T) {
 	tests := []struct {
 		name                  string

google_guest_agent/network/manager/dhclient_linux_test.go

@@ -49,7 +49,7 @@ func (d dhclientMockRunner) Quiet(ctx context.Context, name string, args ...stri
 		for _, arg := range args {
 			msg += fmt.Sprintf(" %v", arg)
 		}
-		return fmt.Errorf(msg)
+		return fmt.Errorf("%s", msg)
 	}
 	return nil
 }

google_guest_agent/network/manager/manager.go

@@ -83,7 +83,7 @@ type Interfaces struct {
 	EthernetInterfaces []metadata.NetworkInterfaces
 
 	// VlanInterfaces are the vLAN interfaces descriptors offered by metadata.
-	VlanInterfaces map[int]VlanInterface
+	VlanInterfaces map[string]VlanInterface
 }
 
 // guestAgentSection is the section added to guest-agent-written ini files to indicate
@@ -163,7 +163,8 @@ func reformatVlanNics(mds *metadata.Descriptor, nics *Interfaces, ethernetInterf
 		}
 
 		for vlanID, vlan := range vlans {
-			nics.VlanInterfaces[vlanID] = VlanInterface{VlanInterface: vlan, ParentInterfaceID: ethernetInterfaces[parentID]}
+			mapID := fmt.Sprintf("%d-%d", parentID, vlanID)
+			nics.VlanInterfaces[mapID] = VlanInterface{VlanInterface: vlan, ParentInterfaceID: ethernetInterfaces[parentID]}
 		}
 	}
 	return nil
@@ -191,7 +192,7 @@ func SetupInterfaces(ctx context.Context, config *cfg.Sections, mds *metadata.De
 
 	nics := &Interfaces{
 		EthernetInterfaces: mds.Instance.NetworkInterfaces,
-		VlanInterfaces:     map[int]VlanInterface{},
+		VlanInterfaces:     map[string]VlanInterface{},
 	}
 
 	interfaces, err := interfaceNames(nics.EthernetInterfaces)
@@ -335,7 +336,7 @@ func FallbackToDefault(ctx context.Context) error {
 func buildInterfacesFromAllPhysicalNICs() (*Interfaces, error) {
 	nics := &Interfaces{
 		EthernetInterfaces: nil,
-		VlanInterfaces:     map[int]VlanInterface{},
+		VlanInterfaces:     map[string]VlanInterface{},
 	}
 
 	interfaces, err := net.Interfaces()

google_guest_agent/network/manager/manager_test.go

@@ -335,11 +335,11 @@ func TestReformatVlanNics(t *testing.T) {
 			},
 		},
 	}}
-	nics := &Interfaces{VlanInterfaces: map[int]VlanInterface{}}
-	want := &Interfaces{VlanInterfaces: map[int]VlanInterface{
-		5: {VlanInterface: metadata.VlanInterface{Mac: "a", ParentInterface: "/computeMetadata/v1/instance/network-interfaces/0/", Vlan: 5}, ParentInterfaceID: "eth0"},
-		6: {VlanInterface: metadata.VlanInterface{Mac: "b", Vlan: 6, IP: "1.2.3.4"}, ParentInterfaceID: "eth0"},
-		7: {VlanInterface: metadata.VlanInterface{Mac: "c", Vlan: 7, DHCPv6Refresh: "123456"}, ParentInterfaceID: "eth1"},
+	nics := &Interfaces{VlanInterfaces: map[string]VlanInterface{}}
+	want := &Interfaces{VlanInterfaces: map[string]VlanInterface{
+		"0-5": {VlanInterface: metadata.VlanInterface{Mac: "a", ParentInterface: "/computeMetadata/v1/instance/network-interfaces/0/", Vlan: 5}, ParentInterfaceID: "eth0"},
+		"0-6": {VlanInterface: metadata.VlanInterface{Mac: "b", Vlan: 6, IP: "1.2.3.4"}, ParentInterfaceID: "eth0"},
+		"1-7": {VlanInterface: metadata.VlanInterface{Mac: "c", Vlan: 7, DHCPv6Refresh: "123456"}, ParentInterfaceID: "eth1"},
 	}}
 
 	ethernetInterfaces := []string{"eth0", "eth1"}
@@ -365,7 +365,7 @@ func TestReformatVlanNicsError(t *testing.T) {
 			},
 		},
 	}}
-	nics := &Interfaces{VlanInterfaces: map[int]VlanInterface{}}
+	nics := &Interfaces{VlanInterfaces: map[string]VlanInterface{}}
 
 	tests := []struct {
 		name               string

google_guest_agent/network/manager/netplan_linux.go

@@ -26,6 +26,7 @@ import (
 	"github.com/GoogleCloudPlatform/guest-agent/google_guest_agent/cfg"
 	"github.com/GoogleCloudPlatform/guest-agent/google_guest_agent/osinfo"
 	"github.com/GoogleCloudPlatform/guest-agent/google_guest_agent/run"
+	"github.com/GoogleCloudPlatform/guest-agent/metadata"
 	"github.com/GoogleCloudPlatform/guest-agent/utils"
 	"github.com/GoogleCloudPlatform/guest-logging-go/logger"
 )
@@ -468,28 +469,33 @@ func (n *netplan) vlanInterfaceName(parentInterface string, vlanID int) string {
 
 // SetupVlanInterface writes the apppropriate vLAN interfaces netplan configuration.
 func (n *netplan) SetupVlanInterface(ctx context.Context, config *cfg.Sections, nics *Interfaces) error {
+	var reload1, reload2, reload3 bool
+	var err error
+
 	toRemove, err := n.findVlanDiff(nics)
 	if err != nil {
 		return fmt.Errorf("unable to detect vlan nics to delete: %w", err)
 	}
 
-	reload1, err := n.rollbackVlanNics(ctx, toRemove)
-	if err != nil {
-		return fmt.Errorf("unable to remove vlan interfaces (%+v): %w", toRemove, err)
+	if toRemove != nil {
+		reload1, err = n.rollbackVlanNics(ctx, toRemove)
+		if err != nil {
+			return fmt.Errorf("unable to remove vlan interfaces (%+v): %w", toRemove, err)
+		}
 	}
 
-	reload2, err := n.writeNetplanVLANDropin(nics)
+	reload2, err = n.writeNetplanVLANDropin(nics)
 	if err != nil {
 		return fmt.Errorf("unable to write netplan VLAN dropin: %w", err)
 	}
 
-	reload3, err := n.writeNetworkdVLANDropin(nics)
+	reload3, err = n.writeNetworkdVLANDropin(nics)
 	if err != nil {
 		return fmt.Errorf("unable to write netplan networkd VLAN dropin: %w", err)
 	}
 
 	if reload1 || reload2 || reload3 {
-		if err := n.reloadConfigs(ctx); err != nil {
+		if err = n.reloadConfigs(ctx); err != nil {
 			return fmt.Errorf("error applying vlan interface configs: %w", err)
 		}
 	}
@@ -512,23 +518,23 @@ func (n *netplan) interfaceFromLink(link string) string {
 // and returns only the vlan interfaces to delete.
 func (n *netplan) findVlanDiff(expectedNics *Interfaces) (*Interfaces, error) {
 	keepInterfaces := make(map[string]string)
-	toRemove := &Interfaces{VlanInterfaces: make(map[int]VlanInterface)}
+	toRemove := Interfaces{VlanInterfaces: make(map[string]VlanInterface)}
 
 	existingVlanCfgs := netplanDropin{}
 	netplanVlanDropinFile := n.dropinFile(netplanVlanSuffix)
 	// There's no config file per interface, single netplan config file lists all the interfaces.
 	if !utils.FileExists(netplanVlanDropinFile, utils.TypeFile) {
 		logger.Infof("File %q does not exist, nothing to rollback", netplanVlanDropinFile)
-		return toRemove, nil
+		return nil, nil
 	}
 
 	if err := readYamlFile(netplanVlanDropinFile, &existingVlanCfgs); err != nil {
-		return toRemove, fmt.Errorf("unable to read %q trying rollback configs: %w", netplanVlanDropinFile, err)
+		return nil, fmt.Errorf("unable to read %q trying rollback configs: %w", netplanVlanDropinFile, err)
 	}
 
 	if len(existingVlanCfgs.Network.Vlans) == 0 {
 		logger.Debugf("No existing VLAN configs found at %q, skipping rollback", netplanVlanDropinFile)
-		return toRemove, nil
+		return nil, nil
 	}
 
 	for _, iface := range expectedNics.VlanInterfaces {
@@ -541,11 +547,18 @@ func (n *netplan) findVlanDiff(expectedNics *Interfaces) (*Interfaces, error) {
 	for vlanKey, vlan := range existingVlanCfgs.Network.Vlans {
 		_, ok := keepInterfaces[vlanKey]
 		if !ok {
-			toRemove.VlanInterfaces[vlan.ID] = VlanInterface{ParentInterfaceID: n.interfaceFromLink(vlan.Link)}
+			parentID := n.interfaceFromLink(vlan.Link)
+			vlanID := fmt.Sprintf("%s-%d", parentID, vlan.ID)
+			toRemove.VlanInterfaces[vlanID] = VlanInterface{
+				ParentInterfaceID: parentID,
+				VlanInterface: metadata.VlanInterface{
+					Vlan: vlan.ID,
+				},
+			}
 		}
 	}
 
-	return toRemove, nil
+	return &toRemove, nil
 }
 
 // rollbackVlanNics removes the [nics] and its config (netplan and networkd dropin both) on disk.
@@ -567,8 +580,8 @@ func (n *netplan) rollbackVlanNics(ctx context.Context, nics *Interfaces) (bool,
 		}
 	}
 
-	for id, iface := range nics.VlanInterfaces {
-		ifaceName := n.vlanInterfaceName(iface.ParentInterfaceID, id)
+	for _, iface := range nics.VlanInterfaces {
+		ifaceName := n.vlanInterfaceName(iface.ParentInterfaceID, iface.VlanInterface.Vlan)
 		key := n.ID(ifaceName)
 
 		deleteNics = append(deleteNics, key)
@@ -663,8 +676,6 @@ func (n *netplan) writeNetplanVLANDropin(nics *Interfaces) (bool, error) {
 func (n *netplan) writeNetworkdVLANDropin(nics *Interfaces) (bool, error) {
 	var reload bool
 
-	googleIpv6Interfaces := vlanInterfaceListsIpv6(nics.VlanInterfaces)
-
 	stat, err := os.Stat(n.networkdDropinDir)
 	if err != nil {
 		return false, fmt.Errorf("failed to stat systemd-networkd's drop-in root dir: %w", err)
@@ -678,7 +689,7 @@ func (n *netplan) writeNetworkdVLANDropin(nics *Interfaces) (bool, error) {
 		logger.Debugf("writing systemd-networkd drop-in config for VLAN ID: %d", iface.Vlan)
 
 		var dhcp = "ipv4"
-		if slices.Contains(googleIpv6Interfaces, iface.Vlan) {
+		if iface.DHCPv6Refresh != "" {
 			dhcp = "yes"
 		}
 

google_guest_agent/network/manager/netplan_linux_test.go

@@ -193,16 +193,16 @@ func TestSetupVlanInterface(t *testing.T) {
 				Mac: ifaces[1].HardwareAddr.String(),
 			},
 		},
-		VlanInterfaces: map[int]VlanInterface{
-			5: {
+		VlanInterfaces: map[string]VlanInterface{
+			"0-5": {
 				VlanInterface: metadata.VlanInterface{
 					Mac:  "mac-address",
 					Vlan: 5,
 					MTU:  1460,
 				},
 				ParentInterfaceID: ifaces[1].Name,
 			},
-			6: {
+			"0-6": {
 				VlanInterface: metadata.VlanInterface{
 					Mac:           "mac-address2",
 					Vlan:          6,
@@ -426,8 +426,8 @@ func TestPartialVlanRemoval(t *testing.T) {
 	}
 
 	nics := &Interfaces{
-		VlanInterfaces: map[int]VlanInterface{
-			5: {
+		VlanInterfaces: map[string]VlanInterface{
+			"ens4-5": {
 				VlanInterface: metadata.VlanInterface{
 					Mac:  "mac-address",
 					Vlan: 5,
@@ -439,8 +439,11 @@ func TestPartialVlanRemoval(t *testing.T) {
 	}
 
 	wantNics := &Interfaces{
-		VlanInterfaces: map[int]VlanInterface{
-			6: {
+		VlanInterfaces: map[string]VlanInterface{
+			"ens4-6": {
+				VlanInterface: metadata.VlanInterface{
+					Vlan: 6,
+				},
 				ParentInterfaceID: "ens4",
 			},
 		},
@@ -451,6 +454,10 @@ func TestPartialVlanRemoval(t *testing.T) {
 		t.Fatalf("netplan.findVlanDiff(%+v) failed unexpectedly with error: %v", nics, err)
 	}
 
+	if got == nil {
+		t.Fatalf("netplan.findVlanDiff(%+v) return nil, expected non nil", nics)
+	}
+
 	if diff := cmp.Diff(wantNics, got); diff != "" {
 		t.Errorf("netplan.findVlanDiff(%+v) returned diff on vlans to remove (-want,+got)\n%s", nics, diff)
 	}

google_guest_agent/network/manager/network_manager_linux_test.go

@@ -446,8 +446,8 @@ func TestVlanInterface(t *testing.T) {
 		EthernetInterfaces: []metadata.NetworkInterfaces{{
 			Mac: ifaces[1].HardwareAddr.String(),
 		}},
-		VlanInterfaces: map[int]VlanInterface{
-			22: {
+		VlanInterfaces: map[string]VlanInterface{
+			"0-22": {
 				VlanInterface: metadata.VlanInterface{
 					Mac:             "foobar",
 					ParentInterface: "/computeMetadata/v1/instance/network-interfaces/0/",

google_guest_agent/network/manager/systemd_networkd_linux_test.go

@@ -647,10 +647,11 @@ func TestSetupVlanInterfaceSuccess(t *testing.T) {
 				dhclientTestTearDown(t)
 			})
 
+			mapIdx := fmt.Sprintf("%s-%d", curr.parentID, curr.vlanInterface.Vlan)
 			nics := &Interfaces{
 				EthernetInterfaces: []metadata.NetworkInterfaces{curr.ethernetInterface},
-				VlanInterfaces: map[int]VlanInterface{
-					curr.vlanInterface.Vlan: {VlanInterface: curr.vlanInterface, ParentInterfaceID: curr.parentID},
+				VlanInterfaces: map[string]VlanInterface{
+					mapIdx: {VlanInterface: curr.vlanInterface, ParentInterfaceID: curr.parentID},
 				},
 			}