TB-414: Parameterise clickhouse password #280
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent changes enhance security and configurability by introducing the Changes
Sequence Diagram(s)The changes are too varied and simple for a meaningful sequence diagram. Poem
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 as PR comments)
Additionally, you can add 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)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1)
Line range hint
24-27: Replace the RegExp constructor with a literal for better performance and readability.- const R_ERROR = new RegExp( - '(Code|Error): ([0-9]{2})[,.] .*Exception: (.+?)$', - 'm' - ); + const R_ERROR = /(Code|Error): ([0-9]{2})[,.] .*Exception: (.+?)$/m;dashboard-visualiser-superset/importer/config/supersetConfig.js (1)
Line range hint
22-22: Use strict equality for consistency and to avoid potential bugs.- const protocol = SUPERSET_SSL == 'false' ? 'http' : 'https'; + const protocol = SUPERSET_SSL === 'false' ? 'http' : 'https';
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
dashboard-visualiser-superset/importer/config/superset-export.zipis excluded by!**/*.zip
Files selected for processing (9)
- analytics-datastore-clickhouse/docker-compose.cluster.yml (6 hunks)
- analytics-datastore-clickhouse/docker-compose.yml (3 hunks)
- analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1 hunks)
- analytics-datastore-clickhouse/importer/docker-compose.config.yml (1 hunks)
- analytics-datastore-clickhouse/package-metadata.json (1 hunks)
- dashboard-visualiser-superset/importer/config/supersetConfig.js (2 hunks)
- dashboard-visualiser-superset/importer/docker-compose.config.yml (2 hunks)
- dashboard-visualiser-superset/package-metadata.json (1 hunks)
- test/cucumber/features/single-mode/superset.feature (1 hunks)
Files skipped from review due to trivial changes (2)
- analytics-datastore-clickhouse/importer/docker-compose.config.yml
- test/cucumber/features/single-mode/superset.feature
Additional context used
Biome
analytics-datastore-clickhouse/package-metadata.json
[error] 12-12: expected
,but instead found"CLICKHOUSE_IMAGE"analytics-datastore-clickhouse/importer/config/clickhouseConfig.js
[error] 1-1: Redundant use strict directive.
[error] 34-34: This else clause can be omitted because previous branches break early.
[error] 8-8: Use Number.parseInt instead of the equivalent global.
[error] 24-27: Use a regular expression literal instead of the RegExp constructor.
dashboard-visualiser-superset/importer/config/supersetConfig.js
[error] 1-1: Redundant use strict directive.
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
[error] 22-22: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 61-70: This var should be declared at the root of the enclosing function.
[error] 32-39: Use let or const instead of var.
[error] 53-53: Use let or const instead of var.
[error] 61-70: Use let or const instead of var.
[error] 105-113: Use let or const instead of var.
Additional comments not posted (10)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (2)
10-10: Ensure the fallback forCLICKHOUSE_PASSWORDis secure or handled appropriately.
16-19: Correctly configured basic authentication for ClickHouse.dashboard-visualiser-superset/importer/docker-compose.config.yml (2)
16-18: Correctly added environment variables for ClickHouse in the Docker configuration.
41-43: Theclickhousenetwork is correctly configured as external. This is a good practice for production environments to ensure that services can communicate across Docker networks.analytics-datastore-clickhouse/docker-compose.yml (2)
9-9: Correctly added theCLICKHOUSE_PASSWORDenvironment variable to the ClickHouse Docker service configuration.
55-57: Thereverse-proxynetwork is correctly configured as external. This is a good practice for production environments to ensure that services can communicate across Docker networks.dashboard-visualiser-superset/importer/config/supersetConfig.js (1)
17-18: Correctly implemented new functions for importing configurations and updating the database connection string in Superset.Also applies to: 52-119
analytics-datastore-clickhouse/docker-compose.cluster.yml (3)
118-119: Securely configuredCLICKHOUSE_PASSWORDand addedreverse-proxynetwork for enhanced security and network management.Also applies to: 225-227
Verification successful
The verification confirms that the
CLICKHOUSE_PASSWORDenvironment variable is consistently set across multiple instances in thedocker-compose.cluster.ymlfile. Additionally, thereverse-proxynetwork configuration is present and correctly defined.
Instances of
CLICKHOUSE_PASSWORDfound in:
analytics-datastore-clickhouse/docker-compose.cluster.yml
reverse-proxynetwork configuration found in:
analytics-datastore-clickhouse/docker-compose.cluster.ymlScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the CLICKHOUSE_PASSWORD environment variable is set and the reverse-proxy network is correctly configured. # Test: Check if the environment variable is set and the network is available. Expect: Non-empty value and network availability. echo $CLICKHOUSE_PASSWORD docker network inspect reverse-proxy_publicLength of output: 127
Script:
#!/bin/bash # Description: Verify the presence of CLICKHOUSE_PASSWORD environment variable and reverse-proxy network configuration in docker-compose.cluster.yml # Search for CLICKHOUSE_PASSWORD environment variable rg 'CLICKHOUSE_PASSWORD' analytics-datastore-clickhouse/docker-compose.cluster.yml # Search for reverse-proxy network configuration rg 'reverse-proxy' analytics-datastore-clickhouse/docker-compose.cluster.ymlLength of output: 428
8-9: Securely configuredCLICKHOUSE_PASSWORDfor enhanced security.
50-51: Securely configuredCLICKHOUSE_PASSWORDfor enhanced security.
dashboard-visualiser-superset/importer/config/supersetConfig.js
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- analytics-datastore-clickhouse/package-metadata.json (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- analytics-datastore-clickhouse/package-metadata.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- dashboard-visualiser-superset/package-metadata.json (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- dashboard-visualiser-superset/package-metadata.json
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (5)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (2)
Line range hint
1-1: Remove the redundant 'use strict' directive as it is unnecessary in ES modules.- 'use strict';
Line range hint
34-34: Theelseclause is redundant after athrowstatement. Simplify the control flow by removing it.- else console.log(query, '\n', r);dashboard-visualiser-superset/importer/config/supersetConfig.js (3)
Line range hint
1-1: Remove the redundant 'use strict' directive as it is unnecessary in ES modules.- 'use strict';
Line range hint
3-3: Import Node.js built-in modules using thenode:protocol for clarity and future-proofing.- const fs = require('fs'); - const path = require('path'); + const fs = require('node:fs'); + const path = require('node:path');Also applies to: 6-6
Line range hint
22-22: Use strict equality for comparisons to avoid potential type coercion issues.- const protocol = SUPERSET_SSL == 'false' ? 'http' : 'https'; + const protocol = SUPERSET_SSL === 'false' ? 'http' : 'https';
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1 hunks)
- dashboard-visualiser-superset/importer/config/supersetConfig.js (2 hunks)
- dashboard-visualiser-superset/package-metadata.json (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- dashboard-visualiser-superset/package-metadata.json
Additional context used
Learnings (1)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1)
User: arran-standish PR: jembi/platform#280 File: analytics-datastore-clickhouse/docker-compose.cluster.yml:86-87 Timestamp: 2024-05-30T07:42:33.445Z Learning: The `CLICKHOUSE_PASSWORD` environment variable is set by default in each `package-metadata.json` and only needs to be explicitly defined in an environment variable file if it is overridden.
Biome
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js
[error] 1-1: Redundant use strict directive.
[error] 34-34: This else clause can be omitted because previous branches break early.
[error] 24-27: Use a regular expression literal instead of the RegExp constructor.
dashboard-visualiser-superset/importer/config/supersetConfig.js
[error] 1-1: Redundant use strict directive.
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
[error] 22-22: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 61-70: This var should be declared at the root of the enclosing function.
[error] 32-39: Use let or const instead of var.
[error] 61-70: Use let or const instead of var.
Additional comments not posted (5)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (3)
16-19: Properly configured basic authentication using the newCLICKHOUSE_PASSWORD. Good job on enhancing security!
Line range hint
24-27: Use a regular expression literal for better performance and readability.
[REFACTOR_SUGGESTion]- const R_ERROR = new RegExp( - '(Code|Error): ([0-9]{2})[,.] .*Exception: (.+?)$', - 'm' - ); + const R_ERROR = /(Code|Error): ([0-9]{2})[,.] .*Exception: (.+?)$/m;
10-10: Ensure that the default password is strong enough for development environments to prevent unauthorized access.dashboard-visualiser-superset/importer/config/supersetConfig.js (2)
17-18: Correctly configured the newSUPERSET_DATABASE_PUT_PATH. This should resolve the issue with the PUT request mentioned in the PR description.
83-119: ThereplaceClickhouseConnectionStringfunction correctly updates the ClickHouse connection string. Good implementation of the new database configuration.
dashboard-visualiser-superset/importer/config/supersetConfig.js
Outdated
Show resolved
Hide resolved
dashboard-visualiser-superset/importer/config/supersetConfig.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1 hunks)
- dashboard-visualiser-superset/importer/config/supersetConfig.js (2 hunks)
- dashboard-visualiser-superset/package-metadata.json (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- dashboard-visualiser-superset/package-metadata.json
Additional context used
Learnings (1)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1)
User: arran-standish PR: jembi/platform#280 File: analytics-datastore-clickhouse/docker-compose.cluster.yml:86-87 Timestamp: 2024-05-30T07:42:33.445Z Learning: The `CLICKHOUSE_PASSWORD` environment variable is set by default in each `package-metadata.json` and only needs to be explicitly defined in an environment variable file if it is overridden.
Biome
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js
[error] 1-1: Redundant use strict directive.
[error] 34-34: This else clause can be omitted because previous branches break early.
[error] 24-27: Use a regular expression literal instead of the RegExp constructor.
dashboard-visualiser-superset/importer/config/supersetConfig.js
[error] 1-1: Redundant use strict directive.
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
[error] 22-22: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 61-70: This var should be declared at the root of the enclosing function.
[error] 32-39: Use let or const instead of var.
[error] 61-70: Use let or const instead of var.
Additional comments not posted (4)
analytics-datastore-clickhouse/importer/config/clickhouseConfig.js (1)
16-19: The integration ofCLICKHOUSE_PASSWORDinto thebasicAuthconfiguration is correctly implemented.dashboard-visualiser-superset/importer/config/supersetConfig.js (3)
17-18: The addition ofSUPERSET_DATABASE_PUT_PATHwith a sensible default is well-implemented.
52-81: The implementation ofimportZipConfigfunction is robust, correctly handling the presence or absence of a configuration file.
83-119: ThereplaceClickhouseConnectionStringfunction is well-implemented, dynamically updating the ClickHouse connection string based on environment variables.
arran-standish
force-pushed
the
TB-414-parameterise-clickhouse-password
branch
from
4858847
to
b1dff16
Compare
By default clickhouse does not have a password set and allows anyone to connect to it and run queries, this is not ideal for production and we should at least have a password set on it. This PR sets out to do that.
The one complexity is superset. Before the zip file we import had a database connection defined in it, however you cannot parameterise that with env variables. I tried doing a PUT to the id it creates but superset was 404'ing for some reason, so I opted to create one instead and remove the default db connection from the zip file. This still allows other implementations to provide their own zip file to bootstrap with while still creating a valid clickhouse db connection.
Summary by CodeRabbit
New Features
Configuration
Tests