Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include all networks in ContainerCreate call if API >= 1.44 #11429

Merged
merged 1 commit into from Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions pkg/compose/compose.go
Expand Up @@ -312,11 +312,13 @@ func (s *composeService) isSWarmEnabled(ctx context.Context) (bool, error) {
return swarmEnabled.val, swarmEnabled.err
}

var runtimeVersion = struct {
type runtimeVersionCache struct {
once sync.Once
val string
err error
}{}
}

var runtimeVersion runtimeVersionCache

func (s *composeService) RuntimeVersion(ctx context.Context) (string, error) {
runtimeVersion.once.Do(func() {
Expand Down
35 changes: 22 additions & 13 deletions pkg/compose/convergence.go
Expand Up @@ -34,6 +34,7 @@
"github.com/docker/compose/v2/internal/tracing"
moby "github.com/docker/docker/api/types"
containerType "github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/versions"
specs "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/sirupsen/logrus"
"golang.org/x/sync/errgroup"
Expand Down Expand Up @@ -600,19 +601,27 @@
},
}

// the highest-priority network is the primary and is included in the ContainerCreate API
// call via container.NetworkMode & network.NetworkingConfig
// any remaining networks are connected one-by-one here after creation (but before start)
serviceNetworks := service.NetworksByPriority()
for _, networkKey := range serviceNetworks {
mobyNetworkName := project.Networks[networkKey].Name
if string(cfgs.Host.NetworkMode) == mobyNetworkName {
// primary network already configured as part of ContainerCreate
continue
}
epSettings := createEndpointSettings(project, service, number, networkKey, cfgs.Links, opts.UseNetworkAliases)
if err := s.apiClient().NetworkConnect(ctx, mobyNetworkName, created.ID, epSettings); err != nil {
return created, err
apiVersion, err := s.RuntimeVersion(ctx)
if err != nil {
return created, err
}

Check warning on line 607 in pkg/compose/convergence.go

View check run for this annotation

Codecov / codecov/patch

pkg/compose/convergence.go#L606-L607

Added lines #L606 - L607 were not covered by tests
// Starting API version 1.44, the ContainerCreate API call takes multiple networks
// so we include all the configurations there and can skip the one-by-one calls here
if versions.LessThan(apiVersion, "1.44") {
// the highest-priority network is the primary and is included in the ContainerCreate API
// call via container.NetworkMode & network.NetworkingConfig
// any remaining networks are connected one-by-one here after creation (but before start)
serviceNetworks := service.NetworksByPriority()
for _, networkKey := range serviceNetworks {
mobyNetworkName := project.Networks[networkKey].Name
if string(cfgs.Host.NetworkMode) == mobyNetworkName {
// primary network already configured as part of ContainerCreate
continue
}
epSettings := createEndpointSettings(project, service, number, networkKey, cfgs.Links, opts.UseNetworkAliases)
if err := s.apiClient().NetworkConnect(ctx, mobyNetworkName, created.ID, epSettings); err != nil {
return created, err
}

Check warning on line 624 in pkg/compose/convergence.go

View check run for this annotation

Codecov / codecov/patch

pkg/compose/convergence.go#L623-L624

Added lines #L623 - L624 were not covered by tests
}
}

Expand Down
183 changes: 183 additions & 0 deletions pkg/compose/convergence_test.go
Expand Up @@ -23,14 +23,18 @@ import (
"testing"

"github.com/compose-spec/compose-go/v2/types"
"github.com/docker/cli/cli/config/configfile"
moby "github.com/docker/docker/api/types"
containerType "github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/network"
"github.com/docker/go-connections/nat"
"go.uber.org/mock/gomock"
"gotest.tools/v3/assert"

"github.com/docker/compose/v2/pkg/api"
"github.com/docker/compose/v2/pkg/mocks"
"github.com/docker/compose/v2/pkg/progress"
)

func TestContainerName(t *testing.T) {
Expand Down Expand Up @@ -251,3 +255,182 @@ func TestWaitDependencies(t *testing.T) {
assert.NilError(t, tested.waitDependencies(context.Background(), &project, "", dependencies, nil))
})
}

func TestCreateMobyContainer(t *testing.T) {
t.Run("connects container networks one by one if API <1.44", func(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
apiClient := mocks.NewMockAPIClient(mockCtrl)
cli := mocks.NewMockCli(mockCtrl)
tested := composeService{
dockerCli: cli,
}
cli.EXPECT().Client().Return(apiClient).AnyTimes()
cli.EXPECT().ConfigFile().Return(&configfile.ConfigFile{}).AnyTimes()
apiClient.EXPECT().DaemonHost().Return("").AnyTimes()
apiClient.EXPECT().ImageInspectWithRaw(gomock.Any(), gomock.Any()).Return(moby.ImageInspect{}, nil, nil).AnyTimes()
// force `RuntimeVersion` to fetch again
runtimeVersion = runtimeVersionCache{}
apiClient.EXPECT().ServerVersion(gomock.Any()).Return(moby.Version{
APIVersion: "1.43",
}, nil).AnyTimes()

service := types.ServiceConfig{
Name: "test",
Networks: map[string]*types.ServiceNetworkConfig{
"a": {
Priority: 10,
},
"b": {
Priority: 100,
},
},
}
project := types.Project{
Name: "bork",
Services: types.Services{
"test": service,
},
Networks: types.Networks{
"a": types.NetworkConfig{
Name: "a-moby-name",
},
"b": types.NetworkConfig{
Name: "b-moby-name",
},
},
}

var falseBool bool
apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any(), gomock.Eq(
&containerType.HostConfig{
PortBindings: nat.PortMap{},
ExtraHosts: []string{},
Tmpfs: map[string]string{},
Resources: containerType.Resources{
OomKillDisable: &falseBool,
},
NetworkMode: "b-moby-name",
}), gomock.Eq(
&network.NetworkingConfig{
EndpointsConfig: map[string]*network.EndpointSettings{
"b-moby-name": {
IPAMConfig: &network.EndpointIPAMConfig{},
Aliases: []string{"bork-test-0"},
},
},
}), gomock.Any(), gomock.Any()).Times(1).Return(
containerType.CreateResponse{
ID: "an-id",
}, nil)

apiClient.EXPECT().ContainerInspect(gomock.Any(), gomock.Eq("an-id")).Times(1).Return(
moby.ContainerJSON{
ContainerJSONBase: &moby.ContainerJSONBase{
ID: "an-id",
Name: "a-name",
},
Config: &containerType.Config{},
NetworkSettings: &moby.NetworkSettings{},
}, nil)

apiClient.EXPECT().NetworkConnect(gomock.Any(), "a-moby-name", "an-id", gomock.Eq(
&network.EndpointSettings{
IPAMConfig: &network.EndpointIPAMConfig{},
Aliases: []string{"bork-test-0"},
}))

_, err := tested.createMobyContainer(context.Background(), &project, service, "test", 0, nil, createOptions{
Labels: make(types.Labels),
}, progress.ContextWriter(context.TODO()))
assert.NilError(t, err)
})

t.Run("includes all container networks in ContainerCreate call if API >=1.44", func(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
apiClient := mocks.NewMockAPIClient(mockCtrl)
cli := mocks.NewMockCli(mockCtrl)
tested := composeService{
dockerCli: cli,
}
cli.EXPECT().Client().Return(apiClient).AnyTimes()
cli.EXPECT().ConfigFile().Return(&configfile.ConfigFile{}).AnyTimes()
apiClient.EXPECT().DaemonHost().Return("").AnyTimes()
apiClient.EXPECT().ImageInspectWithRaw(gomock.Any(), gomock.Any()).Return(moby.ImageInspect{}, nil, nil).AnyTimes()
// force `RuntimeVersion` to fetch fresh version
runtimeVersion = runtimeVersionCache{}
apiClient.EXPECT().ServerVersion(gomock.Any()).Return(moby.Version{
APIVersion: "1.44",
}, nil).AnyTimes()

service := types.ServiceConfig{
Name: "test",
Networks: map[string]*types.ServiceNetworkConfig{
"a": {
Priority: 10,
},
"b": {
Priority: 100,
},
},
}
project := types.Project{
Name: "bork",
Services: types.Services{
"test": service,
},
Networks: types.Networks{
"a": types.NetworkConfig{
Name: "a-moby-name",
},
"b": types.NetworkConfig{
Name: "b-moby-name",
},
},
}

var falseBool bool
apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any(), gomock.Eq(
&containerType.HostConfig{
PortBindings: nat.PortMap{},
ExtraHosts: []string{},
Tmpfs: map[string]string{},
Resources: containerType.Resources{
OomKillDisable: &falseBool,
},
NetworkMode: "b-moby-name",
}), gomock.Eq(
&network.NetworkingConfig{
EndpointsConfig: map[string]*network.EndpointSettings{
"a-moby-name": {
IPAMConfig: &network.EndpointIPAMConfig{},
Aliases: []string{"bork-test-0"},
},
"b-moby-name": {
IPAMConfig: &network.EndpointIPAMConfig{},
Aliases: []string{"bork-test-0"},
},
},
}), gomock.Any(), gomock.Any()).Times(1).Return(
containerType.CreateResponse{
ID: "an-id",
}, nil)

apiClient.EXPECT().ContainerInspect(gomock.Any(), gomock.Eq("an-id")).Times(1).Return(
moby.ContainerJSON{
ContainerJSONBase: &moby.ContainerJSONBase{
ID: "an-id",
Name: "a-name",
},
Config: &containerType.Config{},
NetworkSettings: &moby.NetworkSettings{},
}, nil)

_, err := tested.createMobyContainer(context.Background(), &project, service, "test", 0, nil, createOptions{
Labels: make(types.Labels),
}, progress.ContextWriter(context.TODO()))
assert.NilError(t, err)
})

}
40 changes: 30 additions & 10 deletions pkg/compose/create.go
Expand Up @@ -235,7 +235,11 @@
if err != nil {
return createConfigs{}, err
}
networkMode, networkingConfig := defaultNetworkSettings(p, service, number, links, opts.UseNetworkAliases)
apiVersion, err := s.RuntimeVersion(ctx)
if err != nil {
return createConfigs{}, err
}

