Skip to content

Commit fc2f717

Browse files
authored
chore(mixins-preview): improved find bucket policy (#36144)
### Reason for this change A helper function to reflect on the construct tree to find the closet Bucket Policy for a Bucket. ### Description of changes Implemented a generic function to find two related resources that are "close" to each other. Close is defined here as: - the closest (transitive) child matching the predicate - the above to the closest parent Than added a specific function just for bucket. For now this is hand-written and not publicly exposed. If we can identify these kind of connection patterns, we can likely code gen this for all resources. ### Describe any new or updated permissions being added n/a ### Description of how you validated changes Unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent a6c0288 commit fc2f717

File tree

4 files changed

+336
-3
lines changed

4 files changed

+336
-3
lines changed
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
import type { IConstruct } from 'constructs';
2+
import { CfnResource } from 'aws-cdk-lib/core';
3+
import type { IBucketRef, CfnBucketPolicy } from 'aws-cdk-lib/aws-s3';
4+
5+
/**
6+
* Finds the closest related resource in the construct tree.
7+
* Searches children first (depth-first), then ancestors (breadth-first).
8+
*
9+
* @param primary - The construct to search a related resource for
10+
* @param relatedCfnResourceType - The CloudFormation resource type to search for
11+
* @param isConnected - Predicate to determine if a candidate is related to the primary
12+
* @returns The closest matching resource, or undefined if none found
13+
*/
14+
function findClosestRelatedResource<TPrimary extends IConstruct, TRelated extends CfnResource>(
15+
primary: TPrimary,
16+
relatedCfnResourceType: string,
17+
isConnected: (primary: TPrimary, candidate: TRelated) => boolean,
18+
): TRelated | undefined {
19+
let closestMatch: TRelated | undefined;
20+
let closestDistance = Infinity;
21+
const visited = new Set<IConstruct>();
22+
23+
const isRelatedResource = (child: IConstruct): child is TRelated => {
24+
return CfnResource.isCfnResource(child) && child.cfnResourceType === relatedCfnResourceType;
25+
};
26+
27+
const checkCandidate = (candidate: IConstruct, distance: number) => {
28+
// Check if candidate is the right type, connected to primary, and closer than current match
29+
if (isRelatedResource(candidate) && isConnected(primary, candidate) && distance < closestDistance) {
30+
closestMatch = candidate;
31+
closestDistance = distance;
32+
}
33+
};
34+
35+
// Search all descendants depth-first
36+
const searchChildren = (parent: IConstruct, distance: number) => {
37+
// Stop searching if we're already farther than the closest match
38+
if (distance >= closestDistance) {
39+
return;
40+
}
41+
42+
// Check each child and recursively search its descendants
43+
for (const child of parent.node.children) {
44+
// Skip if already visited
45+
if (visited.has(child)) {
46+
continue;
47+
}
48+
visited.add(child);
49+
50+
checkCandidate(child, distance);
51+
searchChildren(child, distance + 1);
52+
}
53+
};
54+
55+
searchChildren(primary, 1);
56+
57+
// Search ancestors and their descendants breadth-first
58+
let ancestor = primary.node.scope;
59+
let ancestorDistance = 1;
60+
61+
// Walk up the tree while we have ancestors and haven't exceeded closest distance
62+
while (ancestor && ancestorDistance < closestDistance) {
63+
// Check all siblings and their descendants at this ancestor level
64+
for (const sibling of ancestor.node.children) {
65+
searchChildren(sibling, ancestorDistance);
66+
}
67+
ancestor = ancestor.node.scope;
68+
ancestorDistance++;
69+
}
70+
71+
return closestMatch;
72+
}
73+
74+
/**
75+
* Attempts to find an existing bucket policy for the specified S3 bucket.
76+
* Finds the closest matching policy to the specified bucket.
77+
*
78+
* @param bucket - The S3 bucket reference to search for an associated bucket policy
79+
* @returns The bucket policy if found, undefined otherwise
80+
*/
81+
export function tryFindBucketPolicyForBucket(bucket: IBucketRef): CfnBucketPolicy | undefined {
82+
return findClosestRelatedResource<IBucketRef, CfnBucketPolicy>(
83+
bucket,
84+
'AWS::S3::BucketPolicy',
85+
(b: any, policy) => {
86+
const possibleRefs = new Set([b.ref, b.bucketName, b.bucketArn, b.bucketRef?.bucketName, b.bucketRef?.bucketArn].filter(Boolean));
87+
return possibleRefs.has(policy.bucket) || String(policy.bucket).includes(b.node.id);
88+
},
89+
);
90+
}

packages/@aws-cdk/mixins-preview/lib/services/aws-s3/bucket.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import * as s3 from 'aws-cdk-lib/aws-s3';
33
import { CfnResource, CustomResource, Tags } from 'aws-cdk-lib/core';
44
import { AutoDeleteObjectsProvider } from '../../custom-resource-handlers/aws-s3/auto-delete-objects-provider';
55
import type { IMixin } from '../../core';
6+
import { tryFindBucketPolicyForBucket } from '../../mixins/private/reflections';
67

78
const AUTO_DELETE_OBJECTS_RESOURCE_TYPE = 'Custom::S3AutoDeleteObjects';
89
const AUTO_DELETE_OBJECTS_TAG = 'aws-cdk:auto-delete-objects';
@@ -29,7 +30,7 @@ export class AutoDeleteObjects implements IMixin {
2930
});
3031

