r/aws_fsx_file_system: Adding support and resource for managing FSx filesystems

[slapula]

Fixes #7035

Changes proposed in this pull request:

• Adding changes to allow for the provider to support FSx API actions
• Adding resource to manage FSx filesystems: aws_fsx_file_system

Output from acceptance testing:

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

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

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

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

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

[tkidd77]

Any update on getting this merged?

[slapula]

@tkidd77 still waiting for review from the maintainers unfortunately. They are probably pretty busy getting ready for Terraform 0.12 at the moment so it might be awhile.

[GavinCS]

Would love to use this.
Any news on when it might be merged?

[d-mcd]

We have use case for this as well. Any updates?

[slapula]

@bflad sorry to pull you in again but it looks like there is becoming a greater need for this resource? Would it be possible to get a review in the near future?

[mrcrilly]

Would love this to be merged in ASAP. I have a need for us in Government for this.

[rba1-source]

The code written so far can be used for an AWS Managed AD. AWS announced last week that you can also integrate directly with your on-prem AD, so this will need updating in the future to cater for this.

https://aws.amazon.com/about-aws/whats-new/2019/06/amazon-fsx-for-windows-file-server-now-enables-you-to-use-file-systems-directly-with-your-organizations-self-managed-active-directory/

The new config that is needed is the SelfManagedActiveDirectoryConfiguration section:
https://docs.aws.amazon.com/fsx/latest/APIReference/API_CreateFileSystem.html

Additionally, terraform should wait until the Lifecycle value is "Available" after doing the create, because it's only at this point that a describe-file-system will return the "DNSName" field which is the output you need to be able to connect to and use the FSx resource, and will likely pass into the next part of your provisioning pipeline.

[onur-sam-gtn-ai]

I can't see the export settings for Lustre?

Export prefix Info
The prefix to which data is exported
- A unique prefix that FSx creates in your bucket
- The same prefix that you imported from (replace existing objects with updated ones)
- A prefix you specify

[slapula]

🐻 with me, I have some free cycles to make another pass at this. I'll look into adding support for self managed active directory and double check the Lustre code.

[onur-sam-gtn-ai]

Another point: I can't see where KMS is used for Lustre? It seems like it's a mandatory parameter for both FSx types but I could find no reference to KMS for Lustre in the console?
Also, this page says it's not required (which might explain how it's not used for Lustre):
https://docs.aws.amazon.com/fsx/latest/APIReference/API_CreateFileSystem.html#FSx-CreateFileSystem-request-KmsKeyId

[mrcrilly]

@slapula - good man. Do you have a "Buy me a coffee" tipping thing setup anywhere?

[slapula]

@mrcrilly Yup! Look for my github username on buymeacoff.ee. Getting this PR updated is still on my radar and I should have an update this weekend if not earlier.

[mrcrilly]

@mrcrilly Yup! Look for my github username on buymeacoff.ee. Getting this PR updated is still on my radar and I should have an update this weekend if not earlier.

@slapula enjoy your coffee!

[onur-sam-gtn-ai]

I've acknowledged the two points I have raised above in resource_aws_fsx_file_system.go myself. I've never written Golang code before so excuse my copypasta work; but it seems to work beautifully. I'm sharing the diff below.

