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

GolangCI-Lint Fixes #439

Merged
merged 19 commits into from
Feb 27, 2024
Merged

GolangCI-Lint Fixes #439

merged 19 commits into from
Feb 27, 2024

Conversation

sheurich
Copy link
Contributor

@sheurich sheurich commented Feb 23, 2024

This PR addresses #438 by fixing all of the reported issues in Pebble code.

When run with the default lint configuration, golangci-lint reports:

wfe/wfe.go:2365:2: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
        if strings.HasPrefix(ident.Value, "*.") {
        ^
wfe/wfe.go:1443:32: S1039: unnecessary use of fmt.Sprintf (gosimple)
                return acme.MalformedProblem(fmt.Sprintf(
                                             ^
wfe/wfe.go:1405:2: SA5011: possible nil pointer dereference (staticcheck)
        order.RLock()
        ^
wfe/wfe.go:1409:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if order == nil {
           ^
wfe/wfe.go:1406:8: SA5011: possible nil pointer dereference (staticcheck)
        defer order.RUnlock()
              ^
wfe/wfe.go:181:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: As of Go 1.20 there is no reason to call Seed with a random value. Programs that call Seed with a known value to get a specific sequence of results should use New(NewSource(seed)) to obtain a local random generator. (staticcheck)
        rand.Seed(time.Now().UnixNano())
        ^
va/va.go:321:10: S1039: unnecessary use of fmt.Sprintf (gosimple)
                msg := fmt.Sprintf("No TXT records found for DNS challenge")
                       ^
va/va.go:338:9: S1039: unnecessary use of fmt.Sprintf (gosimple)
        msg := fmt.Sprintf("Correct value not found for DNS challenge")
               ^

wfe/wfe.go:178: File is not `gofumpt`-ed (gofumpt)
        strict, requireEAB bool, retryAfterAuthz int, retryAfterOrder int) WebFrontEndImpl {

These errors are addressed here.

Other fixes include:

@sheurich sheurich mentioned this pull request Feb 24, 2024
@sheurich
Copy link
Contributor Author

I've identified a concurrency race condition related to this patch and will be providing a fix shortly:

WARNING: DATA RACE
Read at 0x00c000110000 by goroutine 250:
  math/rand.(*rngSource).Uint64()
  math/rand.(*rngSource).Int63()
  math/rand.(*Rand).Int63()
  math/rand.(*Rand).Int31()
  math/rand.(*Rand).Int31n()
  math/rand.(*Rand).Intn()
  github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).verifyJWS()
  github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).verifyPOST()
  github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).Authz()

@sheurich
Copy link
Contributor Author

Corrected the race condition in fd2b2be (#439). This change is ready for review.

aarongable
aarongable previously approved these changes Feb 26, 2024
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go code changes LGTM. Travis / Appveyor changes seem reasonable but I know nothing about those systems and with #442 it looks like we're heading towards getting rid of them.

@sheurich
Copy link
Contributor Author

@aarongable this change is ready for re-approval after merging main.

@mcpherrinm mcpherrinm merged commit ac381e9 into letsencrypt:main Feb 27, 2024
9 checks passed
@sheurich sheurich deleted the pebble-lint-fixes branch February 27, 2024 16:28
mcpherrinm pushed a commit that referenced this pull request Mar 12, 2024
…mages (#444)

[![Checks](https://github.com/fastly/pebble/actions/workflows/checks.yml/badge.svg)](https://github.com/fastly/pebble/actions/workflows/checks.yml)

[![Tests](https://github.com/fastly/pebble/actions/workflows/tests.yml/badge.svg)](https://github.com/fastly/pebble/actions/workflows/tests.yml)

As a follow-on to #439 this
change request implements the first step of building a new CI system for
Pebble. As this is a testbed environment for Boulder, some of these
changes may prove to be useful in the latter context.

Per
#434 (comment),
GitHub Actions is a reasonable choice for a new CI implementation to
supersede Travis. Much, but not all, of the existing functionality is
present in this initial set of changes.

Items currently implemented include:
- Binary production of `pebble` and `challtestsrv` for platforms:
  - AMD64
    - darwin
    - linux
    - windows
  - ARM64
    - darwin
    - linux
    - windows
- Multiplatform docker images published on the GitHub Container Registry
for platforms:
  - linux/amd64
  - linux/arm64
  - windows/amd64
- Golang linting.
- Golang coverage.
- Local test scripts for Go and Docker:
  - `./build.sh`
  - `./docker.sh`
  - `./test.sh`

This PR also adds a `-version` flag to Pebble, which is set during
release build to the Git commit ref.

This change builds on earlier work done in
#379 and
#442.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants