-
Notifications
You must be signed in to change notification settings - Fork 9
Release test for AWS IAM Redis passwordless #189
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
Conversation
- Add redis_passwordless_aws_use_iam variable to enable AWS IAM Redis auth - Add redis_passwordless_aws_region variable for region specification - Configure TFE_REDIS_PASSWORDLESS_AWS_USE_IAM environment variable - Configure TFE_REDIS_SIDEKIQ_PASSWORDLESS_AWS_USE_IAM for Sidekiq - Configure TFE_REDIS_PASSWORDLESS_AWS_REGION environment variable These changes enable TFE to use AWS IAM authentication for Redis connections instead of password-based authentication, following the same pattern as Azure MSI Redis authentication.
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
- Add TFE_REDIS_SIDEKIQ_* environment variables for separate Sidekiq Redis - Add TFE_REDIS_SIDEKIQ_PASSWORDLESS_AWS_* variables for IAM authentication - Add TFE_REDIS_PASSWORDLESS_AWS_HOST_NAME for cluster name extraction - Support separate Redis instances for main and Sidekiq with fallback to main Redis - Add variables: redis_sidekiq_host, redis_sidekiq_user, redis_sidekiq_password - Enable dual Redis passwordless authentication (main + Sidekiq)
- Add debug output for all Redis environment variables - Add debug output for input variables received - Track Redis username propagation to TFE container
- Add redis_passwordless_aws_use_iam variable to variables.tf - Add TFE_REDIS_PASSWORDLESS_AWS_USE_IAM and TFE_REDIS_SIDEKIQ_PASSWORDLESS_AWS_USE_IAM environment variables to redis_config.tf - Add debug outputs for Redis environment variables and input variables - Ensures Redis IAM authentication is properly configured for AWS ElastiCache
…_config - Fix alignment and indentation in debug_redis_env_vars and debug_redis_input_vars outputs - Standardize spacing to align equals signs consistently with terraform formatting standards - Resolves terraform fmt check failure in CI pipeline
…m-tfe-utility into pravi/IND-5861
ajmera-naman
left a comment
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.
Configuration seems incorrect
…pport - Change redis_passwordless_aws_use_iam to redis_passwordless_aws_use_instance_profile - Add database_passwordless_aws_use_instance_profile and database_passwordless_aws_region variables - Remove debug outputs as requested in PR review - Variables now match TFE documentation exactly Addresses PR review comments: - Configuration now uses correct TFE environment variable names - Database AWS IAM authentication support added - Debug outputs removed for cleaner module interface
- Remove database_passwordless_aws_use_instance_profile variable - Remove database_passwordless_aws_region variable - Remove TFE_DATABASE_PASSWORDLESS_AWS_* environment variables PostgreSQL AWS IAM authentication is out of scope for this Redis passwordless authentication feature. These variables were incorrectly added during the variable name fix and should not be included. Focus remains purely on Redis passwordless authentication with correct variable names matching TFE documentation.
…ables - Add redis_passwordless_aws_region and redis_passwordless_aws_host_name variables - Add corresponding TFE environment variables for Redis AWS region and hostname - Add Sidekiq variants for both region and hostname - Apply terraform formatting alignment for consistency - Complete Redis passwordless authentication implementation
- Added TFE_REDIS_SIDEKIQ_USER (set to redis_user for IAM authentication) - Added TFE_REDIS_SIDEKIQ_USE_TLS (set to redis_use_tls value) These variables are required by the official TFE documentation for Redis IAM authentication.
- Use 'default' username when redis_passwordless_aws_use_instance_profile=true - Set password to null for IAM authentication (TFE generates tokens dynamically) - Apply same fix for both TFE_REDIS_USER and TFE_REDIS_SIDEKIQ_USER
- Use redis_passwordless_aws_iam_user instead of hardcoded 'default' - Add redis_passwordless_aws_iam_user variable - Apply to both TFE_REDIS_USER and TFE_REDIS_SIDEKIQ_USER This enables proper testing of custom ElastiCache IAM users per AWS documentation instead of relying on the 'default' user which bypasses IAM validation.
This will confirm that: 1. Redis IAM authentication mechanism is working 2. The issue is specifically with custom IAM user configuration Once confirmed working, we'll know the problem is the ElastiCache user group doesn't properly include the custom IAM user 'fitg-iam-user'.
Now that IAM policy is fixed with explicit resource ARNs, we can test the custom IAM user authentication properly.
ajmera-naman
left a comment
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.
LGTM
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.
Pull request overview
This PR adds support for AWS IAM authentication for Redis connections in Terraform Enterprise as an alternative to password-based authentication. The changes introduce new configuration variables for AWS-specific Redis authentication and update the Redis configuration logic to conditionally use IAM authentication when enabled.
Key changes:
- Added four new variables to support AWS IAM Redis authentication configuration
- Updated Redis configuration to conditionally use IAM authentication instead of passwords when AWS instance profile is enabled
- Added corresponding environment variables for both main Redis and Sidekiq Redis connections
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/runtime_container_engine_config/variables.tf | Defines new variables for AWS IAM Redis authentication including instance profile flag, region, host name, and IAM user |
| modules/runtime_container_engine_config/redis_config.tf | Updates Redis configuration logic to use IAM authentication when enabled, setting appropriate environment variables for AWS passwordless authentication |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TFE_REDIS_USER = var.redis_passwordless_aws_use_instance_profile ? var.redis_passwordless_aws_iam_user : var.redis_user | ||
| TFE_REDIS_PASSWORD = var.redis_passwordless_aws_use_instance_profile ? null : var.redis_password |
Copilot
AI
Nov 25, 2025
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.
The condition var.redis_passwordless_aws_use_instance_profile is repeated multiple times throughout this file (lines 7, 8, 29). Consider extracting this into a local variable to improve maintainability and reduce duplication.
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 might be nice to group all items affected by the var.redis_passwordless_aws_use_instance_profile by moving the lines closer to each other.
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.
@nikolasrieble Updated.
| TFE_REDIS_SIDEKIQ_PASSWORDLESS_AWS_REGION = var.redis_passwordless_aws_region | ||
| TFE_REDIS_PASSWORDLESS_AWS_HOST_NAME = var.redis_passwordless_aws_host_name | ||
| TFE_REDIS_SIDEKIQ_PASSWORDLESS_AWS_HOST_NAME = var.redis_passwordless_aws_host_name | ||
| TFE_REDIS_SIDEKIQ_USER = var.redis_passwordless_aws_use_instance_profile ? var.redis_passwordless_aws_iam_user : var.redis_user |
Copilot
AI
Nov 25, 2025
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.
The TFE_REDIS_SIDEKIQ_USE_TLS variable is set unconditionally to var.redis_use_tls, but line 9 sets TFE_REDIS_USE_TLS with the same value. This appears to be newly added alongside AWS IAM configuration, but it's unclear why it wasn't already present. If this is fixing missing Sidekiq TLS configuration, it should be documented or split into a separate change; if it's unrelated to AWS IAM authentication, it may indicate incomplete configuration that existed before.
| TFE_REDIS_SIDEKIQ_USER = var.redis_passwordless_aws_use_instance_profile ? var.redis_passwordless_aws_iam_user : var.redis_user | |
| TFE_REDIS_SIDEKIQ_USER = var.redis_passwordless_aws_use_instance_profile ? var.redis_passwordless_aws_iam_user : var.redis_user | |
| # TFE_REDIS_SIDEKIQ_USE_TLS is set to var.redis_use_tls to ensure Sidekiq uses TLS if required. | |
| # This variable was added to fix missing Sidekiq TLS configuration; it is unrelated to AWS IAM authentication. |
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.
Great catch! Why is this change included in the PR?
❓ How is it possible that our release tests for redis_use_tls ever passed ✅, if we never passed this configuration here?
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.
The TFE_REDIS_SIDEKIQ_USE_TLS variable was indeed missing from the configuration before.
While implementing Redis passwordless authentication, I noticed that Sidekiq-specific Redis configuration was incomplete. TFE has separate Redis connections for main application and Sidekiq background jobs - Sidekiq wasn't getting explicit TLS configuration. Sidekiq likely has application-level fallback logic that inherits TLS settings when not explicitly configured.
it was discovered during Redis IAM work and both changes touch Redis config, I included it here with proper documentation explaining it's unrelated to IAM auth.
| variable "redis_passwordless_aws_region" { | ||
| type = string | ||
| description = "AWS Region of the AWS ElastiCache resource for Redis passwordless authentication." | ||
| default = null | ||
| } | ||
|
|
||
| variable "redis_passwordless_aws_host_name" { | ||
| type = string | ||
| description = "The name of the Redis instance on AWS for passwordless authentication." | ||
| default = null | ||
| } | ||
|
|
||
| variable "redis_passwordless_aws_iam_user" { | ||
| type = string | ||
| description = "The IAM username for Redis IAM authentication." | ||
| default = null | ||
| } |
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.
❓ Would it make sense to group these instead?
variable "redis_passwordless_config" {
type = object({
aws_region = string
host_name = string
iam_user = string
})
description = "Configuration for Redis passwordless authentication."
}
That might be a better user experience, and communicate that they should be set together.
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.
This would break existing module usage where these variables are already consumed.
nikolasrieble
left a comment
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.
The changes are fine, I have only minor comments to consider.
One thing thought deserve an independent follow up: This PR sets the unrelated var TFE_REDIS_SIDEKIQ_USE_TLS.
How come we missed this in the past? Do we have release tests for the USE_TLS feature? If yes, why would they ever pass, if we did not pass on this flag to SideKiq?
…n into local variable Address feedback from nikolasrieble and GitHub Copilot: - Extract var.redis_passwordless_aws_use_instance_profile into local.use_redis_passwordless_aws - Replace 5 occurrences of the repeated condition with the local variable - Improves maintainability and reduces duplication as suggested This makes the code easier to maintain if the condition logic ever needs to change.
Address Copilot feedback by adding comments explaining: - TFE_REDIS_SIDEKIQ_USE_TLS ensures Sidekiq uses TLS when required - This variable fixes missing Sidekiq TLS configuration discovered during IAM auth work - Addition is unrelated to AWS IAM authentication functionality This provides context for why Sidekiq-specific TLS configuration was added.
| TFE_REDIS_SIDEKIQ_PASSWORDLESS_AZURE_USE_MSI = var.redis_passwordless_azure_use_msi | ||
| TFE_REDIS_PASSWORDLESS_AZURE_CLIENT_ID = var.redis_passwordless_azure_client_id | ||
| TFE_REDIS_HOST = var.redis_use_tls != null ? var.redis_use_tls ? "${var.redis_host}:6380" : var.redis_host : null | ||
| TFE_REDIS_USER = local.use_redis_passwordless_aws ? var.redis_passwordless_aws_iam_user : var.redis_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.
I don't see the variable redis_passwordless_aws_iam_user coming from the terraform-fdo repo
|
Closing this as this changes in this repo is no longer relevant. |
These changes enable TFE to use AWS IAM authentication for Redis connections instead of password-based authentication.
Background
Added a spec for testing AWS redis passwordless
Part of AWS redis passwordless release test
Related PRs
terraform-enterprise # https://github.com/hashicorp/terraform-enterprise/pull/3149
terraform-terraform-enterprise-fdo: https://github.com/hashicorp/terraform-terraform-enterprise-fdo/pull/67
ptfedev-infra: https://github.com/hashicorp/ptfedev-infra/pull/885
How has this been tested?
CI/CD: https://github.com/hashicorp/terraform-enterprise/actions/runs/19707496447/job/56458820019
JIRA
Screenshots