aws_redshift: parameterGroupName is marked as optional in L1 constructs but cloudformation requires it now

venturaville commented 10 months ago

Describe the bug

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_redshift.CfnClusterParameterGroup.html#parametergroupname-1 The parameterGroupName is listed as optional on the L1 constructs doc page ^

The L2 constructor does not set it (but does have something it tracks called ‘clusterParameterGroupName’): https://github.com/aws/aws-cdk/blob/v2.118.0/packages/%40aws-cdk/aws-redshift-alpha/lib/parameter-group.ts#L76-L86 It also has not changed in ~ 7 months

| CREATE_FAILED | AWS::Redshift::ClusterParameterGroup | MyParameterGroup... Resource handler returned message: "1 validation error detected: Value null at 'parameterGroupName' failed to satisfy constraint: Member must not be null (Service: Redshift, Status Code: 400, Request ID: ...

The L2 constructor provides no way to set the parameterGroupName on the L1 construct manually, the L1 construct CfnClusterParameterGroup does not require it, but cloudformation itself does require it.

AFAIK, this behavior has changed semi recently as we have other CDK setting parameter groups which does not have parameterGroupName set in the final cloudformation generated.

Expected Behavior

Cloudformation should have accepted this if the parameterGroupName was optional as before.

Current Behavior

We get this error from cloudformation itself:

| CREATE_FAILED        | AWS::Redshift::ClusterParameterGroup        | MyParameterGroup...
Resource handler returned message: "1 validation error detected: Value null at 'parameterGroupName' failed to satisfy constraint: Member must not be null (Service: Redshift, Status Code: 400, Request ID: ...

This is different from previous behavior seen with the same CDK creating other clusters.

Reproduction Steps

    import * as redshift from '@aws-cdk/aws-redshift-alpha';

    const wlm = [
      {
        rules: [            ],
          },
      ];
    const loggingParamGroup = new redshift.ClusterParameterGroup(this, 'MyParameterGroup', {
      description: `Parameter Group for Cluster MyCluster`,
      parameters: {
        enable_user_activity_logging: 'true',
        wlm_json_configuration: JSON.stringify(wlm),
      },
    });

Possible Solution

Make the L2 construct in aws-redshift-alpha set the parameterGroupName from clusterParameterGroupName (see: parameter-group.ts

OR

Provide a way for the parameterGroupName to be manually set from the L2 construct onto the L1 construct

OR

Cloudformation should set the name to something reasonable when the name is not provided instead of requiring

If cloudformation is not updated, then the optional setting on parameterGroupName should be marked as required instead.

Additional Information/Context

No response

CDK CLI Version

2.117.0

Framework Version

No response

Node.js Version

v21.5.0

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

pahud commented 10 months ago

I just checked the CNF spec for us-east-1 https://d1uauaxba7bl26.cloudfront.net/latest/gzip/CloudFormationResourceSpecification.json

    "AWS::Redshift::ClusterParameterGroup.Parameter": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-redshift-clusterparametergroup-parameter.html",
      "Properties": {
        "ParameterValue": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-redshift-clusterparametergroup-parameter.html#cfn-redshift-clusterparametergroup-parameter-parametervalue",
          "UpdateType": "Mutable",
          "Required": true,
          "PrimitiveType": "String"
        },
        "ParameterName": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-redshift-clusterparametergroup-parameter.html#cfn-redshift-clusterparametergroup-parameter-parametername",
          "UpdateType": "Mutable",
          "Required": true,
          "PrimitiveType": "String"
        }
      }

Yes it is required.

And I checked this from @aws-cdk/aws-service-spec@v0.0.40 which is required as well. I believe the next release of aws-cdk should include aws-service-spec@v0.0.40 and before that you will need escape hatches e.g. addPropertyOverride() to override this property to satisfy this requirement for cloudformation. And after that, we'll need another PR to improve the L2 construct.

I believe CFN doc might reflect this change in the next few days.

mmarthab commented 10 months ago

Hi, document not updated yet to reflect this. Experienced the same issue today creating the parameter group via CDK failing with the same error.

pahud commented 5 months ago

No I was wrong.

According to the CFN spec: https://d1uauaxba7bl26.cloudfront.net/latest/gzip/CloudFormationResourceSpecification.json

ParameterGroupName is not required.

"AWS::Redshift::ClusterParameterGroup": {
      "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-redshift-clusterparametergroup.html",
      "Properties": {
        "Description": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-redshift-clusterparametergroup.html#cfn-redshift-clusterparametergroup-description",
          "UpdateType": "Immutable",
          "Required": true,
          "PrimitiveType": "String"
        },
        "Parameters": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-redshift-clusterparametergroup.html#cfn-redshift-clusterparametergroup-parameters",
          "UpdateType": "Mutable",
          "Required": false,
          "Type": "List",
          "ItemType": "Parameter",
          "DuplicatesAllowed": true
        },
        "ParameterGroupName": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-redshift-clusterparametergroup.html#cfn-redshift-clusterparametergroup-parametergroupname",
          "UpdateType": "Immutable",
          "Required": false,
          "PrimitiveType": "String"
        },

Are you still having this issue?

We need to report this to the CFN team.

Can you share your synthesized template for this resource?

aws / aws-cdk