chore(mixins-preview): delivery destinations for vended logs #36087

ShadowCat567 · 2025-11-17T19:47:39Z

Reason for this change

Adds ground work for vended logs mixin.

Description of changes

Sets up mini-constructs to be used by the mixin that assists in setting up vended logs.
Max length of name for delivery destination is 60 characters (see: https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_PutDeliveryDestination.html).

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

mrgrain · 2025-11-18T12:36:04Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+interface IDeliveryDestination extends IResource {
+  readonly destinationArn: string;
+}


Instead of this, we can use the existing IDeliveryDestinationRef

Suggested change

interface IDeliveryDestination extends IResource {

readonly destinationArn: string;

}

interface IDeliveryDestination extends IResource {

readonly destinationArn: string;

}

mrgrain · 2025-11-18T12:44:23Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+  readonly destinationArn: string;
+}
+
+abstract class DeliveryDestinationBase extends Resource implements IDeliveryDestination {


Suggested change

abstract class DeliveryDestinationBase extends Resource implements IDeliveryDestination {

abstract class DeliveryDestinationBase extends Resource implements IDeliveryDestinationRef {

mrgrain · 2025-11-18T12:45:46Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+export class S3DeliveryDestination extends DeliveryDestinationBase {
+  public readonly destinationArn: string;
+  constructor(scope: Construct, id: string, props: DeliveryDestinationProps & Required<Pick<DeliveryDestinationProps, 's3Bucket'>>) {


This doesn't compile in jsii. Really we need 3 separate Props objects.

mrgrain · 2025-11-18T12:47:46Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+    const bucketPolicy = new CfnBucketPolicy(scope, 'S3Policy', {
+      bucket: props.s3Bucket.bucketName,
+      policyDocument: bucketPolicyDoc.toJSON(),
+    });


Here is a fun side quest: We need a helper function, that for a given Stack and IBucketRef goes through the whole stack and tries to find an existing CfnBucketPolicy for that IBucketRef.

If we find one, we use that. Otherwise we create a new one.

mrgrain · 2025-11-18T12:48:06Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+    const destinationNamePrefix = 's3-delivery-destination-';
+    const deliveryDestination = new CfnDeliveryDestination(scope, 'S3DeliveryDestination', {
+      destinationResourceArn: props.s3Bucket.bucketArn,
+      name: `${destinationNamePrefix}${Names.uniqueResourceName(props.s3Bucket, { maxLength: 60 - destinationNamePrefix.length })}`,


mrgrain · 2025-11-18T12:48:58Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+      policyDocument: bucketPolicyDoc.toJSON(),
+    });
+
+    const destinationNamePrefix = 's3-delivery-destination-';


let's find a shorter prefix. Maybe something with cdk

mrgrain · 2025-11-18T12:56:39Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+  constructor(scope: Construct, id: string, props: DeliveryDestinationProps & Required<Pick<DeliveryDestinationProps, 'logGroup'>>) {
+    super(scope, id);
+
+    const logGroupPolicy = new CfnResourcePolicy(scope, 'CloudwatchDeliveryPolicy', {


For this one I'd like us to create a new singleton resource policy for delivery destination. We will getOrCreate that policy and than add statements to it.

This is so that when a user has multiple CloudwatchDeliveryDestinations in their stack, we only ever add a single policy. Because they are limited to max 10 per account.

mrgrain · 2025-11-18T12:57:12Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+  }
+}
+
+export class CloudwatchDeliveryDestination extends DeliveryDestinationBase {


This one I think would be this though if we follow the service name plus stuff pattern.

Suggested change

export class CloudwatchDeliveryDestination extends DeliveryDestinationBase {

export class LogsDeliveryDestination extends DeliveryDestinationBase {

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

mrgrain · 2025-11-18T13:07:58Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+      bucketPolicyDoc.addStatements(v1BucketPolicy);
+    }
+
+    const bucketPolicy = new CfnBucketPolicy(scope, 'S3Policy', {


you are allowed to use a L2 BucketPolicy here if it helps

I don't think addDependency works with L2s, since currently I am using addDependency to ensure we don't create our deliveryDestination before it has sufficient permissions. However, considering we want our policies to largely be singletons, I may have to rethink the use of it.

tangent: Curious about the problem here, this might be another L1/L2 paper-cut we should be fixing.

mrgrain

Also needs unit tests ;)

mrgrain · 2025-11-19T16:25:10Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+interface DeliveryDestinationProps {
+  readonly permissionsVersion: 'V1' | 'V2';
+  readonly destinationService: 'S3' | 'CWL' | 'FH' | 'XRAY';


this is very redundant we know which service based on the class

mrgrain · 2025-11-19T16:25:31Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+}
+
+interface DeliveryDestinationProps {
+  readonly permissionsVersion: 'V1' | 'V2';


this is only used for S3, right?

mrgrain · 2025-11-19T16:26:14Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+    this.deliveryDestinationRef = deliveryDestination.deliveryDestinationRef;
+  }
+
+  getOrCreateBucketPolicy(stack: Construct, bucketProps: S3DestinationProps): BucketPolicy {


can you move this search part into a helper function in a separate file please and than make sure you have good test coverage.

mrgrain · 2025-11-19T16:26:47Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+  getOrCreateBucketPolicy(stack: Construct, bucketProps: S3DestinationProps): BucketPolicy {
+    const allConstructs = stack.node.findAll();
+    const bucketPolicies = allConstructs.filter(construct => construct instanceof BucketPolicy) as BucketPolicy[];


You can't assume there's an L2. Look for the L1 instead.

mrgrain · 2025-11-19T16:28:39Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+    const allConstructs = stack.node.findAll();
+    const resourcePolicies = allConstructs.filter(
+      construct => construct instanceof ResourcePolicy &&
+       CfnResource.isCfnResource(construct.node.defaultChild) &&
+       construct.node.defaultChild.cfnResourceType === 'AWS::Logs::ResourcePolicy',
+    ) as ResourcePolicy[];


because Log Resource Policies are not attached to a resource we can use a normal singleton pattern. have a look how this is done elsewhere in the CDK

mrgrain · 2025-11-19T17:25:26Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+export class XRayDeliveryDestination extends DeliveryDestinationBase {
+  public readonly deliveryDestinationRef: DeliveryDestinationReference;
+  constructor(scope: Construct, id: string, props: DeliveryDestinationProps) {


Suggested change

constructor(scope: Construct, id: string, props: DeliveryDestinationProps) {

constructor(scope: IConstruct, id: string, props: DeliveryDestinationProps) {

mrgrain · 2025-11-19T17:25:36Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+export class LogsDeliveryDestination extends DeliveryDestinationBase {
+  public readonly deliveryDestinationRef: DeliveryDestinationReference;
+  constructor(scope: Construct, id: string, props: LogsDestinationProps) {


Suggested change

constructor(scope: Construct, id: string, props: LogsDestinationProps) {

constructor(scope: IConstruct, id: string, props: LogsDestinationProps) {

mrgrain · 2025-11-19T17:26:16Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+export class S3DeliveryDestination extends DeliveryDestinationBase {
+  public readonly deliveryDestinationRef: DeliveryDestinationReference;
+  constructor(scope: Construct, id: string, props: S3DestinationProps) {


Pretty sure we want IConstruct where ever possible. I might be wrong though :D

Suggested change

constructor(scope: Construct, id: string, props: S3DestinationProps) {

constructor(scope: IConstruct, id: string, props: S3DestinationProps) {

mrgrain · 2025-11-19T17:28:12Z

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts

+
+    const destinationNamePrefix = 'cdk-xray-dest-';
+    const deliveryDestination = new CfnDeliveryDestination(scope, 'CDKXRayDeliveryDestination', {
+      name: `${destinationNamePrefix}${Names.uniqueResourceName(scope, { maxLength: 60 - destinationNamePrefix.length })}`, // there is no resource for XRays


Does it work if you do it like that?

Suggested change

name: `${destinationNamePrefix}${Names.uniqueResourceName(scope, { maxLength: 60 - destinationNamePrefix.length })}`, // there is no resource for XRays

name: `${destinationNamePrefix}${Names.uniqueResourceName(this, { maxLength: 60 - destinationNamePrefix.length })}`, // there is no resource for XRays

aws-amplify-bot added 3 commits November 17, 2025 14:33

delivery destinations

d8df3eb

Merge branch 'main' into vended-log-destinations

a6280d8

adjust spacing

9c948f1

github-actions bot added the p2 label Nov 17, 2025

aws-cdk-automation requested a review from a team November 17, 2025 19:47

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 17, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 17, 2025

aws-cdk-automation added the pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes. label Nov 17, 2025

xray and naming adjustment

f552399

mrgrain reviewed Nov 18, 2025

View reviewed changes

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts Show resolved Hide resolved

mrgrain reviewed Nov 18, 2025

View reviewed changes

packages/@aws-cdk/mixins-preview/lib/services/aws-logs/vended-logs.ts Show resolved Hide resolved

mrgrain reviewed Nov 18, 2025

View reviewed changes

mrgrain requested changes Nov 18, 2025

View reviewed changes

aws-amplify-bot added 4 commits November 18, 2025 13:44

policy singletons

0a7fc27

add tests

0ca8f9b

lint fixes

4728e6e

adjustments and fix test

eaf96ac

mrgrain reviewed Nov 19, 2025

View reviewed changes

aws-amplify-bot added 4 commits November 19, 2025 13:20

xrays and updates

db3fb43

split files

c443a79

Merge branch 'main' into vended-log-destinations

227060b

adjust policy generator class

c2fbdfa

	abstract class DeliveryDestinationBase extends Resource implements IDeliveryDestination {
	abstract class DeliveryDestinationBase extends Resource implements IDeliveryDestinationRef {

	export class CloudwatchDeliveryDestination extends DeliveryDestinationBase {
	export class LogsDeliveryDestination extends DeliveryDestinationBase {

	constructor(scope: Construct, id: string, props: DeliveryDestinationProps) {
	constructor(scope: IConstruct, id: string, props: DeliveryDestinationProps) {

	name: `${destinationNamePrefix}${Names.uniqueResourceName(scope, { maxLength: 60 - destinationNamePrefix.length })}`, // there is no resource for XRays
	name: `${destinationNamePrefix}${Names.uniqueResourceName(this, { maxLength: 60 - destinationNamePrefix.length })}`, // there is no resource for XRays

chore(mixins-preview): delivery destinations for vended logs #36087

Are you sure you want to change the base?

chore(mixins-preview): delivery destinations for vended logs #36087

Conversation

ShadowCat567 commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrgrain Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrgrain Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShadowCat567 commented Nov 17, 2025 •

edited

Loading

mrgrain Nov 18, 2025 •

edited

Loading

mrgrain Nov 19, 2025 •

edited

Loading