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

azurerm_redis_cache - support access_keys_authentication_disabled property #27039

Merged
merged 16 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions internal/services/redis/redis_cache_data_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ func dataSourceRedisCache() *pluginsdk.Resource {
Sensitive: true,
},

"access_keys_authentication_enabled": {
Type: pluginsdk.TypeBool,
Computed: true,
},

"tags": commonschema.TagsDataSource(),
},
}
Expand Down Expand Up @@ -344,6 +349,12 @@ func dataSourceRedisCacheRead(d *pluginsdk.ResourceData, meta interface{}) error
d.Set("primary_connection_string", getRedisConnectionString(*props.HostName, *props.SslPort, *keys.Model.PrimaryKey, enableSslPort))
d.Set("secondary_connection_string", getRedisConnectionString(*props.HostName, *props.SslPort, *keys.Model.SecondaryKey, enableSslPort))

accessKeyAuthEnabled := true
if props.DisableAccessKeyAuthentication != nil {
accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)
}
d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)
Copy link
Member

Choose a reason for hiding this comment

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

It's a common pattern in the provider to make use of pointer.From when setting properties that are pointers into state. pointer.From will nil check the input and if the pointer is nil will return the zero value for the type that was passed in, so this would be condensed down to:

Suggested change
accessKeyAuthEnabled := true
if props.DisableAccessKeyAuthentication != nil {
accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)
}
d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)
d.Set("access_keys_authentication_enabled", !pointer.From(props.DisableAccessKeyAuthentication))

Copy link
Collaborator

Choose a reason for hiding this comment

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

@stephybun is right, I definitely missed this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice idea, thx @stephybun . I will alter the suggested code a bit: we want to make the expression defaults to true if we get nil value on DisableAccessKeyAuthentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 50de57e

Copy link
Member

Choose a reason for hiding this comment

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

@gerrytan that is the outcome of my suggestion above.

If props.DisableAccessKeyAuthentication is nil, pointer.From will return false (the zero value for bools) which will result in true since it's being inverted. The variable initialization and nil check on lines 357-361 are redundant. Can you please apply the suggestion as made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephybun ah I get it now. Fixed in 5341f2f.


if err := tags.FlattenAndSet(d, model.Tags); err != nil {
return fmt.Errorf("setting `tags`: %+v", err)
}
Expand Down
1 change: 1 addition & 0 deletions internal/services/redis/redis_cache_data_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func TestAccRedisCacheDataSource_standard(t *testing.T) {
check.That(data.ResourceName).Key("tags.environment").HasValue("production"),
check.That(data.ResourceName).Key("primary_connection_string").Exists(),
check.That(data.ResourceName).Key("secondary_connection_string").Exists(),
check.That(data.ResourceName).Key("access_keys_authentication_enabled").Exists(),
),
},
})
Expand Down
35 changes: 32 additions & 3 deletions internal/services/redis/redis_cache_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,12 @@ func resourceRedisCache() *pluginsdk.Resource {
},
},

"access_keys_authentication_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: true,
},

"tags": commonschema.Tags(),
},

Expand All @@ -376,6 +382,21 @@ func resourceRedisCache() *pluginsdk.Resource {
}
return false
}),
pluginsdk.CustomizeDiffShim(func(ctx context.Context, diff *pluginsdk.ResourceDiff, v interface{}) error {
// Entra (AD) auth has to be set to disable access keys auth
// https://learn.microsoft.com/en-us/azure/azure-cache-for-redis/cache-azure-active-directory-for-authentication

accessKeysAuthenticationEnabled := diff.Get("access_keys_authentication_enabled").(bool)
activeDirectoryAuthenticationEnabled := diff.Get("redis_configuration.0.active_directory_authentication_enabled").(bool)

log.Printf("[DEBUG] CustomizeDiff: access_keys_authentication_enabled: %v, active_directory_authentication_enabled: %v", accessKeysAuthenticationEnabled, activeDirectoryAuthenticationEnabled)

if !accessKeysAuthenticationEnabled && !activeDirectoryAuthenticationEnabled {
return fmt.Errorf("`active_directory_authentication_enabled` must be enabled in order to disable `access_keys_authentication_enabled`")
}

return nil
}),
),
}

Expand Down Expand Up @@ -499,7 +520,8 @@ func resourceRedisCacheCreate(d *pluginsdk.ResourceData, meta interface{}) error
parameters := redis.RedisCreateParameters{
Location: location.Normalize(d.Get("location").(string)),
Properties: redis.RedisCreateProperties{
EnableNonSslPort: pointer.To(enableNonSslPort.(bool)),
DisableAccessKeyAuthentication: pointer.To(!(d.Get("access_keys_authentication_enabled").(bool))),
EnableNonSslPort: pointer.To(enableNonSslPort.(bool)),
Sku: redis.Sku{
Capacity: int64(d.Get("capacity").(int)),
Family: redis.SkuFamily(d.Get("family").(string)),
Expand Down Expand Up @@ -613,8 +635,9 @@ func resourceRedisCacheUpdate(d *pluginsdk.ResourceData, meta interface{}) error

parameters := redis.RedisUpdateParameters{
Properties: &redis.RedisUpdateProperties{
MinimumTlsVersion: pointer.To(redis.TlsVersion(d.Get("minimum_tls_version").(string))),
EnableNonSslPort: pointer.To(enableNonSslPort.(bool)),
DisableAccessKeyAuthentication: pointer.To(!(d.Get("access_keys_authentication_enabled").(bool))),
MinimumTlsVersion: pointer.To(redis.TlsVersion(d.Get("minimum_tls_version").(string))),
EnableNonSslPort: pointer.To(enableNonSslPort.(bool)),
Sku: &redis.Sku{
Capacity: int64(d.Get("capacity").(int)),
Family: redis.SkuFamily(d.Get("family").(string)),
Expand Down Expand Up @@ -837,6 +860,12 @@ func resourceRedisCacheRead(d *pluginsdk.ResourceData, meta interface{}) error {
d.Set("primary_access_key", keysResp.Model.PrimaryKey)
d.Set("secondary_access_key", keysResp.Model.SecondaryKey)

accessKeyAuthEnabled := true
if props.DisableAccessKeyAuthentication != nil {
accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)
}
d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
accessKeyAuthEnabled := true
if props.DisableAccessKeyAuthentication != nil {
accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)
}
d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)
d.Set("access_keys_authentication_enabled", !pointer.From(props.DisableAccessKeyAuthentication))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 50de57e

Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5341f2f


if err := tags.FlattenAndSet(d, model.Tags); err != nil {
return fmt.Errorf("setting `tags`: %+v", err)
}
Expand Down
57 changes: 57 additions & 0 deletions internal/services/redis/redis_cache_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,35 @@ func TestAccRedisCache_SkuDowngrade(t *testing.T) {
})
}

func TestAccRedisCache_AccessKeysAuthenticationEnabledDisabled(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_redis_cache", "test")
r := RedisCacheResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.accessKeysAuthentication(data, true, false),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
Config: r.accessKeysAuthentication(data, false, true),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
Config: r.accessKeysAuthentication(data, true, false),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
})
}

func (t RedisCacheResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := redis.ParseRediID(state.ID)
if err != nil {
Expand Down Expand Up @@ -1594,3 +1623,31 @@ resource "azurerm_redis_cache" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger)
}

func (RedisCacheResource) accessKeysAuthentication(data acceptance.TestData, accessKeysAuthenticationEnabled bool, activeDirectoryAuthenticationEnabled bool) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_redis_cache" "test" {
name = "acctestRedis-%d"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
capacity = 1
family = "C"
sku_name = "Basic"
non_ssl_port_enabled = false
minimum_tls_version = "1.2"
access_keys_authentication_enabled = %t

redis_configuration {
active_directory_authentication_enabled = %t
}
}`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, accessKeysAuthenticationEnabled, activeDirectoryAuthenticationEnabled)
}
2 changes: 2 additions & 0 deletions website/docs/d/redis_cache.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ output "hostname" {

* `secondary_connection_string` - The secondary connection string of the Redis Instance.

* `access_keys_authentication_enabled` - Specifies if access key authentication is enabled.

* `redis_configuration` - A `redis_configuration` block as defined below.

* `zones` - A list of Availability Zones in which this Redis Cache is located.
Expand Down
2 changes: 2 additions & 0 deletions website/docs/r/redis_cache.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ The following arguments are supported:

---

* `access_keys_authentication_enabled` - (Optional) Whether access key authentication is enabled? Defaults to `true`. `active_directory_authentication_enabled` must be set to `true` to disable access key authentication.

* `enable_non_ssl_port` - (Optional) Enable the non-SSL port (6379) - disabled by default.

* `identity` - (Optional) An `identity` block as defined below.
Expand Down
Loading