Skip to content

Commit daea6e3

Browse files
dylanratcliffeactions-user
authored andcommitted
Fix s3 bucket linking (#3000)
Previously, we were incorrectly assuming that the ARN for an S3 bucket would contain an account ID and region, If it did not, we were skipping execution of the query. This makes sense in some scenarios because we might have a search query that hits all scopes and we want to minimise the amount of work done to just scopes that are likely to be able to find it, However, this doesn't make sense when it comes to S3 because everything is globally unique GitOrigin-RevId: d913b5c7c58dc156be6cae2c6993a6069e5e2cde
1 parent f0d92d1 commit daea6e3

6 files changed

Lines changed: 283 additions & 49 deletions

File tree

aws-source/adapterhelpers/util.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,14 @@ import (
2525
)
2626

2727
// FormatScope Formats an account ID and region into the corresponding Overmind
28-
// scope. This will be in the format {accountID}.{region}
28+
// scope. This will be in the format {accountID}.{region}. If both accountID and
29+
// region are empty, returns sdp.WILDCARD (for resources like S3 buckets that
30+
// don't include account/region in their ARNs).
2931
func FormatScope(accountID, region string) string {
32+
if accountID == "" && region == "" {
33+
return sdp.WILDCARD
34+
}
35+
3036
if region == "" {
3137
return accountID
3238
}

aws-source/adapters/ecs-capacity-provider_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ func TestCapacityProviderAdapter(t *testing.T) {
127127
adapter := NewECSCapacityProviderAdapter(&ecsTestClient{}, "", "")
128128

129129
stream := discovery.NewRecordingQueryResultStream()
130-
adapter.ListStream(context.Background(), "", false, stream)
130+
adapter.ListStream(context.Background(), "*", false, stream)
131131

132132
errs := stream.GetErrors()
133133
if len(errs) > 0 {

aws-source/adapters/s3.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -667,11 +667,16 @@ func searchImpl(ctx context.Context, cache *sdpcache.Cache, client S3Client, sco
667667
return nil, sdp.NewQueryError(err)
668668
}
669669

670-
if arnScope := adapterhelpers.FormatScope(a.AccountID, a.Region); arnScope != scope {
671-
return nil, &sdp.QueryError{
672-
ErrorType: sdp.QueryError_NOSCOPE,
673-
ErrorString: fmt.Sprintf("ARN scope %v does not match adapters scope %v", arnScope, scope),
674-
Scope: scope,
670+
// For S3 bucket ARNs, account ID and region are empty, so we skip scope validation
671+
// and use the adapter's scope (which is account-scoped)
672+
// If the ARN does have an account ID, validate it matches the adapter scope
673+
if a.AccountID != "" {
674+
if arnScope := adapterhelpers.FormatScope(a.AccountID, a.Region); arnScope != scope {
675+
return nil, &sdp.QueryError{
676+
ErrorType: sdp.QueryError_NOSCOPE,
677+
ErrorString: fmt.Sprintf("ARN scope %v does not match adapters scope %v", arnScope, scope),
678+
Scope: scope,
679+
}
675680
}
676681
}
677682

aws-source/adapters/s3_test.go

Lines changed: 103 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package adapters
33
import (
44
"context"
55
"errors"
6+
"strings"
67
"testing"
78
"time"
89

@@ -16,48 +17,48 @@ import (
1617

1718
func TestS3SearchImpl(t *testing.T) {
1819
cache := sdpcache.NewCache()
19-
t.Run("with a good ARN", func(t *testing.T) {
20-
items, err := searchImpl(context.Background(), cache, TestS3Client{}, "account-id.region", "arn:partition:service:region:account-id:resource-type:resource-id", false)
2120

21+
t.Run("with S3 bucket ARN format (empty account ID and region)", func(t *testing.T) {
22+
// This test verifies that S3 bucket ARNs with empty account ID and region work correctly
23+
// Format: arn:aws:s3:::bucket-name
24+
// When parsed, AccountID="", Region="", so FormatScope("", "") returns sdp.WILDCARD
25+
// The adapter skips scope validation when accountID is empty and uses its own scope
26+
//
27+
// EXPECTED BEHAVIOR: Search should succeed because S3 bucket ARNs don't include account/region
28+
// (S3 is global), and the adapter should use its own scope since it knows the account ID.
29+
bucketName := "test-bucket-name"
30+
s3ARN := "arn:aws:s3:::" + bucketName
31+
adapterScope := "account-id" // S3 scopes are account-only (no region)
32+
33+
items, err := searchImpl(context.Background(), cache, TestS3Client{}, adapterScope, s3ARN, false)
34+
35+
// We EXPECT this to succeed, but it currently fails with NOSCOPE error
36+
// This test demonstrates the bug existing
2237
if err != nil {
23-
t.Error(err)
24-
}
25-
if len(items) != 1 {
26-
t.Errorf("expected 1 item, got %v", len(items))
27-
}
28-
})
29-
30-
t.Run("with a bad ARN", func(t *testing.T) {
31-
_, err := searchImpl(context.Background(), cache, TestS3Client{}, "account-id.region", "foo", false)
32-
33-
if err == nil {
34-
t.Error("expected error")
35-
} else {
3638
var ire *sdp.QueryError
3739
if errors.As(err, &ire) {
38-
if ire.GetErrorType() != sdp.QueryError_OTHER {
39-
t.Errorf("expected error type to be OTHER, got %v", ire.GetErrorType().String())
40+
if ire.GetErrorType() == sdp.QueryError_NOSCOPE && strings.Contains(ire.GetErrorString(), "ARN scope") {
41+
// This is the bug - the search fails when it should succeed
42+
t.Errorf("BUG REPRODUCED: Search failed with NOSCOPE error when it should succeed. "+
43+
"Error: %v. S3 bucket ARNs don't include account/region, so the adapter should use its own scope.",
44+
ire.GetErrorString())
45+
t.Logf("Expected: Search succeeds and returns bucket item")
46+
t.Logf("Actual: Search fails with NOSCOPE error: %v", ire.GetErrorString())
47+
} else {
48+
t.Errorf("unexpected error: %v", err)
4049
}
4150
} else {
42-
t.Errorf("expected item request error, got %T", err)
51+
t.Errorf("unexpected error type: %T: %v", err, err)
4352
}
53+
return
4454
}
45-
})
4655

47-
t.Run("with an ARN in another scope", func(t *testing.T) {
48-
_, err := searchImpl(context.Background(), cache, TestS3Client{}, "account-id.region", "arn:partition:service:region:account-id-2:resource-type:resource-id", false)
49-
50-
if err == nil {
51-
t.Error("expected error")
52-
} else {
53-
var ire *sdp.QueryError
54-
if errors.As(err, &ire) {
55-
if ire.GetErrorType() != sdp.QueryError_NOSCOPE {
56-
t.Errorf("expected error type to be OTHER, got %v", ire.GetErrorType().String())
57-
}
58-
} else {
59-
t.Errorf("expected item request error, got %T", err)
60-
}
56+
// If we get here, the search succeeded (expected behavior)
57+
if len(items) != 1 {
58+
t.Errorf("expected 1 item, got %v", len(items))
59+
}
60+
if items[0] == nil {
61+
t.Error("expected non-nil item")
6162
}
6263
})
6364
}
@@ -116,14 +117,14 @@ func TestS3GetImpl(t *testing.T) {
116117
{
117118
ExpectedType: "s3-bucket",
118119
ExpectedMethod: sdp.QueryMethod_SEARCH,
119-
ExpectedQuery: "arn:partition:service:region:account-id:resource-type:resource-id",
120-
ExpectedScope: "account-id.region",
120+
ExpectedQuery: "arn:aws:s3:::amzn-s3-demo-bucket",
121+
ExpectedScope: sdp.WILDCARD,
121122
},
122123
{
123124
ExpectedType: "s3-bucket",
124125
ExpectedMethod: sdp.QueryMethod_SEARCH,
125-
ExpectedQuery: "arn:partition:service:region:account-id:resource-type:resource-id",
126-
ExpectedScope: "account-id.region",
126+
ExpectedQuery: "arn:aws:s3:::amzn-s3-demo-bucket",
127+
ExpectedScope: sdp.WILDCARD,
127128
},
128129
}
129130

@@ -206,7 +207,7 @@ func (t TestS3Client) GetBucketAnalyticsConfiguration(ctx context.Context, param
206207
DataExport: &types.StorageClassAnalysisDataExport{
207208
Destination: &types.AnalyticsExportDestination{
208209
S3BucketDestination: &types.AnalyticsS3BucketDestination{
209-
Bucket: adapterhelpers.PtrString("arn:partition:service:region:account-id:resource-type:resource-id"),
210+
Bucket: adapterhelpers.PtrString("arn:aws:s3:::amzn-s3-demo-bucket"),
210211
Format: types.AnalyticsS3ExportFileFormatCsv,
211212
BucketAccountId: adapterhelpers.PtrString("id"),
212213
Prefix: adapterhelpers.PtrString("pre"),
@@ -279,7 +280,7 @@ func (t TestS3Client) GetBucketInventoryConfiguration(ctx context.Context, param
279280
InventoryConfiguration: &types.InventoryConfiguration{
280281
Destination: &types.InventoryDestination{
281282
S3BucketDestination: &types.InventoryS3BucketDestination{
282-
Bucket: adapterhelpers.PtrString("arn:partition:service:region:account-id:resource-type:resource-id"),
283+
Bucket: adapterhelpers.PtrString("arn:aws:s3:::amzn-s3-demo-bucket"),
283284
Format: types.InventoryFormatCsv,
284285
AccountId: adapterhelpers.PtrString("id"),
285286
Encryption: &types.InventoryEncryption{
@@ -675,3 +676,66 @@ func TestNewS3Adapter(t *testing.T) {
675676

676677
test.Run(t)
677678
}
679+
680+
func TestS3SearchWithARNFormat(t *testing.T) {
681+
// This E2E test reproduces the customer issue:
682+
// - Get works with bucket name: harness-sample-three-qa-us-west-2-20251022151048279100000001
683+
// - Search fails with ARN: arn:aws:s3:::harness-sample-three-qa-us-west-2-20251022151048279100000001
684+
//
685+
// EXPECTED BEHAVIOR: Both Get and Search should work
686+
// CURRENT BEHAVIOR: Get works, Search fails with NOSCOPE error - THIS IS THE BUG
687+
config, account, _ := adapterhelpers.GetAutoConfig(t)
688+
689+
adapter := NewS3Adapter(config, account)
690+
scope := adapter.Scopes()[0]
691+
692+
bucketName := "harness-sample-three-qa-us-west-2-20251022151048279100000001"
693+
s3ARN := "arn:aws:s3:::" + bucketName
694+
695+
ctx := context.Background()
696+
697+
// First, verify that Get works with the bucket name directly
698+
t.Run("Get with bucket name", func(t *testing.T) {
699+
item, err := adapter.Get(ctx, scope, bucketName, false)
700+
if err != nil {
701+
t.Logf("Get failed (this is OK if bucket doesn't exist): %v", err)
702+
} else if item != nil {
703+
t.Logf("Get succeeded: found bucket %v", bucketName)
704+
}
705+
})
706+
707+
// Then, test Search with ARN format - this SHOULD succeed, but currently fails with NOSCOPE error
708+
t.Run("Search with S3 ARN format", func(t *testing.T) {
709+
items, err := adapter.Search(ctx, scope, s3ARN, false)
710+
711+
// EXPECTED: Search succeeds because S3 bucket ARNs don't include account/region
712+
// (S3 is global), and the adapter should use its own scope since it knows the account ID.
713+
// CURRENT: Search fails with NOSCOPE error - THIS IS THE BUG
714+
if err != nil {
715+
var ire *sdp.QueryError
716+
if errors.As(err, &ire) {
717+
if ire.GetErrorType() == sdp.QueryError_NOSCOPE && strings.Contains(ire.GetErrorString(), "ARN scope") {
718+
// This is the bug - the search fails when it should succeed
719+
t.Errorf("BUG REPRODUCED: Search failed with NOSCOPE error when it should succeed. "+
720+
"Error: %v. S3 bucket ARNs don't include account/region, so the adapter should use its own scope.",
721+
ire.GetErrorString())
722+
t.Logf("Expected: Search succeeds and returns bucket item (like Get does)")
723+
t.Logf("Actual: Search fails with NOSCOPE error: %v", ire.GetErrorString())
724+
} else {
725+
// Other errors (like bucket not found) are acceptable
726+
t.Logf("Search failed with error (may be expected if bucket doesn't exist): %v", err)
727+
}
728+
} else {
729+
t.Errorf("unexpected error type: %T: %v", err, err)
730+
}
731+
return
732+
}
733+
734+
// If we get here, the search succeeded (expected behavior)
735+
if len(items) == 0 {
736+
t.Error("expected at least 1 item from Search")
737+
} else {
738+
t.Logf("Search succeeded: found %v item(s)", len(items))
739+
}
740+
})
741+
}

k8s-source/adapters/configmap_test.go

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package adapters
22

3-
import "testing"
3+
import (
4+
"testing"
5+
6+
"github.com/overmindtech/cli/sdp-go"
7+
)
48

59
var configMapYAML = `
610
apiVersion: v1
@@ -12,6 +16,16 @@ data:
1216
APP_SECRET_KEY: "mysecretkey123"
1317
`
1418

19+
var configMapWithS3ARNYAML = `
20+
apiVersion: v1
21+
kind: ConfigMap
22+
metadata:
23+
name: configmap-with-s3-arn
24+
data:
25+
S3_BUCKET_ARN: "arn:aws:s3:::example-bucket-name"
26+
S3_BUCKET_NAME: "example-bucket-name"
27+
`
28+
1529
func TestConfigMapAdapter(t *testing.T) {
1630
sd := ScopeDetails{
1731
ClusterName: CurrentCluster.Name,
@@ -30,3 +44,29 @@ func TestConfigMapAdapter(t *testing.T) {
3044

3145
st.Execute(t)
3246
}
47+
48+
func TestConfigMapAdapterWithS3ARN(t *testing.T) {
49+
sd := ScopeDetails{
50+
ClusterName: CurrentCluster.Name,
51+
Namespace: "default",
52+
}
53+
54+
adapter := newConfigMapAdapter(CurrentCluster.ClientSet, sd.ClusterName, []string{sd.Namespace})
55+
56+
st := AdapterTests{
57+
Adapter: adapter,
58+
GetQuery: "configmap-with-s3-arn",
59+
GetScope: sd.String(),
60+
SetupYAML: configMapWithS3ARNYAML,
61+
GetQueryTests: QueryTests{
62+
{
63+
ExpectedType: "s3-bucket",
64+
ExpectedMethod: sdp.QueryMethod_SEARCH,
65+
ExpectedQuery: "arn:aws:s3:::example-bucket-name",
66+
ExpectedScope: sdp.WILDCARD,
67+
},
68+
},
69+
}
70+
71+
st.Execute(t)
72+
}

0 commit comments

Comments
 (0)