fix(dashmate): cleanup zerossl certs command #2298
Conversation
WalkthroughA new command class Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
packages/dashmate/src/commands/ssl/cleanup.js (1)
6-10: Enhance command description with account limits and impact.Consider adding more context to help users understand the importance and impact of this command.
static description = `Cleanup Zero SSL certificate -Cancel all drafted or pending validation certificates on ZeroSSL +Cancel all drafted or pending validation certificates on ZeroSSL. + +This command helps prevent reaching ZeroSSL certificate limits: +- Free accounts: 3 certificates +- Paid accounts: 10,000 certificates + +Note: Only draft and pending validation certificates will be removed. `;packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js (1)
72-72: Enhance progress output for clarityMake the progress output more descriptive to improve the user experience.
Apply this diff to update the progress message:
- observer.next(chalk`{green ${ctx.canceled}} / {red ${ctx.errored}} / ${ctx.total}`); + observer.next(chalk`Canceled: {green ${ctx.canceled}}, Failed: {red ${ctx.errored}}, Total: ${ctx.total}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/dashmate/src/commands/ssl/cleanup.js(1 hunks)packages/dashmate/src/createDIContainer.js(3 hunks)packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js(1 hunks)packages/dashmate/src/ssl/zerossl/cancelCertificate.js(1 hunks)packages/dashmate/src/ssl/zerossl/listCertificates.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/dashmate/src/ssl/zerossl/cancelCertificate.js
🔇 Additional comments (13)
packages/dashmate/src/ssl/zerossl/listCertificates.js (2)
11-11: LGTM! Documentation is clear and consistent.
The JSDoc update properly documents the new page parameter while maintaining the existing documentation style.
16-17: Consider enhancing the pagination implementation.
The pagination implementation looks good and maintains backward compatibility. However, consider these improvements:
- Make the page size limit configurable instead of hard-coding 1000
- Add input validation for the page parameter
- Add error handling for cases where the response doesn't contain results
Here's a suggested implementation:
- export default async function listCertificates(apiKey, statuses = [], page = 1, search = undefined) {
- let url = `https://api.zerossl.com/certificates?access_key=${apiKey}&limit=1000&page=${page}`;
+ const DEFAULT_PAGE_SIZE = 1000;
+
+ export default async function listCertificates(apiKey, statuses = [], page = 1, search = undefined, pageSize = DEFAULT_PAGE_SIZE) {
+ if (!Number.isInteger(page) || page < 1) {
+ throw new Error('Page must be a positive integer');
+ }
+
+ let url = `https://api.zerossl.com/certificates?access_key=${apiKey}&limit=${pageSize}&page=${page}`;Let's verify the response structure handling across the codebase:
packages/dashmate/src/commands/ssl/cleanup.js (3)
1-4: LGTM! Clean and necessary imports.
The imports are well-organized and include all required dependencies for command implementation, task execution, and error handling.
12-15: LGTM! Well-structured command flags.
The verbose flag is properly implemented with clear description and appropriate default value.
52-56: Verify successful cleanup of certificates.
The error handling is good, but we should verify that the cleanup was successful.
packages/dashmate/src/createDIContainer.js (4)
26-28: LGTM! Clean import additions.
The new imports follow the existing code organization pattern and maintain consistency with the project structure.
212-212: LGTM! Proper SSL function registration.
The cancelCertificate is correctly registered as a value in the SSL section, consistent with other similar SSL-related function registrations.
306-306: LGTM! Task registration follows patterns.
The cleanupZeroSSLCertificatesTask is properly registered as a singleton function, maintaining consistency with other task registrations in the container.
Line range hint 26-306: Verify integration with cleanup command.
The container changes look good and support the new cleanup functionality. Let's verify that the cleanup command properly utilizes these registered dependencies.
✅ Verification successful
Let me gather more information about the cleanup command implementation to verify the proper integration.
Dependencies are properly integrated and utilized
The verification confirms that:
- The cleanup command correctly imports and uses the registered dependencies
- The implementation follows the established patterns:
- Uses dependency injection through the container
- Properly utilizes
cleanupZeroSSLCertificatesTaskandcancelCertificate - Implements proper error handling and progress reporting
- The task factory correctly consumes the injected dependencies and implements the cleanup logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the cleanup command properly imports and uses these dependencies
# Test 1: Check if cleanup command imports these dependencies
rg -l "cleanupZeroSSLCertificatesTask|cancelCertificate" "packages/dashmate/src/commands/"
# Test 2: Check the cleanup command implementation
rg -A 10 "class.*Cleanup.*Command" "packages/dashmate/src/commands/"
Length of output: 1058
Script:
#!/bin/bash
# Check the complete implementation of cleanup command
cat packages/dashmate/src/commands/ssl/cleanup.js
# Check the implementation of cleanupZeroSSLCertificatesTaskFactory
cat packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Length of output: 4570
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js (4)
74-74: Review the necessity of the fixed delay in the loop
The 100ms delay between cancellations may slow down the process unnecessarily. Confirm if this delay is needed and remove it if not.
36-44: Confirm the pagination limit and make it configurable
Hardcoding the pagination limit to 1000 may lead to issues if the ZeroSSL API changes its limits. Consider making the limit a constant or fetching it dynamically.
43-43:
Fix string interpolation in task output
To properly interpolate ctx.certificates.length, use backticks instead of single quotes.
Apply this diff to fix the string interpolation:
- task.output = 'Found ${ctx.certificates.length} certificates';
+ task.output = `Found ${ctx.certificates.length} certificates`;Likely invalid or redundant comment.
54-54:
Correct string interpolation in task title
Use backticks for string interpolation to display the correct number of certificates.
Apply this diff:
- task.title = 'Cancel ${ctx.certificates.length} certificates';
+ task.title = `Cancel ${ctx.certificates.length} certificates`;Likely invalid or redundant comment.
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Outdated
Show resolved
Hide resolved
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Outdated
Show resolved
Hide resolved
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Show resolved
Hide resolved
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js (1)
Line range hint
62-64: Consider implementing the suggested dynamic configuration update.The TODO comment suggests using Envoy's dynamic filesystem configuration, which would be a valuable improvement. This could eliminate the need for Gateway restarts when certificates are updated.
Would you like me to create a GitHub issue to track the implementation of dynamic configuration updates using Envoy's filesystem-based configuration?
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js (2)
86-94: Consider adding more detailed renderer options for better progress trackingThe current renderer configuration could be enhanced to provide better visibility of the cleanup progress.
Apply this diff to add more renderer options:
options: { persistentOutput: true, + bottomBar: true, }, }, ], { rendererOptions: { showErrorMessage: true, + collapseErrors: false, + collapseSubtasks: false, }, });
20-21: Add a warning message before certificate cleanupSince this is a destructive operation that cancels certificates, it would be helpful to add a warning message.
Add a new task at the beginning of the Listr tasks:
function cleanupZeroSSLCertificatesTask(config) { const apiKey = config.get('platform.gateway.ssl.providerConfigs.zerossl.apiKey', true); return new Listr([ + { + title: 'Warning', + task: (_, task) => { + task.output = chalk.yellow( + 'This operation will cancel all draft and pending validation certificates.\n' + + 'This action cannot be undone.' + ); + }, + options: { persistentOutput: true }, + }, { title: 'Collect drafted and pending validation certificates',packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (3)
Line range hint
123-134: Add cleanup of draft certificates before creating new ones.To prevent accumulation of draft certificates (as mentioned in PR #2297), consider adding a cleanup step before creating a new certificate. This would help prevent hitting account limits, especially for free accounts limited to three certificates.
{ title: 'Create a certificate', skip: (ctx) => ctx.certificate, task: async (ctx) => { + // Clean up any existing draft certificates before creating a new one + const certificates = await listCertificates(ctx.apiKey); + const draftCerts = certificates.filter(cert => + ['draft', 'pending_validation'].includes(cert.status)); + for (const cert of draftCerts) { + await cancelCertificate(cert.id, ctx.apiKey); + } + ctx.certificate = await createZeroSSLCertificate( ctx.csr, ctx.externalIp, ctx.apiKey,
Line range hint
190-217: Consider making retry parameters configurable.The certificate download retry mechanism uses hard-coded values:
- 50 retry attempts
- 5-second wait between attempts
- Total potential wait time: 250 seconds
Consider making these parameters configurable to allow users to adjust based on their needs and network conditions.
+ const DEFAULT_DOWNLOAD_RETRY_COUNT = 50; + const DEFAULT_DOWNLOAD_RETRY_DELAY = 5000; { title: 'Download certificate file', skip: (ctx) => ctx.isBundleFilePresent, task: async (ctx, task) => { - for (let retry = 0; retry <= 50; retry += 1) { - await wait(5000); + const retryCount = ctx.downloadRetryCount || DEFAULT_DOWNLOAD_RETRY_COUNT; + const retryDelay = ctx.downloadRetryDelay || DEFAULT_DOWNLOAD_RETRY_DELAY; + for (let retry = 0; retry <= retryCount; retry += 1) { + await wait(retryDelay);
Line range hint
218-249: Set secure file permissions for sensitive files.The code writes sensitive cryptographic materials (private key, CSR, certificate) without explicitly setting secure file permissions. This could potentially expose these files if default permissions are too permissive.
{ title: 'Save certificate private key file', enabled: (ctx) => !ctx.isPrivateKeyFilePresent, task: async (ctx, task) => { - fs.writeFileSync(ctx.privateKeyFilePath, ctx.privateKeyFile, 'utf8'); + fs.writeFileSync(ctx.privateKeyFilePath, ctx.privateKeyFile, { + encoding: 'utf8', + mode: 0o600 // Read/write for owner only + }); // eslint-disable-next-line no-param-reassign task.output = ctx.privateKeyFilePath; }, }, { title: 'Save certificate request file', enabled: (ctx) => !ctx.isCsrFilePresent, task: async (ctx, task) => { - fs.writeFileSync(ctx.csrFilePath, ctx.csr, 'utf8'); + fs.writeFileSync(ctx.csrFilePath, ctx.csr, { + encoding: 'utf8', + mode: 0o644 // World-readable but only owner-writable + }); // eslint-disable-next-line no-param-reassign task.output = ctx.csrFilePath; }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/dashmate/src/commands/ssl/cleanup.js(1 hunks)packages/dashmate/src/commands/ssl/obtain.js(1 hunks)packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js(1 hunks)packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js(1 hunks)packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dashmate/src/commands/ssl/cleanup.js
🧰 Additional context used
📓 Learnings (2)
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js (3)
Learnt from: shumkov
PR: dashpay/platform#2298
File: packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js:77-81
Timestamp: 2024-10-31T15:44:50.617Z
Learning: In the `cleanupZeroSSLCertificatesTask`, if some certificates are not canceled, we want to abort listr, as it's the last task.
Learnt from: shumkov
PR: dashpay/platform#2298
File: packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js:66-69
Timestamp: 2024-10-31T15:43:16.094Z
Learning: In Listr tasks, when in debug mode, prefer using `console.warn` for logging instead of `task.output`.
Learnt from: shumkov
PR: dashpay/platform#2298
File: packages/dashmate/src/commands/ssl/cleanup.js:33-50
Timestamp: 2024-10-31T15:37:28.986Z
Learning: In `packages/dashmate/src/commands/ssl/cleanup.js`, the `cleanupZeroSSLCertificatesTask` function already includes progress reporting within its nested `Listr` tasks, so additional progress reporting in the `CleanupCommand` is unnecessary.
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (1)
Learnt from: shumkov
PR: dashpay/platform#2297
File: packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:148-150
Timestamp: 2024-10-31T14:41:42.158Z
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.
🔇 Additional comments (3)
packages/dashmate/src/commands/ssl/obtain.js (1)
48-48: Verify async handling in obtainZeroSSLCertificateTask
The removal of async is correct if obtainZeroSSLCertificateTask now returns a Listr instance directly. However, let's verify the implementation to ensure proper async operation handling.
✅ Verification successful
Removal of async is correct and safe
The removal of async from the task definition is correct because obtainZeroSSLCertificateTask directly returns a Listr instance without any async operations. The async operations are properly handled within individual task definitions using async/await in the task handlers. The function itself is synchronous and serves as a task factory.
Key points verified:
- Task factory returns a
Listrinstance directly - All async operations are properly contained within individual task handlers
- No top-level await operations that would require the factory to be async
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of obtainZeroSSLCertificateTask
# to ensure it properly handles async operations after the refactor
# Search for the task factory implementation
ast-grep --pattern 'obtainZeroSSLCertificateTask = $$$'
# Look for any Listr usage in the implementation
rg -A 5 'new Listr' packages/dashmate/src/ssl
Length of output: 105
Script:
#!/bin/bash
# Let's try a different approach to find the implementation
# Search for the file containing obtainZeroSSLCertificateTask
rg -l "obtainZeroSSLCertificateTask" packages/dashmate/src
# Search for the implementation with more context
rg -B 2 -A 10 "obtainZeroSSLCertificateTask" packages/dashmate/src
# Look for Listr usage in SSL-related files
fd . packages/dashmate/src/ssl --type f --exec rg -l "Listr" {}
Length of output: 19735
Script:
#!/bin/bash
# Let's examine the implementation of obtainZeroSSLCertificateTaskFactory to verify async handling
# Get the full content of the task factory implementation
cat packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js
Length of output: 10391
packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js (1)
54-57: Verify task execution behavior change.
The removal of await from task creation while maintaining it in tasks.run() appears intentional. This change aligns with the PR's goal of improving certificate handling. However, we should verify that error propagation still works as expected.
✅ Verification successful
Task execution behavior change is safe and consistent
The removal of await from obtainZeroSSLCertificateTask(config) is correct as the task factory returns a Listr instance that is properly awaited in the run() call. This pattern is consistently used across the codebase, where task factories create Listr instances that are executed later via run(). The change aligns with the codebase's task execution pattern and maintains proper error propagation through Listr's task management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other task factories to confirm consistency
ast-grep --pattern 'const tasks = $_($$$);
await tasks.run($$$);'
Length of output: 69
Script:
#!/bin/bash
# Let's try a broader search for task execution patterns
rg -U "const tasks.*\n.*await.*run" --type js
# Also search for obtainZeroSSLCertificateTask usage
rg "obtainZeroSSLCertificateTask" --type js -A 3
# And check the implementation of obtainZeroSSLCertificateTask
ast-grep --pattern 'const obtainZeroSSLCertificateTask = $_($$$)'
Length of output: 7508
Script:
#!/bin/bash
# Let's examine the implementation of obtainZeroSSLCertificateTaskFactory to understand the task structure
cat packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js
# Also check the Listr task implementation pattern in other similar files
rg "new Listr\(\[" --type js -A 3
Length of output: 22483
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (1)
39-39: LGTM: Return type change is correct.
The change from Promise<Listr> to Listr better reflects the actual behavior where task creation is synchronous while task execution remains asynchronous.
Also applies to: 41-41
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Show resolved
Hide resolved
packages/dashmate/src/listr/tasks/ssl/zerossl/cleanupZeroSSLCertificatesTaskFactory.js
Show resolved
Hide resolved
Issue being fixed or feature implemented
Due to recent bugs (#2297 and #2296), the dashmate helper created thousands of draft certificates. Free accounts allow only 3 certificates and paid 10000+. Multiple users reported that these limits were reached. We need to provide a simple way to cleanup accounts from created certs.
What was done?
dashmate ssl cleanupcommandHow Has This Been Tested?
Running command with account contained 10123 drafts.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
CleanupCommandfor managing Zero SSL certificate cleanup, allowing users to cancel drafted or pending validation certificates.Improvements
listCertificatesfunction to support pagination, enabling users to retrieve results across multiple pages.ObtainCommandto streamline task execution by changing the task to synchronous.scheduleRenewZeroSslCertificatefunction to improve control flow by removing asynchronous handling.Bug Fixes
These updates enhance SSL management capabilities and improve user experience when handling certificates.