Skip to content

Commit

Permalink
chore(kinesisfirehose-destinations): refactor logging to combine logG…
Browse files Browse the repository at this point in the history
…roup and logging properties into loggingConfig (#31488)

### Reason for this change

The `logging` and `logGroup` properties contain a restriction where a `logGroup` cannot be provided if `logging` is set to `false`. This was previously handled by error handling but we want to change this to make it impossible for a user to run into that scenario in the first place. 

### Description of changes

BREAKING CHANGE: the `logging` and `logGroup` properties in `DestinationLoggingProps` have been removed and replaced with a single optional property `loggingConfig` which accepts a class of type `LoggingConfig`. 

#### Details
Combine the `logging` and `logGroup` properties into a single new optional property called `loggingConfig` which accepts a class of type `LoggingConfig`. 

`LoggingConfig` is an abstract class which can be instantiated through either an instance of `EnableLogging` or `DisableLogging` which can be used in the following 3 ways:

```ts
import * as logs from 'aws-cdk-lib/aws-logs';

const logGroup = new logs.LogGroup(this, 'Log Group');
declare const bucket: s3.Bucket;

// 1. Enable logging with no parameters - a log group will be created for you
const destinationWithLogging = new destinations.S3Bucket(bucket, {
  loggingConfig: new destinations.EnableLogging(),
});

// 2. Enable a logging and pass in a logGroup to be used
const destinationWithLoggingAndMyLogGroup = new destinations.S3Bucket(bucket, {
  loggingConfig: new destinations.EnableLogging(logGroup),
});

// 3. Disable logging (does not accept any parameters so it is now impossible to provide a logGroup in this case)
const destinationWithoutLogging = new destinations.S3Bucket(bucket, {
  loggingConfig: new destinations.DisableLogging(),
});

```

### Description of how you validated changes
unit + integ test

### 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*
…yptionKey
  • Loading branch information
paulhcsun authored Sep 25, 2024
1 parent a22e8cc commit c4bda64
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 54 deletions.
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-kinesisfirehose-alpha/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,16 +183,16 @@ Kinesis Data Firehose will send logs to CloudWatch when data transformation or d
delivery fails. The CDK will enable logging by default and create a CloudWatch LogGroup
and LogStream for your Delivery Stream.

When you create a destination, you can specify a log group. In this log group, The CDK
will create log streams where log events will be sent:
When creating a destination, you can provide an `ILoggingConfig`, which can either be an `EnableLogging` or `DisableLogging` instance.
If you use `EnableLogging`, you can specify a log group where the CDK will create log streams to capture and store log events. For example:

```ts
import * as logs from 'aws-cdk-lib/aws-logs';

const logGroup = new logs.LogGroup(this, 'Log Group');
declare const bucket: s3.Bucket;
const destination = new destinations.S3Bucket(bucket, {
logGroup: logGroup,
loggingConfig: new destinations.EnableLogging(logGroup),
});

new firehose.DeliveryStream(this, 'Delivery Stream', {
Expand All @@ -205,7 +205,7 @@ Logging can also be disabled:
```ts
declare const bucket: s3.Bucket;
const destination = new destinations.S3Bucket(bucket, {
logging: false,
loggingConfig: new destinations.DisableLogging(),
});
new firehose.DeliveryStream(this, 'Delivery Stream', {
destinations: [destination],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as iam from 'aws-cdk-lib/aws-iam';
import * as firehose from '@aws-cdk/aws-kinesisfirehose-alpha';
import * as kms from 'aws-cdk-lib/aws-kms';
import * as logs from 'aws-cdk-lib/aws-logs';
import * as s3 from 'aws-cdk-lib/aws-s3';
import * as cdk from 'aws-cdk-lib/core';
import { ILoggingConfig } from './logging-config';

/**
* Possible compression options Kinesis Data Firehose can use to compress data on delivery.
Expand Down Expand Up @@ -62,20 +62,12 @@ export enum BackupMode {
*/
interface DestinationLoggingProps {
/**
* If true, log errors when data transformation or data delivery fails.
* Configuration that determines whether to log errors during data transformation or delivery failures,
* and specifies the CloudWatch log group for storing error logs.
*
* If `logGroup` is provided, this will be implicitly set to `true`.
*
* @default true - errors are logged.
*/
readonly logging?: boolean;

/**
* The CloudWatch log group where log streams will be created to hold error logs.
*
* @default - if `logging` is set to `true`, a log group will be created for you.
* @default - errors will be logged and a log group will be created for you.
*/
readonly logGroup?: logs.ILogGroup;
readonly loggingConfig?: ILoggingConfig;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './common';
export * from './s3-bucket';
export * from './logging-config';
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import * as logs from 'aws-cdk-lib/aws-logs';

/**
* Configuration interface for logging errors when data transformation or delivery fails.
*
* This interface defines whether logging is enabled and optionally allows specifying a
* CloudWatch Log Group for storing error logs.
*/
export interface ILoggingConfig {
/**
* If true, log errors when data transformation or data delivery fails.
*
* `true` when using `EnableLogging`, `false` when using `DisableLogging`.
*/
readonly logging: boolean;

/**
* The CloudWatch log group where log streams will be created to hold error logs.
*
* @default - if `logging` is set to `true`, a log group will be created for you.
*/
readonly logGroup?: logs.ILogGroup;
}

/**
* Enables logging for error logs with an optional custom CloudWatch log group.
*
* When this class is used, logging is enabled (`logging: true`) and
* you can optionally provide a CloudWatch log group for storing the error logs.
*
* If no log group is provided, a default one will be created automatically.
*/
export class EnableLogging implements ILoggingConfig {
public readonly logGroup?: logs.ILogGroup;
public readonly logging: boolean;

constructor(logGroup?: logs.ILogGroup) {
this.logGroup = logGroup;
this.logging = true;
}
}

/**
* Disables logging for error logs.
*
* When this class is used, logging is disabled (`logging: false`)
* and no CloudWatch log group can be specified.
*/
export class DisableLogging implements ILoggingConfig {
public readonly logging: boolean;

constructor() {
this.logging = false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,16 @@ import * as s3 from 'aws-cdk-lib/aws-s3';
import * as cdk from 'aws-cdk-lib/core';
import { Construct, IDependable, Node } from 'constructs';
import { DestinationS3BackupProps } from '../common';
import { ILoggingConfig } from '../logging-config';

export interface DestinationLoggingProps {
/**
* If true, log errors when data transformation or data delivery fails.
* Configuration that determines whether to log errors during data transformation or delivery failures,
* and specifies the CloudWatch log group for storing error logs.
*
* If `logGroup` is provided, this will be implicitly set to `true`.
*
* @default true - errors are logged.
*/
readonly logging?: boolean;

/**
* The CloudWatch log group where log streams will be created to hold error logs.
*
* @default - if `logging` is set to `true`, a log group will be created for you.
* @default - errors will be logged and a log group will be created for you.
*/
readonly logGroup?: logs.ILogGroup;
readonly loggingConfig?: ILoggingConfig;

/**
* The IAM role associated with this destination.
Expand Down Expand Up @@ -60,11 +53,8 @@ export interface DestinationBackupConfig extends ConfigWithDependables {
}

export function createLoggingOptions(scope: Construct, props: DestinationLoggingProps): DestinationLoggingConfig | undefined {
if (props.logging === false && props.logGroup) {
throw new Error('logging cannot be set to false when logGroup is provided');
}
if (props.logging !== false || props.logGroup) {
const logGroup = props.logGroup ?? Node.of(scope).tryFindChild('LogGroup') as logs.ILogGroup ?? new logs.LogGroup(scope, 'LogGroup');
if (props.loggingConfig?.logging !== false || props.loggingConfig?.logGroup) {
const logGroup = props.loggingConfig?.logGroup ?? Node.of(scope).tryFindChild('LogGroup') as logs.ILogGroup ?? new logs.LogGroup(scope, 'LogGroup');
const logGroupGrant = logGroup.grantWrite(props.role);
return {
loggingOptions: {
Expand Down Expand Up @@ -152,8 +142,7 @@ export function createBackupConfig(scope: Construct, role: iam.IRole, props?: De
const bucketGrant = bucket.grantReadWrite(role);

const { loggingOptions, dependables: loggingDependables } = createLoggingOptions(scope, {
logging: props.logging,
logGroup: props.logGroup,
loggingConfig: props.loggingConfig,
role,
streamId: 'S3Backup',
}) ?? {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ export class S3Bucket implements firehose.IDestination {
const bucketGrant = this.bucket.grantReadWrite(role);

const { loggingOptions, dependables: loggingDependables } = createLoggingOptions(scope, {
logging: this.props.logging,
logGroup: this.props.logGroup,
loggingConfig: this.props.loggingConfig,
role,
streamId: 'S3Destination',
}) ?? {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ const backupKey = new kms.Key(stack, 'BackupKey', {

new firehose.DeliveryStream(stack, 'Delivery Stream', {
destinations: [new destinations.S3Bucket(bucket, {
logging: true,
logGroup: logGroup,
loggingConfig: new destinations.EnableLogging(logGroup),
processor: processor,
compression: destinations.Compression.GZIP,
dataOutputPrefix: 'regularPrefix',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ describe('S3 destination', () => {

it('bucket and log group grants are depended on by delivery stream', () => {
const logGroup = logs.LogGroup.fromLogGroupName(stack, 'Log Group', 'evergreen');
const destination = new firehosedestinations.S3Bucket(bucket, { role: destinationRole, logGroup });
const destination = new firehosedestinations.S3Bucket(bucket, {
role: destinationRole, loggingConfig: new firehosedestinations.EnableLogging(logGroup),
});
new firehose.DeliveryStream(stack, 'DeliveryStream', {
destinations: [destination],
});
Expand Down Expand Up @@ -171,7 +173,7 @@ describe('S3 destination', () => {

it('does not create resources or configuration if disabled', () => {
new firehose.DeliveryStream(stack, 'DeliveryStream', {
destinations: [new firehosedestinations.S3Bucket(bucket, { logging: false })],
destinations: [new firehosedestinations.S3Bucket(bucket, { loggingConfig: new firehosedestinations.DisableLogging() })],
});

Template.fromStack(stack).resourceCountIs('AWS::Logs::LogGroup', 0);
Expand All @@ -186,7 +188,7 @@ describe('S3 destination', () => {
const logGroup = new logs.LogGroup(stack, 'Log Group');

new firehose.DeliveryStream(stack, 'DeliveryStream', {
destinations: [new firehosedestinations.S3Bucket(bucket, { logGroup })],
destinations: [new firehosedestinations.S3Bucket(bucket, { loggingConfig: new firehosedestinations.EnableLogging(logGroup) })],
});

Template.fromStack(stack).resourceCountIs('AWS::Logs::LogGroup', 1);
Expand All @@ -200,19 +202,13 @@ describe('S3 destination', () => {
});
});

it('throws error if logging disabled but log group provided', () => {
const destination = new firehosedestinations.S3Bucket(bucket, { logging: false, logGroup: new logs.LogGroup(stack, 'Log Group') });

expect(() => new firehose.DeliveryStream(stack, 'DeliveryStream', {
destinations: [destination],
})).toThrowError('logging cannot be set to false when logGroup is provided');
});

it('grants log group write permissions to destination role', () => {
const logGroup = new logs.LogGroup(stack, 'Log Group');

new firehose.DeliveryStream(stack, 'DeliveryStream', {
destinations: [new firehosedestinations.S3Bucket(bucket, { logGroup, role: destinationRole })],
destinations: [new firehosedestinations.S3Bucket(bucket, {
loggingConfig: new firehosedestinations.EnableLogging(logGroup), role: destinationRole,
})],
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
Expand Down Expand Up @@ -572,8 +568,7 @@ describe('S3 destination', () => {
bufferingInterval: cdk.Duration.minutes(1),
compression: firehosedestinations.Compression.ZIP,
encryptionKey: key,
logging: true,
logGroup: logGroup,
loggingConfig: new firehosedestinations.EnableLogging(logGroup),
},
});
new firehose.DeliveryStream(stack, 'DeliveryStream', {
Expand Down

0 comments on commit c4bda64

Please sign in to comment.