From 2fbcc6d9c460145fe1979779a62d09951efee9ff Mon Sep 17 00:00:00 2001 From: Swagat Bora Date: Wed, 9 Oct 2024 17:32:46 +0000 Subject: [PATCH 1/6] feat: implementation of enable_icc option + code refactor Signed-off-by: Swagat Bora --- go.mod | 3 +- go.sum | 2 + internal/service/network/bridge_driver.go | 375 ++++++++++++++++++ .../service/network/bridge_driver_test.go | 123 ++++++ internal/service/network/create.go | 177 ++------- internal/service/network/create_test.go | 145 ++++++- internal/service/network/remove.go | 44 ++ mocks/mocks_backend/nerdctlimagesvc.go | 2 +- mocks/mocks_container/containersvc.go | 2 +- mocks/mocks_network/bridge_driver.go | 150 +++++++ mocks/mocks_network/iptables.go | 120 ++++++ 11 files changed, 998 insertions(+), 145 deletions(-) create mode 100644 internal/service/network/bridge_driver.go create mode 100644 internal/service/network/bridge_driver_test.go create mode 100644 mocks/mocks_network/bridge_driver.go create mode 100644 mocks/mocks_network/iptables.go diff --git a/go.mod b/go.mod index b8844b08..d3f192b4 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/containerd/platforms v0.2.1 github.com/containerd/typeurl/v2 v2.2.0 github.com/containernetworking/cni v1.2.2 + github.com/coreos/go-iptables v0.7.0 github.com/coreos/go-systemd/v22 v22.5.0 github.com/distribution/reference v0.6.0 github.com/docker/cli v26.0.0+incompatible @@ -65,7 +66,6 @@ require ( github.com/containerd/typeurl v1.0.3-0.20220422153119-7f6e6d160d67 // indirect github.com/containernetworking/plugins v1.4.1 // indirect github.com/containers/ocicrypt v1.1.10 // indirect - github.com/coreos/go-iptables v0.7.0 // indirect github.com/cyphar/filepath-securejoin v0.2.4 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/djherbis/times v1.6.0 // indirect @@ -142,6 +142,7 @@ require ( go.opentelemetry.io/otel/trace v1.30.0 // indirect golang.org/x/crypto v0.27.0 // indirect golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect + golang.org/x/mod v0.20.0 // indirect golang.org/x/sync v0.8.0 // indirect golang.org/x/term v0.24.0 // indirect golang.org/x/text v0.18.0 // indirect diff --git a/go.sum b/go.sum index fc27189a..c4530bdf 100644 --- a/go.sum +++ b/go.sum @@ -361,6 +361,8 @@ golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.20.0 h1:utOm6MM3R3dnawAiJgn0y+xvuYRsm1RKM/4giyfDgV0= +golang.org/x/mod v0.20.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= 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= golang.org/x/net v0.0.0-20190213061140-3a22650c66bd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= diff --git a/internal/service/network/bridge_driver.go b/internal/service/network/bridge_driver.go new file mode 100644 index 00000000..f6ae1dec --- /dev/null +++ b/internal/service/network/bridge_driver.go @@ -0,0 +1,375 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package network + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strconv" + + "github.com/containerd/nerdctl/pkg/lockutil" + "github.com/containerd/nerdctl/pkg/netutil" + "github.com/coreos/go-iptables/iptables" + "github.com/runfinch/finch-daemon/api/types" + "github.com/runfinch/finch-daemon/internal/backend" + "github.com/runfinch/finch-daemon/pkg/flog" +) + +const ( + FinchICCLabel = "finch.network.bridge.enable_icc" + + BridgeICCOption = "com.docker.network.bridge.enable_icc" + BridgeHostBindingIpv4Option = "com.docker.network.bridge.host_binding_ipv4" + BridgeNameOption = "com.docker.network.bridge.name" +) + +//go:generate mockgen --destination=../../../mocks/mocks_network/bridge_driver.go -package=mocks_network -mock_names BridgeDriverOperations=BridgeDriver . BridgeDriverOperations +type BridgeDriverOperations interface { + HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) + HandlePostCreate(net *netutil.NetworkConfig) (string, error) + SetBridgeName(net *netutil.NetworkConfig, bridge string) error + GetBridgeName(net *netutil.NetworkConfig) (string, error) + GetNetworkByBridgeName(bridge string) (*netutil.NetworkConfig, error) + DisableICC(bridgeIface string, insert bool) error + SetICCDisabled() + ICCDisabled() bool +} + +// IPTablesWrapper is an interface that wraps the methods of iptables.IPTables +// to help with mock +// +//go:generate mockgen --destination=../../../mocks/mocks_network/iptables.go -package=mocks_network . IPTablesWrapper +type IPTablesWrapper interface { + ChainExists(table, chain string) (bool, error) + NewChain(table, chain string) error + Insert(table, chain string, pos int, rulespec ...string) error + Append(table, chain string, rulespec ...string) error + DeleteIfExists(table, chain string, rulespec ...string) error +} + +// IPTablesWrapperImpl implements IPTablesWrapper +// that delegates to an actual iptables.IPTables instance. +type IPTablesWrapperImpl struct { + ipt *iptables.IPTables +} + +func NewIPTablesWrapper() IPTablesWrapper { + iptables, _ := iptables.NewWithProtocol(iptables.ProtocolIPv4) + return &IPTablesWrapperImpl{ipt: iptables} +} + +func (i *IPTablesWrapperImpl) ChainExists(table, chain string) (bool, error) { + return i.ipt.ChainExists(table, chain) +} + +func (i *IPTablesWrapperImpl) NewChain(table, chain string) error { + return i.ipt.NewChain(table, chain) +} + +func (i *IPTablesWrapperImpl) Insert(table, chain string, pos int, rulespec ...string) error { + return i.ipt.Insert(table, chain, pos, rulespec...) +} + +func (i *IPTablesWrapperImpl) Append(table, chain string, rulespec ...string) error { + return i.ipt.Append(table, chain, rulespec...) +} + +func (i *IPTablesWrapperImpl) DeleteIfExists(table, chain string, rulespec ...string) error { + return i.ipt.DeleteIfExists(table, chain, rulespec...) +} + +type bridgeDriver struct { + bridgeName string + disableICC bool + netClient backend.NerdctlNetworkSvc + logger flog.Logger + ipt IPTablesWrapper +} + +var _ BridgeDriverOperations = (*bridgeDriver)(nil) + +var NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logger) BridgeDriverOperations { + return &bridgeDriver{ + netClient: netClient, + logger: logger, + ipt: NewIPTablesWrapper(), + } +} + +// handleBridgeDriverOptions filters unsupported options for the bridge driver. +func (bd *bridgeDriver) HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) { + // enable_icc, host_binding_ipv4, and bridge name network options are not supported by nerdctl. + // So we must filter out any unsupported options which would prevent the network from being created and accept the defaults. + filterUnsupportedOptions := func(original map[string]string) map[string]string { + opts := map[string]string{} + for k, v := range original { + switch k { + case BridgeHostBindingIpv4Option: + if v != "0.0.0.0" { + bd.logger.Warnf("network option com.docker.network.bridge.host_binding_ipv4 is set to %s, but it must be 0.0.0.0", v) + } + case BridgeICCOption: + iccOption, err := strconv.ParseBool(v) //t + if err != nil { + bd.logger.Warnf("invalid value for com.docker.network.bridge.enable_icc: %s", v) + } + if !iccOption { + bd.SetICCDisabled() + } + + case BridgeNameOption: + bd.bridgeName = v + default: + opts[k] = v + } + } + return opts + } + + options.Options = filterUnsupportedOptions(request.Options) + + if bd.ICCDisabled() { + // Append a label when enable_icc is set to false. This is used for clean up during network removal. + options.Labels = append(options.Labels, FinchICCLabel+"=false") + } + + // Return the modified options + return options, nil +} + +func (bd *bridgeDriver) HandlePostCreate(net *netutil.NetworkConfig) (string, error) { + // Handle bridge driver post create actions + var warning string + var err error + bridgeName := bd.bridgeName + if bridgeName != "" { + // Since nerdctl currently does not support custom bridge names, + // we explicitly override bridge name in the conflist file for the network that was just created + if err = bd.SetBridgeName(net, bridgeName); err != nil { + warning = fmt.Sprintf("Failed to set network bridge name %s: %s", bridgeName, err) + } + } + + if bd.ICCDisabled() { + // Handle "enable_icc=false" option if set (bd.disableICC is true) + // By default, CNI allows connectivity between containers attached to the same bridge. + // If "com.docker.network.bridge.enable_icc" option is explicitly set to false, + // we disable inter-container connectivity by applying iptable rules + // If "com.docker.network.bridge.enable_icc=true" is set, it is considered a noop + if bridgeName == "" { + bridgeName, err = bd.GetBridgeName(net) + if err != nil { + return "", fmt.Errorf("failed to get bridge name to enable inter-container connectivity: %w ", err) + } + } + err = bd.DisableICC(bridgeName, true) + if err != nil { + return "", fmt.Errorf("failed to disable inter-container connectivity: %w", err) + } + } + + return warning, nil +} + +// setBridgeName will override the bridge name in an existing CNI config file for a network. +func (bd *bridgeDriver) SetBridgeName(net *netutil.NetworkConfig, bridge string) error { + return lockutil.WithDirLock(bd.netClient.NetconfPath(), func() error { + // first, make sure that the bridge name is not used by any of the existing bridge networks + bridgeNet, err := bd.GetNetworkByBridgeName(bridge) + if err != nil { + return err + } + if bridgeNet != nil { + return fmt.Errorf("bridge name %s already in use by network %s", bridge, bridgeNet.Name) + } + + // load the CNI config file and set bridge name + configFilename := bd.getConfigPathForNetworkName(net.Name) + configFile, err := os.Open(configFilename) + if err != nil { + return err + } + defer configFile.Close() + var netJSON interface{} + if err = json.NewDecoder(configFile).Decode(&netJSON); err != nil { + return err + } + netMap, ok := netJSON.(map[string]interface{}) + if !ok { + return fmt.Errorf("network config file %s is not a valid map", configFilename) + } + plugins, ok := netMap["plugins"] + if !ok { + return fmt.Errorf("could not find plugins in network config file %s", configFilename) + } + pluginsMap, ok := plugins.([]interface{}) + if !ok { + return fmt.Errorf("could not parse plugins in network config file %s", configFilename) + } + for _, plugin := range pluginsMap { + pluginMap, ok := plugin.(map[string]interface{}) + if !ok { + continue + } + if pluginMap["type"] == "bridge" { + pluginMap["bridge"] = bridge + data, err := json.MarshalIndent(netJSON, "", " ") + if err != nil { + return err + } + return os.WriteFile(configFilename, data, 0o644) + } + } + return fmt.Errorf("bridge plugin not found in network config file %s", configFilename) + }) +} + +func (bd *bridgeDriver) GetBridgeName(net *netutil.NetworkConfig) (string, error) { + var bridgeName string + err := lockutil.WithDirLock(bd.netClient.NetconfPath(), func() error { + configFilename := bd.getConfigPathForNetworkName(net.Name) + configFile, err := os.Open(configFilename) + if err != nil { + return err + } + defer configFile.Close() + + var netJSON interface{} + if err = json.NewDecoder(configFile).Decode(&netJSON); err != nil { + return err + } + + netMap, ok := netJSON.(map[string]interface{}) + if !ok { + return fmt.Errorf("network config file %s is not a valid map", configFilename) + } + + plugins, ok := netMap["plugins"] + if !ok { + return fmt.Errorf("could not find plugins in network config file %s", configFilename) + } + + pluginsMap, ok := plugins.([]interface{}) + if !ok { + return fmt.Errorf("could not parse plugins in network config file %s", configFilename) + } + + for _, plugin := range pluginsMap { + pluginMap, ok := plugin.(map[string]interface{}) + if !ok { + continue + } + if pluginMap["type"] == "bridge" { + bridge, ok := pluginMap["bridge"].(string) + if !ok { + return fmt.Errorf("bridge name in config file %s is not a string", configFilename) + } + bridgeName = bridge + return nil + } + } + + return fmt.Errorf("bridge plugin not found in network config file %s", configFilename) + }) + + if err != nil { + return "", err + } + + return bridgeName, nil +} + +type bridgePlugin struct { + Type string `json:"type"` + Bridge string `json:"bridge"` +} + +func (bd *bridgeDriver) GetNetworkByBridgeName(bridge string) (*netutil.NetworkConfig, error) { + networks, err := bd.netClient.FilterNetworks(func(*netutil.NetworkConfig) bool { + return true + }) + if err != nil { + return nil, err + } + for _, network := range networks { + for _, plugin := range network.Plugins { + if plugin.Network.Type != "bridge" { + continue + } + var bridgeJSON bridgePlugin + if err = json.Unmarshal(plugin.Bytes, &bridgeJSON); err != nil { + continue + } + if bridgeJSON.Bridge == bridge { + return network, nil + } + } + } + return nil, nil +} + +func (bd *bridgeDriver) SetICCDisabled() { + bd.disableICC = true +} + +func (bd *bridgeDriver) ICCDisabled() bool { + return bd.disableICC +} + +func (bd *bridgeDriver) DisableICC(bridgeIface string, insert bool) error { + filterTable := "filter" + isolateChain := "FINCH-ISOLATE-CHAIN" + + if bd.ipt == nil { + return fmt.Errorf("iptables is not initialized") + } + + // Check if the FINCH-ISOLATE-CHAIN already exists + exists, err := bd.ipt.ChainExists(filterTable, isolateChain) + if err != nil { + return fmt.Errorf("failed to check if %s chain exists: %v", isolateChain, err) + } + + if !exists { + // Create and setup the FINCH-ISOLATE-CHAIN chain if it doesn't exist + err = bd.ipt.NewChain(filterTable, isolateChain) + if err != nil { + return fmt.Errorf("failed to create %s chain: %v", isolateChain, err) + } + // Add a rule to the FORWARD chain that jumps to the FINCH-ISOLATE-CHAIN for all packets + jumpRule := []string{"-j", isolateChain} + err = bd.ipt.Insert(filterTable, "FORWARD", 1, jumpRule...) + if err != nil { + return fmt.Errorf("failed to add %s jump rule to FORWARD chain: %v", isolateChain, err) + } + // In the FINCH-ISOLATE-CHAIN, add a rule to return to the FORWARD chain if no match + returnRule := []string{"-j", "RETURN"} + err = bd.ipt.Append(filterTable, isolateChain, returnRule...) + if err != nil { + return fmt.Errorf("failed to add RETURN rule to DOCKER-ISOLATE chain: %v", err) + } + } + + // In the FINCH-ISOLATE-CHAIN, add or remove the DROP rule for packets from and to the same bridge + dropRule := []string{"-i", bridgeIface, "-o", bridgeIface, "-j", "DROP"} + if insert { + err = bd.ipt.Insert(filterTable, isolateChain, 1, dropRule...) + if err != nil { + return fmt.Errorf("failed to add DROP rule to %s chain: %v", isolateChain, err) + } + } else { + err = bd.ipt.DeleteIfExists(filterTable, isolateChain, dropRule...) + if err != nil { + return fmt.Errorf("failed to remove DROP rule from %s chain: %v", isolateChain, err) + } + } + return nil +} + +// From https://github.com/containerd/nerdctl/blob/v1.5.0/pkg/netutil/netutil.go#L186-L188 +func (bd *bridgeDriver) getConfigPathForNetworkName(netName string) string { + return filepath.Join(bd.netClient.NetconfPath(), "nerdctl-"+netName+".conflist") +} diff --git a/internal/service/network/bridge_driver_test.go b/internal/service/network/bridge_driver_test.go new file mode 100644 index 00000000..93b3e3a0 --- /dev/null +++ b/internal/service/network/bridge_driver_test.go @@ -0,0 +1,123 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package network + +import ( + "fmt" + + "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/runfinch/finch-daemon/mocks/mocks_network" +) + +var _ = Describe("bridgeDriver DisableICC", func() { + var ( + mockController *gomock.Controller + mockIpt *mocks_network.MockIPTablesWrapper + driver *bridgeDriver + bridgeIface string + ) + + BeforeEach(func() { + mockController = gomock.NewController(GinkgoT()) + mockIpt = mocks_network.NewMockIPTablesWrapper(mockController) + driver = &bridgeDriver{ipt: mockIpt} + bridgeIface = "br-mock" + }) + + Context("FINCH-ISOLATE-CHAIN chain does not exist", func() { + BeforeEach(func() { + // FINCH-ISOLATE-CHAIN does not exist initially + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, nil) + + // NewChain method should be called + mockIpt.EXPECT().NewChain("filter", "FINCH-ISOLATE-CHAIN").Return(nil) + + // Expect a rule to be added to FORWARD chain that jumps to FINCH-ISOLATE-CHAIN + mockIpt.EXPECT().Insert("filter", "FORWARD", 1, "-j", "FINCH-ISOLATE-CHAIN").Return(nil) + // Expect a RETURN rule to be appended in FINCH-ISOLATE-CHAIN + mockIpt.EXPECT().Append("filter", "FINCH-ISOLATE-CHAIN", "-j", "RETURN").Return(nil) + + // Expect the second Insert call to FINCH-ISOLATE-CHAIN for the DROP rule + mockIpt.EXPECT().Insert("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + }) + + It("should create and set up the FINCH-ISOLATE-CHAIN", func() { + err := driver.DisableICC(bridgeIface, true) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + + Context("FINCH-ISOLATE-CHAIN exists", func() { + BeforeEach(func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil) + }) + + When("insert set to true", func() { + BeforeEach(func() { + // Expect the DROP rule to be inserted for packets from and to the same bridge + mockIpt.EXPECT().Insert("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + }) + + It("should add the DROP rule to the FINCH-ISOLATE-CHAIN", func() { + err := driver.DisableICC(bridgeIface, true) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + + When("insert set to false", func() { + BeforeEach(func() { + // Expect the DROP rule to be removed for packets from and to the same bridge + mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + }) + + It("should remove the DROP rule from the FINCH-ISOLATE-CHAIN", func() { + err := driver.DisableICC(bridgeIface, false) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + }) + + Context("when iptables returns an error", func() { + It("should return an error if ChainExists fails", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, fmt.Errorf("iptables error")) + + err := driver.DisableICC(bridgeIface, true) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to check if FINCH-ISOLATE-CHAIN chain exists")) + }) + + It("should return an error if NewChain fails", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, nil) + + mockIpt.EXPECT().NewChain("filter", "FINCH-ISOLATE-CHAIN").Return(fmt.Errorf("iptables error")) + + err := driver.DisableICC(bridgeIface, true) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to create FINCH-ISOLATE-CHAIN chain")) + }) + + It("should return an error if Insert fails while adding DROP rule", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil) + + mockIpt.EXPECT().Insert("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(fmt.Errorf("iptables error")) + + err := driver.DisableICC(bridgeIface, true) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to add DROP rule")) + }) + + It("should return an error if Delete fails while removing DROP rule", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil) + + // Simulate an error while removing the DROP rule + mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(fmt.Errorf("iptables error")) + + err := driver.DisableICC(bridgeIface, false) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to remove DROP rule")) + }) + }) +}) diff --git a/internal/service/network/create.go b/internal/service/network/create.go index 031718d3..1933acfe 100644 --- a/internal/service/network/create.go +++ b/internal/service/network/create.go @@ -5,13 +5,9 @@ package network import ( "context" - "encoding/json" "fmt" - "os" - "path/filepath" "strings" - "github.com/containerd/nerdctl/pkg/lockutil" "github.com/containerd/nerdctl/pkg/netutil" "github.com/runfinch/finch-daemon/api/types" @@ -21,49 +17,28 @@ import ( // Create implements the logic to turn a network create request to the back-end nerdctl create network calls. func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest) (types.NetworkCreateResponse, error) { - // enable_ip_masquerade, host_binding_ipv4, and bridge name network options are not supported by nerdctl. - // So we must filter out any unsupported options which would prevent the network from being created and accept the defaults. - bridge := "" - filterUnsupportedOptions := func(original map[string]string) map[string]string { - options := map[string]string{} - for k, v := range original { - switch k { - case "com.docker.network.bridge.enable_ip_masquerade": - // must be true - if v != "true" { - s.logger.Warnf("network option com.docker.network.bridge.enable_ip_masquerade is set to %s, but it must be true", v) - } - case "com.docker.network.bridge.host_binding_ipv4": - // must be 0.0.0.0 - if v != "0.0.0.0" { - s.logger.Warnf("network option com.docker.network.bridge.host_binding_ipv4 is set to %s, but it must be 0.0.0.0", v) - } - case "com.docker.network.bridge.enable_icc": - s.logger.Warnf("network option com.docker.network.bridge.enable_icc is not currently supported in nerdctl", v) - case "com.docker.network.bridge.name": - bridge = v - default: - options[k] = v - } + var bridgeDriver BridgeDriverOperations + var err error + + createOptionsFrom := func(request types.NetworkCreateRequest) (netutil.CreateOptions, error) { + // Default to "bridge" driver if request does not specify a driver + driver := request.Driver + if driver == "" { + driver = "bridge" } - return options - } - createOptionsFrom := func(r types.NetworkCreateRequest) netutil.CreateOptions { options := netutil.CreateOptions{ - Name: r.Name, - Driver: "bridge", + Name: request.Name, + Driver: driver, IPAMDriver: "default", - IPAMOptions: r.IPAM.Options, - Options: filterUnsupportedOptions(r.Options), - Labels: maputility.Flatten(r.Labels, maputility.KeyEqualsValueFormat), + IPAMOptions: request.IPAM.Options, + Labels: maputility.Flatten(request.Labels, maputility.KeyEqualsValueFormat), } - if r.Driver != "" { - options.Driver = r.Driver - } - if r.IPAM.Driver != "" { - options.IPAMDriver = r.IPAM.Driver + + if request.IPAM.Driver != "" { + options.IPAMDriver = request.IPAM.Driver } + if len(request.IPAM.Config) != 0 { options.Subnets = []string{} if subnet, ok := request.IPAM.Config[0]["Subnet"]; ok { @@ -76,7 +51,18 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest options.Gateway = gateway } } - return options + + // Handle driver-specific options + switch driver { + case "bridge": + bridgeDriver = NewBridgeDriver(s.netClient, s.logger) + options, err = bridgeDriver.HandleCreateOptions(request, options) + return options, err + default: + options.Options = request.Options + } + + return options, nil } if config, err := s.getNetwork(request.Name); err == nil { @@ -92,8 +78,13 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest return response, nil } - net, err := s.netClient.CreateNetwork(createOptionsFrom(request)) - warning := "" + options, err := createOptionsFrom(request) + if err != nil { + return types.NetworkCreateResponse{}, err + } + + // Create network + net, err := s.netClient.CreateNetwork(options) if err != nil && strings.Contains(err.Error(), "unsupported cni driver") { return types.NetworkCreateResponse{}, errdefs.NewNotFound(errPluginNotFound) } else if err != nil { @@ -104,11 +95,12 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest return types.NetworkCreateResponse{}, errNetworkIDNotFound } - // Since nerdctl currently does not support custom bridge names, - // we explicitly override bridge name in the conflist file for the network that was just created - if bridge != "" { - if err = s.setBridgeName(net, bridge); err != nil { - warning = fmt.Sprintf("Failed to set network bridge name %s: %s", bridge, err) + // Handle post network create actions + warning := "" + if bridgeDriver != nil { + warning, err = bridgeDriver.HandlePostCreate(net) + if err != nil { + return types.NetworkCreateResponse{}, err } } @@ -117,90 +109,3 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest Warning: warning, }, nil } - -// setBridgeName will override the bridge name in an existing CNI config file for a network. -func (s *service) setBridgeName(net *netutil.NetworkConfig, bridge string) error { - return lockutil.WithDirLock(s.netClient.NetconfPath(), func() error { - // first, make sure that the bridge name is not used by any of the existing bridge networks - bridgeNet, err := s.getNetworkByBridgeName(bridge) - if err != nil { - return err - } - if bridgeNet != nil { - return fmt.Errorf("bridge name %s already in use by network %s", bridge, bridgeNet.Name) - } - - // load the CNI config file and set bridge name - configFilename := s.getConfigPathForNetworkName(net.Name) - configFile, err := os.Open(configFilename) - if err != nil { - return err - } - defer configFile.Close() - var netJSON interface{} - if err = json.NewDecoder(configFile).Decode(&netJSON); err != nil { - return err - } - netMap, ok := netJSON.(map[string]interface{}) - if !ok { - return fmt.Errorf("network config file %s is not a valid map", configFilename) - } - plugins, ok := netMap["plugins"] - if !ok { - return fmt.Errorf("could not find plugins in network config file %s", configFilename) - } - pluginsMap, ok := plugins.([]interface{}) - if !ok { - return fmt.Errorf("could not parse plugins in network config file %s", configFilename) - } - for _, plugin := range pluginsMap { - pluginMap, ok := plugin.(map[string]interface{}) - if !ok { - continue - } - if pluginMap["type"] == "bridge" { - pluginMap["bridge"] = bridge - data, err := json.MarshalIndent(netJSON, "", " ") - if err != nil { - return err - } - return os.WriteFile(configFilename, data, 0o644) - } - } - return fmt.Errorf("bridge plugin not found in network config file %s", configFilename) - }) -} - -// From https://github.com/containerd/nerdctl/blob/v1.5.0/pkg/netutil/netutil.go#L186-L188 -func (s *service) getConfigPathForNetworkName(netName string) string { - return filepath.Join(s.netClient.NetconfPath(), "nerdctl-"+netName+".conflist") -} - -type bridgePlugin struct { - Type string `json:"type"` - Bridge string `json:"bridge"` -} - -func (s *service) getNetworkByBridgeName(bridge string) (*netutil.NetworkConfig, error) { - networks, err := s.netClient.FilterNetworks(func(*netutil.NetworkConfig) bool { - return true - }) - if err != nil { - return nil, err - } - for _, network := range networks { - for _, plugin := range network.Plugins { - if plugin.Network.Type != "bridge" { - continue - } - var bridgeJSON bridgePlugin - if err = json.Unmarshal(plugin.Bytes, &bridgeJSON); err != nil { - continue - } - if bridgeJSON.Bridge == bridge { - return network, nil - } - } - } - return nil, nil -} diff --git a/internal/service/network/create_test.go b/internal/service/network/create_test.go index edcd302e..294fe6c1 100644 --- a/internal/service/network/create_test.go +++ b/internal/service/network/create_test.go @@ -6,6 +6,7 @@ package network import ( "context" "errors" + "fmt" "github.com/containerd/nerdctl/pkg/netutil" "github.com/golang/mock/gomock" @@ -14,8 +15,11 @@ import ( "github.com/runfinch/finch-daemon/api/handlers/network" "github.com/runfinch/finch-daemon/api/types" + "github.com/runfinch/finch-daemon/internal/backend" "github.com/runfinch/finch-daemon/mocks/mocks_backend" "github.com/runfinch/finch-daemon/mocks/mocks_logger" + "github.com/runfinch/finch-daemon/mocks/mocks_network" + "github.com/runfinch/finch-daemon/pkg/flog" ) var _ = Describe("Network Service Create Network Implementation", func() { @@ -25,12 +29,13 @@ var _ = Describe("Network Service Create Network Implementation", func() { ) var ( - ctx context.Context - mockController *gomock.Controller - cdClient *mocks_backend.MockContainerdClient - ncNetClient *mocks_backend.MockNerdctlNetworkSvc - logger *mocks_logger.Logger - service network.Service + ctx context.Context + mockController *gomock.Controller + cdClient *mocks_backend.MockContainerdClient + ncNetClient *mocks_backend.MockNerdctlNetworkSvc + logger *mocks_logger.Logger + service network.Service + mockBridgeDriver *mocks_network.BridgeDriver ) BeforeEach(func() { @@ -40,6 +45,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { ncNetClient = mocks_backend.NewMockNerdctlNetworkSvc(mockController) logger = mocks_logger.NewLogger(mockController) service = NewService(cdClient, ncNetClient, logger) + mockBridgeDriver = mocks_network.NewBridgeDriver(mockController) }) When("a create network call is successful", func() { @@ -336,4 +342,131 @@ var _ = Describe("Network Service Create Network Implementation", func() { }) }) }) + + Context("ICC configuration", func() { + When("com.docker.network.bridge.enable_icc is set to true in the request", func() { + It("should not change default behavior", func() { + request := types.NewCreateNetworkRequest( + networkName, + types.WithOptions(map[string]string{ + "com.docker.network.bridge.enable_icc": "true", + }), + ) + + ncNetClient.EXPECT().FilterNetworks(gomock.Any()).Return([]*netutil.NetworkConfig{}, nil) + logger.EXPECT().Debugf(gomock.Any(), gomock.Any()) + + nid := networkID + ncNetClient.EXPECT().CreateNetwork(gomock.Any()).DoAndReturn(func(actual netutil.CreateOptions) (*netutil.NetworkConfig, error) { + // Check if the label exists + checkLabel := "com.docker.network.bridge.enable_icc=true" + labelExists := false + for _, label := range actual.Labels { + if label == checkLabel { + labelExists = true + break + } + } + + // Expect that DisableICC is not called + mockBridgeDriver.EXPECT().DisableICC(gomock.Any(), gomock.Any()).Times(0) + + Expect(labelExists).To(BeFalse(), fmt.Sprintf("Label '%s' should not exist in Labels", checkLabel)) + + return &netutil.NetworkConfig{NerdctlID: &nid}, nil + }) + + response, err := service.Create(ctx, *request) + Expect(err).ShouldNot(HaveOccurred()) + Expect(response.ID).Should(Equal(networkID)) + }) + }) + + When("ICC is not specified in the request", func() { + It("should use the default setting in the create options and not set any icc labels", func() { + request := types.NewCreateNetworkRequest(networkName) + + ncNetClient.EXPECT().FilterNetworks(gomock.Any()).Return([]*netutil.NetworkConfig{}, nil) + logger.EXPECT().Debugf(gomock.Any(), gomock.Any()) + + nid := networkID + ncNetClient.EXPECT().CreateNetwork(gomock.Any()).DoAndReturn(func(actual netutil.CreateOptions) (*netutil.NetworkConfig, error) { + // Check if the label exists + expectedLabel := "com.docker.network.bridge.enable_icc=true" + labelExists := false + for _, label := range actual.Labels { + if label == expectedLabel { + labelExists = true + break + } + } + + Expect(labelExists).To(BeFalse(), fmt.Sprintf("Label '%s' should not exist in Labels", expectedLabel)) + + return &netutil.NetworkConfig{NerdctlID: &nid}, nil + }) + + response, err := service.Create(ctx, *request) + Expect(err).ShouldNot(HaveOccurred()) + Expect(response.ID).Should(Equal(networkID)) + }) + }) + + When("com.docker.network.bridge.enable_icc is set to false in the request", func() { + It("should set ICC to false in the create options", func() { + request := types.NewCreateNetworkRequest( + networkName, + types.WithOptions(map[string]string{ + BridgeICCOption: "false", + }), + ) + + ncNetClient.EXPECT().FilterNetworks(gomock.Any()).Return([]*netutil.NetworkConfig{}, nil) + logger.EXPECT().Debugf(gomock.Any(), gomock.Any()) + + nid := networkID + ncNetClient.EXPECT().CreateNetwork(gomock.Any()).DoAndReturn(func(actual netutil.CreateOptions) (*netutil.NetworkConfig, error) { + // Check if the label exists + expectedLabel := FinchICCLabel + "=false" + labelExists := false + for _, label := range actual.Labels { + if label == expectedLabel { + labelExists = true + break + } + } + + Expect(labelExists).To(BeTrue(), fmt.Sprintf("Label '%s' should exist in Labels", expectedLabel)) + + return &netutil.NetworkConfig{NerdctlID: &nid}, nil + }) + + originalDriverFunc := NewBridgeDriver + NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logger) BridgeDriverOperations { + return mockBridgeDriver + } + defer func() { NewBridgeDriver = originalDriverFunc }() + + // Set up expectations for mockBridgeDriver + mockBridgeDriver.EXPECT().HandleCreateOptions(gomock.Any(), gomock.Any()).DoAndReturn( + func(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) { + // Mock the behavior for BridgeICCOption set to false + // Remove the option from the options map + fmt.Println("HandleCreateOptions called with options:", options) + delete(options.Options, BridgeICCOption) + options.Labels = append(options.Labels, FinchICCLabel+"=false") + return options, nil + }).AnyTimes() + + mockBridgeDriver.EXPECT().HandlePostCreate(gomock.Any()).Return("", nil).AnyTimes() + mockBridgeDriver.EXPECT().ICCDisabled().DoAndReturn(func() bool { + return true + }).AnyTimes() + + response, err := service.Create(ctx, *request) + Expect(err).ShouldNot(HaveOccurred()) + Expect(response.ID).Should(Equal(networkID)) + }) + }) + }) }) diff --git a/internal/service/network/remove.go b/internal/service/network/remove.go index 674bb49e..e9c3a6cf 100644 --- a/internal/service/network/remove.go +++ b/internal/service/network/remove.go @@ -7,6 +7,7 @@ import ( "context" "fmt" + "github.com/containerd/nerdctl/pkg/netutil" "github.com/runfinch/finch-daemon/pkg/errdefs" ) @@ -25,8 +26,51 @@ func (s *service) Remove(ctx context.Context, networkId string) error { if value, ok := usedNetworkInfo[net.Name]; ok { return errdefs.NewForbidden(fmt.Errorf("network %q is in use by container %q", networkId, value)) } + if net.File == "" { return errdefs.NewForbidden(fmt.Errorf("%s is a pre-defined network and cannot be removed", networkId)) } + + // Perform additional workflow based on the assigned network labels + if err := s.handleNetworkLabels(net); err != nil { + return fmt.Errorf("failed to handle nerdctl label: %w", err) + } + return s.netClient.RemoveNetwork(net) } + +func (s *service) handleNetworkLabels(net *netutil.NetworkConfig) error { + if net.NerdctlLabels == nil { + return nil + } + + for key, value := range *net.NerdctlLabels { + switch key { + case FinchICCLabel: + if err := s.handleEnableICCOption(net, value); err != nil { + return fmt.Errorf("error handling %s label: %w", BridgeICCOption, err) + } + } + } + return nil +} + +func (s *service) handleEnableICCOption(net *netutil.NetworkConfig, value string) error { + if value != "false" { + // for some reason the label value got modified. + // we will still try to remove the iptable rules. + // iptable.DeleteIfExists is used to ignore non-existent errors + s.logger.Warnf("unexpected value for %s label: %s", BridgeICCOption, value) + } + // Remove iptable rules set for disabling ICC for the network bridge + bridgeDriver := NewBridgeDriver(s.netClient, s.logger) + bridgeName, err := bridgeDriver.GetBridgeName(net) + if err != nil { + return fmt.Errorf("unable to get bridge name: %w", err) + } + err = bridgeDriver.DisableICC(bridgeName, false) + if err != nil { + return fmt.Errorf("unable to remove ICC disable rule: %w", err) + } + return nil +} diff --git a/mocks/mocks_backend/nerdctlimagesvc.go b/mocks/mocks_backend/nerdctlimagesvc.go index 018db5da..e4121ae2 100644 --- a/mocks/mocks_backend/nerdctlimagesvc.go +++ b/mocks/mocks_backend/nerdctlimagesvc.go @@ -10,12 +10,12 @@ import ( reflect "reflect" images "github.com/containerd/containerd/images" - platforms "github.com/containerd/platforms" remotes "github.com/containerd/containerd/remotes" docker "github.com/containerd/containerd/remotes/docker" imgutil "github.com/containerd/nerdctl/pkg/imgutil" dockerconfigresolver "github.com/containerd/nerdctl/pkg/imgutil/dockerconfigresolver" dockercompat "github.com/containerd/nerdctl/pkg/inspecttypes/dockercompat" + platforms "github.com/containerd/platforms" gomock "github.com/golang/mock/gomock" v1 "github.com/opencontainers/image-spec/specs-go/v1" ) diff --git a/mocks/mocks_container/containersvc.go b/mocks/mocks_container/containersvc.go index be39abd1..7b80ef56 100644 --- a/mocks/mocks_container/containersvc.go +++ b/mocks/mocks_container/containersvc.go @@ -191,7 +191,7 @@ func (m *MockService) Restart(arg0 context.Context, arg1 string, arg2 time.Durat ret0, _ := ret[0].(error) return ret0 } - + // Restart indicates an expected call of Restart. func (mr *MockServiceMockRecorder) Restart(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() diff --git a/mocks/mocks_network/bridge_driver.go b/mocks/mocks_network/bridge_driver.go new file mode 100644 index 00000000..ad2f551f --- /dev/null +++ b/mocks/mocks_network/bridge_driver.go @@ -0,0 +1,150 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/runfinch/finch-daemon/internal/service/network (interfaces: BridgeDriverOperations) + +// Package mocks_network is a generated GoMock package. +package mocks_network + +import ( + reflect "reflect" + + netutil "github.com/containerd/nerdctl/pkg/netutil" + gomock "github.com/golang/mock/gomock" + types "github.com/runfinch/finch-daemon/api/types" +) + +// BridgeDriver is a mock of BridgeDriverOperations interface. +type BridgeDriver struct { + ctrl *gomock.Controller + recorder *BridgeDriverMockRecorder +} + +// BridgeDriverMockRecorder is the mock recorder for BridgeDriver. +type BridgeDriverMockRecorder struct { + mock *BridgeDriver +} + +// NewBridgeDriver creates a new mock instance. +func NewBridgeDriver(ctrl *gomock.Controller) *BridgeDriver { + mock := &BridgeDriver{ctrl: ctrl} + mock.recorder = &BridgeDriverMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *BridgeDriver) EXPECT() *BridgeDriverMockRecorder { + return m.recorder +} + +// DisableICC mocks base method. +func (m *BridgeDriver) DisableICC(arg0 string, arg1 bool) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DisableICC", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DisableICC indicates an expected call of DisableICC. +func (mr *BridgeDriverMockRecorder) DisableICC(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DisableICC", reflect.TypeOf((*BridgeDriver)(nil).DisableICC), arg0, arg1) +} + +// GetBridgeName mocks base method. +func (m *BridgeDriver) GetBridgeName(arg0 *netutil.NetworkConfig) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetBridgeName", arg0) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetBridgeName indicates an expected call of GetBridgeName. +func (mr *BridgeDriverMockRecorder) GetBridgeName(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBridgeName", reflect.TypeOf((*BridgeDriver)(nil).GetBridgeName), arg0) +} + +// GetNetworkByBridgeName mocks base method. +func (m *BridgeDriver) GetNetworkByBridgeName(arg0 string) (*netutil.NetworkConfig, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetNetworkByBridgeName", arg0) + ret0, _ := ret[0].(*netutil.NetworkConfig) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetNetworkByBridgeName indicates an expected call of GetNetworkByBridgeName. +func (mr *BridgeDriverMockRecorder) GetNetworkByBridgeName(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkByBridgeName", reflect.TypeOf((*BridgeDriver)(nil).GetNetworkByBridgeName), arg0) +} + +// HandleCreateOptions mocks base method. +func (m *BridgeDriver) HandleCreateOptions(arg0 types.NetworkCreateRequest, arg1 netutil.CreateOptions) (netutil.CreateOptions, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HandleCreateOptions", arg0, arg1) + ret0, _ := ret[0].(netutil.CreateOptions) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HandleCreateOptions indicates an expected call of HandleCreateOptions. +func (mr *BridgeDriverMockRecorder) HandleCreateOptions(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandleCreateOptions", reflect.TypeOf((*BridgeDriver)(nil).HandleCreateOptions), arg0, arg1) +} + +// HandlePostCreate mocks base method. +func (m *BridgeDriver) HandlePostCreate(arg0 *netutil.NetworkConfig) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HandlePostCreate", arg0) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HandlePostCreate indicates an expected call of HandlePostCreate. +func (mr *BridgeDriverMockRecorder) HandlePostCreate(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandlePostCreate", reflect.TypeOf((*BridgeDriver)(nil).HandlePostCreate), arg0) +} + +// ICCDisabled mocks base method. +func (m *BridgeDriver) ICCDisabled() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ICCDisabled") + ret0, _ := ret[0].(bool) + return ret0 +} + +// ICCDisabled indicates an expected call of ICCDisabled. +func (mr *BridgeDriverMockRecorder) ICCDisabled() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ICCDisabled", reflect.TypeOf((*BridgeDriver)(nil).ICCDisabled)) +} + +// SetBridgeName mocks base method. +func (m *BridgeDriver) SetBridgeName(arg0 *netutil.NetworkConfig, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetBridgeName", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetBridgeName indicates an expected call of SetBridgeName. +func (mr *BridgeDriverMockRecorder) SetBridgeName(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetBridgeName", reflect.TypeOf((*BridgeDriver)(nil).SetBridgeName), arg0, arg1) +} + +// SetICCDisabled mocks base method. +func (m *BridgeDriver) SetICCDisabled() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetICCDisabled") +} + +// SetICCDisabled indicates an expected call of SetICCDisabled. +func (mr *BridgeDriverMockRecorder) SetICCDisabled() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetICCDisabled", reflect.TypeOf((*BridgeDriver)(nil).SetICCDisabled)) +} diff --git a/mocks/mocks_network/iptables.go b/mocks/mocks_network/iptables.go new file mode 100644 index 00000000..87a922da --- /dev/null +++ b/mocks/mocks_network/iptables.go @@ -0,0 +1,120 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/runfinch/finch-daemon/internal/service/network (interfaces: IPTablesWrapper) + +// Package mocks_network is a generated GoMock package. +package mocks_network + +import ( + reflect "reflect" + + gomock "github.com/golang/mock/gomock" +) + +// MockIPTablesWrapper is a mock of IPTablesWrapper interface. +type MockIPTablesWrapper struct { + ctrl *gomock.Controller + recorder *MockIPTablesWrapperMockRecorder +} + +// MockIPTablesWrapperMockRecorder is the mock recorder for MockIPTablesWrapper. +type MockIPTablesWrapperMockRecorder struct { + mock *MockIPTablesWrapper +} + +// NewMockIPTablesWrapper creates a new mock instance. +func NewMockIPTablesWrapper(ctrl *gomock.Controller) *MockIPTablesWrapper { + mock := &MockIPTablesWrapper{ctrl: ctrl} + mock.recorder = &MockIPTablesWrapperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockIPTablesWrapper) EXPECT() *MockIPTablesWrapperMockRecorder { + return m.recorder +} + +// Append mocks base method. +func (m *MockIPTablesWrapper) Append(arg0, arg1 string, arg2 ...string) error { + m.ctrl.T.Helper() + varargs := []interface{}{arg0, arg1} + for _, a := range arg2 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Append", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// Append indicates an expected call of Append. +func (mr *MockIPTablesWrapperMockRecorder) Append(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0, arg1}, arg2...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Append", reflect.TypeOf((*MockIPTablesWrapper)(nil).Append), varargs...) +} + +// ChainExists mocks base method. +func (m *MockIPTablesWrapper) ChainExists(arg0, arg1 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ChainExists", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ChainExists indicates an expected call of ChainExists. +func (mr *MockIPTablesWrapperMockRecorder) ChainExists(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ChainExists", reflect.TypeOf((*MockIPTablesWrapper)(nil).ChainExists), arg0, arg1) +} + +// DeleteIfExists mocks base method. +func (m *MockIPTablesWrapper) DeleteIfExists(arg0, arg1 string, arg2 ...string) error { + m.ctrl.T.Helper() + varargs := []interface{}{arg0, arg1} + for _, a := range arg2 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "DeleteIfExists", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteIfExists indicates an expected call of DeleteIfExists. +func (mr *MockIPTablesWrapperMockRecorder) DeleteIfExists(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0, arg1}, arg2...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteIfExists", reflect.TypeOf((*MockIPTablesWrapper)(nil).DeleteIfExists), varargs...) +} + +// Insert mocks base method. +func (m *MockIPTablesWrapper) Insert(arg0, arg1 string, arg2 int, arg3 ...string) error { + m.ctrl.T.Helper() + varargs := []interface{}{arg0, arg1, arg2} + for _, a := range arg3 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Insert", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// Insert indicates an expected call of Insert. +func (mr *MockIPTablesWrapperMockRecorder) Insert(arg0, arg1, arg2 interface{}, arg3 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0, arg1, arg2}, arg3...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Insert", reflect.TypeOf((*MockIPTablesWrapper)(nil).Insert), varargs...) +} + +// NewChain mocks base method. +func (m *MockIPTablesWrapper) NewChain(arg0, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "NewChain", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// NewChain indicates an expected call of NewChain. +func (mr *MockIPTablesWrapperMockRecorder) NewChain(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewChain", reflect.TypeOf((*MockIPTablesWrapper)(nil).NewChain), arg0, arg1) +} From 8f7fd02a6f25efd8940e411c915ae01c5bc3f6c0 Mon Sep 17 00:00:00 2001 From: Swagat Bora Date: Mon, 28 Oct 2024 20:37:44 +0000 Subject: [PATCH 2/6] address review comments and add support for iptables v6 Signed-off-by: Swagat Bora --- api/types/network_types.go | 2 - go.mod | 10 +- go.sum | 13 +- internal/service/network/bridge_driver.go | 375 ------------------ .../service/network/bridge_driver_test.go | 123 ------ internal/service/network/create.go | 19 +- internal/service/network/create_test.go | 35 +- internal/service/network/driver/bridge.go | 303 ++++++++++++++ .../service/network/driver/bridge_test.go | 129 ++++++ internal/service/network/driver/driver.go | 16 + .../service/network/driver/driver_test.go | 17 + internal/service/network/driver/iptables.go | 146 +++++++ internal/service/network/remove.go | 29 +- mocks/mocks_network/bridge_driver.go | 150 ------- mocks/mocks_network/driver.go | 80 ++++ mocks/mocks_network/iptables.go | 40 +- 16 files changed, 774 insertions(+), 713 deletions(-) delete mode 100644 internal/service/network/bridge_driver.go delete mode 100644 internal/service/network/bridge_driver_test.go create mode 100644 internal/service/network/driver/bridge.go create mode 100644 internal/service/network/driver/bridge_test.go create mode 100644 internal/service/network/driver/driver.go create mode 100644 internal/service/network/driver/driver_test.go create mode 100644 internal/service/network/driver/iptables.go delete mode 100644 mocks/mocks_network/bridge_driver.go create mode 100644 mocks/mocks_network/driver.go diff --git a/api/types/network_types.go b/api/types/network_types.go index 7378b829..d9e63bd0 100644 --- a/api/types/network_types.go +++ b/api/types/network_types.go @@ -156,8 +156,6 @@ type NetworkCreateRequest struct { IPAM IPAM `json:"IPAM"` // EnableIPv6 specifies to enable IPv6 on the network. - // - // IPv6 networks are not currently supported. EnableIPv6 bool `json:"EnableIPv6"` // Options specifies network specific options to be used by the drivers. diff --git a/go.mod b/go.mod index d3f192b4..1a131ca1 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,8 @@ module github.com/runfinch/finch-daemon -go 1.22 +go 1.22.0 + +toolchain go1.22.7 require ( github.com/containerd/cgroups/v3 v3.0.3 @@ -66,13 +68,14 @@ require ( github.com/containerd/typeurl v1.0.3-0.20220422153119-7f6e6d160d67 // indirect github.com/containernetworking/plugins v1.4.1 // indirect github.com/containers/ocicrypt v1.1.10 // indirect - github.com/cyphar/filepath-securejoin v0.2.4 // indirect - github.com/davecgh/go-spew v1.1.1 // indirect + github.com/cyphar/filepath-securejoin v0.3.1 // indirect + github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/djherbis/times v1.6.0 // indirect github.com/docker/docker-credential-helpers v0.8.1 // indirect github.com/docker/go-events v0.0.0-20190806004212-e31b211e4f1c // indirect github.com/docker/go-units v0.5.0 // indirect github.com/fahedouch/go-logrotate v0.2.0 // indirect + github.com/fatih/color v1.17.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/fluent/fluent-logger-golang v1.9.0 // indirect github.com/getlantern/mockconn v0.0.0-20200818071412-cb30d065a848 // indirect @@ -142,7 +145,6 @@ require ( go.opentelemetry.io/otel/trace v1.30.0 // indirect golang.org/x/crypto v0.27.0 // indirect golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect - golang.org/x/mod v0.20.0 // indirect golang.org/x/sync v0.8.0 // indirect golang.org/x/term v0.24.0 // indirect golang.org/x/text v0.18.0 // indirect diff --git a/go.sum b/go.sum index c4530bdf..6217b410 100644 --- a/go.sum +++ b/go.sum @@ -78,11 +78,12 @@ github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSV github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= -github.com/cyphar/filepath-securejoin v0.2.4 h1:Ugdm7cg7i6ZK6x3xDF1oEu1nfkyfH53EtKeQYTC3kyg= -github.com/cyphar/filepath-securejoin v0.2.4/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4= +github.com/cyphar/filepath-securejoin v0.3.1 h1:1V7cHiaW+C+39wEfpH6XlLBQo3j/PciWFrgfCLS8XrE= +github.com/cyphar/filepath-securejoin v0.3.1/go.mod h1:F7i41x/9cBF7lzCrVsYs9fuzwRZm4NQsGTBdpp6mETc= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= +github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5Y4f/wlDRiLyi3E= github.com/djherbis/times v1.6.0 h1:w2ctJ92J8fBvWPxugmXIv7Nz7Q3iDMKNx9v5ocVH20c= @@ -105,8 +106,8 @@ github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1m github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/fahedouch/go-logrotate v0.2.0 h1:UR9Fv8MDVfWwnkirmFHck+tRSWzqOwRjVRLMpQgSxaI= github.com/fahedouch/go-logrotate v0.2.0/go.mod h1:1RL/yr7LntS4zadAC6FT6yB/C1CQt3V6eHAZzymfwzE= -github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= -github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= +github.com/fatih/color v1.17.0 h1:GlRw1BRJxkpqUCBKzKOw098ed57fEsKeNjpTe3cSjK4= +github.com/fatih/color v1.17.0/go.mod h1:YZ7TlrGPkiz6ku9fK3TLD/pl3CpsiFyu8N92HLgmosI= github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2Wg= github.com/felixge/httpsnoop v1.0.4/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= github.com/fluent/fluent-logger-golang v1.9.0 h1:zUdY44CHX2oIUc7VTNZc+4m+ORuO/mldQDA7czhWXEg= @@ -361,8 +362,6 @@ golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= -golang.org/x/mod v0.20.0 h1:utOm6MM3R3dnawAiJgn0y+xvuYRsm1RKM/4giyfDgV0= -golang.org/x/mod v0.20.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= 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= golang.org/x/net v0.0.0-20190213061140-3a22650c66bd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= diff --git a/internal/service/network/bridge_driver.go b/internal/service/network/bridge_driver.go deleted file mode 100644 index f6ae1dec..00000000 --- a/internal/service/network/bridge_driver.go +++ /dev/null @@ -1,375 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package network - -import ( - "encoding/json" - "fmt" - "os" - "path/filepath" - "strconv" - - "github.com/containerd/nerdctl/pkg/lockutil" - "github.com/containerd/nerdctl/pkg/netutil" - "github.com/coreos/go-iptables/iptables" - "github.com/runfinch/finch-daemon/api/types" - "github.com/runfinch/finch-daemon/internal/backend" - "github.com/runfinch/finch-daemon/pkg/flog" -) - -const ( - FinchICCLabel = "finch.network.bridge.enable_icc" - - BridgeICCOption = "com.docker.network.bridge.enable_icc" - BridgeHostBindingIpv4Option = "com.docker.network.bridge.host_binding_ipv4" - BridgeNameOption = "com.docker.network.bridge.name" -) - -//go:generate mockgen --destination=../../../mocks/mocks_network/bridge_driver.go -package=mocks_network -mock_names BridgeDriverOperations=BridgeDriver . BridgeDriverOperations -type BridgeDriverOperations interface { - HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) - HandlePostCreate(net *netutil.NetworkConfig) (string, error) - SetBridgeName(net *netutil.NetworkConfig, bridge string) error - GetBridgeName(net *netutil.NetworkConfig) (string, error) - GetNetworkByBridgeName(bridge string) (*netutil.NetworkConfig, error) - DisableICC(bridgeIface string, insert bool) error - SetICCDisabled() - ICCDisabled() bool -} - -// IPTablesWrapper is an interface that wraps the methods of iptables.IPTables -// to help with mock -// -//go:generate mockgen --destination=../../../mocks/mocks_network/iptables.go -package=mocks_network . IPTablesWrapper -type IPTablesWrapper interface { - ChainExists(table, chain string) (bool, error) - NewChain(table, chain string) error - Insert(table, chain string, pos int, rulespec ...string) error - Append(table, chain string, rulespec ...string) error - DeleteIfExists(table, chain string, rulespec ...string) error -} - -// IPTablesWrapperImpl implements IPTablesWrapper -// that delegates to an actual iptables.IPTables instance. -type IPTablesWrapperImpl struct { - ipt *iptables.IPTables -} - -func NewIPTablesWrapper() IPTablesWrapper { - iptables, _ := iptables.NewWithProtocol(iptables.ProtocolIPv4) - return &IPTablesWrapperImpl{ipt: iptables} -} - -func (i *IPTablesWrapperImpl) ChainExists(table, chain string) (bool, error) { - return i.ipt.ChainExists(table, chain) -} - -func (i *IPTablesWrapperImpl) NewChain(table, chain string) error { - return i.ipt.NewChain(table, chain) -} - -func (i *IPTablesWrapperImpl) Insert(table, chain string, pos int, rulespec ...string) error { - return i.ipt.Insert(table, chain, pos, rulespec...) -} - -func (i *IPTablesWrapperImpl) Append(table, chain string, rulespec ...string) error { - return i.ipt.Append(table, chain, rulespec...) -} - -func (i *IPTablesWrapperImpl) DeleteIfExists(table, chain string, rulespec ...string) error { - return i.ipt.DeleteIfExists(table, chain, rulespec...) -} - -type bridgeDriver struct { - bridgeName string - disableICC bool - netClient backend.NerdctlNetworkSvc - logger flog.Logger - ipt IPTablesWrapper -} - -var _ BridgeDriverOperations = (*bridgeDriver)(nil) - -var NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logger) BridgeDriverOperations { - return &bridgeDriver{ - netClient: netClient, - logger: logger, - ipt: NewIPTablesWrapper(), - } -} - -// handleBridgeDriverOptions filters unsupported options for the bridge driver. -func (bd *bridgeDriver) HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) { - // enable_icc, host_binding_ipv4, and bridge name network options are not supported by nerdctl. - // So we must filter out any unsupported options which would prevent the network from being created and accept the defaults. - filterUnsupportedOptions := func(original map[string]string) map[string]string { - opts := map[string]string{} - for k, v := range original { - switch k { - case BridgeHostBindingIpv4Option: - if v != "0.0.0.0" { - bd.logger.Warnf("network option com.docker.network.bridge.host_binding_ipv4 is set to %s, but it must be 0.0.0.0", v) - } - case BridgeICCOption: - iccOption, err := strconv.ParseBool(v) //t - if err != nil { - bd.logger.Warnf("invalid value for com.docker.network.bridge.enable_icc: %s", v) - } - if !iccOption { - bd.SetICCDisabled() - } - - case BridgeNameOption: - bd.bridgeName = v - default: - opts[k] = v - } - } - return opts - } - - options.Options = filterUnsupportedOptions(request.Options) - - if bd.ICCDisabled() { - // Append a label when enable_icc is set to false. This is used for clean up during network removal. - options.Labels = append(options.Labels, FinchICCLabel+"=false") - } - - // Return the modified options - return options, nil -} - -func (bd *bridgeDriver) HandlePostCreate(net *netutil.NetworkConfig) (string, error) { - // Handle bridge driver post create actions - var warning string - var err error - bridgeName := bd.bridgeName - if bridgeName != "" { - // Since nerdctl currently does not support custom bridge names, - // we explicitly override bridge name in the conflist file for the network that was just created - if err = bd.SetBridgeName(net, bridgeName); err != nil { - warning = fmt.Sprintf("Failed to set network bridge name %s: %s", bridgeName, err) - } - } - - if bd.ICCDisabled() { - // Handle "enable_icc=false" option if set (bd.disableICC is true) - // By default, CNI allows connectivity between containers attached to the same bridge. - // If "com.docker.network.bridge.enable_icc" option is explicitly set to false, - // we disable inter-container connectivity by applying iptable rules - // If "com.docker.network.bridge.enable_icc=true" is set, it is considered a noop - if bridgeName == "" { - bridgeName, err = bd.GetBridgeName(net) - if err != nil { - return "", fmt.Errorf("failed to get bridge name to enable inter-container connectivity: %w ", err) - } - } - err = bd.DisableICC(bridgeName, true) - if err != nil { - return "", fmt.Errorf("failed to disable inter-container connectivity: %w", err) - } - } - - return warning, nil -} - -// setBridgeName will override the bridge name in an existing CNI config file for a network. -func (bd *bridgeDriver) SetBridgeName(net *netutil.NetworkConfig, bridge string) error { - return lockutil.WithDirLock(bd.netClient.NetconfPath(), func() error { - // first, make sure that the bridge name is not used by any of the existing bridge networks - bridgeNet, err := bd.GetNetworkByBridgeName(bridge) - if err != nil { - return err - } - if bridgeNet != nil { - return fmt.Errorf("bridge name %s already in use by network %s", bridge, bridgeNet.Name) - } - - // load the CNI config file and set bridge name - configFilename := bd.getConfigPathForNetworkName(net.Name) - configFile, err := os.Open(configFilename) - if err != nil { - return err - } - defer configFile.Close() - var netJSON interface{} - if err = json.NewDecoder(configFile).Decode(&netJSON); err != nil { - return err - } - netMap, ok := netJSON.(map[string]interface{}) - if !ok { - return fmt.Errorf("network config file %s is not a valid map", configFilename) - } - plugins, ok := netMap["plugins"] - if !ok { - return fmt.Errorf("could not find plugins in network config file %s", configFilename) - } - pluginsMap, ok := plugins.([]interface{}) - if !ok { - return fmt.Errorf("could not parse plugins in network config file %s", configFilename) - } - for _, plugin := range pluginsMap { - pluginMap, ok := plugin.(map[string]interface{}) - if !ok { - continue - } - if pluginMap["type"] == "bridge" { - pluginMap["bridge"] = bridge - data, err := json.MarshalIndent(netJSON, "", " ") - if err != nil { - return err - } - return os.WriteFile(configFilename, data, 0o644) - } - } - return fmt.Errorf("bridge plugin not found in network config file %s", configFilename) - }) -} - -func (bd *bridgeDriver) GetBridgeName(net *netutil.NetworkConfig) (string, error) { - var bridgeName string - err := lockutil.WithDirLock(bd.netClient.NetconfPath(), func() error { - configFilename := bd.getConfigPathForNetworkName(net.Name) - configFile, err := os.Open(configFilename) - if err != nil { - return err - } - defer configFile.Close() - - var netJSON interface{} - if err = json.NewDecoder(configFile).Decode(&netJSON); err != nil { - return err - } - - netMap, ok := netJSON.(map[string]interface{}) - if !ok { - return fmt.Errorf("network config file %s is not a valid map", configFilename) - } - - plugins, ok := netMap["plugins"] - if !ok { - return fmt.Errorf("could not find plugins in network config file %s", configFilename) - } - - pluginsMap, ok := plugins.([]interface{}) - if !ok { - return fmt.Errorf("could not parse plugins in network config file %s", configFilename) - } - - for _, plugin := range pluginsMap { - pluginMap, ok := plugin.(map[string]interface{}) - if !ok { - continue - } - if pluginMap["type"] == "bridge" { - bridge, ok := pluginMap["bridge"].(string) - if !ok { - return fmt.Errorf("bridge name in config file %s is not a string", configFilename) - } - bridgeName = bridge - return nil - } - } - - return fmt.Errorf("bridge plugin not found in network config file %s", configFilename) - }) - - if err != nil { - return "", err - } - - return bridgeName, nil -} - -type bridgePlugin struct { - Type string `json:"type"` - Bridge string `json:"bridge"` -} - -func (bd *bridgeDriver) GetNetworkByBridgeName(bridge string) (*netutil.NetworkConfig, error) { - networks, err := bd.netClient.FilterNetworks(func(*netutil.NetworkConfig) bool { - return true - }) - if err != nil { - return nil, err - } - for _, network := range networks { - for _, plugin := range network.Plugins { - if plugin.Network.Type != "bridge" { - continue - } - var bridgeJSON bridgePlugin - if err = json.Unmarshal(plugin.Bytes, &bridgeJSON); err != nil { - continue - } - if bridgeJSON.Bridge == bridge { - return network, nil - } - } - } - return nil, nil -} - -func (bd *bridgeDriver) SetICCDisabled() { - bd.disableICC = true -} - -func (bd *bridgeDriver) ICCDisabled() bool { - return bd.disableICC -} - -func (bd *bridgeDriver) DisableICC(bridgeIface string, insert bool) error { - filterTable := "filter" - isolateChain := "FINCH-ISOLATE-CHAIN" - - if bd.ipt == nil { - return fmt.Errorf("iptables is not initialized") - } - - // Check if the FINCH-ISOLATE-CHAIN already exists - exists, err := bd.ipt.ChainExists(filterTable, isolateChain) - if err != nil { - return fmt.Errorf("failed to check if %s chain exists: %v", isolateChain, err) - } - - if !exists { - // Create and setup the FINCH-ISOLATE-CHAIN chain if it doesn't exist - err = bd.ipt.NewChain(filterTable, isolateChain) - if err != nil { - return fmt.Errorf("failed to create %s chain: %v", isolateChain, err) - } - // Add a rule to the FORWARD chain that jumps to the FINCH-ISOLATE-CHAIN for all packets - jumpRule := []string{"-j", isolateChain} - err = bd.ipt.Insert(filterTable, "FORWARD", 1, jumpRule...) - if err != nil { - return fmt.Errorf("failed to add %s jump rule to FORWARD chain: %v", isolateChain, err) - } - // In the FINCH-ISOLATE-CHAIN, add a rule to return to the FORWARD chain if no match - returnRule := []string{"-j", "RETURN"} - err = bd.ipt.Append(filterTable, isolateChain, returnRule...) - if err != nil { - return fmt.Errorf("failed to add RETURN rule to DOCKER-ISOLATE chain: %v", err) - } - } - - // In the FINCH-ISOLATE-CHAIN, add or remove the DROP rule for packets from and to the same bridge - dropRule := []string{"-i", bridgeIface, "-o", bridgeIface, "-j", "DROP"} - if insert { - err = bd.ipt.Insert(filterTable, isolateChain, 1, dropRule...) - if err != nil { - return fmt.Errorf("failed to add DROP rule to %s chain: %v", isolateChain, err) - } - } else { - err = bd.ipt.DeleteIfExists(filterTable, isolateChain, dropRule...) - if err != nil { - return fmt.Errorf("failed to remove DROP rule from %s chain: %v", isolateChain, err) - } - } - return nil -} - -// From https://github.com/containerd/nerdctl/blob/v1.5.0/pkg/netutil/netutil.go#L186-L188 -func (bd *bridgeDriver) getConfigPathForNetworkName(netName string) string { - return filepath.Join(bd.netClient.NetconfPath(), "nerdctl-"+netName+".conflist") -} diff --git a/internal/service/network/bridge_driver_test.go b/internal/service/network/bridge_driver_test.go deleted file mode 100644 index 93b3e3a0..00000000 --- a/internal/service/network/bridge_driver_test.go +++ /dev/null @@ -1,123 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package network - -import ( - "fmt" - - "github.com/golang/mock/gomock" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "github.com/runfinch/finch-daemon/mocks/mocks_network" -) - -var _ = Describe("bridgeDriver DisableICC", func() { - var ( - mockController *gomock.Controller - mockIpt *mocks_network.MockIPTablesWrapper - driver *bridgeDriver - bridgeIface string - ) - - BeforeEach(func() { - mockController = gomock.NewController(GinkgoT()) - mockIpt = mocks_network.NewMockIPTablesWrapper(mockController) - driver = &bridgeDriver{ipt: mockIpt} - bridgeIface = "br-mock" - }) - - Context("FINCH-ISOLATE-CHAIN chain does not exist", func() { - BeforeEach(func() { - // FINCH-ISOLATE-CHAIN does not exist initially - mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, nil) - - // NewChain method should be called - mockIpt.EXPECT().NewChain("filter", "FINCH-ISOLATE-CHAIN").Return(nil) - - // Expect a rule to be added to FORWARD chain that jumps to FINCH-ISOLATE-CHAIN - mockIpt.EXPECT().Insert("filter", "FORWARD", 1, "-j", "FINCH-ISOLATE-CHAIN").Return(nil) - // Expect a RETURN rule to be appended in FINCH-ISOLATE-CHAIN - mockIpt.EXPECT().Append("filter", "FINCH-ISOLATE-CHAIN", "-j", "RETURN").Return(nil) - - // Expect the second Insert call to FINCH-ISOLATE-CHAIN for the DROP rule - mockIpt.EXPECT().Insert("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) - }) - - It("should create and set up the FINCH-ISOLATE-CHAIN", func() { - err := driver.DisableICC(bridgeIface, true) - Expect(err).ShouldNot(HaveOccurred()) - }) - }) - - Context("FINCH-ISOLATE-CHAIN exists", func() { - BeforeEach(func() { - mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil) - }) - - When("insert set to true", func() { - BeforeEach(func() { - // Expect the DROP rule to be inserted for packets from and to the same bridge - mockIpt.EXPECT().Insert("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) - }) - - It("should add the DROP rule to the FINCH-ISOLATE-CHAIN", func() { - err := driver.DisableICC(bridgeIface, true) - Expect(err).ShouldNot(HaveOccurred()) - }) - }) - - When("insert set to false", func() { - BeforeEach(func() { - // Expect the DROP rule to be removed for packets from and to the same bridge - mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) - }) - - It("should remove the DROP rule from the FINCH-ISOLATE-CHAIN", func() { - err := driver.DisableICC(bridgeIface, false) - Expect(err).ShouldNot(HaveOccurred()) - }) - }) - }) - - Context("when iptables returns an error", func() { - It("should return an error if ChainExists fails", func() { - mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, fmt.Errorf("iptables error")) - - err := driver.DisableICC(bridgeIface, true) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("failed to check if FINCH-ISOLATE-CHAIN chain exists")) - }) - - It("should return an error if NewChain fails", func() { - mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, nil) - - mockIpt.EXPECT().NewChain("filter", "FINCH-ISOLATE-CHAIN").Return(fmt.Errorf("iptables error")) - - err := driver.DisableICC(bridgeIface, true) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("failed to create FINCH-ISOLATE-CHAIN chain")) - }) - - It("should return an error if Insert fails while adding DROP rule", func() { - mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil) - - mockIpt.EXPECT().Insert("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(fmt.Errorf("iptables error")) - - err := driver.DisableICC(bridgeIface, true) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("failed to add DROP rule")) - }) - - It("should return an error if Delete fails while removing DROP rule", func() { - mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil) - - // Simulate an error while removing the DROP rule - mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(fmt.Errorf("iptables error")) - - err := driver.DisableICC(bridgeIface, false) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("failed to remove DROP rule")) - }) - }) -}) diff --git a/internal/service/network/create.go b/internal/service/network/create.go index 1933acfe..e16d08b9 100644 --- a/internal/service/network/create.go +++ b/internal/service/network/create.go @@ -11,28 +11,30 @@ import ( "github.com/containerd/nerdctl/pkg/netutil" "github.com/runfinch/finch-daemon/api/types" + "github.com/runfinch/finch-daemon/internal/service/network/driver" "github.com/runfinch/finch-daemon/pkg/errdefs" "github.com/runfinch/finch-daemon/pkg/utility/maputility" ) // Create implements the logic to turn a network create request to the back-end nerdctl create network calls. func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest) (types.NetworkCreateResponse, error) { - var bridgeDriver BridgeDriverOperations + var bridgeDriver driver.DriverHandler var err error createOptionsFrom := func(request types.NetworkCreateRequest) (netutil.CreateOptions, error) { // Default to "bridge" driver if request does not specify a driver - driver := request.Driver - if driver == "" { - driver = "bridge" + networkDriver := request.Driver + if networkDriver == "" { + networkDriver = "bridge" } options := netutil.CreateOptions{ Name: request.Name, - Driver: driver, + Driver: networkDriver, IPAMDriver: "default", IPAMOptions: request.IPAM.Options, Labels: maputility.Flatten(request.Labels, maputility.KeyEqualsValueFormat), + IPv6: request.EnableIPv6, } if request.IPAM.Driver != "" { @@ -53,9 +55,12 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest } // Handle driver-specific options - switch driver { + switch networkDriver { case "bridge": - bridgeDriver = NewBridgeDriver(s.netClient, s.logger) + bridgeDriver, err = driver.NewBridgeDriver(s.netClient, s.logger, options.IPv6) + if err != nil { + return options, err + } options, err = bridgeDriver.HandleCreateOptions(request, options) return options, err default: diff --git a/internal/service/network/create_test.go b/internal/service/network/create_test.go index 294fe6c1..f93fe505 100644 --- a/internal/service/network/create_test.go +++ b/internal/service/network/create_test.go @@ -16,6 +16,7 @@ import ( "github.com/runfinch/finch-daemon/api/handlers/network" "github.com/runfinch/finch-daemon/api/types" "github.com/runfinch/finch-daemon/internal/backend" + "github.com/runfinch/finch-daemon/internal/service/network/driver" "github.com/runfinch/finch-daemon/mocks/mocks_backend" "github.com/runfinch/finch-daemon/mocks/mocks_logger" "github.com/runfinch/finch-daemon/mocks/mocks_network" @@ -35,7 +36,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { ncNetClient *mocks_backend.MockNerdctlNetworkSvc logger *mocks_logger.Logger service network.Service - mockBridgeDriver *mocks_network.BridgeDriver + mockBridgeDriver *mocks_network.DriverHandler ) BeforeEach(func() { @@ -45,7 +46,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { ncNetClient = mocks_backend.NewMockNerdctlNetworkSvc(mockController) logger = mocks_logger.NewLogger(mockController) service = NewService(cdClient, ncNetClient, logger) - mockBridgeDriver = mocks_network.NewBridgeDriver(mockController) + mockBridgeDriver = mocks_network.NewDriverHandler(mockController) }) When("a create network call is successful", func() { @@ -349,7 +350,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { request := types.NewCreateNetworkRequest( networkName, types.WithOptions(map[string]string{ - "com.docker.network.bridge.enable_icc": "true", + driver.BridgeICCOption: "true", }), ) @@ -359,7 +360,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { nid := networkID ncNetClient.EXPECT().CreateNetwork(gomock.Any()).DoAndReturn(func(actual netutil.CreateOptions) (*netutil.NetworkConfig, error) { // Check if the label exists - checkLabel := "com.docker.network.bridge.enable_icc=true" + checkLabel := driver.BridgeICCOption + "=true" labelExists := false for _, label := range actual.Labels { if label == checkLabel { @@ -368,9 +369,6 @@ var _ = Describe("Network Service Create Network Implementation", func() { } } - // Expect that DisableICC is not called - mockBridgeDriver.EXPECT().DisableICC(gomock.Any(), gomock.Any()).Times(0) - Expect(labelExists).To(BeFalse(), fmt.Sprintf("Label '%s' should not exist in Labels", checkLabel)) return &netutil.NetworkConfig{NerdctlID: &nid}, nil @@ -392,7 +390,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { nid := networkID ncNetClient.EXPECT().CreateNetwork(gomock.Any()).DoAndReturn(func(actual netutil.CreateOptions) (*netutil.NetworkConfig, error) { // Check if the label exists - expectedLabel := "com.docker.network.bridge.enable_icc=true" + expectedLabel := driver.BridgeICCOption + "=true" labelExists := false for _, label := range actual.Labels { if label == expectedLabel { @@ -417,7 +415,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { request := types.NewCreateNetworkRequest( networkName, types.WithOptions(map[string]string{ - BridgeICCOption: "false", + driver.BridgeICCOption: "false", }), ) @@ -427,7 +425,7 @@ var _ = Describe("Network Service Create Network Implementation", func() { nid := networkID ncNetClient.EXPECT().CreateNetwork(gomock.Any()).DoAndReturn(func(actual netutil.CreateOptions) (*netutil.NetworkConfig, error) { // Check if the label exists - expectedLabel := FinchICCLabel + "=false" + expectedLabel := driver.FinchICCLabelIPv4 + "=false" labelExists := false for _, label := range actual.Labels { if label == expectedLabel { @@ -441,27 +439,24 @@ var _ = Describe("Network Service Create Network Implementation", func() { return &netutil.NetworkConfig{NerdctlID: &nid}, nil }) - originalDriverFunc := NewBridgeDriver - NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logger) BridgeDriverOperations { - return mockBridgeDriver + originalDriverFunc := driver.NewBridgeDriver + driver.NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logger, isIPv6 bool) (driver.DriverHandler, error) { + return mockBridgeDriver, nil } - defer func() { NewBridgeDriver = originalDriverFunc }() + defer func() { driver.NewBridgeDriver = originalDriverFunc }() // Set up expectations for mockBridgeDriver mockBridgeDriver.EXPECT().HandleCreateOptions(gomock.Any(), gomock.Any()).DoAndReturn( func(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) { // Mock the behavior for BridgeICCOption set to false // Remove the option from the options map - fmt.Println("HandleCreateOptions called with options:", options) - delete(options.Options, BridgeICCOption) - options.Labels = append(options.Labels, FinchICCLabel+"=false") + + delete(options.Options, driver.BridgeICCOption) + options.Labels = append(options.Labels, driver.FinchICCLabelIPv4+"=false") return options, nil }).AnyTimes() mockBridgeDriver.EXPECT().HandlePostCreate(gomock.Any()).Return("", nil).AnyTimes() - mockBridgeDriver.EXPECT().ICCDisabled().DoAndReturn(func() bool { - return true - }).AnyTimes() response, err := service.Create(ctx, *request) Expect(err).ShouldNot(HaveOccurred()) diff --git a/internal/service/network/driver/bridge.go b/internal/service/network/driver/bridge.go new file mode 100644 index 00000000..136b2d8a --- /dev/null +++ b/internal/service/network/driver/bridge.go @@ -0,0 +1,303 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package driver + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strconv" + + "github.com/containerd/nerdctl/pkg/lockutil" + "github.com/containerd/nerdctl/pkg/netutil" + "github.com/runfinch/finch-daemon/api/types" + "github.com/runfinch/finch-daemon/internal/backend" + "github.com/runfinch/finch-daemon/pkg/flog" +) + +const ( + FinchICCLabelIPv4 = "finch.network.bridge.enable_icc.ipv4" + FinchICCLabelIPv6 = "finch.network.bridge.enable_icc.ipv6" + BridgeICCOption = "com.docker.network.bridge.enable_icc" + BridgeHostBindingIpv4Option = "com.docker.network.bridge.host_binding_ipv4" + BridgeNameOption = "com.docker.network.bridge.name" +) + +type bridgeDriver struct { + bridgeName string + disableICC bool + netClient backend.NerdctlNetworkSvc + logger flog.Logger + IPv6 bool +} + +var _ DriverHandler = (*bridgeDriver)(nil) + +var NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logger, IPv6 bool) (DriverHandler, error) { + return &bridgeDriver{ + netClient: netClient, + logger: logger, + IPv6: IPv6, + }, nil +} + +// handleBridgeDriverOptions filters unsupported options for the bridge driver. +func (bd *bridgeDriver) HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) { + // enable_icc, host_binding_ipv4, and bridge name network options are not supported by nerdctl. + // So we process these options here and filter them out from the network create request to nerdctl. + processUnsupportedOptions := func(original map[string]string) map[string]string { + opts := map[string]string{} + for k, v := range original { + switch k { + case BridgeHostBindingIpv4Option: + if v != "0.0.0.0" { + bd.logger.Warnf("network option com.docker.network.bridge.host_binding_ipv4 is set to %s, but it must be 0.0.0.0", v) + } + case BridgeICCOption: + iccOption, err := strconv.ParseBool(v) + if err != nil { + bd.logger.Warnf("invalid value for com.docker.network.bridge.enable_icc: %s", v) + } + if !iccOption { + bd.disableICC = true + } + + case BridgeNameOption: + bd.bridgeName = v + default: + opts[k] = v + } + } + return opts + } + + options.Options = processUnsupportedOptions(request.Options) + + if bd.disableICC { + finchICCLabel := FinchICCLabelIPv4 + if bd.IPv6 { + finchICCLabel = FinchICCLabelIPv6 + } + options.Labels = append(options.Labels, finchICCLabel+"=false") + } + return options, nil +} + +func (bd *bridgeDriver) HandlePostCreate(net *netutil.NetworkConfig) (string, error) { + // Handle bridge driver post create actions + var warning string + var err error + bridgeName := bd.bridgeName + if bridgeName != "" { + // Since nerdctl currently does not support custom bridge names, + // we explicitly override bridge name in the conflist file for the network that was just created + if err = bd.setBridgeName(net, bridgeName); err != nil { + warning = fmt.Sprintf("Failed to set network bridge name %s: %s", bridgeName, err) + } + } + + if bd.disableICC { + // Handle "enable_icc=false" option if set (bd.disableICC is true) + // By default, CNI allows connectivity between containers attached to the same bridge. + // If "com.docker.network.bridge.enable_icc" option is explicitly set to false, + // we disable inter-container connectivity by applying iptable rules + // If "com.docker.network.bridge.enable_icc=true" is set, it is considered a noop + if bridgeName == "" { + bridgeName, err = bd.getBridgeName(net) + if err != nil { + return "", fmt.Errorf("failed to get bridge name to enable inter-container connectivity: %w ", err) + } + } + + err = bd.addICCDropRule(bridgeName) + if err != nil { + return "", fmt.Errorf("failed to disable inter-container connectivity: %w", err) + } + } + + return warning, nil +} + +func (bd *bridgeDriver) HandleRemove(net *netutil.NetworkConfig) error { + bridgeName, err := bd.getBridgeName(net) + if err != nil { + return fmt.Errorf("failed to get bridge name to remove inter-container connectivity: %w ", err) + } + err = bd.removeICCDropRule(bridgeName) + if err != nil { + return fmt.Errorf("failed to remove iptables DROP rule : %w", err) + } + return nil +} + +// setBridgeName will override the bridge name in an existing CNI config file for a network. +func (bd *bridgeDriver) setBridgeName(net *netutil.NetworkConfig, bridgeName string) error { + return lockutil.WithDirLock(bd.netClient.NetconfPath(), func() error { + // first, make sure that the bridge name is not used by any of the existing bridge networks + bridgeNet, err := bd.getNetworkByBridgeName(bridgeName) + if err != nil { + return err + } + if bridgeNet != nil { + return fmt.Errorf("bridge name %s already in use by network %s", bridgeName, bridgeNet.Name) + } + + configFilename := bd.getConfigPathForNetworkName(net.Name) + netMap, bridgePlugin, err := bd.parseBridgeConfig(configFilename) + if err != nil { + return err + } + + // Update the bridge name in the bridge plugin + bridgePlugin["bridge"] = bridgeName + + // Update the plugins in the full config + plugins := netMap["plugins"].([]interface{}) + for i, plugin := range plugins { + if p, ok := plugin.(map[string]interface{}); ok && p["type"] == "bridge" { + plugins[i] = bridgePlugin + break + } + } + + // Write the updated config back to the file + data, err := json.MarshalIndent(netMap, "", " ") + if err != nil { + return err + } + return os.WriteFile(configFilename, data, 0o644) + }) +} + +func (bd *bridgeDriver) getBridgeName(net *netutil.NetworkConfig) (string, error) { + var bridgeName string + err := lockutil.WithDirLock(bd.netClient.NetconfPath(), func() error { + configFilename := bd.getConfigPathForNetworkName(net.Name) + _, bridgePlugin, err := bd.parseBridgeConfig(configFilename) + if err != nil { + return err + } + + bridge, ok := bridgePlugin["bridge"].(string) + if !ok { + return fmt.Errorf("bridge name in config file %s is not a string", configFilename) + } + bridgeName = bridge + return nil + }) + + if err != nil { + return "", err + } + + return bridgeName, nil +} + +func (bd *bridgeDriver) parseBridgeConfig(configFilename string) (map[string]interface{}, map[string]interface{}, error) { + configFile, err := os.Open(configFilename) + if err != nil { + return nil, nil, err + } + defer configFile.Close() + + var netJSON interface{} + if err = json.NewDecoder(configFile).Decode(&netJSON); err != nil { + return nil, nil, err + } + + netMap, ok := netJSON.(map[string]interface{}) + if !ok { + return nil, nil, fmt.Errorf("network config file %s is not a valid map", configFilename) + } + + plugins, ok := netMap["plugins"] + if !ok { + return nil, nil, fmt.Errorf("could not find plugins in network config file %s", configFilename) + } + + pluginsMap, ok := plugins.([]interface{}) + if !ok { + return nil, nil, fmt.Errorf("could not parse plugins in network config file %s", configFilename) + } + + for _, plugin := range pluginsMap { + pluginMap, ok := plugin.(map[string]interface{}) + if !ok { + continue + } + if pluginMap["type"] == "bridge" { + return netMap, pluginMap, nil + } + } + + return nil, nil, fmt.Errorf("bridge plugin not found in network config file %s", configFilename) +} + +func (bd *bridgeDriver) getNetworkByBridgeName(bridgeName string) (*netutil.NetworkConfig, error) { + networks, err := bd.netClient.FilterNetworks(func(*netutil.NetworkConfig) bool { + return true + }) + if err != nil { + return nil, err + } + + var bridgePlugin struct { + Type string `json:"type"` + Bridge string `json:"bridge"` + } + + for _, network := range networks { + for _, plugin := range network.Plugins { + if plugin.Network.Type != "bridge" { + continue + } + + if err = json.Unmarshal(plugin.Bytes, &bridgePlugin); err != nil { + continue + } + if bridgePlugin.Bridge == bridgeName { + return network, nil + } + } + } + return nil, nil +} + +func (bd *bridgeDriver) addICCDropRule(bridgeIface string) error { + bd.logger.Debugf("adding ICC drop rule for bridge: %s", bridgeIface) + iccDropRule := []string{"-i", bridgeIface, "-o", bridgeIface, "-j", "DROP"} + ipc, err := newIptablesCommand(bd.IPv6) + if err != nil { + return err + } + + err = ipc.AddRule(iccDropRule...) + if err != nil { + return fmt.Errorf("failed to add iptables rule to drop ICC: %v", err) + } + + return nil +} + +func (bd *bridgeDriver) removeICCDropRule(bridgeIface string) error { + bd.logger.Debugf("removing ICC drop rule for bridge: %s", bridgeIface) + iccDropRule := []string{"-i", bridgeIface, "-o", bridgeIface, "-j", "DROP"} + ipc, err := newIptablesCommand(bd.IPv6) + if err != nil { + return err + } + + err = ipc.DelRule(iccDropRule...) + if err != nil { + return fmt.Errorf("failed to remove iptables rules to drop ICC: %v", err) + } + + return nil +} + +// From https://github.com/containerd/nerdctl/blob/v1.5.0/pkg/netutil/netutil.go#L186-L188 +func (bd *bridgeDriver) getConfigPathForNetworkName(netName string) string { + return filepath.Join(bd.netClient.NetconfPath(), "nerdctl-"+netName+".conflist") +} diff --git a/internal/service/network/driver/bridge_test.go b/internal/service/network/driver/bridge_test.go new file mode 100644 index 00000000..8d73161d --- /dev/null +++ b/internal/service/network/driver/bridge_test.go @@ -0,0 +1,129 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package driver + +import ( + "fmt" + + "github.com/coreos/go-iptables/iptables" + "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/runfinch/finch-daemon/mocks/mocks_logger" + "github.com/runfinch/finch-daemon/mocks/mocks_network" +) + +var _ = Describe("bridgeDriver Disable ICC", func() { + var ( + mockController *gomock.Controller + logger *mocks_logger.Logger + mockIpt *mocks_network.MockIPTablesWrapper + driver *bridgeDriver + bridgeIface string + ) + + BeforeEach(func() { + mockController = gomock.NewController(GinkgoT()) + logger = mocks_logger.NewLogger(mockController) + mockIpt = mocks_network.NewMockIPTablesWrapper(mockController) + driver = &bridgeDriver{logger: logger} + bridgeIface = "br-mock" + newIptablesCommand = func(_ bool) (*iptablesCommand, error) { + iptCommand := &iptablesCommand{ + protos: make(map[iptables.Protocol]IPTablesWrapper), + } + iptCommand.protos[iptables.ProtocolIPv4] = mockIpt + return iptCommand, nil + } + logger.EXPECT().Debugf(gomock.Any(), gomock.Any()).AnyTimes() + }) + + Context("chain does not exist", func() { + It("should create and set up the FINCH-ISOLATE-CHAIN", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, nil).Times(1) + mockIpt.EXPECT().NewChain("filter", "FINCH-ISOLATE-CHAIN").Return(nil) + mockIpt.EXPECT().InsertUnique("filter", "FORWARD", 1, "-j", "FINCH-ISOLATE-CHAIN").Return(nil) + mockIpt.EXPECT().AppendUnique("filter", "FINCH-ISOLATE-CHAIN", "-j", "RETURN").Return(nil) + + // Expect the second Insert call to FINCH-ISOLATE-CHAIN for the DROP rule + mockIpt.EXPECT().InsertUnique("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + err := driver.addICCDropRule(bridgeIface) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + + Context("chain setup fails", func() { + When("we fail to check if chain exists", func() { + It("should return an error and cleanup", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, fmt.Errorf("iptables failed")) + + ////expect cleanup to be called + mockIpt.EXPECT().DeleteChain("filter", "FINCH-ISOLATE-CHAIN").Return(nil) + + err := driver.addICCDropRule(bridgeIface) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to check if iptables FINCH-ISOLATE-CHAIN exists")) + }) + }) + When("a new chain creation fails", func() { + It("should return an error and cleanup", func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(false, nil) + mockIpt.EXPECT().NewChain("filter", "FINCH-ISOLATE-CHAIN").Return(fmt.Errorf("iptables failed")) + + //expect cleanup to be called + mockIpt.EXPECT().DeleteChain("filter", "FINCH-ISOLATE-CHAIN").Return(nil) + + err := driver.addICCDropRule(bridgeIface) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to create FINCH-ISOLATE-CHAIN chain")) + }) + }) + }) + + Context("chain already exists", func() { + BeforeEach(func() { + mockIpt.EXPECT().ChainExists("filter", "FINCH-ISOLATE-CHAIN").Return(true, nil).AnyTimes() + mockIpt.EXPECT().InsertUnique("filter", "FORWARD", 1, "-j", "FINCH-ISOLATE-CHAIN").Return(nil).AnyTimes() + mockIpt.EXPECT().AppendUnique("filter", "FINCH-ISOLATE-CHAIN", "-j", "RETURN").Return(nil).AnyTimes() + }) + When("addICCDropRule is successful", func() { + It("should add the DROP rule to the FINCH-ISOLATE-CHAIN", func() { + // Expect the DROP rule to be inserted for packets from and to the same bridge + mockIpt.EXPECT().InsertUnique("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + err := driver.addICCDropRule(bridgeIface) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + + When("removeICCDropRule is successful", func() { + It("should remove the DROP rule from the FINCH-ISOLATE-CHAIN", func() { + mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + err := driver.removeICCDropRule(bridgeIface) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + When("addICCDropRule fails", func() { + It("should return an error", func() { + mockIpt.EXPECT().InsertUnique("filter", "FINCH-ISOLATE-CHAIN", 1, "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(fmt.Errorf("iptables error")) + + // //expect cleanup to be called + mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(nil) + + err := driver.addICCDropRule(bridgeIface) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to add iptables rule to drop ICC")) + }) + }) + + When("removeICCDropRule fails", func() { + It("should return an error ", func() { + mockIpt.EXPECT().DeleteIfExists("filter", "FINCH-ISOLATE-CHAIN", "-i", bridgeIface, "-o", bridgeIface, "-j", "DROP").Return(fmt.Errorf("iptables error")) + + err := driver.removeICCDropRule(bridgeIface) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to remove iptables rules to drop ICC")) + }) + }) + }) +}) diff --git a/internal/service/network/driver/driver.go b/internal/service/network/driver/driver.go new file mode 100644 index 00000000..d55f22b9 --- /dev/null +++ b/internal/service/network/driver/driver.go @@ -0,0 +1,16 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package driver + +import ( + "github.com/containerd/nerdctl/pkg/netutil" + "github.com/runfinch/finch-daemon/api/types" +) + +//go:generate mockgen --destination=../../../../mocks/mocks_network/driver.go -package=mocks_network -mock_names DriverHandler=DriverHandler . DriverHandler +type DriverHandler interface { + HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) + HandlePostCreate(net *netutil.NetworkConfig) (string, error) + HandleRemove(net *netutil.NetworkConfig) error +} diff --git a/internal/service/network/driver/driver_test.go b/internal/service/network/driver/driver_test.go new file mode 100644 index 00000000..8c9ac13c --- /dev/null +++ b/internal/service/network/driver/driver_test.go @@ -0,0 +1,17 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package driver + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// TestNetworkService is the entry point of the network service package's unit tests using ginkgo. +func TestNetworkService(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "UnitTests - Network Driver Operations") +} diff --git a/internal/service/network/driver/iptables.go b/internal/service/network/driver/iptables.go new file mode 100644 index 00000000..2987d8fb --- /dev/null +++ b/internal/service/network/driver/iptables.go @@ -0,0 +1,146 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package driver + +import ( + "errors" + "fmt" + + "github.com/coreos/go-iptables/iptables" +) + +const ( + statusChainExists = 1 + + FilterTableName = "filter" + ForwardChainName = "FORWARD" + FinchIsolateChain = "FINCH-ISOLATE-CHAIN" +) + +// IPTablesWrapper is an interface that wraps the methods of iptables.IPTables +// to help with mock +// +//go:generate mockgen --destination=../../../../mocks/mocks_network/iptables.go -package=mocks_network . IPTablesWrapper +type IPTablesWrapper interface { + ChainExists(table, chain string) (bool, error) + NewChain(table, chain string) error + InsertUnique(table, chain string, pos int, rulespec ...string) error + AppendUnique(table, chain string, rulespec ...string) error + DeleteIfExists(table, chain string, rulespec ...string) error + DeleteChain(table, chain string) error +} + +type iptablesCommand struct { + protos map[iptables.Protocol]IPTablesWrapper +} + +var newIptablesCommand = func(ipv6 bool) (*iptablesCommand, error) { + iptCommand := &iptablesCommand{ + protos: make(map[iptables.Protocol]IPTablesWrapper), + } + + protocols := []iptables.Protocol{iptables.ProtocolIPv4} + if ipv6 { + protocols = append(protocols, iptables.ProtocolIPv6) + } + + for _, proto := range protocols { + ipt, err := iptables.NewWithProtocol(proto) + if err != nil { + return nil, fmt.Errorf("could not initialize iptables protocol %v: %w", proto, err) + } + iptCommand.protos[proto] = ipt + } + + return iptCommand, nil +} + +// EnsureChain creates a new iptables chain if one doesn't exist already. +func (ipc *iptablesCommand) ensureChain(ipt IPTablesWrapper, table, chain string) error { + if ipt == nil { + return errors.New("failed to ensure iptable chain: IPTables was nil") + } + exists, err := ipt.ChainExists(table, chain) + if err != nil { + return fmt.Errorf("failed to check if iptables %s exists: %v", chain, err) + } + if !exists { + err = ipt.NewChain(table, chain) + if err != nil { + eerr, eok := err.(*iptables.Error) + if !eok { + // type assertion failed, return the original error + return fmt.Errorf("failed to create %s chain: %w", chain, err) + } + + // ignore if the chain was created in the meantime + if eerr.ExitStatus() != statusChainExists { + return fmt.Errorf("failed to create %s chain: %w", chain, err) + } + } + } + return nil +} + +func (ipc *iptablesCommand) cleanupChain(ipt IPTablesWrapper, tableName string, chainName string) { + // attempt to delete the chain if it exists and is empty + ipt.DeleteChain(tableName, chainName) +} + +func (ipc *iptablesCommand) setupChains(ipt IPTablesWrapper) error { + if err := ipc.ensureChain(ipt, FilterTableName, FinchIsolateChain); err != nil { + ipc.cleanupChain(ipt, FilterTableName, FinchIsolateChain) + return err + } + // Add a rule to the FORWARD chain that jumps to the FINCH-ISOLATE-CHAIN for all packets + jumpRule := []string{"-j", FinchIsolateChain} + if err := ipt.InsertUnique(FilterTableName, ForwardChainName, 1, jumpRule...); err != nil { + return fmt.Errorf("failed to add jump rule to FINCH-ISOLATE-CHAIN chain: %v", err) + } + // In the FINCH-ISOLATE-CHAIN, add a rule to return to the FORWARD chain when no match + returnRule := []string{"-j", "RETURN"} + if err := ipt.AppendUnique(FilterTableName, FinchIsolateChain, returnRule...); err != nil { + return fmt.Errorf("failed to add RETURN rule in FINCH-ISOLATE-CHAIN chain: %v", err) + } + return nil +} + +func (ipc *iptablesCommand) cleanupRules(ipt IPTablesWrapper, tableName string, chainName string, rulespec ...string) { + ipt.DeleteIfExists(tableName, chainName, rulespec...) +} + +func (ipc *iptablesCommand) addRule(ipt IPTablesWrapper, rulespec ...string) error { + if err := ipc.setupChains(ipt); err != nil { + return err + } + + if err := ipt.InsertUnique(FilterTableName, FinchIsolateChain, 1, rulespec...); err != nil { + ipc.cleanupRules(ipt, FilterTableName, FinchIsolateChain, rulespec...) + return fmt.Errorf("failed to add iptables rule: %w", err) + } + + return nil +} + +func (ipc *iptablesCommand) AddRule(rulespec ...string) error { + for _, ipt := range ipc.protos { + if err := ipc.addRule(ipt, rulespec...); err != nil { + return err + } + } + return nil +} + +func (ipc *iptablesCommand) delRule(ipt IPTablesWrapper, rulespec ...string) error { + return ipt.DeleteIfExists(FilterTableName, FinchIsolateChain, rulespec...) +} + +func (ipc *iptablesCommand) DelRule(rulespec ...string) error { + for _, ipt := range ipc.protos { + if err := ipc.delRule(ipt, rulespec...); err != nil { + return err + } + } + return nil +} diff --git a/internal/service/network/remove.go b/internal/service/network/remove.go index e9c3a6cf..2d557b7b 100644 --- a/internal/service/network/remove.go +++ b/internal/service/network/remove.go @@ -8,6 +8,7 @@ import ( "fmt" "github.com/containerd/nerdctl/pkg/netutil" + "github.com/runfinch/finch-daemon/internal/service/network/driver" "github.com/runfinch/finch-daemon/pkg/errdefs" ) @@ -46,31 +47,35 @@ func (s *service) handleNetworkLabels(net *netutil.NetworkConfig) error { for key, value := range *net.NerdctlLabels { switch key { - case FinchICCLabel: - if err := s.handleEnableICCOption(net, value); err != nil { - return fmt.Errorf("error handling %s label: %w", BridgeICCOption, err) + case driver.FinchICCLabelIPv4: + if err := s.handleICCLabel(net, value, false); err != nil { + return fmt.Errorf("error handling IPv4 ICC label: %w", err) + } + case driver.FinchICCLabelIPv6: + if err := s.handleICCLabel(net, value, true); err != nil { + return fmt.Errorf("error handling IPv6 ICC label: %w", err) } } } return nil } -func (s *service) handleEnableICCOption(net *netutil.NetworkConfig, value string) error { +func (s *service) handleICCLabel(net *netutil.NetworkConfig, value string, isIPv6 bool) error { if value != "false" { // for some reason the label value got modified. // we will still try to remove the iptable rules. // iptable.DeleteIfExists is used to ignore non-existent errors - s.logger.Warnf("unexpected value for %s label: %s", BridgeICCOption, value) + s.logger.Warnf("unexpected value for ICC label: %s", value) } - // Remove iptable rules set for disabling ICC for the network bridge - bridgeDriver := NewBridgeDriver(s.netClient, s.logger) - bridgeName, err := bridgeDriver.GetBridgeName(net) + + bridgeDriver, err := driver.NewBridgeDriver(s.netClient, s.logger, isIPv6) if err != nil { - return fmt.Errorf("unable to get bridge name: %w", err) + return fmt.Errorf("unable to create bridge driver: %w", err) } - err = bridgeDriver.DisableICC(bridgeName, false) - if err != nil { - return fmt.Errorf("unable to remove ICC disable rule: %w", err) + + if err := bridgeDriver.HandleRemove(net); err != nil { + return fmt.Errorf("error handling ICC label: %w", err) } + return nil } diff --git a/mocks/mocks_network/bridge_driver.go b/mocks/mocks_network/bridge_driver.go deleted file mode 100644 index ad2f551f..00000000 --- a/mocks/mocks_network/bridge_driver.go +++ /dev/null @@ -1,150 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/runfinch/finch-daemon/internal/service/network (interfaces: BridgeDriverOperations) - -// Package mocks_network is a generated GoMock package. -package mocks_network - -import ( - reflect "reflect" - - netutil "github.com/containerd/nerdctl/pkg/netutil" - gomock "github.com/golang/mock/gomock" - types "github.com/runfinch/finch-daemon/api/types" -) - -// BridgeDriver is a mock of BridgeDriverOperations interface. -type BridgeDriver struct { - ctrl *gomock.Controller - recorder *BridgeDriverMockRecorder -} - -// BridgeDriverMockRecorder is the mock recorder for BridgeDriver. -type BridgeDriverMockRecorder struct { - mock *BridgeDriver -} - -// NewBridgeDriver creates a new mock instance. -func NewBridgeDriver(ctrl *gomock.Controller) *BridgeDriver { - mock := &BridgeDriver{ctrl: ctrl} - mock.recorder = &BridgeDriverMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *BridgeDriver) EXPECT() *BridgeDriverMockRecorder { - return m.recorder -} - -// DisableICC mocks base method. -func (m *BridgeDriver) DisableICC(arg0 string, arg1 bool) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DisableICC", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// DisableICC indicates an expected call of DisableICC. -func (mr *BridgeDriverMockRecorder) DisableICC(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DisableICC", reflect.TypeOf((*BridgeDriver)(nil).DisableICC), arg0, arg1) -} - -// GetBridgeName mocks base method. -func (m *BridgeDriver) GetBridgeName(arg0 *netutil.NetworkConfig) (string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetBridgeName", arg0) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetBridgeName indicates an expected call of GetBridgeName. -func (mr *BridgeDriverMockRecorder) GetBridgeName(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBridgeName", reflect.TypeOf((*BridgeDriver)(nil).GetBridgeName), arg0) -} - -// GetNetworkByBridgeName mocks base method. -func (m *BridgeDriver) GetNetworkByBridgeName(arg0 string) (*netutil.NetworkConfig, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetNetworkByBridgeName", arg0) - ret0, _ := ret[0].(*netutil.NetworkConfig) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetNetworkByBridgeName indicates an expected call of GetNetworkByBridgeName. -func (mr *BridgeDriverMockRecorder) GetNetworkByBridgeName(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkByBridgeName", reflect.TypeOf((*BridgeDriver)(nil).GetNetworkByBridgeName), arg0) -} - -// HandleCreateOptions mocks base method. -func (m *BridgeDriver) HandleCreateOptions(arg0 types.NetworkCreateRequest, arg1 netutil.CreateOptions) (netutil.CreateOptions, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "HandleCreateOptions", arg0, arg1) - ret0, _ := ret[0].(netutil.CreateOptions) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// HandleCreateOptions indicates an expected call of HandleCreateOptions. -func (mr *BridgeDriverMockRecorder) HandleCreateOptions(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandleCreateOptions", reflect.TypeOf((*BridgeDriver)(nil).HandleCreateOptions), arg0, arg1) -} - -// HandlePostCreate mocks base method. -func (m *BridgeDriver) HandlePostCreate(arg0 *netutil.NetworkConfig) (string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "HandlePostCreate", arg0) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// HandlePostCreate indicates an expected call of HandlePostCreate. -func (mr *BridgeDriverMockRecorder) HandlePostCreate(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandlePostCreate", reflect.TypeOf((*BridgeDriver)(nil).HandlePostCreate), arg0) -} - -// ICCDisabled mocks base method. -func (m *BridgeDriver) ICCDisabled() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ICCDisabled") - ret0, _ := ret[0].(bool) - return ret0 -} - -// ICCDisabled indicates an expected call of ICCDisabled. -func (mr *BridgeDriverMockRecorder) ICCDisabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ICCDisabled", reflect.TypeOf((*BridgeDriver)(nil).ICCDisabled)) -} - -// SetBridgeName mocks base method. -func (m *BridgeDriver) SetBridgeName(arg0 *netutil.NetworkConfig, arg1 string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetBridgeName", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// SetBridgeName indicates an expected call of SetBridgeName. -func (mr *BridgeDriverMockRecorder) SetBridgeName(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetBridgeName", reflect.TypeOf((*BridgeDriver)(nil).SetBridgeName), arg0, arg1) -} - -// SetICCDisabled mocks base method. -func (m *BridgeDriver) SetICCDisabled() { - m.ctrl.T.Helper() - m.ctrl.Call(m, "SetICCDisabled") -} - -// SetICCDisabled indicates an expected call of SetICCDisabled. -func (mr *BridgeDriverMockRecorder) SetICCDisabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetICCDisabled", reflect.TypeOf((*BridgeDriver)(nil).SetICCDisabled)) -} diff --git a/mocks/mocks_network/driver.go b/mocks/mocks_network/driver.go new file mode 100644 index 00000000..21d6846f --- /dev/null +++ b/mocks/mocks_network/driver.go @@ -0,0 +1,80 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/runfinch/finch-daemon/internal/service/network/driver (interfaces: DriverHandler) + +// Package mocks_network is a generated GoMock package. +package mocks_network + +import ( + reflect "reflect" + + netutil "github.com/containerd/nerdctl/pkg/netutil" + gomock "github.com/golang/mock/gomock" + types "github.com/runfinch/finch-daemon/api/types" +) + +// DriverHandler is a mock of DriverHandler interface. +type DriverHandler struct { + ctrl *gomock.Controller + recorder *DriverHandlerMockRecorder +} + +// DriverHandlerMockRecorder is the mock recorder for DriverHandler. +type DriverHandlerMockRecorder struct { + mock *DriverHandler +} + +// NewDriverHandler creates a new mock instance. +func NewDriverHandler(ctrl *gomock.Controller) *DriverHandler { + mock := &DriverHandler{ctrl: ctrl} + mock.recorder = &DriverHandlerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *DriverHandler) EXPECT() *DriverHandlerMockRecorder { + return m.recorder +} + +// HandleCreateOptions mocks base method. +func (m *DriverHandler) HandleCreateOptions(arg0 types.NetworkCreateRequest, arg1 netutil.CreateOptions) (netutil.CreateOptions, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HandleCreateOptions", arg0, arg1) + ret0, _ := ret[0].(netutil.CreateOptions) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HandleCreateOptions indicates an expected call of HandleCreateOptions. +func (mr *DriverHandlerMockRecorder) HandleCreateOptions(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandleCreateOptions", reflect.TypeOf((*DriverHandler)(nil).HandleCreateOptions), arg0, arg1) +} + +// HandlePostCreate mocks base method. +func (m *DriverHandler) HandlePostCreate(arg0 *netutil.NetworkConfig) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HandlePostCreate", arg0) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HandlePostCreate indicates an expected call of HandlePostCreate. +func (mr *DriverHandlerMockRecorder) HandlePostCreate(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandlePostCreate", reflect.TypeOf((*DriverHandler)(nil).HandlePostCreate), arg0) +} + +// HandleRemove mocks base method. +func (m *DriverHandler) HandleRemove(arg0 *netutil.NetworkConfig) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HandleRemove", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// HandleRemove indicates an expected call of HandleRemove. +func (mr *DriverHandlerMockRecorder) HandleRemove(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandleRemove", reflect.TypeOf((*DriverHandler)(nil).HandleRemove), arg0) +} diff --git a/mocks/mocks_network/iptables.go b/mocks/mocks_network/iptables.go index 87a922da..94dc213b 100644 --- a/mocks/mocks_network/iptables.go +++ b/mocks/mocks_network/iptables.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/runfinch/finch-daemon/internal/service/network (interfaces: IPTablesWrapper) +// Source: github.com/runfinch/finch-daemon/internal/service/network/driver (interfaces: IPTablesWrapper) // Package mocks_network is a generated GoMock package. package mocks_network @@ -33,23 +33,23 @@ func (m *MockIPTablesWrapper) EXPECT() *MockIPTablesWrapperMockRecorder { return m.recorder } -// Append mocks base method. -func (m *MockIPTablesWrapper) Append(arg0, arg1 string, arg2 ...string) error { +// AppendUnique mocks base method. +func (m *MockIPTablesWrapper) AppendUnique(arg0, arg1 string, arg2 ...string) error { m.ctrl.T.Helper() varargs := []interface{}{arg0, arg1} for _, a := range arg2 { varargs = append(varargs, a) } - ret := m.ctrl.Call(m, "Append", varargs...) + ret := m.ctrl.Call(m, "AppendUnique", varargs...) ret0, _ := ret[0].(error) return ret0 } -// Append indicates an expected call of Append. -func (mr *MockIPTablesWrapperMockRecorder) Append(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { +// AppendUnique indicates an expected call of AppendUnique. +func (mr *MockIPTablesWrapperMockRecorder) AppendUnique(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() varargs := append([]interface{}{arg0, arg1}, arg2...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Append", reflect.TypeOf((*MockIPTablesWrapper)(nil).Append), varargs...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AppendUnique", reflect.TypeOf((*MockIPTablesWrapper)(nil).AppendUnique), varargs...) } // ChainExists mocks base method. @@ -67,6 +67,20 @@ func (mr *MockIPTablesWrapperMockRecorder) ChainExists(arg0, arg1 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ChainExists", reflect.TypeOf((*MockIPTablesWrapper)(nil).ChainExists), arg0, arg1) } +// DeleteChain mocks base method. +func (m *MockIPTablesWrapper) DeleteChain(arg0, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteChain", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteChain indicates an expected call of DeleteChain. +func (mr *MockIPTablesWrapperMockRecorder) DeleteChain(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteChain", reflect.TypeOf((*MockIPTablesWrapper)(nil).DeleteChain), arg0, arg1) +} + // DeleteIfExists mocks base method. func (m *MockIPTablesWrapper) DeleteIfExists(arg0, arg1 string, arg2 ...string) error { m.ctrl.T.Helper() @@ -86,23 +100,23 @@ func (mr *MockIPTablesWrapperMockRecorder) DeleteIfExists(arg0, arg1 interface{} return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteIfExists", reflect.TypeOf((*MockIPTablesWrapper)(nil).DeleteIfExists), varargs...) } -// Insert mocks base method. -func (m *MockIPTablesWrapper) Insert(arg0, arg1 string, arg2 int, arg3 ...string) error { +// InsertUnique mocks base method. +func (m *MockIPTablesWrapper) InsertUnique(arg0, arg1 string, arg2 int, arg3 ...string) error { m.ctrl.T.Helper() varargs := []interface{}{arg0, arg1, arg2} for _, a := range arg3 { varargs = append(varargs, a) } - ret := m.ctrl.Call(m, "Insert", varargs...) + ret := m.ctrl.Call(m, "InsertUnique", varargs...) ret0, _ := ret[0].(error) return ret0 } -// Insert indicates an expected call of Insert. -func (mr *MockIPTablesWrapperMockRecorder) Insert(arg0, arg1, arg2 interface{}, arg3 ...interface{}) *gomock.Call { +// InsertUnique indicates an expected call of InsertUnique. +func (mr *MockIPTablesWrapperMockRecorder) InsertUnique(arg0, arg1, arg2 interface{}, arg3 ...interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() varargs := append([]interface{}{arg0, arg1, arg2}, arg3...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Insert", reflect.TypeOf((*MockIPTablesWrapper)(nil).Insert), varargs...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertUnique", reflect.TypeOf((*MockIPTablesWrapper)(nil).InsertUnique), varargs...) } // NewChain mocks base method. From 8f66a5c4b9b2c33742f17b68c46278a352ba85b8 Mon Sep 17 00:00:00 2001 From: Swagat Bora Date: Thu, 31 Oct 2024 23:21:59 +0000 Subject: [PATCH 3/6] add cleanup method if create fails during post process Signed-off-by: Swagat Bora --- internal/service/network/create.go | 16 ++++++++++++++++ internal/service/network/driver/bridge.go | 12 +++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/internal/service/network/create.go b/internal/service/network/create.go index e16d08b9..d1bfc3cb 100644 --- a/internal/service/network/create.go +++ b/internal/service/network/create.go @@ -100,6 +100,22 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest return types.NetworkCreateResponse{}, errNetworkIDNotFound } + // Add cleanup func to remove the network if an error is encountered during post processing + cleanup := func(ctx context.Context, name string) { + if cleanupErr := s.Remove(ctx, name); cleanupErr != nil { + // ignore if the network does not exist + if !errdefs.IsNotFound(cleanupErr) { + s.logger.Errorf("cleanup failed in defer %s: %v", name, cleanupErr) + } + } + } + + defer func() { + if err != nil { + cleanup(ctx, request.Name) + } + }() + // Handle post network create actions warning := "" if bridgeDriver != nil { diff --git a/internal/service/network/driver/bridge.go b/internal/service/network/driver/bridge.go index 136b2d8a..bd2886a0 100644 --- a/internal/service/network/driver/bridge.go +++ b/internal/service/network/driver/bridge.go @@ -43,7 +43,7 @@ var NewBridgeDriver = func(netClient backend.NerdctlNetworkSvc, logger flog.Logg }, nil } -// handleBridgeDriverOptions filters unsupported options for the bridge driver. +// HandleCreateOptions processes finch specific options for the bridge driver. func (bd *bridgeDriver) HandleCreateOptions(request types.NetworkCreateRequest, options netutil.CreateOptions) (netutil.CreateOptions, error) { // enable_icc, host_binding_ipv4, and bridge name network options are not supported by nerdctl. // So we process these options here and filter them out from the network create request to nerdctl. @@ -162,12 +162,18 @@ func (bd *bridgeDriver) setBridgeName(net *netutil.NetworkConfig, bridgeName str } } - // Write the updated config back to the file data, err := json.MarshalIndent(netMap, "", " ") if err != nil { return err } - return os.WriteFile(configFilename, data, 0o644) + + // Write the updated config back to the file with the original permissions + fileInfo, err := os.Stat(configFilename) + if err != nil { + return err + } + + return os.WriteFile(configFilename, data, fileInfo.Mode().Perm()) }) } From ca250ee022dd5cedcc7da3166babfd3ac4e5836d Mon Sep 17 00:00:00 2001 From: Swagat Bora Date: Mon, 4 Nov 2024 06:37:59 +0000 Subject: [PATCH 4/6] Add a per-network mutex lock to make Create and Remove calls thread safe Signed-off-by: Swagat Bora --- internal/service/network/create.go | 7 +++++ internal/service/network/network.go | 44 +++++++++++++++++++++++++---- internal/service/network/remove.go | 17 ++++++++++- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/internal/service/network/create.go b/internal/service/network/create.go index d1bfc3cb..dea8252a 100644 --- a/internal/service/network/create.go +++ b/internal/service/network/create.go @@ -88,6 +88,13 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest return types.NetworkCreateResponse{}, err } + // Ensure thread-safety for network operations using a per-network mutex. + // Operations on different network IDs can proceed concurrently. + netMu := s.ensureLock(request.Name) + + netMu.Lock() + defer netMu.Unlock() + // Create network net, err := s.netClient.CreateNetwork(options) if err != nil && strings.Contains(err.Error(), "unsupported cni driver") { diff --git a/internal/service/network/network.go b/internal/service/network/network.go index 8f29f54a..fd7cc866 100644 --- a/internal/service/network/network.go +++ b/internal/service/network/network.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "regexp" + "sync" "github.com/containerd/containerd" "github.com/containerd/nerdctl/pkg/netutil" @@ -25,16 +26,19 @@ var ( ) type service struct { - client backend.ContainerdClient - netClient backend.NerdctlNetworkSvc - logger flog.Logger + client backend.ContainerdClient + netClient backend.NerdctlNetworkSvc + logger flog.Logger + mu sync.RWMutex + networkMutexes map[string]*sync.Mutex } func NewService(client backend.ContainerdClient, netClient backend.NerdctlNetworkSvc, logger flog.Logger) network.Service { return &service{ - client: client, - netClient: netClient, - logger: logger, + client: client, + netClient: netClient, + logger: logger, + networkMutexes: make(map[string]*sync.Mutex), } } @@ -97,3 +101,31 @@ func (s *service) getContainer(ctx context.Context, containerId string) (contain return searchResult[0], nil } + +func (s *service) ensureLock(networkName string) *sync.Mutex { + s.mu.RLock() + if netMu, exists := s.networkMutexes[networkName]; exists { + s.mu.RUnlock() + return netMu + } + s.mu.RUnlock() + + s.mu.Lock() + defer s.mu.Unlock() + + // Double-check in case another goroutine created it while we were waiting for the write lock + if netMu, exists := s.networkMutexes[networkName]; exists { + return netMu + } + + netMu := &sync.Mutex{} + s.networkMutexes[networkName] = netMu + return netMu +} + +func (s *service) clearLock(networkName string) { + s.mu.Lock() + defer s.mu.Unlock() + + delete(s.networkMutexes, networkName) +} diff --git a/internal/service/network/remove.go b/internal/service/network/remove.go index 2d557b7b..78ad4a80 100644 --- a/internal/service/network/remove.go +++ b/internal/service/network/remove.go @@ -32,12 +32,27 @@ func (s *service) Remove(ctx context.Context, networkId string) error { return errdefs.NewForbidden(fmt.Errorf("%s is a pre-defined network and cannot be removed", networkId)) } + // Ensure thread-safety for network operations using a per-network mutex. + // RemoveNetwork and CreateNetwork operations on the same network ID are mutually exclusive. + // Operations on different network IDs can proceed concurrently. + netMu := s.ensureLock(net.Name) + + netMu.Lock() + defer netMu.Unlock() + // Perform additional workflow based on the assigned network labels if err := s.handleNetworkLabels(net); err != nil { return fmt.Errorf("failed to handle nerdctl label: %w", err) } - return s.netClient.RemoveNetwork(net) + err = s.netClient.RemoveNetwork(net) + if err != nil { + return fmt.Errorf("failed to remove network: %w", err) + } + + // Clear the lock if remove was successful + s.clearLock(net.Name) + return nil } func (s *service) handleNetworkLabels(net *netutil.NetworkConfig) error { From b9da5030d0f538f5be8010c276abf54023a6be6d Mon Sep 17 00:00:00 2001 From: Swagat Bora Date: Fri, 8 Nov 2024 06:41:20 +0000 Subject: [PATCH 5/6] Also track active-reqs to prevent multiple lock creation for the same network. Signed-off-by: Swagat Bora --- internal/service/network/create.go | 1 + internal/service/network/network.go | 36 +++++++++++++++-------------- internal/service/network/remove.go | 3 +-- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/internal/service/network/create.go b/internal/service/network/create.go index dea8252a..0c7f1493 100644 --- a/internal/service/network/create.go +++ b/internal/service/network/create.go @@ -94,6 +94,7 @@ func (s *service) Create(ctx context.Context, request types.NetworkCreateRequest netMu.Lock() defer netMu.Unlock() + defer s.releaseLock(request.Name) // Create network net, err := s.netClient.CreateNetwork(options) diff --git a/internal/service/network/network.go b/internal/service/network/network.go index fd7cc866..ca60bd35 100644 --- a/internal/service/network/network.go +++ b/internal/service/network/network.go @@ -25,12 +25,17 @@ var ( errNetworkIDNotFound = errors.New("network ID not found") ) +type mutexCounter struct { + mutex *sync.Mutex + activeReqs int // activeReqs will track concurrent active requests for the same network +} + type service struct { client backend.ContainerdClient netClient backend.NerdctlNetworkSvc logger flog.Logger - mu sync.RWMutex - networkMutexes map[string]*sync.Mutex + mu sync.Mutex + networkMutexes map[string]*mutexCounter } func NewService(client backend.ContainerdClient, netClient backend.NerdctlNetworkSvc, logger flog.Logger) network.Service { @@ -38,7 +43,7 @@ func NewService(client backend.ContainerdClient, netClient backend.NerdctlNetwor client: client, netClient: netClient, logger: logger, - networkMutexes: make(map[string]*sync.Mutex), + networkMutexes: make(map[string]*mutexCounter), } } @@ -103,29 +108,26 @@ func (s *service) getContainer(ctx context.Context, containerId string) (contain } func (s *service) ensureLock(networkName string) *sync.Mutex { - s.mu.RLock() - if netMu, exists := s.networkMutexes[networkName]; exists { - s.mu.RUnlock() - return netMu - } - s.mu.RUnlock() - s.mu.Lock() defer s.mu.Unlock() - - // Double-check in case another goroutine created it while we were waiting for the write lock - if netMu, exists := s.networkMutexes[networkName]; exists { - return netMu + if mwc, exists := s.networkMutexes[networkName]; exists { + mwc.activeReqs++ + return mwc.mutex } netMu := &sync.Mutex{} - s.networkMutexes[networkName] = netMu + s.networkMutexes[networkName] = &mutexCounter{mutex: netMu, activeReqs: 1} return netMu } -func (s *service) clearLock(networkName string) { +func (s *service) releaseLock(networkName string) { s.mu.Lock() defer s.mu.Unlock() - delete(s.networkMutexes, networkName) + if mwc, exists := s.networkMutexes[networkName]; exists { + mwc.activeReqs-- + if mwc.activeReqs == 0 { + delete(s.networkMutexes, networkName) + } + } } diff --git a/internal/service/network/remove.go b/internal/service/network/remove.go index 78ad4a80..a2a6692a 100644 --- a/internal/service/network/remove.go +++ b/internal/service/network/remove.go @@ -39,6 +39,7 @@ func (s *service) Remove(ctx context.Context, networkId string) error { netMu.Lock() defer netMu.Unlock() + defer s.releaseLock(net.Name) // Perform additional workflow based on the assigned network labels if err := s.handleNetworkLabels(net); err != nil { @@ -50,8 +51,6 @@ func (s *service) Remove(ctx context.Context, networkId string) error { return fmt.Errorf("failed to remove network: %w", err) } - // Clear the lock if remove was successful - s.clearLock(net.Name) return nil } From ba979e743aaddaaf9f5c922becba6f0d3c2cc079 Mon Sep 17 00:00:00 2001 From: Swagat Bora Date: Tue, 12 Nov 2024 21:54:26 +0000 Subject: [PATCH 6/6] Add additional unit-test for HandleCreateOptions Signed-off-by: Swagat Bora --- internal/service/network/driver/bridge.go | 8 +- .../service/network/driver/bridge_test.go | 142 ++++++++++++++++++ internal/service/network/network.go | 2 +- 3 files changed, 146 insertions(+), 6 deletions(-) diff --git a/internal/service/network/driver/bridge.go b/internal/service/network/driver/bridge.go index bd2886a0..eb592440 100644 --- a/internal/service/network/driver/bridge.go +++ b/internal/service/network/driver/bridge.go @@ -58,12 +58,10 @@ func (bd *bridgeDriver) HandleCreateOptions(request types.NetworkCreateRequest, case BridgeICCOption: iccOption, err := strconv.ParseBool(v) if err != nil { - bd.logger.Warnf("invalid value for com.docker.network.bridge.enable_icc: %s", v) + bd.logger.Warnf("invalid value for com.docker.network.bridge.enable_icc") + continue } - if !iccOption { - bd.disableICC = true - } - + bd.disableICC = !iccOption case BridgeNameOption: bd.bridgeName = v default: diff --git a/internal/service/network/driver/bridge_test.go b/internal/service/network/driver/bridge_test.go index 8d73161d..dfc4270b 100644 --- a/internal/service/network/driver/bridge_test.go +++ b/internal/service/network/driver/bridge_test.go @@ -6,14 +6,156 @@ package driver import ( "fmt" + "github.com/containerd/nerdctl/pkg/netutil" "github.com/coreos/go-iptables/iptables" "github.com/golang/mock/gomock" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/runfinch/finch-daemon/api/types" "github.com/runfinch/finch-daemon/mocks/mocks_logger" "github.com/runfinch/finch-daemon/mocks/mocks_network" ) +var _ = Describe("bridgeDriver HandleCreateOptions", func() { + var ( + mockController *gomock.Controller + logger *mocks_logger.Logger + driver *bridgeDriver + request types.NetworkCreateRequest + options netutil.CreateOptions + ) + + BeforeEach(func() { + mockController = gomock.NewController(GinkgoT()) + logger = mocks_logger.NewLogger(mockController) + driver = &bridgeDriver{logger: logger} + request = types.NetworkCreateRequest{ + Options: make(map[string]string), + } + options = netutil.CreateOptions{ + Options: make(map[string]string), + Labels: []string{}, + } + }) + + Context("when processing bridge options", func() { + It("should handle empty options", func() { + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + Expect(result.Labels).To(BeEmpty()) + }) + + It("should process valid host binding IPv4", func() { + request.Options = map[string]string{ + BridgeHostBindingIpv4Option: "0.0.0.0", + } + logger.EXPECT().Warnf(gomock.Any(), gomock.Any()).Times(0) + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + }) + + It("should warn on invalid host binding IPv4", func() { + request.Options = map[string]string{ + BridgeHostBindingIpv4Option: "192.168.1.1", + } + logger.EXPECT().Warnf("network option com.docker.network.bridge.host_binding_ipv4 is set to %s, but it must be 0.0.0.0", "192.168.1.1") + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + }) + + It("should handle valid ICC option true", func() { + trueValues := []string{"1", "t", "T", "TRUE", "true", "True"} + for _, v := range trueValues { + request.Options = map[string]string{ + BridgeICCOption: v, + } + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + Expect(driver.disableICC).To(BeFalse(), "Failed for value: "+v) + Expect(result.Labels).To(BeEmpty(), "Failed for value: "+v) + } + }) + + It("should handle valid ICC option false", func() { + falseValues := []string{"0", "f", "F", "FALSE", "false", "False"} + for _, v := range falseValues { + request.Options = map[string]string{ + BridgeICCOption: v, + } + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + Expect(driver.disableICC).To(BeTrue(), "Failed for value: "+v) + Expect(result.Labels).To(ContainElement(FinchICCLabelIPv4+"=false"), "Failed for value: "+v) + } + }) + + It("should handle valid ICC option false with IPv6", func() { + request.Options = map[string]string{ + BridgeICCOption: "false", + } + driver.IPv6 = true + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + Expect(driver.disableICC).To(BeTrue()) + Expect(result.Labels).To(ContainElement(FinchICCLabelIPv6 + "=false")) + }) + + It("should warn on invalid ICC option", func() { + request.Options = map[string]string{ + BridgeICCOption: "invalid", + } + logger.EXPECT().Warnf("invalid value for com.docker.network.bridge.enable_icc") + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + Expect(driver.disableICC).To(BeFalse()) + Expect(result.Labels).To(BeEmpty()) + }) + + It("should set bridge name", func() { + request.Options = map[string]string{ + BridgeNameOption: "testbridge", + } + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(BeEmpty()) + Expect(driver.bridgeName).To(Equal("testbridge")) + }) + + It("should pass through unknown options", func() { + request.Options = map[string]string{ + "unknown.option": "value", + } + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(HaveKey("unknown.option")) + Expect(result.Options["unknown.option"]).To(Equal("value")) + }) + + It("should handle multiple options together", func() { + request.Options = map[string]string{ + BridgeHostBindingIpv4Option: "0.0.0.0", + BridgeICCOption: "false", + BridgeNameOption: "testbridge", + "unknown.option": "value", + } + logger.EXPECT().Warnf(gomock.Any(), gomock.Any()).Times(0) + result, err := driver.HandleCreateOptions(request, options) + Expect(err).NotTo(HaveOccurred()) + Expect(result.Options).To(HaveKey("unknown.option")) + Expect(driver.disableICC).To(BeTrue()) + Expect(driver.bridgeName).To(Equal("testbridge")) + Expect(result.Labels).To(ContainElement(FinchICCLabelIPv4 + "=false")) + }) + }) +}) + var _ = Describe("bridgeDriver Disable ICC", func() { var ( mockController *gomock.Controller diff --git a/internal/service/network/network.go b/internal/service/network/network.go index ca60bd35..c4c0fd68 100644 --- a/internal/service/network/network.go +++ b/internal/service/network/network.go @@ -126,7 +126,7 @@ func (s *service) releaseLock(networkName string) { if mwc, exists := s.networkMutexes[networkName]; exists { mwc.activeReqs-- - if mwc.activeReqs == 0 { + if mwc.activeReqs < 1 { delete(s.networkMutexes, networkName) } }