-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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(stepfunctions): disabling logging still requires LogGroup #30816
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
if (logOptions.level !== LogLevel.OFF && !logOptions.destination) { | ||
throw new Error('Logs destination is required when level is not OFF.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall this validation be done earlier (at line 427) with other validations instead?
if (!logs && !logs.level != LogLevel.OFF && !logs.destination) { ...
reasons for ask:
- with current change, validations will be split across various places and as this file is big (and keep getting bigger), hence, it will become difficult to have complete picture of validation
- ideally, caller shall not even call
buildLoggingConfiguration
if prerequisites are not met
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a separate validation method validateLogOptions()
if (logOptions.destination) { | ||
// https://docs.aws.amazon.com/step-functions/latest/dg/cw-logs.html#cloudwatch-iam-policy | ||
this.addToRolePolicy(new iam.PolicyStatement({ | ||
effect: iam.Effect.ALLOW, | ||
actions: [ | ||
'logs:CreateLogDelivery', | ||
'logs:GetLogDelivery', | ||
'logs:UpdateLogDelivery', | ||
'logs:DeleteLogDelivery', | ||
'logs:ListLogDeliveries', | ||
'logs:PutResourcePolicy', | ||
'logs:DescribeResourcePolicies', | ||
'logs:DescribeLogGroups', | ||
], | ||
resources: ['*'], | ||
})); | ||
} | ||
|
||
return { | ||
destinations: [{ | ||
destinations: logOptions.destination ? [{ | ||
cloudWatchLogsLogGroup: { logGroupArn: logOptions.destination.logGroupArn }, | ||
}], | ||
}] : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we do this like as follows - will it reduce need to check logOptions.destination
twice and simplify a bit?
let destinations = undefined;
if (logOptions.destination) {
// Policy addition
destinations = [{
cloudWatchLogsLogGroup: { logGroupArn: logOptions.destination.logGroupArn },
}];
}
return {
destinations,
...,
level: logOptions.level || LogLevel.ERROR
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
*/ | ||
readonly destination: logs.ILogGroup; | ||
readonly destination?: logs.ILogGroup; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this not be a breaking change for existing usage? example:
const logOptions: LogOptions = {
destination: <LogGroup>
}
const stateMachine: StateMachine = new StateMachine(parent, 'ID', {
logs: logOptions,
...
}
...
stateMachine.logs.destination.logGroupName // Error
stateMachine.logs.destination!.logGroupName // Change needed
Some user(s) might be utilising ILogGroup
's public properties from logOptions
which will now cause issues because now such users will need to make non-null assertions before using such properties (e.g. with !
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the jsii-diff documentation:
You are allowed to make inputs optional
So i think this change is not a breaking change.
|
||
expect(() => { | ||
new sfn.StateMachine(stack, 'MyStateMachine', { | ||
definitionBody: sfn.DefinitionBody.fromChainable(sfn.Chain.start(new sfn.Pass(stack, 'Pass'))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not directly pass new sfn.Pass(stack, 'Pass')
to DefintionBody.fromChainable
as Pass implements IChainable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just a copy & paste.
README also uses sfn.Chain.start()
instead of bare sfn.Pass
.
LogOptions.destination
optional
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #30814.
Reason for this change
To disable logging on a StateMachine (with logging enabled), we should specify
LogLevel.OFF
toLogOptions.level
. But cannot remove the LogGroup becauseLogOptions.destination
is required.Description of changes
LogOptions.destination
optional.LogOptions.destination
is present whenLogOptions.level
is notOFF
.Description of how you validated changes
Unit and integ tests that verify
LogOptions.destination
is opitional whenLogOptions.level
isOFF
and throw an exception otherwise.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license