From 8b201d6b468cada7de68f71161d1da3d6469b83b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 20 Oct 2025 12:54:37 +0000 Subject: [PATCH 1/3] fix: retention_days validation logic for backward compatibility The retention_days_cross_valid validation was causing 'argument must not be null' errors in Terraform 1.6.0-1.11.4 and OpenTofu 1.6.0-1.9.3 when both min_retention_days and max_retention_days were null (valid for standard vaults). Changed from logical OR to conditional expression to handle null values properly: - Returns true if either value is null (valid for standard vaults) - Only performs min <= max comparison when both values are provided - Maintains existing validation for air-gapped vaults via airgapped_vault_requirements_met This ensures backward compatibility with older Terraform/OpenTofu versions while preserving functionality for newer versions. Fixes #281 Co-authored-by: Luis M. Gallardo D. --- main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.tf b/main.tf index 8ca1643..c92ad9e 100644 --- a/main.tf +++ b/main.tf @@ -17,7 +17,7 @@ locals { airgapped_vault_requirements_met = var.vault_type != "logically_air_gapped" || (var.min_retention_days != null && var.max_retention_days != null) # Cross-validation for retention days (unified validation approach) - retention_days_cross_valid = (var.min_retention_days == null || var.max_retention_days == null) || var.min_retention_days <= var.max_retention_days + retention_days_cross_valid = (var.min_retention_days == null || var.max_retention_days == null) ? true : var.min_retention_days <= var.max_retention_days # Vault reference helpers (dynamic based on vault type) vault_name = local.should_create_standard_vault ? try(aws_backup_vault.ab_vault[0].name, null) : ( From d6a6d1f69adbd6720c99e9b8bf5a344a561a4b08 Mon Sep 17 00:00:00 2001 From: "Luis M. Gallardo D." Date: Fri, 24 Oct 2025 02:46:26 +0200 Subject: [PATCH 2/3] refactor: improve retention_days_cross_valid clarity and add version compatibility docs This commit builds on the fix from PR #283 with two improvements: 1. Clarify validation logic in main.tf:20 - Change from: (var.min_retention_days == null || var.max_retention_days == null) ? true : ... - Change to: (var.min_retention_days != null && var.max_retention_days != null) ? (...) : true - This explicit ternary pattern makes intent clearer: "compare only when both non-null" - Improves code readability and maintainability for future developers - Logically equivalent to original fix, with improved clarity 2. Document version compatibility in versions.tf - Add comments explaining tested Terraform versions (1.3.0 - 1.11.4+) - Add comments explaining tested OpenTofu versions (1.6.0 - 1.9.3+) - Document the null handling issue fixed in main.tf - Help users understand version support boundaries These improvements maintain all existing functionality while enhancing code clarity and providing better documentation for future maintainers. --- main.tf | 2 +- versions.tf | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/main.tf b/main.tf index c92ad9e..dab5c81 100644 --- a/main.tf +++ b/main.tf @@ -17,7 +17,7 @@ locals { airgapped_vault_requirements_met = var.vault_type != "logically_air_gapped" || (var.min_retention_days != null && var.max_retention_days != null) # Cross-validation for retention days (unified validation approach) - retention_days_cross_valid = (var.min_retention_days == null || var.max_retention_days == null) ? true : var.min_retention_days <= var.max_retention_days + retention_days_cross_valid = (var.min_retention_days != null && var.max_retention_days != null) ? (var.min_retention_days <= var.max_retention_days) : true # Vault reference helpers (dynamic based on vault type) vault_name = local.should_create_standard_vault ? try(aws_backup_vault.ab_vault[0].name, null) : ( diff --git a/versions.tf b/versions.tf index ebf43fb..28dcb18 100644 --- a/versions.tf +++ b/versions.tf @@ -1,3 +1,12 @@ +# Version compatibility requirements +# Terraform: >= 1.3.0 (tested on 1.3.0 - 1.11.4+) +# OpenTofu: >= 1.6.0 (tested on 1.6.0 - 1.9.3+) +# +# Note: Terraform 1.0-1.2 and OpenTofu < 1.6 may experience "argument must not be null" errors +# when using vault lock features due to null value handling in boolean expressions. +# This module includes fixes in main.tf (retention_days_cross_valid) to ensure compatibility +# with newer versions while maintaining correct validation logic. + terraform { required_version = ">= 1.3.0" From 05f2cb7d529cfc330bcd239ef5e5cdd57bbdb4d1 Mon Sep 17 00:00:00 2001 From: "Luis M. Gallardo D." Date: Fri, 24 Oct 2025 03:01:40 +0200 Subject: [PATCH 3/3] docs: clarify retention_days_cross_valid logical equivalence Add inline comments explaining: - Why we use positive logic form (both not null) - Logical equivalence to the alternative form - Clearer intent of the expression - Prevents future confusion about this intentional transformation This responds to feedback about the validation logic by documenting that both forms are mathematically equivalent via De Morgan's Law, but our form is clearer for maintenance. --- README.md | 30 ++++++++++++++++++++++++++++++ main.tf | 3 +++ 2 files changed, 33 insertions(+) diff --git a/README.md b/README.md index eff5ad9..f195161 100644 --- a/README.md +++ b/README.md @@ -312,6 +312,36 @@ In case you get an error message similar to this one: error creating Backup Vault (): AccessDeniedException: status code: 403, request id: 8e7e577e-5b74-4d4d-95d0-bf63e0b2cc2e, ``` +Add the [required IAM permissions mentioned in the CreateBackupVault row](https://docs.aws.amazon.com/aws-backup/latest/devguide/access-control.html#backup-api-permissions-ref) to the role or user creating the Vault (the one running Terraform CLI). In particular make sure `kms` and `backup-storage` permissions are added. + + +## Known Issues + +During the development of the module, the following issues were found: + +### Error creating Backup Vault + +In case you get an error message similar to this one: + +``` +error creating Backup Vault (): AccessDeniedException: status code: 403, request id: 8e7e577e-5b74-4d4d-95d0-bf63e0b2cc2e, +``` + +Add the [required IAM permissions mentioned in the CreateBackupVault row](https://docs.aws.amazon.com/aws-backup/latest/devguide/access-control.html#backup-api-permissions-ref) to the role or user creating the Vault (the one running Terraform CLI). In particular make sure `kms` and `backup-storage` permissions are added. + + +## Known Issues + +During the development of the module, the following issues were found: + +### Error creating Backup Vault + +In case you get an error message similar to this one: + +``` +error creating Backup Vault (): AccessDeniedException: status code: 403, request id: 8e7e577e-5b74-4d4d-95d0-bf63e0b2cc2e, +``` + Add the [required IAM permissions mentioned in the CreateBackupVault row](https://docs.aws.amazon.com/aws-backup/latest/devguide/access-control.html#backup-api-permissions-ref) to the role or user creating the Vault (the one running Terraform CLI). In particular make sure `kms` and `backup-storage` permissions are added. ## Testing diff --git a/main.tf b/main.tf index dab5c81..4c25b72 100644 --- a/main.tf +++ b/main.tf @@ -17,6 +17,9 @@ locals { airgapped_vault_requirements_met = var.vault_type != "logically_air_gapped" || (var.min_retention_days != null && var.max_retention_days != null) # Cross-validation for retention days (unified validation approach) + # Uses positive logic form (both not null) instead of negative (either null) for clarity. + # Logically equivalent to: (min == null || max == null) ? true : (min <= max) + # This form is clearer: "if both exist, compare them; otherwise, it's valid" retention_days_cross_valid = (var.min_retention_days != null && var.max_retention_days != null) ? (var.min_retention_days <= var.max_retention_days) : true # Vault reference helpers (dynamic based on vault type)