feat(custom-resources): add logging property to AwsSdkCall and create Logging class#29648
feat(custom-resources): add logging property to AwsSdkCall and create Logging class#29648mergify[bot] merged 94 commits intomainfrom
AwsSdkCall and create Logging class#29648Conversation
AwsCustomResourceProps to prevent logging API call response dataAwsCustomResourceProps to prevent logging API call response data
vinayak-kukreja
left a comment
There was a problem hiding this comment.
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.
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
| ### 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.
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: |
| path in `PhysicalResourceId.fromResponse()`. | ||
|
|
||
| ### Custom Resource Examples | ||
| ### Custom Resource Logging |
There was a problem hiding this comment.
Lets add an example Response Object in these docs
There was a problem hiding this comment.
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
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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
AwsCustomResourcelog 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 newloggingproperty on theAwsSdkCallinterface that can be used to provide more control over logging in the SDK v2 and v3 handlers on a per SDK call basis. Theloggingflag is configurable via a newLoggingclass which exposes two static methods:Datafield on the response objectAdditional logging configurations can be added in the future.
Description of changes
Added a
loggingflag to theAwsSdkCallinterface which is configurable via the newLoggingclass. TheLoggingclass has an internalrendermethod which renders the specified logging configuration which is passed as part of thecreate,update, anddeleteResourcePropertiesto the lambda handler. Theseloggingproperties are then used throughout the handler to control what is logged based on their valueDescription of how you validated changes
loggingaswithDataHiddenwas addedrenderon aLogginginstance produces the expected resultloggingwithAwsSdkCallwhile usingAwsCustomResourceproduces 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