fix(api): Display correct cert error message#1355
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds SSL and sys imports, introduces targeted exception handling for SSL/load/file-not-found errors during API subprocess startup, ensures the parent process exits with status 1 if the API child fails, and adds two English locale entries for those error messages. ChangesAPI startup changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nettacker/api/engine.py (1)
616-626:⚠️ Potential issue | 🟠 MajorExit API mode after the child stops, not only on failures.
The API subprocess runs a long-running Flask server (app.run()). If the subprocess exits with code 0, the current code returns to
nettacker/core/arg_parser.pyand resumes target validation, creating an unexpected fallthrough path in API mode.Suggested fix
- while len(multiprocessing.active_children()) != 0: + while p.is_alive(): try: time.sleep(0.3) except KeyboardInterrupt: - for process in multiprocessing.active_children(): - process.terminate() + p.terminate() break - - if p.exitcode != 0: - # The child stopped. Exit the parent so it doesn't print "Cannot specify targets" - sys.exit(1) + p.join() + raise SystemExit(p.exitcode or 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/api/engine.py` around lines 616 - 626, The parent should exit when the API subprocess stops regardless of its exit code; replace the conditional block that only exits on non-zero exit codes with an unconditional exit using the child's exit code. Locate the variable p in nettacker/api/engine.py (the subprocess running the Flask server) and after the loop that waits for multiprocessing.active_children(), call sys.exit(p.exitcode) (or sys.exit(p.exitcode or 0) to guard against None) so the parent process doesn't continue into arg_parser.py when the API child has terminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nettacker/api/engine.py`:
- Around line 616-626: The parent should exit when the API subprocess stops
regardless of its exit code; replace the conditional block that only exits on
non-zero exit codes with an unconditional exit using the child's exit code.
Locate the variable p in nettacker/api/engine.py (the subprocess running the
Flask server) and after the loop that waits for
multiprocessing.active_children(), call sys.exit(p.exitcode) (or
sys.exit(p.exitcode or 0) to guard against None) so the parent process doesn't
continue into arg_parser.py when the API child has terminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 935030db-85cb-4ddd-ab2d-bf57d6720f48
📒 Files selected for processing (2)
nettacker/api/engine.pynettacker/locale/en.yaml
|
Hi @securestep9 and @arkid15r, please review when you have a chance. Thanks. |
|
@arkid15r and @securestep9 - can you review? Thanks. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nettacker/api/engine.py`:
- Around line 624-626: After the KeyboardInterrupt handler that terminates child
processes and breaks out of the loop, ensure you wait for the child process to
fully terminate before inspecting p.exitcode (use p.join() or p.wait(timeout) on
the process object referenced as p), then check that p.exitcode is not None and
!= 0; in other words, replace the direct check "if p.exitcode != 0" with a
sequence that joins/waits for p, then evaluates "if p.exitcode is not None and
p.exitcode != 0" (referencing the process variable p and the KeyboardInterrupt
handling block).
- Around line 595-596: The except clause currently catches (ssl.SSLError,
ValueError) which is too broad; update the handler in nettacker/api/engine.py to
only catch ssl.SSLError for the SSL certificate loading path (leave
die_failure(_("load_ssl_fail")) in place), removing ValueError from the tuple,
so unrelated Flask/ssl_context ValueErrors aren't misreported; other errors
(e.g., FileNotFoundError handled earlier and the generic Exception handler in
this function) will continue to handle remaining cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb700971-60a3-4d33-99d0-95c6d07cedf2
📒 Files selected for processing (2)
nettacker/api/engine.pynettacker/locale/en.yaml
Signed-off-by: jess-tech-lab <221731682+jess-tech-lab@users.noreply.github.com>
Signed-off-by: jess-tech-lab <221731682+jess-tech-lab@users.noreply.github.com>
jess-tech-lab
force-pushed
the
fix-cert-error
branch
from
d03f673 to
fd4d5e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nettacker/api/engine.py (1)
624-626:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRace condition:
p.exitcodemay beNoneafterKeyboardInterrupt.This issue was raised in a previous review and remains unresolved. After the
KeyboardInterrupthandler callsprocess.terminate()and breaks from the loop, the child processphasn't fully terminated yet (.terminate()is asynchronous). At that point,p.exitcodewill still beNone, causing the conditionp.exitcode != 0to evaluate toTrue(sinceNone != 0), which incorrectly triggerssys.exit(1)even for a clean Ctrl+C interruption.🔒 Recommended fix
Wait for the process to fully terminate before checking its exit code:
except KeyboardInterrupt: for process in multiprocessing.active_children(): process.terminate() break + p.join(timeout=5) # Wait for process to fully terminate - if p.exitcode != 0: + if p.exitcode is not None and p.exitcode > 0: # The child stopped. Exit the parent so it doesn't print "Cannot specify targets" sys.exit(1)Changes:
- Added
p.join(timeout=5)to wait for the process to terminate (with a timeout for safety)- Changed condition to
p.exitcode is not None and p.exitcode > 0to only exit with error when the child actually exited with a positive error code (notNone, not signal-based termination, not clean exit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nettacker/api/engine.py` around lines 624 - 626, The current check uses p.exitcode != 0 which treats None (still-running after process.terminate()) as an error; modify the logic to first wait for the child to fully terminate (call p.join(timeout=5) after process.terminate()/break) and then only call sys.exit(1) when p.exitcode is not None and greater than 0; reference the existing variables/methods p, process.terminate(), p.join(), p.exitcode and sys.exit to locate and implement the change.
🧹 Nitpick comments (1)
nettacker/api/engine.py (1)
595-598: ⚡ Quick winConsider adding
PermissionErrorhandling for completeness.The current implementation handles missing files (
FileNotFoundError) and invalid certificate content (ssl.SSLError), but doesn't specifically handle permission-denied errors when certificate files exist but can't be read. While such errors will fall through to the genericExceptionhandler, adding explicit handling would provide a more user-friendly error message.💡 Proposed enhancement
except ssl.SSLError: die_failure(_("load_ssl_fail")) except FileNotFoundError: die_failure(_("file_not_found")) + except PermissionError: + die_failure(_("permission_denied")) except Exception as e: die_failure(str(e))Note: This would require adding a
permission_deniedkey to the locale file (e.g., "Permission denied. Please check file permissions for your --api-cert and --api-cert-key files.").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nettacker/api/engine.py` around lines 595 - 598, Add explicit handling for PermissionError next to the existing except blocks that catch ssl.SSLError and FileNotFoundError (the same block that calls die_failure and uses _("...")), e.g., catch PermissionError and call die_failure(_("permission_denied")) so unreadable cert/key files produce a clear message; also add a "permission_denied" entry to the localization files with a short message like "Permission denied. Please check file permissions for your --api-cert and --api-cert-key files." Ensure this change is placed alongside the existing except handlers that currently reference die_failure and _("...") so behavior remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nettacker/api/engine.py`:
- Line 595: The except clause uses unnecessary parentheses: replace "except
(ssl.SSLError):" with "except ssl.SSLError:" to satisfy the formatter and avoid
the pipeline failure; locate the exception handler referencing ssl.SSLError in
nettacker/api/engine.py (the except block catching SSL exceptions) and remove
the surrounding parentheses so the handler reads "except ssl.SSLError:".
---
Duplicate comments:
In `@nettacker/api/engine.py`:
- Around line 624-626: The current check uses p.exitcode != 0 which treats None
(still-running after process.terminate()) as an error; modify the logic to first
wait for the child to fully terminate (call p.join(timeout=5) after
process.terminate()/break) and then only call sys.exit(1) when p.exitcode is not
None and greater than 0; reference the existing variables/methods p,
process.terminate(), p.join(), p.exitcode and sys.exit to locate and implement
the change.
---
Nitpick comments:
In `@nettacker/api/engine.py`:
- Around line 595-598: Add explicit handling for PermissionError next to the
existing except blocks that catch ssl.SSLError and FileNotFoundError (the same
block that calls die_failure and uses _("...")), e.g., catch PermissionError and
call die_failure(_("permission_denied")) so unreadable cert/key files produce a
clear message; also add a "permission_denied" entry to the localization files
with a short message like "Permission denied. Please check file permissions for
your --api-cert and --api-cert-key files." Ensure this change is placed
alongside the existing except handlers that currently reference die_failure and
_("...") so behavior remains consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88c34b12-f1dc-4f4b-ad09-ecabd07250bb
📒 Files selected for processing (2)
nettacker/api/engine.pynettacker/locale/en.yaml
✅ Files skipped from review due to trivial changes (1)
- nettacker/locale/en.yaml
jess-tech-lab
force-pushed
the
fix-cert-error
branch
from
fd4d5e4 to
4bc479b
Compare
|
@securestep9 - I've addressed the Gemini review suggestions. Would you please take another look? Thanks! |
2 participants
Proposed change
ssl.SSLErrorandValueErrorduring the Flask server initialization.en.yamlto provide user-friendly feedback when certificate loading fails, replacing generic tracebacks with actionable error messages.sys.exit(1)withinstart_api_server. If the child process terminates with a non-zero exit code (e.g., due to an SSL PEM lib error), the parent process now callssys.exit(1)immediately. This prevents the misleadingCannot specify the target(s)error.With invalid cert files,
With wrong cert paths,
Closes #1354
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folder