-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(dashmate): various ZeroSSL cert verification errors #2339
Conversation
WalkthroughThe changes in this pull request focus on enhancing error handling within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (2)
180-188
: LGTM! Consider adding more detailed comments and loggingThe error handling for code 2831 (certificate not ready for verification) is well implemented. The solution to check the certificate status and proceed if it's already issued is a good approach.
Consider adding:
- More detailed comments explaining the ZeroSSL API behavior and why this specific error might occur when the certificate is actually issued
- Debug logging to help troubleshoot similar issues in the future
// Error: The given certificate is not ready for domain verification // Sometimes this error means that certificate is already verified if (e.code === 2831) { + // ZeroSSL API occasionally returns error 2831 even when the certificate + // is already issued. This is a known API inconsistency. const certificate = await getCertificate(ctx.apiKey, ctx.certificate.id); + // Log the actual certificate status for debugging + console.debug(`Certificate ${ctx.certificate.id} status: ${certificate.status}`); // Just proceed on certificate download if we see it's already issued. if (certificate.status === 'issued') { return; } }
194-208
: LGTM! Consider adding error type to debug logsThe error message and retry prompt are well-implemented with clear instructions about port configuration and firewall settings.
Consider adding the error type to debug logs for better troubleshooting:
if (ctx.noRetry !== true) { + console.debug(`Certificate verification failed with error type: ${e.type}`); retry = await task.prompt({ type: 'toggle', header: chalk` An error occurred during verification: {red ${e.message}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (3)
Learnt from: shumkov
PR: dashpay/platform#2297
File: packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:148-150
Timestamp: 2024-11-12T14:56:12.334Z
Learning: In the `obtainZeroSSLCertificateTaskFactory` function in `packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js`, the application already fails appropriately if `configFileRepository.write(configFile)` fails, so additional error handling around this operation is unnecessary.
Learnt from: shumkov
PR: dashpay/platform#2298
File: packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js:27-48
Timestamp: 2024-11-12T14:56:12.334Z
Learning: In `cleanupZeroSSLCertificatesTask` located in `packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js`, if the certificate listing API call fails, we prefer the entire command to fail without additional error handling.
Learnt from: shumkov
PR: dashpay/platform#2298
File: packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js:74-74
Timestamp: 2024-11-12T14:56:12.334Z
Learning: In the `cleanupZeroSSLCertificatesTask` function located in `packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js`, the team prefers to include a small fixed delay between all requests to avoid overloading ZeroSSL, rather than implementing exponential backoff.
Issue being fixed or feature implemented
During obtaining certificate users receive a sequence of errors even if certificate is successfully verified.
First error:
And on retry:
It seem like a ZeroSSL bug but we need to handle it.
What was done?
domain_control_validation_failed
errorthe given certificate is not ready for domain verification
error if status isissued
How Has This Been Tested?
Running locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes