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

(ssm): StringListParameter generate wrong CF template and does not work unlike StringParameter #14364

Closed
sunshineo opened this issue Apr 24, 2021 · 3 comments
Assignees
Labels
@aws-cdk/aws-ssm Related to AWS Systems Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@sunshineo
Copy link

Using StringListParameter produced a piece of CF template code that does not work. The code follows a different pattern compare to the working StringParameter. If use a workaround to make it follow the same pattern then it works.

Reproduction Steps

    const securityGroupId = ssm.StringParameter.fromStringParameterName(this, 'EksClusterSecurityGroupId', 'EksClusterSecurityGroupId').stringValue
    const subnetIds = ssm.StringListParameter.fromStringListParameterName(this, 'EksVpcSubnetIdsList', 'EksVpcSubnetIdsList').stringListValue
    const subnetGroup = new ec.CfnSubnetGroup(this, 'RedisSubnetGroup', {
        subnetIds: subnetIds,
        description: 'SubnetGrop for GraphQL Redis'
    });

    const redis = new ec.CfnReplicationGroup(this, 'graphql-redis', {
      engine: 'redis',
      replicationGroupDescription: 'Redis replication group for GraphQL',
      securityGroupIds: [securityGroupId],
      cacheSubnetGroupName: subnetGroup.ref,
    })

The part of generated CF template relevant:
The StringParameter works

...
Parameters:
  EksClusterSecurityGroupIdParameter:
    Type: AWS::SSM::Parameter::Value<String>
    Default: EksClusterSecurityGroupId
...
    SecurityGroupIds:
        - Ref: EksClusterSecurityGroupIdParameter
...

The StringListParameter part did not work:

    SubnetIds:
        Fn::Split:
          - ","
          - "{{resolve:ssm:EksVpcSubnetIdsList}}"

What did you expect to happen?

Expect to have the same behavior for String and StringList parameters. Expect the CF template looks like this:

Parameters:
  EksClusterSecurityGroupIdParameter:
    Type: AWS::SSM::Parameter::Value<String>
    Default: EksClusterSecurityGroupId
  EksVpcSubnetIdsListParameter:
    Type: AWS::SSM::Parameter::Value<List<String>>
    Default: EksVpcSubnetIdsList
...
      SecurityGroupIds:
        - Ref: EksClusterSecurityGroupIdParameter
...
      SubnetIds:
        Ref: EksVpcSubnetIdsListParameter

What actually happened?

Error when run cdk deploy:

4:07:02 PM | UPDATE_FAILED        | AWS::ElastiCache::SubnetGroup      | RedisSubnetGroup
Some input subnets in :[subnet-0a98619236902dd41,subnet-03dc3c44f20a5b2fd,subnet-0f777749ecdf46d39,subnet-07b25009783baa9c9] are invalid. (Service: AmazonElastiCache; Status Code: 400; Error Code: InvalidParameterValue; Request ID: f8765da2-a0a1-4b4e-b7f6-a4afe8b09780; Proxy: null)

Environment

  • CDK CLI Version : 1.100.0
  • Framework Version: 1.100.0
  • Node.js Version:
  • OS :
  • Language (Version):

Other

There is a workaround to manually create a Parameter

const eksVpcSubnetIdsListParameter = new cdk.CfnParameter(this, 'subnetIds', {
      type: 'AWS::SSM::Parameter::Value<List<String>>',
      default: 'EksVpcSubnetIdsList',
    })
const subnetGroup = new ec.CfnSubnetGroup(this, 'RedisSubnetGroup', {
        subnetIds: eksVpcSubnetIdsListParameter.valueAsList,
        description: 'SubnetGrop for GraphQL Redis'
      });

This is 🐛 Bug Report

@sunshineo sunshineo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 24, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ssm Related to AWS Systems Manager label Apr 24, 2021
@MrArnoldPalmer MrArnoldPalmer added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Apr 27, 2021
@MrArnoldPalmer MrArnoldPalmer removed their assignment Jun 21, 2021
@github-actions
Copy link

This issue has not received any attention in 1 year. 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 Jun 21, 2022
@sunshineo
Copy link
Author

ping! anyone can help?

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 21, 2022
@corymhall corymhall self-assigned this Sep 1, 2022
mergify bot pushed a commit that referenced this issue Sep 15, 2022
This PR fixes some issues with how SSM parameter types are implemented.
Currently this module models a single type of parameter (`ParameterType`
enum) and that type is used to represent _both_ [CloudFormation SSM
Parameter types](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html#aws-ssm-parameter-types)
For example,

```ts
new cdk.CfnParameter(this, 'Param', {
  type: 'AWS::SSM::Parameter::Value<String>', // type
});
```

_and_ the [AWS::SSM::Parameter.type](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ssm-parameter.html#cfn-ssm-parameter-type)
For example,

```ts
new ssm.CfnParameter(this, 'Param', {
  type: 'String',
});
```

This overloading caused the issue in the referenced issue as well as
making it more confusing for the user. For example, You can specify a type when
creating a `StringParameter`, but you shouldn't need to since the only
valid values are `String | StringList` and these are modeled as two
separate classes `StringParameter & StringListParameter`.

To address this, the PR introduces a new enum `ParameterValueType` to
model the CloudFormation SSM Parameter Types. This enum is only used in
the `valueForXXX` and `fromXXX` methods since those return a CFN
parameter.

- Deprecated `ssm.StringParameter.valueForTypedStringParameter` since it
  uses the old overloaded `ParameterType`.
  - Introduce a new `ssm.StringParameter.valueForTypedStringParameterV2`
    that uses the new `ParameterValueType` enum
- Add `ssm.StringListParameter.valueForTypedListParameter`
- Add `ssm.StringListParameter.fromListParameterAttributes`
- Deprecated `StringParameterProps.type` since the value should only be
  `String`.

fix #12477, #14364


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
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.

homakk pushed a commit to homakk/aws-cdk that referenced this issue Dec 1, 2022
This PR fixes some issues with how SSM parameter types are implemented.
Currently this module models a single type of parameter (`ParameterType`
enum) and that type is used to represent _both_ [CloudFormation SSM
Parameter types](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html#aws-ssm-parameter-types)
For example,

```ts
new cdk.CfnParameter(this, 'Param', {
  type: 'AWS::SSM::Parameter::Value<String>', // type
});
```

_and_ the [AWS::SSM::Parameter.type](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ssm-parameter.html#cfn-ssm-parameter-type)
For example,

```ts
new ssm.CfnParameter(this, 'Param', {
  type: 'String',
});
```

This overloading caused the issue in the referenced issue as well as
making it more confusing for the user. For example, You can specify a type when
creating a `StringParameter`, but you shouldn't need to since the only
valid values are `String | StringList` and these are modeled as two
separate classes `StringParameter & StringListParameter`.

To address this, the PR introduces a new enum `ParameterValueType` to
model the CloudFormation SSM Parameter Types. This enum is only used in
the `valueForXXX` and `fromXXX` methods since those return a CFN
parameter.

- Deprecated `ssm.StringParameter.valueForTypedStringParameter` since it
  uses the old overloaded `ParameterType`.
  - Introduce a new `ssm.StringParameter.valueForTypedStringParameterV2`
    that uses the new `ParameterValueType` enum
- Add `ssm.StringListParameter.valueForTypedListParameter`
- Add `ssm.StringListParameter.fromListParameterAttributes`
- Deprecated `StringParameterProps.type` since the value should only be
  `String`.

fix aws#12477, aws#14364


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*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-ssm Related to AWS Systems Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

No branches or pull requests

3 participants