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

Fix panic on 32-bit platforms

.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.
+```

.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:
@@ -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:

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

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

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,

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 {
-	metrics *metrics.Metrics
-	// count of the number of open streaming RPCs on a server. It is accessed
-	// atomically.
+	// 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
 }
 
 // GRPCCountingStreamInterceptor is a grpc.ServerStreamInterceptor that emits a

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

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