Merge pull request #2389 from kolyshkin/golangci-v2

Switch to golangci-lint v2, cleanup config, fix new linter warnings

common/.github/workflows/validate.yml

@@ -11,7 +11,7 @@ on:
 permissions: read-all
 
 env:
-  LINT_VERSION: v1.64.8
+  LINT_VERSION: v2.0
 
 jobs:
   codespell:
@@ -37,13 +37,13 @@ jobs:
         sudo apt-get -qq update
         sudo apt-get -qq install libseccomp-dev
     - name: lint
-      uses: golangci/golangci-lint-action@v6
+      uses: golangci/golangci-lint-action@v7
       with:
         version: "${{ env.LINT_VERSION }}"
         args: --verbose
     # Extra linters, only checking new code from a pull request.
     - name: lint-extra
-      uses: golangci/golangci-lint-action@v6
+      uses: golangci/golangci-lint-action@v7
       with:
         args: --config=.golangci-extra.yml
         version: "${{ env.LINT_VERSION }}"

common/.golangci-extra.yml

@@ -6,7 +6,10 @@
 # The idea is to impose additional rules for newly added code only
 # (rules we can not realistically satisfy for existing code).
 
+version: "2"
+
 run:
+  timeout: 5m
   build-tags:
     - apparmor
     - seccomp
@@ -14,9 +17,13 @@ run:
     - systemd
     - exclude_graphdriver_btrfs
     - containers_image_openpgp
-  timeout: 5m
 
 linters:
-  disable-all: true
+  default: none
   enable:
     - godot
+    - staticcheck
+  settings:
+    staticcheck:
+      checks:
+        - all

common/.golangci.yml

@@ -1,5 +1,7 @@
----
+version: "2"
+
 run:
+  timeout: 5m
   build-tags:
     - apparmor
     - seccomp
@@ -8,81 +10,80 @@ run:
     - exclude_graphdriver_btrfs
     - containers_image_openpgp
     - cni
-  timeout: 5m
+
+formatters:
+  enable:
+    - gofumpt
+
 linters:
-  enable-all: true
-  disable:
-    # linters explicitly disabled for the below mentioned reasons ...
-    - funlen # too aggressive/wishful size/statement limit
-    - gochecknoinits # too many hard to fix init() funcs across the code
-    - gocognit # too aggressive default
-    - wsl # useful but too opinionated
-    # others to be re-enabled one-by-one ...
-    - goconst
-    - godox
-    - lll
-    - nestif
-    - cyclop
-    - depguard
-    - errchkjson
-    - errname
-    - errorlint
-    - exhaustive
-    - gochecknoglobals
-    - err113
-    - nolintlint
-    - wrapcheck
-    - varnamelen
-    - testpackage
-    - tenv
-    - tagliatelle
-    - stylecheck
-    - paralleltest
-    - nonamedreturns
-    - nlreturn
-    - nakedret
-    - musttag
-    - maintidx
-    - ireturn
-    - exhaustruct
-    - gosec
-    - godot
-    - gocyclo
-    - dogsled
-    - tparallel
-    - thelper
-    - mnd #way to many false positives
-    - nilnil
-    - nilerr
-    - interfacebloat
-    - forcetypeassert
-    - gomoddirectives
-    - testifylint # way to many issues to fix right now, however it is a great linter for better test errors
-    - inamedparam # opinionated style
-linters-settings:
+  enable:
+    - asasalint
+    - asciicheck
+    - bidichk
+    - bodyclose
+    - canonicalheader
+    - containedctx
+    - contextcheck
+    - copyloopvar
+    - decorder
+    - dupl
+    - dupword
+    - durationcheck
+    - exptostd
+    - fatcontext
+    - forbidigo
+    - ginkgolinter
+    - gocheckcompilerdirectives
+    - gochecksumtype
+    - gocritic
+    - goprintffuncname
+    - gosmopolitan
+    - iface
+    - intrange
+    - makezero
+    - mirror
+    - misspell
+    - nilnesserr
+    - noctx
+    - nosprintfhostport
+    - perfsprint
+    - prealloc
+    - predeclared
+    - reassign
+    - recvcheck
+    - revive
+    - unconvert
+    - unparam
+    - usestdlibvars
+    - usetesting
+    - wastedassign
+    - whitespace
+  settings:
     errcheck:
       check-type-assertions: true
     gocyclo:
       min-complexity: 35
