Fix linting issues in preparation of Go and GolangCI-lint update #48359

thaJeztah · 2024-08-21T15:05:45Z

relates to update to go1.22.6 #46982

See individual commits for details

thaJeztah · 2024-08-21T15:07:38Z

layer/layer.go

 	if parent == "" {
-		return createChainIDFromParent(ChainID(dgsts[0]), dgsts[1:]...)
+		return createChainIDFromParent(ChainID(dgsts[0]), dgsts[1:]...) //nolint:gosec // FIXME(thaJeztah): ignore G602: slice index out of range, which is a false positive
 	}
 	// H = "H(n-1) SHA256(n)"
-	dgst := digest.FromBytes([]byte(string(parent) + " " + string(dgsts[0])))
-	return createChainIDFromParent(ChainID(dgst), dgsts[1:]...)
+	dgst := digest.FromBytes([]byte(string(parent) + " " + string(dgsts[0]))) //nolint:gosec // FIXME(thaJeztah): ignore G602: slice index out of range, which is a false positive
+	return createChainIDFromParent(ChainID(dgst), dgsts[1:]...)               //nolint:gosec  // FIXME(thaJeztah): ignore G602: slice index out of range, which is a false positive
 }


I tried to create a minimal reproducer for this one, but for some reason GolangCI only picked up this case, but not when trying to write a more minimal one. I'll give it another try to see if I can report this.

corhere

The printf govet lints are all legitimate code bugs. Don't ignore them!

integration-cli/docker_utils_test.go

api/server/router/image/image_routes.go

libnetwork/controller.go

libnetwork/sandbox_externalkey_unix.go

corhere · 2024-08-21T18:29:00Z

libnetwork/cnmallocator/drivers_ipam.go

