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

EC2: EC2.Volume.throughput does not get passed on into CFN #24107

Closed
chrisdotn opened this issue Feb 10, 2023 · 2 comments · Fixed by #24118
Closed

EC2: EC2.Volume.throughput does not get passed on into CFN #24107

chrisdotn opened this issue Feb 10, 2023 · 2 comments · Fixed by #24118
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@chrisdotn
Copy link

Describe the bug

I'm using EC2.Volume to create a GP3-Volume and I specify the throughput explicitly. The value does not make it to the CFN script and hence the resulting volume has the default througput of 125.

This is the CDK:

import { Stack, StackProps, RemovalPolicy, Size, Tags } from "aws-cdk-lib";
import { Construct } from "constructs";
import { Volume, EbsDeviceVolumeType } from "aws-cdk-lib/aws-ec2";
import { NagSuppressions } from "cdk-nag";

export interface DataVolumeStackProps extends StackProps { }

export class DataVolumeStack extends Stack {
  public readonly dataVolume: Volume
  constructor(scope: Construct, id: string, props: DataVolumeStackProps) {
    super(scope, id, props);

    this.dataVolume = new Volume(this, 'DataVolume', {
      availabilityZone: Stack.of(this).availabilityZones[0],
      size: Size.tebibytes(2),
      volumeType: EbsDeviceVolumeType.GP3,
      removalPolicy: RemovalPolicy.RETAIN,
      throughput: 250,
    })

    Tags.of(this.dataVolume).add('Name', 'Data Volume')

    // cdk-nag suppressions
    NagSuppressions.addResourceSuppressions(
      this,
      [
        {
          id: "AwsSolutions-EC26",
          reason: "Volume used for storing public data, no need for encryption",
        },
      ],
      true
    );
  }
}

The resulting CFN doesn't say anything about throughput:

Resources:
  DataVolume175940C2:
    Type: AWS::EC2::Volume
    Properties:
      AvailabilityZone: eu-west-1a
      MultiAttachEnabled: false
      Size: 2048
      Tags:
        - Key: Name
          Value: Data Volume
      VolumeType: gp3
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: DataVolumeStack/DataVolume/Resource
      cdk_nag:
        rules_to_suppress:
          - reason: Volume used for storing public data, no need for encryption
            id: AwsSolutions-EC26
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/zPSMzPWM1BMLC/WTU7J1s3JTNKrDi5JTM7WAQrFpyYb6VWH5eeU5qbqOKflQVi1IGZQanF+aVEykJOXn5Kql1WsX2ZopmdoCTQrqzgzU7eoNK8kMzdVLwhCAwCjDN5cZwAAAA==
    Metadata:
      aws:cdk:path: DataVolumeStack/CDKMetadata/Default
  
// cut Output, Parameters, Rules 

Expected Behavior

The resulting CFN should include a Throughput setting, the resulting Volume should have the correct througput.

Current Behavior

The resulting CFN does not include a Throughput setting, the resulting Volume has the default througput of 125.

Reproduction Steps

cdk synth

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.64.0

Framework Version

No response

Node.js Version

v16.19.0

OS

MacOS (Ventura)

Language

Typescript

Language Version

No response

Other information

No response

@chrisdotn chrisdotn added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 10, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Feb 10, 2023
@peterwoodworth
Copy link
Contributor

You're right, we aren't setting this when we create the L1 resource

const resource = new CfnVolume(this, 'Resource', {
availabilityZone: props.availabilityZone,
autoEnableIo: props.autoEnableIo,
encrypted: props.encrypted,
kmsKeyId: props.encryptionKey?.keyArn,
iops: props.iops,
multiAttachEnabled: props.enableMultiAttach ?? false,
size: props.size?.toGibibytes({ rounding: SizeRoundingBehavior.FAIL }),
snapshotId: props.snapshotId,
volumeType: props.volumeType ?? EbsDeviceVolumeType.GENERAL_PURPOSE_SSD,
});

The only thing the throughput prop gets used for is to verify that it's valid. We don't even have any tests which use the throughput prop that aren't just checking for errors.

Before we support setting the throughput, you can use an escape hatch to override the template with this property

@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p1 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 Feb 10, 2023
@mergify mergify bot closed this as completed in #24118 Feb 12, 2023
mergify bot pushed a commit that referenced this issue Feb 12, 2023
## EC2.Volume.throughput does not get passed on into CFN

The throughput parameter of the Ec2.Volume class (L2 Construct) was not set to the CfnVolume parameter as described in #24107

Fixed it so that the value is set.

Closes #24107 

----

*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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants