From 5e8b0017c5b487e95ee691ce9cd75ada14991ed0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 24 Mar 2025 14:58:12 -0700 Subject: [PATCH 01/16] .golangci.yml: switch to list of enabled linters Instead of enabling all linters, and when disabling some of them because we don't like them, switch to list of explicitly enabled linters. The upside of this is we can easily upgrade golangci-lint (i.e. any new linters won't break our CI). The downside is, we need to explicitly enable extra linters. To me, this is better from the maintainability perspective. NOTE this commit does not change the configuration in any way, in other words, the list of linters being run is the same as before. The next commit will address this. Signed-off-by: Kir Kolyshkin --- common/.golangci.yml | 110 ++++++++++++++++++++++++------------------- 1 file changed, 61 insertions(+), 49 deletions(-) diff --git a/common/.golangci.yml b/common/.golangci.yml index d71b6b310b..0895721366 100644 --- a/common/.golangci.yml +++ b/common/.golangci.yml @@ -10,55 +10,67 @@ run: - cni timeout: 5m 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 + enable: + - asasalint + - asciicheck + - bidichk + - bodyclose + - canonicalheader + - containedctx + - contextcheck + - copyloopvar + - decorder + - dupl + - dupword + - durationcheck + - exptostd + - fatcontext + - forbidigo + - gci + - ginkgolinter + - gocheckcompilerdirectives + - gochecksumtype + - gocritic + - gofmt + - gofumpt + - goheader + - goimports + - gomodguard + - goprintffuncname + - gosmopolitan + - grouper + - iface + - importas + - intrange + - loggercheck + - makezero + - mirror + - misspell + - nilnesserr + - noctx + - nosprintfhostport + - perfsprint + - prealloc + - predeclared + - promlinter + - protogetter + - reassign + - recvcheck + - revive + - rowserrcheck + - sloglint + - spancheck + - sqlclosecheck + - tagalign + - testableexamples + - unconvert + - unparam + - usestdlibvars + - usetesting + - wastedassign + - whitespace + - zerologlint + linters-settings: errcheck: check-type-assertions: true From 9a5008a1ca7a45ab2441ae7ce902768bb8a48a77 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 24 Mar 2025 15:13:30 -0700 Subject: [PATCH 02/16] .golangci.yml: remove some linters Let's take a look at each enabled linter, and find out if it's needed; remove those that make no sense. * gci: formats imports, probably superseded by gofumpt; * gofmt: is a subset of gofumpt; * goheader: does nothing with empty configuration; * goimports: functionality should be covered by gofumpt; * gomodguard: does nothing with empty configuration; * grouper: formats imports, probably superseded by gofumpt; * importas: does nothing with empty configuration; * loggercheck: this repo does not use any loggers it checks (kitlog,klog,logr,zap); * promlinter: this repo does not use Prometheus; * protogetter: this repo does not use protobuf; * rowserrcheck: this repo does not use sql; * sloginit: this repo does not use slog; * spancheck: this repo does not use opentelemetry/opencensus; * sqlclosecheck: this repo does not use sql; * tagalign: this repo does not use multiple struct tags; * testableexamples: this repo does not have any examples; * zerologlint: this repo does not use zerolog. Signed-off-by: Kir Kolyshkin --- common/.golangci.yml | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/common/.golangci.yml b/common/.golangci.yml index 0895721366..68c3b12be8 100644 --- a/common/.golangci.yml +++ b/common/.golangci.yml @@ -26,23 +26,15 @@ linters: - exptostd - fatcontext - forbidigo - - gci - ginkgolinter - gocheckcompilerdirectives - gochecksumtype - gocritic - - gofmt - gofumpt - - goheader - - goimports - - gomodguard - goprintffuncname - gosmopolitan - - grouper - iface - - importas - intrange - - loggercheck - makezero - mirror - misspell @@ -52,24 +44,15 @@ linters: - perfsprint - prealloc - predeclared - - promlinter - - protogetter - reassign - recvcheck - revive - - rowserrcheck - - sloglint - - spancheck - - sqlclosecheck - - tagalign - - testableexamples - unconvert - unparam - usestdlibvars - usetesting - wastedassign - whitespace - - zerologlint linters-settings: errcheck: From 48882d0bb9e0be0ddb1e12118f7a25f2a46a6e4c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:12:43 -0700 Subject: [PATCH 03/16] pkg/secrets: use time.Equal Fix the following warning, as reported by staticcheck from golangci-lint v2.0.1: > pkg/secrets/secrets_test.go:67:5: QF1009: probably want to use time.Time.Equal instead (staticcheck) > if s.CreatedAt == s.UpdatedAt { > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/secrets/secrets_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/pkg/secrets/secrets_test.go b/common/pkg/secrets/secrets_test.go index 7677d7191c..550bf5ee89 100644 --- a/common/pkg/secrets/secrets_test.go +++ b/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") } From d524bfaf6a7a35cfaa8e3500a55eb3f5045875e6 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:17:37 -0700 Subject: [PATCH 04/16] pkg/cgroups: simplify write Fix the following staticcheck warning: > pkg/cgroups/utils_linux.go:264:18: QF1012: Use fmt.Fprintf(...) instead of WriteString(fmt.Sprintf(...)) (staticcheck) > if _, err := f.WriteString(fmt.Sprintf("%d\n", pid)); err != nil { > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/cgroups/utils_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/pkg/cgroups/utils_linux.go b/common/pkg/cgroups/utils_linux.go index 848a59d236..20f1919306 100644 --- a/common/pkg/cgroups/utils_linux.go +++ b/common/pkg/cgroups/utils_linux.go @@ -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) } } From 41e8b24890a8097b6c86512d5e155dc61a578b4d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:18:33 -0700 Subject: [PATCH 05/16] libnetwork: simplify write Fix the following staticcheck warning: > libnetwork/slirp4netns/slirp4netns.go:685:15: QF1012: Use fmt.Fprintf(...) instead of Write([]byte(fmt.Sprintf(...))) (staticcheck) > if _, err := conn.Write([]byte(fmt.Sprintf("%s\n", data))); err != nil { > ^ Signed-off-by: Kir Kolyshkin --- common/libnetwork/slirp4netns/slirp4netns.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/libnetwork/slirp4netns/slirp4netns.go b/common/libnetwork/slirp4netns/slirp4netns.go index 524cabc42a..afb54f60d6 100644 --- a/common/libnetwork/slirp4netns/slirp4netns.go +++ b/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 From f3877451b3d6e84e50caa91c4c74980e0a818296 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:20:44 -0700 Subject: [PATCH 06/16] pkg/timezone: simplify switch This fixes the following staticcheck warning: > pkg/timezone/timezone.go:23:2: QF1002: could use tagged switch on timezone (staticcheck) > switch { > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/timezone/timezone.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/common/pkg/timezone/timezone.go b/common/pkg/timezone/timezone.go index b82dc309d0..e606614578 100644 --- a/common/pkg/timezone/timezone.go +++ b/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) From 555b81763028ca2333074d665d00b0944f633cf6 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:26:31 -0700 Subject: [PATCH 07/16] libnetwork/types: rename RegexError to ErrInvalidName ... and add a deprecated alias so backward compatibility is still preserved (and users can gradually switch to the new name). Done because this is now also reported by staticcheck (in addition to revive) linter. Signed-off-by: Kir Kolyshkin --- common/libnetwork/cni/network.go | 2 +- common/libnetwork/internal/util/bridge.go | 2 +- common/libnetwork/internal/util/create.go | 2 +- common/libnetwork/netavark/network.go | 2 +- common/libnetwork/types/define.go | 6 ++++-- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/common/libnetwork/cni/network.go b/common/libnetwork/cni/network.go index 7e001fab0e..ab3a05b555 100644 --- a/common/libnetwork/cni/network.go +++ b/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 } diff --git a/common/libnetwork/internal/util/bridge.go b/common/libnetwork/internal/util/bridge.go index 44cd555c17..e69c388290 100644 --- a/common/libnetwork/internal/util/bridge.go +++ b/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 diff --git a/common/libnetwork/internal/util/create.go b/common/libnetwork/internal/util/create.go index 376a57315b..42609d1ba7 100644 --- a/common/libnetwork/internal/util/create.go +++ b/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) diff --git a/common/libnetwork/netavark/network.go b/common/libnetwork/netavark/network.go index 985d0db2dd..cf8b0d490e 100644 --- a/common/libnetwork/netavark/network.go +++ b/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 } diff --git a/common/libnetwork/types/define.go b/common/libnetwork/types/define.go index 0bc688103e..e5929c3e87 100644 --- a/common/libnetwork/types/define.go +++ b/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. From 14c831e00378e4609be44f37e0c56f2af950d225 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:31:18 -0700 Subject: [PATCH 08/16] libnetwork/netavark: simplify isMacVlan init This fixes the following staticcheck warning: > libnetwork/netavark/config.go:297:2: QF1007: could merge conditional assignment into variable declaration (staticcheck) > isMacVlan := true > ^ Signed-off-by: Kir Kolyshkin --- common/libnetwork/netavark/config.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/common/libnetwork/netavark/config.go b/common/libnetwork/netavark/config.go index 4916f725e1..6c17bd0a19 100644 --- a/common/libnetwork/netavark/config.go +++ b/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 From 2a6974f3343569a21b3945443f98143d336e5a4e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:37:15 -0700 Subject: [PATCH 09/16] pkg/timetype: fix linter warning, improve comment Fix the following staticcheck warning: > pkg/timetype/timestamp.go:35:21: QF1001: could apply De Morgan's law (staticcheck) > parseInLocation := !(strings.ContainsAny(value, "zZ+") || strings.Count(value, "-") == 3) > ^ While at it, improve the comment. Signed-off-by: Kir Kolyshkin --- common/pkg/timetype/timestamp.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/common/pkg/timetype/timestamp.go b/common/pkg/timetype/timestamp.go index 519884c553..ce03c784c5 100644 --- a/common/pkg/timetype/timestamp.go +++ b/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, "."): From 566c24e7842c9906b8950d35e04f3c2429d07ee9 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:38:37 -0700 Subject: [PATCH 10/16] pkg: apply De Morgan's law This fixes the following staticcheck warnings: > pkg/configmaps/configmaps.go:135:5: QF1001: could apply De Morgan's law (staticcheck) > if !(len(data) > 0 && len(data) < maxConfigMapSize) { > ^ > pkg/secrets/secrets.go:169:5: QF1001: could apply De Morgan's law (staticcheck) > if !(len(data) > 0 && len(data) < maxSecretSize) { > ^ as well as the subsequent gocritic warnings: > pkg/configmaps/configmaps.go:135:5: sloppyLen: len(data) <= 0 can be len(data) == 0 (gocritic) > if len(data) <= 0 || len(data) >= maxConfigMapSize { > ^ > pkg/secrets/secrets.go:169:5: sloppyLen: len(data) <= 0 can be len(data) == 0 (gocritic) > if len(data) <= 0 || len(data) >= maxSecretSize { > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/configmaps/configmaps.go | 2 +- common/pkg/secrets/secrets.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/pkg/configmaps/configmaps.go b/common/pkg/configmaps/configmaps.go index 092cb01534..8fc831c8ea 100644 --- a/common/pkg/configmaps/configmaps.go +++ b/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 } diff --git a/common/pkg/secrets/secrets.go b/common/pkg/secrets/secrets.go index 113c6c43b3..7d8d33a114 100644 --- a/common/pkg/secrets/secrets.go +++ b/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 From 9da372fd2ce1e41d79461a0e45189ab60fa522c4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:42:23 -0700 Subject: [PATCH 11/16] libimage: apply De Morgan's law This fixes the following staticcheck warnings: > libimage/image.go:463:5: QF1001: could apply De Morgan's law (staticcheck) > if !(referencedBy == "" || numNames == 1) { > ^ > libimage/normalize.go:33:5: QF1001: could apply De Morgan's law (staticcheck) > if !(strings.ContainsAny(registry, ".:") || registry == "localhost") { > ^ > libimage/search.go:220:6: QF1001: could apply De Morgan's law (staticcheck) > if !(filterMatchesAutomatedFilter(&options.Filter, results[i]) && filterMatchesOfficialFilter(&options.Filter, results[i]) && filterMatchesStarFilter(&options.Filter, results[i])) { > ^ Signed-off-by: Kir Kolyshkin --- common/libimage/image.go | 4 ++-- common/libimage/normalize.go | 2 +- common/libimage/search.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/libimage/image.go b/common/libimage/image.go index 91e694c16e..8f300e8399 100644 --- a/common/libimage/image.go +++ b/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 { diff --git a/common/libimage/normalize.go b/common/libimage/normalize.go index b00af66a01..1dab82fe27 100644 --- a/common/libimage/normalize.go +++ b/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()) } diff --git a/common/libimage/search.go b/common/libimage/search.go index 6ab016b3b5..2745b52bb3 100644 --- a/common/libimage/search.go +++ b/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 := "" From 163ec20d8a193d0a3ca6307413688a9f176764f8 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:46:09 -0700 Subject: [PATCH 12/16] pkg/cgroups: apply De Morgan's law This fixes the following staticcheck warning: > pkg/cgroups/utils_linux.go:224:25: QF1001: could apply De Morgan's law (staticcheck) > if parts[2] == "/" && !(unifiedMode && parts[1] == "") { > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/cgroups/utils_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/pkg/cgroups/utils_linux.go b/common/pkg/cgroups/utils_linux.go index 20f1919306..505ac1b29f 100644 --- a/common/pkg/cgroups/utils_linux.go +++ b/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 } From a25e5a56a89d33846a4cfebe8811e65ed1a8f84b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:49:03 -0700 Subject: [PATCH 13/16] libimage: silence a staticcheck warning Signed-off-by: Kir Kolyshkin --- common/libimage/load.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/libimage/load.go b/common/libimage/load.go index 48f255e8b1..2f69fd639b 100644 --- a/common/libimage/load.go +++ b/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) From 8b1718ffeeebf0c5179cbdfe899a70c0ac370ff4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:50:08 -0700 Subject: [PATCH 14/16] pkg: do not capitalize error strings This fixes the following linter warnings: > pkg/auth/auth.go:176:11: ST1005: error strings should not be capitalized (staticcheck) > return errors.New("Can't specify both --password-stdin and --password") > ^ > pkg/auth/auth.go:179:11: ST1005: error strings should not be capitalized (staticcheck) > return errors.New("Must provide --username with --password-stdin") > ^ > pkg/subscriptions/subscriptions.go:325:17: ST1005: error strings should not be capitalized (staticcheck) > return false, fmt.Errorf("Container /etc resolution error: %w", err) > ^ > pkg/subscriptions/subscriptions.go:325:17: ST1005: error strings should not be capitalized (staticcheck) > return false, fmt.Errorf("Container /etc resolution error: %w", err) > ^ > pkg/subscriptions/subscriptions.go:334:17: ST1005: error strings should not be capitalized (staticcheck) > return false, fmt.Errorf("Container /etc/system-fips resolution error: %w", err) > ^ > pkg/subscriptions/subscriptions.go:451:10: ST1005: error strings should not be capitalized (staticcheck) > return fmt.Errorf("Could not expand %q in container: %w", srcPolicyConfig, err) > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/auth/auth.go | 4 ++-- common/pkg/parse/parse_unix.go | 2 +- common/pkg/subscriptions/subscriptions.go | 14 +++++++------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/pkg/auth/auth.go b/common/pkg/auth/auth.go index a3d333a99a..4bcd490db3 100644 --- a/common/pkg/auth/auth.go +++ b/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() { diff --git a/common/pkg/parse/parse_unix.go b/common/pkg/parse/parse_unix.go index 86563b3eb6..3e4e1d140d 100644 --- a/common/pkg/parse/parse_unix.go +++ b/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 { diff --git a/common/pkg/subscriptions/subscriptions.go b/common/pkg/subscriptions/subscriptions.go index 2c78cf1e65..202eb6d3c5 100644 --- a/common/pkg/subscriptions/subscriptions.go +++ b/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 From 702b726c215b09bcf992ae7868a9c721512806bc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 16:56:01 -0700 Subject: [PATCH 15/16] pkg/retry: rm unused break This fixes the following linter warning: > pkg/retry/retry.go:50:4: SA4011: ineffective break statement. Did you mean to break out of the outer loop? (staticcheck) > break > ^ Signed-off-by: Kir Kolyshkin --- common/pkg/retry/retry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/pkg/retry/retry.go b/common/pkg/retry/retry.go index d47e47e101..adc42c3b34 100644 --- a/common/pkg/retry/retry.go +++ b/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 } From 670d57ae83d745fba1ab1b14fb65e292c868df1c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 25 Mar 2025 17:04:44 -0700 Subject: [PATCH 16/16] ci: switch to golangci-lint v2.0 The new configuration files were initially generated by `golangci-lint migrate`, when tweaked to minimize and simplify. golangci-lint v2 switches to a new version of staticcheck which shows much more warnings. Some of them were fixed by a few previous commits, and the rest of them are disabled. Also, golangci-extra was modified to include ALL staticcheck linters. Signed-off-by: Kir Kolyshkin --- common/.github/workflows/validate.yml | 6 +-- common/.golangci-extra.yml | 11 ++++- common/.golangci.yml | 66 +++++++++++++++------------ 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/common/.github/workflows/validate.yml b/common/.github/workflows/validate.yml index 14eb0ce22f..28b2413c1d 100644 --- a/common/.github/workflows/validate.yml +++ b/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 }}" diff --git a/common/.golangci-extra.yml b/common/.golangci-extra.yml index 849b7fcb44..f77d1811f6 100644 --- a/common/.golangci-extra.yml +++ b/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 diff --git a/common/.golangci.yml b/common/.golangci.yml index 68c3b12be8..c5e98a9909 100644 --- a/common/.golangci.yml +++ b/common/.golangci.yml @@ -1,5 +1,7 @@ ---- +version: "2" + run: + timeout: 5m build-tags: - apparmor - seccomp @@ -8,7 +10,11 @@ run: - exclude_graphdriver_btrfs - containers_image_openpgp - cni - timeout: 5m + +formatters: + enable: + - gofumpt + linters: enable: - asasalint @@ -30,7 +36,6 @@ linters: - gocheckcompilerdirectives - gochecksumtype - gocritic - - gofumpt - goprintffuncname - gosmopolitan - iface @@ -53,31 +58,32 @@ linters: - usetesting - wastedassign - whitespace - -linters-settings: - errcheck: - check-type-assertions: true - gocyclo: - min-complexity: 35 - gofmt: - rewrite-rules: - - pattern: 'interface{}' - replacement: 'any' - revive: + settings: + errcheck: + check-type-assertions: true + gocyclo: + min-complexity: 35 + revive: + rules: + - name: dot-imports + disabled: true + 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: - - 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: - - dupl - - perfsprint - # Exclude "should pass the context parameter" for libimage.LookupImage because of backward compatibility. - - path: "libimage" - text: "LookupImage" - linters: - - contextcheck + - linters: + - dupl + - perfsprint + path: _test\.go + - linters: + - contextcheck + path: libimage + text: LookupImage