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

feat: Support Valkey #26

Merged

Conversation

magreenbaum
Copy link
Member

Description

Support Valkey.

As a note, there is a bug in the provider where aws_elasticache_cluster doesn't support Valkey.

Motivation and Context

Closes: #25
hashicorp/terraform-provider-aws#39972

Breaking Changes

No.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@BramRoets
Copy link

@magreenbaum I'm unable to deploy valkey using your changes. Using the following code, I get a validation error:

module "elasticache" {
  source = "github.com/magreenbaum/terraform-aws-elasticache?ref=8131ae92fd7d9131d602760af0c92adf9db74a88"

  cluster_id               ="${var.name}-valkey"
  create_cluster           = true
  create_replication_group = false

  engine_version = "7.2"
  engine         = "valkey"
  node_type      = "cache.t4g.micro"


  vpc_id = module.vpc.vpc_id
  security_group_rules = {
    ingress_vpc = {
      cidr_ipv4 = module.vpc.vpc_cidr_block
    }
  }

  subnet_ids = module.vpc.private_subnets

  create_parameter_group = true
  parameter_group_family = "valkey7"
}

Error:

│ Error: expected engine to be one of ["memcached" "redis"], got valkey
│
│   with module.elasticache.aws_elasticache_cluster.this[0],
│   on .terraform/modules/elasticache/main.tf line 23, in resource "aws_elasticache_cluster" "this":
│   23:   engine                     = local.in_replication_group ? null : var.engine```

@magreenbaum
Copy link
Member Author

@magreenbaum I'm unable to deploy valkey using your changes. Using the following code, I get a validation error:

module "elasticache" {
  source = "github.com/magreenbaum/terraform-aws-elasticache?ref=8131ae92fd7d9131d602760af0c92adf9db74a88"

  cluster_id               ="${var.name}-valkey"
  create_cluster           = true
  create_replication_group = false

  engine_version = "7.2"
  engine         = "valkey"
  node_type      = "cache.t4g.micro"


  vpc_id = module.vpc.vpc_id
  security_group_rules = {
    ingress_vpc = {
      cidr_ipv4 = module.vpc.vpc_cidr_block
    }
  }

  subnet_ids = module.vpc.private_subnets

  create_parameter_group = true
  parameter_group_family = "valkey7"
}

Error:

│ Error: expected engine to be one of ["memcached" "redis"], got valkey
│
│   with module.elasticache.aws_elasticache_cluster.this[0],
│   on .terraform/modules/elasticache/main.tf line 23, in resource "aws_elasticache_cluster" "this":
│   23:   engine                     = local.in_replication_group ? null : var.engine```

There is a bug in the provider currently for creating valkey clusters: hashicorp/terraform-provider-aws#39905

@bryantbiggs
Copy link
Member

we should wait for the upstream provider bug to be resolved first, no?

@magreenbaum
Copy link
Member Author

magreenbaum commented Nov 11, 2024

we should wait for the upstream provider bug to be resolved first, no?

@bryantbiggs we can, but I figured this way those that want to use valkey for replication groups and serverless can do so in the mean time since the bug only affects aws_elasticache_cluster resource.

@phene
Copy link

phene commented Nov 13, 2024

Awesome. Please lets merge this regardless of the provider bug.

@bryantbiggs
Copy link
Member

I've pinged a few folks - I don't want to add support for something that is known to not work

@bryantbiggs
Copy link
Member

FYI - hashicorp/terraform-provider-aws#39972 (comment)

So we shouldn't add support for it here for cache clusters (serverless and replication groups do support it and we can support that as well)

@magreenbaum
Copy link
Member Author

Added a conditional to not create clusters if engine is valkey.

@eahangari-8x8
Copy link

Hi All, any update on Valkey support?

@bryantbiggs
Copy link
Member

I will take a look today - it's re:Invent season plus the holiday season so be patient 😅

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

thank you @magreenbaum, another phenomenal contribution 🎉

@bryantbiggs bryantbiggs merged commit 6b1b5aa into terraform-aws-modules:master Nov 29, 2024
15 checks passed
antonbabenko pushed a commit that referenced this pull request Nov 29, 2024
## [1.4.0](v1.3.0...v1.4.0) (2024-11-29)

### Features

* Support Valkey ([#26](#26)) ([6b1b5aa](6b1b5aa))
@antonbabenko
Copy link
Member

This PR is included in version 1.4.0 🎉

@magreenbaum magreenbaum deleted the feat/valkey_support branch November 29, 2024 18:37
@taer
Copy link

taer commented Dec 3, 2024

This line seems to prevent valkey creation still
https://github.com/terraform-aws-modules/terraform-aws-elasticache/blob/master/main.tf#L8

  # elasticache clusters currently do not support engine type valkey
  # TODO: remove this local `create_cluster` conditional once this bug is addressed:
  # https://github.com/hashicorp/terraform-provider-aws/issues/39905
  create_cluster = var.create_cluster && var.engine != "valkey" ? true : false

when I put valkey in for the engine with the latest module, I get a delete of my cluster. Should this above line vanish now? @magreenbaum

@bryantbiggs
Copy link
Member

Valkey is not supported on clusters, only replication groups

Copy link

github-actions bot commented Jan 3, 2025

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 issues. 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 Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants