refactor: reorganize ElastiCache KMS key handling in midaz configuration - #29
Conversation
guimoreirar
commented
Mar 25, 2026
- Moved ElastiCacheKMSKeyArn parameter back to application.yaml for consistency.
- Added a function in helm.yaml to download and return the AWS CA certificate as a base64-encoded string for ElastiCache TLS connections.
- Updated environment variables in helm.yaml to include the new REDIS_CA_CERT for secure Redis connections.
- Moved ElastiCacheKMSKeyArn parameter back to application.yaml for consistency. - Added a function in helm.yaml to download and return the AWS CA certificate as a base64-encoded string for ElastiCache TLS connections. - Updated environment variables in helm.yaml to include the new REDIS_CA_CERT for secure Redis connections.
WalkthroughThe changes touch two files. In application.yaml the Sequence Diagram(s)mermaid 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@products/midaz/helm.yaml`:
- Around line 487-500: The get_aws_ca_cert_base64 function uses
urllib.request.urlretrieve without a timeout and returns an empty string on
failure; update it to perform the download with a bounded timeout (e.g., use
urllib.request.urlopen or requests with a timeout parameter against ca_url) and
read the response into ca_pem, and change the error handling to log the full
exception and fail loudly (raise the exception or return a clear error) instead
of returning an empty string so REDIS_CA_CERT consumers don't get a silent empty
value; ensure ca_path/temporary file handling is safe and cleaned up if you keep
file-based logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cc73ee7-aea2-43dc-99fb-d20693d0882b
📒 Files selected for processing (2)
products/midaz/application.yamlproducts/midaz/helm.yaml
- Replaced the method of downloading the AWS CA bundle with a direct URL fetch to enhance reliability. - Updated error handling to log errors as critical and raise exceptions for better visibility in case of failures.
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
products/midaz/helm.yaml (1)
487-498: 🧹 Nitpick | 🔵 TrivialUse context manager for proper resource cleanup.
The
urlopenresponse should be properly closed to avoid resource leaks. Without a context manager, the connection may not be released promptly.♻️ Proposed fix using context manager
def get_aws_ca_cert_base64(): """Download Amazon root CA bundle and return as base64-encoded string. Required for ElastiCache TLS connections (REDIS_CA_CERT expects base64-encoded PEM).""" ca_url = 'https://truststore.pki.rds.amazonaws.com/global/global-bundle.pem' try: - resp = urllib.request.urlopen(ca_url, timeout=30) - ca_pem = resp.read() + with urllib.request.urlopen(ca_url, timeout=30) as resp: + ca_pem = resp.read() return base64.b64encode(ca_pem).decode('utf-8') except Exception as e: logger.error(f"Failed to download AWS CA bundle: {e}") raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@products/midaz/helm.yaml` around lines 487 - 498, The get_aws_ca_cert_base64 function leaves the urlopen response open; change the implementation to use a context manager so the response is always closed (e.g., "with urllib.request.urlopen(ca_url, timeout=30) as resp:"), read resp inside that block to produce ca_pem, then base64-encode and return; keep the existing exception handling (logger.error and re-raise) but ensure resource cleanup via the with statement around urllib.request.urlopen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@products/midaz/helm.yaml`:
- Around line 487-498: The get_aws_ca_cert_base64 function leaves the urlopen
response open; change the implementation to use a context manager so the
response is always closed (e.g., "with urllib.request.urlopen(ca_url,
timeout=30) as resp:"), read resp inside that block to produce ca_pem, then
base64-encode and return; keep the existing exception handling (logger.error and
re-raise) but ensure resource cleanup via the with statement around
urllib.request.urlopen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc46437d-b4f2-4a3d-8f76-455f80d8b05b
📒 Files selected for processing (1)
products/midaz/helm.yaml