diff --git a/.changelog/10515.txt b/.changelog/10515.txt new file mode 100644 index 000000000000..b83e5e154e65 --- /dev/null +++ b/.changelog/10515.txt @@ -0,0 +1,3 @@ +```release-note:bug +agent: fix a panic on 32-bit platforms caused by misaligned struct fields used with sync/atomic. +``` diff --git a/.circleci/config.yml b/.circleci/config.yml index e553a8bb868d..93ff0b969b14 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -116,14 +116,20 @@ jobs: - run: go get -u github.com/hashicorp/lint-consul-retry && lint-consul-retry - run: *notify-slack-failure - # Runs Go linters lint: + description: "Run golangci-lint" + parameters: + go-arch: + type: string + default: "" docker: - image: *GOLANG_IMAGE environment: GOTAGS: "" # No tags for OSS but there are for enterprise + GOARCH: "<>" steps: - checkout + - run: go env - run: name: Install golangci-lint command: | @@ -275,6 +281,40 @@ jobs: path: /tmp/jsonfile - run: *notify-slack-failure + # go-test-32bit is to catch problems where 64-bit ints must be 64-bit aligned + # to use them with sync/atomic. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG. + # Running tests with GOARCH=386 seems to be the best way to detect this + # problem. Only runs tests that are -short to limit the time we spend checking + # for these bugs. + go-test-32bit: + docker: + - image: *GOLANG_IMAGE + environment: + <<: *ENVIRONMENT + GOTAGS: "" # No tags for OSS but there are for enterprise + steps: + - checkout + - run: *install-gotestsum + - run: go mod download + - run: + name: go test 32-bit + environment: + GOARCH: 386 + command: | + mkdir -p $TEST_RESULTS_DIR /tmp/jsonfile + go env + gotestsum \ + --jsonfile /tmp/jsonfile/go-test-32bit.log \ + --junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- \ + -tags="$GOTAGS" -p 2 \ + -short ./... + + - store_test_results: + path: *TEST_RESULTS_DIR + - store_artifacts: + path: *TEST_RESULTS_DIR + - run: *notify-slack-failure + go-test-lib: description: "test a library against a specific Go version" parameters: @@ -959,6 +999,10 @@ workflows: - check-generated-protobuf: *filter-ignore-non-go-branches - lint-consul-retry: *filter-ignore-non-go-branches - lint: *filter-ignore-non-go-branches + - lint: + name: "lint-32bit" + go-arch: "386" + <<: *filter-ignore-non-go-branches - test-connect-ca-providers: *filter-ignore-non-go-branches - dev-build: *filter-ignore-non-go-branches - go-test: @@ -984,6 +1028,7 @@ workflows: go-version: "1.15" <<: *filter-ignore-non-go-branches - go-test-race: *filter-ignore-non-go-branches + - go-test-32bit: *filter-ignore-non-go-branches build-distros: unless: << pipeline.parameters.trigger-load-test >> jobs: diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index c963e78ba8e3..0f02aadea26d 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -5473,7 +5473,7 @@ func TestLoad_FullConfig(t *testing.T) { HTTPSPort: 15127, HTTPUseCache: false, KeyFile: "IEkkwgIA", - KVMaxValueSize: 1234567800000000, + KVMaxValueSize: 1234567800, LeaveDrainTime: 8265 * time.Second, LeaveOnTerm: true, Logging: logging.Config{ @@ -5868,7 +5868,7 @@ func TestLoad_FullConfig(t *testing.T) { "wan_ipv4": "78.63.37.19", }, TranslateWANAddrs: true, - TxnMaxReqLen: 5678000000000000, + TxnMaxReqLen: 567800000, UIConfig: UIConfig{ Dir: "pVncV4Ey", ContentPath: "/qp1WRhYH/", // slashes are added in parsing diff --git a/agent/config/testdata/full-config.hcl b/agent/config/testdata/full-config.hcl index 28f8248ce988..b2067024f737 100644 --- a/agent/config/testdata/full-config.hcl +++ b/agent/config/testdata/full-config.hcl @@ -285,8 +285,8 @@ limits { rpc_rate = 12029.43 rpc_max_burst = 44848 rpc_max_conns_per_client = 2954 - kv_max_value_size = 1234567800000000 - txn_max_req_len = 5678000000000000 + kv_max_value_size = 1234567800 + txn_max_req_len = 567800000 } log_level = "k1zo9Spt" log_json = true diff --git a/agent/config/testdata/full-config.json b/agent/config/testdata/full-config.json index 66fad69f3a60..d864d1fc85b8 100644 --- a/agent/config/testdata/full-config.json +++ b/agent/config/testdata/full-config.json @@ -285,8 +285,8 @@ "rpc_rate": 12029.43, "rpc_max_burst": 44848, "rpc_max_conns_per_client": 2954, - "kv_max_value_size": 1234567800000000, - "txn_max_req_len": 5678000000000000 + "kv_max_value_size": 1234567800, + "txn_max_req_len": 567800000 }, "log_level": "k1zo9Spt", "log_json": true, diff --git a/agent/grpc/stats.go b/agent/grpc/stats.go index 7ba96f91f4d8..8af6ec4005c1 100644 --- a/agent/grpc/stats.go +++ b/agent/grpc/stats.go @@ -52,8 +52,11 @@ var defaultMetrics = metrics.Default // statsHandler is a grpc/stats.StatsHandler which emits connection and // request metrics to go-metrics. type statsHandler struct { + // activeConns is used with sync/atomic and MUST be 64-bit aligned. To ensure + // alignment on 32-bit platforms this field must remain the first field in + // the struct. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG. + activeConns uint64 metrics *metrics.Metrics - activeConns uint64 // must be 8-byte aligned for atomic access } func newStatsHandler(m *metrics.Metrics) *statsHandler { @@ -103,10 +106,11 @@ func (c *statsHandler) HandleConn(_ context.Context, s stats.ConnStats) { } type activeStreamCounter struct { + // count is used with sync/atomic and MUST be 64-bit aligned. To ensure + // alignment on 32-bit platforms this field must remain the first field in + // the struct. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG. + count uint64 metrics *metrics.Metrics - // count of the number of open streaming RPCs on a server. It is accessed - // atomically. - count uint64 } // GRPCCountingStreamInterceptor is a grpc.ServerStreamInterceptor that emits a diff --git a/command/connect/redirecttraffic/redirect_traffic_test.go b/command/connect/redirecttraffic/redirect_traffic_test.go index 0fde8a37ecf6..5c31af036cf5 100644 --- a/command/connect/redirecttraffic/redirect_traffic_test.go +++ b/command/connect/redirecttraffic/redirect_traffic_test.go @@ -59,6 +59,10 @@ func TestRun_FlagValidation(t *testing.T) { } func TestGenerateConfigFromFlags(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + cases := []struct { name string command func() cmd diff --git a/tlsutil/config.go b/tlsutil/config.go index 6fcdb1a2e9cb..94053b439a85 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -175,6 +175,11 @@ type manual struct { // Configurator receives an initial TLS configuration from agent configuration, // and receives updates from config reloads, auto-encrypt, and auto-config. type Configurator struct { + // version is increased each time the Configurator is updated. Must be accessed + // using sync/atomic. Also MUST be the first field in this struct to ensure + // 64-bit alignment. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG. + version uint64 + // lock synchronizes access to all fields on this struct except for logger and version. lock sync.RWMutex base *Config @@ -188,9 +193,6 @@ type Configurator struct { // logger is not protected by a lock. It must never be changed after // Configurator is created. logger hclog.Logger - // version is increased each time the Configurator is updated. Must be accessed - // using sync/atomic. - version uint64 } // NewConfigurator creates a new Configurator and sets the provided