Merge pull request #10515 from hashicorp/dnephin/fix-arm32-atomic-ali…

…gment Fix panic on 32-bit platforms Conflicts in tlsutil/config.go were resolved by dropping those changes. The issue that was fixed in that file is not in 1.10.x.
hashicorp · Jun 30, 2021 · f81b371 · f81b371
1 parent 374072d
commit f81b371
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 11 deletions.
diff --git a/.changelog/10515.txt 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
@@ -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: "<<parameters.go-arch>>"
     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:
@@ -961,6 +1001,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:
@@ -986,6 +1030,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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