Add transfer for sftp service support only server #6639

teraken0509 · 2018-11-29T10:32:25Z

Reference #6584

Changes proposed in this pull request:

Add resource aws_transfer_server for create AWS Transfer for SFTP Service's server.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSTransferServer_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSTransferServer_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSTransferServer_basic
=== PAUSE TestAccAWSTransferServer_basic
=== CONT  TestAccAWSTransferServer_basic
--- PASS: TestAccAWSTransferServer_basic (55.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	55.107s
...

TODO

Add Tests
Add Documentation file

bflad

This is off to a great start, @kterada0509! Thanks so much for the contribution. Please see the initial comments below and let us know if you have any questions or do not have time to implement the feedback.

aws/resource_aws_transfer_server.go

aws/resource_aws_transfer_server_test.go

bflad · 2018-12-02T03:09:57Z

aws/tagsTransferServer.go

+	// First, we're creating everything we have
+	create := make(map[string]interface{})
+	for _, t := range newTags {
+		create[*t.Key] = *t.Value


To prevent potential panics, we should use the AWS Go SDK provided functions for dereferencing here, e.g.

Suggested change

create[*t.Key] = *t.Value

create[aws.StringValue(t.Key)] = aws.StringValue(t.Value)

Same with the * dereferences below. 👍

aws/tagsTransferServer_test.go

website/aws.erb

website/docs/r/transfer_server.html.markdown

aws/resource_aws_transfer_server.go

aws/resource_aws_transfer_server_test.go

teraken0509 · 2018-12-03T01:35:35Z

@bflad
Thank you very much for review!
I will fix in this week.

teraken0509 · 2018-12-10T03:03:04Z

Test Result

$ make testacc TESTARGS='-run=TestAccAWSTransferServer_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSTransferServer_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSTransferServer_basic
=== PAUSE TestAccAWSTransferServer_basic
=== RUN   TestAccAWSTransferServer_apigateway
=== PAUSE TestAccAWSTransferServer_apigateway
=== RUN   TestAccAWSTransferServer_disappears
=== PAUSE TestAccAWSTransferServer_disappears
=== CONT  TestAccAWSTransferServer_basic
=== CONT  TestAccAWSTransferServer_disappears
=== CONT  TestAccAWSTransferServer_apigateway
--- PASS: TestAccAWSTransferServer_disappears (16.76s)
--- PASS: TestAccAWSTransferServer_apigateway (32.40s)
--- PASS: TestAccAWSTransferServer_basic (43.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	43.290s

bflad

Let's get this in so folks can get started with this today. Thanks, @kterada0509! 🚀

--- PASS: TestAccAWSTransferServer_disappears (8.49s)
--- PASS: TestAccAWSTransferServer_apigateway (21.58s)
--- PASS: TestAccAWSTransferServer_basic (22.16s)

bflad · 2018-12-13T21:40:17Z

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

samandmoore · 2018-12-15T01:11:49Z

website/docs/r/transfer_server.html.markdown

+* `url` - (Optional) - URL of the service endpoint used to authenticate users with an `identity_provider_type` of `API_GATEWAY`.
+* `identity_provider_type` - (Optional) The mode of authentication enabled for this service. The default value is `SERVICE_MANAGED`, which allows you to store and access SFTP user credentials within the service. `API_GATEWAY` indicates that user authentication requires a call to an API Gateway endpoint URL provided by you to integrate an identity provider of your choice.
+* `logging_role` - (Optional) Amazon Resource Name (ARN) of an IAM role that allows the service to write your SFTP users’ activity to your Amazon CloudWatch logs for monitoring and auditing purposes.
+* `tags` - (Optional) A mapping of tags to assign to the resource.


these docs say that the tags are optional, but applying a resource without tags doesn't work:

Error: Error applying plan: 1 error(s) occurred: * aws_transfer_server.sftp_server: 1 error(s) occurred: * aws_transfer_server.sftp_server: Error creating Transfer Server: InvalidParameter: 1 validation error(s) found. - minimum field size of 1, CreateServerInput.Tags.

i'm happy to open a PR to fix, but i guess i wanna verify that these are meant to be optional. i know that the AWS API doesn't require the tags, but i don't know if terraform wants them to be required?

samandmoore · 2018-12-15T01:16:07Z

aws/resource_aws_transfer_server.go

+	tags := tagsFromMapTransferServer(d.Get("tags").(map[string]interface{}))
+
+	createOpts := &transfer.CreateServerInput{
+		Tags: tags,


i think that this is the line that's causing the tags to be "required" when they're supposed to be optional. if we provide the Tags attribute in the CreateServerInput call, they cannot be empty.

dbuzolin-nfl · 2018-12-21T00:02:37Z

Is there way to add hostname property to transfer service, and/or also integrate it with Route53? Current implementation creates service with empty hostname: "-"
#6956

samandmoore · 2018-12-21T00:26:09Z

I think you'd want to use the route53 resources. All the UI does is make the route53 records for you.

…

On Thu, Dec 20, 2018, 19:02 dbuzolin-nfl ***@***.*** wrote: Is there way to add hostname property to transfer service, and/or also integrate it with Route53? Current implementation creates service with empty hostname: "-" — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6639 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4IvBNGBwBcjtC_rQlQ0z5Lvx80CCjks5u7CUkgaJpZM4Y5YQh> .

dbuzolin-nfl · 2018-12-21T20:04:50Z

That is correct, but the issue is different. AWS assigns this link via two tags:

aws:transfer:route53HostedZoneId | /hostedzone/ABCDEFGHIGFK
aws:transfer:customHostname | mysftphost.myco.com

and there is now way to create those in terraform, they are simply ignored. Because of this current implementation is not really usable.

ghost · 2020-03-05T14:51:52Z

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 added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. labels Nov 29, 2018

teraken0509 force-pushed the feature/add-support-transfer-for-sftp-service branch 6 times, most recently from 0e592b5 to 3e4d0f1 Compare November 30, 2018 09:06

ghost added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 30, 2018

teraken0509 changed the title ~~[WIP] Add transfer for sftp service support~~ Add transfer for sftp service support Nov 30, 2018

teraken0509 changed the title ~~Add transfer for sftp service support~~ Add transfer for sftp service support only server Dec 1, 2018

bflad added new-resource Introduces a new resource. service/transfer Issues and PRs that pertain to the transfer service. labels Dec 2, 2018

bflad reviewed Dec 2, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 2, 2018

bflad reviewed Dec 2, 2018

View reviewed changes

aws/resource_aws_transfer_server_test.go Show resolved Hide resolved

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 3, 2018

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 3, 2018

teraken0509 added 8 commits December 10, 2018 11:04

Add transfer for sftp service support

7d98fb2

Add Transfer Server Test

b81675e

Add aws_transfer_server resource document

e2ba554

Fix Error messages

a5d79f4

fix to follow coding policy

e68975f

Fix using aws.StringValue

8b05c8c

Using aws_iam_policy/aws_iam_policy_attachment -> aws_iam_role_policy

f40e29a

Fixed Copy-pasta

714c4b7

teraken0509 force-pushed the feature/add-support-transfer-for-sftp-service branch from 3e4d0f1 to 98927a3 Compare December 10, 2018 03:02

teraken0509 added 12 commits December 12, 2018 09:45

Remove Extraneous logging

7637195

Remove unnecessary step

7240ff0

Fix err return

406cde3

fix to use aws_iam_role_policy

034d1bc

Fix Argument Reference document

e6467a4

Add example of id & endpoint

98d93d5

Remove unused package

c860728

Fix log format

54e9c07

Fix test check

0aa61cf

fix document

becfafb

Add wait deletion function

23336de

Fix to use waitForTransferServerDeletion function

fbb6571

ojongerius mentioned this pull request Dec 12, 2018

Feature request: AWS Transfer for SFTP service #6584

Closed

fix err check

807b6fa

bflad added this to the v1.52.0 milestone Dec 13, 2018

bflad approved these changes Dec 13, 2018

View reviewed changes

bflad merged commit c9ccde0 into hashicorp:master Dec 13, 2018

bflad added a commit that referenced this pull request Dec 13, 2018

Update CHANGELOG for #6639

09792d7

samandmoore reviewed Dec 15, 2018

View reviewed changes

samandmoore mentioned this pull request Dec 17, 2018

Make tags truly optional on the transfer sftp server resource #6883

Merged

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 21, 2018

bflad mentioned this pull request Dec 21, 2018

Add hostname property for aws_transfer_server #6956

Closed

teraken0509 deleted the feature/add-support-transfer-for-sftp-service branch March 5, 2020 14:05

ghost locked and limited conversation to collaborators Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transfer for sftp service support only server #6639

Add transfer for sftp service support only server #6639

teraken0509 commented Nov 29, 2018 •

edited by bflad

Loading

bflad left a comment

bflad Dec 2, 2018

teraken0509 commented Dec 3, 2018

teraken0509 commented Dec 10, 2018

bflad left a comment

bflad commented Dec 13, 2018

samandmoore Dec 15, 2018

samandmoore Dec 15, 2018

dbuzolin-nfl commented Dec 21, 2018 •

edited

Loading

samandmoore commented Dec 21, 2018 via email

dbuzolin-nfl commented Dec 21, 2018

ghost commented Mar 5, 2020

	create[t.Key] = t.Value
	create[aws.StringValue(t.Key)] = aws.StringValue(t.Value)

Add transfer for sftp service support only server #6639

Add transfer for sftp service support only server #6639

Conversation

teraken0509 commented Nov 29, 2018 • edited by bflad Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Dec 2, 2018

Choose a reason for hiding this comment

teraken0509 commented Dec 3, 2018

teraken0509 commented Dec 10, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Dec 13, 2018

samandmoore Dec 15, 2018

Choose a reason for hiding this comment

samandmoore Dec 15, 2018

Choose a reason for hiding this comment

dbuzolin-nfl commented Dec 21, 2018 • edited Loading

samandmoore commented Dec 21, 2018 via email

dbuzolin-nfl commented Dec 21, 2018

ghost commented Mar 5, 2020

teraken0509 commented Nov 29, 2018 •

edited by bflad

Loading

dbuzolin-nfl commented Dec 21, 2018 •

edited

Loading