Skip to content

Commit 2cf4d94

Browse files
fix(discovery): correct cross-region S3 object listing and firewall-scoped LoggingConfiguration discovery
Two discovery-path errors surfaced as recurring failures in the agent log. S3 object discovery enumerates every bucket in the account, but ListObjectsV2 must be addressed to the bucket's home region. A bucket in a different region than the configured client answered with a 301 PermanentRedirect, so discovery logged a list error and skipped that bucket's objects. The list path now catches the redirect, reads the bucket's home region from the x-amz-bucket-region response header, and retries the request against that region. CloudControl does not support the LIST action for AWS::NetworkFirewall::LoggingConfiguration: ListResources returns an UnsupportedActionException, so background discovery hit a 400 on this type every cycle. A logging configuration is a per-firewall singleton, and CloudControl's GetResource keyed by the firewall ARN does work, so the plugin now registers a custom List that, scoped to one firewall via the FirewallArn list parameter, reads that firewall's logging configuration and returns its identifier when log destinations are configured. The Firewall is declared as the parent so discovery iterates firewalls and lists each one's logging configuration. Only List is custom; create, read, update, delete, and status continue to use the CloudControl path. A conformance fixture with the logging configuration as the leaf resource exercises both the CRUD and discovery lifecycles.
1 parent bb454b2 commit 2cf4d94

7 files changed

Lines changed: 459 additions & 3 deletions

