Skip to content

fix: eliminate data races in fake IAM and SSM API test doubles - #9428

Open
Archong-Liu wants to merge 2 commits into
aws:mainfrom
Archong-Liu:fix/fake-client-data-races
Open

fix: eliminate data races in fake IAM and SSM API test doubles#9428
Archong-Liu wants to merge 2 commits into
aws:mainfrom
Archong-Liu:fix/fake-client-data-races

Conversation

@Archong-Liu

Copy link
Copy Markdown

Related to #7969

Description

While looking into #7969 (data races surfaced when running tests with the race detector), I found three concrete, reproducible data races in the fake AWS API test doubles under pkg/fake. These are test-harness-only bugs (the production code paths deserialize a fresh AWS response per call and never alias state), but they can cause flaky failures and even hard panics when tests run concurrently under -race.

1. IAMAPI.Reset() doesn't hold the lock (pkg/fake/iamapi.go)

IAMAPI embeds sync.Mutex, and every other method (GetInstanceProfile, CreateInstanceProfile, DeleteInstanceProfile, TagInstanceProfile, AddRoleToInstanceProfile, RemoveRoleFromInstanceProfile, ListInstanceProfiles) takes s.Lock() before touching s.InstanceProfiles/s.Roles. Reset() alone reassigned both maps with no lock, racing against any concurrent locked method. Fixed by acquiring the lock in Reset() like every other method.

2. Pointer escape from IAMAPI getters (pkg/fake/iamapi.go)

GetInstanceProfile and CreateInstanceProfile returned the same *iamtypes.InstanceProfile pointer stored in the internal map. The instanceprofile provider caches that pointer (instanceProfileCache.SetDefault(...)) and then mutates it directly (instanceProfile.Roles = []iamtypes.Role{...}) outside the fake's lock, so the mutex protected nothing once the pointer escaped. A real AWS API returns a freshly deserialized, independent object per response, so these methods now return a copy (shallow struct copy plus cloned Roles/Tags slices). ListInstanceProfiles had the same slice-aliasing issue (it copied the struct by value but shared the Roles/Tags backing arrays) and now returns copies too.

3. SSMAPI has no mutex (pkg/fake/ssmapi.go)

SSMAPI had no synchronization at all. GetParameter did an unsynchronized check-then-act on the shared defaultParameters map (read, check, write) while Reset() reassigned it. Since Go maps are not safe for concurrent read/write, this can trigger a fatal error: concurrent map read and map write panic, not just wrong data. Note GetParameter used a value receiver, so even adding a field mutex wouldn't have helped — a value receiver copies the mutex. Fixed by adding an embedded sync.Mutex, switching GetParameter to a pointer receiver, and locking in both GetParameter and Reset().

I also checked the other fakes in pkg/fake/*.go for the same two anti-patterns. The rest are already safe (ec2api.go uses sync.Map; eksapi.go/sqsapi.go/arczonalshiftapi.go use the atomic helpers in atomic.go/types.go; pricingapi.go builds fresh maps per call), so this change is intentionally scoped to just iamapi.go and ssmapi.go.

Note this does not fully resolve #7969 — a separate integration/E2E-test race described in that issue is being investigated independently and is out of scope here. This PR only addresses the fake-client races above.

How was this change tested?

  • Added -race regression tests in pkg/fake (iamapi_test.go, ssmapi_test.go): the pointer-escape tests fail on the unfixed code (mutation leaks into internal state), and the concurrency tests report WARNING: DATA RACE on the unfixed code for both IAMAPI.Reset() and SSMAPI.GetParameter()/Reset(). All pass after the fix.
  • go test -race ./pkg/fake/... — pass (repeated).
  • go test -race ./pkg/providers/instanceprofile/... and ./pkg/providers/amifamily/... (heavy consumers of these fakes) — pass under -race.
  • gofmt -l, go vet ./..., and go build ./... — clean.

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The fake IAMAPI and SSMAPI used by unit tests had three data races:

- IAMAPI.Reset() reassigned the InstanceProfiles/Roles maps without
  holding the embedded mutex, racing with the API methods that access
  those maps under lock.
- GetInstanceProfile and CreateInstanceProfile returned the pointer
  stored in the internal map, so callers could mutate fake-internal
  state after the lock was released (the instanceprofile provider's
  Create does exactly this). ListInstanceProfiles similarly aliased the
  stored Roles/Tags slices.
- SSMAPI had no mutex at all; GetParameter performed an unsynchronized
  check-then-act on the shared defaultParameters map while Reset
  reassigned it, which can trigger a concurrent-map-read-and-write
  fatal runtime panic.

Reset() now acquires the lock like the other methods; the IAM getters
return independent copies (struct plus cloned Roles/Tags slices), which
also mirrors how a real AWS API returns a fresh object per response; and
SSMAPI gains a mutex with GetParameter switched to a pointer receiver so
the lock actually protects the shared map.

Adds -race regression tests in pkg/fake for all three.
@Archong-Liu
Archong-Liu force-pushed the fix/fake-client-data-races branch from a2b8fcd to 36b3edb Compare July 22, 2026 10:30
@Archong-Liu
Archong-Liu marked this pull request as ready for review July 22, 2026 14:10
@Archong-Liu
Archong-Liu requested a review from a team as a code owner July 22, 2026 14:10
@Archong-Liu
Archong-Liu requested a review from DerekFrank July 22, 2026 14:10
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.

Testing: Fix Data Races in E2Es

1 participant