3132
// Get or create bucket policy
32-
let policy = construct.node.tryFindChild('Policy') as s3.CfnBucketPolicy | undefined;
33+
let policy = tryFindBucketPolicyForBucket(construct);
3334
if (!policy) {
3435
policy = new s3.CfnBucketPolicy(construct, 'Policy', {
3536
bucket: ref.bucketName,
Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
import { Construct } from 'constructs';
2+
import { Stack, App, Resource } from 'aws-cdk-lib/core';
3+
import * as s3 from 'aws-cdk-lib/aws-s3';
4+
import { tryFindBucketPolicyForBucket } from '../../lib/mixins/private/reflections';
5+
6+
class CustomBucket extends Resource implements s3.IBucketRef {
7+
public readonly bucketRef: s3.BucketReference;
8+
9+
constructor(scope: Construct, id: string, bucketName: string) {
10+
super(scope, id);
11+
this.bucketRef = {
12+
bucketName,
13+
bucketArn: `arn:aws:s3:::${bucketName}`,
14+
};
15+
}
16+
}
17+
18+
describe('find bucket policy', () => {
19+
let app: App;
20+
let stack: Stack;
21+
22+
beforeEach(() => {
23+
app = new App();
24+
stack = new Stack(app, 'TestStack');
25+
});
26+
27+
describe('find in construct tree', () => {
28+
test('returns undefined when no bucket policy exists', () => {
29+
const bucket = new s3.CfnBucket(stack, 'Bucket');
30+
expect(tryFindBucketPolicyForBucket(bucket)).toBeUndefined();
31+
});
32+
33+
test('finds bucket policy as direct child of bucket', () => {
34+
const bucket = new s3.CfnBucket(stack, 'Bucket');
35+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
36+
bucket: bucket.ref,
37+
policyDocument: {},
38+
});
39+
40+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
41+
});
42+
43+
test('finds bucket policy as transitive child of bucket', () => {
44+
const bucket = new s3.CfnBucket(stack, 'Bucket');
45+
const intermediate = new Construct(bucket, 'Intermediate');
46+
const policy = new s3.CfnBucketPolicy(intermediate, 'Policy', {
47+
bucket: bucket.ref,
48+
policyDocument: {},
49+
});
50+
51+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
52+
});
53+
54+
test('finds bucket policy as sibling (child of parent)', () => {
55+
const parent = new Construct(stack, 'Parent');
56+
const bucket = new s3.CfnBucket(parent, 'Bucket');
57+
const policy = new s3.CfnBucketPolicy(parent, 'Policy', {
58+
bucket: bucket.ref,
59+
policyDocument: {},
60+
});
61+
62+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
63+
});
64+
65+
test('finds bucket policy in parent hierarchy', () => {
66+
const grandparent = new Construct(stack, 'Grandparent');
67+
const parent = new Construct(grandparent, 'Parent');
68+
const bucket = new s3.CfnBucket(parent, 'Bucket');
69+
const policy = new s3.CfnBucketPolicy(grandparent, 'Policy', {
70+
bucket: bucket.ref,
71+
policyDocument: {},
72+
});
73+
74+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
75+
});
76+
77+
test('finds cousin bucket policies', () => {
78+
const grandparent = new Construct(stack, 'Grandparent');
79+
const parent = new Construct(grandparent, 'Parent');
80+
const auncle = new Construct(grandparent, 'Auncle');
81+
82+
const bucket = new s3.CfnBucket(parent, 'Bucket');
83+
const policy = new s3.CfnBucketPolicy(auncle, 'Policy', {
84+
bucket: bucket.ref,
85+
policyDocument: {},
86+
});
87+
88+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
89+
});
90+
91+
test('prefers closest child over parent policy', () => {
92+
const parent = new Construct(stack, 'Parent');
93+
const bucket = new s3.CfnBucket(parent, 'Bucket');
94+
const childPolicy = new s3.CfnBucketPolicy(bucket, 'ChildPolicy', {
95+
bucket: bucket.ref,
96+
policyDocument: {},
97+
});
98+
new s3.CfnBucketPolicy(parent, 'ParentPolicy', {
99+
bucket: bucket.ref,
100+
policyDocument: {},
101+
});
102+
103+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(childPolicy);
104+
});
105+
106+
test('prefers closer transitive child over distant one', () => {
107+
const bucket = new s3.CfnBucket(stack, 'Bucket');
108+
const child1 = new Construct(bucket, 'Child1');
109+
const closerPolicy = new s3.CfnBucketPolicy(child1, 'CloserPolicy', {
110+
bucket: bucket.ref,
111+
policyDocument: {},
112+
});
113+
const child2 = new Construct(bucket, 'Child2');
114+
const intermediate = new Construct(child2, 'Intermediate');
115+
new s3.CfnBucketPolicy(intermediate, 'FartherPolicy', {
116+
bucket: bucket.ref,
117+
policyDocument: {},
118+
});
119+
120+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(closerPolicy);
121+
});
122+
123+
test('prefers closer parent over distant parent', () => {
124+
const grandparent = new Construct(stack, 'Grandparent');
125+
const parent = new Construct(grandparent, 'Parent');
126+
const bucket = new s3.CfnBucket(parent, 'Bucket');
127+
const closerPolicy = new s3.CfnBucketPolicy(parent, 'CloserPolicy', {
128+
bucket: bucket.ref,
129+
policyDocument: {},
130+
});
131+
new s3.CfnBucketPolicy(grandparent, 'FartherPolicy', {
132+
bucket: bucket.ref,
133+
policyDocument: {},
134+
});
135+
136+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(closerPolicy);
137+
});
138+
139+
test('ignores unrelated bucket policies', () => {
140+
const bucket1 = new s3.CfnBucket(stack, 'Bucket1');
141+
const bucket2 = new s3.CfnBucket(stack, 'Bucket2');
142+
new s3.CfnBucketPolicy(bucket2, 'Policy', {
143+
bucket: bucket2.ref,
144+
policyDocument: {},
145+
});
146+
147+
expect(tryFindBucketPolicyForBucket(bucket1)).toBeUndefined();
148+
});
149+
});
150+
151+
describe('matches different buckets', () => {
152+
test('matches L2 Bucket with policy using bucketName', () => {
153+
const bucket = new s3.Bucket(stack, 'Bucket');
154+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
155+
bucket: bucket.bucketName,
156+
policyDocument: {},
157+
});
158+
159+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
160+
});
161+
162+
test('matches L2 Bucket with policy using bucketRef.bucketName', () => {
163+
const bucket = new s3.Bucket(stack, 'Bucket');
164+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
165+
bucket: bucket.bucketRef.bucketName,
166+
policyDocument: {},
167+
});
168+
169+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
170+
});
171+
172+
test('matches L2 Bucket with policy using bucketArn', () => {
173+
const bucket = new s3.Bucket(stack, 'Bucket');
174+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
175+
bucket: bucket.bucketArn,
176+
policyDocument: {},
177+
});
178+
179+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
180+
});
181+
182+
test('matches L2 Bucket', () => {
183+
const bucket = new s3.Bucket(stack, 'Bucket');
184+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
185+
bucket: bucket,
186+
policyDocument: {},
187+
});
188+
189+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
190+
});
191+
192+
test('matches L1 Bucket', () => {
193+
const bucket = new s3.CfnBucket(stack, 'Bucket');
194+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
195+
bucket: bucket,
196+
policyDocument: {},
197+
});
198+
199+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
200+
});
201+
202+
test('matches custom IBucketRef implementation', () => {
203+
const bucket = new CustomBucket(stack, 'CustomBucket', 'my-bucket');
204+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
205+
bucket: bucket,
206+
policyDocument: {},
207+
});
208+
209+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
210+
});
211+
212+
test('matches L2 BucketPolicy', () => {
213+
const bucket = new s3.Bucket(stack, 'Bucket');
214+
const policy = new s3.BucketPolicy(bucket, 'Policy', {
215+
bucket: bucket,
216+
});
217+
218+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy.node.defaultChild);
219+
});
220+
221+
test('ignores policy for different bucket name', () => {
222+
const bucket = new s3.CfnBucket(stack, 'Bucket');
223+
bucket.bucketName = 'my-bucket';
224+
new s3.CfnBucketPolicy(bucket, 'Policy', {
225+
bucket: 'other-bucket',
226+
policyDocument: {},
227+
});
228+
229+
expect(tryFindBucketPolicyForBucket(bucket)).toBeUndefined();
230+
});
231+
232+
test('matches using attrArn', () => {
233+
const bucket = new s3.CfnBucket(stack, 'Bucket');
234+
const policy = new s3.CfnBucketPolicy(bucket, 'Policy', {
235+
bucket: bucket.attrArn,
236+
policyDocument: {},
237+
});
238+
239+
expect(tryFindBucketPolicyForBucket(bucket)).toBe(policy);
240+
});
241+
});
242+
});

packages/aws-cdk-lib/core/lib/cfn-element.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,14 +177,14 @@ export abstract class CfnElement extends Construct {
177177
}
178178

179179
/**
180-
* Base class for referencable CloudFormation constructs which are not Resources
180+
* Base class for referenceable CloudFormation constructs which are not Resources
181181
*
182182
* These constructs are things like Conditions and Parameters, can be
183183
* referenced by taking the `.ref` attribute.
184184
*
185185
* Resource constructs do not inherit from CfnRefElement because they have their
186186
* own, more specific types returned from the .ref attribute. Also, some
187-
* resources aren't referencable at all (such as BucketPolicies or GatewayAttachments).
187+
* resources aren't referenceable at all (such as BucketPolicies or GatewayAttachments).
188188
*/
189189
export abstract class CfnRefElement extends CfnElement {
190190
/**

0 commit comments

Comments
 (0)