File tree

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
// © 2025 Platform Engineering Labs Inc.
2+
//
3+
// SPDX-License-Identifier: FSL-1.1-ALv2
4+
5+
package networkfirewall
6+
7+
import (
8+
"context"
9+
"encoding/json"
10+
"fmt"
11+
12+
"github.com/platform-engineering-labs/formae/pkg/plugin/resource"
13+
14+
"github.com/platform-engineering-labs/formae-plugin-aws/pkg/ccx"
15+
"github.com/platform-engineering-labs/formae-plugin-aws/pkg/cfres/prov"
16+
"github.com/platform-engineering-labs/formae-plugin-aws/pkg/cfres/registry"
17+
"github.com/platform-engineering-labs/formae-plugin-aws/pkg/config"
18+
)
19+
20+
const loggingConfigurationType = "AWS::NetworkFirewall::LoggingConfiguration"
21+
22+
// loggingConfigClient abstracts the CloudControl read this List synthesizes from,
23+
// so it can be mocked in unit tests. *ccx.Client satisfies this interface.
24+
type loggingConfigClient interface {
25+
ReadResource(ctx context.Context, request *resource.ReadRequest) (*resource.ReadResult, error)
26+
}
27+
28+
// LoggingConfiguration supplies a custom List for AWS::NetworkFirewall::LoggingConfiguration.
29+
// CloudControl does not support the LIST action for this type (ListResources returns
30+
// UnsupportedActionException), so discovery cannot enumerate it the generic way. A
31+
// logging configuration is a per-firewall singleton, though, and CloudControl's GetResource
32+
// (keyed by the firewall ARN) does work. Discovery scopes this List to one firewall via the
33+
// FirewallArn list parameter; we read that firewall's logging configuration and return its
34+
// identifier when destinations are configured. Only List is registered — Create/Read/Update/
35+
// Delete/Status fall through to the generic CloudControl path in aws.go.
36+
type LoggingConfiguration struct {
37+
cfg *config.Config
38+
// client is injectable for testing; nil means construct a real ccx.Client.
39+
client loggingConfigClient
40+
}
41+
42+
var _ prov.Provisioner = &LoggingConfiguration{}
43+
44+
func init() {
45+
registry.Register(loggingConfigurationType,
46+
[]resource.Operation{resource.OperationList},
47+
func(cfg *config.Config) prov.Provisioner {
48+
return &LoggingConfiguration{cfg: cfg}
49+
})
50+
}
51+
52+
func (l *LoggingConfiguration) getClient() (loggingConfigClient, error) {
53+
if l.client != nil {
54+
return l.client, nil
55+
}
56+
return ccx.NewClient(l.cfg)
57+
}
58+
59+
// List returns the firewall's ARN (the logging configuration's identifier) when the
60+
// firewall named by the FirewallArn list parameter has at least one log destination
61+
// configured. A firewall with no logging, or one that no longer exists, contributes
62+
// nothing. Discovery reads each returned identifier back through the generic Read path.
63+
func (l *LoggingConfiguration) List(ctx context.Context, request *resource.ListRequest) (*resource.ListResult, error) {
64+
firewallArn := request.AdditionalProperties["FirewallArn"]
65+
if firewallArn == "" {
66+
return nil, fmt.Errorf("FirewallArn is required to list network firewall logging configurations")
67+
}
68+
69+
client, err := l.getClient()
70+
if err != nil {
71+
return nil, fmt.Errorf("creating cloudcontrol client: %w", err)
72+
}
73+
74+
result, err := client.ReadResource(ctx, &resource.ReadRequest{
75+
NativeID: firewallArn,
76+
ResourceType: loggingConfigurationType,
77+
TargetConfig: request.TargetConfig,
78+
})
79+
if err != nil {
80+
return nil, fmt.Errorf("reading logging configuration for firewall %s: %w", firewallArn, err)
81+
}
82+
if result.ErrorCode == resource.OperationErrorCodeNotFound {
83+
return &resource.ListResult{}, nil
84+
}
85+
if result.ErrorCode != "" {
86+
return nil, fmt.Errorf("reading logging configuration for firewall %s: %s", firewallArn, result.ErrorCode)
87+
}
88+
if !hasLogDestinations(result.Properties) {
89+
return &resource.ListResult{}, nil
90+
}
91+
return &resource.ListResult{NativeIDs: []string{firewallArn}}, nil
92+
}
93+
94+
// hasLogDestinations reports whether a read logging configuration has at least one
95+
// destination configured. An empty configuration is not a discoverable resource.
96+
func hasLogDestinations(properties string) bool {
97+
if properties == "" {
98+
return false
99+
}
100+
var p struct {
101+
LoggingConfiguration struct {
102+
LogDestinationConfigs []json.RawMessage `json:"LogDestinationConfigs"`
103+
} `json:"LoggingConfiguration"`
104+
}
105+
if err := json.Unmarshal([]byte(properties), &p); err != nil {
106+
return false
107+
}
108+
return len(p.LoggingConfiguration.LogDestinationConfigs) > 0
109+
}
110+
111+
// The remaining Provisioner methods are unreachable: only List is registered, so
112+
// Create/Read/Update/Delete/Status always route to CloudControl in aws.go.
113+
func (l *LoggingConfiguration) Create(_ context.Context, _ *resource.CreateRequest) (*resource.CreateResult, error) {
114+
return nil, fmt.Errorf("create not implemented - cloudcontrol handles this")
115+
}
116+
117+
func (l *LoggingConfiguration) Read(_ context.Context, _ *resource.ReadRequest) (*resource.ReadResult, error) {
118+
return nil, fmt.Errorf("read not implemented - cloudcontrol handles this")
119+
}
120+
121+
func (l *LoggingConfiguration) Update(_ context.Context, _ *resource.UpdateRequest) (*resource.UpdateResult, error) {
122+
return nil, fmt.Errorf("update not implemented - cloudcontrol handles this")
123+
}
124+
125+
func (l *LoggingConfiguration) Delete(_ context.Context, _ *resource.DeleteRequest) (*resource.DeleteResult, error) {
126+
return nil, fmt.Errorf("delete not implemented - cloudcontrol handles this")
127+
}
128+
129+
func (l *LoggingConfiguration) Status(_ context.Context, _ *resource.StatusRequest) (*resource.StatusResult, error) {
130+
return nil, fmt.Errorf("status not implemented - cloudcontrol handles this")
131+
}
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
// © 2025 Platform Engineering Labs Inc.
2+
//
3+
// SPDX-License-Identifier: FSL-1.1-ALv2
4+
5+
//go:build unit
6+
7+
package networkfirewall
8+
9+
import (
10+
"context"
11+
"testing"
12+
13+
"github.com/platform-engineering-labs/formae/pkg/plugin/resource"
14+
"github.com/stretchr/testify/mock"
15+
"github.com/stretchr/testify/require"
16+
)
17+
18+
const testFirewallArn = "arn:aws:network-firewall:us-east-1:111122223333:firewall/fw-1"
19+
20+
func readReq(r *resource.ReadRequest) bool {
21+
return r.NativeID == testFirewallArn && r.ResourceType == loggingConfigurationType
22+
}
23+
24+
func TestLoggingConfigurationList_ReturnsFirewallArnWhenLoggingConfigured(t *testing.T) {
25+
ctx := context.Background()
26+
client := &mockCCXClient{}
27+
client.On("ReadResource", ctx, mock.MatchedBy(readReq)).Return(&resource.ReadResult{
28+
Properties: `{"FirewallArn":"` + testFirewallArn + `","LoggingConfiguration":{"LogDestinationConfigs":[` +
29+
`{"LogType":"FLOW","LogDestinationType":"CloudWatchLogs","LogDestination":{"logGroup":"/x"}}]}}`,
30+
}, nil)
31+
32+
l := &LoggingConfiguration{client: client}
33+
result, err := l.List(ctx, &resource.ListRequest{
34+
ResourceType: loggingConfigurationType,
35+
AdditionalProperties: map[string]string{"FirewallArn": testFirewallArn},
36+
})
37+
38+
require.NoError(t, err)
39+
require.Equal(t, []string{testFirewallArn}, result.NativeIDs)
40+
client.AssertExpectations(t)
41+
}
42+
43+
func TestLoggingConfigurationList_EmptyWhenNoDestinations(t *testing.T) {
44+
ctx := context.Background()
45+
client := &mockCCXClient{}
46+
client.On("ReadResource", ctx, mock.MatchedBy(readReq)).Return(&resource.ReadResult{
47+
Properties: `{"FirewallArn":"` + testFirewallArn + `","LoggingConfiguration":{"LogDestinationConfigs":[]}}`,
48+
}, nil)
49+
50+
l := &LoggingConfiguration{client: client}
51+
result, err := l.List(ctx, &resource.ListRequest{
52+
ResourceType: loggingConfigurationType,
53+
AdditionalProperties: map[string]string{"FirewallArn": testFirewallArn},
54+
})
55+
56+
require.NoError(t, err)
57+
require.Empty(t, result.NativeIDs)
58+
client.AssertExpectations(t)
59+
}
60+
61+
func TestLoggingConfigurationList_EmptyWhenFirewallNotFound(t *testing.T) {
62+
ctx := context.Background()
63+
client := &mockCCXClient{}
64+
client.On("ReadResource", ctx, mock.MatchedBy(readReq)).Return(&resource.ReadResult{
65+
ErrorCode: resource.OperationErrorCodeNotFound,
66+
}, nil)
67+
68+
l := &LoggingConfiguration{client: client}
69+
result, err := l.List(ctx, &resource.ListRequest{
70+
ResourceType: loggingConfigurationType,
71+
AdditionalProperties: map[string]string{"FirewallArn": testFirewallArn},
72+
})
73+
74+
require.NoError(t, err)
75+
require.Empty(t, result.NativeIDs)
76+
client.AssertExpectations(t)
77+
}
78+
79+
func TestLoggingConfigurationList_ErrorsWithoutFirewallArn(t *testing.T) {
80+
l := &LoggingConfiguration{}
81+
_, err := l.List(context.Background(), &resource.ListRequest{
82+
ResourceType: loggingConfigurationType,
83+
})
84+
require.Error(t, err)
85+
}

