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

(aws-efs): EFS mount target will fail on subnet change #25099

Closed
kochie opened this issue Apr 13, 2023 · 4 comments · Fixed by #26155
Closed

(aws-efs): EFS mount target will fail on subnet change #25099

kochie opened this issue Apr 13, 2023 · 4 comments · Fixed by #26155
Labels
@aws-cdk/aws-efs Related to Amazon Elastic File System bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@kochie
Copy link
Contributor

kochie commented Apr 13, 2023

Describe the bug

When removing an EFS Mount point from a CDK deployment (say by defining fewer subnets than what are previously deployed), CDK will remove the correct mount point but will use the old unique Id for the deleted one if that subnet is in the middle of the subnet list. This will cause CloudFormation to fail as it tries to make a new Mount Point for the same EFS File System in the same AZ, which it can't do.

Expected Behavior

If I have a CDK construct

const fileSystem = new FileSystem(this, "EfsFileSystem", {
    vpc,
    vpcSubnets: {
        subnets: [subnetA, subnetB, subnetC]
    }
})

This will synth 3 Mount points, eg.

EfsFileSystemEfsMountTarget01:
  Properties:
    SubnetId: subnetA
EfsFileSystemEfsMountTarget02:
  Properties:
    SubnetId: subnetB
EfsFileSystemEfsMountTarget03:
  Properties:
    SubnetId: subnetC

When I remove one of the subnets, i.e. remove subnet B

const fileSystem = new FileSystem(this, "EfsFileSystem", {
    vpc,
    vpcSubnets: {
        subnets: [subnetA, subnetC]
    }
})

CDK synth will create 2 mount points with correct Ids

EfsFileSystemEfsMountTarget01:
  Properties:
    SubnetId: subnetA
EfsFileSystemEfsMountTarget03:
  Properties:
    SubnetId: subnetC

Current Behavior

If I have a CDK construct

const fileSystem = new FileSystem(this, "EfsFileSystem", {
    vpc,
    vpcSubnets: {
        subnets: [subnetA, subnetB, subnetC]
    }
})

This will synth 3 Mount points, eg.

EfsFileSystemEfsMountTarget01:
  Properties:
    SubnetId: subnetA
EfsFileSystemEfsMountTarget02:
  Properties:
    SubnetId: subnetB
EfsFileSystemEfsMountTarget03:
  Properties:
    SubnetId: subnetC

When I remove one of the subnets, i.e. remove subnet B

const fileSystem = new FileSystem(this, "EfsFileSystem", {
    vpc,
    vpcSubnets: {
        subnets: [subnetA, subnetC]
    }
})

CDK synth will create 2 mount points with an incorrect Id

EfsFileSystemEfsMountTarget01:
  Properties:
    SubnetId: subnetA
EfsFileSystemEfsMountTarget02:
  Properties:
    SubnetId: subnetC

Note the subnet names and unique Ids have changed for one of the mount targets.

Which is technically correct, but now CloudFormation will fail with this error:

mount target already exists in this AZ

This is because since the uniqueid of the mount target in subnetC has changed it's trying to create a new one.

Reproduction Steps

Create a CDK construct with an EFS attached to 3 subnets.

const fileSystem = new FileSystem(this, "EfsFileSystem", {
    vpc,
    vpcSubnets: {
        subnets: [subnetA, subnetB, subnetC]
    }
})

now delete subnetB.

Possible Solution

CDK needs to keep track of the subnets and mount target unique IDs somehow

Additional Information/Context

No response

CDK CLI Version

2.67.0

Framework Version

No response

Node.js Version

18.12.1

OS

Windows

Language

Typescript

Language Version

No response

Other information

No response

@kochie kochie added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 13, 2023
@github-actions github-actions bot added the @aws-cdk/aws-efs Related to Amazon Elastic File System label Apr 13, 2023
@kochie kochie changed the title (efs): (efs mount target will fail on subnet change) (aws-efs): EFS mount target will fail on subnet change Apr 13, 2023
@pahud
Copy link
Contributor

pahud commented Apr 13, 2023

Agree. I think we probably should not name the MountTarget ID based on its index here:

'EfsMountTarget' + (++mountTargetCount),

@pahud pahud added p2 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 Apr 13, 2023
@kochie
Copy link
Contributor Author

kochie commented Apr 13, 2023

Perhaps based on the Subnet ID?

@pahud
Copy link
Contributor

pahud commented Jun 14, 2023

Perhaps based on the Subnet ID?

Yes that might be a good idea. Per our discussion on Slack, this might need a new feature flag for the breaking changes but we welcome and appreciate your PR if any.

@mergify mergify bot closed this as completed in #26155 Aug 22, 2023
mergify bot pushed a commit that referenced this issue Aug 22, 2023
Changing Logical IDs for EfsMountTarget

While using the forEach, index will be added as a suffix to logical id of EfsMountTarget 
this is causing an error when the subnets array in the props has changed.

Closes #25099

----

*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-efs Related to Amazon Elastic File System bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
2 participants