From 8f59f841ae1e734c60d45b47e23a69f041850508 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 7 Nov 2023 18:45:48 +0100 Subject: [PATCH 1/2] cli/command/container: mustParse: return network.NetworkingConfig Next commit will need this change to test whether the endpoint-specific MacAddress is correctly set. Signed-off-by: Albin Kerouanton --- cli/command/container/opts_test.go | 52 +++++++++++++++--------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index d0f50b34a6a2..e86e7a774667 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -64,21 +64,21 @@ func setupRunFlags() (*pflag.FlagSet, *containerOptions) { return flags, copts } -func mustParse(t *testing.T, args string) (*container.Config, *container.HostConfig) { +func mustParse(t *testing.T, args string) (*container.Config, *container.HostConfig, *networktypes.NetworkingConfig) { t.Helper() - config, hostConfig, _, err := parseRun(append(strings.Split(args, " "), "ubuntu", "bash")) + config, hostConfig, nwConfig, err := parseRun(append(strings.Split(args, " "), "ubuntu", "bash")) assert.NilError(t, err) - return config, hostConfig + return config, hostConfig, nwConfig } func TestParseRunLinks(t *testing.T) { - if _, hostConfig := mustParse(t, "--link a:b"); len(hostConfig.Links) == 0 || hostConfig.Links[0] != "a:b" { + if _, hostConfig, _ := mustParse(t, "--link a:b"); len(hostConfig.Links) == 0 || hostConfig.Links[0] != "a:b" { t.Fatalf("Error parsing links. Expected []string{\"a:b\"}, received: %v", hostConfig.Links) } - if _, hostConfig := mustParse(t, "--link a:b --link c:d"); len(hostConfig.Links) < 2 || hostConfig.Links[0] != "a:b" || hostConfig.Links[1] != "c:d" { + if _, hostConfig, _ := mustParse(t, "--link a:b --link c:d"); len(hostConfig.Links) < 2 || hostConfig.Links[0] != "a:b" || hostConfig.Links[1] != "c:d" { t.Fatalf("Error parsing links. Expected []string{\"a:b\", \"c:d\"}, received: %v", hostConfig.Links) } - if _, hostConfig := mustParse(t, ""); len(hostConfig.Links) != 0 { + if _, hostConfig, _ := mustParse(t, ""); len(hostConfig.Links) != 0 { t.Fatalf("Error parsing links. No link expected, received: %v", hostConfig.Links) } } @@ -128,7 +128,7 @@ func TestParseRunAttach(t *testing.T) { for _, tc := range tests { tc := tc t.Run(tc.input, func(t *testing.T) { - config, _ := mustParse(t, tc.input) + config, _, _ := mustParse(t, tc.input) assert.Equal(t, config.AttachStdin, tc.expected.AttachStdin) assert.Equal(t, config.AttachStdout, tc.expected.AttachStdout) assert.Equal(t, config.AttachStderr, tc.expected.AttachStderr) @@ -186,7 +186,7 @@ func TestParseRunWithInvalidArgs(t *testing.T) { func TestParseWithVolumes(t *testing.T) { // A single volume arr, tryit := setupPlatformVolume([]string{`/tmp`}, []string{`c:\tmp`}) - if config, hostConfig := mustParse(t, tryit); hostConfig.Binds != nil { + if config, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds != nil { t.Fatalf("Error parsing volume flags, %q should not mount-bind anything. Received %v", tryit, hostConfig.Binds) } else if _, exists := config.Volumes[arr[0]]; !exists { t.Fatalf("Error parsing volume flags, %q is missing from volumes. Received %v", tryit, config.Volumes) @@ -194,7 +194,7 @@ func TestParseWithVolumes(t *testing.T) { // Two volumes arr, tryit = setupPlatformVolume([]string{`/tmp`, `/var`}, []string{`c:\tmp`, `c:\var`}) - if config, hostConfig := mustParse(t, tryit); hostConfig.Binds != nil { + if config, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds != nil { t.Fatalf("Error parsing volume flags, %q should not mount-bind anything. Received %v", tryit, hostConfig.Binds) } else if _, exists := config.Volumes[arr[0]]; !exists { t.Fatalf("Error parsing volume flags, %s is missing from volumes. Received %v", arr[0], config.Volumes) @@ -204,13 +204,13 @@ func TestParseWithVolumes(t *testing.T) { // A single bind mount arr, tryit = setupPlatformVolume([]string{`/hostTmp:/containerTmp`}, []string{os.Getenv("TEMP") + `:c:\containerTmp`}) - if config, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || hostConfig.Binds[0] != arr[0] { + if config, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || hostConfig.Binds[0] != arr[0] { t.Fatalf("Error parsing volume flags, %q should mount-bind the path before the colon into the path after the colon. Received %v %v", arr[0], hostConfig.Binds, config.Volumes) } // Two bind mounts. arr, tryit = setupPlatformVolume([]string{`/hostTmp:/containerTmp`, `/hostVar:/containerVar`}, []string{os.Getenv("ProgramData") + `:c:\ContainerPD`, os.Getenv("TEMP") + `:c:\containerTmp`}) - if _, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { + if _, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { t.Fatalf("Error parsing volume flags, `%s and %s` did not mount-bind correctly. Received %v", arr[0], arr[1], hostConfig.Binds) } @@ -219,26 +219,26 @@ func TestParseWithVolumes(t *testing.T) { arr, tryit = setupPlatformVolume( []string{`/hostTmp:/containerTmp:ro`, `/hostVar:/containerVar:rw`}, []string{os.Getenv("TEMP") + `:c:\containerTmp:rw`, os.Getenv("ProgramData") + `:c:\ContainerPD:rw`}) - if _, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { + if _, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { t.Fatalf("Error parsing volume flags, `%s and %s` did not mount-bind correctly. Received %v", arr[0], arr[1], hostConfig.Binds) } // Similar to previous test but with alternate modes which are only supported by Linux if runtime.GOOS != "windows" { arr, tryit = setupPlatformVolume([]string{`/hostTmp:/containerTmp:ro,Z`, `/hostVar:/containerVar:rw,Z`}, []string{}) - if _, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { + if _, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { t.Fatalf("Error parsing volume flags, `%s and %s` did not mount-bind correctly. Received %v", arr[0], arr[1], hostConfig.Binds) } arr, tryit = setupPlatformVolume([]string{`/hostTmp:/containerTmp:Z`, `/hostVar:/containerVar:z`}, []string{}) - if _, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { + if _, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], arr[0], arr[1]) != nil { t.Fatalf("Error parsing volume flags, `%s and %s` did not mount-bind correctly. Received %v", arr[0], arr[1], hostConfig.Binds) } } // One bind mount and one volume arr, tryit = setupPlatformVolume([]string{`/hostTmp:/containerTmp`, `/containerVar`}, []string{os.Getenv("TEMP") + `:c:\containerTmp`, `c:\containerTmp`}) - if config, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || len(hostConfig.Binds) > 1 || hostConfig.Binds[0] != arr[0] { + if config, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || len(hostConfig.Binds) > 1 || hostConfig.Binds[0] != arr[0] { t.Fatalf("Error parsing volume flags, %s and %s should only one and only one bind mount %s. Received %s", arr[0], arr[1], arr[0], hostConfig.Binds) } else if _, exists := config.Volumes[arr[1]]; !exists { t.Fatalf("Error parsing volume flags %s and %s. %s is missing from volumes. Received %v", arr[0], arr[1], arr[1], config.Volumes) @@ -247,7 +247,7 @@ func TestParseWithVolumes(t *testing.T) { // Root to non-c: drive letter (Windows specific) if runtime.GOOS == "windows" { arr, tryit = setupPlatformVolume([]string{}, []string{os.Getenv("SystemDrive") + `\:d:`}) - if config, hostConfig := mustParse(t, tryit); hostConfig.Binds == nil || len(hostConfig.Binds) > 1 || hostConfig.Binds[0] != arr[0] || len(config.Volumes) != 0 { + if config, hostConfig, _ := mustParse(t, tryit); hostConfig.Binds == nil || len(hostConfig.Binds) > 1 || hostConfig.Binds[0] != arr[0] || len(config.Volumes) != 0 { t.Fatalf("Error parsing %s. Should have a single bind mount and no volumes", arr[0]) } } @@ -290,7 +290,7 @@ func TestParseWithMacAddress(t *testing.T) { if _, _, _, err := parseRun([]string{invalidMacAddress, "img", "cmd"}); err != nil && err.Error() != "invalidMacAddress is not a valid mac address" { t.Fatalf("Expected an error with %v mac-address, got %v", invalidMacAddress, err) } - if config, _ := mustParse(t, validMacAddress); config.MacAddress != "92:d0:c6:0a:29:33" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + if config, _, _ := mustParse(t, validMacAddress); config.MacAddress != "92:d0:c6:0a:29:33" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. t.Fatalf("Expected the config to have '92:d0:c6:0a:29:33' as container-wide MacAddress, got '%v'", config.MacAddress) //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. } @@ -302,7 +302,7 @@ func TestRunFlagsParseWithMemory(t *testing.T) { err := flags.Parse(args) assert.ErrorContains(t, err, `invalid argument "invalid" for "-m, --memory" flag`) - _, hostconfig := mustParse(t, "--memory=1G") + _, hostconfig, _ := mustParse(t, "--memory=1G") assert.Check(t, is.Equal(int64(1073741824), hostconfig.Memory)) } @@ -312,10 +312,10 @@ func TestParseWithMemorySwap(t *testing.T) { err := flags.Parse(args) assert.ErrorContains(t, err, `invalid argument "invalid" for "--memory-swap" flag`) - _, hostconfig := mustParse(t, "--memory-swap=1G") + _, hostconfig, _ := mustParse(t, "--memory-swap=1G") assert.Check(t, is.Equal(int64(1073741824), hostconfig.MemorySwap)) - _, hostconfig = mustParse(t, "--memory-swap=-1") + _, hostconfig, _ = mustParse(t, "--memory-swap=-1") assert.Check(t, is.Equal(int64(-1), hostconfig.MemorySwap)) } @@ -330,14 +330,14 @@ func TestParseHostname(t *testing.T) { hostnameWithDomain := "--hostname=hostname.domainname" hostnameWithDomainTld := "--hostname=hostname.domainname.tld" for hostname, expectedHostname := range validHostnames { - if config, _ := mustParse(t, fmt.Sprintf("--hostname=%s", hostname)); config.Hostname != expectedHostname { + if config, _, _ := mustParse(t, fmt.Sprintf("--hostname=%s", hostname)); config.Hostname != expectedHostname { t.Fatalf("Expected the config to have 'hostname' as %q, got %q", expectedHostname, config.Hostname) } } - if config, _ := mustParse(t, hostnameWithDomain); config.Hostname != "hostname.domainname" || config.Domainname != "" { + if config, _, _ := mustParse(t, hostnameWithDomain); config.Hostname != "hostname.domainname" || config.Domainname != "" { t.Fatalf("Expected the config to have 'hostname' as hostname.domainname, got %q", config.Hostname) } - if config, _ := mustParse(t, hostnameWithDomainTld); config.Hostname != "hostname.domainname.tld" || config.Domainname != "" { + if config, _, _ := mustParse(t, hostnameWithDomainTld); config.Hostname != "hostname.domainname.tld" || config.Domainname != "" { t.Fatalf("Expected the config to have 'hostname' as hostname.domainname.tld, got %q", config.Hostname) } } @@ -351,14 +351,14 @@ func TestParseHostnameDomainname(t *testing.T) { "domainname-63-bytes-long-should-be-valid-and-without-any-errors": "domainname-63-bytes-long-should-be-valid-and-without-any-errors", } for domainname, expectedDomainname := range validDomainnames { - if config, _ := mustParse(t, "--domainname="+domainname); config.Domainname != expectedDomainname { + if config, _, _ := mustParse(t, "--domainname="+domainname); config.Domainname != expectedDomainname { t.Fatalf("Expected the config to have 'domainname' as %q, got %q", expectedDomainname, config.Domainname) } } - if config, _ := mustParse(t, "--hostname=some.prefix --domainname=domainname"); config.Hostname != "some.prefix" || config.Domainname != "domainname" { + if config, _, _ := mustParse(t, "--hostname=some.prefix --domainname=domainname"); config.Hostname != "some.prefix" || config.Domainname != "domainname" { t.Fatalf("Expected the config to have 'hostname' as 'some.prefix' and 'domainname' as 'domainname', got %q and %q", config.Hostname, config.Domainname) } - if config, _ := mustParse(t, "--hostname=another-prefix --domainname=domainname.tld"); config.Hostname != "another-prefix" || config.Domainname != "domainname.tld" { + if config, _, _ := mustParse(t, "--hostname=another-prefix --domainname=domainname.tld"); config.Hostname != "another-prefix" || config.Domainname != "domainname.tld" { t.Fatalf("Expected the config to have 'hostname' as 'another-prefix' and 'domainname' as 'domainname.tld', got %q and %q", config.Hostname, config.Domainname) } } From 2b1f5a2e3844d8a24f2ff18e10878f46661381c2 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 7 Nov 2023 18:54:01 +0100 Subject: [PATCH 2/2] TestParseWithMacAddress: check endpoint-specific MacAddress field This is a follow-up of https://github.com/docker/cli/pull/4419. That PR leveraged the fact that EndpointSettings.MacAddress is already available, although not used by the CreateNetwork endpoint. TestParseWithMacAddress was testing whether the container-wide MacAddress field is set, and we still need to test that to ensure backward compatibility. But we now also need to test whether the endpoint-specific MacAddress is set. Signed-off-by: Albin Kerouanton --- cli/command/container/opts_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index e86e7a774667..54a39ce8168d 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -290,10 +290,15 @@ func TestParseWithMacAddress(t *testing.T) { if _, _, _, err := parseRun([]string{invalidMacAddress, "img", "cmd"}); err != nil && err.Error() != "invalidMacAddress is not a valid mac address" { t.Fatalf("Expected an error with %v mac-address, got %v", invalidMacAddress, err) } - if config, _, _ := mustParse(t, validMacAddress); config.MacAddress != "92:d0:c6:0a:29:33" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. + config, hostConfig, nwConfig := mustParse(t, validMacAddress) + if config.MacAddress != "92:d0:c6:0a:29:33" { //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. t.Fatalf("Expected the config to have '92:d0:c6:0a:29:33' as container-wide MacAddress, got '%v'", config.MacAddress) //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. } + defaultNw := hostConfig.NetworkMode.NetworkName() + if nwConfig.EndpointsConfig[defaultNw].MacAddress != "92:d0:c6:0a:29:33" { + t.Fatalf("Expected the default endpoint to have the MacAddress '92:d0:c6:0a:29:33' set, got '%v'", nwConfig.EndpointsConfig[defaultNw].MacAddress) + } } func TestRunFlagsParseWithMemory(t *testing.T) {