Transfer Server VPC Endpoint Type and User Home Directory Type / Mapping

[sshearn]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #11281
Closes #11632
Closes #11724
Closes #11569
Closes #11593

Release note for CHANGELOG:

Transfer server now supports VPC for endpoint_type.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransferServer_Vpc'

...

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransferUser_restricted'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSTransferUser_restricted -timeout 120m
=== RUN   TestAccAWSTransferUser_restricted
=== PAUSE TestAccAWSTransferUser_restricted
=== CONT  TestAccAWSTransferUser_restricted
--- PASS: TestAccAWSTransferUser_restricted (40.44s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       41.599s

Note:

TestAccAWSTransferServer_Vpc test failed because it couldn't release the eip's on destroy due to policies on my account. But the actual test works.

[sshearn]

@bflad I'm using my fork of this in prod. So I know it works great. I'm hoping this can get into the next release so I don't have to maintain my fork for long though. Let me know if I can help with anything!

[momer]

@sshearn at a quick glance, this looks great. Watching this PR as well! Thank you for this.

I should add, that I'll be using this today - will respond with any issues

[momer]

@sshearn This is great - I played around quite a bit, here are a couple of pieces of feedback:

1. I'd add a note in the docs about home_directory not being able to be set if home_directory_mappings is set.
2. For a user with home_directory_mappings, if a change-set is applied which indicates a change of the Target will occur, and is then applied, it will succeed, but no change will actually occur (i.e. the mapping does not actually change).

All in all, this was a joy to use. Thank you for this contribution!

[ewbankkit]

@sshearn Thanks for this.
We usually prefer not to manipulate AWS resources from differing AWS services in a single Terraform resource and I notice that the vpce_security_group_ids attribute manipulates an EC2 VPC Endpoint resource.
Is there a way of managing these security groups through a aws_vpc_endpoint resource?
I remember this comment about why there is no VPC Endpoint/Security Group Association resource.

[derekmurawsky]

@sshearn Thanks for this.
We usually prefer not to manipulate AWS resources from differing AWS services in a single Terraform resource and I notice that the vpce_security_group_ids attribute manipulates an EC2 VPC Endpoint resource.
Is there a way of managing these security groups through a aws_vpc_endpoint resource?
I remember this comment about why there is no VPC Endpoint/Security Group Association resource.

@ewbankkit
The transfer server brings up its own VPC endpoint that cannot be brought up or configured via the normal endpoint service.
I manually brought up the transfer server and this is the endpoint that was created.

When creating an endpoint via the normal method, there is no way to create an endpoint of this type.

As such, I would suggest the proper way to manage this service-created endpoint would be through the transfer-server module.

[derekmurawsky]

As such, I would suggest the proper way to manage this service-created endpoint would be through the transfer-server module.

I did some more digging today into this. I created a transfer server and then attempted to use the following code to attach the security groups per @ewbankkit 's suggestion.

data "aws_transfer_server" "transfer-server" {
  server_id = aws_cloudformation_stack.transfer-server.outputs["ServerId"]
}

data "aws_vpc_endpoint" "transfer-server" {
  vpc_id = aws_vpc.external.id
}

resource "aws_network_interface_sg_attachment" "transfer-server-endpoint" {
  for_each             = data.aws_vpc_endpoint.transfer-server.network_interface_ids
  security_group_id    = aws_security_group.allow_sftp_all.id
  network_interface_id = each.value
}

It failed with the following

Error: AuthFailure: You do not have permission to access the specified resource.
        status code: 400, request id: 4be36bc7-ffff-4e11-af1a-977a52646147

  on transfer-server-cf.tf line 20, in resource "aws_network_interface_sg_attachment" "transfer-server-endpoint":
  20: resource "aws_network_interface_sg_attachment" "transfer-server-endpoint" {

This error makes sense; these interfaces are managed through the endpoint service and, as such, can't be managed directly. The web UI also has the same error:

There doesn't appear to be a way to manage this without something like an aws_vpc_endpoint_sg_attachement resource.

[ewbankkit]

@derekmurawsky Thanks for looking into this. We are considering creating an aws_vpc_endpoint_security_group_association resource to handle this functionality. I'll keep this issue update with the status.

[sshearn]

@derekmurawsky Thanks for looking into this. We are considering creating an aws_vpc_endpoint_security_group_association resource to handle this functionality. I'll keep this issue update with the status.

I can update this PR to account for this once it's merged in.

[DrFaust92]

some minor changes, ill try to dive a bit deeper a bit later

[dturnbu]

Is anyone planning on making the changes suggested by @DrFaust92?

I'd really like to see this PR merged soon....

[bflad]

Hi @sshearn 👋 Thank you for your work here. Are you still interested in working on this contribution? There are some review items noted above that prevent merging this and we separately just merged aws_transfer_user resource changes that preceded this contribution, so these should be removed from this pull request to remove the merge conflicts. Please reach out if you have any questions.

If we do not hear back in two weeks, we will default to providing review of #15105, which includes these commits and some of these review item fixes.

[sshearn]

Hi @sshearn 👋 Thank you for your work here. Are you still interested in working on this contribution? There are some review items noted above that prevent merging this and we separately just merged aws_transfer_user resource changes that preceded this contribution, so these should be removed from this pull request to remove the merge conflicts. Please reach out if you have any questions.

If we do not hear back in two weeks, we will default to providing review of #15105, which includes these commits and some of these review item fixes.

I basically abandoned this for my own provider that I've been using in production for months. I guess I could pick this back up and making the new required changes. I'll start working on it.

EDIT: Also the PR for aws_vpc_endpoint_security_group_association #13737 hasn't been merged yet. Wasn't this deemed required for this PR to be completed?

[sshearn]

All changes requested are addressed

[bflad]

Thank you for your work on this, @sshearn. I think this is now in a good enough state where we can discuss the overall approach here to prepare to get this functionality merged with a few relevant changes. 👍 Please reach out if you have any questions or if you would prefer to not spend additional time on this (no big deal either which way, we'll make sure you get credit).

1. Terraform AWS Provider resources should avoid cross-service functionality (e.g. no service/ec2 handling in aws_transfer_server resource) as this type of handling has created many issues in the past and can introduce confusion about IAM permissions necessary for the resource. Our preference will be to create some form of Terraform AWS Provider functionality, after this pull request and separate from the aws_transfer_server resource, that manages the EC2 VPC Endpoint that is automatically created by the Transfer service. To that end and especially because it is not part of the Transfer API, let's remove all the vpce_security_group_ids and associated logic.
2. Switch the vpc_endpoint_id attribute from DiffSuppressFunc to Computed: true -- this will allow Terraform core to properly know that the value is unknown for downstream references when unconfigured.
3. Include a quick comment about why AddressAllocationIds needs to be handled via update after creation (e.g. reference https://docs.aws.amazon.com/transfer/latest/userguide/API_EndpointDetails.html)

Thanks again.

[andwoods]

@sshearn Thank you for your effort on this functionality! It is making a sizable impact for a lot of people!

[sshearn]

Thank you for your work on this, @sshearn. I think this is now in a good enough state where we can discuss the overall approach here to prepare to get this functionality merged with a few relevant changes. 👍 Please reach out if you have any questions or if you would prefer to not spend additional time on this (no big deal either which way, we'll make sure you get credit).

1. Terraform AWS Provider resources should avoid cross-service functionality (e.g. no `service/ec2` handling in `aws_transfer_server` resource) as this type of handling has created many issues in the past and can introduce confusion about IAM permissions necessary for the resource. Our preference will be to create some form of Terraform AWS Provider functionality, after this pull request and separate from the `aws_transfer_server` resource, that manages the EC2 VPC Endpoint that is automatically created by the Transfer service. To that end and especially because it is not part of the Transfer API, let's remove all the `vpce_security_group_ids` and associated logic.

2. Switch the `vpc_endpoint_id` attribute from `DiffSuppressFunc` to `Computed: true` -- this will allow Terraform core to properly know that the value is unknown for downstream references when unconfigured.

3. Include a quick comment about why `AddressAllocationIds` needs to be handled via update after creation (e.g. reference https://docs.aws.amazon.com/transfer/latest/userguide/API_EndpointDetails.html)

Thanks again.

1. As I said before, this is addressed in New resource aws_vpc_endpoint_security_group_association #13737 which isn't merged yet. I will remove service/ec2 references but that means that New resource aws_vpc_endpoint_security_group_association #13737 is a dependency for this to be valuable.
2. Will do.
3. The api only supported AddressAllocationIds on UpdateServer. I can check if that's changed since I originally created this PR, and if its now supported on create, I can refactor that. If not, then ec2 is required to check that the endpoint is up before stopping the transfer server in order to update it, and then start it again. It's referenced here: https://docs.aws.amazon.com/sdk-for-go/api/service/transfer/#EndpointDetails

[sshearn]

I tested via the cli:

% aws transfer create-server --endpoint-details '{"AddressAllocationIds":["eipalloc-XXX"],"SubnetIds":["subnet-XXX"],"VpcId":"vpc-XXX"}' --endpoint-type VPC

An error occurred (InvalidRequestException) when calling the CreateServer operation: AddressAllocationIds cannot be set in CreateServer

[sshearn]

I've removed vpce_security_group_ids, but had to leave in the support for waiting on the creation of the VPC endpoint in order to stop and then update the AddressAllocationIds.

[bflad]

Alrighty! After diving into this more, was able to fully remove the EC2 dependency by introducing retry logic for the specific error on update after creation: ConflictException: VPC Endpoint state is not yet available -- this allows us to not require unexpected IAM permissions for this resource and prevents any other potential cross-service eventual consistency that might occur. It was certainly interesting that the Transfer Server state of ONLINE didn't wait for that.

After fixing up the test configuration to remove the non-existent data source reference and fixing the expandTransferServerEndpointDetails function so VPC_ENDPOINT support continued to work, looks good, thank you @sshearn 🚀

Output from acceptance testing:

--- PASS: TestAccAWSTransferServer_apigateway (190.91s)
--- PASS: TestAccAWSTransferServer_basic (196.10s)
--- PASS: TestAccAWSTransferServer_disappears (180.81s)
--- PASS: TestAccAWSTransferServer_forcedestroy (155.75s)
--- PASS: TestAccAWSTransferServer_hostKey (155.02s)
--- PASS: TestAccAWSTransferServer_Vpc (288.89s)
--- PASS: TestAccAWSTransferServer_vpcEndpointId (143.93s)

[ghost]

This has been released in version 3.8.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

[rowmow]

Thanks so much @sshearn for these additions. Is there a way to also manage a "custom hostname" within the endpoint_details additions? I did not see this mentioned in the latest documentation.

https://docs.aws.amazon.com/transfer/latest/userguide/requirements-dns.html

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!