Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RDS Proxy: adding multiple proxies via addProxy causes a circular dependency in the generated target groups #25633

Closed
benbelow opened this issue May 17, 2023 · 11 comments · Fixed by #28471
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@benbelow
Copy link

Describe the bug

adding multiple proxies via addProxy causes a circular dependency in the generated target groups.

Each target group declares an explicit dependency on the other.

Expected Behavior

Two proxies are generated independently

Current Behavior

CDK template cannot be deployed due to circular dependency

Reproduction Steps

const db = new rds.DatabaseCluster(this, 'DatabaseCluster', {
      engine: rds.DatabaseClusterEngine.auroraPostgres({
        version: rds.AuroraPostgresEngineVersion.VER_13_7,
      }),
      instanceProps: {
        instanceType: ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MEDIUM),
      },
      instances: 1,
      backup: { retention: Duration.days(7) },
      removalPolicy: RemovalPolicy.RETAIN,
      deletionProtection: true,
      credentials: rdsCredentials,
      defaultDatabaseName: this.dbName,
    });

    this.dbProxy = db.addProxy(id + 'proxy', {
      secrets: [rdsSecret],
      vpc: props.vpc,
      debugLogging: false,
      iamAuth: true,
    });

    this.readOnlyDbProxy = db.addProxy(id + 'read-only-proxy', {
      secrets: [rdsReadOnlySecret],
      vpc: props.vpc,
      debugLogging: false,
      iamAuth: true,
    });

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.79.1

Framework Version

No response

Node.js Version

19.7.0

OS

MacOS

Language

Typescript

Language Version

No response

Other information

No response

@benbelow benbelow added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 17, 2023
@github-actions github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label May 17, 2023
@peterwoodworth
Copy link
Contributor

I didn't get a circular reference here. Could you please provide information as to which resources are coming from another stack, as well as the error message?

@peterwoodworth peterwoodworth added p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels May 18, 2023
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 21, 2023
@benbelow
Copy link
Author

Here's the full error message:

Error: Template is undeployable, these resources have a dependency cycle: PersistenceDatabaseClusterPersistenceproxyProxyTargetGroupEF583265 -> PersistenceDatabaseClusterPersistencereadonlyproxyProxyTargetGroup59EC51C3 -> PersistenceDatabaseClusterPersistenceproxyProxyTargetGroupEF583265:

