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

[20.10 backport] Dockerfile: update golangci-lint v1.44.0 #43975

Merged
merged 25 commits into from Aug 18, 2022

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 17, 2022

Some small changes needed;

  • version is specified in the installer in this branch
  • libnetwork commit is not needed in 20.10 branch
  • some conflicts in golangci yaml config (but CI should show if it's correct)

@thaJeztah

This comment was marked as resolved.

@thaJeztah
Copy link
Member Author

Looks like I need #42445

These should be ok to ignore for the purpose they're used

    pkg/namesgenerator/names-generator.go:843:36: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
        name := fmt.Sprintf("%s_%s", left[rand.Intn(len(left))], right[rand.Intn(len(right))])
                                          ^
    pkg/namesgenerator/names-generator.go:849:36: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            name = fmt.Sprintf("%s%d", name, rand.Intn(10))
                                             ^
    testutil/stringutils.go:11:18: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            b[i] = letters[rand.Intn(len(letters))]
                           ^
    pkg/namesgenerator/names-generator.go:849:36: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            name = fmt.Sprintf("%s%d", name, rand.Intn(10))
                                             ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 6b0ecac)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/logger/journald/read.go:128:3 comment on exported function `CErr` should be of the form `CErr ...`

    daemon/logger/journald/read.go:131:36: unnecessary conversion (unconvert)
            return C.GoString(C.strerror(C.int(-ret)))
	                                  ^
    daemon/logger/journald/read.go:380:2: S1023: redundant `return` statement (gosimple)
        return
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit d43bcc8)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    client/request.go:245:2: S1031: unnecessary nil check around range (gosimple)
        if headers != nil {
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit b92be7e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/volumes_unix_test.go:228:13: SA4001: &*x will be simplified to x. It will not copy x. (staticcheck)
                mp:      &(*c.MountPoints["/jambolan"]), // copy the mountpoint, expect no changes
                         ^
    daemon/logger/local/local_test.go:214:22: SA4001: &*x will be simplified to x. It will not copy x. (staticcheck)
            dst.PLogMetaData = &(*src.PLogMetaData)
                               ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit f7433d6)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    pkg/devicemapper/devmapper.go:383:28: S1039: unnecessary use of fmt.Sprintf (gosimple)
        if err := task.setMessage(fmt.Sprintf("@cancel_deferred_remove")); err != nil {
                                  ^
    integration/plugin/graphdriver/external_test.go:321:18: S1039: unnecessary use of fmt.Sprintf (gosimple)
                http.Error(w, fmt.Sprintf("missing id"), 409)
                              ^
    integration-cli/docker_api_stats_test.go:70:31: S1039: unnecessary use of fmt.Sprintf (gosimple)
            _, body, err := request.Get(fmt.Sprintf("/info"))
                                        ^
    integration-cli/docker_cli_build_test.go:4547:19: S1039: unnecessary use of fmt.Sprintf (gosimple)
                "--build-arg", fmt.Sprintf("FOO1=fromcmd"),
                               ^
    integration-cli/docker_cli_build_test.go:4548:19: S1039: unnecessary use of fmt.Sprintf (gosimple)
                "--build-arg", fmt.Sprintf("FOO2="),
                               ^
    integration-cli/docker_cli_build_test.go:4549:19: S1039: unnecessary use of fmt.Sprintf (gosimple)
                "--build-arg", fmt.Sprintf("FOO3"), // set in env
                               ^
    integration-cli/docker_cli_build_test.go:4668:32: S1039: unnecessary use of fmt.Sprintf (gosimple)
            cli.WithFlags("--build-arg", fmt.Sprintf("tag=latest")))
                                         ^
    integration-cli/docker_cli_build_test.go:4690:32: S1039: unnecessary use of fmt.Sprintf (gosimple)
            cli.WithFlags("--build-arg", fmt.Sprintf("baz=abc")))
                                         ^
    pkg/jsonmessage/jsonmessage_test.go:255:4: S1039: unnecessary use of fmt.Sprintf (gosimple)
                fmt.Sprintf("ID: status\n"),
                ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit f77213e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    plugin/v2/plugin.go:141:50: G601: Implicit memory aliasing in for loop. (gosec)
                    updateSettingsEnv(&p.PluginObj.Settings.Env, &s)
                                                                 ^
    libcontainerd/remote/client.go:572:13: G601: Implicit memory aliasing in for loop. (gosec)
                cpDesc = &m
                         ^
    distribution/push_v2.go:400:34: G601: Implicit memory aliasing in for loop. (gosec)
                (metadata.CheckV2MetadataHMAC(&mountCandidate, pd.hmacKey) ||
                                              ^
    builder/dockerfile/builder.go:261:84: G601: Implicit memory aliasing in for loop. (gosec)
            currentCommandIndex = printCommand(b.Stdout, currentCommandIndex, totalCommands, &meta)
                                                                                             ^
    builder/dockerfile/builder.go:278:46: G601: Implicit memory aliasing in for loop. (gosec)
            if err := initializeStage(dispatchRequest, &stage); err != nil {
                                                       ^
    daemon/container.go:283:40: G601: Implicit memory aliasing in for loop. (gosec)
            if err := parser.ValidateMountConfig(&cfg); err != nil {
                                                 ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit d13997b)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/cluster/executor/container/adapter.go:446:42: G601: Implicit memory aliasing in for loop. (gosec)
            req := c.container.volumeCreateRequest(&mount)
                                                   ^
    daemon/network.go:577:10: G601: Implicit memory aliasing in for loop. (gosec)
                np := &n
                      ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit b4c0c7c)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    integration/build/build_session_test.go:92:6: func `testBuildWithSession` is unused (unused)
    func testBuildWithSession(t *testing.T, client dclient.APIClient, daemonHost string, dir, dockerfile string) (outStr string) {
         ^
    integration/container/checkpoint_test.go:23:6: func `containerExec` is unused (unused)
    func containerExec(t *testing.T, client client.APIClient, cID string, cmd []string) {
         ^
    integration/network/service_test.go:295:6: func `swarmIngressReady` is unused (unused)
    func swarmIngressReady(client client.NetworkAPIClient) func(log poll.LogT) poll.Result {
         ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 7c91fd4)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…se positive)

Also looks like a false positive, but given that these were basically
testing for the `errdefs.Conflict` and `errdefs.NotFound` interfaces, I
replaced these with those;

    daemon/stats/collector.go:154:6: type `notRunningErr` is unused (unused)
    type notRunningErr interface {
         ^
    daemon/stats/collector.go:159:6: type `notFoundErr` is unused (unused)
    type notFoundErr interface {
         ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 09191c0)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    pkg/archive/copy.go:357:16: G110: Potential DoS vulnerability via decompression bomb (gosec)
                if _, err = io.Copy(rebasedTar, srcTar); err != nil {
                            ^

Ignoring GoSec G110. See securego/gosec#433
and https://cure53.de/pentest-report_opa.pdf, which recommends to
replace io.Copy with io.CopyN7. The latter allows to specify the
maximum number of bytes that should be read. By properly defining
the limit, it can be assured that a GZip compression bomb cannot
easily cause a Denial-of-Service.
After reviewing, this should not affect us, because here we do not
read into memory.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 7b071e0)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/logger/splunk/splunk.go:173:16: G402: TLS MinVersion too low. (gosec)
    	tlsConfig := &tls.Config{}
    	              ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 4004a39)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    builder/builder-next/adapters/snapshot/snapshot.go:386:3: if-return: redundant if ...; err != nil check, just return error instead. (revive)
            if err := b.Put(keyIsCommitted, []byte{}); err != nil {
                return err
            }

    plugin/fetch_linux.go:112:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
        if err := images.Dispatch(ctx, images.Handlers(handlers...), nil, desc); err != nil {
            return err
        }

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit dd1374f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Unlike regular comments, nolint comments should not have a leading space.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit bb17074)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/config/config_unix.go:92:21: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
            return fmt.Errorf("Default cgroup namespace mode (%v) is invalid. Use \"host\" or \"private\".", cm) // nolint: golint
                              ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 16ced76)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/list.go:556:18: var-declaration: should omit type bool from declaration of var shouldSkip; it will be inferred from the right-hand side (revive)
                shouldSkip    bool = true
                              ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit d61b7c1)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
