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

custom resources: Generated step function generates finding due to missing logging #27283

Closed
rv2673 opened this issue Sep 25, 2023 · 3 comments · Fixed by #27310, #28706, rwlxxvii/containers#124 or rwlxxvii/containers#140 · May be fixed by NOUIY/aws-solutions-constructs#98
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@rv2673
Copy link
Contributor

rv2673 commented Sep 25, 2023

Describe the bug

When custom resources with provider framework has a completedHandler function it generates a aws step function for polling for completion with a configured interval and total timeout.

This generated step function however has no logging configuration. Which causes the StepFunctions.1 rule from the AWS Foundational Security Best Practices (FSBP) standard set to be triggered.

This is due to the missing logging configuration. The provider construct however also does not provide any option to configure this logging.

Expected Behavior

The provider construct adds logging by default following the foundational best practice and/or allows this to be configured.

Current Behavior

The provider construct does not add logging configuration on step function nor does it allow for it to be configured through construct props.

Reproduction Steps

Enable StepFunctions.1 rule.
Create stack with custom resource with both evenHandler and isCompleteHandler to trigger generation of state machine.
Deploy generated template
This template should triggger the rule due to missing logging configuration

Possible Solution

  • Add logging configuration by default (with logRetention from props if provided)
  • And/or allow for loggroup (configuration) to be provided to provider construct for use in state machine.

Additional Information/Context

No response

CDK CLI Version

2.96.0

Framework Version

No response

Node.js Version

18

OS

Ubuntu 20.04

Language

Typescript

Language Version

No response

Other information

No response

@rv2673 rv2673 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2023
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Sep 25, 2023
@pahud pahud self-assigned this Sep 25, 2023
@pahud
Copy link
Contributor

pahud commented Sep 25, 2023

Yes we should add logging here by default unless disabled.

properties: {
DefinitionString: definition,
RoleArn: role.roleArn,
},

Before it is supported, you can work it around with addPropertyOverride() as below to configure the loggingConfigurations.

const provider = new cr.Provider(this, 'Provider', {
	onEventHandler,
	isCompleteHandler,
	
});

const logGroup = new LogGroup(this, 'LogGroup');

const waiterStateMachine = provider.node.tryFindChild('waiter-state-machine') as Construct
const cfnstateMachine = waiterStateMachine.node.tryFindChild('Resource') as CfnResource
cfnstateMachine.addPropertyOverride('LoggingConfiguration', {
	'CloudWatchLogsLogGroup': {
		'LogGroupArn': logGroup.logGroupArn,
	}
})

@pahud pahud removed their assignment Sep 25, 2023
@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2023
@mergify mergify bot closed this as completed in #27310 Dec 20, 2023
mergify bot pushed a commit that referenced this issue Dec 20, 2023
… in CompleteHandler (#27310)

This PR fixes the case that `StateMachine` generated for `CompleteHandler` in `Provider` cannot set logging.

Closes #27283.

----

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

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this issue Jan 5, 2024
… in CompleteHandler (aws#27310)

This PR fixes the case that `StateMachine` generated for `CompleteHandler` in `Provider` cannot set logging.

Closes aws#27283.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot closed this as completed in #28706 Apr 19, 2024
mergify bot pushed a commit that referenced this issue Apr 19, 2024
… in CompleteHandler (#28706)

This PR fixes the case that `StateMachine` generated for `CompleteHandler` in `Provider` cannot set logging.

The default log group name of the state machine automatically created by the `Provider` with `isCompleteHandler` should start with `/aws/vendedlogs/states`. Otherwise, each time the state machine is created, the log resource policy size increases. 

https://docs.aws.amazon.com/step-functions/latest/dg/bp-cwl.html

Closes #27283
Fixes #28577 #28744

Related PR #27310(reverted by #28699), #28587

----

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

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment