fix: tap interface attach to bridge (#479)

When requesting that a tap device is added to a microvm then we should
attach (i.e. set the master) to a bridge.

The bridge will be supplied via a new bridge-name flag. And the attach
should not be done for the mmds network interface.

The grpc api has been updated to allow the consumer to specify a
bridge-name that is different as part of the create microvm call.

Signed-off-by: Richard Case <[email protected]>

api/services/microvm/v1alpha1/microvms.swagger.json

@@ -404,6 +404,10 @@
         "address": {
           "$ref": "#/definitions/typesStaticAddress",
           "description": "Address is an optional static IP address to manually assign to this interface. \nIf not supplied then DHCP will be used."
+        },
+        "overrides": {
+          "$ref": "#/definitions/typesNetworkOverrides",
+          "description": "Overrides is optional overrides applicable for network configuration."
         }
       }
     },
@@ -425,6 +429,16 @@
         }
       }
     },
+    "typesNetworkOverrides": {
+      "type": "object",
+      "properties": {
+        "bridgeName": {
+          "type": "string",
+          "description": "BridgeName is the name of the Linux bridge to attach TAP devices to. This overrides\nany value set at the overall flintlock level."
+        }
+      },
+      "description": "NetworkOverrides represents override values for a network interface."
+    },
     "typesStaticAddress": {
       "type": "object",
       "properties": {

api/types/microvm.proto

@@ -110,6 +110,9 @@ message NetworkInterface {
   // Address is an optional static IP address to manually assign to this interface. 
   // If not supplied then DHCP will be used.
   optional StaticAddress address = 5;
+  // Overrides is optional overrides applicable for network configuration.
+  optional NetworkOverrides overrides = 6;
+
 }
 
 // StaticAddress represents a static IPv4 or IPv6 address.
@@ -201,3 +204,10 @@ message NetworkInterfaceStatus {
   // MACAddress is the MAC address of the host interface.
   string mac_address = 3;
 }
+
+// NetworkOverrides represents override values for a network interface.
+message NetworkOverrides {
+  // BridgeName is the name of the Linux bridge to attach TAP devices to. This overrides
+  // any value set at the overall flintlock level.
+  optional string bridge_name = 1;
+}

buf.lock

@@ -4,7 +4,7 @@ deps:
   - remote: buf.build
     owner: googleapis
     repository: googleapis
-    commit: 86d30bdfc34044fb9339e1bd9673839b
+    commit: 2646de1347094058879360e68cb2ccc6
   - remote: buf.build
     owner: grpc-ecosystem
     repository: grpc-gateway

core/errors/errors.go

@@ -6,22 +6,23 @@ import (
 )
 
 var (
-	ErrSpecRequired            = errors.New("microvm spec is required")
-	ErrVMIDRequired            = errors.New("id for microvm is required")
-	ErrNameRequired            = errors.New("name is required")
-	ErrUIDRequired             = errors.New("uid is required")
-	ErrNamespaceRequired       = errors.New("namespace is required")
-	ErrKernelImageRequired     = errors.New("kernel image is required")
-	ErrVolumeRequired          = errors.New("no volumes specified, at least 1 volume is required")
-	ErrRootVolumeRequired      = errors.New("a root volume is required")
-	ErrNoMount                 = errors.New("no image mount point")
-	ErrNoVolumeMount           = errors.New("no volume mount point")
-	ErrParentIfaceRequired     = errors.New("a parent network device name is required")
-	ErrGuestDeviceNameRequired = errors.New("a guest device name is required")
-	ErrUnsupportedIfaceType    = errors.New("unsupported network interface type")
-	ErrIfaceNotFound           = errors.New("network interface not found")
-	ErrMissingStatusInfo       = errors.New("status is not defined")
-	ErrUnableToBoot            = errors.New("microvm is unable to boot")
+	ErrSpecRequired                       = errors.New("microvm spec is required")
+	ErrVMIDRequired                       = errors.New("id for microvm is required")
+	ErrNameRequired                       = errors.New("name is required")
+	ErrUIDRequired                        = errors.New("uid is required")
+	ErrNamespaceRequired                  = errors.New("namespace is required")
+	ErrKernelImageRequired                = errors.New("kernel image is required")
+	ErrVolumeRequired                     = errors.New("no volumes specified, at least 1 volume is required")
+	ErrRootVolumeRequired                 = errors.New("a root volume is required")
+	ErrNoMount                            = errors.New("no image mount point")
+	ErrNoVolumeMount                      = errors.New("no volume mount point")
+	ErrParentIfaceRequiredForMacvtap      = errors.New("a parent network device name is required for macvtap interfaces")
+	ErrParentIfaceRequiredForAttachingTap = errors.New("a parent network device name is required for attaching a TAP interface")
+	ErrGuestDeviceNameRequired            = errors.New("a guest device name is required")
+	ErrUnsupportedIfaceType               = errors.New("unsupported network interface type")
+	ErrIfaceNotFound                      = errors.New("network interface not found")
+	ErrMissingStatusInfo                  = errors.New("status is not defined")
+	ErrUnableToBoot                       = errors.New("microvm is unable to boot")
 )
 
 // TopicNotFoundError is an error created when a topic with a specific name isn't found.

core/models/network.go

@@ -23,7 +23,9 @@ type NetworkInterface struct {
 	Type IfaceType `json:"type" validate:"oneof=tap macvtap unsupported"`
 	// StaticAddress is an optional static IP address to assign to this interface.
 	// If not supplied then DHCP will be used.
-	StaticAddress *StaticAddress `json:"staticAddress,omitempty"`
+	StaticAddress *StaticAddress `json:"staticAddrss,omitempty"`
+	// BridgeName is the name of the Linux bridge to attach the TAP device to.
+	BridgeName string `json:"branch_name,omitempty"`
 }
 
 // StaticAddress specifies a static IP address configuration.

core/ports/services.go

@@ -124,6 +124,10 @@ type IfaceCreateInput struct {
 	// MAC allows the specifying of a specific MAC address to use for the interface. If
 	// not supplied a autogenerated MAC address will be used.
 	MAC string
+	// Attach indicates if this device should be attached to the parent bridge. Only applicable to TAP devices.
+	Attach bool
+	// BridgeName is the name of the bridge to attach to. Only if this is a tap device and attach is true.
+	BridgeName string
 }
 
 type IfaceDetails struct {

core/steps/network/interface_create.go

@@ -109,6 +109,12 @@ func (s *createInterface) Do(ctx context.Context) ([]planner.Procedure, error) {
 		DeviceName: deviceName,
 		Type:       s.iface.Type,
 		MAC:        s.iface.GuestMAC,
+		Attach:     true,
+		BridgeName: s.iface.BridgeName,
+	}
+
+	if s.iface.Type == models.IfaceTypeTap && s.iface.AllowMetadataRequests {
+		input.Attach = false
 	}
 
 	output, err := s.svc.IfaceCreate(ctx, *input)

core/steps/network/interface_create_test.go

@@ -211,6 +211,7 @@ func TestNewNetworkInterface_missingInterface(t *testing.T) {
 		IfaceCreate(gomock.Eq(ctx), gomock.Eq(ports.IfaceCreateInput{
 			DeviceName: expectedTapDeviceName,
 			MAC:        defaultMACAddress,
+			Attach:     true,
 		})).
 		Return(&ports.IfaceDetails{
 			DeviceName: expectedTapDeviceName,
@@ -239,7 +240,7 @@ func TestNewNetworkInterface_svcError(t *testing.T) {
 
 	svc.EXPECT().
 		IfaceExists(gomock.Eq(ctx), gomock.Eq(expectedTapDeviceName)).
-		Return(false, errors.ErrParentIfaceRequired).
+		Return(false, errors.ErrParentIfaceRequiredForAttachingTap).
 		Times(2)
 
 	step := network.NewNetworkInterface(vmid, iface, status, svc)
@@ -249,7 +250,7 @@ func TestNewNetworkInterface_svcError(t *testing.T) {
 	g.Expect(shouldDo).To(g.BeFalse())
 
 	_, err = step.Do(ctx)
-	g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequired))
+	g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequiredForAttachingTap))
 
 	verifyErr := step.Verify(ctx)
 	g.Expect(verifyErr).To(g.BeNil())
@@ -322,11 +323,11 @@ func TestNewNetworkInterface_createError(t *testing.T) {
 
 	svc.EXPECT().
 		IfaceCreate(gomock.Eq(ctx), &ifaceCreateInputMatcher{}).
-		Return(nil, errors.ErrParentIfaceRequired).
+		Return(nil, errors.ErrParentIfaceRequiredForAttachingTap).
 		Times(1)
 
 	_, err = step.Do(ctx)
-	g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequired))
+	g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequiredForAttachingTap))
 
 	verifyErr := step.Verify(ctx)
 	g.Expect(verifyErr).To(g.BeNil())

core/steps/network/interface_delete_test.go

@@ -169,7 +169,7 @@ func TestDeleteNetworkInterface_IfaceExistsError(t *testing.T) {
 
 	svc.EXPECT().
 		IfaceExists(gomock.Eq(ctx), gomock.Eq(expectedTapDeviceName)).
-		Return(false, errors.ErrParentIfaceRequired).
+		Return(false, errors.ErrParentIfaceRequiredForAttachingTap).
 		Times(2)
 
 	step := network.DeleteNetworkInterface(vmid, iface, svc)
@@ -179,7 +179,7 @@ func TestDeleteNetworkInterface_IfaceExistsError(t *testing.T) {
 	g.Expect(shouldDo).To(g.BeFalse())
 
 	_, err = step.Do(ctx)
-	g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequired))
+	g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequiredForAttachingTap))
 
 	verifyErr := step.Verify(ctx)
 	g.Expect(verifyErr).To(g.BeNil())

go.mod

@@ -44,7 +44,6 @@ require (
 	github.com/gorilla/mux v1.8.0
 	github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
 	github.com/urfave/cli/v2 v2.10.3
-	github.com/weaveworks-liquidmetal/flintlock/api v0.0.0-20211217111250-5f8d70c4a581
 	github.com/weaveworks-liquidmetal/flintlock/client v0.0.0-00010101000000-000000000000
 	github.com/yitsushi/file-tailor v1.0.0
 	sigs.k8s.io/yaml v1.3.0
@@ -84,6 +83,7 @@ require (
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.2 // indirect
+	github.com/google/subcommands v1.0.1 // indirect
 	github.com/hashicorp/errwrap v1.0.0 // indirect
 	github.com/hashicorp/go-multierror v1.1.0 // indirect
 	github.com/hashicorp/hcl v1.0.0 // indirect
@@ -102,6 +102,7 @@ require (
 	github.com/opencontainers/selinux v1.8.2 // indirect
 	github.com/pelletier/go-toml/v2 v2.0.1 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_model v0.2.0 // indirect
 	github.com/prometheus/common v0.32.1 // indirect
 	github.com/prometheus/procfs v0.7.3 // indirect
@@ -110,13 +111,17 @@ require (
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect
 	github.com/subosito/gotenv v1.3.0 // indirect
 	github.com/vishvananda/netns v0.0.0-20200728191858-db3c7e526aae // indirect
+	github.com/weaveworks-liquidmetal/flintlock/api v0.0.0-20220628141946-264f4544f49f // indirect
 	github.com/xrash/smetrics v0.0.0-20201216005158-039620a65673 // indirect
 	go.mongodb.org/mongo-driver v1.7.5 // indirect
 	go.opencensus.io v0.23.0 // indirect
 	golang.org/x/crypto v0.0.0-20220411220226-7b82a4e95df4 // indirect
+	golang.org/x/mod v0.4.2 // indirect
 	golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect
 	golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect
 	golang.org/x/text v0.3.7 // indirect
+	golang.org/x/tools v0.1.1 // indirect
+	golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df // indirect
 	google.golang.org/genproto v0.0.0-20220519153652-3a47de7e79bd // indirect
 	gopkg.in/ini.v1 v1.66.4 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect

go.sum

@@ -512,6 +512,7 @@ github.com/google/pprof v0.0.0-20201023163331-3e6fc7fc9c4c/go.mod h1:kpwsk12EmLe
 github.com/google/pprof v0.0.0-20201203190320-1bf35d6f28c2/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
 github.com/google/pprof v0.0.0-20201218002935-b9804c9f04c2/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
 github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
+github.com/google/subcommands v1.0.1 h1:/eqq+otEXm5vhfBrbREPCSVQbvofip6kIz+mX5TUH7k=
 github.com/google/subcommands v1.0.1/go.mod h1:ZjhPrFU+Olkh9WazFPsl27BQ4UPiG37m3yTrtFlrHVk=
 github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
@@ -974,6 +975,7 @@ golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.4.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
+golang.org/x/mod v0.4.2 h1:Gz96sIWK3OalVv/I/qNygP42zyoKp3xptRVCWRFEBvo=
 golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
@@ -1216,11 +1218,14 @@ golang.org/x/tools v0.0.0-20210105154028-b0ab187a4818/go.mod h1:emZCQorbCU4vsT4f
 golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.0.0-20210108195828-e2f9c7f1fc8e/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0=
+golang.org/x/tools v0.1.1 h1:wGiQel/hW0NnEkJUk8lbzkX2gFJU6PFxf1v5OlCfuOs=
 golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df h1:5Pf6pFKu98ODmgnpvkJ3kFUOQGGLIzLIkbzUHp47618=
+golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df/go.mod h1:K8+ghG5WaK9qNqU5K3HdILfMLy1f3aNYFI/wnl100a8=
 google.golang.org/api v0.0.0-20160322025152-9bf6e6e569ff/go.mod h1:4mhQ8q/RsB7i+udVvVy5NUi08OU8ZlA0gRVgrF7VFY0=
 google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE=
 google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M=

infrastructure/grpc/convert.go

@@ -108,6 +108,9 @@ func convertNetworkInterfaceToModel(netInt *types.NetworkInterface) *models.Netw
 			converted.StaticAddress.Nameservers = append(converted.StaticAddress.Nameservers, nameserver)
 		}
 	}
+	if netInt.Overrides != nil {
+		converted.BridgeName = *netInt.Overrides.BridgeName
+	}
 
 	switch netInt.Type {
 	case types.NetworkInterface_MACVTAP:

infrastructure/network/network_service.go

@@ -17,16 +17,19 @@ import (
 
 type Config struct {
 	ParentDeviceName string
+	BridgeName       string
 }
 
 func New(cfg *Config) ports.NetworkService {
 	return &networkService{
 		parentDeviceName: cfg.ParentDeviceName,
+		bridgeName:       cfg.BridgeName,
 	}
 }
 
 type networkService struct {
 	parentDeviceName string
+	bridgeName       string
 }
 
 // IfaceCreate will create the network interface.
@@ -47,14 +50,20 @@ func (n *networkService) IfaceCreate(ctx context.Context, input ports.IfaceCreat
 		err        error
 	)
 
-	if input.Type == models.IfaceTypeMacvtap {
-		if n.parentDeviceName == "" {
-			return nil, errors.ErrParentIfaceRequired
+	parentDeviceName := n.getParentIfaceName(input)
+	if parentDeviceName == "" {
+		if input.Type == models.IfaceTypeMacvtap {
+			return nil, errors.ErrParentIfaceRequiredForMacvtap
+		}
+		if input.Type == models.IfaceTypeTap && input.Attach {
+			return nil, errors.ErrParentIfaceRequiredForAttachingTap
 		}
+	}
 
-		parentLink, err = netlink.LinkByName(n.parentDeviceName)
+	if parentDeviceName != "" {
+		parentLink, err = netlink.LinkByName(parentDeviceName)
 		if err != nil {
-			return nil, fmt.Errorf("failed to lookup parent network interface %q: %w", n.parentDeviceName, err)
+			return nil, fmt.Errorf("failed to lookup parent network interface %q: %w", parentDeviceName, err)
 		}
 	}
 
@@ -113,6 +122,14 @@ func (n *networkService) IfaceCreate(ctx context.Context, input ports.IfaceCreat
 
 	logger.Debugf("created interface with mac %s", macIf.Attrs().HardwareAddr.String())
 
+	if input.Type == models.IfaceTypeTap && input.Attach {
+		if err := netlink.LinkSetMaster(macIf, parentLink); err != nil {
+			return nil, fmt.Errorf("setting master for %s to %s: %w", macIf.Attrs().Name, parentLink.Attrs().Name, err)
+		}
+
+		logger.Debugf("added interface %s to bridge %s", macIf.Attrs().Name, parentLink.Attrs().Name)
+	}
+
 	return &ports.IfaceDetails{
 		DeviceName: input.DeviceName,
 		Type:       input.Type,
@@ -209,3 +226,15 @@ func (n *networkService) getIface(name string) (bool, netlink.Link, error) {
 
 	return true, link, nil
 }
+
+func (n *networkService) getParentIfaceName(input ports.IfaceCreateInput) string {
+	if input.Type == models.IfaceTypeMacvtap {
+		return n.parentDeviceName
+	}
+
+	if input.BridgeName != "" {
+		return input.BridgeName
+	}
+
+	return n.bridgeName
+}

internal/command/flags/flags.go

@@ -12,6 +12,7 @@ const (
 	grpcEndpointFlag      = "grpc-endpoint"
 	httpEndpointFlag      = "http-endpoint"
 	parentIfaceFlag       = "parent-iface"
+	bridgeNameFlag        = "bridge-name"
 	disableReconcileFlag  = "disable-reconcile"
 	disableAPIFlag        = "disable-api"
 	firecrackerBinFlag    = "firecracker-bin"
@@ -107,9 +108,11 @@ func AddNetworkFlagsToCommand(cmd *cobra.Command, cfg *config.Config) error {
 		"",
 		"The parent iface for the network interfaces. Note it could also be a bond")
 
-	if err := cmd.MarkFlagRequired(parentIfaceFlag); err != nil {
-		return fmt.Errorf("setting %s as required: %w", parentIfaceFlag, err)
-	}
+	cmd.Flags().StringVar(
+		&cfg.BridgeName,
+		bridgeNameFlag,
+		"",
+		"The name of the Linux bridge to attach tap devices to by default")
 
 	return nil
 }

internal/command/run/run.go

@@ -2,6 +2,7 @@ package run
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
 	"net/http"
@@ -42,6 +43,10 @@ func NewCommand(cfg *config.Config) (*cobra.Command, error) {
 				version.CommitHash,
 			)
 
+			if cfg.ParentIface == "" && cfg.BridgeName == "" {
+				return errors.New("You must supply at least one of parent interface, bridge name")
+			}
+
 			return nil
 		},
 		RunE: func(c *cobra.Command, _ []string) error {