Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(custom-resources): cannot set logging for state machine generated in CompleteHandler #27310

Merged
merged 64 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
c7d5edf
fix(custom-resources): StateMachine for isCompleteHandler in provider…
go-to-k Sep 27, 2023
5fddc2b
change codes and integ
go-to-k Sep 27, 2023
f20e343
Revert "change codes and integ"
go-to-k Sep 27, 2023
d798bf7
Merge branch 'main' of https://github.com/go-to-k/aws-cdk into fix/cu…
go-to-k Sep 27, 2023
256b91b
fix: waiter-state-machine
go-to-k Sep 27, 2023
1fe40cc
change integ tests
go-to-k Sep 27, 2023
ce58bdc
change property name and fix unit tests
go-to-k Sep 27, 2023
1b4ec5f
change name to disableWaiterStateMachineLogging
go-to-k Sep 27, 2023
7da8364
change index for export
go-to-k Sep 27, 2023
6c941e9
change import order
go-to-k Sep 27, 2023
a14775f
add docstring
go-to-k Sep 28, 2023
9f7c0f8
change props for logs and integ tests
go-to-k Sep 28, 2023
e8dcfd7
add default for destination
go-to-k Sep 28, 2023
1755f07
change doc
go-to-k Sep 28, 2023
c60d5f4
change other doc
go-to-k Sep 28, 2023
3b6da20
change aws-eks/test/integ.alb-controller
go-to-k Sep 29, 2023
bbe0fe4
aws-dynamodb/test/integ.global.js
go-to-k Sep 29, 2023
b9c5fda
integ for aws-dynamodb/test/integ.global-replicas-provisioned.js
go-to-k Sep 29, 2023
bd9f44b
integ.eks-cluster-private-endpoint.js.snapshot
go-to-k Sep 30, 2023
d674127
integ.eks-bottlerocket-ng.js.snapshot
go-to-k Sep 30, 2023
1b00eb2
rest for integ.eks-cluster-private-endpoint.js.snapshot
go-to-k Sep 30, 2023
9b0e7e4
rest for integ.eks-bottlerocket-ng.js.snapshot
go-to-k Sep 30, 2023
5f96f54
integ.eks-cluster-handlers-vpc-aws-cdk-eks-handlers-in-vpc
go-to-k Sep 30, 2023
beb5d60
add integs for fargate-cluster and eks-windows-ng
go-to-k Sep 30, 2023
1c7e988
integ.call.js.snapshot
go-to-k Sep 30, 2023
83039d8
integ.job-submission-workflow.js.snapshot
go-to-k Sep 30, 2023
09e05dc
test/emrcontainers/integ.start-job-run.js.snapshot
go-to-k Sep 30, 2023
d1a6cc6
fix a test using docker and npm
go-to-k Sep 30, 2023
58b08d3
integ.eks-service-account-sdk-call
go-to-k Sep 30, 2023
1a4a7e5
fix typo for doc in aws-cdk-lib/aws-eks/lib/k8s-manifest.ts
go-to-k Sep 30, 2023
69dde5d
add url in waiter-state-machine
go-to-k Sep 30, 2023
8be78f3
test for integ.eks-cluster.js.snapshot template
go-to-k Oct 1, 2023
a25cb4b
Revert "test for integ.eks-cluster.js.snapshot template"
go-to-k Oct 1, 2023
18f72fc
test/integ.eks-helm-asset
go-to-k Oct 5, 2023
767dd40
test/integ.eks-cluster-imported
go-to-k Oct 5, 2023
5a68fa5
test/integ.eks-cluster
go-to-k Oct 5, 2023
aca913d
test/integ.eks-cluster-ipv6
go-to-k Oct 6, 2023
67114ab
packages/@aws-cdk/aws-amplify-alpha/test/integ.app-asset-deployment.j…
go-to-k Oct 6, 2023
23e23c0
test/integ.eks-inference.js.snapshot
go-to-k Oct 6, 2023
893dff2
Merge branch 'main' into fix/custom-resource-provider-statemachine
go-to-k Oct 6, 2023
48dbb49
Merge branch 'main' into fix/custom-resource-provider-statemachine
go-to-k Oct 18, 2023
6932cb4
change integ.global and integ.global-replicas-provisioned in aws-dyna…
go-to-k Oct 18, 2023
3ebccbd
change without integ tests by review
go-to-k Oct 27, 2023
b80f7c7
change unit tests
go-to-k Oct 27, 2023
494e381
add a new integ test
go-to-k Oct 27, 2023
40be425
change integ tests integ.global and integ.global-replicas-provisioned
go-to-k Oct 27, 2023
b8a495b
update waiter-state-machine.ts for default
go-to-k Oct 27, 2023
d2c9211
update waiter-state-machine.ts
go-to-k Oct 27, 2023
63b60f3
chore: Dockerfile version to alpine3.18
go-to-k Oct 30, 2023
74e2d90
Merge branch 'main' into fix/custom-resource-provider-statemachine
go-to-k Nov 4, 2023
a6311b5
Merge branch 'main' into fix/custom-resource-provider-statemachine
go-to-k Nov 6, 2023
b27e221
Merge branch 'main' of https://github.com/go-to-k/aws-cdk into fix/cu…
go-to-k Nov 12, 2023
e10707b
Merge branch 'main' of https://github.com/go-to-k/aws-cdk into fix/cu…
go-to-k Nov 12, 2023
30136be
Merge branch 'main'
go-to-k Nov 16, 2023
313e3d9
change integ snap shots for aws-amplify-alpha
go-to-k Nov 16, 2023
b0be805
change integ test snapshots for emrcontainers/integ.start-job-run
go-to-k Nov 16, 2023
18d4f33
Merge branch 'main'
go-to-k Nov 22, 2023
e2de141
change integ.provider-with-waiter-state-machine snapshot
go-to-k Nov 22, 2023
676f877
Merge branch 'main' of https://github.com/go-to-k/aws-cdk into fix/cu…
go-to-k Nov 30, 2023
cf76b87
update snapshot for integ.provider-with-waiter-state-machine
go-to-k Nov 30, 2023
dfa7771
Merge branch 'main' into fix/custom-resource-provider-statemachine
kaizencc Dec 19, 2023
7163c86
modify docs by review
go-to-k Dec 20, 2023
ca92b3c
change snapshots
go-to-k Dec 20, 2023
0e7aeb9
Merge branch 'main' into fix/custom-resource-provider-statemachine
mergify[bot] Dec 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as rds from 'aws-cdk-lib/aws-rds';
import { ClusterInstance } from 'aws-cdk-lib/aws-rds';
import { IntegTest } from '@aws-cdk/integ-tests-alpha';
import { STANDARD_NODEJS_RUNTIME } from '../../config';
import { RetentionDays } from 'aws-cdk-lib/aws-logs';

class TestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
Expand Down Expand Up @@ -107,6 +108,10 @@ class Snapshoter extends Construct {
const provider = new cr.Provider(this, 'SnapshotProvider', {
onEventHandler,
isCompleteHandler,
logOptionsForWaiterStateMachine: {
logRetention: RetentionDays.ONE_DAY,
},
disableLoggingForWaiterStateMachine: false,
});

const customResource = new CustomResource(this, 'Snapshot', {
Expand Down
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 { 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 @@ -126,6 +126,25 @@ export interface ProviderProps {
* @default - AWS Lambda creates and uses an AWS managed customer master key (CMK)
*/
readonly providerFunctionEnvEncryption?: kms.IKey;

/**
* Log options for `WaiterStateMachine` with `isCompleteHandler` in the provider.
*
* This must not be used if `isCompleteHandler` is not specified or
* `disableLoggingForWaiterStateMachine` is true.
*
* @default - no log options
*/
readonly logOptionsForWaiterStateMachine?: LogOptions;

/**
* Disable logging for `WaiterStateMachine` with `isCompleteHandler` in the provider.
*
* This must not be used if `isCompleteHandler` is not specified.
*
* @default false
*/
readonly disableLoggingForWaiterStateMachine?: boolean;
}

/**
Expand Down Expand Up @@ -162,9 +181,17 @@ export class Provider extends Construct implements ICustomResourceProvider {
constructor(scope: Construct, id: string, props: ProviderProps) {
super(scope, id);

if (!props.isCompleteHandler && (props.queryInterval || props.totalTimeout)) {
throw new Error('"queryInterval" and "totalTimeout" can only be configured if "isCompleteHandler" is specified. '
+ 'Otherwise, they have no meaning');
if (!props.isCompleteHandler) {
if (
props.queryInterval
|| props.totalTimeout
|| props.logOptionsForWaiterStateMachine
|| props.disableLoggingForWaiterStateMachine !== undefined
) {
throw new Error('"queryInterval" and "totalTimeout" and "logOptionsForWaiterStateMachine" and "disableLoggingForWaiterStateMachine" '
+ 'can only be configured if "isCompleteHandler" is specified. '
+ 'Otherwise, they have no meaning');
}
}

this.onEventHandler = props.onEventHandler;
Expand All @@ -181,6 +208,10 @@ export class Provider extends Construct implements ICustomResourceProvider {
const onEventFunction = this.createFunction(consts.FRAMEWORK_ON_EVENT_HANDLER_NAME, props.providerFunctionName);

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

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

Expand All @@ -191,6 +222,8 @@ export class Provider extends Construct implements ICustomResourceProvider {
backoffRate: retry.backoffRate,
interval: retry.interval,
maxAttempts: retry.maxAttempts,
logOptions: !props.disableLoggingForWaiterStateMachine ? props.logOptionsForWaiterStateMachine : undefined,
disableLogging: props.disableLoggingForWaiterStateMachine,
});
// the on-event entrypoint is going to start the execution of the waiter
onEventFunction.addEnvironment(consts.WAITER_STATE_MACHINE_ARN_ENV, waiterStateMachine.stateMachineArn);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,34 @@
import { Construct } from 'constructs';
import { Grant, IGrantable, Role, ServicePrincipal } from '../../../aws-iam';
import { Grant, IGrantable, PolicyStatement, Role, ServicePrincipal } from '../../../aws-iam';
import { IFunction } from '../../../aws-lambda';
import { CfnResource, Duration, Stack } from '../../../core';
import { LogGroup, RetentionDays } from '../../../aws-logs';
import { LogLevel } from '../../../aws-stepfunctions';

export interface LogOptions {
/**
* Determines whether execution data is included in your log.
*
* @default false
go-to-k marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly includeExecutionData?: boolean;

/**
* Defines which category of execution history events are logged.
*
* @default ERROR
go-to-k marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly level?: LogLevel;

/**
* The number of days framework log events are kept in CloudWatch Logs. When
* updating this property, unsetting it doesn't remove the log retention policy.
* To remove the retention policy, set the value to `INFINITE`.
*
* @default logs.RetentionDays.INFINITE
*/
readonly logRetention?: RetentionDays;
}

export interface WaiterStateMachineProps {
/**
Expand All @@ -28,6 +55,22 @@ export interface WaiterStateMachineProps {
* Backoff between attempts.
*/
readonly backoffRate: number;

/**
* Options for StateMachine logging.
*
* If `disableLogging` is true, this property is ignored.
*
* @default - no logOptions
*/
readonly logOptions?: LogOptions;

/**
* Disable StateMachine logging.
*
* @default false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Disable StateMachine logging.
*
* @default false
* Whether logging for the state machine is disabled.
*
* @default - false

*/
readonly disableLogging?: boolean;
}

/**
Expand All @@ -49,6 +92,32 @@ export class WaiterStateMachine extends Construct {
props.isCompleteHandler.grantInvoke(role);
props.timeoutHandler.grantInvoke(role);

let logGroup: LogGroup | undefined;
if (props.disableLogging) {
if (props.logOptions) {
throw new Error('logOptions must not be used if disableLogging is true');
}
} else {
logGroup = new LogGroup(this, 'LogGroup', {
retention: props.logOptions?.logRetention,
});
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 @@ -75,11 +144,24 @@ 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 CfnResource(this, 'Resource', {
type: 'AWS::StepFunctions::StateMachine',
properties: {
DefinitionString: definition,
RoleArn: role.roleArn,
LoggingConfiguration: this.renderLoggingConfiguration(role, props.logOptions, props.disableLogging),

Declaring a separate method for all of this is cleaner, something like:

  private renderLoggingConfiguration(role: Role, logOptions?: WaiterStateMachineLogOptions, disableLogging?: boolean): CfnStateMachine.LoggingConfigurationProperty | undefined {
    if (!logOptions || 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,
    },
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will change like this.

However I will use if (disableLogging) return undefined; instead of if (!logOptions || disableLogging) return undefined;. Because it needs to create logs and policies by default even if logOptions is not specified, according to this issue.

Copy link
Contributor Author

@go-to-k go-to-k Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think a return value type of the method cannot be CfnStateMachine.LoggingConfigurationProperty.

Because AWS::StepFunctions::StateMachine is created by not L1 Construct for StateMachine but CfnResource.

So I may return by not specifying type for now.

    const resource = new CfnResource(this, 'Resource', {
      type: 'AWS::StepFunctions::StateMachine',
      properties: {
        DefinitionString: definition,
        RoleArn: role.roleArn,

Or how about using CfnStateMachine instead of CfnResource as follows?

    const resource = new CfnStateMachine(this, 'Resource', {
      definitionString: definition,
      roleArn: role.roleArn,
      loggingConfiguration: this.renderLoggingConfiguration(role, props.logOptions, props.disableLogging),
    });

Please see the commit.

3ebccbd#diff-ba99cb0b24f721e7bd6405b5f84ba44cd64f95932fa1f0974d7676855680a25bR126-R129

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the current implementation should be fine.
Thanks for the clarification and the adjustment on the if condition.

},
});
resource.node.addDependency(role);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as iam from '../../../aws-iam';
import * as kms from '../../../aws-kms';
import * as lambda from '../../../aws-lambda';
import * as logs from '../../../aws-logs';
import { LogLevel } from '../../../aws-stepfunctions';
import { Duration, Stack } from '../../../core';
import * as cr from '../../lib';
import * as util from '../../lib/provider-framework/util';
Expand Down Expand Up @@ -180,6 +181,12 @@ test('if isComplete is specified, the isComplete framework handler is also inclu
new cr.Provider(stack, 'MyProvider', {
onEventHandler: handler,
isCompleteHandler: handler,
logOptionsForWaiterStateMachine: {
includeExecutionData: true,
level: LogLevel.ALL,
logRetention: logs.RetentionDays.ONE_DAY,
},
disableLoggingForWaiterStateMachine: false,
});

// THEN
Expand Down Expand Up @@ -238,10 +245,51 @@ test('if isComplete is specified, the isComplete framework handler is also inclu
],
],
},
LoggingConfiguration: {
Destinations: [
{
CloudWatchLogsLogGroup: {
LogGroupArn: {
'Fn::GetAtt': [
'MyProviderwaiterstatemachineLogGroupD43CA868',
'Arn',
],
},
},
},
],
},
IncludeExecutionData: true,
Level: 'ALL',
});
});

test('fails if "queryInterval" and/or "totalTimeout" are set without "isCompleteHandler"', () => {
test('fails if logOptionsForWaiterStateMachine is specified and disableLoggingForWaiterStateMachine 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
// THEN
expect(() => {
new cr.Provider(stack, 'MyProvider', {
onEventHandler: handler,
isCompleteHandler: handler,
logOptionsForWaiterStateMachine: {
includeExecutionData: true,
level: LogLevel.ALL,
logRetention: logs.RetentionDays.ONE_DAY,
},
disableLoggingForWaiterStateMachine: true,
});
}).toThrow(/logOptionsForWaiterStateMachine must not be used if disableLoggingForWaiterStateMachine is true/);
});

test('fails if "queryInterval" or "totalTimeout" or "logOptionsForWaiterStateMachine" or "disableLoggingForWaiterStateMachine" are set without "isCompleteHandler"', () => {
// GIVEN
const stack = new Stack();
const handler = new lambda.Function(stack, 'MyHandler', {
Expand All @@ -254,12 +302,26 @@ test('fails if "queryInterval" and/or "totalTimeout" are set without "isComplete
expect(() => new cr.Provider(stack, 'provider1', {
onEventHandler: handler,
queryInterval: Duration.seconds(10),
})).toThrow(/\"queryInterval\" and \"totalTimeout\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
})).toThrow(/\"queryInterval\" and \"totalTimeout\" and \"logOptionsForWaiterStateMachine\" and \"disableLoggingForWaiterStateMachine\" 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\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);
})).toThrow(/\"queryInterval\" and \"totalTimeout\" and \"logOptionsForWaiterStateMachine\" and \"disableLoggingForWaiterStateMachine\" can only be configured if \"isCompleteHandler\" is specified. Otherwise, they have no meaning/);

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

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

describe('retry policy', () => {
Expand Down
Loading
Loading