@@ -40,7 +40,7 @@ func initIPAMDrivers(r ipamapi.Registerer, netConfig *networkallocator.Config) e
 	}

 	if len(addressPool) > 0 {
-		log.G(context.TODO()).Infof("Swarm initialized global default address pool to: " + str.String())
+		log.G(context.TODO()).Info("Swarm initialized global default address pool to: " + str.String())


Alternatively:

Suggested change

log.G(context.TODO()).Info("Swarm initialized global default address pool to: " + str.String())

log.G(context.TODO()).Infof("Swarm initialized global default address pool to: %v", str.String())

layer/layer.go

api/server/router/container/container_routes.go

corhere · 2024-08-21T18:53:50Z

api/server/router/container/container_routes.go

+			_, _ = fmt.Fprint(conn, "HTTP/1.1 101 UPGRADED\r\nContent-Type: "+contentType+"\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n")
 		} else {
-			fmt.Fprintf(conn, "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n")
+			_, _ = fmt.Fprint(conn, "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n")


Should we really be ignoring errors encountered while writing to the socket? If the connection has e.g. been reset by the remote end, any attempt to continue handling the request is just wasting cycles.

And on the topic of ignoring write errors, see line 977.

I'll have a look at this in a follow-up; I had a quick peek at how this code is used, and saw some other parts that I want to have a look at.

thaJeztah · 2024-08-21T20:30:32Z

Failure looks like it may be related to changes deployed to docker hub w.r.t. search. I tried both on master and this PR, and both fail;

make TEST_FILTER=TestSearchWithLimit DOCKER_GRAPHDRIVER=vfs test-integration

...

=== FAIL: arm64.integration-cli TestDockerCLISearchSuite/TestSearchWithLimit (1.42s)
    docker_cli_search_test.go:88: assertion failed: 51 (int) != 52 (limit + 2 int)
    --- FAIL: TestDockerCLISearchSuite/TestSearchWithLimit (1.42s)

=== FAIL: arm64.integration-cli TestDockerCLISearchSuite (1.42s)

OK confirmed; something odd with limits. I’m able to reproduce with just curl and jq;

# no limit produces 25 results (default)
curl -fsSL 'https://index.docker.io/v1/search?q=docker' | jq '.results | length'
25

# searching for 'docker' return 98 results (I'd expect there to be more than that) 
curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=100' | jq '.results | length'
98

# try giving more than 100 limit (doesn't work; but perhaps it's capping to 100
curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=110' | jq '.results | length'
98

# limiting to 50 produces 49 results
curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=50' | jq '.results | length'
49

# limiting to 51 produces 49 results
curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=51' | jq '.results | length'
49

# limiting to 52 produces 50 results
curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=52' | jq '.results | length'
50

# limiting to 10 produces 10 reesults
curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=10' | jq '.results | length'
10

w.r.t. “expected to be more than 100 results for docker“; looks like it indeed has;

curl -fsSL 'https://index.docker.io/v1/search?q=docker&n=100' | jq
{
  "num_pages": 100,
  "num_results": 10000,
  "page_size": 100,
  "page": 1,
  "query": "docker",
...
...

thaJeztah

@corhere updated; I took most of your suggestions; I still want to look at the error-handling in a follow-up

corhere · 2024-08-26T16:26:02Z

container/stream/streams.go

-	if len(errors) > 0 {
-		return fmt.Errorf(strings.Join(errors, "\n"))
+	if len(errs) > 0 {
+		return errors.New(strings.Join(errs, "\n"))


This looks like an ideal candidate for errors.Join (it even formats the same!) but that'd be out of scope for this PR. Follow up?

Heh; yup, I was looking at exactly that! 👍

corhere · 2024-08-26T16:41:28Z

api/server/router/container/container_routes.go

+			// FIXME(thaJeztah): we should not ignore errors here; see https://github.com/moby/moby/pull/48359#discussion_r1725562802
+			_, _ = fmt.Fprintf(conn, "HTTP/1.1 101 UPGRADED\r\nContent-Type: %v\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n", contentType)


Explicitly assigning an error-return value to underscore signifies that discarding the error is intentional and correct. That is not the case here, given the need for the FIXME comment. Our linter settings currently exclude the errcheck rule that complains about implicitly-ignored error returns for printf-family functions so there is no need to appease the linter here and now to get CI to pass. Adding underscore-assignments here does our future selves a disservice as it prevents us from being able to find all the call sites in need of fixes by linting the project without that lint exclusion rule.

This condition was added in 0215a62, which removed pkg/homedir as abstraction, but didn't consider that this test is currently only ran on Unix. integration-cli/docker_cli_run_unix_test.go:254:5: SA4032: due to the file's build constraints, runtime.GOOS will never equal "windows" (staticcheck) if runtime.GOOS == "windows" { ^ integration-cli/docker_cli_run_unix_test.go:338:5: SA4032: due to the file's build constraints, runtime.GOOS will never equal "windows" (staticcheck) if runtime.GOOS == "windows" { ^ Added a TODO, because this functionality should also be tested on Windows, probably as part of tests in docker/cli instead. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork/sandbox_dns_unix_test.go:17:13: SA4032: due to the file's build constraints, runtime.GOOS will never equal "windows" (staticcheck) skip.If(t, runtime.GOOS == "windows", "test only works on linux") ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

integration-cli/benchmark_test.go:49:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ integration-cli/benchmark_test.go:62:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ integration-cli/benchmark_test.go:68:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ integration-cli/benchmark_test.go:73:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ integration-cli/benchmark_test.go:78:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ integration-cli/benchmark_test.go:84:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ integration-cli/benchmark_test.go:94:27: printf: non-constant format string in call to fmt.Errorf (govet) chErr <- fmt.Errorf(out) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Also rename some variables that shadowed imports, and fix some unhandled errors. integration-cli/docker_cli_network_unix_test.go:102:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"Error":"failed to add veth pair: `+err.Error()+`"}`) ^ integration-cli/docker_cli_network_unix_test.go:136:18: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"LocalDefaultAddressSpace":"`+lAS+`", "GlobalDefaultAddressSpace": "`+gAS+`"}`) ^ integration-cli/docker_cli_network_unix_test.go:147:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"Error":"Unknown address space in pool request: `+poolRequest.AddressSpace+`"}`) ^ integration-cli/docker_cli_network_unix_test.go:151:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"PoolID":"`+poolID+`", "Pool":"`+pool+`"}`) ^ integration-cli/docker_cli_network_unix_test.go:168:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"Address":"`+gw+`"}`) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Also fix some unhandled errors. integration-cli/docker_cli_swarm_test.go:697:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"Error":"failed to add veth pair: `+err.Error()+`"}`) ^ integration-cli/docker_cli_swarm_test.go:731:18: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"LocalDefaultAddressSpace":"`+lAS+`", "GlobalDefaultAddressSpace": "`+gAS+`"}`) ^ integration-cli/docker_cli_swarm_test.go:742:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"Error":"Unknown address space in pool request: `+poolRequest.AddressSpace+`"}`) ^ integration-cli/docker_cli_swarm_test.go:746:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"PoolID":"`+poolID+`", "Pool":"`+pool+`"}`) ^ integration-cli/docker_cli_swarm_test.go:763:19: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(w, `{"Address":"`+gw+`"}`) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork/controller.go:1054:32: printf: non-constant format string in call to github.com/docker/docker/libnetwork/types.NotFoundErrorf (govet) return types.NotFoundErrorf(err.Error()) ^ libnetwork/controller.go:1073:32: printf: non-constant format string in call to github.com/docker/docker/libnetwork/types.NotFoundErrorf (govet) return types.NotFoundErrorf(err.Error()) ^ libnetwork/sandbox_externalkey_unix.go:113:21: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(string(buf[0:n])) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cmd/dockerd/required.go:17:24: printf: non-constant format string in call to github.com/docker/docker/vendor/github.com/pkg/errors.Errorf (govet) return errors.Errorf("\n" + strings.TrimRight(cmd.UsageString(), "\n")) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork/cnmallocator/drivers_ipam.go:43:31: printf: non-constant format string in call to (*github.com/docker/docker/vendor/github.com/sirupsen/logrus.Entry).Infof (govet) log.G(context.TODO()).Infof("Swarm initialized global default address pool to: " + str.String()) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

internal/cleanups/composite_test.go:46:9: printf: non-constant format string in call to (*testing.common).Logf (govet) t.Logf(err.Error()) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This looks to be a false positive; layer/layer.go:202:47: G602: slice index out of range (gosec) return createChainIDFromParent(ChainID(dgsts[0]), dgsts[1:]...) ^ layer/layer.go:205:69: G602: slice index out of range (gosec) dgst := digest.FromBytes([]byte(string(parent) + " " + string(dgsts[0]))) ^ layer/layer.go:206:53: G602: slice bounds out of range (gosec) return createChainIDFromParent(ChainID(dgst), dgsts[1:]...) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

… call (govet) builder/dockerfile/internals_linux.go:38:48: printf: non-constant format string in call to github.com/docker/docker/vendor/github.com/pkg/errors.Wrapf (govet) return idtools.Identity{}, errors.Wrapf(err, "can't find uid for user "+userStr) ^ builder/dockerfile/internals_linux.go:42:48: printf: non-constant format string in call to github.com/docker/docker/vendor/github.com/pkg/errors.Wrapf (govet) return idtools.Identity{}, errors.Wrapf(err, "can't find gid for group "+grpStr) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

volume/testutils/testutils.go:98:26: printf: non-constant format string in call to fmt.Errorf (govet) return nil, fmt.Errorf(opts["error"]) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

…vet) libnetwork/drivers/bridge/setup_ip_tables_linux.go:385:23: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(msg) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

container/stream/streams.go:111:21: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(strings.Join(errors, "\n")) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

api/server/router/container/container_routes.go:943:22: printf: non-constant format string in call to fmt.Fprintf (govet) fmt.Fprintf(conn, "HTTP/1.1 101 UPGRADED\r\nContent-Type: "+contentType+"\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n") ^ api/server/router/image/image_routes.go:144:50: printf: non-constant format string in call to github.com/docker/docker/pkg/streamformatter.FormatStatus (govet) output.Write(streamformatter.FormatStatus("", id.String())) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

api/types/container/hostconfig.go:328:43: printf: non-constant format string in call to fmt.Errorf (govet) return &errInvalidParameter{fmt.Errorf(msg)} ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/daemon.go:942:21: printf: non-constant format string in call to (*github.com/docker/docker/vendor/github.com/sirupsen/logrus.Entry).Errorf (govet) log.G(ctx).Errorf(err.Error()) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2024-08-27T14:59:15Z

@corhere updated; let me know if this is what you had in mind for the error-handling

corhere

LGTM!

robmry

LGTM

thaJeztah added status/2-code-review area/testing process/cherry-pick/27.2 labels Aug 21, 2024

thaJeztah added this to the 28.0.0 milestone Aug 21, 2024

thaJeztah requested a review from tonistiigi as a code owner August 21, 2024 15:05

thaJeztah mentioned this pull request Aug 21, 2024

update to go1.22.6 #46982

Merged

6 tasks

thaJeztah commented Aug 21, 2024

View reviewed changes

thaJeztah self-assigned this Aug 21, 2024

corhere requested changes Aug 21, 2024

View reviewed changes

thaJeztah mentioned this pull request Aug 22, 2024

poll: Continue(): use format.Message for formatting gotestyourself/gotest.tools#279

Merged

thaJeztah force-pushed the fix_linting_issues branch from 59886ec to 573af21 Compare August 26, 2024 09:15

thaJeztah commented Aug 26, 2024

View reviewed changes

thaJeztah mentioned this pull request Aug 26, 2024

Fix linting issues in preparation of Go and GolangCI-lint update docker/cli#5370

Merged

corhere reviewed Aug 26, 2024

View reviewed changes

thaJeztah added 15 commits August 27, 2024 09:57

integration-cli: fix non-constant format string in call (govet)

b79a469

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

internal/cleanups: fix non-constant format string in call (govet)

c308bd3

internal/cleanups/composite_test.go:46:9: printf: non-constant format string in call to (*testing.common).Logf (govet) t.Logf(err.Error()) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

volume/testutils: fix non-constant format string in call (govet)

f434cdd

volume/testutils/testutils.go:98:26: printf: non-constant format string in call to fmt.Errorf (govet) return nil, fmt.Errorf(opts["error"]) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork/drivers/bridge: fix non-constant format string in call (go…

068c1bf

…vet) libnetwork/drivers/bridge/setup_ip_tables_linux.go:385:23: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(msg) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

container/stream: fix non-constant format string in call (govet)

4a93233

container/stream/streams.go:111:21: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(strings.Join(errors, "\n")) ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah added 3 commits August 27, 2024 10:41

api/types: fix non-constant format string in call (govet)

005b488

api/types/container/hostconfig.go:328:43: printf: non-constant format string in call to fmt.Errorf (govet) return &errInvalidParameter{fmt.Errorf(msg)} ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the fix_linting_issues branch from 573af21 to 1ad5b5a Compare August 27, 2024 08:42

corhere approved these changes Aug 27, 2024

View reviewed changes

thaJeztah mentioned this pull request Aug 28, 2024

[27.x backport] Fix linting issues in preparation of Go and GolangCI-lint update #48395

Merged

robmry approved these changes Aug 28, 2024

View reviewed changes

thaJeztah merged commit 92a05cf into moby:master Aug 28, 2024
127 checks passed

thaJeztah deleted the fix_linting_issues branch August 28, 2024 13:48

austinvazquez added process/cherry-picked and removed process/cherry-pick/27.2 labels Aug 30, 2024

This was referenced Sep 3, 2024

[26.1 backport] update to go1.22.6 #48427

Merged

[25.0 backport] update to go1.22.6 #48428

Merged

This was referenced Sep 6, 2024

container/stream: Config.CloseStreams(): use errors.Join #48435

Merged

Bump engine cli v27.2.1 docker/compose#12115

Merged

This was referenced Jan 2, 2025

fix non-constant format string (caught by go1.24) #49201

Merged

[27.x backport] fix non-constant format string (caught by go1.24) #49204

Merged

	log.G(context.TODO()).Info("Swarm initialized global default address pool to: " + str.String())
	log.G(context.TODO()).Infof("Swarm initialized global default address pool to: %v", str.String())

		// FIXME(thaJeztah): we should not ignore errors here; see https://github.com/moby/moby/pull/48359#discussion_r1725562802
		_, _ = fmt.Fprintf(conn, "HTTP/1.1 101 UPGRADED\r\nContent-Type: %v\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n", contentType)

Fix linting issues in preparation of Go and GolangCI-lint update #48359

Fix linting issues in preparation of Go and GolangCI-lint update #48359

Uh oh!

Conversation

thaJeztah commented Aug 21, 2024

Uh oh!

thaJeztah Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corhere Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corhere Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

corhere Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

corhere Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 27, 2024

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Aug 21, 2024 •

edited

Loading