f/aws_fsx_lustre_file_system specify custom KMS Key #15057

nikhil-goenka · 2020-09-08T07:47:32Z

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

Relates OR Closes #14815

Release note for CHANGELOG:

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSFsxLustreFileSystem_KmsKeyId'
--- PASS: TestAccAWSFsxLustreFileSystem_KmsKeyId (1175.23s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       1175.551s
...

DrFaust92 · 2020-09-19T10:51:31Z

Hey @nikhil-goenka , can you rebase?

…m-KMS-Key' into aws_fsx_lustre_file_system-custom-KMS-Key

# Conflicts: # aws/resource_aws_fsx_lustre_file_system.go # website/docs/r/fsx_lustre_file_system.html.markdown

…ferredBackupWindow

nikhil-goenka · 2020-09-21T16:15:43Z

Hey @nikhil-goenka , can you rebase?

Done

DrFaust92

some minor changes

DrFaust92 · 2020-09-21T16:16:59Z

website/docs/r/fsx_lustre_file_system.html.markdown

-* `deployment_type` - (Optional) The filesystem deployment type. One of: `SCRATCH_1`, `SCRATCH_2`, `PERSISTENT_1`.
-* `per_unit_storage_throughput` - (Optional) Describes the amount of read and write throughput for each 1 tebibyte of storage, in MB/s/TiB, required for the `PERSISTENT_1` deployment_type. For valid values, see the [AWS documentation](https://docs.aws.amazon.com/fsx/latest/APIReference/API_CreateFileSystemLustreConfiguration.html).
+* `deployment_type` - (Optional) - The filesystem deployment type. One of: `SCRATCH_1`, `SCRATCH_2`, `PERSISTENT_1`.
+* `kms_key_id` - (Optional) ARN for the KMS Key to encrypt the file system at rest. Defaults to an AWS managed KMS Key, required for the `PERSISTENT_1`.


Its not really required as the default key is used if not passed, let remove the required comment is misleading,

updated the comment.

DrFaust92 · 2020-09-21T16:18:16Z

aws/resource_aws_fsx_lustre_file_system.go

+	}
+
+	if v, ok := d.GetOk("kms_key_id"); ok {
+		if t == fsx.LustreDeploymentTypePersistent1 {


let remove the type check, AFAIK this kind of check isn't usually done in terraform as this should return an error to the user as the actual API would for an illegal combination.

we can add a comment that this is relevant only for TypePersistent1 instead of the required one

DrFaust92 · 2020-09-21T16:19:52Z

also run gofmt to fix lint issue

…ferredBackupWindow

DrFaust92 · 2020-09-21T17:25:47Z

Looks good, @nikhil-goenka, can you add a check for the TypePersistent1 to verify the default value? should be something along the lines of kms/fsx.

nikhil-goenka · 2020-09-21T17:47:46Z

Looks good, @nikhil-goenka, can you add a check for the TypePersistent1 to verify the default value? should be something along the lines of kms/fsx.

Check as in a test case ?

--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (493.11s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 493.414s

DrFaust92 · 2020-09-21T18:09:01Z

yes, add something along the lines of:

resource.TestCheckResourceAttr(resourceName, "kms_key_id", "kms/fsx"),

nikhil-goenka · 2020-09-22T04:31:58Z

yes, add something along the lines of:
resource.TestCheckResourceAttr(resourceName, "kms_key_id", "kms/fsx"),

added
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (535.46s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 535.750s

DrFaust92

LGTM

--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (523.35s)
--- PASS: TestAccAWSFsxLustreFileSystem_KmsKeyId (1170.71s)
--- PASS: TestAccAWSFsxLustreFileSystem_basic (612.21s)

bflad

This looks great overall, @nikhil-goenka, just one non-obvious testing issue, then we can get this in. 👍

bflad · 2020-09-22T12:42:47Z

aws/resource_aws_fsx_lustre_file_system_test.go

+
+data "aws_kms_alias" "fsx" {
+  name = "alias/aws/fsx"
+}


Is this KMS Key guaranteed to exist in AWS accounts that have not provisioned FSx before? Generally, AWS-managed keys are not created until then. It might be best to remove this here and update the test step to just check for a regular expression so there is not a test dependency the first run in a new account.

Suggested change

data "aws_kms_alias" "fsx" {

name = "alias/aws/fsx"

}

that is a valid reason, either we can add regex match or just remove the check, since it has to be kms by default and we don't need to verify it.(how it was before)
let me know your thoughts?

I was able to confirm in a separate AWS account that the FSx KMS Key and its associated Alias is not automatically available. 👍

bflad · 2020-09-22T12:43:01Z

aws/resource_aws_fsx_lustre_file_system_test.go

@@ -432,6 +432,7 @@ func TestAccAWSFsxLustreFileSystem_automaticBackupRetentionDays(t *testing.T) {
 func TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1(t *testing.T) {
 	var filesystem fsx.FileSystem
 	resourceName := "aws_fsx_lustre_file_system.test"
+	datakmsKeyArn := "data.aws_kms_alias.fsx"


Related to below

Suggested change

datakmsKeyArn := "data.aws_kms_alias.fsx"

bflad · 2020-09-22T12:45:17Z

aws/resource_aws_fsx_lustre_file_system_test.go

@@ -446,6 +447,7 @@ func TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "per_unit_storage_throughput", "50"),
 					resource.TestCheckResourceAttr(resourceName, "deployment_type", fsx.LustreDeploymentTypePersistent1),
 					resource.TestCheckResourceAttr(resourceName, "automatic_backup_retention_days", "0"),
+					resource.TestCheckResourceAttrPair(resourceName, "kms_key_id", datakmsKeyArn, "target_key_arn"),


Generally TestCheckResourceAttrPair() is a really good check, however since the data source is not guaranteed to exist the first FSx provisioning in a new account, we should opt for the less fun regular expression:

Suggested change

resource.TestCheckResourceAttrPair(resourceName, "kms_key_id", datakmsKeyArn, "target_key_arn"),

testAccMatchResourceAttrRegionalARN(resourceName, "kms_key_id", "kms", regexp.MustCompile(`key/.+`)),

bflad

Nice work, @nikhil-goenka 🚀

Output from acceptance testing:

--- PASS: TestAccAWSFsxLustreFileSystem_disappears (482.28s)
--- PASS: TestAccAWSFsxLustreFileSystem_Tags (508.59s)
--- PASS: TestAccAWSFsxLustreFileSystem_automaticBackupRetentionDays (551.98s)
--- PASS: TestAccAWSFsxLustreFileSystem_WeeklyMaintenanceStartTime (571.03s)
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypeScratch2 (575.30s)
--- PASS: TestAccAWSFsxLustreFileSystem_basic (593.79s)
--- PASS: TestAccAWSFsxLustreFileSystem_DeploymentTypePersistent1 (617.47s)
--- PASS: TestAccAWSFsxLustreFileSystem_KmsKeyId (914.80s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageCapacity (1056.09s)
--- PASS: TestAccAWSFsxLustreFileSystem_SecurityGroupIds (1181.96s)
--- PASS: TestAccAWSFsxLustreFileSystem_ImportedFileChunkSize (1219.00s)
--- PASS: TestAccAWSFsxLustreFileSystem_ImportPath (1347.76s)
--- PASS: TestAccAWSFsxLustreFileSystem_ExportPath (1609.11s)

ghost · 2020-09-24T22:14:28Z

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!

ghost · 2020-10-23T17:10:25Z

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!

aws_fsx_lustre_file_system specify custom KMS Key

1cf3995

nikhil-goenka requested a review from a team September 8, 2020 07:47

aws_fsx_lustre_file_system specify custom KMS Key

1138cb1

ghost added the documentation Introduces or discusses updates to documentation. label Sep 8, 2020

aws_fsx_lustre_file_system specify custom KMS Key

9f6f591

nikhil-goenka changed the title ~~aws_fsx_lustre_file_system specify custom KMS Key~~ f/aws_fsx_lustre_file_system specify custom KMS Key Sep 12, 2020

nikhil-goenka marked this pull request as draft September 16, 2020 15:24

nikhil-goenka marked this pull request as ready for review September 18, 2020 17:35

nikhil-goenka added 5 commits September 20, 2020 20:59

Merge branch 'master' into aws_fsx_lustre_file_system-custom-KMS-Key

62af651

aws_fsx_lustre_file_system specify custom KMS Key

bc0219f

aws_fsx_lustre_file_system specify custom KMS Key

4875d2f

aws_fsx_lustre_file_system specify custom KMS Key

55a1184

Merge remote-tracking branch 'origin/aws_fsx_lustre_file_system-custo…

d5889b1

…m-KMS-Key' into aws_fsx_lustre_file_system-custom-KMS-Key

nikhil-goenka closed this Sep 20, 2020

nikhil-goenka reopened this Sep 20, 2020

nikhil-goenka added 7 commits September 21, 2020 13:11

aws_fsx_lustre_file_system specify custom KMS Key

8b564f7

aws_fsx_lustre_file_system specify custom KMS Key

919a1d5

aws_fsx_lustre_file_system specify custom KMS Key

1a0f5dd

Merge remote-tracking branch 'origin/aws_fsx_lustre_file_system-custo…

9305b8b

…m-KMS-Key' into aws_fsx_lustre_file_system-custom-KMS-Key

Merge branch 'master' into aws_fsx_lustre_file_system-custom-KMS-Key

36c9331

# Conflicts: # aws/resource_aws_fsx_lustre_file_system.go # website/docs/r/fsx_lustre_file_system.html.markdown

tech-debt/aws_rds_cluster:TestAccAWSRDSCluster_SnapshotIdentifier_Pre…

9eab2ac

…ferredBackupWindow

tech-debt/aws_rds_cluster:TestAccAWSRDSCluster_SnapshotIdentifier_Pre…

e372705

…ferredBackupWindow

DrFaust92 suggested changes Sep 21, 2020

View reviewed changes

tech-debt/aws_rds_cluster:TestAccAWSRDSCluster_SnapshotIdentifier_Pre…

2301bd0

…ferredBackupWindow

f/aws_fsx_lustre_file_system specify custom KMS Key

ca7d23a

DrFaust92 approved these changes Sep 22, 2020

View reviewed changes

bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 22, 2020

bflad reviewed Sep 22, 2020

View reviewed changes

f/aws_fsx_lustre_file_system specify custom KMS Key

07ac294

bflad added this to the v3.8.0 milestone Sep 22, 2020

bflad approved these changes Sep 22, 2020

View reviewed changes

bflad merged commit 60db9d7 into hashicorp:master Sep 22, 2020

bflad added a commit that referenced this pull request Sep 22, 2020

Update CHANGELOG for #15057

fca055d

nikhil-goenka deleted the aws_fsx_lustre_file_system-custom-KMS-Key branch September 22, 2020 17:25

ghost locked as resolved and limited conversation to collaborators Oct 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

f/aws_fsx_lustre_file_system specify custom KMS Key #15057

f/aws_fsx_lustre_file_system specify custom KMS Key #15057

nikhil-goenka commented Sep 8, 2020

DrFaust92 commented Sep 19, 2020 •

edited

Loading

nikhil-goenka commented Sep 21, 2020

DrFaust92 left a comment

DrFaust92 Sep 21, 2020

nikhil-goenka Sep 21, 2020

DrFaust92 Sep 21, 2020

DrFaust92 Sep 21, 2020

nikhil-goenka Sep 21, 2020

DrFaust92 commented Sep 21, 2020

DrFaust92 commented Sep 21, 2020

nikhil-goenka commented Sep 21, 2020

DrFaust92 commented Sep 21, 2020

nikhil-goenka commented Sep 22, 2020

DrFaust92 left a comment •

edited

Loading

bflad left a comment

bflad Sep 22, 2020

nikhil-goenka Sep 22, 2020 •

edited

Loading

bflad Sep 22, 2020

nikhil-goenka Sep 22, 2020

bflad Sep 22, 2020

bflad Sep 22, 2020

nikhil-goenka Sep 22, 2020

bflad left a comment

ghost commented Sep 24, 2020

ghost commented Oct 23, 2020

	resource.TestCheckResourceAttrPair(resourceName, "kms_key_id", datakmsKeyArn, "target_key_arn"),
	testAccMatchResourceAttrRegionalARN(resourceName, "kms_key_id", "kms", regexp.MustCompile(`key/.+`)),

f/aws_fsx_lustre_file_system specify custom KMS Key #15057

f/aws_fsx_lustre_file_system specify custom KMS Key #15057

Conversation

nikhil-goenka commented Sep 8, 2020

Community Note

DrFaust92 commented Sep 19, 2020 • edited Loading

nikhil-goenka commented Sep 21, 2020

DrFaust92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrFaust92 commented Sep 21, 2020

DrFaust92 commented Sep 21, 2020

nikhil-goenka commented Sep 21, 2020

DrFaust92 commented Sep 21, 2020

nikhil-goenka commented Sep 22, 2020

DrFaust92 left a comment • edited Loading

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhil-goenka Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

ghost commented Sep 24, 2020

ghost commented Oct 23, 2020

DrFaust92 commented Sep 19, 2020 •

edited

Loading

DrFaust92 left a comment •

edited

Loading

nikhil-goenka Sep 22, 2020 •

edited

Loading