feat(rds): database proxy #8476

civilizeddev · 2020-06-10T15:59:18Z

closes #8475

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

viktor-ku · 2020-06-12T10:03:09Z

Cool, so I'll be able to take proxy.endpoint; and add it to lambda env vars to have access to my db?

civilizeddev · 2020-06-12T14:28:59Z

Cool, so I'll be able to take proxy.endpoint; and add it to lambda env vars to have access to my db?

@viktor-ku As for the endpoint, yes. But you still need to provide credentials to authenticate.

nija-at

Thanks for submitting this PR!

I've requested some changes below, as a first round.

packages/@aws-cdk/aws-rds/lib/proxy.ts

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

packages/@aws-cdk/aws-rds/lib/proxy.ts

Pull request has been modified.

civilizeddev · 2020-06-23T00:13:55Z

affected by #8686

nija-at

@civilizeddev - sorry about the delayed response to this PR.

See some of my comments below.

packages/@aws-cdk/aws-rds/lib/proxy.ts

nija-at · 2020-06-24T09:33:36Z

packages/@aws-cdk/aws-rds/lib/proxy.ts

+  /**
+   * The kinds of databases that the proxy can connect to.
+   * This value determines which database network protocol the proxy recognizes when it interprets network traffic to
+   * and from the database.
+   * The engine family applies to MySQL and PostgreSQL for both RDS and Aurora.
+   */
+  readonly engineFamily: ProxyEngineFamily;


This information is already available in the DatabaseInstance or DatabaseCluster from its engine. We should determine this automatically, at least as its default and make the property optional.

@skinny85 - is this something that can be easily done after #8686

nija-at · 2020-06-24T09:39:55Z

packages/@aws-cdk/aws-rds/lib/proxy.ts

+   * A Boolean parameter that specifies whether Transport Layer Security (TLS) encryption is required for connections to the proxy.
+   * By enabling this setting, you can enforce encrypted TLS connections to the proxy.
+   *
+   * @default false


Enabling TLS by default is a common security best practice. Any particular reason it should be different here?

packages/@aws-cdk/aws-rds/lib/proxy.ts

Pull request has been modified.

closes #8475

civilizeddev · 2020-06-29T01:05:58Z

Everything cannot be perfect at once.

Is it worth weeks to make this perfect?