-  gofmt:
-    rewrite-rules:
-      - pattern: 'interface{}'
-        replacement: 'any'
     revive:
       rules:
         - name: dot-imports
           disabled: true
-
-issues:
-    # Excluding configuration per-path, per-linter, per-text and per-source
-  exclude-rules:
-    # Exclude some linters from running on tests files.
-    - path: _test\.go
-      linters:
+    staticcheck:
+      checks:
+        - all
+        - -ST1003 # https://staticcheck.dev/docs/checks/#ST1003 Poorly chosen identifier.
+        - -QF1008 # https://staticcheck.dev/docs/checks/#QF1008 Omit embedded fields from selector expression.
+  exclusions:
+    generated: strict
+    presets:
+      - comments
+      - common-false-positives
+      - std-error-handling
+    rules:
+      - linters:
           - dupl
           - perfsprint
-    # Exclude "should pass the context parameter" for libimage.LookupImage because of backward compatibility.
-    - path: "libimage"
-      text: "LookupImage"
-      linters:
+        path: _test\.go
+      - linters:
           - contextcheck
+        path: libimage
+        text: LookupImage

common/libimage/image.go

@@ -463,13 +463,13 @@ func (i *Image) removeRecursive(ctx context.Context, rmMap map[string]*RemoveIma
 	skipRemove := false
 	numNames := len(i.Names())
 
-	// NOTE: the `numNames == 1` check is not only a performance
+	// NOTE: the `numNames != 1` check is not only a performance
 	// optimization but also preserves existing Podman/Docker behaviour.
 	// If image "foo" is used by a container and has only this tag/name,
 	// an `rmi foo` will not untag "foo" but instead attempt to remove the
 	// entire image.  If there's a container using "foo", we should get an
 	// error.
-	if !(referencedBy == "" || numNames == 1) {
+	if referencedBy != "" && numNames != 1 {
 		byID := strings.HasPrefix(i.ID(), referencedBy)
 		byDigest := strings.HasPrefix(referencedBy, "sha256:")
 		if !options.Force {

common/libimage/load.go

@@ -115,7 +115,7 @@ func (r *Runtime) Load(ctx context.Context, path string, options *LoadOptions) (
 
 	// Give a decent error message if nothing above worked.
 	// we want the colon here for the multiline error
-	//nolint:revive
+	//nolint:revive,staticcheck
 	loadError := errors.New("payload does not match any of the supported image formats:")
 	for _, err := range loadErrors {
 		loadError = fmt.Errorf("%v\n * %v", loadError, err)

common/libimage/normalize.go

@@ -30,7 +30,7 @@ func NormalizeName(name string) (reference.Named, error) {
 
 	// Enforce "localhost" if needed.
 	registry := reference.Domain(named)
-	if !(strings.ContainsAny(registry, ".:") || registry == "localhost") {
+	if !strings.ContainsAny(registry, ".:") && registry != "localhost" {
 		name = toLocalImageName(ref.String())
 	}
 

common/libimage/search.go

@@ -217,7 +217,7 @@ func (r *Runtime) searchImageInRegistry(ctx context.Context, term, registry stri
 	paramsArr := []SearchResult{}
 	for i := range limit {
 		// Check whether query matches filters
-		if !(filterMatchesAutomatedFilter(&options.Filter, results[i]) && filterMatchesOfficialFilter(&options.Filter, results[i]) && filterMatchesStarFilter(&options.Filter, results[i])) {
+		if !filterMatchesAutomatedFilter(&options.Filter, results[i]) || !filterMatchesOfficialFilter(&options.Filter, results[i]) || !filterMatchesStarFilter(&options.Filter, results[i]) {
 			continue
 		}
 		official := ""

common/libnetwork/cni/network.go

@@ -191,7 +191,7 @@ func (n *cniNetwork) loadNetworks() error {
 		}
 
 		if !types.NameRegex.MatchString(conf.Name) {
-			logrus.Warnf("CNI config list %s has invalid name, skipping: %v", file, types.RegexError)
+			logrus.Warnf("CNI config list %s has invalid name, skipping: %v", file, types.ErrInvalidName)
 			continue
 		}
 

common/libnetwork/internal/util/bridge.go

@@ -19,7 +19,7 @@ func CreateBridge(n NetUtil, network *types.Network, usedNetworks []*net.IPNet,
 			}
 		}
 		if !types.NameRegex.MatchString(network.NetworkInterface) {
-			return fmt.Errorf("bridge name %s invalid: %w", network.NetworkInterface, types.RegexError)
+			return fmt.Errorf("bridge name %s invalid: %w", network.NetworkInterface, types.ErrInvalidName)
 		}
 	} else {
 		var err error

common/libnetwork/internal/util/create.go

@@ -23,7 +23,7 @@ func CommonNetworkCreate(n NetUtil, network *types.Network) error {
 	// validate the name when given
 	if network.Name != "" {
 		if !types.NameRegex.MatchString(network.Name) {
-			return fmt.Errorf("network name %s invalid: %w", network.Name, types.RegexError)
+			return fmt.Errorf("network name %s invalid: %w", network.Name, types.ErrInvalidName)
 		}
 		if _, err := n.Network(network.Name); err == nil {
 			return fmt.Errorf("network name %s already used: %w", network.Name, types.ErrNetworkExists)

common/libnetwork/netavark/config.go

@@ -294,10 +294,7 @@ func createIpvlanOrMacvlan(network *types.Network) error {
 	}
 
 	driver := network.Driver
-	isMacVlan := true
-	if driver == types.IPVLANNetworkDriver {
-		isMacVlan = false
-	}
+	isMacVlan := driver != types.IPVLANNetworkDriver
 
 	// always turn dns off with macvlan, it is not implemented in netavark
 	// and makes little sense to support with macvlan

common/libnetwork/netavark/network.go

@@ -248,7 +248,7 @@ func (n *netavarkNetwork) loadNetworks() error {
 		}
 
 		if !types.NameRegex.MatchString(network.Name) {
-			logrus.Warnf("Network config %q has invalid name: %q, skipping: %v", path, network.Name, types.RegexError)
+			logrus.Warnf("Network config %q has invalid name: %q, skipping: %v", path, network.Name, types.ErrInvalidName)
 			continue
 		}
 

common/libnetwork/slirp4netns/slirp4netns.go

@@ -682,7 +682,7 @@ func openSlirp4netnsPort(apiSocket, proto, hostip string, hostport, guestport ui
 	if err != nil {
 		return fmt.Errorf("cannot marshal JSON for slirp4netns: %w", err)
 	}
-	if _, err := conn.Write([]byte(fmt.Sprintf("%s\n", data))); err != nil {
+	if _, err := fmt.Fprintf(conn, "%s\n", data); err != nil {
 		return fmt.Errorf("cannot write to control socket %s: %w", apiSocket, err)
 	}
 	//nolint:errcheck // This cast should never fail, if it does we get a interface

common/libnetwork/types/define.go

@@ -24,8 +24,10 @@ var (
 	// NameRegex is a regular expression to validate names.
 	// This must NOT be changed.
 	NameRegex = regexp.Delayed("^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
-	// RegexError is thrown in presence of an invalid name.
-	RegexError = fmt.Errorf("names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*: %w", ErrInvalidArg) // nolint:revive // This lint is new and we do not want to break the API.
+	// ErrInvalidName is thrown in presence of an invalid name.
+	ErrInvalidName = fmt.Errorf("names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*: %w", ErrInvalidArg)
+	// Deprecated: use [ErrInvalidName] instead.
+	RegexError = ErrInvalidName
 
 	// NotHexRegex is a regular expression to check if a string is
 	// a hexadecimal string.

common/pkg/auth/auth.go

@@ -173,10 +173,10 @@ func Login(ctx context.Context, systemContext *types.SystemContext, opts *LoginO
 	if opts.StdinPassword {
 		var stdinPasswordStrBuilder strings.Builder
 		if opts.Password != "" {
-			return errors.New("Can't specify both --password-stdin and --password")
+			return errors.New("can't specify both --password-stdin and --password")
 		}
 		if opts.Username == "" {
-			return errors.New("Must provide --username with --password-stdin")
+			return errors.New("must provide --username with --password-stdin")
 		}
 		scanner := bufio.NewScanner(opts.Stdin)
 		for scanner.Scan() {

common/pkg/cgroups/utils_linux.go

@@ -221,7 +221,7 @@ func MoveUnderCgroup(cgroup, subtree string, processes []uint32) error {
 		}
 
 		// root cgroup, skip it
-		if parts[2] == "/" && !(unifiedMode && parts[1] == "") {
+		if parts[2] == "/" && (!unifiedMode || parts[1] != "") {
 			continue
 		}
 
@@ -261,7 +261,7 @@ func MoveUnderCgroup(cgroup, subtree string, processes []uint32) error {
 
 		if len(processes) > 0 {
 			for _, pid := range processes {
-				if _, err := f.WriteString(fmt.Sprintf("%d\n", pid)); err != nil {
+				if _, err := fmt.Fprintf(f, "%d\n", pid); err != nil {
 					logrus.Debugf("Cannot move process %d to cgroup %q: %v", pid, newCgroup, err)
 				}
 			}

common/pkg/configmaps/configmaps.go

@@ -132,7 +132,7 @@ func (s *ConfigMapManager) Store(name string, data []byte, driverType string, dr
 		return "", err
 	}
 
-	if !(len(data) > 0 && len(data) < maxConfigMapSize) {
+	if len(data) == 0 || len(data) >= maxConfigMapSize {
 		return "", errDataSize
 	}
 

common/pkg/parse/parse_unix.go

@@ -17,7 +17,7 @@ func DeviceFromPath(device string) ([]devices.Device, error) {
 		return nil, err
 	}
 	if unshare.IsRootless() && src != dst {
-		return nil, fmt.Errorf("Renaming device %s to %s is not supported in rootless containers", src, dst)
+		return nil, fmt.Errorf("renaming device %s to %s is not supported in rootless containers", src, dst)
 	}
 	srcInfo, err := os.Stat(src)
 	if err != nil {

common/pkg/retry/retry.go

@@ -47,7 +47,7 @@ func IfNecessary(ctx context.Context, operation func() error, options *Options)
 		logrus.Warnf("Failed, retrying in %s ... (%d/%d). Error: %v", delay, attempt+1, options.MaxRetry, err)
 		select {
 		case <-time.After(delay):
-			break
+			// Do nothing.
 		case <-ctx.Done():
 			return err
 		}

common/pkg/secrets/secrets.go

@@ -166,7 +166,7 @@ func (s *SecretsManager) Store(name string, data []byte, driverType string, opti
 		return "", err
 	}
 
-	if !(len(data) > 0 && len(data) < maxSecretSize) {
+	if len(data) == 0 || len(data) >= maxSecretSize {
 		return "", errDataSize
 	}
 	var secr *Secret

common/pkg/secrets/secrets_test.go

@@ -64,7 +64,7 @@ func TestAddSecretAndLookupData(t *testing.T) {
 
 	s, _, err = manager.LookupSecretData("mysecret")
 	require.NoError(t, err)
-	if s.CreatedAt == s.UpdatedAt {
+	if s.CreatedAt.Equal(s.UpdatedAt) {
 		t.Errorf("error: secret CreatedAt should not equal UpdatedAt after a Replace")
 	}
 

common/pkg/subscriptions/subscriptions.go

@@ -322,7 +322,7 @@ func addSubscriptionsFromMountsFile(filePath, mountLabel, containerRunDir string
 func containerHasEtcSystemFips(subscriptionsDir, mountPoint string) (bool, error) {
 	containerEtc, err := securejoin.SecureJoin(mountPoint, "etc")
 	if err != nil {
-		return false, fmt.Errorf("Container /etc resolution error: %w", err)
+		return false, fmt.Errorf("container /etc resolution error: %w", err)
 	}
 	if fileutils.Lexists(filepath.Join(containerEtc, "system-fips")) != nil {
 		logrus.Debug("/etc/system-fips does not exist in the container, not creating /run/secrets/system-fips")
@@ -331,7 +331,7 @@ func containerHasEtcSystemFips(subscriptionsDir, mountPoint string) (bool, error
 
 	fipsFileTarget, err := securejoin.SecureJoin(mountPoint, "etc/system-fips")
 	if err != nil {
-		return false, fmt.Errorf("Container /etc/system-fips resolution error: %w", err)
+		return false, fmt.Errorf("container /etc/system-fips resolution error: %w", err)
 	}
 	if fipsFileTarget != filepath.Join(mountPoint, subscriptionsDir, "system-fips") {
 		logrus.Warnf("/etc/system-fips exists in the container, but is not a symlink to %[1]v/system-fips; not creating %[1]v/system-fips", subscriptionsDir)
@@ -448,24 +448,24 @@ func addFIPSMounts(mounts *[]rspec.Mount, containerRunDir, mountPoint, mountLabe
 	destPolicyConfig := "/etc/crypto-policies/config"
 	srcPolicyConfigOnHost, err := securejoin.SecureJoin(mountPoint, srcPolicyConfig)
 	if err != nil {
-		return fmt.Errorf("Could not expand %q in container: %w", srcPolicyConfig, err)
+		return fmt.Errorf("could not expand %q in container: %w", srcPolicyConfig, err)
 	}
 
 	if err = fileutils.Exists(srcPolicyConfigOnHost); err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
-			return fmt.Errorf("Could not check whether %q exists in container: %w", srcPolicyConfig, err)
+			return fmt.Errorf("could not check whether %q exists in container: %w", srcPolicyConfig, err)
 		}
 
 		// /usr/share/crypto-policies/default-fips-config does not exist, let's create it ourselves
 		cryptoPoliciesConfigFile := filepath.Join(containerRunDir, "fips-config")
 		if err := os.WriteFile(cryptoPoliciesConfigFile, []byte("FIPS\n"), 0o644); err != nil {
-			return fmt.Errorf("Failed to write fips config file in container for FIPS mode: %w", err)
+			return fmt.Errorf("failed to write fips config file in container for FIPS mode: %w", err)
 		}
 		if err = label.Relabel(cryptoPoliciesConfigFile, mountLabel, false); err != nil {
-			return fmt.Errorf("Failed to apply correct labels on fips config file: %w", err)
+			return fmt.Errorf("failed to apply correct labels on fips config file: %w", err)
 		}
 		if err := os.Chown(cryptoPoliciesConfigFile, uid, gid); err != nil {
-			return fmt.Errorf("Failed to chown fips config file: %w", err)
+			return fmt.Errorf("failed to chown fips config file: %w", err)
 		}
 
 		srcPolicyConfigOnHost = cryptoPoliciesConfigFile

common/pkg/timetype/timestamp.go

@@ -31,8 +31,9 @@ func GetTimestamp(value string, reference time.Time) (string, error) {
 	}
 
 	var format string
-	// if the string has a Z or a + or three dashes use parse otherwise use parseinlocation
-	parseInLocation := !(strings.ContainsAny(value, "zZ+") || strings.Count(value, "-") == 3)
+	// If the string has a Z, or a +, or three dashes,
+	// then use time.Parse, otherwise use time.ParseInLocation.
+	parseInLocation := !strings.ContainsAny(value, "zZ+") && strings.Count(value, "-") != 3
 
 	switch {
 	case strings.Contains(value, "."):

common/pkg/timezone/timezone.go

@@ -20,10 +20,10 @@ import (
 // It returns the path of the created /etc/localtime file if needed.
 func ConfigureContainerTimeZone(timezone, containerRunDir, mountPoint, etcPath, containerID string) (localTimePath string, err error) {
 	var timezonePath string
-	switch {
-	case timezone == "":
+	switch timezone {
+	case "":
 		return "", nil
-	case timezone == "local":
+	case "local":
 		timezonePath, err = filepath.EvalSymlinks("/etc/localtime")
 		if err != nil {
 			return "", fmt.Errorf("finding local timezone for container %s: %w", containerID, err)