Feature/buildingblocks standard vnet conf edit #87
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
| # name = "tfstates-standard-vnet" | ||
| # storage_account_name = data.azurerm_storage_account.tfstates.name | ||
| # container_access_type = "blob" | ||
| # } |
There was a problem hiding this comment.
can you adding a comment why its commented out or removing it?
There was a problem hiding this comment.
We are creating the container using another provider here
There was a problem hiding this comment.
Agreed, please remove it (or add a comment why it's there but commented out, but if there is another provider, removing it seems better to me).
| # name = "tfstates-standard-vnet" | ||
| # storage_account_name = data.azurerm_storage_account.tfstates.name | ||
| # container_access_type = "blob" | ||
| # } |
There was a problem hiding this comment.
Agreed, please remove it (or add a comment why it's there but commented out, but if there is another provider, removing it seems better to me).
| @@ -62,6 +42,7 @@ resource "time_rotating" "building_blocks_secret_rotation" { | |||
| } | |||
| resource "azuread_application_password" "building_blocks_application_pw" { | |||
| application_id = azuread_application.building_blocks.id | |||
| #application_object_id = azuread_application.building_blocks.object_id | |||
There was a problem hiding this comment.
Please remove commented out code or add an explanation for why it's in here
| @@ -72,6 +53,7 @@ resource "azuread_application_password" "building_blocks_application_pw" { | |||
| //--------------------------------------------------------------------------- | |||
| resource "azuread_service_principal" "building_blocks_spn" { | |||
| client_id = azuread_application.building_blocks.client_id | |||
| #application_id = azuread_application.building_blocks.application_id | |||
There was a problem hiding this comment.
Please remove commented out code or add an explanation for why it's in here
| parent_id = "${data.azurerm_storage_account.tfstates.id}/blobServices/default" | ||
| body = jsonencode({ | ||
| properties = { | ||
| defaultEncryptionScope = "$account-encryption-key" |
There was a problem hiding this comment.
Q: Which encryption scope does $account-encryption-key map to? Is it the one from the account and if yes, do we need to set it explicitly?
What's the advantage of a dedicate scope over using the scope created for the storage account?
There was a problem hiding this comment.
this is the default one that the other containers have, in general you can change the encryption key on the blob level if e.g. different teams use a same storage account. can be microsoft managed or customer managed keys
There was a problem hiding this comment.
If it's the default one, can we leave it out of the definition or does it have to be here?
There was a problem hiding this comment.
it has to be there, azapi is a bit different, needs more manual intervention
| enabled = false | ||
| } | ||
| metadata = {} | ||
| publicAccess = "Blob" |
There was a problem hiding this comment.
D: As far as I understand, this means we rely on the storage account not allowing public access to overwrite this setting otherwise it would allow anonymous public access to terraform state stored in the blobs. If my understanding is correct, I'm in favour of not allowing public access here at all.
There was a problem hiding this comment.
good point, i've changed it to None
No description provided.