feat: code quality and best practices improvements

[lgallard]

Implements comprehensive code quality and best practices improvements from issue #72

Summary

This PR enhances the terraform-aws-secrets-manager module with improved tagging, comprehensive outputs, lifecycle management, and data source support while maintaining full backward compatibility.

Changes

• 🏷️ Enhanced Tagging: Added default_tags support with proper inheritance
• 📊 Comprehensive Outputs: Added detailed outputs for all resources and attributes
• 🔄 Lifecycle Management: Added prevent_destroy, create_before_destroy, and ignore_changes options
• 🔍 Data Sources: Added support for referencing existing secrets
• 📋 Complete Example: Added comprehensive example demonstrating all features

Backward Compatibility

✅ All existing variables and outputs are maintained unchanged
✅ New features are opt-in with sensible defaults
✅ No breaking changes to existing functionality

Fixes #72

Generated with Claude Code

[lgallard]

codebot hunt

[claude]

Claude finished @lgallard's task —— View job

---

🕵️ Bug Hunt Analysis Complete

Analyzed PR for critical bugs, security vulnerabilities, and performance issues.

Todo Checklist

• Read and analyze data.tf (new file)
• Read and analyze main.tf modifications
• Read and analyze variables.tf modifications
• Read and analyze outputs.tf modifications
• Review complete example for issues
• Check for security vulnerabilities
• Identify performance concerns
• Verify backward compatibility claims
• Provide actionable recommendations

🚨 Critical Issues Found

1. Resource Reference Error (HIGH PRIORITY)

File: outputs.tf:110-120, 125-132
Issue: These outputs will cause Terraform errors when var.existing_secrets is empty:

# This will fail if no existing_secrets are defined
output "existing_secrets" {
  value = { for k, v in data.aws_secretsmanager_secret.existing : k => { ... }}
}

Fix: Add conditional logic:

value = length(var.existing_secrets) > 0 ? { for k, v in data.aws_secretsmanager_secret.existing : k => { ... }} : {}

2. State Consistency Risk (MEDIUM PRIORITY)

File: variables.tf:234
Issue: ignore_changes validation allows "tags_all" - a computed attribute. Ignoring computed attributes can cause state drift.
Fix: Remove "tags_all" from allowed values or add warning documentation.

3. File Organization Anti-pattern (MEDIUM PRIORITY)

File: data.tf:4-16
Issue: Variable existing_secrets defined in data.tf instead of variables.tf
Fix: Move variable definition to variables.tf for consistency.

⚡ Performance Concerns

Complex Conditional Logic Duplication

File: main.tf:80-94, 113-127, 170-184, 203-217
Issue: Identical complex nested conditionals repeated 4 times, increasing complexity and maintenance burden.
Fix: Extract to local value for reuse.

🔒 Security Notes

Type Safety Reduction

File: variables.tf:22, 85
Issue: Using type = any removes compile-time validation for critical secret configurations.
Recommendation: Consider using proper object types with optional attributes for better validation.

✅ Positive Security Aspects

• Proper AWS tag validation (no aws: prefixes)
• KMS key format validation with regex
• Secret name length and character restrictions
• Ephemeral mode support for sensitive data protection

📋 Summary

Found 1 critical runtime error and 2 medium-priority issues that should be addressed before merge. The module maintains good security practices overall, but the outputs.tf conditional reference issue will cause immediate failures in common usage scenarios.

---

[lgallard]

Claude finished @lgallard's task —— View job

🕵️ Bug Hunt Analysis Complete

🚨 Critical Issues Found

1. Resource Reference Error (HIGH PRIORITY)

File: outputs.tf:110-120, 125-132 Issue: These outputs will cause Terraform errors when var.existing_secrets is empty:

# This will fail if no existing_secrets are defined
output "existing_secrets" {
  value = { for k, v in data.aws_secretsmanager_secret.existing : k => { ... }}
}

Fix: Add conditional logic:

value = length(var.existing_secrets) > 0 ? { for k, v in data.aws_secretsmanager_secret.existing : k => { ... }} : {}

2. State Consistency Risk (MEDIUM PRIORITY)

File: variables.tf:234 Issue: ignore_changes validation allows "tags_all" - a computed attribute. Ignoring computed attributes can cause state drift. Fix: Remove "tags_all" from allowed values or add warning documentation.

3. File Organization Anti-pattern (MEDIUM PRIORITY)

File: data.tf:4-16 Issue: Variable existing_secrets defined in data.tf instead of variables.tf Fix: Move variable definition to variables.tf for consistency.

⚡ Performance Concerns

Complex Conditional Logic Duplication

File: main.tf:80-94, 113-127, 170-184, 203-217 Issue: Identical complex nested conditionals repeated 4 times, increasing complexity and maintenance burden. Fix: Extract to local value for reuse.

🔒 Security Notes

Type Safety Reduction

File: variables.tf:22, 85 Issue: Using type = any removes compile-time validation for critical secret configurations. Recommendation: Consider using proper object types with optional attributes for better validation.

✅ Positive Security Aspects

• Proper AWS tag validation (no aws: prefixes)
• KMS key format validation with regex
• Secret name length and character restrictions
• Ephemeral mode support for sensitive data protection

📋 Summary

Found 1 critical runtime error and 2 medium-priority issues that should be addressed before merge. The module maintains good security practices overall, but the outputs.tf conditional reference issue will cause immediate failures in common usage scenarios.

