Skip to content

Commit 7895a5a

Browse files
committed
fix: pass APIVersion through createConfigs, reorder inspect
- Add APIVersion field to createConfigs so createMobyContainer can use the version already fetched by getCreateConfigs instead of calling RuntimeVersion a second time. - Move ContainerInspect after the NetworkConnect loop so the returned container.Summary includes all attached networks. - Add unit tests for the < 1.44 code path in both defaultNetworkSettings and createMobyContainer. Signed-off-by: Jarek Krochmalski <jkrochmalski@gmail.com>
1 parent 4f14ab8 commit 7895a5a

4 files changed

Lines changed: 154 additions & 26 deletions

File tree

pkg/compose/convergence.go

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -733,28 +733,12 @@ func (s *composeService) createMobyContainer(ctx context.Context, project *types
733733
Text: warning,
734734
})
735735
}
736-
res, err := s.apiClient().ContainerInspect(ctx, response.ID, client.ContainerInspectOptions{})
737-
if err != nil {
738-
return created, err
739-
}
740-
created = container.Summary{
741-
ID: res.Container.ID,
742-
Labels: res.Container.Config.Labels,
743-
Names: []string{res.Container.Name},
744-
NetworkSettings: &container.NetworkSettingsSummary{
745-
Networks: res.Container.NetworkSettings.Networks,
746-
},
747-
}
748736

749737
// Starting API version 1.44, the ContainerCreate API call takes multiple networks
750738
// so we include all configurations there and can skip the one-by-one calls here.
751739
// For older API versions (e.g. Docker 20.10/API 1.41, Synology DSM 7.1/7.2),
752740
// extra networks must be connected individually after creation via NetworkConnect.
753-
apiVersion, err := s.RuntimeVersion(ctx)
754-
if err != nil {
755-
return created, err
756-
}
757-
if versions.LessThan(apiVersion, apiVersion144) {
741+
if versions.LessThan(cfgs.APIVersion, apiVersion144) {
758742
// The highest-priority network is the primary and is already included in the
759743
// ContainerCreate API call via NetworkMode & NetworkingConfig.
760744
// Any remaining networks are connected one-by-one here after creation (but before start).
@@ -770,14 +754,27 @@ func (s *composeService) createMobyContainer(ctx context.Context, project *types
770754
return created, err
771755
}
772756
if _, err := s.apiClient().NetworkConnect(ctx, mobyNetworkName, client.NetworkConnectOptions{
773-
Container: created.ID,
757+
Container: response.ID,
774758
EndpointConfig: epSettings,
775759
}); err != nil {
776760
return created, err
777761
}
778762
}
779763
}
780764

765+
res, err := s.apiClient().ContainerInspect(ctx, response.ID, client.ContainerInspectOptions{})
766+
if err != nil {
767+
return created, err
768+
}
769+
created = container.Summary{
770+
ID: res.Container.ID,
771+
Labels: res.Container.Config.Labels,
772+
Names: []string{res.Container.Name},
773+
NetworkSettings: &container.NetworkSettingsSummary{
774+
Networks: res.Container.NetworkSettings.Networks,
775+
},
776+
}
777+
781778
return created, nil
782779
}
783780

