GCP memory-store module fixes

[srikxcipher]

GCP memory-store module fix

[Copilot]

Pull request overview

This pull request adds TLS encryption support to the GCP Memorystore Redis module and makes the network input mandatory. The changes introduce a new security configuration section that allows enabling/disabling TLS encryption (in-transit encryption), with TLS enabled by default for production security.

Key changes:

• Added configurable TLS encryption with enable_tls flag in security configuration
• Made network input mandatory (changed from optional to required)
• Enhanced outputs to expose TLS-related attributes including server CA certificates and connection strings with correct scheme (rediss:// vs redis://)

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
variables.tf	Added security object with enable_tls field; changed network input from optional to required
outputs.tf	Enhanced output attributes with TLS-related fields; added dynamic connection string format based on TLS status
main.tf	Implemented TLS configuration with conditional transit_encryption_mode; removed optional network fallback logic
locals.tf	Simplified local variables; added enable_tls from spec; removed network module fallback logic
facets.yaml	Bumped version to 1.2; added security configuration spec with enable_tls; made network input non-optional; changed default Redis version to 7.0
README.md	Comprehensive documentation covering TLS configuration, performance considerations, client setup, and troubleshooting

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Pull Request Review: GCP Memory-store Module Fixes

This PR introduces comprehensive improvements to the GCP Memorystore Redis module, adding TLS encryption support, improving documentation, and refactoring configuration. Well-documented with +301/-121 changes across 6 files.

Strengths

Code Quality

• Clear separation of concerns with security configuration properly isolated
• Good use of locals for clean abstraction
• Comprehensive documentation with detailed TLS setup and performance considerations
• Proper validation for memory size with STANDARD_HA tier

Security Improvements

• Major enhancement with configurable in-transit encryption
• Secure defaults with TLS enabled by default
• Proper use of sensitive() for auth tokens and certificates
• Enforced private service access

Documentation

• Excellent README with comprehensive TLS configuration coverage
• Well-documented connection limit tables and version recommendations
• Practical examples and troubleshooting guide

CRITICAL Issues

1. Lifecycle Management Anti-pattern (main.tf:44-63)

The ignore_changes block is extremely broad, ignoring tier, memory_size_gb, redis_version, replica_count, and read_replicas_mode.

Problems: Users update config but nothing happens. Manual GCP console changes won't be detected. Terraform state becomes out of sync.

Fix: Remove tier, memory_size_gb, and user-configurable fields. Keep only immutable fields: name, region, location_id, authorized_network, connect_mode, transit_encryption_mode.

2. Missing Sensitive Marking (outputs.tf:18)

The cluster object contains auth_token and connection_string but values are not wrapped in sensitive(). Terraform may log these in plan output.

Fix: Wrap auth_token and connection_string with sensitive() function.

3. Missing TLS Certificate in Outputs (outputs.tf:18)

README documents clients need server_ca_certs when TLS is enabled, but cluster interface doesn't expose this field.

Fix: Add server_ca_certs field and tls_enabled flag to cluster interface.

Other Issues

4. Hardcoded Zone (locals.tf:28) - Always uses zone a. Make configurable or document why.

5. Missing Validation - README mentions 1-300GB limits but not enforced in variables.tf.

6. Breaking Change - network changed from optional to required in facets.yaml but not highlighted in PR description.

7. Redundant Secrets - server_ca_certs in both secrets arrays. Clarify if intentional.

Summary

Must Fix (Before Merge)

1. Refactor ignore_changes - Remove user-configurable fields
2. Fix output sensitivity - Mark sensitive values
3. Add server_ca_certs to cluster output

Should Fix

4. Make location_id configurable or document assumption
5. Document breaking change (network required)
6. Add memory size validation (1-300GB)

Nice to Have

7. Add tests (terraform validate, plan tests)
8. Add upgrade guide
9. Consider customer-managed encryption keys

Overall Assessment: High-quality PR with excellent documentation and important security features. Main concerns are lifecycle management and output handling. With critical fixes, this will be production-ready.

Great work on TLS implementation and documentation!

[mbhandari31]

remove server_ca_certs from interfaces as it not here