While not perfect, it can deliver value to those who want to use the L2 Contstruct for DB Proxy at the moment.
(That's I, I want to use it directly from aws-cdk)

And let someone after me have a chance to fix the rest later.

nija-at · 2020-06-29T10:08:43Z

Everything cannot be perfect at once.

Is it worth weeks to make this perfect?

The goal of these comments is to get the public API as close to correct as possible and consistent with the rest of the CDK. The implementation can always be fixed up later, without breaking customers.

I have pushed a change to adjust the API and make it a bit cleaner. I hope you don't mind.

civilizeddev · 2020-06-29T12:49:39Z

#8476 (comment)

I think ProxyTarget.fromInstance(), ProxyTarget.fromCluster() make more sense as @jogold suggests.

There are many cases that static fromXXX, which means 'to create <Return Type> from XXX', are used in aws-cdk,

and I am also very familiar with it.

But now you introduced .atXXX() for the first time.

Is it really for "consistent with the rest of the CDK."

nija-at · 2020-06-29T13:26:07Z

The fromXXX() methods are usually import APIs that allow importing an external resource into a CDK app. This is not what this API is doing.

I've changed for it to be forXXX() instead. This way it means 'create a proxy target for "this" db instance'. Hope that's better.

civilizeddev · 2020-06-29T13:55:46Z

Objection, your honor!

There are many counter evidences in aws-cdk.

fromXXX() is not just for importing external resources.

amplify

aws-cdk/packages/@aws-cdk/aws-amplify/lib/basic-auth.ts

Line 60 in af69f80

public static fromCredentials(username: string, password: SecretValue) {

aws-cdk/packages/@aws-cdk/aws-amplify/lib/basic-auth.ts

Line 72 in af69f80

    
           public static fromGeneratedPassword(username: string, encryptionKey?: kms.IKey) {

apigateway

aws-cdk/packages/@aws-cdk/aws-apigateway/lib/api-definition.ts

Line 14 in af69f80

    
           public static fromBucket(bucket: s3.IBucket, key: string, objectVersion?: string): S3ApiDefinition {

aws-cdk/packages/@aws-cdk/aws-apigateway/lib/api-definition.ts

Line 64 in af69f80

public static fromInline(definition: any): InlineApiDefinition {

aws-cdk/packages/@aws-cdk/aws-apigateway/lib/api-definition.ts

Line 72 in af69f80

    
           public static fromAsset(file: string, options?: s3_assets.AssetOptions): AssetApiDefinition {

appsync

aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

Line 1195 in af69f80

public static fromString(template: string): MappingTemplate {

aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

Line 1202 in af69f80

public static fromFile(fileName: string): MappingTemplate {

cloudformation

aws-cdk/packages/@aws-cdk/aws-cloudformation/lib/custom-resource.ts

Line 43 in af69f80

public static fromLambda(handler: lambda.IFunction): CustomResourceProvider {

aws-cdk/packages/@aws-cdk/aws-cloudformation/lib/custom-resource.ts

Line 50 in af69f80

public static fromTopic(topic: sns.ITopic): CustomResourceProvider {

cloudfront

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

Line 438 in af69f80

    
           public static fromAcmCertificate(certificate: certificatemanager.ICertificate, options: ViewerCertificateOptions = {}) {

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

Line 456 in af69f80

    
           public static fromIamCertificate(iamCertificateId: string, options: ViewerCertificateOptions = {}) {

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

Line 476 in af69f80

public static fromCloudFrontDefaultCertificate(...aliases: string[]) {

codebuild

aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

Line 1340 in af69f80

    
           public static fromDockerRegistry(name: string, options: DockerImageOptions = {}): IBuildImage {

aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

Line 1359 in af69f80

    
           public static fromEcrRepository(repository: ecr.IRepository, tag: string = 'latest'): IBuildImage {

aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

Line 1370 in af69f80

    
           public static fromAsset(scope: Construct, id: string, props: DockerImageAssetProps): IBuildImage {

aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

Line 1389 in af69f80

public static fromCodeBuildImageId(id: string): IBuildImage {

ecs

aws-cdk/packages/@aws-cdk/aws-ecs/lib/container-definition.ts

Line 19 in af69f80

public static fromSsmParameter(parameter: ssm.IParameter): Secret {

aws-cdk/packages/@aws-cdk/aws-ecs/lib/container-definition.ts

Line 36 in af69f80

    
           public static fromSecretsManager(secret: secretsmanager.ISecret, field?: string): Secret {

aws-cdk/packages/@aws-cdk/aws-ecs/lib/container-image.ts

Line 13 in af69f80

public static fromRegistry(name: string, props: RepositoryImageProps = {}) {

aws-cdk/packages/@aws-cdk/aws-ecs/lib/container-image.ts

Line 20 in af69f80

    
           public static fromEcrRepository(repository: ecr.IRepository, tag: string = 'latest') {

aws-cdk/packages/@aws-cdk/aws-ecs/lib/container-image.ts

Line 32 in af69f80

public static fromAsset(directory: string, props: AssetImageProps = {}) {

route53

aws-cdk/packages/@aws-cdk/aws-route53/lib/record-set.ts

Line 154 in af69f80

public static fromValues(...values: string[]) {

aws-cdk/packages/@aws-cdk/aws-route53/lib/record-set.ts

Line 161 in af69f80

public static fromAlias(aliasTarget: IAliasRecordTarget) {

aws-cdk/packages/@aws-cdk/aws-route53/lib/record-set.ts

Line 168 in af69f80

public static fromIpAddresses(...ipAddresses: string[]) {

nija-at · 2020-06-29T14:29:55Z

In many of these cases, 'from' is the correct preposition, probably not all.

Feel free to change the API back to using from - I don't mind at all. And I'll be happy to merge.

civilizeddev · 2020-07-01T00:45:26Z

I would like to get feedback from cdk users and follow it up.

viktor-ku · 2020-07-01T08:02:08Z

Thanks for your effort @civilizeddev and reviewers! I cannot wait for it to be merged, because it seems like a such enabler for lambdas!

nija-at

Looks good.

mergify · 2020-07-01T08:27:42Z

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 · 2020-07-01T08:46:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2c07dff
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-07-01T08:47:05Z

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).

civilizeddev · 2020-07-04T01:32:50Z

As for #8896, I wonder how it passed the integration tests at that moment.

fixes #8885 follows #8476 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

SomayaB assigned nija-at Jun 11, 2020

nija-at previously requested changes Jun 16, 2020

View reviewed changes

jogold reviewed Jun 16, 2020

View reviewed changes

packages/@aws-cdk/aws-rds/lib/proxy.ts Outdated Show resolved Hide resolved

nija-at previously requested changes Jun 24, 2020

View reviewed changes

nija-at changed the title ~~feat(rds): Add L2 DatabaseProxy (AWS::RDS::DBProxy)~~ feat(rds): database proxy Jun 24, 2020

civilizeddev added 4 commits June 26, 2020 09:20

feat(rds): Add L2 DatabaseProxy (AWS::RDS::DBProxy)

d243230

closes #8475

feat(rds): Add L2 DatabaseProxy (AWS::RDS::DBProxy)

8ff4418

closes #8475

feat(rds): database proxy

31ca83c

feat(rds): database proxy

f51fbee

adjustments to the proxy target API

4a8eb85

switch atXXX() to forXXX()

25f977b

Just simply use 'fromXXX'

a68cd96

nija-at approved these changes Jul 1, 2020

View reviewed changes

Merge branch 'master' into feat/rds

2c07dff

mergify bot merged commit e0e5e03 into aws:master Jul 1, 2020

civilizeddev deleted the feat/rds branch July 1, 2020 08:53

civilizeddev mentioned this pull request Jul 4, 2020

fix(rds): proxy for db cluster fails with model validation error #8896

Merged

civilizeddev mentioned this pull request Jul 27, 2020

fix(rds): Add engine to IDatabaseInstance, IClusterInstance #9257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): database proxy #8476

feat(rds): database proxy #8476

civilizeddev commented Jun 10, 2020 •

edited by nija-at

viktor-ku commented Jun 12, 2020

civilizeddev commented Jun 12, 2020 •

edited

nija-at left a comment

civilizeddev commented Jun 23, 2020

nija-at left a comment

nija-at Jun 24, 2020

nija-at Jun 24, 2020

civilizeddev commented Jun 29, 2020

nija-at commented Jun 29, 2020

civilizeddev commented Jun 29, 2020

nija-at commented Jun 29, 2020

civilizeddev commented Jun 29, 2020 •

edited

nija-at commented Jun 29, 2020

civilizeddev commented Jul 1, 2020

viktor-ku commented Jul 1, 2020

nija-at left a comment

mergify bot commented Jul 1, 2020

aws-cdk-automation commented Jul 1, 2020

mergify bot commented Jul 1, 2020

civilizeddev commented Jul 4, 2020

feat(rds): database proxy #8476

feat(rds): database proxy #8476

Conversation

civilizeddev commented Jun 10, 2020 • edited by nija-at

viktor-ku commented Jun 12, 2020

civilizeddev commented Jun 12, 2020 • edited

nija-at left a comment

Choose a reason for hiding this comment

civilizeddev commented Jun 23, 2020

nija-at left a comment

Choose a reason for hiding this comment

nija-at Jun 24, 2020

Choose a reason for hiding this comment

nija-at Jun 24, 2020

Choose a reason for hiding this comment

civilizeddev commented Jun 29, 2020

nija-at commented Jun 29, 2020

civilizeddev commented Jun 29, 2020

nija-at commented Jun 29, 2020

civilizeddev commented Jun 29, 2020 • edited

amplify

apigateway

appsync

cloudformation

cloudfront

codebuild

ecs

route53

nija-at commented Jun 29, 2020

civilizeddev commented Jul 1, 2020

viktor-ku commented Jul 1, 2020

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 1, 2020

aws-cdk-automation commented Jul 1, 2020

AWS CodeBuild CI Report

mergify bot commented Jul 1, 2020

civilizeddev commented Jul 4, 2020

civilizeddev commented Jun 10, 2020 •

edited by nija-at

civilizeddev commented Jun 12, 2020 •

edited

civilizeddev commented Jun 29, 2020 •

edited