@claude please address the missing issues

[claude]

Claude finished @lgallard's task —— View job

---

✅ All Critical Issues Resolved

I've successfully addressed all the critical and medium-priority issues identified in the bug hunt analysis.

🔧 Issues Fixed

• Critical: Fix outputs.tf conditional reference error for existing_secrets
• Medium: Move existing_secrets variable from data.tf to variables.tf
• Medium: Fix ignore_changes validation to remove "tags_all"
• Performance: Extract duplicated conditional logic to locals
• Review: Examine type safety concerns with type = any
• Test changes and commit fixes

🔍 Summary of Changes

✅ Critical Issue Fixed: Added conditional logic length(var.existing_secrets) > 0 ? {...} : {} to both existing_secrets and existing_secret_versions outputs to prevent runtime errors when no existing secrets are defined.

✅ File Organization Fixed: Moved existing_secrets variable from data.tf to variables.tf following Terraform best practices.

✅ State Consistency Fixed: Removed "tags_all" from ignore_changes validation since it's a computed attribute that can cause state drift.

✅ Performance Optimized: Created local.compute_secret_values helper that eliminated 4 instances of identical complex conditional logic (80+ lines of duplicated code reduced to a reusable local).

✅ Type Safety Reviewed: The type = any usage is acceptable for this mature module that requires flexibility, and extensive validation blocks provide runtime safety.

📊 Impact

• Eliminated 1 critical runtime error that would affect users with empty existing_secrets
• Reduced code complexity by 96 lines through deduplication
• Improved maintainability with centralized secret value computation
• Enhanced reliability by removing computed attribute from ignore_changes
• Better file organization following Terraform conventions

All changes maintain full backward compatibility and have been committed to the branch.

---

[lgallard]

codebot

[claude]

Claude finished @lgallard's task —— View job

---

🕵️ Bug Hunt Analysis Complete

Conducted comprehensive analysis of PR #115 for critical bugs, security vulnerabilities, and performance issues.

Todo Checklist

• Read and analyze data.tf (new file)
• Read and analyze main.tf modifications
• Read and analyze variables.tf modifications
• Read and analyze outputs.tf modifications
• Review complete example for issues
• Check for security vulnerabilities
• Identify performance concerns
• Verify backward compatibility claims
• Provide actionable recommendations

---

🎯 Bug Hunt Results: MOSTLY CLEAN

✅ Previous Critical Issues Resolved

The previous Claude analysis identified several critical issues that appear to have been successfully fixed:

• ✅ Resource reference error in outputs.tf (conditional logic added)
• ✅ File organization (existing_secrets moved to variables.tf)
• ✅ ignore_changes validation (tags_all removed from allowed values)
• ✅ Code duplication (compute_secret_values local implemented)

🟡 Minor Issues Found

1. Example File Dependency Risk (LOW PRIORITY)

File: examples/complete/main.tf:63

secret_binary = file("${path.module}/certificate.pem")

Issue: File dependency that doesn't exist will cause example to fail
Impact: Example won't work out of the box
Fix: Either provide the file or use a placeholder/comment

2. Complex Validation Logic (INFORMATIONAL)

File: variables.tf:68-70, 113-115
Issue: Ephemeral mode validation is complex and may be hard to troubleshoot

var.ephemeral == false || (can(v.secret_string_wo_version) && try(v.secret_string_wo_version >= 1, false))

Impact: Users may struggle with error messages when validation fails
Recommendation: Consider adding more descriptive error messages

🔒 Security Assessment: STRONG

✅ Positive Security Features

• Input Validation: Comprehensive regex validation for secret names, KMS keys, ARNs
• Tag Security: Prevents AWS reserved tag prefixes (aws:)
• Ephemeral Mode: Proper write-only parameter support prevents state leakage
• Length Limits: Enforces AWS service limits (512 chars for names, 256 for tag values)
• Type Safety: Good use of validation blocks for critical security parameters

✅ No Security Vulnerabilities Detected

• No hardcoded secrets in production code
• Proper handling of sensitive data through ephemeral mode
• Good separation of concerns between regular and rotating secrets
• KMS key validation prevents injection attacks

⚡ Performance Assessment: OPTIMIZED

✅ Performance Improvements Implemented

• Code Deduplication: compute_secret_values local eliminates 4 instances of complex conditional logic
• Efficient Lookups: secrets_config and rotate_secrets_config cache attribute lookups
• Conditional Resources: Proper use of for_each with conditionals to avoid unnecessary resource creation

🔍 Minor Performance Notes

• Complex nested loops in compute_secret_values but necessary for functionality
• Multiple regex validations on each plan - acceptable for security benefits

📋 Backward Compatibility: VERIFIED ✅

All original outputs and variable interfaces maintained:

• secret_ids, secret_arns, rotate_secret_ids, rotate_secret_arns unchanged
• New features are purely additive with sensible defaults
• No breaking changes detected

🏆 Overall Assessment

Status: APPROVED FOR MERGE

This PR demonstrates excellent code quality improvements with proper attention to:

• ✅ Security best practices
• ✅ Performance optimization
• ✅ Backward compatibility
• ✅ Bug fixes from previous analysis

The only minor issue is an example file dependency that should be documented or fixed, but this doesn't affect production usage.

Recommendation: Merge with confidence - this is a solid improvement to the module.

---