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 - Check if redis_configuration.rdb_backup_enabled and redis_configuration.aof_backup_enabled configured #22309

Merged
merged 4 commits into from
Jul 20, 2023

Conversation

xuzhang3
Copy link
Contributor

Fixes: #21967

Check if redis_configuration.rdb_backup_enabled and redis_configuration.aof_backup_enabled configured. Ignore the default values.

@em-le-ts
Copy link

hi @xuzhang3, we using this provider with version 3.0.0. Please help me consider releasing this tag

@xuzhang3
Copy link
Contributor Author

@em-le-ts I'm afraid you may need to upgrade to the latest version of AzureRM provider. One workaround not upgrade the AzureRM provider you can build a private package.

if connStr := raw["rdb_storage_connection_string"].(string); connStr == "" {
return nil, fmt.Errorf("The rdb_storage_connection_string property must be set when rdb_backup_enabled is true")
}
config := d.GetRawConfig().AsValueMap()["redis_configuration"].AsValueSlice()[0].AsValueMap()
Copy link
Member

Choose a reason for hiding this comment

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

We can't be using GetRawConfig since this functionality won't be available when we move to framework.

There is a wrapper function that can be used instead pluginsdk.IsExplicitlyNullInConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rdb_storage_connection_string is nested in redis_configuration. Cannot get it with pluginsdk.IsExplicitlyNullInConfig(d, "redis_configuration.0.rdb_backup_enabled")

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case there's no need to use either IsExplicitlyNullInConfig or d.GetRawConfig().AsValueMap() - this should only be set if the value is set to true?

Copy link
Contributor Author

@xuzhang3 xuzhang3 Jul 10, 2023

Choose a reason for hiding this comment

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

rdb_backup_enabled can be false or true and it can only be set when SKU is Premium

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avendretter @em-le-ts rdb_backup_enabled can be set when SKU is Premium. Current V2 SDK will auto set a default value(false) to it even not configured. So I need to know if this is configured in the HCL, One way to get it is use d.GetRawConfig().AsValueMap() but this function is not compatible with the terraform framework plugin. Currently we are blocked here.

Copy link

Choose a reason for hiding this comment

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

@xuzhang3 I used the Basic SKU and it was failing, saying that the rdb_backup_enabled option is only allowed for Premium, so I assume it will be set to true regardless of the inputs of SKU

@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Jul 3, 2023

@stephybun rdb_storage_connection_string is nested in redis_configuration block. Cannot get it with pluginsdk.IsExplicitlyNullInConfig(d, "redis_configuration.0.rdb_backup_enabled")

@em-le-ts
Copy link

em-le-ts commented Jul 6, 2023

any update sir @xuzhang3 ?

@ghost
Copy link

ghost commented Jul 11, 2023

g'day @xuzhang3,
Any update on this? It is currently blocking for my team as well.

@xuzhang3
Copy link
Contributor Author

@avendretter I updated the PR rdb_backup_enabled to be sent to service only if the SKU is Premium, this will ignore the default value or explicit configurations.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Thanks for updating @xuzhang3. I think this looks ok, but we should also check for the opposite scenario (see inline comment below).

return nil, fmt.Errorf("The rdb_storage_connection_string property must be set when rdb_backup_enabled is true")
skuName := d.Get("sku_name").(string)
// rdb_backup_enabled is available when SKU is Premium
if strings.EqualFold(skuName, string(redis.SkuNamePremium)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also check if the SKU is not valid for this property, and return an error in case the user sets rdb_backup_enabled = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a new constraint to cover the rdb_backup_enabled = true scenario.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Thanks for the update @xuzhang3, this LGTM

Screenshot 2023-07-20 at 01 12 40

@manicminer manicminer added this to the v3.66.0 milestone Jul 20, 2023
@manicminer manicminer merged commit d60f1b9 into hashicorp:main Jul 20, 2023
manicminer added a commit that referenced this pull request Jul 20, 2023
@raswinraaj
Copy link

raswinraaj commented Jul 27, 2023

@manicminer I am still getting error on 3.66.0 for the field aof_backup_enabled for a Standard SKU of redis. Are you sure this has been released? I have also tried by adding the field to ignore_changes block. But still I get the error.
Below is the error:
unexpected status 400 with error: BadRequest: Feature properties.redisConfiguration.aof-backup-enabled requires a Premium sku to be set.

This is my code

resource "azurerm_redis_cache" "main" {
  name                = "${var.environmentprefix}-test-redis"
  location            = var.location
  resource_group_name = var.resource_group_name
  capacity            = var.redis_capacity
  family              = var.redis_family
  sku_name            = var.redis_sku_name
  redis_version       = var.redis_version
  enable_non_ssl_port = false
  minimum_tls_version = "1.2"

  redis_configuration {
    aof_backup_enabled              = false
    maxmemory_policy                = "volatile-lru"
  }

  tags = var.tags
  lifecycle {
    ignore_changes = [        
        redis_configuration[0].aof_backup_enabled
    ]
  }
}

@raimdev
Copy link

raimdev commented Jul 27, 2023

After upgrading to 3.66.0 problem still persists, only rolling back to 3.53.0 helped to get successfull run

@manicminer
Copy link
Contributor

@xuzhang3 Could you take a look into this?

@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Jul 28, 2023

@raswinraaj Remove the redis_configuration.aof_backup_enabled. Currently the provider cannot detect if aof_backup_enabled=false is explicit configured or default value. redis_configuration.aof_backup_enabled is available when SKU is Premium

@raswinraaj
Copy link

@xuzhang3 if I remove that, i get this below error for a Redis with Standard SKU . I have actually imported this resource into my existing TF State.

unexpected status 400 with error: BadRequest: Feature properties.redisConfiguration.aof-backup-enabled requires a Premium sku to be set.

@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Aug 1, 2023

@raswinraaj I see, this is the same issue like rdb_backup_enabled. Though aof-backup-enabled not configured but a default value will be set which is false(bool type value is a bit special in Terraform SDK).

@raswinraaj
Copy link

raswinraaj commented Aug 1, 2023

ok. so is there a solution for this?

I currently got it working by setting redis_configuration under ignore_changes lifecycle.

resource "azurerm_redis_cache" "main" {
  name                = "${var.environmentprefix}-test-redis"
  location            = var.location
  resource_group_name = var.resource_group_name
  capacity            = var.redis_capacity
  family              = var.redis_family
  sku_name            = var.redis_sku_name
  redis_version       = var.redis_version
  enable_non_ssl_port = false
  minimum_tls_version = "1.2"

  redis_configuration {
    aof_backup_enabled              = false
    maxmemory_policy                = "volatile-lru"
  }

  tags = var.tags
  lifecycle {
    ignore_changes = [        
        redis_configuration[0]
    ]
  }
}

@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Aug 1, 2023

@raswinraaj add a constraint to make sure the configuration is map to the required SKU type. Bool type will auto set to a default value false and currently we cannot distinguish if it is explicit configured to false or default value. This is the root issue. v3.53.0 works because it only accept true otherwise it will be ignored.

@fethullahmisir
Copy link

We are currently using the most recent version of the provider, v3.67.0, and we have also come across this problem. However, we found a workaround by setting aof_backup_enabled to null. This means we didn't need to downgrade to an earlier version of the provider in our case.

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2024
@xuzhang3 xuzhang3 deleted the fix/redis_21967 branch August 14, 2024 02:39
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.

RDP Backup on Redis Cache is enable by default in Standard SKU
8 participants