Skip to content

Commit

Permalink
change without integ tests by review
Browse files Browse the repository at this point in the history
  • Loading branch information
@go-to-k
go-to-k committed Oct 27, 2023
1 parent 6932cb4 commit 3ebccbd
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 111 deletions.
25 changes: 8 additions & 17 deletions packages/aws-cdk-lib/custom-resources/lib/provider-framework/provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as path from 'path';
import { Construct } from 'constructs';
import * as consts from './runtime/consts';
import { calculateRetryPolicy } from './util';
import { WaiterStateMachineLogOptions, WaiterStateMachine } from './waiter-state-machine';
import { LogOptions, WaiterStateMachine } from './waiter-state-machine';
import { CustomResourceProviderConfig, ICustomResourceProvider } from '../../../aws-cloudformation';
import * as ec2 from '../../../aws-ec2';
import * as iam from '../../../aws-iam';
Expand Down Expand Up @@ -128,21 +128,16 @@ export interface ProviderProps {
readonly providerFunctionEnvEncryption?: kms.IKey;

/**
* Log options for `WaiterStateMachine` with `isCompleteHandler` in the provider.
* Defines what execution history events of the waiter state machine are logged and where they are logged.
*
* This property must not be used if `isCompleteHandler` is not specified or
* `disableWaiterStateMachineLogging` is true.
*
* @default - no log options
* @default - A default log group will be created if logging for the waiter state machine is enabled.
*/
readonly waiterStateMachineLogOptions?: WaiterStateMachineLogOptions;
readonly waiterStateMachineLogOptions?: LogOptions;

/**
* Disable logging for `WaiterStateMachine` with `isCompleteHandler` in the provider.
*
* This property must not be used if `isCompleteHandler` is not specified.
* Whether logging for the waiter state machine is disabled.
*
* @default false
* @default - false
*/
readonly disableWaiterStateMachineLogging?: boolean;
}
Expand Down Expand Up @@ -188,7 +183,7 @@ export class Provider extends Construct implements ICustomResourceProvider {
|| props.waiterStateMachineLogOptions
|| props.disableWaiterStateMachineLogging !== undefined
) {
throw new Error('"queryInterval" and "totalTimeout" and "waiterStateMachineLogOptions" and "disableWaiterStateMachineLogging" '
throw new Error('"queryInterval", "totalTimeout", "waiterStateMachineLogOptions", and "disableWaiterStateMachineLogging" '
+ 'can only be configured if "isCompleteHandler" is specified. '
+ 'Otherwise, they have no meaning');
}
Expand All @@ -208,10 +203,6 @@ export class Provider extends Construct implements ICustomResourceProvider {
const onEventFunction = this.createFunction(consts.FRAMEWORK_ON_EVENT_HANDLER_NAME, props.providerFunctionName);

if (this.isCompleteHandler) {
if (props.disableWaiterStateMachineLogging && props.waiterStateMachineLogOptions) {
throw new Error('waiterStateMachineLogOptions must not be used if disableWaiterStateMachineLogging is true');
}

const isCompleteFunction = this.createFunction(consts.FRAMEWORK_IS_COMPLETE_HANDLER_NAME);
const timeoutFunction = this.createFunction(consts.FRAMEWORK_ON_TIMEOUT_HANDLER_NAME);

Expand All @@ -222,7 +213,7 @@ export class Provider extends Construct implements ICustomResourceProvider {
backoffRate: retry.backoffRate,
interval: retry.interval,
maxAttempts: retry.maxAttempts,
logOptions: !props.disableWaiterStateMachineLogging ? props.waiterStateMachineLogOptions : undefined,
logOptions: props.waiterStateMachineLogOptions,
disableLogging: props.disableWaiterStateMachineLogging,
});
// the on-event entrypoint is going to start the execution of the waiter
Expand Down
108 changes: 50 additions & 58 deletions packages/aws-cdk-lib/custom-resources/lib/provider-framework/waiter-state-machine.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@ import { Construct } from 'constructs';
import { Grant, IGrantable, PolicyStatement, Role, ServicePrincipal } from '../../../aws-iam';
import { IFunction } from '../../../aws-lambda';
import { ILogGroup, LogGroup } from '../../../aws-logs';
import { LogLevel } from '../../../aws-stepfunctions';
import { CfnResource, Duration, Stack } from '../../../core';
import { CfnStateMachine, LogLevel } from '../../../aws-stepfunctions';
import { Duration, Stack } from '../../../core';

/**
* Log Options for the state machine.
*/
export interface WaiterStateMachineLogOptions {
export interface LogOptions {
/**
* The log group where the execution history events will be logged.
*
* @default - Log group generated automatically
* @default - a new log group will be created
*/
readonly destination?: ILogGroup;

Expand Down Expand Up @@ -61,18 +61,16 @@ export interface WaiterStateMachineProps {
readonly backoffRate: number;

/**
* Options for StateMachine logging.
* Defines what execution history events are logged and where they are logged.
*
* This property must not be used if `disableLogging` is true.
*
* @default - no logOptions
* @default - A default log group will be created if logging is enabled.
*/
readonly logOptions?: WaiterStateMachineLogOptions;
readonly logOptions?: LogOptions;

/**
* Disable StateMachine logging.
* Whether logging for the state machine is disabled.
*
* @default false
* @default - false
*/
readonly disableLogging?: boolean;
}
Expand All @@ -99,34 +97,6 @@ export class WaiterStateMachine extends Construct {
props.isCompleteHandler.grantInvoke(role);
props.timeoutHandler.grantInvoke(role);

let logGroup: ILogGroup | undefined;
if (props.disableLogging) {
if (props.logOptions) {
throw new Error('logOptions must not be used if disableLogging is true');
}
} else {
logGroup = props.logOptions?.destination
? props.logOptions.destination
: new LogGroup(this, 'LogGroup');

// https://docs.aws.amazon.com/step-functions/latest/dg/cw-logs.html#cloudwatch-iam-policy
role.addToPrincipalPolicy(new PolicyStatement({
actions: [
'logs:CreateLogDelivery',
'logs:CreateLogStream',
'logs:GetLogDelivery',
'logs:UpdateLogDelivery',
'logs:DeleteLogDelivery',
'logs:ListLogDeliveries',
'logs:PutLogEvents',
'logs:PutResourcePolicy',
'logs:DescribeResourcePolicies',
'logs:DescribeLogGroups',
],
resources: ['*'],
}));
}

const definition = Stack.of(this).toJsonString({
StartAt: 'framework-isComplete-task',
States: {
Expand All @@ -153,25 +123,10 @@ export class WaiterStateMachine extends Construct {
},
});

const logOptions = logGroup ? {
LoggingConfiguration: {
Destinations: [{
CloudWatchLogsLogGroup: {
LogGroupArn: logGroup.logGroupArn,
},
}],
IncludeExecutionData: props.logOptions?.includeExecutionData ?? false,
Level: props.logOptions?.level ?? LogLevel.ERROR,
},
} : undefined;

const resource = new CfnResource(this, 'Resource', {
type: 'AWS::StepFunctions::StateMachine',
properties: {
DefinitionString: definition,
RoleArn: role.roleArn,
...logOptions,
},
const resource = new CfnStateMachine(this, 'Resource', {
definitionString: definition,
roleArn: role.roleArn,
loggingConfiguration: this.renderLoggingConfiguration(role, props.logOptions, props.disableLogging),
});
resource.node.addDependency(role);

Expand All @@ -188,4 +143,41 @@ export class WaiterStateMachine extends Construct {
resourceArns: [this.stateMachineArn],
});
}

private renderLoggingConfiguration(
role: Role,
logOptions?: LogOptions,
disableLogging?: boolean,
): CfnStateMachine.LoggingConfigurationProperty | undefined {
if (disableLogging) return undefined;

// https://docs.aws.amazon.com/step-functions/latest/dg/cw-logs.html#cloudwatch-iam-policy
role.addToPrincipalPolicy(new PolicyStatement({
actions: [
'logs:CreateLogDelivery',
'logs:CreateLogStream',
'logs:GetLogDelivery',
'logs:UpdateLogDelivery',
'logs:DeleteLogDelivery',
'logs:ListLogDeliveries',
'logs:PutLogEvents',
'logs:PutResourcePolicy',
'logs:DescribeResourcePolicies',
'logs:DescribeLogGroups',
],
resources: ['*'],
}));

const logGroup = logOptions?.destination ?? new LogGroup(this, 'LogGroup');

return {
destinations: [{
cloudWatchLogsLogGroup: {
logGroupArn: logGroup.logGroupArn,
},
}],
includeExecutionData: logOptions?.includeExecutionData ?? false,
level: logOptions?.level ?? LogLevel.ERROR,
};
}
}
69 changes: 51 additions & 18 deletions packages/aws-cdk-lib/custom-resources/test/provider-framework/provider.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Template } from '../../../assertions';
import { Match, Template } from '../../../assertions';
import * as ec2 from '../../../aws-ec2';
import * as iam from '../../../aws-iam';
import * as kms from '../../../aws-kms';
Expand Down Expand Up @@ -185,7 +185,6 @@ test('if isComplete is specified, the isComplete framework handler is also inclu
includeExecutionData: true,
level: LogLevel.ALL,
},
disableWaiterStateMachineLogging: false,
});

// THEN
Expand Down Expand Up @@ -263,7 +262,7 @@ test('if isComplete is specified, the isComplete framework handler is also inclu
});
});

test('fails if waiterStateMachineLogOptions is specified and disableWaiterStateMachineLogging is true', () => {
test('a default LoggingConfiguration will be created even if waiterStateMachineLogOptions is not specified', () => {
// GIVEN
const stack = new Stack();
const handler = new lambda.Function(stack, 'MyHandler', {
Expand All @@ -273,18 +272,52 @@ test('fails if waiterStateMachineLogOptions is specified and disableWaiterStateM
});

// WHEN
new cr.Provider(stack, 'MyProvider', {
onEventHandler: handler,
isCompleteHandler: handler,
});

// THEN
expect(() => {
new cr.Provider(stack, 'MyProvider', {
onEventHandler: handler,
isCompleteHandler: handler,
waiterStateMachineLogOptions: {
includeExecutionData: true,
level: LogLevel.ALL,
},
disableWaiterStateMachineLogging: true,
});
}).toThrow(/waiterStateMachineLogOptions must not be used if disableWaiterStateMachineLogging is true/);
Template.fromStack(stack).hasResourceProperties('AWS::StepFunctions::StateMachine', {
LoggingConfiguration: {
Destinations: [
{
CloudWatchLogsLogGroup: {
LogGroupArn: {
'Fn::GetAtt': [
'MyProviderwaiterstatemachineLogGroupD43CA868',
'Arn',
],
},
},
},
],
IncludeExecutionData: false,
Level: 'ERROR',
},
});
});

test('a default LoggingConfiguration will not be created if disableWaiterStateMachineLogging is true', () => {
// GIVEN
const stack = new Stack();
const handler = new lambda.Function(stack, 'MyHandler', {
code: new lambda.InlineCode('foo'),
handler: 'index.onEvent',
runtime: lambda.Runtime.NODEJS_LATEST,
});

// WHEN
new cr.Provider(stack, 'MyProvider', {
onEventHandler: handler,
isCompleteHandler: handler,
disableWaiterStateMachineLogging: true,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::StepFunctions::StateMachine', {
LoggingConfiguration: Match.absent(),
});
});

test('fails if "queryInterval" or "totalTimeout" or "waiterStateMachineLogOptions" or "disableWaiterStateMachineLogging" are set without "isCompleteHandler"', () => {
Expand All @@ -300,25 +333,25 @@ test('fails if "queryInterval" or "totalTimeout" or "waiterStateMachineLogOption
expect(() => new cr.Provider(stack, 'provider1', {
onEventHandler: handler,
queryInterval: Duration.seconds(10),
})).toThrow(/\"queryInterval\" and \"totalTimeout\" and \"waiterStateMachineLogOptions\" and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
})).toThrow(/\"queryInterval\", \"totalTimeout\", \"waiterStateMachineLogOptions\", and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);

expect(() => new cr.Provider(stack, 'provider2', {
onEventHandler: handler,
totalTimeout: Duration.seconds(100),
})).toThrow(/\"queryInterval\" and \"totalTimeout\" and \"waiterStateMachineLogOptions\" and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
})).toThrow(/\"queryInterval\", \"totalTimeout\", \"waiterStateMachineLogOptions\", and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);

expect(() => new cr.Provider(stack, 'provider3', {
onEventHandler: handler,
waiterStateMachineLogOptions: {
includeExecutionData: true,
level: LogLevel.ALL,
},
})).toThrow(/\"queryInterval\" and \"totalTimeout\" and \"waiterStateMachineLogOptions\" and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
})).toThrow(/\"queryInterval\", \"totalTimeout\", \"waiterStateMachineLogOptions\", and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);

expect(() => new cr.Provider(stack, 'provider4', {
onEventHandler: handler,
disableWaiterStateMachineLogging: false,
})).toThrow(/\"queryInterval\" and \"totalTimeout\" and \"waiterStateMachineLogOptions\" and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
})).toThrow(/\"queryInterval\", \"totalTimeout\", \"waiterStateMachineLogOptions\", and \"disableWaiterStateMachineLogging\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
});

describe('retry policy', () => {
Expand Down
18 changes: 0 additions & 18 deletions packages/aws-cdk-lib/custom-resources/test/provider-framework/waiter-state-machine.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,24 +175,6 @@ describe('state machine', () => {
// THEN
const roleId = 'statemachineRole52044F93';
Template.fromStack(stack).resourceCountIs('AWS::Logs::LogGroup', 0);
Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: 'lambda:InvokeFunction',
Effect: 'Allow',
Resource: stack.resolve(isCompleteHandler.resourceArnsForGrantInvoke),
},
{
Action: 'lambda:InvokeFunction',
Effect: 'Allow',
Resource: stack.resolve(timeoutHandler.resourceArnsForGrantInvoke),
},
],
Version: '2012-10-17',
},
Roles: [{ Ref: roleId }],
});
});

test('fails if logOptions is specified and disableLogging is true', () => {
Expand Down

0 comments on commit 3ebccbd

Please sign in to comment.