{
  "PersistenceDatabaseClusterPersistenceproxyProxyTargetGroupEF583265": {
    "Type": "AWS::RDS::DBProxyTargetGroup",
    "Properties": {
      "DBProxyName": {
        "Ref": "PersistenceDatabaseClusterPersistenceproxyE7E4B050"
      },
      "TargetGroupName": "default",
      "ConnectionPoolConfigurationInfo": {},
      "DBClusterIdentifiers": [
        {
          "Ref": "PersistenceDatabaseCluster18BA320A"
        }
      ]
    },
    "DependsOn": [
      "PersistenceDatabaseClusterInstance1DE398355",
      "PersistenceDatabaseClusterPersistenceproxyIAMRoleDefaultPolicy5A0465A0",
      "PersistenceDatabaseClusterPersistenceproxyIAMRoleD34900C0",
      "PersistenceDatabaseClusterPersistenceproxyProxySecurityGroup44F927CD",
      "PersistenceDatabaseClusterPersistenceproxyE7E4B050",
      "PersistenceDatabaseClusterPersistencereadonlyproxyIAMRoleDefaultPolicy6B659FA3",
      "PersistenceDatabaseClusterPersistencereadonlyproxyIAMRole609B991A",
      "PersistenceDatabaseClusterPersistencereadonlyproxyProxySecurityGroupD87486CE",
      "PersistenceDatabaseClusterPersistencereadonlyproxyProxyTargetGroup59EC51C3",
      "PersistenceDatabaseClusterPersistencereadonlyproxy164F9104",
      "PersistenceDatabaseCluster18BA320A",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistenceproxyProxySecurityGroup2E370FAEIndirectPort73146E71",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistenceproxyProxySecurityGroup2E370FAE5432C33FFAD3",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistencereadonlyproxyProxySecurityGroup9909B589IndirectPort828CD53D",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistencereadonlyproxyProxySecurityGroup9909B5895432FD41BB9E",
      "PersistenceDatabaseClusterSecurityGroup84299FB3",
      "PersistenceDatabaseClusterSubnets420E817A"
    ]
  },
  "PersistenceDatabaseClusterPersistencereadonlyproxyProxyTargetGroup59EC51C3": {
    "Type": "AWS::RDS::DBProxyTargetGroup",
    "Properties": {
      "DBProxyName": {
        "Ref": "PersistenceDatabaseClusterPersistencereadonlyproxy164F9104"
      },
      "TargetGroupName": "default",
      "ConnectionPoolConfigurationInfo": {},
      "DBClusterIdentifiers": [
        {
          "Ref": "PersistenceDatabaseCluster18BA320A"
        }
      ]
    },
    "DependsOn": [
      "PersistenceDatabaseClusterInstance1DE398355",
      "PersistenceDatabaseClusterPersistenceproxyIAMRoleDefaultPolicy5A0465A0",
      "PersistenceDatabaseClusterPersistenceproxyIAMRoleD34900C0",
      "PersistenceDatabaseClusterPersistenceproxyProxySecurityGroup44F927CD",
      "PersistenceDatabaseClusterPersistenceproxyProxyTargetGroupEF583265",
      "PersistenceDatabaseClusterPersistenceproxyE7E4B050",
      "PersistenceDatabaseClusterPersistencereadonlyproxyIAMRoleDefaultPolicy6B659FA3",
      "PersistenceDatabaseClusterPersistencereadonlyproxyIAMRole609B991A",
      "PersistenceDatabaseClusterPersistencereadonlyproxyProxySecurityGroupD87486CE",
      "PersistenceDatabaseClusterPersistencereadonlyproxy164F9104",
      "PersistenceDatabaseCluster18BA320A",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistenceproxyProxySecurityGroup2E370FAEIndirectPort73146E71",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistenceproxyProxySecurityGroup2E370FAE5432C33FFAD3",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistencereadonlyproxyProxySecurityGroup9909B589IndirectPort828CD53D",
      "PersistenceDatabaseClusterSecurityGroupfromPartnerHubPersistentTestsPersistenceDatabaseClusterPersistencereadonlyproxyProxySecurityGroup9909B5895432FD41BB9E",
      "PersistenceDatabaseClusterSecurityGroup84299FB3",
      "PersistenceDatabaseClusterSubnets420E817A"
    ]
  }
}

    at checkTemplateForCyclicDependencies (/Users/benbel/Repos/partner-hub/infrastructure/node_modules/aws-cdk-lib/assertions/lib/private/cyclic.js:1:768)
    at new Template (/Users/benbel/Repos/partner-hub/infrastructure/node_modules/aws-cdk-lib/assertions/lib/template.js:1:1931)
    at Function.fromStack (/Users/benbel/Repos/partner-hub/infrastructure/node_modules/aws-cdk-lib/assertions/lib/template.js:1:1021)
    at persistenceTemplate (/Users/benbel/Repos/partner-hub/infrastructure/stacks/partner-hub/persistence/tests/persistence-stack.test.ts:24:19)
    at Object.<anonymous> (/Users/benbel/Repos/partner-hub/infrastructure/stacks/partner-hub/persistence/tests/persistence-stack.test.ts:30:20)
    at Promise.then.completed (/Users/benbel/Repos/partner-hub/infrastructure/node_modules/jest-circus/build/utils.js:391:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/Users/benbel/Repos/partner-hub/infrastructure/node_modules/jest-circus/build/utils.js:316:10)
    at _callCircusTest (/Users/benbel/Repos/partner-hub/infrastructure/node_modules/jest-circus/build/run.js:218:40)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels May 22, 2023
@peterwoodworth
Copy link
Contributor

@benbelow can you please provide a snippet which will actually produce this error? Like I said before, I didn't reproduce this with the snippet linked, which cuts some resources out that are referenced within your snippet

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 22, 2023
@benbelow
Copy link
Author

The following code seems to repro the issue for me with no other dependencies / resources from my application.

