fix(dashmate): invalid mount path from helper #2296
Conversation
WalkthroughThe changes primarily involve modifications to 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/dashmate/docker/entrypoint.sh (1)
34-34: Add validation for DASHMATE_HOME_DIR environment variable.While the change to use
DASHMATE_HOME_DIRaligns with fixing the mount path issue, the script should validate this critical environment variable.Add this validation before the exec line:
+if [ -z "$DASHMATE_HOME_DIR" ]; then + echo "Error: DASHMATE_HOME_DIR environment variable is required" + exit 1 +fi + exec su $USERNAME -c "cd /platform;DASHMATE_HOME_DIR=$DASHMATE_HOME_DIR $*"packages/dashmate/scripts/helper.js (1)
Line range hint
1-89: Add environment validation and improve error handling.Given the PR's focus on fixing mount path issues, consider these improvements:
- Validate DASHMATE_HOME_DIR environment variable
- Add explicit error handling for SSL certificate operations
- Add logging for certificate renewal operations to track potential issues with draft certificates
Add this validation at the start of the script:
// Read environment variables from .env file dotenv.config(); + +// Validate required environment variables +if (!process.env.DASHMATE_HOME_DIR) { + throw new Error('DASHMATE_HOME_DIR environment variable is required'); +}And enhance the SSL certificate renewal section:
if (isEnabled && provider === 'zerossl') { const scheduleRenewZeroSslCertificate = container.resolve('scheduleRenewZeroSslCertificate'); - await scheduleRenewZeroSslCertificate(config); + try { + await scheduleRenewZeroSslCertificate(config); + console.info('SSL certificate renewal scheduled successfully'); + } catch (error) { + console.error('Failed to schedule SSL certificate renewal:', error); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/dashmate/docker-compose.yml(1 hunks)packages/dashmate/docker/entrypoint.sh(1 hunks)packages/dashmate/scripts/helper.js(1 hunks)packages/dashmate/src/createDIContainer.js(1 hunks)
🔇 Additional comments (6)
packages/dashmate/docker/entrypoint.sh (1)
34-34: Verify DASHMATE_HOME_DIR is properly configured.
The command structure looks good and preserving the environment by removing su - is the right approach. Let's verify the environment variable is properly set in the docker configuration.
✅ Verification successful
DASHMATE_HOME_DIR is properly configured across the environment
The verification confirms that DASHMATE_HOME_DIR is well-configured:
- It's properly set in the Dockerfile as
/home/dashmate/.dashmate - All docker-compose files consistently use
${DASHMATE_HOME_DIR:?err}with proper error handling - The environment variable is correctly passed through in the entrypoint.sh
- The variable is properly integrated in the codebase through
HomeDirclass and config generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DASHMATE_HOME_DIR is properly configured in docker-compose files
# Test: Check if DASHMATE_HOME_DIR is defined in docker-compose files
echo "Checking docker-compose files for DASHMATE_HOME_DIR configuration:"
fd --type f "docker-compose" --exec rg -l "DASHMATE_HOME_DIR"
# Test: Check if there are any hardcoded paths that should use DASHMATE_HOME_DIR
echo -e "\nChecking for potential hardcoded paths:"
rg -i "(/\.dashmate|dashmate.*home)"
Length of output: 5658
packages/dashmate/scripts/helper.js (1)
48-48: LGTM! Verify complete removal of DASHMATE_HELPER env var.
The explicit registration of isHelper as true is a cleaner approach than using an environment variable.
Let's verify that the old environment variable approach has been completely removed:
✅ Verification successful
The search results show that DASHMATE_HELPER references are only about the Docker image name constant (DASHMATE_HELPER_DOCKER_IMAGE), which is different from the environment variable approach that was meant to be replaced. Let me verify if there are any actual environment variable usages by searching for environment variable access patterns.
✅ Environment variable approach has been completely removed
The verification confirms that the old environment variable approach using DASHMATE_HELPER has been fully removed. The only remaining references are to the Docker image name constant DASHMATE_HELPER_DOCKER_IMAGE, which is the expected and correct usage. The new DI container registration with isHelper: asValue(true) is a clean replacement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to DASHMATE_HELPER
rg "DASHMATE_HELPER" --type js
Length of output: 608
Script:
#!/bin/bash
# Search for process.env.DASHMATE_HELPER or similar patterns
rg "process\.env\.DASHMATE_HELPER|getenv.*DASHMATE_HELPER" --type js
# Search for environment variable declarations in Docker compose or similar files
rg "DASHMATE_HELPER:" --type yaml
rg "DASHMATE_HELPER=" --type yaml
Length of output: 138
packages/dashmate/docker-compose.yml (2)
20-20: LGTM: Required environment variable addition
The addition of DASHMATE_HOME_DIR as a required environment variable is correct and aligns with the PR objective to use the host machine's Dashmate home directory path.
27-27: Verify SSL server functionality with new volume mount
The volume mount change from a fixed container path to using ${DASHMATE_HOME_DIR:?err} on both sides ensures path consistency between host and container, which should fix the SSL verification server mount issue.
Let's verify the impact on SSL-related files:
✅ Verification successful
Based on the search results, I can verify that the SSL-related volume mount change is consistent with the codebase. The key findings:
- The SSL certificate files are stored under
${DASHMATE_HOME_DIR}/${CONFIG_NAME}/platform/gateway/ssl/as seen in docker-compose.yml:
- ${DASHMATE_HOME_DIR:?err}/${CONFIG_NAME:?err}/platform/gateway/ssl/bundle.crt:/etc/ssl/bundle.crt:ro
- ${DASHMATE_HOME_DIR:?err}/${CONFIG_NAME:?err}/platform/gateway/ssl/private.key:/etc/ssl/private.key:ro
- The codebase has extensive SSL certificate handling logic that relies on these paths, including:
- Certificate generation and validation
- Support for different providers (ZeroSSL, self-signed, file)
- Certificate renewal and verification
- The volume mount change ensures consistent path mapping between host and container, which is critical for SSL certificate functionality.
SSL certificate mount paths are correctly configured
The volume mount change to use ${DASHMATE_HOME_DIR} consistently ensures proper SSL certificate file access between host and container. The paths align with the SSL certificate handling implementation in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SSL-related paths are referenced consistently
# Expected: All SSL paths should use DASHMATE_HOME_DIR
# Search for SSL-related path references
rg -i "ssl|cert|key" packages/dashmate/
Length of output: 139255
packages/dashmate/src/createDIContainer.js (2)
165-165: LGTM! Clean separation of helper configuration.
The change from environment variable to static value aligns with the PR's objective to improve helper container configuration. This creates a cleaner separation between the main container and helper contexts.
165-165: Verify related changes in helper configuration.
Since we're removing the environment variable dependency, let's ensure all related changes are in place.
✅ Verification successful
Let me analyze the helper configuration changes based on the results:
helper.jsstill setsisHelperto true, which is consistent with the change to set it asfalseby default in the DI container.DASHMATE_HELPERenvironment variable is still used in:- Docker compose files for image and port configuration
- Constants for docker image naming
- Dockerfile with
ENV DASHMATE_HELPER=1
Let me verify if these remaining usages are related to the helper service configuration rather than the runtime environment detection:
All helper-related changes are properly aligned
The changes are consistent across the codebase:
- The DI container correctly sets
isHelper: falseby default - The helper script explicitly overrides it to
truewhen needed - The remaining
DASHMATE_HELPERreferences in Docker files are for helper service configuration, not for runtime detection - The
isHelperflag is properly used ingetConnectionHostFactoryfor connection handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify helper configuration and usage of DASHMATE_HELPER
# Test 1: Check if helper.js properly sets isHelper
rg -l "isHelper.*true" "packages/dashmate/scripts/"
# Test 2: Verify no remaining uses of DASHMATE_HELPER env var
rg "DASHMATE_HELPER"
# Test 3: Check for any potential mount path related configurations
rg -A 3 "mountPath|DASHMATE_HOME"
Length of output: 6258
Script:
#!/bin/bash
# Check the context of DASHMATE_HELPER usage in helper.js
rg -A 5 "isHelper.*true" "packages/dashmate/scripts/helper.js"
# Check if there are any other files that might be using isHelper for configuration
rg "isHelper" --type js
Length of output: 685
shumkov
dismissed stale reviews from pshenmic and QuantumExplorer
via
8917557
Issue being fixed or feature implemented
Dashmate helper (a separate docker container) is using the dashmate home dir path as mount path for container for SSL verification server. The problem is that the dashmate home dir is hardcoded for the dashmate helper but mount expect the path on host machine. When dashmate home paths inside helper and outside aren't matching then we trying to mount path which is doesn't exist on host machine.
This leads SSL server to fail, which in turn, fails the SSL renewal process in a middle, so we spamming ZeroSSL with draft certificate. This PR solves the first issue - correct mount path. Spamming with drafts if the SSL renewal process was interrupted will be addressed in a separate PR.
What was done?
How Has This Been Tested?
Running locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
DASHMATE_HOME_DIRfor improved configurability of thedashmate_helperservice.dashmate_helperto allow for more flexible directory usage.Bug Fixes
These changes enhance the flexibility and usability of the
dashmate_helperservice for end-users.