fix(rds): Add engine to IDatabaseInstance, IClusterInstance

[civilizeddev]

fixes #9195
related to #8686

BREAKING CHANGE: Constructs for imported resources take additional parameters in props

• rds: DatabaseCluster.fromDatabaseClusterAttributes takes engine: IClusterEngine
• rds: DatabaseInstanceBase.fromDatabaseInstanceAttributes takes engine: IInstanceEngine

---

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 7ffd696
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[skinny85]

Thanks for the fix @civilizeddev , but I don't think this is the correct way to go here.

Instead, I think the solution here is to add optional parameters to ProxyTarget's fromInstance and fromCluster methods, of type IInstanceEngine and IClusterEngine, respectively.

[civilizeddev]

Thank you for reviewing @skinny85

But that way still lead to a weird interface,

then cdk users will not be able to .addProxy() with their imported instance or cluster.

I just want to achieve the goal mentioned below:

We should be able to interpret this automatically from the underlying engine of the given instance or cluster. How hard would that be?

cc @skinny85

Originally posted by @nija-at in #8476 (comment)

And to remove these ulgy and unstable implementation.

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

Lines 69 to 75 in 489eae6

	if (this.dbCluster && this.dbInstance) {
	throw new Error('Proxy cannot target both database cluster and database instance.');
	} else if (this.dbCluster) {
	engine = (this.dbCluster.node.defaultChild as CfnDBCluster).engine;
	} else if (this.dbInstance) {
	engine = (this.dbInstance.node.defaultChild as CfnDBInstance).engine;
	}

---

Likewise, the property vpc can also be derived from the instance or cluster, so that users don't have to pass to the props redundantly.

So I think it's better to have vpc, engine in IInstance and ICluster

But It might affect users of Import resources.

[civilizeddev]

Precedent cases

aws-ecs

interface ICluster

aws-cdk/packages/@aws-cdk/aws-ecs/lib/cluster.ts

Lines 479 to 482 in ac612c6

	/**
	* The VPC associated with the cluster.
	*/
	readonly vpc: ec2.IVpc;

this.vpc

aws-cdk/packages/@aws-cdk/aws-ecs/lib/cluster.ts

Line 121 in ac612c6

this.vpc = props.vpc || new ec2.Vpc(this, 'Vpc', { maxAzs: 2 });

Cluster. fromClusterAttributes(attrs: ClusterAttributes)

aws-cdk/packages/@aws-cdk/aws-ecs/lib/cluster.ts

Lines 55 to 61 in ac612c6

	export class Cluster extends Resource implements ICluster {
	/**
	* Import an existing cluster to the stack from its attributes.
	*/
	public static fromClusterAttributes(scope: Construct, id: string, attrs: ClusterAttributes): ICluster {
	return new ImportedCluster(scope, id, attrs);
	}

interface ClusterAttributes

aws-cdk/packages/@aws-cdk/aws-ecs/lib/cluster.ts

Lines 521 to 524 in ac612c6

	/**
	* The VPC associated with the cluster.
	*/
	readonly vpc: ec2.IVpc;

[skinny85]

@civilizeddev I will take your opinion up with the team, and we'll make a group decision, so it's not only up to me.

Marking as "Request changes" for now to stop if from being merged until we came up with a decision.

[civilizeddev]

Related Issue

[rds] rds proxy only works with certain version, check at compile time

IEngine is very required.

[skinny85]

Hey @civilizeddev ,

I've talked this over with the team, and unfortunately they sided with me 🙂.

The problem in your solution is that you are now requiring everyone who imports IDatabaseCluster and IDatabaseInstanceto provide the engine, even if they have no intention of using it! That seems like a very harsh thing to do.

So, I think adding them into fromInstance and fromCluster methods in ProxyTarget is the way to go.

Thanks,
Adam

[skinny85]

Hey @civilizeddev ,

I took the liberty of submitting my version of this functionality in #10488 . I went a very similar way to you, I just made engine optional in IDatabaseInstance and IDatabaseCluster.

Let me know what you think of my version!

[civilizeddev]

@skinny85

It looks better for now. You can go on with your version.