….SliceHeader

Probably needs a similar change as c208f03,
but this code makes my head spin, so for now suppressing, and created a
tracking issue:

    daemon/graphdriver/graphtest/graphtest_unix.go:305:12: unsafeptr: possible misuse of reflect.SliceHeader (govet)
        header := *(*reflect.SliceHeader)(unsafe.Pointer(&buf))
                  ^
    daemon/graphdriver/graphtest/graphtest_unix.go:308:36: unsafeptr: possible misuse of reflect.SliceHeader (govet)
        data := *(*[]byte)(unsafe.Pointer(&header))
                                          ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit e6dabfa)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The message changed from "is deprecated" to "has been deprecated":

    client/hijack.go:85:16: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
        clientconn := httputil.NewClientConn(conn, nil)
                      ^
    integration/plugin/authz/authz_plugin_test.go:180:7: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
        c := httputil.NewClientConn(conn, nil)
             ^
    integration/plugin/authz/authz_plugin_test.go:479:12: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
        client := httputil.NewClientConn(conn, nil)
                  ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit ea74765)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 22ce0f8)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 594c972)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the 20.10_backport_update_golangci_lint branch from 471b989 to d279c40 Compare August 17, 2022 16:21
@thaJeztah
Copy link
Member Author

Almost there; one remaining;


 registry/endpoint_v1.go:92:2: S1017: should replace this if statement with an unconditional strings.TrimSuffix (gosimple)
 	if strings.HasSuffix(address, "/") {
 	^

@thaJeztah
Copy link
Member Author

Looks like 4a52c46 (part of #42915) fixed that

strings.TrimSuffix() does exactly the same as this code, but is
a bit more readable.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 4a52c46)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…heck)

