feat: ClickHouse bring-your-own-warehouse connections#8020
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
api
github-actions
Bot
added
feature
…arehouse-backend # Conflicts: # docs/docs/deployment-self-hosting/observability/_events-catalogue.md
github-actions
Bot
added
feature
| allOf: | ||
| - $ref: '#/components/schemas/WarehouseConnectionStatusEnum' | ||
| readOnly: true | ||
| status_detail: |
There was a problem hiding this comment.
This is nice to have, I believe especially with other warehouses that we don't know so well, it will be helpful to keep track of the error
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 1501 1508 +7
Lines 59474 59846 +372
========================================
+ Hits 58668 59040 +372
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
|
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
| def _get_fernet() -> Fernet: | ||
| digest = hashlib.sha256(settings.SECRET_KEY.encode()).digest() | ||
| return Fernet(base64.urlsafe_b64encode(digest)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## api/core/fields.py\n'
cat -n api/core/fields.py | sed -n '1,120p'
printf '\n## Search for _get_fernet and related encryption settings/tests\n'
rg -n "_get_fernet|Fernet|MultiFernet|SECRET_KEY|FIELD_ENCRYPTION|decrypt|from_db_value|Stored connection details are incomplete" api -S
printf '\n## api/core/services.py (relevant slices)\n'
if [ -f api/core/services.py ]; then
cat -n api/core/services.py | sed -n '1,240p'
fi
printf '\n## tests mentioning SECRET_KEY rotation or from_db_value\n'
rg -n "secret_key changed|SECRET_KEY changed|from_db_value__secret_key_changed|returns none and logs|incomplete" tests api -SRepository: Flagsmith/flagsmith
Length of output: 7390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('api/core/fields.py')
print(p.exists(), p)
if p.exists():
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 80:
print(f"{i:4d}: {line}")
PYRepository: Flagsmith/flagsmith
Length of output: 1754
Use a dedicated key for encrypted fields. Deriving the Fernet key from SECRET_KEY couples credential encryption to Django’s global signing secret, and rotating SECRET_KEY will make stored values undecryptable while from_db_value returns None, which downstream surfaces as “Stored connection details are incomplete.” Add a separate encryption setting and a rotation path instead of reusing SECRET_KEY.
| def verify_clickhouse_connection(connection: WarehouseConnection) -> None: | ||
| """Run SELECT 1 against the customer's ClickHouse and set the status to | ||
| connected or errored; never raises.""" | ||
| log = logger.bind(environment__id=connection.environment_id) | ||
| try: | ||
| log = log.bind(organisation__id=connection.environment.project.organisation_id) | ||
| config = typing.cast(ClickHouseConfig, connection.config or {}) | ||
| credentials = typing.cast(ClickHouseCredentials, connection.credentials or {}) | ||
| client = Client( | ||
| config["host"], | ||
| port=config["port"], | ||
| user=config["username"], | ||
| password=credentials["password"], | ||
| database=config["database"], | ||
| secure=config["secure"], | ||
| connect_timeout=CLICKHOUSE_CONNECT_TIMEOUT_SECONDS, | ||
| send_receive_timeout=CLICKHOUSE_QUERY_TIMEOUT_SECONDS, | ||
| ) | ||
| try: | ||
| client.execute("SELECT 1") | ||
| finally: | ||
| client.disconnect() | ||
| except Exception as error: | ||
| connection.status = WarehouseConnectionStatus.ERRORED | ||
| connection.status_detail = _describe_verification_error(error) | ||
| connection.save(update_fields=["status", "status_detail"]) | ||
| flagsmith_experimentation_warehouse_connection_verifications_total.labels( | ||
| result="failure" | ||
| ).inc() | ||
| log.warning("connection.verification_failed", exc_info=True) | ||
| return | ||
|
|
||
| connection.status = WarehouseConnectionStatus.CONNECTED | ||
| connection.status_detail = None | ||
| connection.save(update_fields=["status", "status_detail"]) | ||
| flagsmith_experimentation_warehouse_connection_verifications_total.labels( | ||
| result="success" | ||
| ).inc() | ||
| log.info("connection.verification_succeeded") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the verifier and its call sites.
printf '\n== services.py around verify_clickhouse_connection ==\n'
sed -n '780,900p' api/experimentation/services.py
printf '\n== views.py references ==\n'
rg -n "verify_clickhouse_connection|write_environment_ingestion_keys\.delay|AFTER_CREATE|perform_create|perform_update|test connection|connection.verification" api/experimentation -n
printf '\n== views.py relevant slices ==\n'
for f in api/experimentation/views.py api/experimentation/*.py; do
if [ -f "$f" ] && rg -n "verify_clickhouse_connection|write_environment_ingestion_keys\.delay|perform_create|perform_update|AFTER_CREATE" "$f" >/dev/null; then
echo "--- $f ---"
sed -n '1,260p' "$f" | rg -n -A40 -B20 "verify_clickhouse_connection|write_environment_ingestion_keys\.delay|perform_create|perform_update|AFTER_CREATE"
fi
doneRepository: Flagsmith/flagsmith
Length of output: 18677
Move ClickHouse verification off the request thread Up to ~35s of network I/O still runs inline in perform_create/perform_update, so a slow or unreachable ClickHouse host can block API workers on every create/update. Queue this asynchronously and update status/status_detail out of band.
| def is_internal_address(hostname: str) -> bool: | ||
| try: | ||
| ips = [ipaddress.ip_address(hostname)] | ||
| except ValueError: | ||
| # hostname is a name rather than a literal IP — resolve it. | ||
| try: | ||
| results = socket.getaddrinfo(hostname, None, socket.AF_UNSPEC) | ||
| ips = [ipaddress.ip_address(str(r[4][0]).split("%")[0]) for r in results] | ||
| except socket.gaierror: | ||
| # Unresolvable hostname; leave it to the URL validator. | ||
| return False | ||
|
|
||
| return any( | ||
| ip.is_loopback | ||
| or ip.is_private | ||
| or ip.is_link_local | ||
| or ip.is_reserved | ||
| or ip.is_multicast | ||
| for ip in ips | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Prevent DNS rebinding from bypassing the SSRF check.
Validation resolves the hostname once, but the later outbound connection resolves the original hostname again. An attacker-controlled domain can return a public address first and an internal address second. Connect to a validated address while preserving the hostname for TLS, verify the actual peer address, or enforce network-level egress controls.
Also applies to: 48-53
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/experimentation/warehouse_validation.py (1)
66-78: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSnowflake config validator still accepts and merges unvalidated arbitrary keys.
Unlike
validate_clickhouse_config(which now rejects unknown keys and validates every field's type),validate_snowflake_configmerges**configunchanged. A caller can inject unexpected keys (or wrong types forwarehouse/database/schema/role/user) and they will be persisted and returned via the API, the same class of problem raised previously for ClickHouse but not carried over here.🔒 Suggested allowlist + type check
def validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: if not isinstance(config, dict): raise serializers.ValidationError({"config": "Must be an object."}) + if unknown_keys := set(config) - set(SNOWFLAKE_DEFAULTS) - {"account_identifier"}: + raise serializers.ValidationError( + {"config": {key: "Unknown field." for key in sorted(unknown_keys)}} + ) account_identifier = config.get("account_identifier", "") if not account_identifier: raise serializers.ValidationError( {"config": {"account_identifier": "This field is required."}} ) + for key, value in config.items(): + if key != "account_identifier" and not isinstance(value, str): + raise serializers.ValidationError({"config": {key: "Must be a string."}}) merged: SnowflakeConfig = { **SNOWFLAKE_DEFAULTS, **config, # type: ignore[typeddict-item] } return mergedapi/experimentation/serializers.py (1)
44-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrop
default=Noneon these JSON fields.WarehouseConnectionViewSetexposesPUT, so omittedconfig/credentialsare still inserted intoattrsasNoneand the"not in attrs"preservation logic invalidate()/validate_credentials()never runs. That turns a partial omission into an explicit clear and can reject updates that should keep the existing values.api/experimentation/services.py (1)
845-857: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent stale verification results overwriting newer configuration state.
Concurrent PATCH/test requests can verify different configuration snapshots; whichever finishes last overwrites
statusandstatus_detail, even when its configuration is no longer current. Persist a configuration revision or fingerprint and update the result conditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 609b19f1-40b7-4181-8628-960b4584dafe
📒 Files selected for processing (14)
api/app/settings/common.pyapi/app/settings/test.pyapi/core/network.pyapi/experimentation/serializers.pyapi/experimentation/services.pyapi/experimentation/views.pyapi/experimentation/warehouse_validation.pyapi/tests/unit/core/test_fields.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.md
| try: | ||
| ips = [ipaddress.ip_address(hostname)] | ||
| except ValueError: | ||
| # hostname is a name rather than a literal IP — resolve it. | ||
| try: | ||
| results = socket.getaddrinfo(hostname, None, socket.AF_UNSPEC) | ||
| ips = [ipaddress.ip_address(str(r[4][0]).split("%")[0]) for r in results] | ||
| except socket.gaierror: | ||
| return False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'api/core/*' 'api/*webhooks*' 'api/*warehouse*' | sed -n '1,200p'
printf '\nOutline of api/core/network.py:\n'
ast-grep outline api/core/network.py --view expanded || true
printf '\nRelevant call sites for is_internal_address:\n'
rg -n "is_internal_address\(" api -S
printf '\nRead api/core/network.py with line numbers:\n'
cat -n api/core/network.py | sed -n '1,220p'
printf '\nRead the call-site files around matches (if any):\n'
for f in $(rg -l "is_internal_address\(" api -S || true); do
printf '\n### %s ###\n' "$f"
cat -n "$f" | sed -n '1,240p'
doneRepository: Flagsmith/flagsmith
Length of output: 20160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for validate_clickhouse_config / validate_credentials usage:\n'
rg -n "validate_clickhouse_config|validate_credentials|warehouse_validation" api -S
printf '\nRead experimentation serializers/models around the matches:\n'
for f in $(rg -l "validate_clickhouse_config|validate_credentials|warehouse_validation" api -S || true); do
printf '\n### %s ###\n' "$f"
cat -n "$f" | sed -n '1,260p'
doneRepository: Flagsmith/flagsmith
Length of output: 16963
Bound DNS resolution in this validator. socket.getaddrinfo has no timeout, so a hostile or broken resolver can hold this request path open for a long time. This runs synchronously during webhook URL and warehouse host validation, so a single hostname can tie up workers; use a bounded, fail-closed lookup strategy instead.
| return any( | ||
| ip.is_loopback | ||
| or ip.is_private | ||
| or ip.is_link_local | ||
| or ip.is_reserved | ||
| or ip.is_multicast | ||
| for ip in ips | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== api/core/network.py ==\n'
wc -l api/core/network.py
cat -n api/core/network.py
printf '\n== usages of the helper ==\n'
rg -n "is_loopback|is_private|is_link_local|is_reserved|is_multicast|ip\.ipv4_mapped|100\.64\.0\.0/10|shared address" api -S
printf '\n== python ipaddress probe ==\n'
python3 - <<'PY'
import ipaddress
tests = [
"100.64.0.1",
"100.127.255.254",
"100.128.0.1",
"::ffff:192.168.1.1",
"::ffff:127.0.0.1",
"::ffff:100.64.0.1",
"192.168.1.1",
"127.0.0.1",
"169.254.1.1",
"8.8.8.8",
]
for s in tests:
ip = ipaddress.ip_address(s)
attrs = {
"is_global": ip.is_global,
"is_private": ip.is_private,
"is_loopback": ip.is_loopback,
"is_link_local": ip.is_link_local,
"is_reserved": ip.is_reserved,
"is_multicast": ip.is_multicast,
}
if isinstance(ip, ipaddress.IPv6Address):
attrs["ipv4_mapped"] = str(ip.ipv4_mapped) if ip.ipv4_mapped else None
print(s, attrs)
PYRepository: Flagsmith/flagsmith
Length of output: 3074
🌐 Web query:
Python ipaddress IPv4-mapped IPv6 is_private loopback behaviour shared address space 100.64.0.0/10 docs
💡 Result:
In Python's ipaddress module, the is_private and is_global attributes follow specific logic concerning the shared address space (100.64.0.0/10) and IPv4-mapped IPv6 addresses [1][2]. Shared Address Space (100.64.0.0/10) The 100.64.0.0/10 range is treated as a special case in the ipaddress module [1][2]. Both is_private and is_global return False for this network [2][3]. This is distinct from most other address ranges where is_private is the logical opposite of is_global [1][4]. IPv4-mapped IPv6 Addresses For IPv4-mapped IPv6 addresses (addresses starting with::FFFF/96), the is_private and is_global attributes are determined by the semantics of the underlying embedded IPv4 address [2][3]. The library enforces that the boolean value of the mapped address matches the value of the IPv4 address it contains [2]: address.is_private == address.ipv4_mapped.is_private address.is_global == address.ipv4_mapped.is_global This ensures consistency between the IPv6 representation and the underlying IPv4 address rules, including the special treatment of the 100.64.0.0/10 range [1][4]. General Behavior The is_private property is defined as True if an address is not globally reachable according to the IANA IPv4 or IPv6 special-purpose address registries, with the specified exceptions [1][5]. Conversely, is_global is True if the address is defined as globally reachable, again with the exception of the 100.64.0.0/10 range [2][3]. Developers should be aware that these properties reflect these IANA-based definitions rather than purely local or enterprise network configuration [5][6].
Citations:
- 1: https://docs.python.org/3/library/ipaddress.html
- 2: https://docs.python.org/3.10/library/ipaddress.html
- 3: https://docs.python.org/3.11/library/ipaddress.html
- 4: https://github.com/python/cpython/blob/master/Lib/ipaddress.py
- 5: https://github.com/python/cpython/blob/3.13/Lib/ipaddress.py
- 6: ipaddress considers some not globally reachable addresses global and vice versa python/cpython#113171
Cover RFC 6598 shared address space
100.64.0.0/10 is neither is_private nor caught by the other predicates, so it still bypasses this internal-address check. Add an explicit shared-address-space check if these hosts should be treated as internal.
| # Re-check right before connecting: DNS may resolve differently than it | ||
| # did at validation time, and rows may predate host validation. | ||
| if is_internal_address(config["host"]): | ||
| raise InternalAddressError(config["host"]) | ||
| client = Client( | ||
| config["host"], |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Close the DNS-rebinding gap before opening the socket.
is_internal_address(config["host"]) and Client(config["host"], ...) resolve the hostname independently. An attacker-controlled hostname can return a public address for the check and an internal address for the client connection. Pin the validated address or enforce network-level egress denial for private/link-local destinations.
| def get_throttles(self) -> list[BaseThrottle]: | ||
| if self.action in ( | ||
| "create", | ||
| "update", | ||
| "partial_update", | ||
| "test_warehouse_connection", | ||
| ): | ||
| self.throttle_scope = "warehouse_connection_write" | ||
| return [ScopedRateThrottle()] | ||
| return super().get_throttles() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== views.py ==\n'
sed -n '1,180p' api/experimentation/views.py
printf '\n== test_views.py around throttle test ==\n'
sed -n '1020,1100p' api/tests/unit/experimentation/test_views.py
printf '\n== search for get_throttles definitions/usages ==\n'
rg -n "def get_throttles|throttle_scope|ScopedRateThrottle|throttle_classes" api/experimentation api/tests/unit/experimentation -SRepository: Flagsmith/flagsmith
Length of output: 10305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' api/experimentation/views.py
sed -n '1020,1100p' api/tests/unit/experimentation/test_views.py
rg -n "def get_throttles|throttle_scope|ScopedRateThrottle|throttle_classes" api/experimentation api/tests/unit/experimentation -SRepository: Flagsmith/flagsmith
Length of output: 10196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "DEFAULT_THROTTLE_CLASSES|throttle_classes\s*=" -S .Repository: Flagsmith/flagsmith
Length of output: 1775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== NestedEnvironmentViewSet definition ==\n'
rg -n "class NestedEnvironmentViewSet|throttle_classes|get_throttles" api environments core -S
printf '\n== likely file for NestedEnvironmentViewSet ==\n'
fd -a "views.py" api environments core | rg "environments/views.py|NestedEnvironmentViewSet|views.py$"Repository: Flagsmith/flagsmith
Length of output: 2911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.py'):
try:
text = p.read_text()
except Exception:
continue
if 'class NestedEnvironmentViewSet' in text:
print(p)
PYRepository: Flagsmith/flagsmith
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,380p' api/environments/views.pyRepository: Flagsmith/flagsmith
Length of output: 3267
Preserve the existing throttle stack for write actions. In api/experimentation/views.py#L94-L103, append ScopedRateThrottle to super().get_throttles() instead of returning it on its own, so any configured throttles still apply. In api/tests/unit/experimentation/test_views.py#L1054-L1069, assert the warehouse scope is present without requiring it to be the only throttle.
📍 Affects 2 files
api/experimentation/views.py#L94-L103(this comment)api/tests/unit/experimentation/test_views.py#L1054-L1069
1 participant
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/.