-
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
feat(custom-resources): add logging property to AwsSdkCall
and create Logging
class
#29648
Conversation
AwsCustomResourceProps
to prevent logging API call response dataAwsCustomResourceProps
to prevent logging API call response data
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.
I agree with the changes. But, we should get consensus among the team too about this. Especially stakeholders you have already synced with about this.
/** | ||
* Whether or not to include API response data in the logs for the underlying lambda. | ||
* | ||
* If this value is false, the raw API response and the `Data` field in the response |
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.
I think we should add an example of Response Object
here so that we explicitly mention what we will still be logging with this. And also add this to our documentation
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.
### Custom Resource Examples | ||
### Custom Resource Logging | ||
|
||
The underlying lamdda function used by the `AwsCustomResource` construct logs the following information: |
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.
NIT
The underlying lamdda function used by the `AwsCustomResource` construct logs the following information: | |
The underlying lambda function used by the `AwsCustomResource` construct logs the following information: |
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.
@@ -607,9 +607,16 @@ new cr.AwsCustomResource(this, 'ListObjects', { | |||
Note that even if you restrict the output of your custom resource you can still use any | |||
path in `PhysicalResourceId.fromResponse()`. | |||
|
|||
### Custom Resource Examples | |||
### Custom Resource Logging |
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.
Lets add an example Response Object in these docs
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.
Good call out. This has been updated.
Did you close the original PR altogether? I was looking for my comments on it but couldn't find them. This still has the issue of this being a boolean without an option for additional customization. |
@TheRealAmazonKendra Can you provide more detail on why you think that? We can provide this flag and still keep the door open for additional customization. This flag would only control the logging of the Say, for example, we wanted to add something to control log levels. If The previous PR is linked here. The flag proposed in that PR was too overbearing and would have blocked us from implementing additional customization in the future (as you correctly stated). I don't think this has that problem. |
33b18df
to
33c7ac8
Compare
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.
@TheRealAmazonKendra pushed initial changes we discussed offline. To summarize we should provide a |
Signed-off-by: Francis <colifran@amazon.com>
AwsCustomResourceProps
to prevent logging API call response dataAwsCustomResourceProps
and create Logging
class
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Signed-off-by: Francis <colifran@amazon.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…sSdkCall` in unit tests (#29860) ### Reason for this change #29648 introduced a change to the `AwsSdkCall` representation used in the v2 and v3 handler code. Our handler unit tests use `satisfies` to validate that the event object satisfies `AwsSdkCall`. All unit tests and the build still pass, but the linter calls out that the event object doesn't actually satisfy `AwsSdkCall`. #29845 removed the dependency `@aws-cdk/custom-resource-handlers` had on `aws-sdk`. We should add this as devDependency since we're using `aws-sdk` in v2 handler mocks. ### Description of changes I added `logApiResponseData` property to the event objects being tested to make the event satisfy `AwsSdkCall`. I added `aws-sdk` as a dev dependency. We will remove this as part of the v2 handler removal. ### 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*
…ce event properties by default (#30418) Closes #30121, #29949 ### Reason for this change PR #29648 introduced a new resource property `logApiResponseData`. This resource property is `true` by default which forces an update for `AwsCustomResource`. For users without `onUpdate` configured an empty data object is returned if no SDK call is configured. This can cause an attribute error if the user is depending on a data from a specific SDK call. ### Description of changes Made `logApiResponseData` undefined by default which will not trigger `onUpdate`. To maintain backwards compatibility with the original PR introducing `logApiResponseData` as true by default, I've also introduced a feature flag that will allow users to keep the current behavior so they aren't now forced into another `onUpdate` event. ### Description of how you validated changes Updated unit tests where `logApiResponseData` was added as a resource property. Added new unit test to verify that `logApiResponseData` could be added to the event. Updated unit tests that test `_render()` to ensure that the default case will result in an empty object. Updated integ tests. ### 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*
…ce event properties by default (aws#30418) Closes aws#30121, aws#29949 ### Reason for this change PR aws#29648 introduced a new resource property `logApiResponseData`. This resource property is `true` by default which forces an update for `AwsCustomResource`. For users without `onUpdate` configured an empty data object is returned if no SDK call is configured. This can cause an attribute error if the user is depending on a data from a specific SDK call. ### Description of changes Made `logApiResponseData` undefined by default which will not trigger `onUpdate`. To maintain backwards compatibility with the original PR introducing `logApiResponseData` as true by default, I've also introduced a feature flag that will allow users to keep the current behavior so they aren't now forced into another `onUpdate` event. ### Description of how you validated changes Updated unit tests where `logApiResponseData` was added as a resource property. Added new unit test to verify that `logApiResponseData` could be added to the event. Updated unit tests that test `_render()` to ensure that the default case will result in an empty object. Updated integ tests. ### 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*
…ce event properties by default (aws#30418) Closes aws#30121, aws#29949 ### Reason for this change PR aws#29648 introduced a new resource property `logApiResponseData`. This resource property is `true` by default which forces an update for `AwsCustomResource`. For users without `onUpdate` configured an empty data object is returned if no SDK call is configured. This can cause an attribute error if the user is depending on a data from a specific SDK call. ### Description of changes Made `logApiResponseData` undefined by default which will not trigger `onUpdate`. To maintain backwards compatibility with the original PR introducing `logApiResponseData` as true by default, I've also introduced a feature flag that will allow users to keep the current behavior so they aren't now forced into another `onUpdate` event. ### Description of how you validated changes Updated unit tests where `logApiResponseData` was added as a resource property. Added new unit test to verify that `logApiResponseData` could be added to the event. Updated unit tests that test `_render()` to ensure that the default case will result in an empty object. Updated integ tests. ### 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*
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Reason for this change
SDK v2 and v3 handlers for
AwsCustomResource
log the event object passed to the handler, API responses, and caught /uncaught errors for each SDK call made. This can potentially result in logging sensitive information that a user may wish to hide. This PR introduces a newlogging
property on theAwsSdkCall
interface that can be used to provide more control over logging in the SDK v2 and v3 handlers on a per SDK call basis. Thelogging
flag is configurable via a newLogging
class which exposes two static methods:Data
field on the response objectAdditional logging configurations can be added in the future.
Description of changes
Added a
logging
flag to theAwsSdkCall
interface which is configurable via the newLogging
class. TheLogging
class has an internalrender
method which renders the specified logging configuration which is passed as part of thecreate
,update
, anddelete
ResourceProperties
to the lambda handler. Theselogging
properties are then used throughout the handler to control what is logged based on their valueDescription of how you validated changes
logging
aswithDataHidden
was addedrender
on aLogging
instance produces the expected resultlogging
withAwsSdkCall
while usingAwsCustomResource
produces the correct CloudFormation templateChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license