It's deprecated in Go 1.18:

    client/request.go:157:8: SA1019: err.Temporary is deprecated: Temporary errors are not well-defined. Most "temporary" errors are timeouts, and the few exceptions are surprising. Do not use this method. (staticcheck)
        if !err.Temporary() {
            ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 2cff05e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…sec)

    daemon/logger/awslogs/cloudwatchlogs.go:42:2: G101: Potential hardcoded credentials (gosec)
        credentialsEndpointKey = "awslogs-credentials-endpoint"
        ^
    daemon/logger/awslogs/cloudwatchlogs.go:67:2: G101: Potential hardcoded credentials (gosec)
        credentialsEndpoint = "http://169.254.170.2"
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit b88f4e2)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
As caught by gosimple:

    client/client.go:138:14: S1040: type assertion to the same type: c.client.Transport already has type http.RoundTripper (gosimple)
        if _, ok := c.client.Transport.(http.RoundTripper); !ok {
                    ^

This check was originally added in dc9f5c2, to
check if the passed option was a `http.Transport`, and later changed in
e345cd1 to check for `http.RoundTripper` instead.

Client.client is a http.Client, for which the Transport field is a RoundTripper,
so this check is redundant.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 99935ff)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I think the original intent here was to make passing t optional (62a856e),
but it looks like that's not done anywhere, so let's remove it.

    integration-cli/docker_utils_test.go:81:2: SA5011: possible nil pointer dereference (staticcheck)
        c.Helper()
        ^
    integration-cli/docker_utils_test.go:84:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if c != nil {
           ^
    integration-cli/docker_utils_test.go:106:2: SA5011: possible nil pointer dereference (staticcheck)
        c.Helper()
        ^
    integration-cli/docker_utils_test.go:108:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if c != nil {
           ^
    integration-cli/docker_utils_test.go:116:2: SA5011: possible nil pointer dereference (staticcheck)
        c.Helper()
        ^
    integration-cli/docker_utils_test.go:118:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if c != nil {
           ^
    integration-cli/docker_utils_test.go:126:2: SA5011: possible nil pointer dereference (staticcheck)
        c.Helper()
        ^
    integration-cli/docker_utils_test.go:128:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if c != nil {
           ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 89f63f4)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Looks like this may be needed for Go 1.18

Also updating the golangci-lint configuration to account for updated
exclusion rules.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 646ace6)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the 20.10_backport_update_golangci_lint branch from d279c40 to a7299ae Compare August 17, 2022 17:03
@thaJeztah thaJeztah marked this pull request as ready for review August 17, 2022 18:17
@thaJeztah thaJeztah merged commit 32debe0 into moby:20.10 Aug 18, 2022
@thaJeztah thaJeztah deleted the 20.10_backport_update_golangci_lint branch August 18, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants