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

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

Merged
merged 13 commits into from
Sep 24, 2020

Conversation

sshearn
Copy link
Contributor

@sshearn sshearn commented Mar 31, 2020

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 sshearn requested a review from a team March 31, 2020 21:21
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. documentation Introduces or discusses updates to documentation. service/transfer Issues and PRs that pertain to the transfer service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Mar 31, 2020
@sshearn sshearn changed the title VPC Endpoint Type and User Home Directory Type / Mapping Transfer Server VPC Endpoint Type and User Home Directory Type / Mapping Mar 31, 2020
@sshearn
Copy link
Contributor Author

sshearn commented Apr 7, 2020

@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
Copy link

momer commented May 8, 2020

@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
Copy link

momer commented May 8, 2020

@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
Copy link
Contributor

ewbankkit commented May 18, 2020

@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
Copy link

derekmurawsky commented Jun 8, 2020

@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.
image

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

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

@derekmurawsky
Copy link

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:
image

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

@ewbankkit
Copy link
Contributor

@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
Copy link
Contributor Author

sshearn commented Jun 23, 2020

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

Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

aws/resource_aws_transfer_server.go Outdated Show resolved Hide resolved
aws/resource_aws_transfer_server.go Outdated Show resolved Hide resolved
aws/resource_aws_transfer_server.go Outdated Show resolved Hide resolved
aws/resource_aws_transfer_server.go Outdated Show resolved Hide resolved
aws/resource_aws_transfer_user.go Outdated Show resolved Hide resolved
aws/resource_aws_transfer_user.go Outdated Show resolved Hide resolved
aws/resource_aws_transfer_server_test.go Show resolved Hide resolved
aws/resource_aws_transfer_user_test.go Outdated Show resolved Hide resolved
@dturnbu
Copy link

dturnbu commented Jul 16, 2020

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

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

@bflad
Copy link
Contributor

bflad commented Sep 21, 2020

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.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 21, 2020
@sshearn
Copy link
Contributor Author

sshearn commented Sep 23, 2020

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?

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 23, 2020
@sshearn
Copy link
Contributor Author

sshearn commented Sep 23, 2020

All changes requested are addressed

@bflad
Copy link
Contributor

bflad commented Sep 23, 2020

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
Copy link

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

@sshearn
Copy link
Contributor Author

sshearn commented Sep 23, 2020

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
Copy link
Contributor Author

sshearn commented Sep 23, 2020

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
Copy link
Contributor Author

sshearn commented Sep 23, 2020

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.

@ewbankkit ewbankkit removed their assignment Sep 24, 2020
@bflad bflad added this to the v3.8.0 milestone Sep 24, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@bflad bflad merged commit 6bab16f into hashicorp:master Sep 24, 2020
bflad added a commit that referenced this pull request Sep 24, 2020
@ghost
Copy link

ghost commented Sep 24, 2020

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
Copy link

rowmow commented Sep 30, 2020

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
Copy link

ghost commented Oct 25, 2020

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!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 25, 2020
@DrFaust92 DrFaust92 removed their assignment Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/transfer Issues and PRs that pertain to the transfer service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet