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

f/aws_fsx_lustre_file_system specify custom KMS Key #15057

Merged

Conversation

nikhil-goenka
Copy link
Contributor

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

@nikhil-goenka nikhil-goenka requested a review from a team September 8, 2020 07:47
@ghost ghost added size/M Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/fsx Issues and PRs that pertain to the fsx service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Sep 8, 2020
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Sep 8, 2020
@nikhil-goenka 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 nikhil-goenka marked this pull request as draft September 16, 2020 15:24
@nikhil-goenka nikhil-goenka marked this pull request as ready for review September 18, 2020 17:35
@DrFaust92
Copy link
Collaborator

DrFaust92 commented Sep 19, 2020

Hey @nikhil-goenka , can you rebase?

@nikhil-goenka
Copy link
Contributor Author

Hey @nikhil-goenka , can you rebase?

Done

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

* `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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the comment.

}

if v, ok := d.GetOk("kms_key_id"); ok {
if t == fsx.LustreDeploymentTypePersistent1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@DrFaust92
Copy link
Collaborator

also run gofmt to fix lint issue

@DrFaust92
Copy link
Collaborator

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

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

yes, add something along the lines of:

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

@nikhil-goenka
Copy link
Contributor Author

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

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.

LGTM

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

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

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

Comment on lines 829 to 832

data "aws_kms_alias" "fsx" {
name = "alias/aws/fsx"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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"
}

Copy link
Contributor Author

@nikhil-goenka nikhil-goenka Sep 22, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

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

Choose a reason for hiding this comment

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

Related to below

Suggested change
datakmsKeyArn := "data.aws_kms_alias.fsx"

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

Choose a reason for hiding this comment

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

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/.+`)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

@bflad bflad added this to the v3.8.0 milestone Sep 22, 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.

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)

@bflad bflad merged commit 60db9d7 into hashicorp:master Sep 22, 2020
bflad added a commit that referenced this pull request Sep 22, 2020
@nikhil-goenka nikhil-goenka deleted the aws_fsx_lustre_file_system-custom-KMS-Key branch September 22, 2020 17:25
@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!

@ghost
Copy link

ghost commented Oct 23, 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 23, 2020
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/fsx Issues and PRs that pertain to the fsx service. size/M 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
Development

Successfully merging this pull request may close these issues.

aws_fsx_lustre_file_system specify custom KMS Key
3 participants