search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.36k stars 3.77k forks source link

[aws-rds] ParameterGroup is missing from CloudFormation template #9741

Open manospasj opened 3 years ago

manospasj commented 3 years ago

Since version 1.58.0 ParameterGroup resources are missing from the generated CloudFormation template.

Reproduction Steps

What did you expect to happen?

The AWS::RDS::DBParameterGroup resource should exist in both versions of aws cdk.

What actually happened?

The AWS::RDS::DBParameterGroup resource exists in version 1.51.0 but not in 1.58.0.

Environment

This is :bug: Bug Report

Chriscbr commented 3 years ago

It looks like this was caused by #8959, and the current behavior is that the parameter group only gets constructed when it gets bound to an RDS cluster or instance - I think this can be achieved by either passing it in as a parameter in your constructor, as shown in this example here, or by manually calling the bindToInstance or bindToCluster methods described in the docs here.

Would either of these options work for your use case? If not, I think this would be a pretty easy fix to return to the previous behavior.

manospasj commented 3 years ago

I don't want to bound the parameter group to an instance, as the RDS instances are not part of my cdk app. Technically creating a parameter group without an RDS instance is possible using CloudFormation, I don't see why cdk should behave in a different way.

Also, since the bindToInstance doesn't require the instance as a parameter, how will this help? What does this method do?

Chriscbr commented 3 years ago

If you call the method like so, you should find that the parameter group now appears in the CloudFormation template:

new ParameterGroup(this, "RDS-ParameterGroup", {
  engine: DatabaseInstanceEngine.mysql({
    version: MysqlEngineVersion.VER_5_7,
  }),
}).bindToInstance({});

Maybe @skinny85 can shed some light here - but I think the reason it's implemented like this currently is that right now, a ParameterGroup can be constructed to make either a DBParameterGroup by calling bindToInstance, or a DBClusterParameterGroup by calling bindToCluster, so the class is somewhat multi-purpose. But perhaps the docs could be clearer about this to explain how these work / which resources they generate.

~(It also looks like right now, if you tried to call bindToInstance() and then bindToCluster() afterwards, it would just ignore the second call, and perhaps it should really throw an error - but that's a separate issue.)~ EDIT: I misread a bit of the code, you can bind both as described by skinny85 below after all.

manospasj commented 3 years ago

Thanks for your explanation, that makes perfect sense now. I agree that the docs could be better on this, as I spend quite some time reading them before opening this ticket and I would never guess that this is how it works.

Also, this line in the source code needs to be updated as it suggests that the ParameterGroup class can only create a AWS::RDS::DBParameterGroup resource. The AWS::RDS::DBClusterParameterGroup resource needs to be included.

skinny85 commented 3 years ago

Hey @manospasj ,

yes, the issue is exactly what @Chriscbr said. Basically, the ParameterGroup class is used both for AWS::RDS::DBParameterGroup and AWS::RDS::DBClusterParameterGroup, and creates the correct underlying resource when it's passed to Instance or Cluster, respectively.

Also, this line in the source code needs to be updated as it suggests that the ParameterGroup class can only create a AWS::RDS::DBParameterGroup resource. The AWS::RDS::DBClusterParameterGroup resource needs to be included.

Unfortunately, we can't have multiple CloudFormation resource types there. But I'll try to update the docs to make it sure it's more clear.

(It also looks like right now, if you tried to call bindToInstance() and then bindToCluster() afterwards, it would just ignore the second call, and perhaps it should really throw an error - but that's a separate issue.)

Nope, that is not the behavior currently. The second call, to bindToCluster(), will actually create the AWS::RDS::DBClusterParameterGroup. So you'll have 2 groups, an AWS::RDS::DBParameterGroup and an AWS::RDS::DBClusterParameterGroup.

lossanarch commented 2 years ago

imho this bindTo...() method is weird and non-standard when compared to the rest of cdk. It may result in a small amount of duplication but would be more transparent and intuitive to reverse #8959 and just have the object constructed as normal and have the user decide which they want by deliberately constructing one or the other. This is magic, which is why it's catching people out and needs better documentation. It would document itself if it were two separate classes. Just my 2c.

skinny85 commented 2 years ago

@lossanarch I'm not disagreeing with your assessment, but it's a little late for that now 😛 (I'm not sure how easy it would be to reverse https://github.com/aws/aws-cdk/pull/8959 without introducing a breaking change).

lossanarch commented 2 years ago

Yeah you're probably right, likely a fairly hard breaking change. Sorry, I only first came across this today. I wonder if the bindTo methods could be left in a working but deprecated state to prevent the break, while re/adding the NewParameterGroup/NewClusterParameterGroup methods. Looks feasible at first glance although I haven't had an in-depth look yet. Hey thanks for the quick response 👍

skinny85 commented 2 years ago

@lossanarch if you want to shoot me a quick PR (doesn't have to be super polished, just a draft with the rough proposal is fine), I'd be down to discuss the concrete steps to fix this 😉.