aws-lambda-secretsmanager #162
Conversation
…ew changes requested
…ew changes requested
…crets manager secret.
feat(aws-lambda-secretsmanager): Create core helpers to provision secrets manager secret.
…m/aws-solutions-constructs into allen/aws-lambda-secrets-manager
…tests, and unit tests
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
On the whole this looks really good. A couple things that nibble around the edges.
For the integration tests - how did you generate your .expected files? They should be generated using cdk-integ, which will actually deploy the stack and grab the template. We weren't clear enough about this earlier and some contributors hand edited files and had integ tests that didn't deploy. Please confirm that yours are cdk-integ generated.
source/patterns/@aws-solutions-constructs/aws-lambda-secretsmanager/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-lambda-secretsmanager/lib/index.ts
Outdated
Show resolved
Hide resolved
| * @param {cdk.App} scope - represents the scope for all the resources. | ||
| * @param {string} id - this is a a scope-unique id. | ||
| * @param {LambdaToSecretsmanagerProps} props - user provided props for the construct. | ||
| * @since 1.49.0 |
There was a problem hiding this comment.
This will probably be 1.100.0 or 1.101.0 (depending upon how fast these reviews go). I'm going to ask the team about cutting this line entirely from now on - I don't want to have to come back an edit it when we know what release contains this PR.
| enableDnsSupport: true, | ||
| }, | ||
| }); | ||
| defaults.AddAwsServiceEndpoint(stack, vpc, defaults.ServiceEndpointTypes.SECRETS_MANAGER); |
There was a problem hiding this comment.
This test should not add the ServiceEndpoint - it should test that the construct adds a service endpoint to the existing VPC.
| removalPolicy: RemovalPolicy.DESTROY, | ||
| }); | ||
| // Assertion 1 | ||
| expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot(); |
There was a problem hiding this comment.
Only use snapshot for 1 test (probably all defaults) - for the rest just use the inspection functions to check the specific target of the test. (My first construct I used snapshots everywhere, Hitendra suggested not to do that because refreshing snapshots can be onerous - in the time since then I have learned exactly what he means. :-) )
| handler: "index.handler", | ||
| code: lambda.Code.fromAsset(`${__dirname}/lambda`), | ||
| }, | ||
| existingVpc: vpc |
There was a problem hiding this comment.
For integration tests, include secretProps with removalPolicy of DESTROY any time the integration test is creating a secret - otherwise every time we refresh all the integration tests we leave a bunch of secrets behind when we clean up.
| existingSecretObj: secret | ||
| }); | ||
| // Assertion 1 | ||
| expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot(); |
There was a problem hiding this comment.
Only use the snapshot on the default deployment - use inspection functions for other tests.
| * or in the 'license' file accompanying this file. This file is distributed on an 'AS IS' BASIS, WITHOUT WARRANTIES | ||
| * OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions | ||
| * and limitations under the License. | ||
| */ |
There was a problem hiding this comment.
Unit tests that appear to be missing:
- Existing function
- Grant Write access
(let me know if I just missed them)
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…ew changes requested
…crets manager secret.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
...terns/@aws-solutions-constructs/aws-lambda-secretsmanager/test/lambda-secretsmanager.test.ts
Show resolved
Hide resolved
...terns/@aws-solutions-constructs/aws-lambda-secretsmanager/test/lambda-secretsmanager.test.ts
Show resolved
Hide resolved
| const secret = new Secret(stack, 'secret', {}); | ||
| new LambdaToSecretsmanager(stack, 'lambda-to-secretsmanager-stack', { | ||
| const existingSecret = new Secret(stack, 'secret', {}); | ||
| const pattern = new LambdaToSecretsmanager(stack, 'lambda-to-secretsmanager-stack', { | ||
| lambdaFunctionProps: { |
There was a problem hiding this comment.
secretProps: { removalPolicy: RemovalPolicy.DESTROY } ?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -0,0 +1,387 @@ | |||
| /** | |||
There was a problem hiding this comment.
Request to add a unit test case for overriding readonly secretProps?: secretsmanager.SecretProps; the most common use case I can think of is passing in CMK SecretProps.encryptionKey to be used instead of the default aws/secretsmanager key for secret encryption
There was a problem hiding this comment.
Updated thanks @hnishar !
…ecretProps and validate the KMS key permissions.
…m/aws-solutions-constructs into allen/aws-lambda-secrets-manager
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This is now the most important solutions construct! I'm so happy! Shared it with a bunch of people. 💯 🥇 👍 You rock! Be encouraged! |
Issue #, if available: #76
Description of changes:
Implemented aws-lambda-secretsmanger construct
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.