feat(custom-resources): add logging property to `AwsSdkCall` and create `Logging` class #29648

colifran · 2024-03-29T08:30:41Z

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 new logging property on the AwsSdkCall interface that can be used to provide more control over logging in the SDK v2 and v3 handlers on a per SDK call basis. The logging flag is configurable via a new Logging class which exposes two static methods:

all: all logging during lambda execution is turned on
withDataHidden: hides all logged data associated with the API call response. This includes the raw response as well as the Data field on the response object

Additional logging configurations can be added in the future.

Description of changes

Added a logging flag to the AwsSdkCall interface which is configurable via the new Logging class. The Logging class has an internal render method which renders the specified logging configuration which is passed as part of the create, update, and delete ResourceProperties to the lambda handler. These logging properties are then used throughout the handler to control what is logged based on their value

Description of how you validated changes

A new integ test with logging as withDataHidden was added
Unit tests to ensure calling render on a Logging instance produces the expected result
Unit tests to ensure that using logging with AwsSdkCall while using AwsCustomResource produces the correct CloudFormation template

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

vinayak-kukreja

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.

vinayak-kukreja · 2024-04-01T23:07:59Z

packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

+  /**
+   * 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


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

vinayak-kukreja · 2024-04-01T23:08:37Z

packages/aws-cdk-lib/custom-resources/README.md

-### Custom Resource Examples
+### Custom Resource Logging
+
+The underlying lamdda function used by the `AwsCustomResource` construct logs the following information:


NIT

Suggested change

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:

vinayak-kukreja · 2024-04-01T23:09:20Z

packages/aws-cdk-lib/custom-resources/README.md

@@ -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


Lets add an example Response Object in these docs

Good call out. This has been updated.

TheRealAmazonKendra · 2024-04-03T01:43:18Z

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.

colifran · 2024-04-03T04:42:16Z

@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 Data field in the response object. Any additional customization could come with a future PR and this flag could be set or unset without blocking a future property that focuses on customization.

Say, for example, we wanted to add something to control log levels. If logApiResponseData is set to false and the log level is DEBUG then CloudWatch Logs still has all errors (caught and uncaught), the event received by the handler, and the response object (just without the Data field). That would open the door for a user to have all the benefits of a DEBUG log level while making an API call that returns sensitive data. Except now they don't have to worry about exposing data in CloudWatch Logs that they want hidden. If that isn't an issue for them then they can ignore logApiResponseData or explicitly set it to true.

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.

Signed-off-by: Francis <colifran@amazon.com>

aws-cdk-automation

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.

colifran · 2024-04-04T02:38:24Z

@TheRealAmazonKendra pushed initial changes we discussed offline. To summarize we should provide a Logging class that consolidates all of logging levels into an enum-like class. Additionally, this doesn't need to follow standard logging convention (info, trace, debug, etc.). We can provide something like on, off, and selective where selective would give granular control over logged data to the user.

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>

scanlonp

One comment as a point of discussion. But overall good from me.

scanlonp · 2024-04-12T23:24:31Z

...custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/construct-types.ts

Question on if this property should be optional or not. By the way we have set up the logic with _render(), we will always have a value for this field, so it can be "required".

@colifran and I talked offline, I do not think that this is a problem. The main discussion is at what level we decide to set the default, and having it passed explicitly to the call is fine.

For historical purposes, I want to make sure it is clear that the AwsSdkCall interface referenced above is not a type used in the aws-cdk-lib. Instead, this is a mirror of AwsSdkCall which is defined as part of the AwsCustomResource construct. However, the format of this represents the AwsSdkCall type as it would be received by the v2 and v3 handler, e.g., after render has been called on a Logging instance and all Logging properties are rendered. This interface just exists to provide a strongly typed call in the v2 and v3 handlers.

My opinion is that we should set the default prior to passing the event so that the event properties are visible in the synthesized CloudFormation template.

Signed-off-by: Francis <colifran@amazon.com>

Code has been updated following this review. The review is no longer relevant to the current code.

aws-cdk-automation · 2024-04-13T19:06:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 1f08e31
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-04-14T00:24:11Z

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*

github-actions bot added the p2 label Mar 29, 2024

aws-cdk-automation requested a review from a team March 29, 2024 08:30

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 29, 2024

This was referenced Apr 1, 2024

Monthly PR metrics report #29668

Closed

Weekly PR metrics report #29673

Closed

colifran changed the title ~~feat(custom-resources): add property to AwsCustomResourceProps to prevent logging API call response data~~ feat(custom-resources): add optional property to AwsCustomResourceProps to prevent logging API call response data Apr 1, 2024

vinayak-kukreja previously requested changes Apr 1, 2024

View reviewed changes

logging

33c7ac8

Signed-off-by: Francis <colifran@amazon.com>

colifran force-pushed the colifran/response-data branch from 33b18df to 33c7ac8 Compare April 4, 2024 02:35

aws-cdk-automation previously requested changes Apr 4, 2024

View reviewed changes

colifran added 2 commits April 3, 2024 19:46

v2 handler

3e0e2e1

Signed-off-by: Francis <colifran@amazon.com>

docstring

e87c3e6

Signed-off-by: Francis <colifran@amazon.com>

colifran changed the title ~~feat(custom-resources): add optional property to AwsCustomResourceProps to prevent logging API call response data~~ feat(custom-resources): add logging property to AwsCustomResourceProps and create Logging class Apr 4, 2024

colifran added 12 commits April 3, 2024 20:33

make logging abstract and implement render

415b0c6

Signed-off-by: Francis <colifran@amazon.com>

unit tests

dcc0dc8

Signed-off-by: Francis <colifran@amazon.com>

unit test for off

4e61b2d

Signed-off-by: Francis <colifran@amazon.com>

log sdk version

c8c0b64

Signed-off-by: Francis <colifran@amazon.com>

README

bcc1ab8

Signed-off-by: Francis <colifran@amazon.com>

readme

97660cd

Signed-off-by: Francis <colifran@amazon.com>

logging integ tests

d20b6b4

Signed-off-by: Francis <colifran@amazon.com>

integ test

ea8d4de

Signed-off-by: Francis <colifran@amazon.com>

selective logging should select things to log

840746a

Signed-off-by: Francis <colifran@amazon.com>

align readme

63914b9

Signed-off-by: Francis <colifran@amazon.com>

logging options

da86462

Signed-off-by: Francis <colifran@amazon.com>

snaps

43bdef9

Signed-off-by: Francis <colifran@amazon.com>

class doc string

af67a3b

Signed-off-by: Francis <colifran@amazon.com>

colifran added 2 commits April 12, 2024 15:55

response->responding

f4f5bcf

Signed-off-by: Francis <colifran@amazon.com>

comment

cd5016d

Signed-off-by: Francis <colifran@amazon.com>

colifran added the pr/do-not-merge This PR should not be merged at this time. label Apr 12, 2024

colifran added 4 commits April 12, 2024 16:19

update v3 handler

6b3f7e9

Signed-off-by: Francis <colifran@amazon.com>

fix v2 handler

770ef5d

Signed-off-by: Francis <colifran@amazon.com>

comment

3ebb3cf

Signed-off-by: Francis <colifran@amazon.com>

update aws custom resource snaps

4630b9b

Signed-off-by: Francis <colifran@amazon.com>

scanlonp approved these changes Apr 12, 2024

View reviewed changes

colifran added 10 commits April 12, 2024 17:38

update open searh snaps

0a08663

Signed-off-by: Francis <colifran@amazon.com>

events targets integ tests

da38d95

Signed-off-by: Francis <colifran@amazon.com>

elastic search integ tests

d8902ed

Signed-off-by: Francis <colifran@amazon.com>

synthetics integ tests

15ef34c

Signed-off-by: Francis <colifran@amazon.com>

s3 integ tests

53640b3

Signed-off-by: Francis <colifran@amazon.com>

route53 integ tests

33d6a5b

Signed-off-by: Francis <colifran@amazon.com>

logs detinations integ tests

4d934d7

Signed-off-by: Francis <colifran@amazon.com>

global accel snaps

a6384e7

Signed-off-by: Francis <colifran@amazon.com>

elastic load balancing v2 snaps

90da2b2

Signed-off-by: Francis <colifran@amazon.com>

elastic load balancing v2 snaps

e3b2344

Signed-off-by: Francis <colifran@amazon.com>

Merge branch 'main' into colifran/response-data

1f08e31

colifran removed the pr/do-not-merge This PR should not be merged at this time. label Apr 14, 2024

mergify bot merged commit b049064 into main Apr 14, 2024
15 checks passed

mergify bot deleted the colifran/response-data branch April 14, 2024 00:24

colifran mentioned this pull request Apr 17, 2024

chore: add aws-sdk as dev dependency for unit tests and satisfy AwsSdkCall in unit tests #29860

Merged

1 task

p3tek mentioned this pull request May 9, 2024

custom-resources: PR:29648 caused Custom resources to be replaced #30121

Open

This was referenced May 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#98

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#99

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #29648

colifran commented Mar 29, 2024 •

edited

vinayak-kukreja left a comment

vinayak-kukreja Apr 1, 2024

colifran Apr 1, 2024

vinayak-kukreja Apr 1, 2024

colifran Apr 1, 2024

vinayak-kukreja Apr 1, 2024

colifran Apr 1, 2024

TheRealAmazonKendra commented Apr 3, 2024

colifran commented Apr 3, 2024

aws-cdk-automation left a comment

colifran commented Apr 4, 2024

scanlonp left a comment

scanlonp Apr 12, 2024

colifran Apr 13, 2024 •

edited

aws-cdk-automation commented Apr 13, 2024

mergify bot commented Apr 14, 2024

	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:

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 #29648

Conversation

colifran commented Mar 29, 2024 • edited

Reason for this change

Description of changes

Description of how you validated changes

Checklist

vinayak-kukreja left a comment

Choose a reason for hiding this comment

vinayak-kukreja Apr 1, 2024

Choose a reason for hiding this comment

colifran Apr 1, 2024

Choose a reason for hiding this comment

vinayak-kukreja Apr 1, 2024

Choose a reason for hiding this comment

colifran Apr 1, 2024

Choose a reason for hiding this comment

vinayak-kukreja Apr 1, 2024

Choose a reason for hiding this comment

colifran Apr 1, 2024

Choose a reason for hiding this comment

TheRealAmazonKendra commented Apr 3, 2024

colifran commented Apr 3, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

colifran commented Apr 4, 2024

scanlonp left a comment

Choose a reason for hiding this comment

scanlonp Apr 12, 2024

Choose a reason for hiding this comment

colifran Apr 13, 2024 • edited

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 13, 2024

AWS CodeBuild CI Report

mergify bot commented Apr 14, 2024

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 #29648

colifran commented Mar 29, 2024 •

edited

colifran Apr 13, 2024 •

edited