feat: ClickHouse bring-your-own-warehouse connections#8020
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
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:
📝 WalkthroughWalkthroughAdds encrypted ClickHouse credentials, status details, typed defaults, shared internal-address validation, and warehouse-specific serializer validation. ClickHouse connections are verified during relevant lifecycle actions and explicit testing, with bounded queries, error details, status updates, metrics, logging, and write throttling. API schemas and observability catalogues are updated, alongside migrations and unit/API coverage. Webhook SSRF validation now uses the shared network helper. Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
github-actions
Bot
added
api
github-actions
Bot
added
feature
github-actions
Bot
added
feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8020 +/- ##
========================================
Coverage 98.64% 98.65%
========================================
Files 1506 1512 +6
Lines 59573 59971 +398
========================================
+ Hits 58767 59165 +398
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
feature
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/experimentation/serializers.py (1)
66-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFully normalise state when changing warehouse type.
The transition currently clears credentials but can retain configuration and verification state belonging to the previous backend.
api/experimentation/serializers.py#L66-L87: preserve omitted config only when the warehouse type is unchanged; otherwise validate or clear it for the new type.api/experimentation/views.py#L110-L119: resetstatusandstatus_detailwhen the type changes.api/tests/unit/experimentation/test_views.py#L1686-L1709: assert configuration and status reset alongside credential clearing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a79b4d5-56fe-458d-83ce-a1457dc09356
📒 Files selected for processing (21)
api/app/settings/common.pyapi/app/settings/test.pyapi/core/fields.pyapi/experimentation/metrics.pyapi/experimentation/migrations/0010_warehouse_connection_credentials.pyapi/experimentation/migrations/0011_warehouse_connection_status_detail.pyapi/experimentation/models.pyapi/experimentation/serializers.pyapi/experimentation/services.pyapi/experimentation/types.pyapi/experimentation/views.pyapi/experimentation/warehouse_validation.pyapi/tests/unit/core/test_fields.pyapi/tests/unit/experimentation/conftest.pyapi/tests/unit/experimentation/test_models.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/experimentation/test_views.pyapi/webhooks/fields.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/deployment-self-hosting/observability/_metrics-catalogue.mdopenapi.yaml
for more information, see https://pre-commit.ci
The required-field check ran on the submitted config dict before merging with the stored config, so any PATCH that omitted account_identifier got a spurious 400 even when the stored value was valid. Mirrors the ClickHouse validator pattern: validate on merged, not on incoming.
Use a separate env var for warehouse credential encryption instead of deriving the Fernet key from Django's SECRET_KEY. Defaults to SECRET_KEY for backwards compatibility.
c9cb041 to
4b54dc7
Compare
github-actions
Bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
api/core/network.py (1)
20-24: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftBound the DNS lookup with a timeout.
socket.getaddrinfohas no timeout here, so a slow/malicious resolver can block the calling thread indefinitely. This function is invoked synchronously from webhook URL validation and ClickHouse verification (api/experimentation/services.py), so a single hostname can tie up API workers.🔧 Suggested approach
+import socket + +_DNS_TIMEOUT_SECONDS = 2 + def is_internal_address(...): ... try: - results = socket.getaddrinfo(hostname, None, socket.AF_UNSPEC) + socket.setdefaulttimeout(_DNS_TIMEOUT_SECONDS) + results = socket.getaddrinfo(hostname, None, socket.AF_UNSPEC) except socket.gaierror: return False + except socket.timeout: + return FalseNote:
socket.setdefaulttimeoutis process-global; a more robust fix would resolve in a bounded worker/thread with its own timeout.api/experimentation/services.py (2)
821-863: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSynchronous verification still blocks the request thread.
Up to
CLICKHOUSE_CONNECT_TIMEOUT_SECONDS+CLICKHOUSE_VERIFY_TIMEOUT_SECONDS(~10s) of network I/O runs inline wheneverperform_create/perform_update/test_warehouse_connectioncall this (seeapi/experimentation/views.py). A slow or unreachable customer ClickHouse host will block API workers on every write.
831-842: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDNS-rebinding gap between the internal-address check and the actual connection.
is_internal_address(config["host"], include_shared=True)andClient(config["host"], ...)each independently resolve the hostname. An attacker-controlled domain can return a public address for the check and a private/internal address when the driver connects moments later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f042ff0-f231-4251-a399-e7810b67ee56
📒 Files selected for processing (23)
api/app/settings/common.pyapi/app/settings/test.pyapi/core/fields.pyapi/core/network.pyapi/experimentation/metrics.pyapi/experimentation/migrations/0010_warehouse_connection_credentials_and_status_detail.pyapi/experimentation/models.pyapi/experimentation/serializers.pyapi/experimentation/services.pyapi/experimentation/types.pyapi/experimentation/views.pyapi/experimentation/warehouse_validation.pyapi/tests/unit/core/test_fields.pyapi/tests/unit/experimentation/conftest.pyapi/tests/unit/experimentation/test_models.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/experimentation/test_views.pyapi/tests/unit/webhooks/test_unit_webhooks.pyapi/tests/unit/webhooks/test_webhooks_fields.pyapi/webhooks/fields.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/deployment-self-hosting/observability/_metrics-catalogue.mdopenapi.yaml
Points at the same Secrets Manager key as DJANGO_SECRET_KEY for now. Update the ARN when a dedicated secret is provisioned.
github-actions
Bot
added
infrastructure
feature
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Adds the
clickhousewarehouse connection type so customers can bring their own ClickHouse instance for experimentation. This PR covers connection setup and credentials management; event routing is a follow-up inflagsmith-analytics-pipeline, and the frontend ships separately behind theclickhouse_warehouseflag.EncryptedJSONFieldstores connection credentials encrypted at rest, keyed offSECRET_KEY— no new configuration required.config(hostrequired, sensible defaults for port/database/username/TLS) and a write-onlycredentials: {password}that is never returned by the API. Omitting credentials on PATCH keeps the stored password; switching the connection to another type clears it.SELECT 1on create, on config or credential change, and via the existing test-connection endpoint, setting the status toconnectedorerrored.How did you test this code?
Unit tests (100% diff coverage),
make lint,make typecheck.Manually: POST
/api/v1/environments/<api_key>/warehouse-connections/with aclickhousepayload returns 201 with"status": "connected"or"errored"; re-verify via POST…/test-warehouse-connection/.