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

New Resource: 'azurerm_storage_account_encryption_settings' to enable storage account encryption using key vault customer-managed keys #2046

Closed

Conversation

liemnotliam
Copy link
Contributor

@liemnotliam liemnotliam commented Oct 9, 2018

This PR adds storage account key vault properties to enable support for customer-managed keys for storage service encryption.

Issue #658

Test results:

~/go/src/github.com/terraform-providers/terraform-provider-azurerm
λ TESTARGS="-run TestAccAzureRMStorageAccount_keyVault" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccAzureRMStorageAccount_keyVault -count=1 -timeout 180m
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMStorageAccount_keyVault
--- PASS: TestAccAzureRMStorageAccount_keyVault (224.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	224.051s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/authentication	0.027s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure	0.030s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/kubernetes	0.010s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response	0.026s [no tests to run]
?   	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/set	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress	0.028s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate	0.018s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils	0.020s [no tests to run]

* Add key_vault_properties optional parameter to storage account.
* Add test for storage account with custom key for SSE.
* Update docs.
@LaurentLesle
Copy link
Contributor

LaurentLesle commented Oct 11, 2018

In order to get this PR working I need the KeyVault to have
enablePurgeProtection and enableSoftDelete

https://docs.microsoft.com/en-us/rest/api/keyvault/vaults/vaults_createorupdate

Looks like the KeyVault provider needs to be updated. Have you noticed that?

@LaurentLesle
Copy link
Contributor

added request to support enablePurgeProtection and enableSoftDelete in KV #2066

@liemnotliam
Copy link
Contributor Author

@LaurentLesle good catch, enablePurgeProtection and enableSoftDelete properties do need to be set on the key vault. A request to set storage account keyvault properties returns OK, which is why everything looked good.

The test case needs to be updated when the enablePurgeProtection and enableSoftDelete properties are added (PR #1805) to the keyvault resource and this PR should be good to review again.

@tombuildsstuff tombuildsstuff added this to the 1.20.0 milestone Oct 25, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 1.20.0, 1.21.0 Dec 6, 2018
@kvaes
Copy link

kvaes commented Dec 13, 2018

Out-of-curiosity ; Won't this go into a cycle/loop?
Next to that ; Interested in seeing it merged. ;-)

@katbyte katbyte modified the milestones: 1.21.0, 2.0.0 Jan 11, 2019
@ghost ghost added the documentation label Jan 15, 2019
Copy link
Contributor

@metacpp metacpp left a comment

Choose a reason for hiding this comment

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

@liemnotliam Thanks for the contributions and I added some comments.

azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
@WodansSon
Copy link
Collaborator

@liemnotliam Thanks for this PR, if you don't mind I would like to build on top of what you did here in your fork to get this merged. I am currently splitting this resource into two resources since there are multiple steps that need to be done (e.g. create the service account, created a key vault policy on the key vault for the newly created service account, then update the service account with the correct encryption settings).

@ghost ghost added size/XL and removed size/L labels Feb 8, 2019
@tombuildsstuff
Copy link
Contributor

@jeffreyCline heads up this is dependent on Key Vault Soft Delete (#1805) which requires some thought / will be a breaking change

@metacpp
Copy link
Contributor

metacpp commented Feb 21, 2019

@jeffreyCline thanks for pushing the commits.

I notice that the Travis CI failed due to lint error:

$ make lint
==> Checking source code against linters...
golangci-lint run ./...
azurerm/resource_arm_storage_account_key_vault_custom_keys.go:110:34: unnecessary conversion (unconvert)
					KeySource: storage.KeySource(storage.MicrosoftStorage),
					                            ^
azurerm/resource_arm_storage_account_key_vault_custom_keys.go:151:43: unnecessary conversion (unconvert)
					KeySource:          storage.KeySource(storage.MicrosoftKeyvault),
					                                     ^
azurerm/resource_arm_storage_account_key_vault_custom_keys.go:235:5: S1009: should omit nil check; len() for nil slices is defined as zero (gosimple)
	if vs == nil || len(vs) == 0 {
	   ^
make: *** [lint] Error 1
The command "make lint" exited with 2.

Can we get it fixed so that we move forward ?

@WodansSon
Copy link
Collaborator

@metacpp I am removing your review since it is outdated.

@WodansSon WodansSon dismissed metacpp’s stale review February 22, 2019 05:52

Review is outdated

@WodansSon WodansSon changed the title Storage Account: Add key vault properties New Resource: 'azurerm_storage_account_encryption_settings' to enable storage account encryption using key vault customer-managed keys Feb 22, 2019
@ghost ghost added size/XXL and removed size/XL labels Feb 27, 2019
@msuringa
Copy link

Is there an ETA on when this feature will be implemented? I would very much like to have this functionality in the provider, at the moment I'm having to work around it by following the Terraform run with a Powershell script to add the encryption settings.

@ntmggr
Copy link

ntmggr commented Jul 19, 2019

This is very much needed. I don't see the point of having the option to choose the account_encryption_source to be Microsoft.Keyvault but without actually working due to not being able to pass the keyvault properties

@msakowski
Copy link

Wouldn't it be nice to just pass the key URI that already includes the full path to the keyVault, keyName and keyVersion?

@WodansSon
Copy link
Collaborator

I am porting this to the new code base for 2.0 release.

@WodansSon WodansSon closed this Feb 10, 2020
@ghost
Copy link

ghost commented Feb 24, 2020

This has been released in version 2.0.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.0.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 28, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants