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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[NET-4968] Upgrade Go to 1.21 #20062

Merged
merged 2 commits into from
Jan 12, 2024
Merged

[NET-4968] Upgrade Go to 1.21 #20062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01ca961
Upgrade Go to 1.21
zalimeni Dec 22, 2023
e71ae3a
ci: detect Go backwards compatibility test version automatically
zalimeni Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/20062.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
Upgrade to use Go 1.21.6.
Copy link
Member Author

Choose a reason for hiding this comment

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

@curtbushko FYI for K8s + DP bumps, we got 1.21.6 earlier this week

```
30 changes: 18 additions & 12 deletions .github/workflows/go-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,17 +408,19 @@ jobs:
consul-license: ${{secrets.CONSUL_LICENSE}}
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}"

go-test-api-1-19:
go-test-api-backwards-compatibility:
name: go-test-api-${{ needs.get-go-version.outputs.go-version-previous }}
Comment on lines -411 to +412
Copy link
Member Author

@zalimeni zalimeni Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Prior art: #10488

The second commit in this PR (e71ae3a) automates the version selection so we don't have to remember to bump it every time we upgrade the minor version of Go.

needs:
- setup
- get-go-version
- dev-build
uses: ./.github/workflows/reusable-unit.yml
with:
directory: api
runs-on: ${{ needs.setup.outputs.compute-large }}
repository-name: ${{ github.repository }}
go-tags: "${{ github.event.repository.name == 'consul-enterprise' && 'consulent consuldev' || '' }}"
go-version: "1.19"
go-version: ${{ needs.get-go-version.outputs.go-version-previous }}
permissions:
id-token: write # NOTE: this permission is explicitly required for Vault auth.
contents: read
Expand All @@ -427,17 +429,18 @@ jobs:
consul-license: ${{secrets.CONSUL_LICENSE}}
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}"

go-test-api-1-20:
go-test-api:
needs:
- setup
- get-go-version
- dev-build
uses: ./.github/workflows/reusable-unit.yml
with:
directory: api
runs-on: ${{ needs.setup.outputs.compute-large }}
repository-name: ${{ github.repository }}
go-tags: "${{ github.event.repository.name == 'consul-enterprise' && 'consulent consuldev' || '' }}"
go-version: "1.20"
go-version: ${{ needs.get-go-version.outputs.go-version }}
permissions:
id-token: write # NOTE: this permission is explicitly required for Vault auth.
contents: read
Expand All @@ -446,17 +449,19 @@ jobs:
consul-license: ${{secrets.CONSUL_LICENSE}}
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}"

go-test-sdk-1-19:
go-test-sdk-backwards-compatibility:
Copy link
Member

Choose a reason for hiding this comment

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

I love this. No more twiddling versions whenever we update the go major version.

name: go-test-sdk-${{ needs.get-go-version.outputs.go-version-previous }}
needs:
- setup
- get-go-version
- dev-build
uses: ./.github/workflows/reusable-unit.yml
with:
directory: sdk
runs-on: ${{ needs.setup.outputs.compute-large }}
repository-name: ${{ github.repository }}
go-tags: "${{ github.event.repository.name == 'consul-enterprise' && 'consulent consuldev' || '' }}"
go-version: "1.19"
go-version: ${{ needs.get-go-version.outputs.go-version-previous }}
permissions:
id-token: write # NOTE: this permission is explicitly required for Vault auth.
contents: read
Expand All @@ -465,17 +470,18 @@ jobs:
consul-license: ${{secrets.CONSUL_LICENSE}}
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}"

go-test-sdk-1-20:
go-test-sdk:
needs:
- setup
- get-go-version
- dev-build
uses: ./.github/workflows/reusable-unit.yml
with:
directory: sdk
runs-on: ${{ needs.setup.outputs.compute-large }}
repository-name: ${{ github.repository }}
go-tags: "${{ github.event.repository.name == 'consul-enterprise' && 'consulent consuldev' || '' }}"
go-version: "1.20"
go-version: ${{ needs.get-go-version.outputs.go-version }}
permissions:
id-token: write # NOTE: this permission is explicitly required for Vault auth.
contents: read
Expand Down Expand Up @@ -521,10 +527,10 @@ jobs:
- go-test-race
- go-test-envoyextensions
- go-test-troubleshoot
- go-test-api-1-19
- go-test-api-1-20
- go-test-sdk-1-19
- go-test-sdk-1-20
- go-test-api-backwards-compatibility
- go-test-api
- go-test-sdk-backwards-compatibility
- go-test-sdk
- go-test-32bit
# - go-test-s390x
runs-on: ${{ fromJSON(needs.setup.outputs.compute-small) }}
Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/reusable-get-go-version.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@ on:
go-version:
description: "The Go version detected by this workflow"
value: ${{ jobs.get-go-version.outputs.go-version }}
go-version-previous:
description: "The Go version (MAJOR.MINOR) prior to the current one, used for backwards compatibility testing"
value: ${{ jobs.get-go-version.outputs.go-version-previous }}

jobs:
get-go-version:
name: "Determine Go toolchain version"
runs-on: ubuntu-latest
outputs:
go-version: ${{ steps.get-go-version.outputs.go-version }}
go-version-previous: ${{ steps.get-go-version.outputs.go-version-previous }}
steps:
- uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
- name: Determine Go version
Expand All @@ -32,3 +36,7 @@ jobs:
GO_VERSION=$(head -n 1 .go-version)
echo "Building with Go ${GO_VERSION}"
echo "go-version=${GO_VERSION}" >> $GITHUB_OUTPUT
GO_MINOR_VERSION=${GO_VERSION%.*}
GO_VERSION_PREVIOUS="${GO_MINOR_VERSION%.*}.$((${GO_MINOR_VERSION#*.}-1))"
echo "Previous version ${GO_VERSION_PREVIOUS}"
echo "go-version-previous=${GO_VERSION_PREVIOUS}" >> $GITHUB_OUTPUT
4 changes: 3 additions & 1 deletion .github/workflows/reusable-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@ jobs:
with:
go-version: ${{ inputs.go-version }}
- run: go env
- name: Set golangci-lint version
run: echo "GOLANGCI_LINT_VERSION=$(make --no-print-directory print-GOLANGCI_LINT_VERSION)" >> $GITHUB_ENV
- name: lint-${{ matrix.directory }}
uses: golangci/golangci-lint-action@639cd343e1d3b897ff35927a75193d57cfcba299 # v3.6.0
with:
working-directory: ${{ matrix.directory }}
version: v1.51.1
version: ${{ env.GOLANGCI_LINT_VERSION }}
args: --build-tags="${{ env.GOTAGS }}" -v
skip-cache: true
2 changes: 1 addition & 1 deletion .go-version
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.20.12
1.21.6
20 changes: 8 additions & 12 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,18 +98,14 @@ linters-settings:
- google.golang.org/protobuf

depguard:
list-type: denylist
include-go-root: true
# A list of packages for the list type specified.
# Default: []
packages:
- net/rpc
# A list of packages for the list type specified.
# Specify an error message to output when a denied package is used.
# Default: []
packages-with-error-message:
- net/rpc: "only use forked copy in github.com/hashicorp/consul-net-rpc/net/rpc"
- github.com/golang/protobuf: "only use google.golang.org/protobuf"
rules:
main:
listMode: lax
deny:
- pkg: net/rpc
desc: "only use forked copy in github.com/hashicorp/consul-net-rpc/net/rpc"
- pkg: github.com/golang/protobuf
desc: "only use google.golang.org/protobuf"
Comment on lines -101 to +108
Copy link
Member Author

Choose a reason for hiding this comment

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

See golangci/golangci-lint#3906. Some other projects disabled this linter or replaced w/ another tool like Semgrep in response, but since we use custom rules and don't yet use Semgrep, I tried to adapt ours in-place.


run:
timeout: 10m
Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ GO_MODULES := $(shell find . -name go.mod -exec dirname {} \; | grep -v "proto-g
# These version variables can either be a valid string for "go install <module>@<version>"
# or the string @DEV to imply use what is currently installed locally.
###
GOLANGCI_LINT_VERSION='v1.51.1'
GOLANGCI_LINT_VERSION='v1.55.2'
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed for compatibility w/ Go 1.21

MOCKERY_VERSION='v2.37.1'
BUF_VERSION='v1.26.0'

Expand Down
7 changes: 5 additions & 2 deletions api/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,8 +717,11 @@ func TestAPI_ClientTLSOptions(t *testing.T) {

// Should fail
_, err = client.Agent().Self()
if err == nil || !strings.Contains(err.Error(), "bad certificate") {
t.Fatal(err)
// Check for one of the possible cert error messages
// See https://cs.opensource.google/go/go/+/62a994837a57a7d0c58bb364b580a389488446c9
if err == nil || !(strings.Contains(err.Error(), "tls: bad certificate") ||
strings.Contains(err.Error(), "tls: certificate required")) {
t.Fatalf("expected tls certificate error, but got '%v'", err)
}
})

Expand Down
1 change: 1 addition & 0 deletions internal/mesh/internal/controllers/sidecarproxy/builder/local_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ func destinationRulesByPort(allPorts []string, destinationRules []*pbauth.Destin
return out
}

//nolint:unparam
Copy link
Member

Choose a reason for hiding this comment

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

Just curious but why was this necessary. AFAICT allPorts and dr are both used in the function.

Copy link
Member

Choose a reason for hiding this comment

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

Or is the linter even smarter and figuring out that one of the two is always nil based on other code.

Copy link
Member Author

@zalimeni zalimeni Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the latter - that was my best explanation for why it suddenly started failing on the linter upgrade (same question you asked led me to notice the same)

func convertDestinationRule(allPorts []string, dr *pbauth.DestinationRule) ([]string, DestinationRule) {
ports := make(map[string]struct{})
if len(dr.PortNames) > 0 {
Expand Down