feat(synthetics): add vpc configuration #18447

RichiCoder1 · 2022-01-14T23:46:03Z

This PR adds vpc support to synthetics and is a continuation of #11865.

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

This PR is a continuation of aws#11865 with the intention of filling out the remaining RunConfig values as well as adding missing VpcConfig properties. I wasn't sure whether VPC settings should be top level (like Lambda) or in their own configuration object, so I made them seperate for now.

gitpod-io · 2022-01-14T23:46:06Z

packages/@aws-cdk/aws-synthetics/test/integ.canary.expected.json

packages/@aws-cdk/aws-synthetics/lib/schedule.ts

packages/@aws-cdk/aws-synthetics/lib/canary.ts

kaizencc · 2022-01-24T16:53:49Z

Hi @RichiCoder1, thanks for opening this PR! As a first step, lets split this into two PRs, one for runConfig and one for vpcConfig. Specifically for runConfig, can you tell me the value add for exposing the rest of those properties over just using escape hatches? I don't see much of a use case for them and we're certainly not blocking anyone. That's why I didn't implement them in the L2 and I don't see anyone knocking down the door for them (the way they were for environment variables). vpcConfig might be helpful, but also might be harder to get right.

RichiCoder1 · 2022-01-24T17:17:23Z

Specifically for runConfig, can you tell me the value add for exposing the rest of those properties over just using escape hatches? I don't see much of a use case for them and we're certainly not blocking anyone. That's why I didn't implement them in the L2 and I don't see anyone knocking down the door for them (the way they were for environment variables)

Partially completeness, partially convenience. I'm personally using the timeout and tracing properties, and while "normal" it's weird to drop the Cfn just to set them when they're simple to implement (and Duration/Size are much nicer to work with).

As a first step, lets split this into two PRs, one for runConfig and one for vpcConfig

I probably should have seen this ask coming, I'll see what I can do :). Before I do so, do we want the VPC configuration options to be top level options, or a sub config like I have in this PR?

kaizencc · 2022-01-24T21:37:55Z

I don't think its partially complete -- environment variables is its own thing separate from timeout and tracing. The canary L2 is not simply a shell for the L1 and we do not need to adhere to how cfn clusters properties; this means that supplying env variables in the L2 is visible at the top level. But sure, I hear your point that its low-hanging fruit and Duration/Size is better to work with. Happy to add them.

For vpcConfig, I think we should expose a property vpc: ec2.IVpc similar to what we do in lambda.

RichiCoder1 · 2022-02-25T17:55:06Z

Sorry for taking so long to circle back around on this. Gonna scope it back down to VPC (current biggest pain point IMHO) and then can do a follow up on Run Config props.

RichiCoder1 · 2022-03-03T21:07:03Z

This is should be good to review now!

kaizencc

Hi @RichiCoder1! Thanks for the PR - it looks like a great start. I don't have much experience with vpcs, so I'm hoping we can lean on your context to provide the best possible experience for our users.

packages/@aws-cdk/aws-synthetics/README.md

kaizencc · 2022-03-04T18:42:15Z

packages/@aws-cdk/aws-synthetics/lib/canary.ts

+ * Returns a canary schedule object
+ */


This probably should go back to the way it was

kaizencc · 2022-03-04T18:51:41Z

packages/@aws-cdk/aws-synthetics/test/integ.canary.ts

@@ -69,4 +70,15 @@ new synthetics.Canary(stack, 'MyPythonCanary', {
 runtime: synthetics.Runtime.SYNTHETICS_PYTHON_SELENIUM_1_0,
 });

+const vpc = new ec2.Vpc(stack, 'MyVpc', { maxAzs: 2 });


Please put this in a new integ test file called integ.vpc-canary.ts. If you can come up with stack verification steps that would be great too.

Sounds good. For stack verification, do mean something like checking the underlying Lambda is in a VPC, or simulate some sort of VPC-only scenario?

best if we can check underlying lambda is in vpc, if possible. i don't actually know how.

kaizencc · 2022-03-04T18:51:48Z

packages/aws-cdk-lib/package.json

@@ -499,6 +499,7 @@
 "./aws-iotthingsgraph": "./aws-iotthingsgraph/index.js",
 "./aws-iotwireless": "./aws-iotwireless/index.js",
 "./aws-ivs": "./aws-ivs/index.js",
+ "./aws-kafkaconnect": "./aws-kafkaconnect/index.js",


whats this?

Not sure, don't know how I missed this. Might have been added by some tooling or a bad merge. I'll strip it out and see if it comes back

packages/@aws-cdk/aws-synthetics/lib/canary.ts

Co-authored-by: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com>

Pull request has been modified.

kaizencc

Looks good to me @RichiCoder1. Approved, though I am going to wait on a second pair of eyes before we get this merged.

corymhall

@RichiCoder1 just a couple of comments, the main one around implementing IConnectable.

Since the Canary class can now have security groups, it should implement ec2.IConnectable

export class Canary extends cdk.Resource implements ec2.IConnectable {

corymhall · 2022-03-14T12:42:26Z

packages/@aws-cdk/aws-synthetics/lib/canary.ts

+ *
+ * @default false
+ */
+ readonly allowPrivateSubnet?: boolean;


I don't think this is needed. I think it should be fine to just add this to the documentation in the readme and the vpcSubnets docstring. Users will specify if they want to place it in isolated subnets via the vpcSubnets prop, asking again here feels like a "are you sure you know what you are doing?" and I don't think that is needed.

RichiCoder1 · 2022-03-14T20:44:46Z

Nuked allowPrivateSubnet and added IConnectable! Let me know if that looks good and the implementation looks appropriate @corymhall.

Pull request has been modified.

RichiCoder1 · 2022-03-14T20:44:53Z

Nuked allowPrivateSubnet and added IConnectable! Let me know if that looks good and the implementation looks appropriate @corymhall. Implementation more or less swiped from Lambda with wording tweaks (and a slightly variance in how security groups are appended)

corymhall

Just 1 more comment and then we are good to go!

corymhall · 2022-03-15T11:56:44Z

packages/@aws-cdk/aws-synthetics/lib/canary.ts

- environmentVariables: props.environmentVariables,
+ vpcId: props.vpc.vpcId,
+ subnetIds,
+ securityGroupIds: securityGroups.map(sg => sg.securityGroupId),


We should pull the list of security groups from this.connections since users will be able to modify this connections object.

securityGroupIds: Lazy.listValue({ produce: () => this.connections.securityGroups.map(sg => sg.securityGroupId) }),

@corymhall Done! Though used Lazy.list as I got a warning that listValue was deprecated and that seemed to be the replacement. Good to know too, I was wondering how this sort of lazy evaluation worked!

Pull request has been modified.

kaizencc

Looks great @RichiCoder1. Thanks for the contribution!

mergify · 2022-03-15T18:47:50Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-03-15T20:10:20Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 2a38371
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-03-15T20:10:37Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions bot added the @aws-cdk/aws-synthetics Related to Amazon CloudWatch Synthetics label Jan 14, 2022

github-actions bot assigned kaizencc Jan 14, 2022

RichiCoder1 added 3 commits January 15, 2022 00:47

update tests and integ commit

432eea0

remove cdk.out template stuff

0c23ec0

Merge branch 'aws:master' into aws-synthetic-runconfig-and-vpc

c293aa5

RichiCoder1 marked this pull request as ready for review January 16, 2022 02:18

RichiCoder1 commented Jan 16, 2022

View reviewed changes

packages/@aws-cdk/aws-synthetics/test/integ.canary.expected.json Outdated Show resolved Hide resolved

RichiCoder1 commented Jan 16, 2022

View reviewed changes

packages/@aws-cdk/aws-synthetics/lib/schedule.ts Outdated Show resolved Hide resolved

RichiCoder1 commented Jan 16, 2022

View reviewed changes

packages/@aws-cdk/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved

RichiCoder1 mentioned this pull request Jan 19, 2022

feat(synthetics): add RunConfig parameters to Cloudwatch Synthetics Canary L2 construct #11865

Closed

kaizencc removed their assignment Feb 3, 2022

Merge branch 'master' into aws-synthetic-runconfig-and-vpc

e7941a0

RichiCoder1 changed the title ~~feat(aws-synthetics): complete runConfig and add vpcConfig~~ feat(aws-synthetics): add vpc configuration Feb 25, 2022

github-actions bot assigned kaizencc Feb 25, 2022

RichiCoder1 added 6 commits February 25, 2022 18:00

remove runconfig changes, scope down to vpc

cde330c

revert runconfig related canary changes

12d2d10

add readme docs for vpc

303cb7e

fixup tests and docs

8b8ac53

include integration fixes

7ca0270

fix docs rosetta compilation

c2ef535

kaizencc previously requested changes Mar 4, 2022

View reviewed changes

Apply suggestions from code review

daa317f

Co-authored-by: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com>

kaizencc added pr/do-not-merge This PR should not be merged at this time. and removed @aws-cdk/aws-synthetics Related to Amazon CloudWatch Synthetics labels Mar 11, 2022

Merge branch 'master' into aws-synthetic-runconfig-and-vpc

6449e13

kaizencc changed the title ~~feat(aws-synthetics): add vpc configuration~~ feat(synthetics): add vpc configuration Mar 11, 2022

github-actions bot added the @aws-cdk/aws-synthetics Related to Amazon CloudWatch Synthetics label Mar 11, 2022

kaizencc previously approved these changes Mar 11, 2022

View reviewed changes

corymhall previously requested changes Mar 14, 2022

View reviewed changes

remove private subnet warning and add IConnectable

8e38971

RichiCoder1 requested a review from corymhall March 14, 2022 20:44

RichiCoder1 added 2 commits March 14, 2022 20:49

tweak to only init connections if vpc, like lambda

adb0f98

further tweak wording

1445e2e

corymhall previously requested changes Mar 15, 2022

View reviewed changes

make sg ids be evaluated at render time

5eeefae

corymhall previously approved these changes Mar 15, 2022

View reviewed changes

corymhall requested a review from kaizencc March 15, 2022 17:26

Merge branch 'master' into aws-synthetic-runconfig-and-vpc

c29d7a6

kaizencc approved these changes Mar 15, 2022

View reviewed changes

kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Mar 15, 2022

Merge branch 'master' into aws-synthetic-runconfig-and-vpc

2a38371

mergify bot merged commit c991e92 into aws:master Mar 15, 2022

RichiCoder1 deleted the aws-synthetic-runconfig-and-vpc branch March 15, 2022 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(synthetics): add vpc configuration #18447

feat(synthetics): add vpc configuration #18447

RichiCoder1 commented Jan 14, 2022 •

edited by kaizencc

Loading

gitpod-io bot commented Jan 14, 2022

kaizencc commented Jan 24, 2022

RichiCoder1 commented Jan 24, 2022 •

edited

Loading

kaizencc commented Jan 24, 2022

RichiCoder1 commented Feb 25, 2022

RichiCoder1 commented Mar 3, 2022

kaizencc left a comment

kaizencc Mar 4, 2022

kaizencc Mar 4, 2022

RichiCoder1 Mar 4, 2022

kaizencc Mar 4, 2022

kaizencc Mar 4, 2022

RichiCoder1 Mar 4, 2022

kaizencc left a comment

corymhall left a comment

corymhall Mar 14, 2022

RichiCoder1 commented Mar 14, 2022

RichiCoder1 commented Mar 14, 2022 •

edited

Loading

corymhall left a comment

corymhall Mar 15, 2022

RichiCoder1 Mar 15, 2022 •

edited

Loading

kaizencc left a comment

mergify bot commented Mar 15, 2022

aws-cdk-automation commented Mar 15, 2022

mergify bot commented Mar 15, 2022

feat(synthetics): add vpc configuration #18447

feat(synthetics): add vpc configuration #18447

Conversation

RichiCoder1 commented Jan 14, 2022 • edited by kaizencc Loading

gitpod-io bot commented Jan 14, 2022

kaizencc commented Jan 24, 2022

RichiCoder1 commented Jan 24, 2022 • edited Loading

kaizencc commented Jan 24, 2022

RichiCoder1 commented Feb 25, 2022

RichiCoder1 commented Mar 3, 2022

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichiCoder1 commented Mar 14, 2022

RichiCoder1 commented Mar 14, 2022 • edited Loading

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichiCoder1 Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 15, 2022

aws-cdk-automation commented Mar 15, 2022

AWS CodeBuild CI Report

mergify bot commented Mar 15, 2022

RichiCoder1 commented Jan 14, 2022 •

edited by kaizencc

Loading

RichiCoder1 commented Jan 24, 2022 •

edited

Loading

RichiCoder1 commented Mar 14, 2022 •

edited

Loading

RichiCoder1 Mar 15, 2022 •

edited

Loading