`azurerm_container_app` add support for key vault secret references as `secret` #23958

x-delfino · 2023-11-19T21:35:21Z

Adding support for key vault references for container apps. Resolving #23668 and #21739. Introduces the following properties for secret blocks for azurerm_container_app resources:

identity
key_vault_url

Have included tests & docs and have run make pr-check - all tests are passing successfully.

I had to split out the Dapr secrets as they don't look to actually support these additional properties. I know that the API reference for Dapr components includes them, but the API doesn't actually support them as far as I can tell. Trying to use these values against the API results in: Secret with name 'secretName' cannot have have a null value. And including value just adds a normal secret.

I believe Dapr component key vault secret references are actually handled as follows:

Configure key vault secret store component as detailed here.
Reference these in the secretsRef metadata properties for another component as detailed here.

I can take a look at raising a separate PR for adding secret reference support for the Dapr components. Apologies this took a while, I got a bit thrown off by the Dapr bits and never got round into reading up on it properly - but I'm fairly confident in my conclusion

magodo

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

internal/services/containerapps/helpers/container_apps.go

website/docs/r/container_app.html.markdown

internal/services/containerapps/helpers/container_apps.go

internal/services/containerapps/container_app_resource_test.go

magodo · 2023-11-20T05:57:05Z

internal/services/containerapps/container_app_resource_test.go

+  }
+  secret {
+    name          = "key-vault-secret"
+    identity      = azurerm_user_assigned_identity.test.id


Can we also test for the system assigned identity case?

do we need this in a separate test or within the existing one?

Can be in the same test case, but in some separate steps, e.g. UAI -> UAI,SAI -> SAI, so that we can test out the update logic.

i've added withKeyVaultSecretIdentityUpdate which does:

User Assigned

User Assigned & System Assigned

System Assigned

I've included the provisioning of the System Assigned identity within the User Assigned template (withKeyVaultSecretUserIdentity). This is as the API validates managed identity access to secrets when the container app is updated - so the identity and role assignment need to be created prior to the secret reference. I couldn't think of a cleaner way of doing this - any suggestions?

Typically, this requires a separate TF resource which extract these key vault parts out, e.g. https://github.com/hashicorp/terraform-provider-azurerm/blob/7596b072d0b8bf04585d0b77facd8296640d8a2a/internal/services/kusto/kusto_cluster_customer_managed_key_resource.go. In the container app case, there probably wants a azurerm_container_app_secret resource, which allows users to manage a secret at one time.

With above, users then are able to provision the resources in the following order: container app -> role assignment to the container app system assigned identity -> one or more container app secrets

okay cool i'll work on getting that created

internal/services/containerapps/container_app_resource_test.go

x-delfino · 2023-11-20T10:18:45Z

thanks for taking a look at this so quickly @magodo - will make the requested changes this evening

…cret_id, added validation for MI

internal/services/containerapps/helpers/container_apps.go

x-delfino · 2023-11-23T00:39:35Z

I need to update the docs for the dapr secrets still

jackofallops

Hi @x-delfino - I've left a few comments in addition to the earlier review. If you can take a look at those, I'll continue review.

Thanks

jackofallops · 2023-12-06T08:35:10Z

internal/services/containerapps/helpers/container_apps.go

 }

 func SecretsSchema() *pluginsdk.Schema {
 	return &pluginsdk.Schema{
-		Type:      pluginsdk.TypeSet,
+		Type:      pluginsdk.TypeList,


Unfortunately, this has to be a set as the API does not guarantee secret order on return, meaning that it can cause a diff in cases where there are 2 or more secrets.

Suggested change

Type: pluginsdk.TypeList,

Type: pluginsdk.TypeSet,

I've checked with the service team about this, they told me:

Currently, it is ordered. But we recommend you not depend on the order.

I've checked with the service team about this, they told me:

Currently, it is ordered. But we recommend you not depend on the order.

@magodo - I tested this as part of the review, and the response is not ordered unfortunately I saw intermittent diffs on the responses. If it's supposed to be ordered per the service team, we should have an issue on the API specs to track this bug?

From their response, the service team doesn't mean to guarantee the order as their goal, as they strongly recommend me not depend on the order..

apologies for delayed response. okay, my rudimentary testing was returning them in the same order but good to have clarification from the service team.

The issue is that the response includes the key vault secret value when key vault references are used. So we need to be able to ignore the secret value for secrets that use key vault references, but can't use ignore_changes on a list. I'm not sure of the best way to approach this. I think we could strip the secret value in FlattenContainerAppSecrets if a key vault reference is being used? Would that be an acceptable approach?

I've reverted to a set now. The secret value is not being stored when key vault references are used now (done in FlattenContainerAppSecrets). Is this an acceptable approach @jackofallops, @magodo?

internal/services/containerapps/helpers/container_apps.go

jackofallops · 2023-12-06T08:48:45Z

internal/services/containerapps/container_app_resource.go

-				stateSecrets := stateSecretsRaw.(*schema.Set).List()
-				configSecrets := configSecretsRaw.(*schema.Set).List()
+				stateSecrets := stateSecretsRaw.([]interface{})
+				configSecrets := configSecretsRaw.([]interface{})


See below, this needs to remain a TypeSet unfortunately.

davidkarlsen · 2023-12-17T10:43:00Z

@x-delfino Will you continue in this PR with the suggested changes?

x-delfino · 2023-12-17T21:54:06Z

@x-delfino Will you continue in this PR with the suggested changes?

apologies, i've been away. I'll have time to work on this over the next couple of days

Co-authored-by: jackofallops <[email protected]>

davidkarlsen · 2024-01-14T14:10:44Z

Any more changes needed or is this ready for review and merge?

btpemercier · 2024-01-15T12:50:51Z

internal/services/containerapps/helpers/container_apps.go

@@ -2214,17 +2218,33 @@ func SecretsSchema() *pluginsdk.Schema {
 		Sensitive: true,
 		Elem: &pluginsdk.Resource{
 			Schema: map[string]*pluginsdk.Schema{
+				"identity": {


Maybe you should use RequireWith and ConflictWith to not allow a user to fill in all 4 fields at once

btpemercier · 2024-01-15T12:51:36Z

internal/services/containerapps/helpers/container_apps.go

+					Description: "The identity to use for accessing key vault reference.",
+				},
+
+				"key_vault_secret_id": {


Why not using key_vault_secret_url ? the value is an URL and not an identifier and keeping the same name everywhere may be better for understanding

btpemercier · 2024-01-15T12:54:48Z

internal/services/containerapps/helpers/container_apps.go

+						commonids.ValidateUserAssignedIdentityID,
+						validation.StringInSlice([]string{"System"}, false),
+					),
+					Description: "The identity to use for accessing key vault reference.",


Suggested change

Description: "The identity to use for accessing key vault reference.",

Description: "Resource ID of a managed identity to authenticate with Azure Key Vault, or System to use a system-assigned identity.",

https://learn.microsoft.com/en-us/azure/templates/microsoft.app/containerapps?pivots=deployment-language-terraform#secret-2

btpemercier · 2024-01-15T12:55:06Z

internal/services/containerapps/helpers/container_apps.go

+					Type:         pluginsdk.TypeString,
+					Optional:     true,
+					ValidateFunc: keyVaultValidate.NestedItemIdWithOptionalVersion,
+					Description:  "The id of the key vault secret.",


Suggested change

Description: "The id of the key vault secret.",

Description: "Azure Key Vault URL pointing to the secret referenced by the container app.",

https://learn.microsoft.com/en-us/azure/templates/microsoft.app/containerapps?pivots=deployment-language-terraform#secret-2

btpemercier · 2024-01-15T12:55:36Z

internal/services/containerapps/helpers/container_apps.go

+				"key_vault_secret_id": {
+					Type:        pluginsdk.TypeString,
+					Computed:    true,
+					Description: "The id of the key vault secret.",


Suggested change

Description: "The id of the key vault secret.",

Description: "Azure Key Vault URL pointing to the secret referenced by the container app.",

https://learn.microsoft.com/en-us/azure/templates/microsoft.app/containerapps?pivots=deployment-language-terraform#secret-2

btpemercier · 2024-01-15T12:55:53Z

internal/services/containerapps/helpers/container_apps.go

+				"identity": {
+					Type:        pluginsdk.TypeString,
+					Computed:    true,
+					Description: "The identity to use for accessing key vault reference.",


Suggested change

Description: "The identity to use for accessing key vault reference.",

Description: "Resource ID of a managed identity to authenticate with Azure Key Vault, or System to use a system-assigned identity.",

https://learn.microsoft.com/en-us/azure/templates/microsoft.app/containerapps?pivots=deployment-language-terraform#secret-2

btpemercier · 2024-01-15T12:58:46Z

internal/services/containerapps/helpers/container_apps.go

@@ -2257,21 +2289,36 @@ func SecretsDataSourceSchema() *pluginsdk.Schema {
 	}
 }

-func ExpandContainerSecrets(input []Secret) *[]containerapps.Secret {
+func validateContainerSecret(s Secret) error {


Is this useful if you use RequireWith and ConflictWith?

eddy-vera · 2024-01-18T08:18:03Z

@x-delfino: Do you know when you have time to continue this PR? Thanks in advance for your reply and work on this.

x-delfino · 2024-01-22T19:35:57Z

unfortunately a few other commitments have taken priority and I'm not going to have time to give this attention for a couple of weeks realistically. If anyone has a bit more capacity to look at this in the meantime - feel free, otherwise I'll work on it when I have some time

eddy-vera · 2024-02-27T11:13:53Z

@x-delfino Any idea when you would have time to pick this up? Or are there others capable of doing this?

GurliGebis · 2024-02-27T11:33:31Z

@x-delfino Any idea when you would have time to pick this up? Or are there others capable of doing this?

Look at #24773

stephybun · 2024-03-05T13:33:05Z

Thanks for all your effort on this @x-delfino!

Since you're unable to continue this in the near future and another PR has been opened building on the work done here, I'm going to close this so that the work and review effort can be consolidated in one PR.

For everyone interested in this feature please subscribe to #24773 for updates. Thanks!

github-actions · 2024-04-19T02:08:21Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

x-delfino and others added 4 commits September 5, 2023 08:07

adding keyvault secret reference support for container apps

5a9e47a

removing erroneous newline

2d0cedb

removing erroneous space

163f22b

Merge branch 'hashicorp:main' into containerapp-keyvault

0ecacc0

github-actions bot added documentation service/container-apps size/L labels Nov 19, 2023

magodo added the enhancement label Nov 20, 2023

magodo requested changes Nov 20, 2023

View reviewed changes

github-actions bot added the waiting-response label Nov 20, 2023

github-actions bot added waiting-response and removed waiting-response labels Nov 20, 2023

updating primitive behaviours, secrets to list, renaming key_vault_se…

42ee774

…cret_id, added validation for MI

github-actions bot added size/XL and removed waiting-response size/L labels Nov 22, 2023

x-delfino commented Nov 22, 2023

View reviewed changes

internal/services/containerapps/helpers/container_apps.go Outdated Show resolved Hide resolved

adding docs, updating tests, fixing secret data schemas

c37d344

github-actions bot added the waiting-response label Nov 23, 2023

fixing dapr secrets schema

7f75fc1

github-actions bot removed the waiting-response label Nov 23, 2023

github-actions bot added the waiting-response label Nov 23, 2023

formatting

7db760f

github-actions bot added waiting-response and removed waiting-response labels Nov 23, 2023

This was linked to issues Nov 29, 2023

Support for Key Vault integration with Container Apps Secrets #23668

Closed

Support for key vault reference secrets in container apps #21739

Closed

jackofallops requested changes Dec 6, 2023

View reviewed changes

simon-ellefsen mentioned this pull request Dec 12, 2023

Infrastructure for auth-audit-log component Altinn/altinn-auth-audit-log#27

Closed

18 tasks

github-actions bot removed the waiting-response label Dec 17, 2023

x-delfino and others added 2 commits December 19, 2023 19:52

Update container app secret identity 'System' case sensitivity

3db3b88

Co-authored-by: jackofallops <[email protected]>

reverting to set. dont store kv secret value

421d329

btpemercier reviewed Jan 15, 2024

View reviewed changes

GurliGebis mentioned this pull request Feb 5, 2024

azurerm_container_app - Add key_vault_id and identity for secret nested block. #24773

Merged

stephybun closed this Mar 5, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_container_app` add support for key vault secret references as `secret` #23958

`azurerm_container_app` add support for key vault secret references as `secret` #23958

x-delfino commented Nov 19, 2023 •

edited

Loading

magodo left a comment

magodo Nov 20, 2023

x-delfino Nov 21, 2023

magodo Nov 22, 2023

x-delfino Nov 23, 2023 •

edited

Loading

magodo Nov 23, 2023

x-delfino Dec 19, 2023

x-delfino commented Nov 20, 2023

x-delfino commented Nov 23, 2023

jackofallops left a comment

jackofallops Dec 6, 2023

magodo Dec 7, 2023

jackofallops Dec 7, 2023

magodo Dec 8, 2023

x-delfino Dec 19, 2023

x-delfino Jan 4, 2024

jackofallops Dec 6, 2023

davidkarlsen commented Dec 17, 2023

x-delfino commented Dec 17, 2023

davidkarlsen commented Jan 14, 2024

btpemercier Jan 15, 2024

btpemercier Jan 15, 2024

btpemercier Jan 15, 2024

btpemercier Jan 15, 2024

btpemercier Jan 15, 2024

btpemercier Jan 15, 2024

btpemercier Jan 15, 2024

eddy-vera commented Jan 18, 2024

x-delfino commented Jan 22, 2024

eddy-vera commented Feb 27, 2024

GurliGebis commented Feb 27, 2024

stephybun commented Mar 5, 2024

github-actions bot commented Apr 19, 2024

	Description: "The identity to use for accessing key vault reference.",
	Description: "Resource ID of a managed identity to authenticate with Azure Key Vault, or System to use a system-assigned identity.",

	Description: "The id of the key vault secret.",
	Description: "Azure Key Vault URL pointing to the secret referenced by the container app.",

azurerm_container_app add support for key vault secret references as secret #23958

azurerm_container_app add support for key vault secret references as secret #23958

Conversation

x-delfino commented Nov 19, 2023 • edited Loading

magodo 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

x-delfino Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x-delfino commented Nov 20, 2023

x-delfino commented Nov 23, 2023

jackofallops 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkarlsen commented Dec 17, 2023

x-delfino commented Dec 17, 2023

davidkarlsen commented Jan 14, 2024

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

Choose a reason for hiding this comment

eddy-vera commented Jan 18, 2024

x-delfino commented Jan 22, 2024

eddy-vera commented Feb 27, 2024

GurliGebis commented Feb 27, 2024

stephybun commented Mar 5, 2024

github-actions bot commented Apr 19, 2024

`azurerm_container_app` add support for key vault secret references as `secret` #23958

`azurerm_container_app` add support for key vault secret references as `secret` #23958

x-delfino commented Nov 19, 2023 •

edited

Loading

x-delfino Nov 23, 2023 •

edited

Loading