-
Notifications
You must be signed in to change notification settings - Fork 37
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
Redis authentication managed identity #213
base: main
Are you sure you want to change the base?
Conversation
…dentity client id
…or Azure Cache for Redis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a terraform expert, but left a few comments from my experience with redis+managed identity
@@ -26,6 +26,8 @@ resource "azurerm_redis_cache" "cache" { | |||
|
|||
# https://learn.microsoft.com/en-us/azure/azure-cache-for-redis/cache-configure#default-redis-server-configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need for en-us; if you remove that, it will automatically redirect to the right locale for the user
@@ -92,6 +106,13 @@ module "dev_application" { | |||
frontdoor_profile_uuid = module.dev_frontdoor[0].resource_guid | |||
public_network_access_enabled = true | |||
|
|||
identity = { | |||
type = "SystemAssigned, UserAssigned" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this create both a systemassigned and userassigned? do you need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will create both. We need both as Redis is the first use case that uses user assigned managed identity. Key Vault still uses the System Assigned managed identity. We have an item in the backlog to transition to user assigned managed identity.
infra/terraform/cache.tf
Outdated
count = var.environment == "prod" ? 1 : 0 | ||
name = "secondarycurrentuser" | ||
redis_cache_id = module.secondary_cache[0].cache_id | ||
access_policy_name = "Data Owner" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found Data Contributor
was sufficent and least privilege
count = var.environment == "prod" ? 1 : 0 | ||
name = "secondaryappuser" | ||
redis_cache_id = module.secondary_cache[0].cache_id | ||
access_policy_name = "Data Contributor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found Data Contributor
was sufficient and least privilege
object_id_alias = azurerm_user_assigned_identity.secondary_app_service_identity[0].principal_id | ||
|
||
depends_on = [ | ||
azurerm_redis_cache_access_policy_assignment.secondary_current_user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe leave a comment as an fyi that access policy assignments must be done serially, hence why you're depending on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this was fixed - hashicorp/terraform-provider-azurerm#26085. I'm going to get rid of this dependency.
infra/terraform/cache.tf
Outdated
count = var.environment == "dev" ? 1 : 0 | ||
name = "devcurrentuser" | ||
redis_cache_id = module.dev-cache[0].cache_id | ||
access_policy_name = "Data Owner" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall comment: Try to avoid whitespace changes.
@@ -123,6 +125,11 @@ | |||
<artifactId>spring-boot-starter-aop</artifactId> | |||
</dependency> | |||
|
|||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this dependency necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable support for managed identity authentication for Azure Cache for Redis.