import { Construct } from 'constructs/lib/construct';
import {
  aws_ec2 as ec2,
  aws_rds as rds,
  aws_secretsmanager as secretsManager,
  Duration,
  RemovalPolicy,
  Stack,
} from 'aws-cdk-lib';
import { DatabaseProxy } from 'aws-cdk-lib/aws-rds';
import { NodejsFunction } from 'aws-cdk-lib/aws-lambda-nodejs';

export class ExampleStack extends Stack {
  constructor(scope: Construct, id: string) {
    super(scope, id);

    const vpc = new ec2.Vpc(this, 'ExampleVPC', {
      subnetConfiguration: [
        {
          name: 'Isolated',
          subnetType: ec2.SubnetType.PRIVATE_ISOLATED,
        },
        {
          name: 'Public',
          subnetType: ec2.SubnetType.PUBLIC,
        },
        {
          name: 'PrivateWithEgress',
          subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS,
        },
      ],
      maxAzs: 2,
    });

    new ExampleDatabase(this, 'ExampleDatabase', { vpc });
  }
}

export class ExampleDatabase extends Construct {
  dbName = 'exampleDb';
  dbUsername = 'syscdk';
  dbUsernameReadOnly = 'syscdkreadonly';
  dbProxy: DatabaseProxy;
  readOnlyDbProxy: DatabaseProxy;

  constructor(scope: Construct, id: string, props: { vpc: ec2.Vpc }) {
    super(scope, id);

    const rdsSecret = new secretsManager.Secret(this, 'RdsCredentials', {
      generateSecretString: {
        secretStringTemplate: JSON.stringify({ username: this.dbUsername }),
        generateStringKey: 'password',
        excludePunctuation: true,
        includeSpace: false,
      },
    });

    const rdsReadOnlySecret = new secretsManager.Secret(this, 'RdsReadOnlyCredentials', {
      generateSecretString: {
        secretStringTemplate: JSON.stringify({ username: this.dbUsernameReadOnly }),
        generateStringKey: 'password',
        excludePunctuation: true,
        includeSpace: false,
      },
    });

    const rdsCredentials = rds.Credentials.fromSecret(rdsSecret);

    const db = new rds.DatabaseCluster(this, 'DatabaseCluster', {
      engine: rds.DatabaseClusterEngine.auroraPostgres({
        version: rds.AuroraPostgresEngineVersion.VER_13_7,
      }),
      instanceProps: {
        vpc: props.vpc,
        vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
        // TODO: Move to serverless when supported. See https://github.com/aws/aws-cdk/issues/20197
        instanceType: ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MEDIUM),
      },
      instances: 1,
      backup: { retention: Duration.days(7) },
      removalPolicy: RemovalPolicy.RETAIN,
      deletionProtection: true,
      credentials: rdsCredentials,
      defaultDatabaseName: this.dbName,
    });

    this.dbProxy = db.addProxy(id + 'proxy', {
      secrets: [rdsSecret],
      vpc: props.vpc,
      debugLogging: false,
      iamAuth: true,
    });

    this.readOnlyDbProxy = db.addProxy(id + 'read-only-proxy', {
      secrets: [rdsReadOnlySecret],
      vpc: props.vpc,
      debugLogging: false,
      iamAuth: true,
    });


    db.connections.allowFrom(this.dbProxy, ec2.Port.tcp(1234), 'Allow RDS Proxy connection to db');
    db.connections.allowFrom(
      this.readOnlyDbProxy,
      ec2.Port.tcp(1234),
      'Allow RDS (Readonly) Proxy connection to db',
    );
  }

  grantAccess(lambda: NodejsFunction) {
    this.dbProxy.grantConnect(lambda, this.dbUsername);
    lambda.connections.allowTo(
      this.dbProxy,
      ec2.Port.tcp(1234),
      `Allow ${lambda.functionName} Lambda connection to RDS Proxy`,
    );
  }

  grantReadOnlyAccess(lambda: NodejsFunction) {
    this.readOnlyDbProxy.grantConnect(lambda, this.dbUsernameReadOnly);
    lambda.connections.allowTo(
      this.readOnlyDbProxy,
      ec2.Port.tcp(1234),
      `Allow ${lambda.functionName} Lambda read-only connection to RDS Proxy`,
    );
  }
}

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 23, 2023
@peterwoodworth
Copy link
Contributor

