-
Notifications
You must be signed in to change notification settings - Fork 549
feat: ClickHouse bring-your-own-warehouse connections #8020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5c91188
770a97e
73640e9
61a2a3a
9a05fe6
6695381
98c0c76
81cf471
f91b461
b8c97cb
808513a
50891b5
171c14c
f7a302b
3589d6c
77c343a
87861f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import base64 | ||
| import hashlib | ||
| import json | ||
| from typing import Any | ||
|
|
||
| import structlog | ||
| from cryptography.fernet import Fernet, InvalidToken | ||
| from django.conf import settings | ||
| from django.db import models | ||
|
|
||
| logger = structlog.get_logger("core") | ||
|
|
||
|
|
||
| def _get_fernet() -> Fernet: | ||
| digest = hashlib.sha256(settings.SECRET_KEY.encode()).digest() | ||
| return Fernet(base64.urlsafe_b64encode(digest)) | ||
|
|
||
|
|
||
| class EncryptedJSONField(models.TextField[Any, Any]): | ||
| def get_prep_value(self, value: Any) -> str | None: | ||
| if value is None: | ||
| return None | ||
| return _get_fernet().encrypt(json.dumps(value).encode()).decode() | ||
|
|
||
| def from_db_value( | ||
| self, | ||
| value: str | None, | ||
| expression: object, | ||
| connection: object, | ||
| ) -> Any: | ||
| if value is None: | ||
| return None | ||
| try: | ||
| plaintext = _get_fernet().decrypt(value.encode()) | ||
| except InvalidToken: | ||
| logger.warning("encrypted_field.decrypt_failed", exc_info=True) | ||
| return None | ||
| return json.loads(plaintext) | ||
|
|
||
| def get_lookup(self, lookup_name: str) -> Any: | ||
| if lookup_name != "isnull": | ||
| raise NotImplementedError( | ||
| "EncryptedJSONField only supports isnull lookups." | ||
| ) | ||
| return super().get_lookup(lookup_name) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import ipaddress | ||
| import socket | ||
|
|
||
|
|
||
| def is_internal_address(hostname: str) -> bool: | ||
| """Return True if the hostname is, or resolves to, an internal network | ||
| address: loopback, RFC 1918 private, link-local, reserved, or multicast. | ||
| Unresolvable hostnames are not considered internal.""" | ||
| 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 | ||
|
Comment on lines
+9
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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. |
||
|
|
||
| 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 | ||
| ) | ||
|
Comment on lines
+19
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 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:
💡 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:
Cover RFC 6598 shared address space
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import prometheus_client | ||
|
|
||
| flagsmith_experimentation_warehouse_connection_verifications_total = ( | ||
| prometheus_client.Counter( | ||
| "flagsmith_experimentation_warehouse_connection_verifications_total", | ||
| "Outcomes of connection verification attempts against customers' own " | ||
| "data warehouses. `result` label is either `success` or `failure`.", | ||
| ["result"], | ||
| ) | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| from django.db import migrations | ||
|
|
||
| import core.fields | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("experimentation", "0009_add_rollout_segment"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="warehouseconnection", | ||
| name="credentials", | ||
| field=core.fields.EncryptedJSONField(blank=True, null=True), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Generated by Django 5.2.16 on 2026-07-16 09:01 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("experimentation", "0010_warehouse_connection_credentials"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="warehouseconnection", | ||
| name="status_detail", | ||
| field=models.CharField(blank=True, max_length=255, null=True), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| import structlog | ||
| from clickhouse_driver import Client | ||
| from clickhouse_driver import errors as clickhouse_errors | ||
| from clickhouse_driver.util.helpers import parse_url | ||
| from django.conf import settings | ||
| from django.db import transaction | ||
|
|
@@ -17,6 +18,7 @@ | |
| from audit.models import AuditLog | ||
| from audit.related_object_type import RelatedObjectType | ||
| from core.dataclasses import AuthorData | ||
| from core.network import is_internal_address | ||
| from environments.tasks import rebuild_environment_document | ||
| from experimentation.constants import ( | ||
| CONTROL_VARIANT_KEY, | ||
|
|
@@ -40,6 +42,9 @@ | |
| RolloutSpec, | ||
| WarehouseEventStats, | ||
| ) | ||
| from experimentation.metrics import ( | ||
| flagsmith_experimentation_warehouse_connection_verifications_total, | ||
| ) | ||
| from experimentation.models import ( | ||
| VALID_STATUS_TRANSITIONS, | ||
| Experiment, | ||
|
|
@@ -56,6 +61,7 @@ | |
| compare_to_control, | ||
| srm_p_value, | ||
| ) | ||
| from experimentation.types import ClickHouseConfig, ClickHouseCredentials | ||
| from features.models import FeatureState | ||
| from features.value_types import BOOLEAN, INTEGER, STRING | ||
| from features.versioning.dataclasses import FlagChangeSet, MultivariateValueChangeSet | ||
|
|
@@ -88,6 +94,7 @@ | |
|
|
||
| CLICKHOUSE_CONNECT_TIMEOUT_SECONDS = 5 | ||
| CLICKHOUSE_QUERY_TIMEOUT_SECONDS = 30 | ||
| CLICKHOUSE_VERIFY_TIMEOUT_SECONDS = 5 | ||
|
|
||
|
|
||
| def is_warehouse_feature_enabled(organisation: Organisation) -> bool: | ||
|
|
@@ -787,6 +794,73 @@ def mark_warehouse_pending_connection( | |
| return connection | ||
|
|
||
|
|
||
| class InternalAddressError(Exception): | ||
| pass | ||
|
|
||
|
|
||
| def _describe_verification_error(error: Exception) -> str: | ||
| if isinstance(error, clickhouse_errors.ServerException): | ||
| if error.code == 516: | ||
| return "Authentication failed." | ||
| if error.code == 81: | ||
| return "Database does not exist." | ||
| return "The ClickHouse server rejected the request." | ||
| if isinstance(error, (clickhouse_errors.SocketTimeoutError, TimeoutError)): | ||
| return "The connection timed out." | ||
| if isinstance(error, clickhouse_errors.NetworkError): | ||
| return "Could not connect to the host." | ||
| if isinstance(error, InternalAddressError): | ||
| return "Host must not target internal or private network addresses." | ||
| if isinstance(error, KeyError): | ||
| return "Stored connection details are incomplete." | ||
| return "Verification failed." | ||
|
|
||
|
|
||
| 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 {}) | ||
| # 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"], | ||
|
Comment on lines
+827
to
+832
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Close the DNS-rebinding gap before opening the socket.
|
||
| 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_VERIFY_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") | ||
|
Comment on lines
+819
to
+861
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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 |
||
|
|
||
|
|
||
| def refresh_warehouse_connection_status( | ||
| connection: WarehouseConnection, | ||
| stats: WarehouseEventStats, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: Flagsmith/flagsmith
Length of output: 7390
🏁 Script executed:
Repository: Flagsmith/flagsmith
Length of output: 1754
Use a dedicated key for encrypted fields. Deriving the Fernet key from
SECRET_KEYcouples credential encryption to Django’s global signing secret, and rotatingSECRET_KEYwill make stored values undecryptable whilefrom_db_valuereturnsNone, which downstream surfaces as “Stored connection details are incomplete.” Add a separate encryption setting and a rotation path instead of reusingSECRET_KEY.