Check warning on line 241 in pkg/compose/create.go

View check run for this annotation

Codecov / codecov/patch

pkg/compose/create.go#L240-L241

Added lines #L240 - L241 were not covered by tests
networkMode, networkingConfig := defaultNetworkSettings(p, service, number, links, opts.UseNetworkAliases, apiVersion)
portBindings := buildContainerPortBindingOptions(service)

// MISC
Expand Down Expand Up @@ -456,6 +460,7 @@
serviceIndex int,
links []string,
useNetworkAliases bool,
version string,
) (container.NetworkMode, *network.NetworkingConfig) {
if service.NetworkMode != "" {
return container.NetworkMode(service.NetworkMode), nil
Expand All @@ -465,23 +470,38 @@
return "none", nil
}

var networkKey string
var primaryNetworkKey string
if len(service.Networks) > 0 {
networkKey = service.NetworksByPriority()[0]
primaryNetworkKey = service.NetworksByPriority()[0]
} else {
networkKey = "default"
primaryNetworkKey = "default"
}
primaryNetworkMobyNetworkName := project.Networks[primaryNetworkKey].Name
endpointsConfig := map[string]*network.EndpointSettings{
primaryNetworkMobyNetworkName: createEndpointSettings(project, service, serviceIndex, primaryNetworkKey, links, useNetworkAliases),
}

// Starting from API version 1.44, the Engine will take several EndpointsConfigs
// so we can pass all the extra networks we want the container to be connected to
// in the network configuration instead of connecting the container to each extra
// network individually after creation.
if versions.GreaterThanOrEqualTo(version, "1.44") && len(service.Networks) > 1 {
serviceNetworks := service.NetworksByPriority()
for _, networkKey := range serviceNetworks[1:] {
mobyNetworkName := project.Networks[networkKey].Name
epSettings := createEndpointSettings(project, service, serviceIndex, networkKey, links, useNetworkAliases)
endpointsConfig[mobyNetworkName] = epSettings
}
}
mobyNetworkName := project.Networks[networkKey].Name
epSettings := createEndpointSettings(project, service, serviceIndex, networkKey, links, useNetworkAliases)

networkConfig := &network.NetworkingConfig{
EndpointsConfig: map[string]*network.EndpointSettings{
mobyNetworkName: epSettings,
},
EndpointsConfig: endpointsConfig,
}

// From the Engine API docs:
// > Supported standard values are: bridge, host, none, and container:<name|id>.
// > Any other value is taken as a custom network's name to which this container should connect to.
return container.NetworkMode(mobyNetworkName), networkConfig
return container.NetworkMode(primaryNetworkMobyNetworkName), networkConfig
}