diff --git a/aws/resource_aws_fsx_file_system.go b/aws/resource_aws_fsx_file_system.go
index 91413b709..a2340e82f 100644
--- a/aws/resource_aws_fsx_file_system.go
+++ b/aws/resource_aws_fsx_file_system.go
@@ -39,7 +39,7 @@ func resourceAwsFsxFileSystem() *schema.Resource {
                        },
                        "kms_key_id": {
                                Type:     schema.TypeString,
-                               Required: true,
+                               Optional: true,
                                ForceNew: true,
                        },
                        "subnet_ids": {
@@ -61,7 +61,11 @@ func resourceAwsFsxFileSystem() *schema.Resource {
                                        Schema: map[string]*schema.Schema{
                                                "import_path": {
                                                        Type:     schema.TypeString,
-                                                       Required: true,
+                                                       Optional: true,
+                                               },
+                                               "export_path": {
+                                                       Type:     schema.TypeString,
+                                                       Optional: true,
                                                },
                                                "chunk_size": {
                                                        Type:     schema.TypeInt,
@@ -134,7 +138,10 @@ func resourceAwsFsxFileSystemCreate(d *schema.ResourceData, meta interface{}) er
                FileSystemType:  aws.String(d.Get("type").(string)),
                StorageCapacity: aws.Int64(int64(d.Get("capacity").(int))),
                SubnetIds:       expandStringList(d.Get("subnet_ids").([]interface{})),
-               KmsKeyId:        aws.String(d.Get("kms_key_id").(string)),
+       }
+
+       if value, ok := d.GetOk("kms_key_id"); ok {
+               request.KmsKeyId = aws.String(value.(string))
        }
 
        if value, ok := d.GetOk("security_group_ids"); ok {
@@ -345,6 +352,9 @@ func expandFsxLustreConfigurationCreate(l []interface{}) *fsx.CreateFileSystemLu
        if data["import_path"].(string) != "" {
                req.ImportPath = aws.String(data["import_path"].(string))
        }
+       if data["export_path"].(string) != "" {
+               req.ExportPath = aws.String(data["export_path"].(string))
+       }
  
        if data["chunk_size"] != nil {
                req.ImportedFileChunkSize = aws.Int64(int64(data["chunk_size"].(int)))
@@ -423,6 +433,7 @@ func flattenLustreOptsConfig(lopts *fsx.LustreFileSystemConfiguration) []map[str
 
        m := map[string]interface{}{
                "import_path":                   aws.StringValue(lopts.DataRepositoryConfiguration.ImportPath),
+               "export_path":                   aws.StringValue(lopts.DataRepositoryConfiguration.ExportPath),
                "chunk_size":                    aws.Int64Value(lopts.DataRepositoryConfiguration.ImportedFileChunkSize),
                "weekly_maintenance_start_time": aws.StringValue(lopts.WeeklyMaintenanceStartTime),
        }

[onur-sam-gtn-ai]

Another big dealbreaker: Timeout doesn't seem to be configurable. I tried creating an FSx Lustre with a big S3 import and it took a few hours. Terraform indeed timed out after 60 minutes and now the state file is not updated, causing Terraform to try to recreate the resource. Timeout should be configurable or at least much longer than 60 minutes for FSx.

[onur-sam-gtn-ai]

After doing an import, I see that Terraform also completely missed out on the lustre_configuration and the security_group_ids, they're both null in the state file.

[slapula]

@onur-sam-gtn-ai Thanks for the review! I'm working through these issues at the moment and should have them dealt with soon.

I haven't testing it yet but your export_path changes look good. If you are not seeing security_group_ids in state that is because I forgot to add them! They are only referenced at creation time and do not show up when updating or describing the file system. I will likely just have to reference what is being fed into Terraform by the user until Amazon updates the API. I'm not quite sure what AWS is doing there but it seems like an oversight to me.

[bflad]

Hi @slapula 👋 Thank you for submitting this. I have provided an initial review below. Please reach out if you have any questions or do not have time to implement the feedback.

[bflad]

This attribute is missing acceptance testing.

[slapula]

It would probably help if I finished implementing the attribute 😅

[slapula]

In this case, security_group_ids is only ever used during Create actions. It's not even tracked when you describe a filesystem. Do you guys have an official way of handling these kinds of attributes? This seems like an oversight on Amazon's part.

[slapula]

@bflad I believe I've addressed pretty much all of your items:

$ make testacc TESTARGS='-run=TestAccAWSFsxFileSystem_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSFsxFileSystem_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSFsxFileSystem_lustreBasic
=== PAUSE TestAccAWSFsxFileSystem_lustreBasic
=== RUN   TestAccAWSFsxFileSystem_lustreConfig
=== PAUSE TestAccAWSFsxFileSystem_lustreConfig
=== RUN   TestAccAWSFsxFileSystem_lustreUpdate
=== PAUSE TestAccAWSFsxFileSystem_lustreUpdate
=== RUN   TestAccAWSFsxFileSystem_windowsConfig
=== PAUSE TestAccAWSFsxFileSystem_windowsConfig
=== RUN   TestAccAWSFsxFileSystem_windowsUpdate
=== PAUSE TestAccAWSFsxFileSystem_windowsUpdate
=== CONT  TestAccAWSFsxFileSystem_lustreBasic
=== CONT  TestAccAWSFsxFileSystem_windowsConfig
=== CONT  TestAccAWSFsxFileSystem_windowsUpdate
=== CONT  TestAccAWSFsxFileSystem_lustreUpdate
=== CONT  TestAccAWSFsxFileSystem_lustreConfig
--- PASS: TestAccAWSFsxFileSystem_lustreBasic (602.78s)
--- PASS: TestAccAWSFsxFileSystem_lustreUpdate (812.14s)
--- PASS: TestAccAWSFsxFileSystem_lustreConfig (1051.66s)
--- PASS: TestAccAWSFsxFileSystem_windowsConfig (2864.00s)
--- PASS: TestAccAWSFsxFileSystem_windowsUpdate (2920.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2920.840s

[slapula]

@bflad I'm aware that I still need to add unit tests for FSx tags. That's next on my agenda. I am also not sure on how to approach acceptance tests for the self_managed_active_directory configuration block for Window File Servers. This block assumes a non-Directory Service AD server up and running somewhere.

[slapula]

@bflad I just added some basic unit tests for FSx tags. The only item I believe is outstanding now is testing self_managed_active_directory. I'm not a Windows guy so standing up an AD server is a bit outside my wheelhouse. If anyone wants to give it a shot it would be much appreciated.

[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!