Thanks for sharing this @benbelow, I've managed to reproduce it now. I was expecting that you were running into the error on cdk synth, however it's only being thrown on deploy here.

I am not sure, but if I were to guess I think this line is causing trouble somehow. https://github.com/aws/aws-cdk/blob/bbdb16ad65b1217901f92782a74a6f13222cd684/packages/aws-cdk-lib/aws-rds/lib/proxy.ts#LL493C7-L493C7

But I'm not really sure why the first Proxy is depending on the second. Thanks for reporting!

@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort and removed p2 effort/small Small work item – less than a day of effort labels May 23, 2023
@LorneCurrie
Copy link

Is there any update on this or a workaround?

@clarkmains
Copy link

Hi folks, this is an issue for us too - any new updates on this? Thanks.

@frankpengau
Copy link
Contributor

Any updates in regards to workarounds or solutions to this issue?

@jarredkenny
Copy link

Also blocked by this

@mergify mergify bot closed this as completed in #28471 Dec 28, 2023
mergify bot pushed a commit that referenced this issue Dec 28, 2023
#28471)

### Description
The related issue reports that deployment fails due to circular dependencies when multiple RDSProxy are created.
The `DatabaseProxy` uses the `node.addDependency` method to ensure that the `CfnDBProxyTargetGroup` is created after the `DBCluster` and `DBInstance` are created (#12237).

This works well for a single `DatabaseProxy`, but does not work well when multiple `DatabaseProxy` are created with `DatabaseCluster.addProxy`.
When creating a `DatabaseProxy` with the `DatabaseCluster.addProxy` method, it is created as a child of the `DatabaseCluster`.
https://github.com/aws/aws-cdk/blob/cd54c4239ec29182e30fd91634505df560d6e5f8/packages/aws-cdk-lib/aws-rds/lib/cluster.ts#L446

The `node.addDependency` method recursively sets dependencies on child Constructs, so if multiple `DatabaseProxy` are created as a child of a `DatabaseCluster` in the construct tree, multiple `DatabaseProxy` dependencies on each other.
If the `addProxy` method is not used, the user initializes the `DatabaseProxy` directly and it does not become a child of `DatabaseCluster`.
For example,
```ts
new DatabaseProxy(stack, 'DBProxy', {
  proxyTarget: rds.ProxyTarget.fromCluster(cluster),
  vpc,
});
```

I believe this is the cause of the `these resources have a dependency cycle` error reported in the related issue.

To correct this error, this PR uses `CfnResource.addDependency` instead of `node.addDependency` to avoid recurrent dependencies.

Closes #25633

----

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

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this issue Jan 5, 2024
aws#28471)

### Description
The related issue reports that deployment fails due to circular dependencies when multiple RDSProxy are created.
The `DatabaseProxy` uses the `node.addDependency` method to ensure that the `CfnDBProxyTargetGroup` is created after the `DBCluster` and `DBInstance` are created (aws#12237).

This works well for a single `DatabaseProxy`, but does not work well when multiple `DatabaseProxy` are created with `DatabaseCluster.addProxy`.
When creating a `DatabaseProxy` with the `DatabaseCluster.addProxy` method, it is created as a child of the `DatabaseCluster`.
https://github.com/aws/aws-cdk/blob/cd54c4239ec29182e30fd91634505df560d6e5f8/packages/aws-cdk-lib/aws-rds/lib/cluster.ts#L446

The `node.addDependency` method recursively sets dependencies on child Constructs, so if multiple `DatabaseProxy` are created as a child of a `DatabaseCluster` in the construct tree, multiple `DatabaseProxy` dependencies on each other.
If the `addProxy` method is not used, the user initializes the `DatabaseProxy` directly and it does not become a child of `DatabaseCluster`.
For example,
```ts
new DatabaseProxy(stack, 'DBProxy', {
  proxyTarget: rds.ProxyTarget.fromCluster(cluster),
  vpc,
});
```

I believe this is the cause of the `these resources have a dependency cycle` error reported in the related issue.

To correct this error, this PR uses `CfnResource.addDependency` instead of `node.addDependency` to avoid recurrent dependencies.

Closes aws#25633

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants