Skip to content

Commit a2b8fcd

Browse files
Archong-Liuclaude
andcommitted
fix: eliminate data races in fake IAM and SSM API test doubles
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. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NxjVFg5mc97XBxpbm55Px
1 parent a2496cc commit a2b8fcd

4 files changed

Lines changed: 197 additions & 4 deletions

File tree

pkg/fake/iamapi.go

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package fake
1717
import (
1818
"context"
1919
"fmt"
20+
"slices"
2021
"strings"
2122
"sync"
2223
"time"
@@ -65,7 +66,23 @@ func NewIAMAPI() *IAMAPI {
6566
}
6667
}
6768

69+
// copyInstanceProfile returns an independent copy of the given instance profile. It performs a shallow copy of the
70+
// struct and clones the Roles and Tags slices so that mutations to the returned object (e.g. reassigning or appending
71+
// to Roles/Tags) can't race with, or leak into, the copy held in the fake's internal map.
72+
func copyInstanceProfile(ip *iamtypes.InstanceProfile) *iamtypes.InstanceProfile {
73+
if ip == nil {
74+
return nil
75+
}
76+
cp := *ip
77+
cp.Roles = slices.Clone(ip.Roles)
78+
cp.Tags = slices.Clone(ip.Tags)
79+
return &cp
80+
}
81+
6882
func (s *IAMAPI) Reset() {
83+
s.Lock()
84+
defer s.Unlock()
85+
6986
s.GetInstanceProfileBehavior.Reset()
7087
s.CreateInstanceProfileBehavior.Reset()
7188
s.DeleteInstanceProfileBehavior.Reset()
@@ -84,7 +101,9 @@ func (s *IAMAPI) GetInstanceProfile(_ context.Context, input *iam.GetInstancePro
84101
defer s.Unlock()
85102

86103
if i, ok := s.InstanceProfiles[aws.ToString(input.InstanceProfileName)]; ok {
87-
return &iam.GetInstanceProfileOutput{InstanceProfile: i}, nil
104+
// Return a copy so callers can't mutate the internally-stored object without going through the
105+
// (locked) API methods. A real AWS API deserializes a fresh, independent object per response.
106+
return &iam.GetInstanceProfileOutput{InstanceProfile: copyInstanceProfile(i)}, nil
88107
}
89108
return nil, &smithy.GenericAPIError{
90109
Code: "NoSuchEntity",
@@ -114,7 +133,8 @@ func (s *IAMAPI) CreateInstanceProfile(_ context.Context, input *iam.CreateInsta
114133
Tags: input.Tags,
115134
}
116135
s.InstanceProfiles[aws.ToString(input.InstanceProfileName)] = instanceProfile
117-
return &iam.CreateInstanceProfileOutput{InstanceProfile: instanceProfile}, nil
136+
// Return a copy so callers can't mutate the internally-stored object outside of the (locked) API methods.
137+
return &iam.CreateInstanceProfileOutput{InstanceProfile: copyInstanceProfile(instanceProfile)}, nil
118138
})
119139
}
120140

@@ -225,7 +245,8 @@ func (s *IAMAPI) ListInstanceProfiles(_ context.Context, input *iam.ListInstance
225245
var profiles []iamtypes.InstanceProfile
226246
for _, profile := range s.InstanceProfiles {
227247
if profile.Path != nil && strings.HasPrefix(*profile.Path, *input.PathPrefix) {
228-
profiles = append(profiles, *profile)
248+
// Append a copy so the returned slice doesn't alias the internally-stored objects' Roles/Tags slices.
249+
profiles = append(profiles, *copyInstanceProfile(profile))
229250
}
230251
}
231252
return &iam.ListInstanceProfilesOutput{

pkg/fake/iamapi_test.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/*
2+
Licensed under the Apache License, Version 2.0 (the "License");
3+
you may not use this file except in compliance with the License.
4+
You may obtain a copy of the License at
5+
6+
http://www.apache.org/licenses/LICENSE-2.0
7+
8+
Unless required by applicable law or agreed to in writing, software
9+
distributed under the License is distributed on an "AS IS" BASIS,
10+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
See the License for the specific language governing permissions and
12+
limitations under the License.
13+
*/
14+
15+
package fake
16+
17+
import (
18+
"context"
19+
"sync"
20+
"testing"
21+
22+
"github.com/aws/aws-sdk-go-v2/aws"
23+
"github.com/aws/aws-sdk-go-v2/service/iam"
24+
iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"
25+
)
26+
27+
// TestIAMAPICreateInstanceProfileReturnsCopy ensures CreateInstanceProfile returns an object that is independent from
28+
// the one retained in the fake's internal map. Before the fix, the same pointer was returned, so a caller mutating the
29+
// returned profile (as the instanceprofile provider does) silently mutated the fake's internal state.
30+
func TestIAMAPICreateInstanceProfileReturnsCopy(t *testing.T) {
31+
api := NewIAMAPI()
32+
const name = "test-profile"
33+
34+
out, err := api.CreateInstanceProfile(context.Background(), &iam.CreateInstanceProfileInput{
35+
InstanceProfileName: aws.String(name),
36+
})
37+
if err != nil {
38+
t.Fatalf("CreateInstanceProfile: unexpected error: %v", err)
39+
}
40+
41+
// Mutate the returned object the way the provider's Create does.
42+
out.InstanceProfile.Roles = []iamtypes.Role{{RoleName: aws.String("some-role")}}
43+
44+
if got := len(api.InstanceProfiles[name].Roles); got != 0 {
45+
t.Fatalf("mutating the returned instance profile leaked into internal state: internal Roles len = %d, want 0", got)
46+
}
47+
}
48+
49+
// TestIAMAPIGetInstanceProfileReturnsCopy ensures GetInstanceProfile returns an object independent from the internally
50+
// stored one.
51+
func TestIAMAPIGetInstanceProfileReturnsCopy(t *testing.T) {
52+
api := NewIAMAPI()
53+
const name = "test-profile"
54+
55+
if _, err := api.CreateInstanceProfile(context.Background(), &iam.CreateInstanceProfileInput{
56+
InstanceProfileName: aws.String(name),
57+
}); err != nil {
58+
t.Fatalf("CreateInstanceProfile: unexpected error: %v", err)
59+
}
60+
61+
out, err := api.GetInstanceProfile(context.Background(), &iam.GetInstanceProfileInput{
62+
InstanceProfileName: aws.String(name),
63+
})
64+
if err != nil {
65+
t.Fatalf("GetInstanceProfile: unexpected error: %v", err)
66+
}
67+
68+
out.InstanceProfile.Roles = append(out.InstanceProfile.Roles, iamtypes.Role{RoleName: aws.String("some-role")})
69+
70+
if got := len(api.InstanceProfiles[name].Roles); got != 0 {
71+
t.Fatalf("mutating the returned instance profile leaked into internal state: internal Roles len = %d, want 0", got)
72+
}
73+
}
74+
75+
// TestIAMAPIConcurrentResetAndAccess exercises Reset concurrently with the locked API methods. Before the fix, Reset
76+
// reassigned the InstanceProfiles/Roles maps without holding the mutex, racing with (and potentially panicking against)
77+
// the concurrent map access performed under lock by the other methods. Run with -race to observe the data race.
78+
func TestIAMAPIConcurrentResetAndAccess(t *testing.T) {
79+
api := NewIAMAPI()
80+
ctx := context.Background()
81+
const name = "p"
82+
83+
var wg sync.WaitGroup
84+
for range 200 {
85+
wg.Add(5)
86+
go func() { defer wg.Done(); api.Reset() }()
87+
go func() {
88+
defer wg.Done()
89+
_, _ = api.CreateInstanceProfile(ctx, &iam.CreateInstanceProfileInput{InstanceProfileName: aws.String(name)})
90+
}()
91+
go func() {
92+
defer wg.Done()
93+
_, _ = api.GetInstanceProfile(ctx, &iam.GetInstanceProfileInput{InstanceProfileName: aws.String(name)})
94+
}()
95+
go func() {
96+
defer wg.Done()
97+
_, _ = api.AddRoleToInstanceProfile(ctx, &iam.AddRoleToInstanceProfileInput{
98+
InstanceProfileName: aws.String(name),
99+
RoleName: aws.String("r"),
100+
})
101+
}()
102+
go func() {
103+
defer wg.Done()
104+
_, _ = api.ListInstanceProfiles(ctx, &iam.ListInstanceProfilesInput{PathPrefix: aws.String("/")})
105+
}()
106+
}
107+
wg.Wait()
108+
}

pkg/fake/ssmapi.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package fake
1717
import (
1818
"context"
1919
"fmt"
20+
"sync"
2021

2122
"github.com/Pallinder/go-randomdata"
2223
"github.com/awslabs/operatorpkg/serrors"
@@ -29,6 +30,8 @@ import (
2930
)
3031

3132
type SSMAPI struct {
33+
sync.Mutex
34+
3235
sdk.SSMAPI
3336
Parameters map[string]string
3437
GetParameterOutput *ssm.GetParameterOutput
@@ -43,7 +46,10 @@ func NewSSMAPI() *SSMAPI {
4346
}
4447
}
4548

46-
func (a SSMAPI) GetParameter(_ context.Context, input *ssm.GetParameterInput, _ ...func(*ssm.Options)) (*ssm.GetParameterOutput, error) {
49+
func (a *SSMAPI) GetParameter(_ context.Context, input *ssm.GetParameterInput, _ ...func(*ssm.Options)) (*ssm.GetParameterOutput, error) {
50+
a.Lock()
51+
defer a.Unlock()
52+
4753
parameter := lo.FromPtr(input.Name)
4854
if a.WantErr != nil {
4955
return &ssm.GetParameterOutput{}, a.WantErr
@@ -79,6 +85,9 @@ func (a SSMAPI) GetParameter(_ context.Context, input *ssm.GetParameterInput, _
7985
}
8086

8187
func (a *SSMAPI) Reset() {
88+
a.Lock()
89+
defer a.Unlock()
90+
8291
a.Parameters = nil
8392
a.GetParameterOutput = nil
8493
a.WantErr = nil

pkg/fake/ssmapi_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
Licensed under the Apache License, Version 2.0 (the "License");
3+
you may not use this file except in compliance with the License.
4+
You may obtain a copy of the License at
5+
6+
http://www.apache.org/licenses/LICENSE-2.0
7+
8+
Unless required by applicable law or agreed to in writing, software
9+
distributed under the License is distributed on an "AS IS" BASIS,
10+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
See the License for the specific language governing permissions and
12+
limitations under the License.
13+
*/
14+
15+
package fake
16+
17+
import (
18+
"context"
19+
"fmt"
20+
"sync"
21+
"testing"
22+
23+
"github.com/aws/aws-sdk-go-v2/aws"
24+
"github.com/aws/aws-sdk-go-v2/service/ssm"
25+
)
26+
27+
// TestSSMAPIConcurrentGetParameterAndReset exercises the default-parameter caching path of GetParameter concurrently
28+
// with Reset. Before the fix, SSMAPI had no mutex: GetParameter performed an unsynchronized check-then-act on the
29+
// defaultParameters map while Reset reassigned it, which is a data race and can trigger a "concurrent map read and map
30+
// write" fatal runtime panic. Run with -race to observe the data race.
31+
func TestSSMAPIConcurrentGetParameterAndReset(t *testing.T) {
32+
api := NewSSMAPI()
33+
ctx := context.Background()
34+
35+
var wg sync.WaitGroup
36+
for i := range 200 {
37+
// Reuse a small set of names so multiple goroutines contend on the same defaultParameters entries,
38+
// maximizing the chance of a concurrent read/write against the shared map.
39+
name := fmt.Sprintf("param-%d", i%4)
40+
wg.Add(3)
41+
go func() {
42+
defer wg.Done()
43+
_, _ = api.GetParameter(ctx, &ssm.GetParameterInput{Name: aws.String(name)})
44+
}()
45+
go func() {
46+
defer wg.Done()
47+
_, _ = api.GetParameter(ctx, &ssm.GetParameterInput{Name: aws.String(name)})
48+
}()
49+
go func() {
50+
defer wg.Done()
51+
api.Reset()
52+
}()
53+
}
54+
wg.Wait()
55+
}

0 commit comments

Comments
 (0)