func getRestartPolicy(service types.ServiceConfig) container.RestartPolicy {
Expand Down
8 changes: 4 additions & 4 deletions pkg/compose/create_test.go
Expand Up @@ -193,7 +193,7 @@ func TestDefaultNetworkSettings(t *testing.T) {
}),
}

networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true, "1.43")
assert.Equal(t, string(networkMode), "myProject_myNetwork2")
assert.Check(t, cmp.Len(networkConfig.EndpointsConfig, 1))
assert.Check(t, cmp.Contains(networkConfig.EndpointsConfig, "myProject_myNetwork2"))
Expand Down Expand Up @@ -221,7 +221,7 @@ func TestDefaultNetworkSettings(t *testing.T) {
}),
}

networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true, "1.43")
assert.Equal(t, string(networkMode), "myProject_default")
assert.Check(t, cmp.Len(networkConfig.EndpointsConfig, 1))
assert.Check(t, cmp.Contains(networkConfig.EndpointsConfig, "myProject_default"))
Expand All @@ -238,7 +238,7 @@ func TestDefaultNetworkSettings(t *testing.T) {
},
}

networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true, "1.43")
assert.Equal(t, string(networkMode), "none")
assert.Check(t, cmp.Nil(networkConfig))
})
Expand All @@ -258,7 +258,7 @@ func TestDefaultNetworkSettings(t *testing.T) {
}),
}

networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true, "1.43")
assert.Equal(t, string(networkMode), "host")
assert.Check(t, cmp.Nil(networkConfig))
})
Expand Down