pkg/compose/convergence_test.go

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,3 +498,98 @@ func TestCreateMobyContainer(t *testing.T) {
498498
assert.DeepEqual(t, want, got, cmpopts.EquateComparable(netip.Addr{}), cmpopts.EquateEmpty())
499499
assert.NilError(t, err)
500500
}
501+
502+
func TestCreateMobyContainerLegacyAPI(t *testing.T) {
503+
mockCtrl := gomock.NewController(t)
504+
defer mockCtrl.Finish()
505+
apiClient := mocks.NewMockAPIClient(mockCtrl)
506+
cli := mocks.NewMockCli(mockCtrl)
507+
tested, err := NewComposeService(cli)
508+
assert.NilError(t, err)
509+
cli.EXPECT().Client().Return(apiClient).AnyTimes()
510+
cli.EXPECT().ConfigFile().Return(&configfile.ConfigFile{}).AnyTimes()
511+
apiClient.EXPECT().DaemonHost().Return("").AnyTimes()
512+
apiClient.EXPECT().ImageInspect(anyCancellableContext(), gomock.Any()).Return(client.ImageInspectResult{}, nil).AnyTimes()
513+
514+
// force `RuntimeVersion` to return a pre-1.44 API version
515+
runtimeVersion = runtimeVersionCache{}
516+
apiClient.EXPECT().ServerVersion(gomock.Any(), gomock.Any()).Return(client.ServerVersionResult{
517+
APIVersion: "1.43",
518+
}, nil).AnyTimes()
519+
520+
service := types.ServiceConfig{
521+
Name: "test",
522+
Networks: map[string]*types.ServiceNetworkConfig{
523+
"a": {
524+
Priority: 10,
525+
},
526+
"b": {
527+
Priority: 100,
528+
},
529+
},
530+
}
531+
project := types.Project{
532+
Name: "bork",
533+
Services: types.Services{
534+
"test": service,
535+
},
536+
Networks: types.Networks{
537+
"a": types.NetworkConfig{
538+
Name: "a-moby-name",
539+
},
540+
"b": types.NetworkConfig{
541+
Name: "b-moby-name",
542+
},
543+
},
544+
}
545+
546+
var gotCreate client.ContainerCreateOptions
547+
apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) {
548+
gotCreate = opts
549+
return client.ContainerCreateResult{ID: "an-id"}, nil
550+
})
551+
552+
// For API < 1.44, the secondary network "a" should be connected via NetworkConnect.
553+
// Using gomock.InOrder to verify NetworkConnect is called before ContainerInspect.
554+
var gotConnect client.NetworkConnectOptions
555+
connectCall := apiClient.EXPECT().NetworkConnect(gomock.Any(), gomock.Eq("a-moby-name"), gomock.Any()).DoAndReturn(func(_ context.Context, _ string, opts client.NetworkConnectOptions) (client.NetworkConnectResult, error) {
556+
gotConnect = opts
557+
return client.NetworkConnectResult{}, nil
558+
})
559+
560+
apiClient.EXPECT().ContainerInspect(gomock.Any(), gomock.Eq("an-id"), gomock.Any()).Times(1).After(connectCall).Return(client.ContainerInspectResult{
561+
Container: container.InspectResponse{
562+
ID: "an-id",
563+
Name: "a-name",
564+
Config: &container.Config{},
565+
NetworkSettings: &container.NetworkSettings{
566+
Networks: map[string]*network.EndpointSettings{
567+
"b-moby-name": {
568+
IPAMConfig: &network.EndpointIPAMConfig{},
569+
Aliases: []string{"bork-test-0"},
570+
},
571+
"a-moby-name": {
572+
IPAMConfig: &network.EndpointIPAMConfig{},
573+
Aliases: []string{"bork-test-0"},
574+
},
575+
},
576+
},
577+
},
578+
}, nil)
579+
580+
_, err = tested.(*composeService).createMobyContainer(t.Context(), &project, service, "test", 0, nil, createOptions{
581+
Labels: make(types.Labels),
582+
UseNetworkAliases: true,
583+
})
584+
assert.NilError(t, err)
585+
586+
// ContainerCreate should only have the primary network (b, highest priority) in EndpointsConfig
587+
assert.Check(t, gotCreate.NetworkingConfig != nil)
588+
assert.Equal(t, len(gotCreate.NetworkingConfig.EndpointsConfig), 1)
589+
_, hasPrimary := gotCreate.NetworkingConfig.EndpointsConfig["b-moby-name"]
590+
assert.Check(t, hasPrimary, "primary network b-moby-name should be in ContainerCreate EndpointsConfig")
591+
592+
// NetworkConnect should have been called for the secondary network "a"
593+
assert.Equal(t, gotConnect.Container, "an-id")
594+
assert.Check(t, gotConnect.EndpointConfig != nil)
595+
}

pkg/compose/create.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,11 @@ type createOptions struct {
5353
}
5454

5555
type createConfigs struct {
56-
Container *container.Config
57-
Host *container.HostConfig
58-
Network *network.NetworkingConfig
59-
Links []string
56+
Container *container.Config
57+
Host *container.HostConfig
58+
Network *network.NetworkingConfig
59+
Links []string
60+
APIVersion string
6061
}
6162

6263
func (s *composeService) Create(ctx context.Context, project *types.Project, createOpts api.CreateOptions) error {
@@ -332,10 +333,11 @@ func (s *composeService) getCreateConfigs(ctx context.Context,
332333
}
333334

334335
cfgs := createConfigs{
335-
Container: &containerConfig,
336-
Host: &hostConfig,
337-
Network: networkingConfig,
338-
Links: links,
336+
Container: &containerConfig,
337+
Host: &hostConfig,
338+
Network: networkingConfig,
339+
Links: links,
340+
APIVersion: apiVersion,
339341
}
340342
return cfgs, nil
341343
}

pkg/compose/create_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,40 @@ func TestDefaultNetworkSettings(t *testing.T) {
273273
assert.Check(t, cmp.Nil(networkConfig))
274274
})
275275

276+
t.Run("returns only primary network in EndpointsConfig for API < 1.44", func(t *testing.T) {
277+
service := composetypes.ServiceConfig{
278+
Name: "myService",
279+
Networks: map[string]*composetypes.ServiceNetworkConfig{
280+
"myNetwork1": {
281+
Priority: 10,
282+
},
283+
"myNetwork2": {
284+
Priority: 1000,
285+
},
286+
},
287+
}
288+
project := composetypes.Project{
289+
Name: "myProject",
290+
Services: composetypes.Services{
291+
"myService": service,
292+
},
293+
Networks: composetypes.Networks(map[string]composetypes.NetworkConfig{
294+
"myNetwork1": {
295+
Name: "myProject_myNetwork1",
296+
},
297+
"myNetwork2": {
298+
Name: "myProject_myNetwork2",
299+
},
300+
}),
301+
}
302+
303+
networkMode, networkConfig, err := defaultNetworkSettings(&project, service, 1, nil, true, "1.43")
304+
assert.NilError(t, err)
305+
assert.Equal(t, string(networkMode), "myProject_myNetwork2")
306+
assert.Check(t, cmp.Len(networkConfig.EndpointsConfig, 1))
307+
assert.Check(t, cmp.Contains(networkConfig.EndpointsConfig, "myProject_myNetwork2"))
308+
})
309+
276310
t.Run("returns defined network mode if explicitly set", func(t *testing.T) {
277311
service := composetypes.ServiceConfig{
278312
Name: "myService",

0 commit comments

Comments
 (0)