clean-up "nolint" comments, remove unused ones, update golangci-lint #7349

thaJeztah · 2022-08-31T07:48:49Z

see individual commits for details

k8s-ci-robot · 2022-08-31T07:48:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

thaJeztah · 2022-10-08T12:08:20Z

cmd/containerd-stress/main.go

+		srv := &http.Server{
+			Addr:              c.Metrics,
+			Handler:           metrics.Handler(),
+			ReadHeaderTimeout: 5 * time.Minute, // "G112: Potential Slowloris Attack (gosec)"; not a real concern for our use, so setting a long timeout.
+		}
+		if err := srv.ListenAndServe(); err != nil {


Taking similar approach as I dd in moby/moby#44174 (we discussed this warning there, and considered that it was not an actual problem for how it's used, but setting a timeout doesn't hurt either, and somewhat better than "just add some //nolint comments")

thaJeztah · 2022-10-08T12:52:13Z

cmd/containerd/command/service_windows.go

 	}

-	interactive, err := svc.IsAnInteractiveSession() // nolint:staticcheck
+	interactive, err := svc.IsAnInteractiveSession() //nolint:staticcheck


This one was added in edc671d (#4626). I'll open a separate PR to replace it

thaJeztah · 2022-10-08T13:50:15Z

pkg/cri/server/image_pull.go

 			tlsConfig.Certificates = []tls.Certificate{cert}
 		}
-		tlsConfig.BuildNameToCertificate() // nolint:staticcheck
+		tlsConfig.BuildNameToCertificate() //nolint:staticcheck


This //nolint was added in containerd/cri@f5c7ac9 (containerd/cri#1517)
to suppress warnings about the NameToCertificate function being deprecated:

// Deprecated: NameToCertificate only allows associating a single certificate // with a given name. Leave that field nil to let the library select the first // compatible chain from Certificates.

Looking at that, I see it was deprecated in Go 1.14 through golang/go@eb93c68 (https://go-review.googlesource.com/c/go/+/205059), which describes:

crypto/tls: select only compatible chains from Certificates Now that we have a full implementation of the logic to check certificate compatibility, we can let applications just list multiple chains in Certificates (for example, an RSA and an ECDSA one) and choose the most appropriate automatically. NameToCertificate only maps each name to one chain, so simply deprecate it, and while at it simplify its implementation by not stripping trailing dots from the SNI (which is specified not to have any, see RFC 6066, Section 3) and by not supporting multi-level wildcards, which are not a thing in the WebPKI (and in crypto/x509).

I think we should at least have a comment describing why we're ignoring this, but preferably review whether we should still use it. I'm kinda horrible on TLS stuff, so perhaps someone else would be able to assist on this (I'm happy to open a pull request to make the actual changes).

thaJeztah · 2022-10-08T13:50:58Z

pkg/cri/sbserver/image_pull.go

 			tlsConfig.Certificates = []tls.Certificate{cert}
 		}
-		tlsConfig.BuildNameToCertificate() // nolint:staticcheck
+		tlsConfig.BuildNameToCertificate() //nolint:staticcheck


same as https://github.com/containerd/containerd/pull/7349/files#r990644833 (this code was forked from pkg/cri/server

thaJeztah · 2022-10-08T15:44:17Z

This one should be ready for review 👍

@samuelkarp @estesp @mikebrow PTAL 🤗

mikebrow

just a nit on FIXME->TODO

pkg/cri/server/image_pull.go

.golangci.yml

GOGC=75 golangci-lint run services/server/server.go:320:27: G114: Use of net/http serve function that has no support for setting timeouts (gosec) return trapClosedConnErr(http.Serve(l, m)) ^ services/server/server.go:340:27: G114: Use of net/http serve function that has no support for setting timeouts (gosec) return trapClosedConnErr(http.Serve(l, m)) ^ cmd/containerd-stress/main.go:238:13: G114: Use of net/http serve function that has no support for setting timeouts (gosec) if err := http.ListenAndServe(c.Metrics, metrics.Handler()); err != nil { ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

THis makes it easier to find which linters are enabled when going through the list. Signed-off-by: Sebastiaan van Stijn <[email protected]>

…fy nolint This `//nolint` was added in containerd/cri@f5c7ac9 to suppress warnings about the `NameToCertificate` function being deprecated: // Deprecated: NameToCertificate only allows associating a single certificate // with a given name. Leave that field nil to let the library select the first // compatible chain from Certificates. Looking at that, it was deprecated in Go 1.14 through golang/go@eb93c68 (https://go-review.googlesource.com/c/go/+/205059), which describes: crypto/tls: select only compatible chains from Certificates Now that we have a full implementation of the logic to check certificate compatibility, we can let applications just list multiple chains in Certificates (for example, an RSA and an ECDSA one) and choose the most appropriate automatically. NameToCertificate only maps each name to one chain, so simply deprecate it, and while at it simplify its implementation by not stripping trailing dots from the SNI (which is specified not to have any, see RFC 6066, Section 3) and by not supporting multi-level wildcards, which are not a thing in the WebPKI (and in crypto/x509). We should at least have a comment describing why we are ignoring this, but preferably review whether we should still use it. Signed-off-by: Sebastiaan van Stijn <[email protected]>

- fix "nolint" comments to be in the correct format (`//nolint:<linters>[,<linter>` no leading space, required colon (`:`) and linters. - remove "nolint" comments for errcheck, which is disabled in our config. - remove "nolint" comments that were no longer needed (nolintlint). - where known, add a comment describing why a "nolint" was applied. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Remove nolint-comments that weren't hit by linters, and remove the "structcheck" and "varcheck" linters, as they have been deprecated: WARN [runner] The linter 'structcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. WARN [runner] The linter 'varcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. WARN [linters context] structcheck is disabled because of generics. You can track the evolution of the generics support by following the golangci/golangci-lint#2649. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Also remove "nolint" comments for deadcode, which is deprecated, and removed from the defaults. Signed-off-by: Sebastiaan van Stijn <[email protected]>

estesp

LGTM

thaJeztah · 2022-10-16T16:10:44Z

@mikebrow PTAL; this one good to go?

mikebrow

LGTM

This comment was marked as outdated.

Sign in to view

thaJeztah force-pushed the gofmt_119 branch from 8b62c85 to a22c02f Compare August 31, 2022 13:08

thaJeztah force-pushed the gofmt_119 branch 3 times, most recently from 505c13d to 0393b32 Compare October 8, 2022 12:04

thaJeztah commented Oct 8, 2022

View reviewed changes

thaJeztah force-pushed the gofmt_119 branch from 0393b32 to 8144802 Compare October 8, 2022 12:12

thaJeztah changed the title ~~[WIP] clean-up nolint-tags, remove unused ones, update golangci-lint~~ clean-up "nolint" comments, remove unused ones, update golangci-lint Oct 8, 2022

thaJeztah force-pushed the gofmt_119 branch from 8144802 to 4d5d045 Compare October 8, 2022 12:25

thaJeztah commented Oct 8, 2022

View reviewed changes

thaJeztah mentioned this pull request Oct 8, 2022

cmd/containerd: replace deprecated windows.IsAnInteractiveSession() #7497

Merged

thaJeztah commented Oct 8, 2022

View reviewed changes

thaJeztah force-pushed the gofmt_119 branch from 4d5d045 to 5fe9dfd Compare October 8, 2022 13:54

thaJeztah marked this pull request as ready for review October 8, 2022 15:43

mikebrow reviewed Oct 10, 2022

View reviewed changes

pkg/cri/server/image_pull.go Outdated Show resolved Hide resolved

.golangci.yml Show resolved Hide resolved

thaJeztah force-pushed the gofmt_119 branch from 5fe9dfd to d2a33fd Compare October 10, 2022 22:08

thaJeztah closed this Oct 11, 2022

thaJeztah reopened this Oct 11, 2022

thaJeztah added 6 commits October 12, 2022 14:40

golangci-lint: sort linters in config file

0eaace3

THis makes it easier to find which linters are enabled when going through the list. Signed-off-by: Sebastiaan van Stijn <[email protected]>

update golangci-lint to v1.49.0

8b5df7d

Also remove "nolint" comments for deadcode, which is deprecated, and removed from the defaults. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the gofmt_119 branch from d2a33fd to 8b5df7d Compare October 12, 2022 12:41

estesp approved these changes Oct 12, 2022

View reviewed changes

mikebrow approved these changes Oct 17, 2022

View reviewed changes

mikebrow merged commit 3ce301d into containerd:main Oct 17, 2022

thaJeztah deleted the gofmt_119 branch October 17, 2022 16:01

thaJeztah mentioned this pull request Mar 7, 2023

[release/1.6 backport] assorted linting, and golang update-related changes #8229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

clean-up "nolint" comments, remove unused ones, update golangci-lint #7349

clean-up "nolint" comments, remove unused ones, update golangci-lint #7349

Uh oh!

thaJeztah commented Aug 31, 2022 •

edited

Loading

Uh oh!

k8s-ci-robot commented Aug 31, 2022

Uh oh!

This comment was marked as outdated.

thaJeztah Oct 8, 2022

Uh oh!

thaJeztah Oct 8, 2022

Uh oh!

thaJeztah Oct 8, 2022

Uh oh!

thaJeztah Oct 8, 2022

Uh oh!

thaJeztah commented Oct 8, 2022

Uh oh!

mikebrow left a comment

Uh oh!

Uh oh!

Uh oh!

estesp left a comment

Uh oh!

thaJeztah commented Oct 16, 2022

Uh oh!

mikebrow left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clean-up "nolint" comments, remove unused ones, update golangci-lint #7349

clean-up "nolint" comments, remove unused ones, update golangci-lint #7349

Uh oh!

Conversation

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

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 31, 2022

Uh oh!

This comment was marked as outdated.

thaJeztah Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 8, 2022

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 16, 2022

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Aug 31, 2022 •

edited

Loading