pkg/cfres/s3/object.go

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/aws/aws-sdk-go-v2/aws"
2121
"github.com/aws/aws-sdk-go-v2/service/s3"
2222
s3types "github.com/aws/aws-sdk-go-v2/service/s3/types"
23+
smithyhttp "github.com/aws/smithy-go/transport/http"
2324

2425
"github.com/platform-engineering-labs/formae-plugin-aws/pkg/cfres/prov"
2526
"github.com/platform-engineering-labs/formae-plugin-aws/pkg/cfres/registry"
@@ -70,9 +71,9 @@ func parseNativeID(nativeID string) (bucket, key string, err error) {
7071
}
7172

7273
const (
73-
maxDownloadBytes = 256 << 20
74+
maxDownloadBytes = 256 << 20
7475
maxDecompressedBytes = 256 << 20
75-
fetchTimeout = 5 * time.Minute
76+
fetchTimeout = 5 * time.Minute
7677
)
7778

7879
// resolveBodyWithCloser returns an io.Reader for the object body and a closer function.
@@ -576,6 +577,24 @@ func (o *Object) List(ctx context.Context, request *resource.ListRequest) (*reso
576577
return o.listWithClient(ctx, client, request)
577578
}
578579

580+
// bucketRegionFromRedirect returns the bucket's home region when err is an S3
581+
// 301 PermanentRedirect that carries an x-amz-bucket-region header, so the
582+
// caller can retry the request against the correct region.
583+
func bucketRegionFromRedirect(err error) (string, bool) {
584+
var respErr *smithyhttp.ResponseError
585+
if !errors.As(err, &respErr) {
586+
return "", false
587+
}
588+
if respErr.HTTPStatusCode() != http.StatusMovedPermanently || respErr.Response == nil {
589+
return "", false
590+
}
591+
region := respErr.Response.Header.Get("x-amz-bucket-region")
592+
if region == "" {
593+
return "", false
594+
}
595+
return region, true
596+
}
597+
579598
func (o *Object) listWithClient(ctx context.Context, client s3ObjectClient, request *resource.ListRequest) (*resource.ListResult, error) {
580599
if request.AdditionalProperties == nil {
581600
return nil, fmt.Errorf("BucketName required for listing S3 objects")
@@ -595,7 +614,16 @@ func (o *Object) listWithClient(ctx context.Context, client s3ObjectClient, requ
595614

596615
resp, err := client.ListObjectsV2(ctx, input)
597616
if err != nil {
598-
return nil, fmt.Errorf("failed to list objects in bucket %s: %w", bucketName, err)
617+
// The S3 bucket namespace is global but ListObjectsV2 must be addressed
618+
// to the bucket's home region. A bucket in another region than the
619+
// configured client answers with a 301 PermanentRedirect carrying the
620+
// real region in the x-amz-bucket-region header; retry there.
621+
if region, ok := bucketRegionFromRedirect(err); ok {
622+
resp, err = client.ListObjectsV2(ctx, input, func(o *s3.Options) { o.Region = region })
623+
}
624+
if err != nil {
625+
return nil, fmt.Errorf("failed to list objects in bucket %s: %w", bucketName, err)
626+
}
599627
}
600628

601629
var nativeIDs []string

pkg/cfres/s3/object_mock_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ import (
1515

1616
type mockS3ObjectClient struct {
1717
mock.Mock
18+
// listRegions records the region applied (via option functions) on each
19+
// ListObjectsV2 call, so tests can assert cross-region redirect handling.
20+
listRegions []string
1821
}
1922

2023
func (m *mockS3ObjectClient) PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
@@ -33,6 +36,11 @@ func (m *mockS3ObjectClient) DeleteObject(ctx context.Context, params *s3.Delete
3336
}
3437

3538
func (m *mockS3ObjectClient) ListObjectsV2(ctx context.Context, params *s3.ListObjectsV2Input, optFns ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) {
39+
opts := s3.Options{}
40+
for _, fn := range optFns {
41+
fn(&opts)
42+
}
43+
m.listRegions = append(m.listRegions, opts.Region)
3644
args := m.Called(ctx, params)
3745
return args.Get(0).(*s3.ListObjectsV2Output), args.Error(1)
3846
}

pkg/cfres/s3/object_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/aws/aws-sdk-go-v2/aws"
2626
"github.com/aws/aws-sdk-go-v2/service/s3"
2727
s3types "github.com/aws/aws-sdk-go-v2/service/s3/types"
28+
smithyhttp "github.com/aws/smithy-go/transport/http"
2829
"github.com/platform-engineering-labs/formae/pkg/plugin/resource"
2930
"github.com/stretchr/testify/assert"
3031
"github.com/stretchr/testify/mock"
@@ -554,6 +555,50 @@ func TestList_WithPagination(t *testing.T) {
554555
client.AssertExpectations(t)
555556
}
556557

558+
func TestList_CrossRegionBucket_RetriesWithRedirectedRegion(t *testing.T) {
559+
ctx := context.Background()
560+
client := &mockS3ObjectClient{}
561+
562+
// A bucket that lives in a different region than the client's configured
563+
// region answers ListObjectsV2 with a 301 PermanentRedirect whose
564+
// x-amz-bucket-region header names the bucket's home region.
565+
redirect := &smithyhttp.ResponseError{
566+
Response: &smithyhttp.Response{Response: &http.Response{
567+
StatusCode: http.StatusMovedPermanently,
568+
Header: http.Header{"X-Amz-Bucket-Region": []string{"eu-west-1"}},
569+
}},
570+
Err: errors.New("api error PermanentRedirect: The bucket you are attempting to access must be addressed using the specified endpoint."),
571+
}
572+
573+
// First attempt (default region) redirects; retry (redirected region) succeeds.
574+
client.On("ListObjectsV2", ctx, mock.MatchedBy(func(input *s3.ListObjectsV2Input) bool {
575+
return *input.Bucket == "out-of-region-bucket"
576+
})).Return((*s3.ListObjectsV2Output)(nil), redirect).Once()
577+
client.On("ListObjectsV2", ctx, mock.MatchedBy(func(input *s3.ListObjectsV2Input) bool {
578+
return *input.Bucket == "out-of-region-bucket"
579+
})).Return(&s3.ListObjectsV2Output{
580+
Contents: []s3types.Object{{Key: aws.String("file1.txt")}},
581+
IsTruncated: aws.Bool(false),
582+
}, nil).Once()
583+
584+
o := &Object{}
585+
result, err := o.listWithClient(ctx, client, &resource.ListRequest{
586+
ResourceType: "AWS::S3::Object",
587+
PageSize: 100,
588+
AdditionalProperties: map[string]string{
589+
"BucketName": "out-of-region-bucket",
590+
},
591+
})
592+
593+
require.NoError(t, err)
594+
require.NotNil(t, result)
595+
assert.Equal(t, []string{"out-of-region-bucket|file1.txt"}, result.NativeIDs)
596+
// First call used the default region; the retry targeted the redirected region.
597+
assert.Equal(t, []string{"", "eu-west-1"}, client.listRegions)
598+
599+
client.AssertExpectations(t)
600+
}
601+
557602
func TestList_MissingBucketName(t *testing.T) {
558603
o := &Object{}
559604
result, err := o.listWithClient(context.Background(), nil, &resource.ListRequest{

schema/pkl/networkfirewall/loggingconfiguration.pkl

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,17 @@ open class LoggingConfigurationData extends formae.SubResource {
2727
logDestinationConfigs: Listing<LogDestinationConfig>
2828
}
2929

30+
// CloudControl does not support the LIST action for this type (ListResources
31+
// returns "UnsupportedActionException: ... does not support LIST action"), so the
32+
// AWS plugin registers a custom List that synthesizes it from a per-firewall
33+
// GetResource. Discovery scopes that List to each firewall via the FirewallArn
34+
// list parameter below; the Firewall is the parent, so discovery iterates
35+
// firewalls and lists each one's logging configuration.
3036
@aws.ResourceHint {
3137
type = module.type
3238
identifier = "FirewallArn"
39+
parent = "AWS::NetworkFirewall::Firewall"
40+
listParam = new formae.ListProperty { parentProperty = "FirewallArn" listParameter = "FirewallArn" }
3341
}
3442
open class LoggingConfiguration extends formae.Resource {
3543

0 commit comments

Comments
 (0)