strict golangci-lint

[pohly]

What would you like to be added?

Let's use this issue to discuss how to proceed with strict linting in Kubernetes.

For each issue found by hack/verify-golangci-lint.sh -s (for "strict") we need to decide how severe it is:

• ignore it completely, i.e. disable the check = :-1:
• merely point it out in new code, but don't prevent merging = :eyes:
• merge-blocking = :+1:
• should be fixed in existing code = :rocket:

I am going to summarize the different issues in separate comments below, sorted by linter. For each comment, vote with reactions. I am using the first issue that gets reported, so this is not necessarily representative. See this gist for the full output.

Remember that intentional exceptions are okay (// nolint:<something>), so let's not disable a check just because it didn't make sense for that particular example. One occurrence of a real bug in production code may make a check worthwhile.

There's no need to vote on all issues. When there's no clear opinion either way, the default will be "merge-blocking".

This was presented at KubeCon (see https://kcseu2023.sched.com/event/1LXBP/improving-code-quality-enabling-additional-linters, which was recorded) and it seems useful to gather opinions here, so let's start voting. We may have Prow support for GitHub annotations by middle of May or at least will know how much longer that might take, so let's keep this open for a month and then revisit the topic.

Why is this needed?

Improve the quality of Kubernetes code.

[k8s-ci-robot]

There are no sig labels on this issue. Please add an appropriate label by using one of the following commands:

• /sig <group-name>
• /wg <group-name>
• /committee <group-name>

Please see the group list for a listing of the SIGs, working groups, and committees available.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[pohly]

First, a meta question: should pull-kubernetes-verify-strict-lint (currently not run by default, not merge-blocking) be enabled once we have consensus on what it should check, even when posting issues as GitHub annotations is not supported yet?

The user experience for contributors will be that they'll have to dig issues out of the Prow job output instead of exposing issues directly in the diff view. Issues that are not merge-blocking are likely to be missed when the job passes when there are no other, more severe issues.

Please vote with :+1: (= enabled without annotations) and :-1: (= wait for annotation support).

[pohly]

Count: 512 build/pause/windows/wincat/wincat.go:55.18 Error return value of conn.CloseRead is not checked (errcheck)

            conn.CloseRead()
                          ^

[pohly]

Count: 14 pkg/util/iptables/iptables.go:421.14 appendAssign: append result not assigned to the same slice (gocritic)

    fullArgs := append(runner.restoreWaitFlag, args...)
                ^

[pohly]

Count: 51 cmd/genutils/genutils.go:41.2 assignOp: replace outDir = outDir + "/" with outDir += "/" (gocritic)

    outDir = outDir + "/"
    ^

[pohly]

Count: 2 staging/src/k8s.io/code-generator/cmd/informer-gen/generators/packages.go:104.32 badCall: suspicious Join on 1 argument (gocritic)

    internalVersionPackagePath := filepath.Join(arguments.OutputPackagePath)
                                  ^

[pohly]

Count: 29 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go:1095.33 captLocal: `Delegate' should not be capitalized (gocritic)

func newUnstructuredObjectTyper(Delegate runtime.ObjectTyper) UnstructuredObjectTyper {
                                ^

[pohly]

Count: 84 pkg/volume/util/subpath/subpath_linux_test.go:925.2 commentFormatting: put a space between // and comment text (gocritic)

    //complete code
    ^

[pohly]

Count: 37 staging/src/k8s.io/api/rbac/v1alpha1/types.go:105.1 deprecatedComment: the proper format is Deprecated: <text> (gocritic)

// Deprecated in v1.17 in favor of rbac.authorization.k8s.io/v1 Role, and will no longer be served in v1.22.
^

[pohly]

Count: 5 staging/src/k8s.io/apimachinery/pkg/api/resource/quantity_test.go:1462.6 dupArg: suspicious method call with the same argument and receiver (gocritic)

        if q.Cmp(q) != 0 {
           ^

[pohly]

Count: 3 staging/src/k8s.io/kubectl/pkg/cmd/events/events_test.go:113.5 dupSubExpr: suspicious identical LHS and RHS for != operator (gocritic)

    if err != err {
       ^

[pohly]

Count: 31 staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go:1056.14 elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)

                        } else {
                               ^

[pohly]

Count: 4 test/conformance/walk.go:101.4 exitAfterDefer: log.Fatal will exit, and defer f.Close() will not run (gocritic)

            log.Fatal(err)
            ^

[pohly]

Count: 91 cmd/dependencyverifier/dependencyverifier.go:161.2 ifElseChain: rewrite if-else to switch statement (gocritic)

    if len(os.Args) == 2 {
    ^

[pohly]

Count: 7 staging/src/k8s.io/component-base/metrics/desc.go:184.26 newDeref: replace *new(sync.Once) with sync.Once{} (gocritic)

    d.markDeprecationOnce = *new(sync.Once)
                            ^

[pohly]

Count: 8 test/e2e_node/environment/conformance.go:181.23 regexpMust: for const patterns like iptablesForwardRegexStr, use regexp.MustCompile (gocritic)

    forwardRegex, err := regexp.Compile(iptablesForwardRegexStr)
                         ^

[pohly]

Count: 44 test/instrumentation/decode_metric.go:62.3 singleCaseSwitch: should rewrite switch statement to if statement (gocritic)

        switch v := fc.Fun.(type) {
        ^

[pohly]

Count: 3 staging/src/k8s.io/dynamic-resource-allocation/kubeletplugin/nonblockinggrpcserver.go:89.5 sloppyLen: len(interceptors) >= 0 is always true (gocritic)

    if len(interceptors) >= 0 {
       ^

[pohly]

Count: 2 staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go:524.2 typeSwitchVar: 1 case can benefit from type switch with assignment (gocritic)

    switch err.(type) {
    ^

[pohly]

Count: 8 staging/src/k8s.io/cli-runtime/pkg/resource/crd_finder.go:51.23 underef: could simplify (*list).Items to list.Items (gocritic)

        for _, crd := range (*list).Items {
                            ^

[pohly]

Count: 2 pkg/kubelet/cm/cpuset/cpuset.go:127.18 unlambda: replace func(cpu int) bool { return s2.Contains(cpu) } with s2.Contains (gocritic)

    return s.filter(func(cpu int) bool { return s2.Contains(cpu) })
                    ^

[pohly]

Count: 8 test/images/regression-issue-74839/main.go:127.61 unslice: could simplify data[:] to data (gocritic)

                _, err := conn.WriteTo(badPkt.encode(localIP, remoteIP, data[:]), addr)
                                                                        ^

[pohly]

Count: 1 staging/src/k8s.io/code-generator/cmd/go-to-protobuf/protobuf/cmd.go:477.2 valSwap: can re-write as o.elements[i], o.elements[j] = o.elements[j], o.elements[i] (gocritic)

    x := o.elements[i]
    ^

[pohly]

Count: 41 test/conformance/image/go-runner/tar.go:67.36 wrapperFunc: use strings.ReplaceAll method in strings.Replace(file, dir, "", -1) (gocritic)

        header.Name = strings.TrimPrefix(strings.Replace(file, dir, "", -1), string(filepath.Separator))
                                         ^

[pohly]

Count: 5 staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go:308.2 S1000: should use a simple channel send/receive instead of select with a single case (gosimple)

    select {
    ^

[pohly]

Count: 32 staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go:239.42 S1002: should omit comparison to bool constant, can be simplified to !*spec.PreserveUnknownFields (gosimple)

    if spec.PreserveUnknownFields == nil || *spec.PreserveUnknownFields == false {
                                            ^

[pohly]

Count: 3 staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go:1577.8 S1003: should use !strings.Contains(cond.Message, v) instead (gosimple)

                if strings.Index(cond.Message, v) == -1 {
                   ^

[pohly]

Count: 3 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go:820.8 S1004: should use !bytes.Equal(objBytes, expectedBytes) instead (gosimple)

                if bytes.Compare(objBytes, expectedBytes) != 0 {
                   ^

[pohly]

Count: 4 staging/src/k8s.io/apiserver/pkg/server/storage/resource_config.go:108.2 S1005: unnecessary assignment to the blank identifier (gosimple)

    enabled, _ := o.GroupVersionConfigs[version]
    ^

[pohly]

Count: 14 test/e2e/kubectl/portforward.go:62.22 S1007: should use raw string (...) with regexp.MustCompile to avoid having to escape twice (gosimple)

    portForwardRegexp = regexp.MustCompile("Forwarding from (127.0.0.1|\\[::1\\]):([0-9]+) -> 80")
                        ^

[pohly]

Count: 16 staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go:61.2 S1008: should use 'return err != nil' instead of 'if err != nil { return true }; return false' (gosimple)

    if err != nil {
    ^

[pohly]

Count: 6 staging/src/k8s.io/apimachinery/pkg/labels/selector.go:922.5 S1009: should omit nil check; len() for k8s.io/apimachinery/pkg/labels.Set is defined as zero (gosimple)

    if ls == nil || len(ls) == 0 {
       ^

[pohly]

Count: 5 staging/src/k8s.io/apiserver/pkg/cel/common/schemas.go:116.5 S1011: should replace loop with enumValues = append(enumValues, prop.Enum()...) (gosimple)

                for _, e := range prop.Enum() {
                ^

[pohly]

Count: 1 staging/src/k8s.io/kubectl/pkg/drain/filters.go:250.7 S1012: should use time.Since instead of time.Now().Sub (gosimple)

        int(time.Now().Sub(pod.ObjectMeta.GetDeletionTimestamp().Time).Seconds()) > skipDeletedTimeoutSeconds
            ^

[pohly]

Count: 2 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/algorithm.go:58.7 S1016: should convert opts (type CoerceOptions) to coercer instead of using struct literal (gosimple)

    c := coercer{DropInvalidFields: opts.DropInvalidFields, ReturnUnknownFieldPaths: opts.ReturnUnknownFieldPaths}
         ^

[pohly]

Count: 2 staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go:101.2 S1017: should replace this if statement with an unconditional strings.TrimSuffix (gosimple)

    if strings.HasSuffix(name, ".") {
    ^

[pohly]

Count: 3 staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go:443.27 S1019: should use make([]interface{}, size) instead (gosimple)

    s := make([]interface{}, size, size)
                             ^

[pohly]

Count: 15 staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go:153.2 S1021: should merge variable declaration with assignment on next line (gosimple)

    var agg Aggregate
    ^

[pohly]

Count: 8 staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json_test.go:76.2 S1023: redundant return statement (gosimple)

    return
    ^

[pohly]

Count: 3 staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go:151.15 S1024: should use time.Until instead of t.Sub(time.Now()) (gosimple)

    remaining := req.TLS.PeerCertificates[0].NotAfter.Sub(time.Now())
                 ^

[pohly]

Count: 5 staging/src/k8s.io/apiserver/pkg/audit/union.go:68.43 S1025: should use String() instead of fmt.Sprintf (gosimple)

        backendStrings = append(backendStrings, fmt.Sprintf("%s", backend))
                                                ^

[pohly]

Count: 3 staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go:356.11 S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (gosimple)

            return errors.New(fmt.Sprintf("invalid DeleteRange response: %v", txnResp.Responses))
                   ^

[pohly]

Count: 15 test/e2e/framework/metrics/e2e_metrics.go:105.9 S1030: should use formatted.String() instead of string(formatted.Bytes()) (gosimple)

    return string(formatted.Bytes())
           ^

[pohly]

Count: 1 staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go:1790.3 S1033: unnecessary guard around call to delete (gosimple)

        if _, ok := sourceRanges[defaultLoadBalancerSourceRanges]; ok {
        ^

[pohly]

Count: 4 staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go:539.8 S1034(related information): could eliminate this type assertion (gosimple)

        e := err.(*exec.ExitError)
             ^

[pohly]

Count: 4 staging/src/k8s.io/legacy-cloud-providers/azure/clients/armclient/azure_armclient_test.go:308.4 S1035: calling net/http.CanonicalHeaderKey on the 'key' argument of (net/http.Header).Set is redundant (gosimple)

            rw.Header().Set(http.CanonicalHeaderKey("Azure-AsyncOperation"),
            ^

[pohly]

Count: 1 staging/src/k8s.io/client-go/util/flowcontrol/throttle_test.go:171.3 S1038: should use t.Logf(...) instead of t.Log(fmt.Sprintf(...)) (gosimple)

        t.Log(fmt.Sprintf("wait err: %v", err))
        ^

[pohly]

Count: 54 pkg/registry/authentication/tokenreview/storage.go:80.39 S1039: unnecessary use of fmt.Sprintf (gosimple)

        return nil, apierrors.NewBadRequest(fmt.Sprintf("token is required for TokenReview in authentication"))
                                            ^

[pohly]

Count: 501 pkg/apis/certificates/helpers.go:65.63 SA1019: sets.String is deprecated: use generic Set instead. new ways: s1 := Set[string]{} s2 := New[string]() (staticcheck)

func IsKubeletServingCSR(req *x509.CertificateRequest, usages sets.String) bool {
                                                              ^