Enhance error handling and import robustness in event_gate_lambda #62
Conversation
oto-macenauer-absa
requested review from
ABMC831,
Zejnilovic and
petr-pokorny-absa
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughAdds guarded imports for cryptography exceptions and writer modules, hardens token public key retrieval/deserialization with expanded exception handling and logging, raises RuntimeError on failure, and ensures Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor AWS as AWS Lambda
participant EG as event_gate_lambda
participant WR as writer_postgres
participant HT as Token Key HTTP Endpoint
participant CR as CryptographyLib
AWS->>EG: Cold start / import
EG->>EG: Guarded imports (UnsupportedAlgorithm, writers)
EG->>HT: GET /token-public-key
alt Success
HT-->>EG: Base64-encoded key
EG->>CR: base64 decode + load_der_public_key
CR-->>EG: Public key
EG->>WR: init(logger)
WR-->>EG: Initialized
EG-->>AWS: Ready
else Failure (requests/ValueError/KeyError/UnsupportedAlgorithm)
HT-->>EG: Error or invalid data
EG->>EG: logger.exception(...) / log error
EG-->>AWS: Raise RuntimeError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/event_gate_lambda.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
src/event_gate_lambda.py
36-36: Do not catch blind exception: Exception
(BLE001)
101-101: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
112-112: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Build and Test
src/event_gate_lambda.py
[error] 1-1: Command 'black --check $(git ls-files '*.py')' failed: 1 file would be reformatted.
🔇 Additional comments (2)
src/event_gate_lambda.py (2)
39-44: Guarded writer imports look good.The relative→absolute import fallback is appropriate for both package and ad-hoc execution contexts.
1-1: CI: Black failing — run Black locally and commit formatted code.CI reports Black formatting errors for src/event_gate_lambda.py. My verification run couldn't execute black (installed to ~/.local/bin, not on PATH). Run locally and commit the result:
python -m pip install --user black ruff
python -m black src/event_gate_lambda.py
python -m ruff check src/event_gate_lambda.py
| # Added explicit import for serialization-related exceptions | ||
| try: # pragma: no cover - import guard | ||
| from cryptography.exceptions import UnsupportedAlgorithm # type: ignore | ||
| except Exception: # pragma: no cover - very defensive | ||
| UnsupportedAlgorithm = Exception # type: ignore |
There was a problem hiding this comment.
Don't alias UnsupportedAlgorithm to Exception — it turns the later except into a catch‑all.
If the import fails, UnsupportedAlgorithm = Exception makes except (..., UnsupportedAlgorithm) equivalent to except Exception, hiding unrelated bugs. Catch only ImportError and provide a narrow fallback type.
Apply:
-try: # pragma: no cover - import guard
- from cryptography.exceptions import UnsupportedAlgorithm # type: ignore
-except Exception: # pragma: no cover - very defensive
- UnsupportedAlgorithm = Exception # type: ignore
+try: # pragma: no cover - import guard
+ from cryptography.exceptions import UnsupportedAlgorithm # type: ignore
+except ImportError: # pragma: no cover - only missing dependency
+ class UnsupportedAlgorithm(Exception): # narrow fallback; avoids catch-all later
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Added explicit import for serialization-related exceptions | |
| try: # pragma: no cover - import guard | |
| from cryptography.exceptions import UnsupportedAlgorithm # type: ignore | |
| except Exception: # pragma: no cover - very defensive | |
| UnsupportedAlgorithm = Exception # type: ignore | |
| # Added explicit import for serialization-related exceptions | |
| try: # pragma: no cover - import guard | |
| from cryptography.exceptions import UnsupportedAlgorithm # type: ignore | |
| except ImportError: # pragma: no cover - only missing dependency | |
| class UnsupportedAlgorithm(Exception): # narrow fallback; avoids catch-all later | |
| pass |
🧰 Tools
🪛 Ruff (0.12.2)
36-36: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In src/event_gate_lambda.py around lines 33 to 37, the current blanket except
and alias UnsupportedAlgorithm = Exception makes later except (...,
UnsupportedAlgorithm) behave like a catch‑all; change the import guard to only
catch ImportError (not Exception) and, if the import fails, define a narrow
fallback exception type (e.g. a small subclass of Exception named
UnsupportedAlgorithm) so downstream except clauses only match that specific type
rather than all exceptions.
| # Add timeout to avoid hanging requests; wrap in robust error handling so failures are explicit | ||
| try: | ||
| response_json = requests.get( | ||
| CONFIG["token_public_key_url"], verify=False, timeout=5 | ||
| ).json() # nosec external | ||
| token_public_key_encoded = response_json["key"] | ||
| TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key( | ||
| base64.b64decode(token_public_key_encoded) | ||
| ) | ||
| logger.debug("Loaded TOKEN_PUBLIC_KEY") | ||
| except (requests.RequestException, ValueError, KeyError, UnsupportedAlgorithm) as exc: | ||
| logger.exception( | ||
| "Failed to fetch or deserialize token public key from %s", CONFIG.get("token_public_key_url") | ||
| ) | ||
| raise RuntimeError("Token public key initialization failed") from exc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the HTTP path further: raise on non-2xx and catch TypeError. Optionally avoid verify=False.
Raise on bad status before attempting JSON; include TypeError for non-string/bytes base64 inputs. Consider making TLS verification configurable instead of permanently disabled (S501).
Apply:
-try:
- response_json = requests.get(
- CONFIG["token_public_key_url"], verify=False, timeout=5
- ).json() # nosec external
- token_public_key_encoded = response_json["key"]
- TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(
- base64.b64decode(token_public_key_encoded)
- )
- logger.debug("Loaded TOKEN_PUBLIC_KEY")
-except (requests.RequestException, ValueError, KeyError, UnsupportedAlgorithm) as exc:
+try:
+ url = CONFIG["token_public_key_url"]
+ resp = requests.get(url, verify=False, timeout=(3, 5)) # nosec external
+ resp.raise_for_status()
+ response_json = resp.json()
+ token_public_key_encoded = response_json["key"]
+ TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(
+ base64.b64decode(token_public_key_encoded)
+ )
+ logger.debug("Loaded TOKEN_PUBLIC_KEY")
+except (requests.RequestException, ValueError, KeyError, TypeError, UnsupportedAlgorithm) as exc:
logger.exception(
- "Failed to fetch or deserialize token public key from %s", CONFIG.get("token_public_key_url")
+ "Failed to fetch or deserialize token public key from %s", CONFIG.get("token_public_key_url")
)
raise RuntimeError("Token public key initialization failed") from excOptional follow-up (not in diff): make verification configurable, e.g., verify=CONFIG.get("token_public_key_verify_ssl", True).
🧰 Tools
🪛 Ruff (0.12.2)
101-101: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
112-112: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/event_gate_lambda.py around lines 98 to 112, the HTTP fetch should raise
on non-2xx responses and handle TypeError from base64 decoding, and TLS
verification should be configurable; modify the requests call to call
response.raise_for_status() (i.e., check status before .json()), add TypeError
to the except tuple alongside RequestException, ValueError, KeyError,
UnsupportedAlgorithm, and replace verify=False with a configurable flag (e.g.,
CONFIG.get("token_public_key_verify_ssl", True)) so TLS verification is not
permanently disabled.
This pull request enhances the robustness and reliability of the
src/event_gate_lambda.pymodule by improving error handling during imports and the initialization of the token public key. The changes ensure that failures are caught early and reported explicitly, which will help with debugging and stability.Improvements to import handling:
UnsupportedAlgorithmfromcryptography.exceptionswith a fallback to prevent crashes if the import fails.writer_eventbridge,writer_kafka,writer_postgres) to use a try/except block, providing a fallback for environments where relative imports may fail.Enhancements to token public key initialization:
RuntimeErroris raised.Release notes:
Summary by CodeRabbit
Bug Fixes
Chores