From f28b0f81bbed1ae7b8dfcae4e72d9fa454126a85 Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 14 Jul 2026 09:37:05 +0200 Subject: [PATCH 01/25] feat: add EncryptedJSONField backed by a rotatable Fernet key ring --- api/app/settings/common.py | 5 + api/core/fields.py | 50 ++++++ api/tests/unit/core/test_fields.py | 147 ++++++++++++++++++ .../observability/_events-catalogue.md | 8 + 4 files changed, 210 insertions(+) create mode 100644 api/core/fields.py create mode 100644 api/tests/unit/core/test_fields.py diff --git a/api/app/settings/common.py b/api/app/settings/common.py index 5adde3332224..f325b02ab0e4 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -1505,6 +1505,11 @@ # https://github.com/Flagsmith/flagsmith/issues/8033 EXPERIMENTATION_CLICKHOUSE_URL = env.str("EXPERIMENTATION_CLICKHOUSE_URL", default=None) +# Comma-separated Fernet keys: the first encrypts, all decrypt (rotation). +CREDENTIALS_ENCRYPTION_KEYS: list[str] = env.list( + "CREDENTIALS_ENCRYPTION_KEYS", default=[] +) + SEGMENT_MEMBERSHIP_REFRESH_INTERVAL_HOURS = env.int( "SEGMENT_MEMBERSHIP_REFRESH_INTERVAL_HOURS", default=6 ) diff --git a/api/core/fields.py b/api/core/fields.py new file mode 100644 index 000000000000..b4e411a37ff6 --- /dev/null +++ b/api/core/fields.py @@ -0,0 +1,50 @@ +import json +from typing import Any + +import structlog +from cryptography.fernet import Fernet, InvalidToken, MultiFernet +from django.conf import settings +from django.core.exceptions import ImproperlyConfigured +from django.db import models + +logger = structlog.get_logger("core") + + +def _get_fernet() -> MultiFernet: + if not settings.CREDENTIALS_ENCRYPTION_KEYS: + raise ImproperlyConfigured( + "CREDENTIALS_ENCRYPTION_KEYS must be set to store encrypted fields." + ) + return MultiFernet([Fernet(key) for key in settings.CREDENTIALS_ENCRYPTION_KEYS]) + + +class EncryptedJSONField(models.TextField[Any, Any]): + """A JSON value stored Fernet-encrypted in a text column; a value whose + key left the ring loads as None rather than raising.""" + + 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, ImproperlyConfigured, ValueError): + 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) diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py new file mode 100644 index 000000000000..e6424091bf24 --- /dev/null +++ b/api/tests/unit/core/test_fields.py @@ -0,0 +1,147 @@ +import pytest +from cryptography.fernet import Fernet +from django.core.exceptions import ImproperlyConfigured +from pytest_django.fixtures import SettingsWrapper +from pytest_structlog import StructuredLogCapture + +from core.fields import EncryptedJSONField + + +@pytest.fixture() +def encryption_keys(settings: SettingsWrapper) -> list[str]: + keys = [Fernet.generate_key().decode()] + settings.CREDENTIALS_ENCRYPTION_KEYS = keys + return keys + + +def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips( + encryption_keys: list[str], +) -> None: + # Given + field = EncryptedJSONField() + value = {"password": "hunter2"} + + # When + stored = field.get_prep_value(value) + + # Then + assert stored is not None + assert "hunter2" not in stored + assert field.from_db_value(stored, None, None) == value + + +def test_get_prep_value__none__returns_none( + encryption_keys: list[str], +) -> None: + # Given + field = EncryptedJSONField() + + # When & Then + assert field.get_prep_value(None) is None + + +def test_from_db_value__none__returns_none( + encryption_keys: list[str], +) -> None: + # Given + field = EncryptedJSONField() + + # When & Then + assert field.from_db_value(None, None, None) is None + + +def test_get_prep_value__no_keys_configured__raises_improperly_configured( + settings: SettingsWrapper, +) -> None: + # Given + settings.CREDENTIALS_ENCRYPTION_KEYS = [] + field = EncryptedJSONField() + + # When & Then + with pytest.raises(ImproperlyConfigured): + field.get_prep_value({"password": "hunter2"}) + + +def test_from_db_value__new_key_prepended__old_token_still_decrypts( + settings: SettingsWrapper, +) -> None: + # Given: a value encrypted under the original key + old_key = Fernet.generate_key().decode() + settings.CREDENTIALS_ENCRYPTION_KEYS = [old_key] + field = EncryptedJSONField() + stored = field.get_prep_value({"password": "hunter2"}) + + # When: a new primary key is prepended to the ring + settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode(), old_key] + + # Then + assert field.from_db_value(stored, None, None) == {"password": "hunter2"} + + +def test_from_db_value__token_key_removed_from_ring__returns_none_and_logs( + settings: SettingsWrapper, + log: StructuredLogCapture, +) -> None: + # Given: a value encrypted under a key no longer in the ring + settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] + field = EncryptedJSONField() + stored = field.get_prep_value({"password": "hunter2"}) + settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] + + # When + value = field.from_db_value(stored, None, None) + + # Then + assert value is None + assert { + "level": "warning", + "event": "encrypted_field.decrypt_failed", + } in [{"level": e["level"], "event": e["event"]} for e in log.events] + + +def test_from_db_value__malformed_key_in_ring__returns_none_and_logs( + settings: SettingsWrapper, + log: StructuredLogCapture, +) -> None: + # Given: a valid token, then a malformed key lands in the ring + settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] + field = EncryptedJSONField() + stored = field.get_prep_value({"password": "hunter2"}) + settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode(), ""] + + # When + value = field.from_db_value(stored, None, None) + + # Then + assert value is None + assert any(e["event"] == "encrypted_field.decrypt_failed" for e in log.events) + + +def test_from_db_value__no_keys_configured__returns_none_and_logs( + settings: SettingsWrapper, + log: StructuredLogCapture, +) -> None: + # Given: a stored token but an empty key ring + settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] + field = EncryptedJSONField() + stored = field.get_prep_value({"password": "hunter2"}) + settings.CREDENTIALS_ENCRYPTION_KEYS = [] + + # When + value = field.from_db_value(stored, None, None) + + # Then + assert value is None + assert any(e["event"] == "encrypted_field.decrypt_failed" for e in log.events) + + +def test_get_lookup__non_isnull__raises_not_implemented( + encryption_keys: list[str], +) -> None: + # Given + field = EncryptedJSONField() + + # When & Then + with pytest.raises(NotImplementedError): + field.get_lookup("exact") + assert field.get_lookup("isnull") is not None diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 40fe1dd98bb8..63f37d06c251 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -71,6 +71,14 @@ Attributes: - `feature.count` - `organisation.id` +### `core.encrypted_field.decrypt_failed` + +Logged at `warning` from: + - `api/core/fields.py:41` + +Attributes: + - `exc_info` + ### `dynamodb.environment_document_compressed` Logged at `info` from: From 7c511a0fe921192bcf9f001c1c5fc05685fcc4a5 Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 14 Jul 2026 10:08:38 +0200 Subject: [PATCH 02/25] feat: add encrypted credentials column to warehouse connections --- .../0010_warehouse_connection_credentials.py | 17 +++++++++++ api/experimentation/models.py | 2 ++ api/tests/unit/experimentation/conftest.py | 30 +++++++++++++++++++ api/tests/unit/experimentation/test_models.py | 20 +++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 api/experimentation/migrations/0010_warehouse_connection_credentials.py diff --git a/api/experimentation/migrations/0010_warehouse_connection_credentials.py b/api/experimentation/migrations/0010_warehouse_connection_credentials.py new file mode 100644 index 000000000000..2de6ccf8ce95 --- /dev/null +++ b/api/experimentation/migrations/0010_warehouse_connection_credentials.py @@ -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), + ), + ] diff --git a/api/experimentation/models.py b/api/experimentation/models.py index a53b16fa28db..a796190affa2 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -12,6 +12,7 @@ hook, ) +from core.fields import EncryptedJSONField from core.models import SoftDeleteExportableModel from environments.models import Environment from experimentation.dataclasses import ( @@ -58,6 +59,7 @@ class WarehouseConnection(LifecycleModelMixin, SoftDeleteExportableModel): # ty config: models.JSONField[dict[str, object] | None, dict[str, object] | None] = ( models.JSONField(null=True, blank=True) ) + credentials = EncryptedJSONField(null=True, blank=True) created_at = models.DateTimeField(auto_now_add=True) # Populated at serialization time for flagsmith connections from ClickHouse; diff --git a/api/tests/unit/experimentation/conftest.py b/api/tests/unit/experimentation/conftest.py index ad9c74cce4b7..256672c7cf93 100644 --- a/api/tests/unit/experimentation/conftest.py +++ b/api/tests/unit/experimentation/conftest.py @@ -1,5 +1,7 @@ import pytest +from cryptography.fernet import Fernet from django.urls import reverse +from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from core.dataclasses import AuthorData @@ -91,3 +93,31 @@ def experiment_with_rollout( ), ) return experiment + + +@pytest.fixture() +def credentials_encryption_keys(settings: SettingsWrapper) -> list[str]: + keys = [Fernet.generate_key().decode()] + settings.CREDENTIALS_ENCRYPTION_KEYS = keys + return keys + + +@pytest.fixture() +def clickhouse_connection( + environment: Environment, + credentials_encryption_keys: list[str], +) -> WarehouseConnection: + connection: WarehouseConnection = WarehouseConnection.objects.create( + environment=environment, + warehouse_type=WarehouseType.CLICKHOUSE, + name="Production ClickHouse", + config={ + "host": "ch.example.com", + "port": 9440, + "database": "flagsmith", + "username": "default", + "secure": True, + }, + credentials={"password": "hunter2"}, + ) + return connection diff --git a/api/tests/unit/experimentation/test_models.py b/api/tests/unit/experimentation/test_models.py index ba3ae0c173fe..bbedcad4efd5 100644 --- a/api/tests/unit/experimentation/test_models.py +++ b/api/tests/unit/experimentation/test_models.py @@ -3,6 +3,7 @@ from datetime import timezone as dt_timezone import pytest +from django.db import connection as django_db_connection from django.utils import timezone from pytest_mock import MockerFixture @@ -241,3 +242,22 @@ def test_experiment_results__is_final__reflects_window_coverage( # When / Then the row is final only once it covers the experiment's end results = ExperimentResults(experiment=experiment, as_of=as_of) assert results.is_final is expected + + +def test_warehouse_connection_credentials__saved__ciphertext_in_db_and_roundtrips( + clickhouse_connection: WarehouseConnection, +) -> None: + # Given: the fixture-created ClickHouse connection + + # When: reading the raw column value + with django_db_connection.cursor() as cursor: + cursor.execute( + "SELECT credentials FROM experimentation_warehouseconnection WHERE id = %s", + [clickhouse_connection.id], + ) + raw = cursor.fetchone()[0] + + # Then: the DB stores ciphertext, and the ORM decrypts on load + assert "hunter2" not in raw + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.credentials == {"password": "hunter2"} From 4f0c9bb63db0999daa51012343ac57420175c568 Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 14 Jul 2026 10:35:44 +0200 Subject: [PATCH 03/25] feat: accept ClickHouse warehouse connection config and credentials --- api/experimentation/serializers.py | 73 +++++ api/experimentation/types.py | 21 ++ api/tests/unit/experimentation/test_views.py | 311 +++++++++++++++++++ 3 files changed, 405 insertions(+) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 92ca8cd0ace4..fd6d87af9d47 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -1,5 +1,6 @@ from typing import Any +from django.conf import settings from django.db import transaction from django.db.models import QuerySet from rest_framework import serializers @@ -24,7 +25,10 @@ get_experiment_rollout, ) from experimentation.types import ( + CLICKHOUSE_DEFAULTS, SNOWFLAKE_DEFAULTS, + ClickHouseConfig, + ClickHouseCredentials, MetricExperimentResult, SnowflakeConfig, ) @@ -41,6 +45,9 @@ class WarehouseConnectionSerializer(serializers.ModelSerializer): # type: ignore[type-arg] name = serializers.CharField(max_length=255, required=False) config = serializers.JSONField(default=None, required=False, allow_null=True) + credentials = serializers.JSONField( + default=None, required=False, allow_null=True, write_only=True + ) total_events_received = serializers.SerializerMethodField() unique_events_count = serializers.SerializerMethodField() @@ -52,6 +59,7 @@ class Meta: "status", "name", "config", + "credentials", "created_at", "total_events_received", "unique_events_count", @@ -64,6 +72,8 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: getattr(self.instance, "warehouse_type", ""), ) + self._validate_credentials(attrs, warehouse_type) + if "config" not in attrs and self.instance is not None: return attrs @@ -71,6 +81,8 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: if warehouse_type == WarehouseType.SNOWFLAKE: attrs["config"] = self._validate_snowflake_config(config or {}) + elif warehouse_type == WarehouseType.CLICKHOUSE: + attrs["config"] = self._validate_clickhouse_config(config or {}) elif warehouse_type == WarehouseType.FLAGSMITH: if config: raise serializers.ValidationError( @@ -79,6 +91,67 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: attrs["config"] = None return attrs + def _validate_credentials(self, attrs: dict[str, Any], warehouse_type: str) -> None: + credentials: dict[str, Any] | None = attrs.get("credentials") + if warehouse_type != WarehouseType.CLICKHOUSE: + if credentials is not None: + raise serializers.ValidationError( + {"credentials": "Only ClickHouse connections accept credentials."} + ) + return + if not settings.CREDENTIALS_ENCRYPTION_KEYS: + raise serializers.ValidationError( + { + "credentials": ( + "Credentials encryption is not configured on this installation." + ) + } + ) + if ( + credentials is None + and self.instance is not None + and getattr(self.instance, "warehouse_type", "") == WarehouseType.CLICKHOUSE + ): + attrs.pop("credentials", None) + return + attrs["credentials"] = self._validate_clickhouse_credentials(credentials or {}) + + @staticmethod + def _validate_clickhouse_credentials( + credentials: dict[str, Any], + ) -> ClickHouseCredentials: + if not isinstance(credentials, dict): + raise serializers.ValidationError({"credentials": "Must be an object."}) + password = credentials.get("password") + if not password or not isinstance(password, str): + raise serializers.ValidationError( + {"credentials": {"password": "This field is required."}} + ) + return {"password": password} + + @staticmethod + def _validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: + if not isinstance(config, dict): + raise serializers.ValidationError({"config": "Must be an object."}) + if not config.get("host"): + raise serializers.ValidationError( + {"config": {"host": "This field is required."}} + ) + merged: ClickHouseConfig = { + **CLICKHOUSE_DEFAULTS, + **config, # type: ignore[typeddict-item] + } + port = merged["port"] + if ( + isinstance(port, bool) + or not isinstance(port, int) + or not (1 <= port <= 65535) + ): + raise serializers.ValidationError( + {"config": {"port": "Enter a valid port number (1-65535)."}} + ) + return merged + def create( self, validated_data: dict[str, Any], diff --git a/api/experimentation/types.py b/api/experimentation/types.py index 996384956c6c..6122499cb6b8 100644 --- a/api/experimentation/types.py +++ b/api/experimentation/types.py @@ -44,3 +44,24 @@ class SnowflakeConfig(TypedDict): "role": "FLAGSMITH_LOADER", "user": "FLAGSMITH_SERVICE", } + + +class ClickHouseConfig(TypedDict): + host: str + port: int + database: str + username: str + secure: bool + + +CLICKHOUSE_DEFAULTS: ClickHouseConfig = { + "host": "", + "port": 9440, + "database": "flagsmith", + "username": "default", + "secure": True, +} + + +class ClickHouseCredentials(TypedDict): + password: str diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index dc6068edb1d5..b386bd9e7ea9 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1126,3 +1126,314 @@ def test_get__clickhouse_errors__returns_200_without_stats( assert data["unique_events_count"] is None # connection stays pending (GET never writes) assert data["status"] == "pending_connection" + + +def test_post__clickhouse_minimal_config__applies_defaults( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + environment: Environment, + mocker: MockerFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mocker.patch("experimentation.services.Client") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "name": "Production ClickHouse", + "config": {"host": "ch.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["config"] == { + "host": "ch.example.com", + "port": 9440, + "database": "flagsmith", + "username": "default", + "secure": True, + } + assert "credentials" not in response.json() + + +def test_post__clickhouse_missing_host__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "host" in response.json()["config"] + + +@pytest.mark.parametrize("port", [0, 65536, "not-a-port", True]) +def test_post__clickhouse_invalid_port__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + port: object, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "port": port}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "port" in response.json()["config"] + + +def test_post__clickhouse_missing_password__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "password" in response.json()["credentials"] + + +def test_post__clickhouse_no_encryption_keys__returns_400( + admin_client: APIClient, + enable_features: EnableFeaturesFixture, + settings: SettingsWrapper, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + settings.CREDENTIALS_ENCRYPTION_KEYS = [] + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "credentials": [ + "Credentials encryption is not configured on this installation." + ] + } + + +@pytest.mark.parametrize( + "warehouse_type, config", + [ + ("flagsmith", None), + ("snowflake", {"account_identifier": "xy12345.us-east-1"}), + ], + ids=["flagsmith", "snowflake"], +) +def test_post__non_clickhouse_with_credentials__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + warehouse_type: str, + config: dict[str, str] | None, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": warehouse_type, + "config": config, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "credentials": ["Only ClickHouse connections accept credentials."] + } + + +def test_post__clickhouse_no_name__auto_generates_name( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + environment: Environment, + mocker: MockerFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mocker.patch("experimentation.services.Client") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["name"] == f"ClickHouse Warehouse - {environment.name}" + + +def test_post__clickhouse_non_dict_credentials__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + "credentials": "hunter2", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "credentials" in response.json() + + +def test_post__clickhouse_non_dict_config__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": "not-a-dict", + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "config" in response.json() + + +def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + environment: Environment, + warehouse_connection: WarehouseConnection, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, warehouse_connection.id], + ) + + # When + response = admin_client.patch( + url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "password" in response.json()["credentials"] + + +def test_post__flagsmith_empty_credentials__returns_400( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "flagsmith", + "credentials": {}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "credentials": ["Only ClickHouse connections accept credentials."] + } From 0fdac551c0aafd460b0da7c13a9f6ed78d5328cb Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 14 Jul 2026 11:17:57 +0200 Subject: [PATCH 04/25] feat: verify ClickHouse warehouse connections against the customer instance --- api/experimentation/metrics.py | 10 ++ api/experimentation/services.py | 43 +++++++ .../unit/experimentation/test_services.py | 111 ++++++++++++++++++ .../observability/_events-catalogue.md | 34 ++++-- .../observability/_metrics-catalogue.md | 9 ++ 5 files changed, 199 insertions(+), 8 deletions(-) create mode 100644 api/experimentation/metrics.py diff --git a/api/experimentation/metrics.py b/api/experimentation/metrics.py new file mode 100644 index 000000000000..8c9d14d1771e --- /dev/null +++ b/api/experimentation/metrics.py @@ -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"], + ) +) diff --git a/api/experimentation/services.py b/api/experimentation/services.py index c304f2878b1a..4c37caf5caf1 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -40,6 +40,9 @@ RolloutSpec, WarehouseEventStats, ) +from experimentation.metrics import ( + flagsmith_experimentation_warehouse_connection_verifications_total, +) from experimentation.models import ( VALID_STATUS_TRANSITIONS, Experiment, @@ -56,6 +59,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 @@ -788,6 +792,45 @@ def mark_warehouse_pending_connection( return connection +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: + connection.status = WarehouseConnectionStatus.ERRORED + connection.save(update_fields=["status"]) + flagsmith_experimentation_warehouse_connection_verifications_total.labels( + result="failure" + ).inc() + log.warning("connection.verification_failed", exc_info=True) + return + + connection.status = WarehouseConnectionStatus.CONNECTED + connection.save(update_fields=["status"]) + flagsmith_experimentation_warehouse_connection_verifications_total.labels( + result="success" + ).inc() + log.info("connection.verification_succeeded") + + def refresh_warehouse_connection_status( connection: WarehouseConnection, stats: WarehouseEventStats, diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 81f5561291d5..35b0f5b11b0e 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -5,6 +5,7 @@ import pytest from django.db.models import Q from flag_engine.segments.constants import PERCENTAGE_SPLIT +from prometheus_client import REGISTRY from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from pytest_structlog import StructuredLogCapture @@ -36,6 +37,7 @@ WarehouseType, ) from experimentation.results_query import _MetricSlot +from experimentation.services import verify_clickhouse_connection from experimentation.stats import VariantStats from features.feature_types import MULTIVARIATE from features.models import Feature, FeatureState @@ -2005,3 +2007,112 @@ def variant_assignment() -> dict[str, int]: # Then every already-enrolled identity keeps the variant it was first # assigned; tuning the rollout must not re-randomise the split. assert before == after + + +def _verification_count(result: str) -> float: + return ( + REGISTRY.get_sample_value( + "flagsmith_experimentation_warehouse_connection_verifications_total", + {"result": result}, + ) + or 0.0 + ) + + +def test_verify_clickhouse_connection__reachable__sets_connected( + clickhouse_connection: WarehouseConnection, + log: StructuredLogCapture, + mocker: MockerFixture, +) -> None: + # Given + mock_client = mocker.patch("experimentation.services.Client") + success_count_before = _verification_count("success") + + # When + verify_clickhouse_connection(clickhouse_connection) + + # Then + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.status == WarehouseConnectionStatus.CONNECTED + mock_client.assert_called_once_with( + "ch.example.com", + port=9440, + user="default", + password="hunter2", + database="flagsmith", + secure=True, + connect_timeout=5, + send_receive_timeout=30, + ) + mock_client.return_value.execute.assert_called_once_with("SELECT 1") + mock_client.return_value.disconnect.assert_called_once_with() + assert _verification_count("success") == success_count_before + 1 + assert { + "level": "info", + "event": "connection.verification_succeeded", + "environment__id": clickhouse_connection.environment_id, + "organisation__id": (clickhouse_connection.environment.project.organisation_id), + } in log.events + + +def test_verify_clickhouse_connection__driver_error__sets_errored( + clickhouse_connection: WarehouseConnection, + log: StructuredLogCapture, + mocker: MockerFixture, +) -> None: + # Given + mock_client = mocker.patch("experimentation.services.Client") + mock_client.return_value.execute.side_effect = Exception("connection refused") + failure_count_before = _verification_count("failure") + + # When + verify_clickhouse_connection(clickhouse_connection) + + # Then + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + mock_client.return_value.disconnect.assert_called_once_with() + assert _verification_count("failure") == failure_count_before + 1 + assert any( + event["event"] == "connection.verification_failed" for event in log.events + ) + + +def test_verify_clickhouse_connection__missing_credentials__sets_errored( + clickhouse_connection: WarehouseConnection, + mocker: MockerFixture, +) -> None: + # Given: credentials lost (e.g. undecryptable — the field loads them as None) + mocker.patch("experimentation.services.Client") + clickhouse_connection.credentials = None + clickhouse_connection.save() + + # When + verify_clickhouse_connection(clickhouse_connection) + + # Then + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + + +def test_verify_clickhouse_connection__environment_lookup_fails__sets_errored( + clickhouse_connection: WarehouseConnection, + mocker: MockerFixture, +) -> None: + # Given + mocker.patch("experimentation.services.Client") + mocker.patch.object( + Environment, + "project", + new_callable=mocker.PropertyMock, + side_effect=Exception("database unavailable"), + ) + failure_count_before = _verification_count("failure") + + # When + verify_clickhouse_connection(clickhouse_connection) + + # Then + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + assert _verification_count("failure") == failure_count_before + 1 diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 63f37d06c251..da7a66fcbf97 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -97,13 +97,12 @@ Attributes: - `environment.id` - `exc_info` - `experiment.id` - - `feature.id` - `organisation.id` ### `experimentation.results.compute_failed` Logged at `error` from: - - `api/experimentation/tasks.py:136` + - `api/experimentation/tasks.py:135` Attributes: - `environment.id` @@ -378,7 +377,7 @@ Attributes: ### `segment_membership.compute.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:149` + - `api/segment_membership/services.py:145` Attributes: - `project.id` @@ -388,7 +387,7 @@ Attributes: ### `segment_membership.members.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:215` + - `api/segment_membership/services.py:209` Attributes: - `reason` @@ -541,7 +540,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:803` + - `api/experimentation/services.py:785` Attributes: - `environment.id` @@ -550,7 +549,26 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:783` + - `api/experimentation/services.py:726` + +Attributes: + - `environment.id` + - `organisation.id` + +### `warehouse.connection.verification_failed` + +Logged at `warning` from: + - `api/experimentation/services.py:762` + +Attributes: + - `environment.id` + - `exc_info` + - `organisation.id` + +### `warehouse.connection.verification_succeeded` + +Logged at `info` from: + - `api/experimentation/services.py:770` Attributes: - `environment.id` @@ -559,7 +577,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:405` + - `api/experimentation/services.py:400` Attributes: - `environment.id` @@ -569,7 +587,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:391` + - `api/experimentation/services.py:386` Attributes: - `environment.id` diff --git a/docs/docs/deployment-self-hosting/observability/_metrics-catalogue.md b/docs/docs/deployment-self-hosting/observability/_metrics-catalogue.md index 649284cdac6b..ffe186d0d105 100644 --- a/docs/docs/deployment-self-hosting/observability/_metrics-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_metrics-catalogue.md @@ -34,6 +34,15 @@ Counter. Results of cache retrieval for environment document. `result` label is either `hit` or `miss`. +Labels: + - `result` + +### `flagsmith_experimentation_warehouse_connection_verifications` + +Counter. + +Outcomes of connection verification attempts against customers' own data warehouses. `result` label is either `success` or `failure`. + Labels: - `result` From ac2d89b5ff9606b743e3c4f27b6872cca4bd707a Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 14 Jul 2026 14:57:40 +0200 Subject: [PATCH 05/25] feat: wire ClickHouse verification into warehouse connection endpoints --- api/experimentation/serializers.py | 7 + api/experimentation/views.py | 11 + api/tests/unit/experimentation/test_views.py | 217 +++++++++++++++++++ 3 files changed, 235 insertions(+) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index fd6d87af9d47..7fd36f8f0ca1 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -74,6 +74,13 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: self._validate_credentials(attrs, warehouse_type) + if ( + warehouse_type != WarehouseType.CLICKHOUSE + and self.instance is not None + and getattr(self.instance, "credentials", None) is not None + ): + attrs["credentials"] = None + if "config" not in attrs and self.instance is not None: return attrs diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 897b3d0546e0..b115ccc9b1bc 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -62,6 +62,7 @@ mark_warehouse_pending_connection, refresh_warehouse_connection_status, transition_experiment_status, + verify_clickhouse_connection, ) from experimentation.tasks import ( compute_experiment_exposures, @@ -96,12 +97,19 @@ def perform_create(self, serializer: BaseSerializer[WarehouseConnection]) -> Non create_warehouse_audit_log( connection, self._get_user(self.request), action="created" ) + if connection.warehouse_type == WarehouseType.CLICKHOUSE: + verify_clickhouse_connection(connection) def perform_update(self, serializer: BaseSerializer[WarehouseConnection]) -> None: connection: WarehouseConnection = serializer.save() create_warehouse_audit_log( connection, self._get_user(self.request), action="updated" ) + if connection.warehouse_type == WarehouseType.CLICKHOUSE and ( + "config" in serializer.validated_data + or "credentials" in serializer.validated_data + ): + verify_clickhouse_connection(connection) def perform_destroy(self, instance: WarehouseConnection) -> None: create_warehouse_audit_log( @@ -130,6 +138,9 @@ def retrieve(self, request: Request, *args: object, **kwargs: object) -> Respons @action(detail=True, methods=["post"], url_path="test-warehouse-connection") def test_warehouse_connection(self, request: Request, **kwargs: object) -> Response: connection: WarehouseConnection = self.get_object() + if connection.warehouse_type == WarehouseType.CLICKHOUSE: + verify_clickhouse_connection(connection) + return Response(self.get_serializer(connection).data) if connection.warehouse_type != WarehouseType.FLAGSMITH: return Response( {"detail": "Test events are only supported for Flagsmith warehouses."}, diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index b386bd9e7ea9..134ac2b641cf 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1,4 +1,5 @@ import pytest +from django.db import connection as django_db_connection from django.urls import reverse from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture @@ -1437,3 +1438,219 @@ def test_post__flagsmith_empty_credentials__returns_400( assert response.json() == { "credentials": ["Only ClickHouse connections accept credentials."] } + + +def test_post__clickhouse_reachable__returns_201_connected( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + mocker: MockerFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mock_client = mocker.patch("experimentation.services.Client") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "name": "Production ClickHouse", + "config": {"host": "ch.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["status"] == "connected" + assert "credentials" not in response.json() + mock_client.return_value.execute.assert_called_once_with("SELECT 1") + + +def test_post__clickhouse_unreachable__returns_201_errored( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + mocker: MockerFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mock_client = mocker.patch("experimentation.services.Client") + mock_client.return_value.execute.side_effect = Exception("unreachable") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["status"] == "errored" + + +def test_post__clickhouse__password_stored_encrypted( + admin_client: APIClient, + credentials_encryption_keys: list[str], + enable_features: EnableFeaturesFixture, + mocker: MockerFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mocker.patch("experimentation.services.Client") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + with django_db_connection.cursor() as cursor: + cursor.execute( + "SELECT credentials FROM experimentation_warehouseconnection WHERE id = %s", + [response.json()["id"]], + ) + raw = cursor.fetchone()[0] + assert "hunter2" not in raw + + +def test_get__clickhouse__credentials_not_in_response( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.get(warehouse_connection_url) + + # Then + assert response.status_code == status.HTTP_200_OK + (data,) = response.json() + assert "credentials" not in data + assert "password" not in str(data) + + +def test_patch__clickhouse_config_without_credentials__keeps_stored_password( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + environment: Environment, + mocker: MockerFixture, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mock_client = mocker.patch("experimentation.services.Client") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, clickhouse_connection.id], + ) + + # When + response = admin_client.patch( + url, + data={"config": {"host": "ch.example.com", "port": 9000}}, + format="json", + ) + + # Then: re-verified with the stored password + assert response.status_code == status.HTTP_200_OK + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.credentials == {"password": "hunter2"} + assert mock_client.call_args.kwargs["password"] == "hunter2" + assert mock_client.call_args.kwargs["port"] == 9000 + + +def test_patch__clickhouse_name_only__does_not_reverify( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + environment: Environment, + mocker: MockerFixture, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mock_client = mocker.patch("experimentation.services.Client") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, clickhouse_connection.id], + ) + + # When + response = admin_client.patch(url, data={"name": "Renamed"}, format="json") + + # Then + assert response.status_code == status.HTTP_200_OK + mock_client.assert_not_called() + + +def test_test_warehouse_connection__clickhouse__reverifies_and_returns_status( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + environment: Environment, + mocker: MockerFixture, +) -> None: + # Given: a previously errored connection whose warehouse is now reachable + enable_features("experimentation_warehouse_connection") + clickhouse_connection.status = WarehouseConnectionStatus.ERRORED + clickhouse_connection.save() + mocker.patch("experimentation.services.Client") + url = reverse( + "api-v1:environments:experimentation:" + "warehouse-connections-test-warehouse-connection", + args=[environment.api_key, clickhouse_connection.id], + ) + + # When + response = admin_client.post(url, format="json") + + # Then + assert response.status_code == status.HTTP_200_OK + assert response.json()["status"] == "connected" + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.status == WarehouseConnectionStatus.CONNECTED + + +def test_patch__clickhouse_to_flagsmith__clears_stored_credentials( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + environment: Environment, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, clickhouse_connection.id], + ) + + # When + response = admin_client.patch( + url, + data={"warehouse_type": "flagsmith"}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.credentials is None From fb4c66364d8d4efcccd73135a4808a630617ee8c Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 15 Jul 2026 11:16:04 +0200 Subject: [PATCH 06/25] refactor: extract warehouse validation into a dedicated module --- api/experimentation/serializers.py | 101 ++------------------ api/experimentation/warehouse_validation.py | 101 ++++++++++++++++++++ 2 files changed, 108 insertions(+), 94 deletions(-) create mode 100644 api/experimentation/warehouse_validation.py diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 7fd36f8f0ca1..f7938baff9f3 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -1,6 +1,5 @@ from typing import Any -from django.conf import settings from django.db import transaction from django.db.models import QuerySet from rest_framework import serializers @@ -24,13 +23,10 @@ apply_experiment_rollout, get_experiment_rollout, ) -from experimentation.types import ( - CLICKHOUSE_DEFAULTS, - SNOWFLAKE_DEFAULTS, - ClickHouseConfig, - ClickHouseCredentials, - MetricExperimentResult, - SnowflakeConfig, +from experimentation.types import MetricExperimentResult +from experimentation.warehouse_validation import ( + CONFIG_VALIDATORS, + validate_credentials, ) from features.feature_states.serializers import ( FeatureValueSerializer, @@ -72,24 +68,15 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: getattr(self.instance, "warehouse_type", ""), ) - self._validate_credentials(attrs, warehouse_type) - - if ( - warehouse_type != WarehouseType.CLICKHOUSE - and self.instance is not None - and getattr(self.instance, "credentials", None) is not None - ): - attrs["credentials"] = None + validate_credentials(attrs, warehouse_type, self.instance) # type: ignore[arg-type] if "config" not in attrs and self.instance is not None: return attrs config: dict[str, Any] | None = attrs.get("config") - if warehouse_type == WarehouseType.SNOWFLAKE: - attrs["config"] = self._validate_snowflake_config(config or {}) - elif warehouse_type == WarehouseType.CLICKHOUSE: - attrs["config"] = self._validate_clickhouse_config(config or {}) + if config_validator := CONFIG_VALIDATORS.get(warehouse_type): + attrs["config"] = config_validator(config or {}) elif warehouse_type == WarehouseType.FLAGSMITH: if config: raise serializers.ValidationError( @@ -98,67 +85,6 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: attrs["config"] = None return attrs - def _validate_credentials(self, attrs: dict[str, Any], warehouse_type: str) -> None: - credentials: dict[str, Any] | None = attrs.get("credentials") - if warehouse_type != WarehouseType.CLICKHOUSE: - if credentials is not None: - raise serializers.ValidationError( - {"credentials": "Only ClickHouse connections accept credentials."} - ) - return - if not settings.CREDENTIALS_ENCRYPTION_KEYS: - raise serializers.ValidationError( - { - "credentials": ( - "Credentials encryption is not configured on this installation." - ) - } - ) - if ( - credentials is None - and self.instance is not None - and getattr(self.instance, "warehouse_type", "") == WarehouseType.CLICKHOUSE - ): - attrs.pop("credentials", None) - return - attrs["credentials"] = self._validate_clickhouse_credentials(credentials or {}) - - @staticmethod - def _validate_clickhouse_credentials( - credentials: dict[str, Any], - ) -> ClickHouseCredentials: - if not isinstance(credentials, dict): - raise serializers.ValidationError({"credentials": "Must be an object."}) - password = credentials.get("password") - if not password or not isinstance(password, str): - raise serializers.ValidationError( - {"credentials": {"password": "This field is required."}} - ) - return {"password": password} - - @staticmethod - def _validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: - if not isinstance(config, dict): - raise serializers.ValidationError({"config": "Must be an object."}) - if not config.get("host"): - raise serializers.ValidationError( - {"config": {"host": "This field is required."}} - ) - merged: ClickHouseConfig = { - **CLICKHOUSE_DEFAULTS, - **config, # type: ignore[typeddict-item] - } - port = merged["port"] - if ( - isinstance(port, bool) - or not isinstance(port, int) - or not (1 <= port <= 65535) - ): - raise serializers.ValidationError( - {"config": {"port": "Enter a valid port number (1-65535)."}} - ) - return merged - def create( self, validated_data: dict[str, Any], @@ -185,19 +111,6 @@ def _generate_name(warehouse_type: str, environment: Environment) -> str: label = WarehouseType(warehouse_type).label return f"{label} Warehouse - {environment.name}" - @staticmethod - def _validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: - account_identifier = config.get("account_identifier", "") - if not account_identifier: - raise serializers.ValidationError( - {"config": {"account_identifier": "This field is required."}} - ) - merged: SnowflakeConfig = { - **SNOWFLAKE_DEFAULTS, - **config, # type: ignore[typeddict-item] - } - return merged - class MetricSerializer(serializers.ModelSerializer): # type: ignore[type-arg] experiments = serializers.SerializerMethodField() diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py new file mode 100644 index 000000000000..e9a6220be5fd --- /dev/null +++ b/api/experimentation/warehouse_validation.py @@ -0,0 +1,101 @@ +from typing import Any, Callable + +from django.conf import settings +from rest_framework import serializers + +from experimentation.models import WarehouseConnection, WarehouseType +from experimentation.types import ( + CLICKHOUSE_DEFAULTS, + SNOWFLAKE_DEFAULTS, + ClickHouseConfig, + ClickHouseCredentials, + SnowflakeConfig, +) + + +def validate_clickhouse_credentials( + credentials: dict[str, Any], +) -> ClickHouseCredentials: + if not isinstance(credentials, dict): + raise serializers.ValidationError({"credentials": "Must be an object."}) + password = credentials.get("password") + if not password or not isinstance(password, str): + raise serializers.ValidationError( + {"credentials": {"password": "This field is required."}} + ) + return {"password": password} + + +def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: + if not isinstance(config, dict): + raise serializers.ValidationError({"config": "Must be an object."}) + if not config.get("host"): + raise serializers.ValidationError( + {"config": {"host": "This field is required."}} + ) + merged: ClickHouseConfig = { + **CLICKHOUSE_DEFAULTS, + **config, # type: ignore[typeddict-item] + } + port = merged["port"] + if isinstance(port, bool) or not isinstance(port, int) or not (1 <= port <= 65535): + raise serializers.ValidationError( + {"config": {"port": "Enter a valid port number (1-65535)."}} + ) + return merged + + +def validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: + account_identifier = config.get("account_identifier", "") + if not account_identifier: + raise serializers.ValidationError( + {"config": {"account_identifier": "This field is required."}} + ) + merged: SnowflakeConfig = { + **SNOWFLAKE_DEFAULTS, + **config, # type: ignore[typeddict-item] + } + return merged + + +CONFIG_VALIDATORS: dict[str, Callable[[dict[str, Any]], Any]] = { + WarehouseType.SNOWFLAKE: validate_snowflake_config, + WarehouseType.CLICKHOUSE: validate_clickhouse_config, +} + +CREDENTIAL_VALIDATORS: dict[str, Callable[[dict[str, Any]], Any]] = { + WarehouseType.CLICKHOUSE: validate_clickhouse_credentials, +} + + +def validate_credentials( + attrs: dict[str, Any], + warehouse_type: str, + instance: WarehouseConnection | None, +) -> None: + validator = CREDENTIAL_VALIDATORS.get(warehouse_type) + credentials: dict[str, Any] | None = attrs.get("credentials") + if validator is None: + if credentials is not None: + raise serializers.ValidationError( + {"credentials": "Only ClickHouse connections accept credentials."} + ) + if instance is not None and instance.credentials is not None: + attrs["credentials"] = None + return + if not settings.CREDENTIALS_ENCRYPTION_KEYS: + raise serializers.ValidationError( + { + "credentials": ( + "Credentials encryption is not configured on this installation." + ) + } + ) + if ( + credentials is None + and instance is not None + and instance.warehouse_type == warehouse_type + ): + attrs.pop("credentials", None) + return + attrs["credentials"] = validator(credentials or {}) From c543dc60f8af5e92c374f50f1b00c5a6bc4f6b1f Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 15 Jul 2026 11:29:40 +0200 Subject: [PATCH 07/25] test: neutral ClickHouse fixture data and bare Given/When/Then markers --- api/tests/unit/core/test_fields.py | 10 +++++----- api/tests/unit/experimentation/conftest.py | 6 +++--- api/tests/unit/experimentation/test_models.py | 6 +++--- api/tests/unit/experimentation/test_services.py | 8 ++++---- api/tests/unit/experimentation/test_views.py | 4 ++-- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py index e6424091bf24..334358ce3836 100644 --- a/api/tests/unit/core/test_fields.py +++ b/api/tests/unit/core/test_fields.py @@ -65,13 +65,13 @@ def test_get_prep_value__no_keys_configured__raises_improperly_configured( def test_from_db_value__new_key_prepended__old_token_still_decrypts( settings: SettingsWrapper, ) -> None: - # Given: a value encrypted under the original key + # Given old_key = Fernet.generate_key().decode() settings.CREDENTIALS_ENCRYPTION_KEYS = [old_key] field = EncryptedJSONField() stored = field.get_prep_value({"password": "hunter2"}) - # When: a new primary key is prepended to the ring + # When settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode(), old_key] # Then @@ -82,7 +82,7 @@ def test_from_db_value__token_key_removed_from_ring__returns_none_and_logs( settings: SettingsWrapper, log: StructuredLogCapture, ) -> None: - # Given: a value encrypted under a key no longer in the ring + # Given settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] field = EncryptedJSONField() stored = field.get_prep_value({"password": "hunter2"}) @@ -103,7 +103,7 @@ def test_from_db_value__malformed_key_in_ring__returns_none_and_logs( settings: SettingsWrapper, log: StructuredLogCapture, ) -> None: - # Given: a valid token, then a malformed key lands in the ring + # Given settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] field = EncryptedJSONField() stored = field.get_prep_value({"password": "hunter2"}) @@ -121,7 +121,7 @@ def test_from_db_value__no_keys_configured__returns_none_and_logs( settings: SettingsWrapper, log: StructuredLogCapture, ) -> None: - # Given: a stored token but an empty key ring + # Given settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] field = EncryptedJSONField() stored = field.get_prep_value({"password": "hunter2"}) diff --git a/api/tests/unit/experimentation/conftest.py b/api/tests/unit/experimentation/conftest.py index 256672c7cf93..491aadcc051a 100644 --- a/api/tests/unit/experimentation/conftest.py +++ b/api/tests/unit/experimentation/conftest.py @@ -112,10 +112,10 @@ def clickhouse_connection( warehouse_type=WarehouseType.CLICKHOUSE, name="Production ClickHouse", config={ - "host": "ch.example.com", + "host": "ch.acme-corp.example", "port": 9440, - "database": "flagsmith", - "username": "default", + "database": "acme_dwh", + "username": "acme_svc", "secure": True, }, credentials={"password": "hunter2"}, diff --git a/api/tests/unit/experimentation/test_models.py b/api/tests/unit/experimentation/test_models.py index bbedcad4efd5..a058e872010e 100644 --- a/api/tests/unit/experimentation/test_models.py +++ b/api/tests/unit/experimentation/test_models.py @@ -247,9 +247,9 @@ def test_experiment_results__is_final__reflects_window_coverage( def test_warehouse_connection_credentials__saved__ciphertext_in_db_and_roundtrips( clickhouse_connection: WarehouseConnection, ) -> None: - # Given: the fixture-created ClickHouse connection + # Given - # When: reading the raw column value + # When with django_db_connection.cursor() as cursor: cursor.execute( "SELECT credentials FROM experimentation_warehouseconnection WHERE id = %s", @@ -257,7 +257,7 @@ def test_warehouse_connection_credentials__saved__ciphertext_in_db_and_roundtrip ) raw = cursor.fetchone()[0] - # Then: the DB stores ciphertext, and the ORM decrypts on load + # Then assert "hunter2" not in raw clickhouse_connection.refresh_from_db() assert clickhouse_connection.credentials == {"password": "hunter2"} diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 35b0f5b11b0e..8735df51c440 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -2035,11 +2035,11 @@ def test_verify_clickhouse_connection__reachable__sets_connected( clickhouse_connection.refresh_from_db() assert clickhouse_connection.status == WarehouseConnectionStatus.CONNECTED mock_client.assert_called_once_with( - "ch.example.com", + "ch.acme-corp.example", port=9440, - user="default", + user="acme_svc", password="hunter2", - database="flagsmith", + database="acme_dwh", secure=True, connect_timeout=5, send_receive_timeout=30, @@ -2082,7 +2082,7 @@ def test_verify_clickhouse_connection__missing_credentials__sets_errored( clickhouse_connection: WarehouseConnection, mocker: MockerFixture, ) -> None: - # Given: credentials lost (e.g. undecryptable — the field loads them as None) + # Given mocker.patch("experimentation.services.Client") clickhouse_connection.credentials = None clickhouse_connection.save() diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 134ac2b641cf..94f9ed29e566 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1571,7 +1571,7 @@ def test_patch__clickhouse_config_without_credentials__keeps_stored_password( format="json", ) - # Then: re-verified with the stored password + # Then assert response.status_code == status.HTTP_200_OK clickhouse_connection.refresh_from_db() assert clickhouse_connection.credentials == {"password": "hunter2"} @@ -1609,7 +1609,7 @@ def test_test_warehouse_connection__clickhouse__reverifies_and_returns_status( environment: Environment, mocker: MockerFixture, ) -> None: - # Given: a previously errored connection whose warehouse is now reachable + # Given enable_features("experimentation_warehouse_connection") clickhouse_connection.status = WarehouseConnectionStatus.ERRORED clickhouse_connection.save() From d7a91dc3cd587554253374c048f05f984d3513cd Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 15 Jul 2026 18:02:02 +0200 Subject: [PATCH 08/25] refactor: encrypt warehouse credentials with a SECRET_KEY-derived Fernet key --- api/app/settings/common.py | 10 +-- api/core/fields.py | 19 ++-- api/experimentation/warehouse_validation.py | 5 +- api/tests/unit/core/test_fields.py | 86 ++----------------- api/tests/unit/experimentation/conftest.py | 10 +-- api/tests/unit/experimentation/test_views.py | 36 ++++---- .../observability/_events-catalogue.md | 2 +- 7 files changed, 44 insertions(+), 124 deletions(-) diff --git a/api/app/settings/common.py b/api/app/settings/common.py index f325b02ab0e4..46f4c5105900 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -56,7 +56,10 @@ # Enables gzip compression ENABLE_GZIP_COMPRESSION = env.bool("ENABLE_GZIP_COMPRESSION", default=False) -SECRET_KEY = env("DJANGO_SECRET_KEY", default=get_random_secret_key()) +SECRET_KEY = env.str("DJANGO_SECRET_KEY", default="") +SECRET_KEY_IS_EPHEMERAL = not SECRET_KEY +if SECRET_KEY_IS_EPHEMERAL: + SECRET_KEY = get_random_secret_key() HOSTED_SEATS_LIMIT = env.int("HOSTED_SEATS_LIMIT", default=0) @@ -1505,11 +1508,6 @@ # https://github.com/Flagsmith/flagsmith/issues/8033 EXPERIMENTATION_CLICKHOUSE_URL = env.str("EXPERIMENTATION_CLICKHOUSE_URL", default=None) -# Comma-separated Fernet keys: the first encrypts, all decrypt (rotation). -CREDENTIALS_ENCRYPTION_KEYS: list[str] = env.list( - "CREDENTIALS_ENCRYPTION_KEYS", default=[] -) - SEGMENT_MEMBERSHIP_REFRESH_INTERVAL_HOURS = env.int( "SEGMENT_MEMBERSHIP_REFRESH_INTERVAL_HOURS", default=6 ) diff --git a/api/core/fields.py b/api/core/fields.py index b4e411a37ff6..637d0c282865 100644 --- a/api/core/fields.py +++ b/api/core/fields.py @@ -1,27 +1,22 @@ +import base64 +import hashlib import json from typing import Any import structlog -from cryptography.fernet import Fernet, InvalidToken, MultiFernet +from cryptography.fernet import Fernet, InvalidToken from django.conf import settings -from django.core.exceptions import ImproperlyConfigured from django.db import models logger = structlog.get_logger("core") -def _get_fernet() -> MultiFernet: - if not settings.CREDENTIALS_ENCRYPTION_KEYS: - raise ImproperlyConfigured( - "CREDENTIALS_ENCRYPTION_KEYS must be set to store encrypted fields." - ) - return MultiFernet([Fernet(key) for key in settings.CREDENTIALS_ENCRYPTION_KEYS]) +def _get_fernet() -> Fernet: + digest = hashlib.sha256(settings.SECRET_KEY.encode()).digest() + return Fernet(base64.urlsafe_b64encode(digest)) class EncryptedJSONField(models.TextField[Any, Any]): - """A JSON value stored Fernet-encrypted in a text column; a value whose - key left the ring loads as None rather than raising.""" - def get_prep_value(self, value: Any) -> str | None: if value is None: return None @@ -37,7 +32,7 @@ def from_db_value( return None try: plaintext = _get_fernet().decrypt(value.encode()) - except (InvalidToken, ImproperlyConfigured, ValueError): + except InvalidToken: logger.warning("encrypted_field.decrypt_failed", exc_info=True) return None return json.loads(plaintext) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index e9a6220be5fd..708e87e694d9 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -83,11 +83,12 @@ def validate_credentials( if instance is not None and instance.credentials is not None: attrs["credentials"] = None return - if not settings.CREDENTIALS_ENCRYPTION_KEYS: + if settings.SECRET_KEY_IS_EPHEMERAL: raise serializers.ValidationError( { "credentials": ( - "Credentials encryption is not configured on this installation." + "Storing credentials requires the DJANGO_SECRET_KEY " + "environment variable to be set." ) } ) diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py index 334358ce3836..3e664aeb7f0d 100644 --- a/api/tests/unit/core/test_fields.py +++ b/api/tests/unit/core/test_fields.py @@ -1,6 +1,4 @@ import pytest -from cryptography.fernet import Fernet -from django.core.exceptions import ImproperlyConfigured from pytest_django.fixtures import SettingsWrapper from pytest_structlog import StructuredLogCapture @@ -8,14 +6,12 @@ @pytest.fixture() -def encryption_keys(settings: SettingsWrapper) -> list[str]: - keys = [Fernet.generate_key().decode()] - settings.CREDENTIALS_ENCRYPTION_KEYS = keys - return keys +def stable_secret_key(settings: SettingsWrapper) -> None: + settings.SECRET_KEY = "test-django-secret-key" def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips( - encryption_keys: list[str], + stable_secret_key: None, ) -> None: # Given field = EncryptedJSONField() @@ -31,7 +27,7 @@ def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips( def test_get_prep_value__none__returns_none( - encryption_keys: list[str], + stable_secret_key: None, ) -> None: # Given field = EncryptedJSONField() @@ -41,7 +37,7 @@ def test_get_prep_value__none__returns_none( def test_from_db_value__none__returns_none( - encryption_keys: list[str], + stable_secret_key: None, ) -> None: # Given field = EncryptedJSONField() @@ -50,43 +46,15 @@ def test_from_db_value__none__returns_none( assert field.from_db_value(None, None, None) is None -def test_get_prep_value__no_keys_configured__raises_improperly_configured( - settings: SettingsWrapper, -) -> None: - # Given - settings.CREDENTIALS_ENCRYPTION_KEYS = [] - field = EncryptedJSONField() - - # When & Then - with pytest.raises(ImproperlyConfigured): - field.get_prep_value({"password": "hunter2"}) - - -def test_from_db_value__new_key_prepended__old_token_still_decrypts( - settings: SettingsWrapper, -) -> None: - # Given - old_key = Fernet.generate_key().decode() - settings.CREDENTIALS_ENCRYPTION_KEYS = [old_key] - field = EncryptedJSONField() - stored = field.get_prep_value({"password": "hunter2"}) - - # When - settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode(), old_key] - - # Then - assert field.from_db_value(stored, None, None) == {"password": "hunter2"} - - -def test_from_db_value__token_key_removed_from_ring__returns_none_and_logs( +def test_from_db_value__secret_key_changed__returns_none_and_logs( settings: SettingsWrapper, log: StructuredLogCapture, ) -> None: # Given - settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] + settings.SECRET_KEY = "old-secret" field = EncryptedJSONField() stored = field.get_prep_value({"password": "hunter2"}) - settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] + settings.SECRET_KEY = "new-secret" # When value = field.from_db_value(stored, None, None) @@ -99,44 +67,8 @@ def test_from_db_value__token_key_removed_from_ring__returns_none_and_logs( } in [{"level": e["level"], "event": e["event"]} for e in log.events] -def test_from_db_value__malformed_key_in_ring__returns_none_and_logs( - settings: SettingsWrapper, - log: StructuredLogCapture, -) -> None: - # Given - settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] - field = EncryptedJSONField() - stored = field.get_prep_value({"password": "hunter2"}) - settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode(), ""] - - # When - value = field.from_db_value(stored, None, None) - - # Then - assert value is None - assert any(e["event"] == "encrypted_field.decrypt_failed" for e in log.events) - - -def test_from_db_value__no_keys_configured__returns_none_and_logs( - settings: SettingsWrapper, - log: StructuredLogCapture, -) -> None: - # Given - settings.CREDENTIALS_ENCRYPTION_KEYS = [Fernet.generate_key().decode()] - field = EncryptedJSONField() - stored = field.get_prep_value({"password": "hunter2"}) - settings.CREDENTIALS_ENCRYPTION_KEYS = [] - - # When - value = field.from_db_value(stored, None, None) - - # Then - assert value is None - assert any(e["event"] == "encrypted_field.decrypt_failed" for e in log.events) - - def test_get_lookup__non_isnull__raises_not_implemented( - encryption_keys: list[str], + stable_secret_key: None, ) -> None: # Given field = EncryptedJSONField() diff --git a/api/tests/unit/experimentation/conftest.py b/api/tests/unit/experimentation/conftest.py index 491aadcc051a..4ec4e767cae7 100644 --- a/api/tests/unit/experimentation/conftest.py +++ b/api/tests/unit/experimentation/conftest.py @@ -1,5 +1,4 @@ import pytest -from cryptography.fernet import Fernet from django.urls import reverse from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture @@ -96,16 +95,15 @@ def experiment_with_rollout( @pytest.fixture() -def credentials_encryption_keys(settings: SettingsWrapper) -> list[str]: - keys = [Fernet.generate_key().decode()] - settings.CREDENTIALS_ENCRYPTION_KEYS = keys - return keys +def stable_secret_key(settings: SettingsWrapper) -> None: + settings.SECRET_KEY = "test-django-secret-key" + settings.SECRET_KEY_IS_EPHEMERAL = False @pytest.fixture() def clickhouse_connection( environment: Environment, - credentials_encryption_keys: list[str], + stable_secret_key: None, ) -> WarehouseConnection: connection: WarehouseConnection = WarehouseConnection.objects.create( environment=environment, diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 94f9ed29e566..1857b207f617 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1131,7 +1131,7 @@ def test_get__clickhouse_errors__returns_200_without_stats( def test_post__clickhouse_minimal_config__applies_defaults( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, environment: Environment, mocker: MockerFixture, @@ -1167,7 +1167,7 @@ def test_post__clickhouse_minimal_config__applies_defaults( def test_post__clickhouse_missing_host__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1192,7 +1192,7 @@ def test_post__clickhouse_missing_host__returns_400( @pytest.mark.parametrize("port", [0, 65536, "not-a-port", True]) def test_post__clickhouse_invalid_port__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, port: object, warehouse_connection_url: str, @@ -1218,7 +1218,7 @@ def test_post__clickhouse_invalid_port__returns_400( def test_post__clickhouse_missing_password__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1240,7 +1240,7 @@ def test_post__clickhouse_missing_password__returns_400( assert "password" in response.json()["credentials"] -def test_post__clickhouse_no_encryption_keys__returns_400( +def test_post__clickhouse_ephemeral_secret_key__returns_400( admin_client: APIClient, enable_features: EnableFeaturesFixture, settings: SettingsWrapper, @@ -1248,7 +1248,7 @@ def test_post__clickhouse_no_encryption_keys__returns_400( ) -> None: # Given enable_features("experimentation_warehouse_connection") - settings.CREDENTIALS_ENCRYPTION_KEYS = [] + settings.SECRET_KEY_IS_EPHEMERAL = True # When response = admin_client.post( @@ -1263,11 +1263,7 @@ def test_post__clickhouse_no_encryption_keys__returns_400( # Then assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == { - "credentials": [ - "Credentials encryption is not configured on this installation." - ] - } + assert "credentials" in response.json() @pytest.mark.parametrize( @@ -1280,7 +1276,7 @@ def test_post__clickhouse_no_encryption_keys__returns_400( ) def test_post__non_clickhouse_with_credentials__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_type: str, config: dict[str, str] | None, @@ -1309,7 +1305,7 @@ def test_post__non_clickhouse_with_credentials__returns_400( def test_post__clickhouse_no_name__auto_generates_name( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, environment: Environment, mocker: MockerFixture, @@ -1337,7 +1333,7 @@ def test_post__clickhouse_no_name__auto_generates_name( def test_post__clickhouse_non_dict_credentials__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1362,7 +1358,7 @@ def test_post__clickhouse_non_dict_credentials__returns_400( def test_post__clickhouse_non_dict_config__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1387,7 +1383,7 @@ def test_post__clickhouse_non_dict_config__returns_400( def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, environment: Environment, warehouse_connection: WarehouseConnection, @@ -1416,7 +1412,7 @@ def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( def test_post__flagsmith_empty_credentials__returns_400( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1442,7 +1438,7 @@ def test_post__flagsmith_empty_credentials__returns_400( def test_post__clickhouse_reachable__returns_201_connected( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, mocker: MockerFixture, warehouse_connection_url: str, @@ -1472,7 +1468,7 @@ def test_post__clickhouse_reachable__returns_201_connected( def test_post__clickhouse_unreachable__returns_201_errored( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, mocker: MockerFixture, warehouse_connection_url: str, @@ -1500,7 +1496,7 @@ def test_post__clickhouse_unreachable__returns_201_errored( def test_post__clickhouse__password_stored_encrypted( admin_client: APIClient, - credentials_encryption_keys: list[str], + stable_secret_key: None, enable_features: EnableFeaturesFixture, mocker: MockerFixture, warehouse_connection_url: str, diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index da7a66fcbf97..9cf5c2a49fa9 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -74,7 +74,7 @@ Attributes: ### `core.encrypted_field.decrypt_failed` Logged at `warning` from: - - `api/core/fields.py:41` + - `api/core/fields.py:36` Attributes: - `exc_info` From 9a09a32965cc94d3984abb5f1c18e2c48ac92ede Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 10:28:44 +0200 Subject: [PATCH 09/25] refactor: drop the ephemeral SECRET_KEY guard for warehouse credentials --- api/app/settings/common.py | 5 +-- api/experimentation/warehouse_validation.py | 10 ----- api/tests/unit/core/test_fields.py | 21 ++--------- api/tests/unit/experimentation/conftest.py | 8 ---- api/tests/unit/experimentation/test_views.py | 39 -------------------- 5 files changed, 5 insertions(+), 78 deletions(-) diff --git a/api/app/settings/common.py b/api/app/settings/common.py index 46f4c5105900..5adde3332224 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -56,10 +56,7 @@ # Enables gzip compression ENABLE_GZIP_COMPRESSION = env.bool("ENABLE_GZIP_COMPRESSION", default=False) -SECRET_KEY = env.str("DJANGO_SECRET_KEY", default="") -SECRET_KEY_IS_EPHEMERAL = not SECRET_KEY -if SECRET_KEY_IS_EPHEMERAL: - SECRET_KEY = get_random_secret_key() +SECRET_KEY = env("DJANGO_SECRET_KEY", default=get_random_secret_key()) HOSTED_SEATS_LIMIT = env.int("HOSTED_SEATS_LIMIT", default=0) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 708e87e694d9..907702511552 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -1,6 +1,5 @@ from typing import Any, Callable -from django.conf import settings from rest_framework import serializers from experimentation.models import WarehouseConnection, WarehouseType @@ -83,15 +82,6 @@ def validate_credentials( if instance is not None and instance.credentials is not None: attrs["credentials"] = None return - if settings.SECRET_KEY_IS_EPHEMERAL: - raise serializers.ValidationError( - { - "credentials": ( - "Storing credentials requires the DJANGO_SECRET_KEY " - "environment variable to be set." - ) - } - ) if ( credentials is None and instance is not None diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py index 3e664aeb7f0d..4cd57836eacb 100644 --- a/api/tests/unit/core/test_fields.py +++ b/api/tests/unit/core/test_fields.py @@ -5,14 +5,7 @@ from core.fields import EncryptedJSONField -@pytest.fixture() -def stable_secret_key(settings: SettingsWrapper) -> None: - settings.SECRET_KEY = "test-django-secret-key" - - -def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips( - stable_secret_key: None, -) -> None: +def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips() -> None: # Given field = EncryptedJSONField() value = {"password": "hunter2"} @@ -26,9 +19,7 @@ def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips( assert field.from_db_value(stored, None, None) == value -def test_get_prep_value__none__returns_none( - stable_secret_key: None, -) -> None: +def test_get_prep_value__none__returns_none() -> None: # Given field = EncryptedJSONField() @@ -36,9 +27,7 @@ def test_get_prep_value__none__returns_none( assert field.get_prep_value(None) is None -def test_from_db_value__none__returns_none( - stable_secret_key: None, -) -> None: +def test_from_db_value__none__returns_none() -> None: # Given field = EncryptedJSONField() @@ -67,9 +56,7 @@ def test_from_db_value__secret_key_changed__returns_none_and_logs( } in [{"level": e["level"], "event": e["event"]} for e in log.events] -def test_get_lookup__non_isnull__raises_not_implemented( - stable_secret_key: None, -) -> None: +def test_get_lookup__non_isnull__raises_not_implemented() -> None: # Given field = EncryptedJSONField() diff --git a/api/tests/unit/experimentation/conftest.py b/api/tests/unit/experimentation/conftest.py index 4ec4e767cae7..04d814c42ce6 100644 --- a/api/tests/unit/experimentation/conftest.py +++ b/api/tests/unit/experimentation/conftest.py @@ -1,6 +1,5 @@ import pytest from django.urls import reverse -from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from core.dataclasses import AuthorData @@ -94,16 +93,9 @@ def experiment_with_rollout( return experiment -@pytest.fixture() -def stable_secret_key(settings: SettingsWrapper) -> None: - settings.SECRET_KEY = "test-django-secret-key" - settings.SECRET_KEY_IS_EPHEMERAL = False - - @pytest.fixture() def clickhouse_connection( environment: Environment, - stable_secret_key: None, ) -> WarehouseConnection: connection: WarehouseConnection = WarehouseConnection.objects.create( environment=environment, diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 1857b207f617..c1c196f6855f 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1131,7 +1131,6 @@ def test_get__clickhouse_errors__returns_200_without_stats( def test_post__clickhouse_minimal_config__applies_defaults( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, environment: Environment, mocker: MockerFixture, @@ -1167,7 +1166,6 @@ def test_post__clickhouse_minimal_config__applies_defaults( def test_post__clickhouse_missing_host__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1192,7 +1190,6 @@ def test_post__clickhouse_missing_host__returns_400( @pytest.mark.parametrize("port", [0, 65536, "not-a-port", True]) def test_post__clickhouse_invalid_port__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, port: object, warehouse_connection_url: str, @@ -1218,7 +1215,6 @@ def test_post__clickhouse_invalid_port__returns_400( def test_post__clickhouse_missing_password__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1240,32 +1236,6 @@ def test_post__clickhouse_missing_password__returns_400( assert "password" in response.json()["credentials"] -def test_post__clickhouse_ephemeral_secret_key__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - settings: SettingsWrapper, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - settings.SECRET_KEY_IS_EPHEMERAL = True - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com"}, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "credentials" in response.json() - - @pytest.mark.parametrize( "warehouse_type, config", [ @@ -1276,7 +1246,6 @@ def test_post__clickhouse_ephemeral_secret_key__returns_400( ) def test_post__non_clickhouse_with_credentials__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_type: str, config: dict[str, str] | None, @@ -1305,7 +1274,6 @@ def test_post__non_clickhouse_with_credentials__returns_400( def test_post__clickhouse_no_name__auto_generates_name( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, environment: Environment, mocker: MockerFixture, @@ -1333,7 +1301,6 @@ def test_post__clickhouse_no_name__auto_generates_name( def test_post__clickhouse_non_dict_credentials__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1358,7 +1325,6 @@ def test_post__clickhouse_non_dict_credentials__returns_400( def test_post__clickhouse_non_dict_config__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1383,7 +1349,6 @@ def test_post__clickhouse_non_dict_config__returns_400( def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, environment: Environment, warehouse_connection: WarehouseConnection, @@ -1412,7 +1377,6 @@ def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( def test_post__flagsmith_empty_credentials__returns_400( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, warehouse_connection_url: str, ) -> None: @@ -1438,7 +1402,6 @@ def test_post__flagsmith_empty_credentials__returns_400( def test_post__clickhouse_reachable__returns_201_connected( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, mocker: MockerFixture, warehouse_connection_url: str, @@ -1468,7 +1431,6 @@ def test_post__clickhouse_reachable__returns_201_connected( def test_post__clickhouse_unreachable__returns_201_errored( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, mocker: MockerFixture, warehouse_connection_url: str, @@ -1496,7 +1458,6 @@ def test_post__clickhouse_unreachable__returns_201_errored( def test_post__clickhouse__password_stored_encrypted( admin_client: APIClient, - stable_secret_key: None, enable_features: EnableFeaturesFixture, mocker: MockerFixture, warehouse_connection_url: str, From dfa81af253e54afee146e67e47c84723873f01d4 Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 10:48:14 +0200 Subject: [PATCH 10/25] fix: reject internal network addresses for ClickHouse warehouse hosts --- api/experimentation/warehouse_validation.py | 11 ++++ api/tests/unit/experimentation/test_views.py | 60 ++++++++++++++++++++ api/webhooks/fields.py | 47 +++++++-------- 3 files changed, 95 insertions(+), 23 deletions(-) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 907702511552..cc54b14217c6 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -10,6 +10,7 @@ ClickHouseCredentials, SnowflakeConfig, ) +from webhooks.fields import is_internal_address def validate_clickhouse_credentials( @@ -32,6 +33,16 @@ def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: raise serializers.ValidationError( {"config": {"host": "This field is required."}} ) + if is_internal_address(str(config["host"])): + raise serializers.ValidationError( + { + "config": { + "host": ( + "Host must not target internal or private network addresses." + ) + } + } + ) merged: ClickHouseConfig = { **CLICKHOUSE_DEFAULTS, **config, # type: ignore[typeddict-item] diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index c1c196f6855f..2e8702be6112 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1,3 +1,5 @@ +import socket + import pytest from django.db import connection as django_db_connection from django.urls import reverse @@ -1187,6 +1189,64 @@ def test_post__clickhouse_missing_host__returns_400( assert "host" in response.json()["config"] +@pytest.mark.parametrize( + "host", + ["127.0.0.1", "10.0.0.1", "169.254.169.254"], +) +def test_post__clickhouse_internal_host__returns_400( + admin_client: APIClient, + enable_features: EnableFeaturesFixture, + host: str, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": host}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "host" in response.json()["config"] + + +def test_post__clickhouse_hostname_resolves_to_internal_ip__returns_400( + admin_client: APIClient, + enable_features: EnableFeaturesFixture, + mocker: MockerFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mocker.patch( + "webhooks.fields.socket.getaddrinfo", + return_value=[(socket.AF_INET, None, None, None, ("192.168.1.100", 0))], + ) + + # When + response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "clickhouse", + "config": {"host": "internal.example.com"}, + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "host" in response.json()["config"] + + @pytest.mark.parametrize("port", [0, 65536, "not-a-port", True]) def test_post__clickhouse_invalid_port__returns_400( admin_client: APIClient, diff --git a/api/webhooks/fields.py b/api/webhooks/fields.py index 8729c1262b0a..d552d64238a2 100644 --- a/api/webhooks/fields.py +++ b/api/webhooks/fields.py @@ -5,6 +5,28 @@ from rest_framework import serializers +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 + ) + + class NoSSRFURLField(serializers.URLField): """ A URL field that rejects URLs resolving to internal network addresses, @@ -27,26 +49,5 @@ def run_validators(self, value: str) -> None: super().run_validators(value) hostname = urlparse(value).hostname or "" - - 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 - - for ip in ips: - if ( - ip.is_loopback - or ip.is_private - or ip.is_link_local - or ip.is_reserved - or ip.is_multicast - ): - self.fail("internal_address") + if is_internal_address(hostname): + self.fail("internal_address") From cf0b5ee70020d938baf54547dc9c8a329b67b92c Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 10:59:54 +0200 Subject: [PATCH 11/25] feat: throttle the warehouse test-connection endpoint --- api/app/settings/common.py | 1 + api/app/settings/test.py | 1 + api/experimentation/views.py | 7 ++++ api/tests/unit/experimentation/test_views.py | 34 ++++++++++++++++++++ 4 files changed, 43 insertions(+) diff --git a/api/app/settings/common.py b/api/app/settings/common.py index 5adde3332224..bcc51dc93c5e 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -361,6 +361,7 @@ "invite": "10/min", "user": USER_THROTTLE_RATE, "influx_query": "5/min", + "warehouse_connection_test": "10/min", }, "DEFAULT_FILTER_BACKENDS": ["django_filters.rest_framework.DjangoFilterBackend"], "DEFAULT_RENDERER_CLASSES": [ diff --git a/api/app/settings/test.py b/api/app/settings/test.py index c659649528a0..854ad2d7da85 100644 --- a/api/app/settings/test.py +++ b/api/app/settings/test.py @@ -27,6 +27,7 @@ "user": "100000/day", "master_api_key": "100000/day", "influx_query": "50/min", + "warehouse_connection_test": "100/min", } AWS_SSE_LOGS_BUCKET_NAME = "test_bucket" diff --git a/api/experimentation/views.py b/api/experimentation/views.py index b115ccc9b1bc..1805a88c509d 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -15,6 +15,7 @@ from rest_framework.request import Request from rest_framework.response import Response from rest_framework.serializers import BaseSerializer +from rest_framework.throttling import BaseThrottle, ScopedRateThrottle from rest_framework.viewsets import GenericViewSet from app.pagination import CustomPagination @@ -90,6 +91,12 @@ class WarehouseConnectionViewSet( lookup_field = "id" lookup_url_kwarg = "connection_id" + def get_throttles(self) -> list[BaseThrottle]: + if self.action == "test_warehouse_connection": + self.throttle_scope = "warehouse_connection_test" + return [ScopedRateThrottle()] + return super().get_throttles() + def perform_create(self, serializer: BaseSerializer[WarehouseConnection]) -> None: connection: WarehouseConnection = serializer.save( environment=self._get_environment() diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 2e8702be6112..e2b9998554ee 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -7,6 +7,7 @@ from pytest_mock import MockerFixture from rest_framework import status from rest_framework.test import APIClient +from rest_framework.throttling import ScopedRateThrottle from audit.models import AuditLog from audit.related_object_type import RelatedObjectType @@ -18,6 +19,7 @@ WarehouseConnectionStatus, WarehouseType, ) +from experimentation.views import WarehouseConnectionViewSet from tests.types import EnableFeaturesFixture pytestmark = pytest.mark.django_db @@ -1050,6 +1052,38 @@ def test_test_warehouse_connection__non_admin__returns_403( assert response.status_code == status.HTTP_403_FORBIDDEN +def test_get_throttles__test_connection_action__returns_scoped_throttle() -> None: + # Given + view = WarehouseConnectionViewSet() + view.action = "test_warehouse_connection" + + # When + throttles = view.get_throttles() + + # Then + assert len(throttles) == 1 + assert isinstance(throttles[0], ScopedRateThrottle) + assert view.throttle_scope == "warehouse_connection_test" + + +@pytest.mark.parametrize( + "action", + ["list", "retrieve", "create", "update", "destroy"], +) +def test_get_throttles__other_actions__returns_view_default_throttles( + action: str, +) -> None: + # Given + view = WarehouseConnectionViewSet() + view.action = action + + # When + throttles = view.get_throttles() + + # Then + assert [type(throttle) for throttle in throttles] == list(view.throttle_classes) + + def test_get__clickhouse_unconfigured__returns_200_without_stats( admin_client: APIClient, environment: Environment, From d9ed01880ed6e91e8dbf9f97752df1da54973418 Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 11:10:06 +0200 Subject: [PATCH 12/25] feat: record why warehouse connection verification failed --- ...0011_warehouse_connection_status_detail.py | 17 +++++ api/experimentation/models.py | 1 + api/experimentation/serializers.py | 3 +- api/experimentation/services.py | 25 ++++++- .../unit/experimentation/test_services.py | 75 ++++++++++++++++++- api/tests/unit/experimentation/test_views.py | 2 + .../observability/_events-catalogue.md | 12 +-- 7 files changed, 124 insertions(+), 11 deletions(-) create mode 100644 api/experimentation/migrations/0011_warehouse_connection_status_detail.py diff --git a/api/experimentation/migrations/0011_warehouse_connection_status_detail.py b/api/experimentation/migrations/0011_warehouse_connection_status_detail.py new file mode 100644 index 000000000000..579fc28f37b0 --- /dev/null +++ b/api/experimentation/migrations/0011_warehouse_connection_status_detail.py @@ -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), + ), + ] diff --git a/api/experimentation/models.py b/api/experimentation/models.py index a796190affa2..75501d1d5048 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -55,6 +55,7 @@ class WarehouseConnection(LifecycleModelMixin, SoftDeleteExportableModel): # ty choices=WarehouseConnectionStatus.choices, default=WarehouseConnectionStatus.CREATED, ) + status_detail = models.CharField(max_length=255, null=True, blank=True) name = models.CharField(max_length=255) config: models.JSONField[dict[str, object] | None, dict[str, object] | None] = ( models.JSONField(null=True, blank=True) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index f7938baff9f3..3ed1bb16228e 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -53,6 +53,7 @@ class Meta: "id", "warehouse_type", "status", + "status_detail", "name", "config", "credentials", @@ -60,7 +61,7 @@ class Meta: "total_events_received", "unique_events_count", ) - read_only_fields = ("id", "status", "created_at") + read_only_fields = ("id", "status", "status_detail", "created_at") def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: warehouse_type: str = attrs.get( diff --git a/api/experimentation/services.py b/api/experimentation/services.py index 4c37caf5caf1..7b1b2cf62380 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -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 @@ -792,6 +793,22 @@ def mark_warehouse_pending_connection( return connection +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, 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.""" @@ -814,9 +831,10 @@ def verify_clickhouse_connection(connection: WarehouseConnection) -> None: client.execute("SELECT 1") finally: client.disconnect() - except Exception: + except Exception as error: connection.status = WarehouseConnectionStatus.ERRORED - connection.save(update_fields=["status"]) + connection.status_detail = _describe_verification_error(error) + connection.save(update_fields=["status", "status_detail"]) flagsmith_experimentation_warehouse_connection_verifications_total.labels( result="failure" ).inc() @@ -824,7 +842,8 @@ def verify_clickhouse_connection(connection: WarehouseConnection) -> None: return connection.status = WarehouseConnectionStatus.CONNECTED - connection.save(update_fields=["status"]) + connection.status_detail = None + connection.save(update_fields=["status", "status_detail"]) flagsmith_experimentation_warehouse_connection_verifications_total.labels( result="success" ).inc() diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 8735df51c440..0a61da32018e 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -3,6 +3,7 @@ from unittest.mock import MagicMock import pytest +from clickhouse_driver import errors as clickhouse_errors from django.db.models import Q from flag_engine.segments.constants import PERCENTAGE_SPLIT from prometheus_client import REGISTRY @@ -37,7 +38,10 @@ WarehouseType, ) from experimentation.results_query import _MetricSlot -from experimentation.services import verify_clickhouse_connection +from experimentation.services import ( + _describe_verification_error, + verify_clickhouse_connection, +) from experimentation.stats import VariantStats from features.feature_types import MULTIVARIATE from features.models import Feature, FeatureState @@ -2027,6 +2031,8 @@ def test_verify_clickhouse_connection__reachable__sets_connected( # Given mock_client = mocker.patch("experimentation.services.Client") success_count_before = _verification_count("success") + clickhouse_connection.status_detail = "stale detail" + clickhouse_connection.save() # When verify_clickhouse_connection(clickhouse_connection) @@ -2034,6 +2040,7 @@ def test_verify_clickhouse_connection__reachable__sets_connected( # Then clickhouse_connection.refresh_from_db() assert clickhouse_connection.status == WarehouseConnectionStatus.CONNECTED + assert clickhouse_connection.status_detail is None mock_client.assert_called_once_with( "ch.acme-corp.example", port=9440, @@ -2071,6 +2078,7 @@ def test_verify_clickhouse_connection__driver_error__sets_errored( # Then clickhouse_connection.refresh_from_db() assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + assert clickhouse_connection.status_detail == "Verification failed." mock_client.return_value.disconnect.assert_called_once_with() assert _verification_count("failure") == failure_count_before + 1 assert any( @@ -2093,6 +2101,10 @@ def test_verify_clickhouse_connection__missing_credentials__sets_errored( # Then clickhouse_connection.refresh_from_db() assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + assert ( + clickhouse_connection.status_detail + == "Stored connection details are incomplete." + ) def test_verify_clickhouse_connection__environment_lookup_fails__sets_errored( @@ -2115,4 +2127,65 @@ def test_verify_clickhouse_connection__environment_lookup_fails__sets_errored( # Then clickhouse_connection.refresh_from_db() assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + assert clickhouse_connection.status_detail == "Verification failed." assert _verification_count("failure") == failure_count_before + 1 + + +@pytest.mark.parametrize( + "error,expected_detail", + [ + ( + clickhouse_errors.ServerException("Authentication failed", code=516), + "Authentication failed.", + ), + ( + clickhouse_errors.ServerException( + "Database not_a_real_db does not exist", code=81 + ), + "Database does not exist.", + ), + ( + clickhouse_errors.ServerException("Some other server error", code=999), + "The ClickHouse server rejected the request.", + ), + ( + clickhouse_errors.SocketTimeoutError("(10.255.255.1:9000)"), + "The connection timed out.", + ), + ( + TimeoutError("timed out"), + "The connection timed out.", + ), + ( + clickhouse_errors.NetworkError("Connection refused"), + "Could not connect to the host.", + ), + ( + KeyError("host"), + "Stored connection details are incomplete.", + ), + ( + ValueError("unexpected"), + "Verification failed.", + ), + ], + ids=[ + "server_exception_auth_failure", + "server_exception_unknown_database", + "server_exception_other", + "socket_timeout_error", + "builtin_timeout_error", + "network_error", + "key_error", + "generic_exception", + ], +) +def test_describe_verification_error__known_error_types__returns_expected_detail( + error: Exception, + expected_detail: str, +) -> None: + # Given / When + detail = _describe_verification_error(error) + + # Then + assert detail == expected_detail diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index e2b9998554ee..aaceb22660be 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1519,6 +1519,7 @@ def test_post__clickhouse_reachable__returns_201_connected( # Then assert response.status_code == status.HTTP_201_CREATED assert response.json()["status"] == "connected" + assert response.json()["status_detail"] is None assert "credentials" not in response.json() mock_client.return_value.execute.assert_called_once_with("SELECT 1") @@ -1548,6 +1549,7 @@ def test_post__clickhouse_unreachable__returns_201_errored( # Then assert response.status_code == status.HTTP_201_CREATED assert response.json()["status"] == "errored" + assert response.json()["status_detail"] def test_post__clickhouse__password_stored_encrypted( diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 9cf5c2a49fa9..d7a226fe3c19 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -540,7 +540,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:785` + - `api/experimentation/services.py:804` Attributes: - `environment.id` @@ -549,7 +549,7 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:726` + - `api/experimentation/services.py:727` Attributes: - `environment.id` @@ -558,7 +558,7 @@ Attributes: ### `warehouse.connection.verification_failed` Logged at `warning` from: - - `api/experimentation/services.py:762` + - `api/experimentation/services.py:780` Attributes: - `environment.id` @@ -568,7 +568,7 @@ Attributes: ### `warehouse.connection.verification_succeeded` Logged at `info` from: - - `api/experimentation/services.py:770` + - `api/experimentation/services.py:789` Attributes: - `environment.id` @@ -577,7 +577,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:400` + - `api/experimentation/services.py:401` Attributes: - `environment.id` @@ -587,7 +587,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:386` + - `api/experimentation/services.py:387` Attributes: - `environment.id` From 393b4eb7c7d2820ca164886c1c210a24a918e598 Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 11:26:13 +0200 Subject: [PATCH 13/25] chore: update OpenAPI schema with warehouse connection fields --- openapi.yaml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/openapi.yaml b/openapi.yaml index 9c04337341d9..b27f5a3cef04 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -24889,6 +24889,11 @@ components: allOf: - $ref: '#/components/schemas/WarehouseConnectionStatusEnum' readOnly: true + status_detail: + type: + - string + - 'null' + readOnly: true name: type: string maxLength: 255 @@ -24896,6 +24901,11 @@ components: oneOf: - {} - type: 'null' + credentials: + oneOf: + - {} + - type: 'null' + writeOnly: true created_at: type: string format: date-time @@ -27807,6 +27817,11 @@ components: allOf: - $ref: '#/components/schemas/WarehouseConnectionStatusEnum' readOnly: true + status_detail: + type: + - string + - 'null' + readOnly: true name: type: string maxLength: 255 @@ -27814,6 +27829,11 @@ components: oneOf: - {} - type: 'null' + credentials: + oneOf: + - {} + - type: 'null' + writeOnly: true created_at: type: string format: date-time From 5022ad6989f5a8e5c020a4872bd90b66ac6ba415 Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 12:17:28 +0200 Subject: [PATCH 14/25] refactor: move the internal-address check from webhooks to core --- api/core/network.py | 26 +++++++++++++++++++ api/experimentation/warehouse_validation.py | 2 +- api/tests/unit/webhooks/test_unit_webhooks.py | 10 +++---- .../unit/webhooks/test_webhooks_fields.py | 6 ++--- api/webhooks/fields.py | 24 +---------------- 5 files changed, 36 insertions(+), 32 deletions(-) create mode 100644 api/core/network.py diff --git a/api/core/network.py b/api/core/network.py new file mode 100644 index 000000000000..acd056fdd268 --- /dev/null +++ b/api/core/network.py @@ -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 + + 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 + ) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index cc54b14217c6..3058ce117163 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -2,6 +2,7 @@ from rest_framework import serializers +from core.network import is_internal_address from experimentation.models import WarehouseConnection, WarehouseType from experimentation.types import ( CLICKHOUSE_DEFAULTS, @@ -10,7 +11,6 @@ ClickHouseCredentials, SnowflakeConfig, ) -from webhooks.fields import is_internal_address def validate_clickhouse_credentials( diff --git a/api/tests/unit/webhooks/test_unit_webhooks.py b/api/tests/unit/webhooks/test_unit_webhooks.py index 620d210a23e6..411e629ea4fe 100644 --- a/api/tests/unit/webhooks/test_unit_webhooks.py +++ b/api/tests/unit/webhooks/test_unit_webhooks.py @@ -373,7 +373,7 @@ def test_send_test_webhook__200_response_from_webhook__returns_correct_response( mock_response.text = "success" mock_post.return_value = mock_response mocker.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("93.184.216.34", 0))], ) @@ -421,7 +421,7 @@ def test_send_test_webhook__various_2xx_status_codes__returns_success( mock_response.text = "success" mock_post.return_value = mock_response mocker.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("93.184.216.34", 0))], ) @@ -471,7 +471,7 @@ def test_send_test_webhook__various_error_status_codes__returns_correct_response mock_response.text = external_api_error_text mock_post.return_value = mock_response mocker.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("93.184.216.34", 0))], ) @@ -540,7 +540,7 @@ def test_send_test_webhook__various_secrets__sends_correct_payload( mock_response.ok = True mock_post.return_value = mock_response mocker.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("93.184.216.34", 0))], ) @@ -583,7 +583,7 @@ def test_send_test_webhook__request_exception__returns_error_response( "Some internal exception details that should not be exposed!" ) mocker.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("93.184.216.34", 0))], ) diff --git a/api/tests/unit/webhooks/test_webhooks_fields.py b/api/tests/unit/webhooks/test_webhooks_fields.py index f2763a2c85b6..5706a8337279 100644 --- a/api/tests/unit/webhooks/test_webhooks_fields.py +++ b/api/tests/unit/webhooks/test_webhooks_fields.py @@ -58,7 +58,7 @@ def test_no_ssrf_url_field__hostname_resolving_to_private_ip__raises_validation_ ) -> None: # Given — a hostname that resolves to an RFC1918 address with mock.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("192.168.1.100", 0))], ): # When / Then @@ -73,7 +73,7 @@ def test_no_ssrf_url_field__hostname_resolving_to_private_ipv6__raises_validatio ) -> None: # Given — an AAAA-only hostname resolving to a private IPv6 address with mock.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET6, None, None, None, ("fc00::1", 0, 0, 0))], ): # When / Then @@ -108,7 +108,7 @@ def test_no_ssrf_url_field__unresolvable_hostname__returns_value( ) -> None: # Given — the hostname cannot be resolved; URL format is still valid with mock.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", side_effect=socket.gaierror, ): # When diff --git a/api/webhooks/fields.py b/api/webhooks/fields.py index d552d64238a2..13ffe8fa8b9b 100644 --- a/api/webhooks/fields.py +++ b/api/webhooks/fields.py @@ -1,30 +1,8 @@ -import ipaddress -import socket from urllib.parse import urlparse from rest_framework import serializers - -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 - ) +from core.network import is_internal_address class NoSSRFURLField(serializers.URLField): From 9d0104021d21dce41d6e6509a555ea8d295c004d Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 12:17:28 +0200 Subject: [PATCH 15/25] test: consolidate warehouse connection tests with parametrisation --- api/tests/unit/core/test_fields.py | 9 +- .../unit/experimentation/test_services.py | 69 +-- api/tests/unit/experimentation/test_views.py | 427 ++++++------------ 3 files changed, 160 insertions(+), 345 deletions(-) diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py index 4cd57836eacb..cdef06839da6 100644 --- a/api/tests/unit/core/test_fields.py +++ b/api/tests/unit/core/test_fields.py @@ -19,19 +19,12 @@ def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips() -> Non assert field.from_db_value(stored, None, None) == value -def test_get_prep_value__none__returns_none() -> None: +def test_field_methods__none__returns_none() -> None: # Given field = EncryptedJSONField() # When & Then assert field.get_prep_value(None) is None - - -def test_from_db_value__none__returns_none() -> None: - # Given - field = EncryptedJSONField() - - # When & Then assert field.from_db_value(None, None, None) is None diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 0a61da32018e..9dc849ca7e81 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -2062,14 +2062,31 @@ def test_verify_clickhouse_connection__reachable__sets_connected( } in log.events -def test_verify_clickhouse_connection__driver_error__sets_errored( +@pytest.mark.parametrize( + "credentials, execute_side_effect, expected_detail", + [ + ( + {"password": "hunter2"}, + Exception("connection refused"), + "Verification failed.", + ), + (None, None, "Stored connection details are incomplete."), + ], + ids=["driver_error", "missing_credentials"], +) +def test_verify_clickhouse_connection__failure__sets_errored_with_detail( clickhouse_connection: WarehouseConnection, + credentials: dict[str, str] | None, + execute_side_effect: Exception | None, + expected_detail: str, log: StructuredLogCapture, mocker: MockerFixture, ) -> None: # Given mock_client = mocker.patch("experimentation.services.Client") - mock_client.return_value.execute.side_effect = Exception("connection refused") + mock_client.return_value.execute.side_effect = execute_side_effect + clickhouse_connection.credentials = credentials + clickhouse_connection.save() failure_count_before = _verification_count("failure") # When @@ -2078,59 +2095,13 @@ def test_verify_clickhouse_connection__driver_error__sets_errored( # Then clickhouse_connection.refresh_from_db() assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED - assert clickhouse_connection.status_detail == "Verification failed." - mock_client.return_value.disconnect.assert_called_once_with() + assert clickhouse_connection.status_detail == expected_detail assert _verification_count("failure") == failure_count_before + 1 assert any( event["event"] == "connection.verification_failed" for event in log.events ) -def test_verify_clickhouse_connection__missing_credentials__sets_errored( - clickhouse_connection: WarehouseConnection, - mocker: MockerFixture, -) -> None: - # Given - mocker.patch("experimentation.services.Client") - clickhouse_connection.credentials = None - clickhouse_connection.save() - - # When - verify_clickhouse_connection(clickhouse_connection) - - # Then - clickhouse_connection.refresh_from_db() - assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED - assert ( - clickhouse_connection.status_detail - == "Stored connection details are incomplete." - ) - - -def test_verify_clickhouse_connection__environment_lookup_fails__sets_errored( - clickhouse_connection: WarehouseConnection, - mocker: MockerFixture, -) -> None: - # Given - mocker.patch("experimentation.services.Client") - mocker.patch.object( - Environment, - "project", - new_callable=mocker.PropertyMock, - side_effect=Exception("database unavailable"), - ) - failure_count_before = _verification_count("failure") - - # When - verify_clickhouse_connection(clickhouse_connection) - - # Then - clickhouse_connection.refresh_from_db() - assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED - assert clickhouse_connection.status_detail == "Verification failed." - assert _verification_count("failure") == failure_count_before + 1 - - @pytest.mark.parametrize( "error,expected_detail", [ diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index aaceb22660be..58ff67c3cd4a 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1,7 +1,6 @@ import socket import pytest -from django.db import connection as django_db_connection from django.urls import reverse from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture @@ -1165,7 +1164,7 @@ def test_get__clickhouse_errors__returns_200_without_stats( assert data["status"] == "pending_connection" -def test_post__clickhouse_minimal_config__applies_defaults( +def test_post__clickhouse_minimal_payload__applies_defaults_and_generates_name( admin_client: APIClient, enable_features: EnableFeaturesFixture, environment: Environment, @@ -1181,7 +1180,6 @@ def test_post__clickhouse_minimal_config__applies_defaults( warehouse_connection_url, data={ "warehouse_type": "clickhouse", - "name": "Production ClickHouse", "config": {"host": "ch.example.com"}, "credentials": {"password": "hunter2"}, }, @@ -1197,59 +1195,144 @@ def test_post__clickhouse_minimal_config__applies_defaults( "username": "default", "secure": True, } + assert response.json()["name"] == f"ClickHouse Warehouse - {environment.name}" assert "credentials" not in response.json() -def test_post__clickhouse_missing_host__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "host" in response.json()["config"] - - @pytest.mark.parametrize( - "host", - ["127.0.0.1", "10.0.0.1", "169.254.169.254"], + "data, error_path", + [ + pytest.param( + {"warehouse_type": "clickhouse", "credentials": {"password": "hunter2"}}, + ("config", "host"), + id="missing_host", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "127.0.0.1"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "host"), + id="loopback_host", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "10.0.0.1"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "host"), + id="private_host", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "169.254.169.254"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "host"), + id="link_local_host", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "port": 0}, + "credentials": {"password": "hunter2"}, + }, + ("config", "port"), + id="port_zero", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "port": 65536}, + "credentials": {"password": "hunter2"}, + }, + ("config", "port"), + id="port_above_range", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "port": "not-a-port"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "port"), + id="port_not_a_number", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "port": True}, + "credentials": {"password": "hunter2"}, + }, + ("config", "port"), + id="port_boolean", + ), + pytest.param( + {"warehouse_type": "clickhouse", "config": {"host": "ch.example.com"}}, + ("credentials", "password"), + id="missing_password", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com"}, + "credentials": "hunter2", + }, + ("credentials",), + id="non_dict_credentials", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": "not-a-dict", + "credentials": {"password": "hunter2"}, + }, + ("config",), + id="non_dict_config", + ), + pytest.param( + {"warehouse_type": "flagsmith", "credentials": {"password": "hunter2"}}, + ("credentials",), + id="flagsmith_with_credentials", + ), + pytest.param( + { + "warehouse_type": "snowflake", + "config": {"account_identifier": "xy12345.us-east-1"}, + "credentials": {"password": "hunter2"}, + }, + ("credentials",), + id="snowflake_with_credentials", + ), + pytest.param( + {"warehouse_type": "flagsmith", "credentials": {}}, + ("credentials",), + id="flagsmith_empty_credentials", + ), + ], ) -def test_post__clickhouse_internal_host__returns_400( +def test_post__invalid_payload__returns_400( admin_client: APIClient, + data: dict[str, object], enable_features: EnableFeaturesFixture, - host: str, + error_path: tuple[str, ...], warehouse_connection_url: str, ) -> None: # Given enable_features("experimentation_warehouse_connection") # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": host}, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) + response = admin_client.post(warehouse_connection_url, data=data, format="json") # Then assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "host" in response.json()["config"] + errors = response.json() + for key in error_path[:-1]: + errors = errors[key] + assert error_path[-1] in errors def test_post__clickhouse_hostname_resolves_to_internal_ip__returns_400( @@ -1261,7 +1344,7 @@ def test_post__clickhouse_hostname_resolves_to_internal_ip__returns_400( # Given enable_features("experimentation_warehouse_connection") mocker.patch( - "webhooks.fields.socket.getaddrinfo", + "core.network.socket.getaddrinfo", return_value=[(socket.AF_INET, None, None, None, ("192.168.1.100", 0))], ) @@ -1281,166 +1364,6 @@ def test_post__clickhouse_hostname_resolves_to_internal_ip__returns_400( assert "host" in response.json()["config"] -@pytest.mark.parametrize("port", [0, 65536, "not-a-port", True]) -def test_post__clickhouse_invalid_port__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - port: object, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com", "port": port}, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "port" in response.json()["config"] - - -def test_post__clickhouse_missing_password__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "password" in response.json()["credentials"] - - -@pytest.mark.parametrize( - "warehouse_type, config", - [ - ("flagsmith", None), - ("snowflake", {"account_identifier": "xy12345.us-east-1"}), - ], - ids=["flagsmith", "snowflake"], -) -def test_post__non_clickhouse_with_credentials__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - warehouse_type: str, - config: dict[str, str] | None, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": warehouse_type, - "config": config, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == { - "credentials": ["Only ClickHouse connections accept credentials."] - } - - -def test_post__clickhouse_no_name__auto_generates_name( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - environment: Environment, - mocker: MockerFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - mocker.patch("experimentation.services.Client") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com"}, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_201_CREATED - assert response.json()["name"] == f"ClickHouse Warehouse - {environment.name}" - - -def test_post__clickhouse_non_dict_credentials__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com"}, - "credentials": "hunter2", - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "credentials" in response.json() - - -def test_post__clickhouse_non_dict_config__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": "not-a-dict", - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "config" in response.json() - - def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( admin_client: APIClient, enable_features: EnableFeaturesFixture, @@ -1469,40 +1392,27 @@ def test_patch__flagsmith_to_clickhouse_without_credentials__returns_400( assert "password" in response.json()["credentials"] -def test_post__flagsmith_empty_credentials__returns_400( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "flagsmith", - "credentials": {}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == { - "credentials": ["Only ClickHouse connections accept credentials."] - } - - -def test_post__clickhouse_reachable__returns_201_connected( +@pytest.mark.parametrize( + "execute_side_effect, expected_status, expected_detail", + [ + (None, "connected", None), + (Exception("unreachable"), "errored", "Verification failed."), + ], + ids=["reachable", "unreachable"], +) +def test_post__clickhouse_verification_outcome__returns_201_with_status( admin_client: APIClient, enable_features: EnableFeaturesFixture, + execute_side_effect: Exception | None, + expected_detail: str | None, + expected_status: str, mocker: MockerFixture, warehouse_connection_url: str, ) -> None: # Given enable_features("experimentation_warehouse_connection") mock_client = mocker.patch("experimentation.services.Client") + mock_client.return_value.execute.side_effect = execute_side_effect # When response = admin_client.post( @@ -1518,71 +1428,12 @@ def test_post__clickhouse_reachable__returns_201_connected( # Then assert response.status_code == status.HTTP_201_CREATED - assert response.json()["status"] == "connected" - assert response.json()["status_detail"] is None + assert response.json()["status"] == expected_status + assert response.json()["status_detail"] == expected_detail assert "credentials" not in response.json() mock_client.return_value.execute.assert_called_once_with("SELECT 1") -def test_post__clickhouse_unreachable__returns_201_errored( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - mocker: MockerFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - mock_client = mocker.patch("experimentation.services.Client") - mock_client.return_value.execute.side_effect = Exception("unreachable") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com"}, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - assert response.status_code == status.HTTP_201_CREATED - assert response.json()["status"] == "errored" - assert response.json()["status_detail"] - - -def test_post__clickhouse__password_stored_encrypted( - admin_client: APIClient, - enable_features: EnableFeaturesFixture, - mocker: MockerFixture, - warehouse_connection_url: str, -) -> None: - # Given - enable_features("experimentation_warehouse_connection") - mocker.patch("experimentation.services.Client") - - # When - response = admin_client.post( - warehouse_connection_url, - data={ - "warehouse_type": "clickhouse", - "config": {"host": "ch.example.com"}, - "credentials": {"password": "hunter2"}, - }, - format="json", - ) - - # Then - with django_db_connection.cursor() as cursor: - cursor.execute( - "SELECT credentials FROM experimentation_warehouseconnection WHERE id = %s", - [response.json()["id"]], - ) - raw = cursor.fetchone()[0] - assert "hunter2" not in raw - - def test_get__clickhouse__credentials_not_in_response( admin_client: APIClient, clickhouse_connection: WarehouseConnection, From 621c54d005967b686d7a0553a341d239b0ef14c8 Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 16 Jul 2026 14:03:26 +0200 Subject: [PATCH 16/25] fix: harden warehouse connection validation, throttling, and verification --- api/app/settings/common.py | 2 +- api/app/settings/test.py | 2 +- api/experimentation/serializers.py | 11 +- api/experimentation/services.py | 14 ++- api/experimentation/views.py | 9 +- api/experimentation/warehouse_validation.py | 29 +++-- .../unit/experimentation/test_services.py | 33 +++++- api/tests/unit/experimentation/test_views.py | 107 +++++++++++++++++- .../observability/_events-catalogue.md | 16 +-- 9 files changed, 193 insertions(+), 30 deletions(-) diff --git a/api/app/settings/common.py b/api/app/settings/common.py index bcc51dc93c5e..21cc5128193e 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -361,7 +361,7 @@ "invite": "10/min", "user": USER_THROTTLE_RATE, "influx_query": "5/min", - "warehouse_connection_test": "10/min", + "warehouse_connection_write": "10/min", }, "DEFAULT_FILTER_BACKENDS": ["django_filters.rest_framework.DjangoFilterBackend"], "DEFAULT_RENDERER_CLASSES": [ diff --git a/api/app/settings/test.py b/api/app/settings/test.py index 854ad2d7da85..c6e4954a94be 100644 --- a/api/app/settings/test.py +++ b/api/app/settings/test.py @@ -27,7 +27,7 @@ "user": "100000/day", "master_api_key": "100000/day", "influx_query": "50/min", - "warehouse_connection_test": "100/min", + "warehouse_connection_write": "1000/min", } AWS_SSE_LOGS_BUCKET_NAME = "test_bucket" diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 3ed1bb16228e..e497eebd4c7b 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -17,6 +17,7 @@ ExperimentStatus, Metric, WarehouseConnection, + WarehouseConnectionStatus, WarehouseType, ) from experimentation.services import ( @@ -68,10 +69,14 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: "warehouse_type", getattr(self.instance, "warehouse_type", ""), ) + type_changed = ( + self.instance is not None + and getattr(self.instance, "warehouse_type", "") != warehouse_type + ) validate_credentials(attrs, warehouse_type, self.instance) # type: ignore[arg-type] - if "config" not in attrs and self.instance is not None: + if "config" not in attrs and self.instance is not None and not type_changed: return attrs config: dict[str, Any] | None = attrs.get("config") @@ -84,6 +89,10 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: {"config": "Flagsmith warehouse does not accept configuration."} ) attrs["config"] = None + + if type_changed: + attrs["status"] = WarehouseConnectionStatus.CREATED + attrs["status_detail"] = None return attrs def create( diff --git a/api/experimentation/services.py b/api/experimentation/services.py index 7b1b2cf62380..0a84a19649b6 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -18,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, @@ -93,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: @@ -793,6 +795,10 @@ 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: @@ -804,6 +810,8 @@ def _describe_verification_error(error: Exception) -> str: 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." @@ -817,6 +825,10 @@ def verify_clickhouse_connection(connection: WarehouseConnection) -> None: 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"], port=config["port"], @@ -825,7 +837,7 @@ def verify_clickhouse_connection(connection: WarehouseConnection) -> None: database=config["database"], secure=config["secure"], connect_timeout=CLICKHOUSE_CONNECT_TIMEOUT_SECONDS, - send_receive_timeout=CLICKHOUSE_QUERY_TIMEOUT_SECONDS, + send_receive_timeout=CLICKHOUSE_VERIFY_TIMEOUT_SECONDS, ) try: client.execute("SELECT 1") diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 1805a88c509d..17ef0c06bccc 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -92,8 +92,13 @@ class WarehouseConnectionViewSet( lookup_url_kwarg = "connection_id" def get_throttles(self) -> list[BaseThrottle]: - if self.action == "test_warehouse_connection": - self.throttle_scope = "warehouse_connection_test" + if self.action in ( + "create", + "update", + "partial_update", + "test_warehouse_connection", + ): + self.throttle_scope = "warehouse_connection_write" return [ScopedRateThrottle()] return super().get_throttles() diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 3058ce117163..1621da32abb3 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -1,4 +1,4 @@ -from typing import Any, Callable +from typing import Any, Callable, cast from rest_framework import serializers @@ -29,11 +29,16 @@ def validate_clickhouse_credentials( def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: if not isinstance(config, dict): raise serializers.ValidationError({"config": "Must be an object."}) - if not config.get("host"): + if unknown_keys := set(config) - set(CLICKHOUSE_DEFAULTS): + raise serializers.ValidationError( + {"config": {key: "Unknown field." for key in sorted(unknown_keys)}} + ) + merged: dict[str, Any] = {**CLICKHOUSE_DEFAULTS, **config} + if not merged["host"] or not isinstance(merged["host"], str): raise serializers.ValidationError( {"config": {"host": "This field is required."}} ) - if is_internal_address(str(config["host"])): + if is_internal_address(merged["host"]): raise serializers.ValidationError( { "config": { @@ -43,19 +48,24 @@ def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: } } ) - merged: ClickHouseConfig = { - **CLICKHOUSE_DEFAULTS, - **config, # type: ignore[typeddict-item] - } port = merged["port"] if isinstance(port, bool) or not isinstance(port, int) or not (1 <= port <= 65535): raise serializers.ValidationError( {"config": {"port": "Enter a valid port number (1-65535)."}} ) - return merged + for key in ("database", "username"): + if not merged[key] or not isinstance(merged[key], str): + raise serializers.ValidationError( + {"config": {key: "Must be a non-empty string."}} + ) + if not isinstance(merged["secure"], bool): + raise serializers.ValidationError({"config": {"secure": "Must be a boolean."}}) + return cast(ClickHouseConfig, merged) def validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: + if not isinstance(config, dict): + raise serializers.ValidationError({"config": "Must be an object."}) account_identifier = config.get("account_identifier", "") if not account_identifier: raise serializers.ValidationError( @@ -94,10 +104,9 @@ def validate_credentials( attrs["credentials"] = None return if ( - credentials is None + "credentials" not in attrs and instance is not None and instance.warehouse_type == warehouse_type ): - attrs.pop("credentials", None) return attrs["credentials"] = validator(credentials or {}) diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 9dc849ca7e81..9ee8437f86ca 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -39,6 +39,7 @@ ) from experimentation.results_query import _MetricSlot from experimentation.services import ( + InternalAddressError, _describe_verification_error, verify_clickhouse_connection, ) @@ -2049,7 +2050,7 @@ def test_verify_clickhouse_connection__reachable__sets_connected( database="acme_dwh", secure=True, connect_timeout=5, - send_receive_timeout=30, + send_receive_timeout=5, ) mock_client.return_value.execute.assert_called_once_with("SELECT 1") mock_client.return_value.disconnect.assert_called_once_with() @@ -2102,6 +2103,31 @@ def test_verify_clickhouse_connection__failure__sets_errored_with_detail( ) +def test_verify_clickhouse_connection__internal_host__sets_errored_without_connecting( + clickhouse_connection: WarehouseConnection, + mocker: MockerFixture, +) -> None: + # Given + mock_client = mocker.patch("experimentation.services.Client") + clickhouse_connection.config = { + **(clickhouse_connection.config or {}), + "host": "10.0.0.5", + } + clickhouse_connection.save() + + # When + verify_clickhouse_connection(clickhouse_connection) + + # Then + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.status == WarehouseConnectionStatus.ERRORED + assert ( + clickhouse_connection.status_detail + == "Host must not target internal or private network addresses." + ) + mock_client.assert_not_called() + + @pytest.mark.parametrize( "error,expected_detail", [ @@ -2131,6 +2157,10 @@ def test_verify_clickhouse_connection__failure__sets_errored_with_detail( clickhouse_errors.NetworkError("Connection refused"), "Could not connect to the host.", ), + ( + InternalAddressError("10.0.0.5"), + "Host must not target internal or private network addresses.", + ), ( KeyError("host"), "Stored connection details are incomplete.", @@ -2147,6 +2177,7 @@ def test_verify_clickhouse_connection__failure__sets_errored_with_detail( "socket_timeout_error", "builtin_timeout_error", "network_error", + "internal_address_error", "key_error", "generic_exception", ], diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 58ff67c3cd4a..8486c94c1bb8 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1051,10 +1051,14 @@ def test_test_warehouse_connection__non_admin__returns_403( assert response.status_code == status.HTTP_403_FORBIDDEN -def test_get_throttles__test_connection_action__returns_scoped_throttle() -> None: +@pytest.mark.parametrize( + "action", + ["create", "update", "partial_update", "test_warehouse_connection"], +) +def test_get_throttles__write_actions__returns_scoped_throttle(action: str) -> None: # Given view = WarehouseConnectionViewSet() - view.action = "test_warehouse_connection" + view.action = action # When throttles = view.get_throttles() @@ -1062,12 +1066,12 @@ def test_get_throttles__test_connection_action__returns_scoped_throttle() -> Non # Then assert len(throttles) == 1 assert isinstance(throttles[0], ScopedRateThrottle) - assert view.throttle_scope == "warehouse_connection_test" + assert view.throttle_scope == "warehouse_connection_write" @pytest.mark.parametrize( "action", - ["list", "retrieve", "create", "update", "destroy"], + ["list", "retrieve", "destroy"], ) def test_get_throttles__other_actions__returns_view_default_throttles( action: str, @@ -1312,6 +1316,38 @@ def test_post__clickhouse_minimal_payload__applies_defaults_and_generates_name( ("credentials",), id="flagsmith_empty_credentials", ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "password": "oops"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "password"), + id="unknown_config_key", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "secure": "false"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "secure"), + id="secure_not_boolean", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "ch.example.com", "database": 123}, + "credentials": {"password": "hunter2"}, + }, + ("config", "database"), + id="database_not_string", + ), + pytest.param( + {"warehouse_type": "snowflake", "config": "not-a-dict"}, + ("config",), + id="snowflake_non_dict_config", + ), ], ) def test_post__invalid_payload__returns_400( @@ -1534,7 +1570,7 @@ def test_test_warehouse_connection__clickhouse__reverifies_and_returns_status( assert clickhouse_connection.status == WarehouseConnectionStatus.CONNECTED -def test_patch__clickhouse_to_flagsmith__clears_stored_credentials( +def test_patch__clickhouse_to_flagsmith__resets_connection_state( admin_client: APIClient, clickhouse_connection: WarehouseConnection, enable_features: EnableFeaturesFixture, @@ -1542,6 +1578,9 @@ def test_patch__clickhouse_to_flagsmith__clears_stored_credentials( ) -> None: # Given enable_features("experimentation_warehouse_connection") + clickhouse_connection.status = WarehouseConnectionStatus.ERRORED + clickhouse_connection.status_detail = "Authentication failed." + clickhouse_connection.save() url = reverse( "api-v1:environments:experimentation:warehouse-connections-detail", args=[environment.api_key, clickhouse_connection.id], @@ -1558,3 +1597,61 @@ def test_patch__clickhouse_to_flagsmith__clears_stored_credentials( assert response.status_code == status.HTTP_200_OK clickhouse_connection.refresh_from_db() assert clickhouse_connection.credentials is None + assert clickhouse_connection.config is None + assert clickhouse_connection.status == WarehouseConnectionStatus.CREATED + assert clickhouse_connection.status_detail is None + + +def test_patch__flagsmith_to_clickhouse_without_config__returns_400( + admin_client: APIClient, + enable_features: EnableFeaturesFixture, + environment: Environment, + warehouse_connection: WarehouseConnection, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, warehouse_connection.id], + ) + + # When + response = admin_client.patch( + url, + data={ + "warehouse_type": "clickhouse", + "credentials": {"password": "hunter2"}, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "host" in response.json()["config"] + + +def test_patch__clickhouse_null_credentials__returns_400( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + environment: Environment, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, clickhouse_connection.id], + ) + + # When + response = admin_client.patch( + url, + data={"credentials": None}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "password" in response.json()["credentials"] + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.credentials == {"password": "hunter2"} diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index d7a226fe3c19..90a8a1af9546 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -377,7 +377,7 @@ Attributes: ### `segment_membership.compute.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:145` + - `api/segment_membership/services.py:149` Attributes: - `project.id` @@ -387,7 +387,7 @@ Attributes: ### `segment_membership.members.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:209` + - `api/segment_membership/services.py:215` Attributes: - `reason` @@ -540,7 +540,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:804` + - `api/experimentation/services.py:876` Attributes: - `environment.id` @@ -549,7 +549,7 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:727` + - `api/experimentation/services.py:789` Attributes: - `environment.id` @@ -558,7 +558,7 @@ Attributes: ### `warehouse.connection.verification_failed` Logged at `warning` from: - - `api/experimentation/services.py:780` + - `api/experimentation/services.py:852` Attributes: - `environment.id` @@ -568,7 +568,7 @@ Attributes: ### `warehouse.connection.verification_succeeded` Logged at `info` from: - - `api/experimentation/services.py:789` + - `api/experimentation/services.py:861` Attributes: - `environment.id` @@ -577,7 +577,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:401` + - `api/experimentation/services.py:411` Attributes: - `environment.id` @@ -587,7 +587,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:387` + - `api/experimentation/services.py:397` Attributes: - `environment.id` From 13b612f67139415f76b5d3ed6fcd9905b2200fac Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 09:57:47 +0200 Subject: [PATCH 17/25] fix: tighten serializer defaults, throttle stack, Snowflake allowlist, RFC 6598 --- api/core/network.py | 3 + api/experimentation/serializers.py | 4 +- api/experimentation/views.py | 2 +- api/experimentation/warehouse_validation.py | 9 +++ api/tests/unit/experimentation/test_views.py | 60 +++++++++++++++++++- 5 files changed, 73 insertions(+), 5 deletions(-) diff --git a/api/core/network.py b/api/core/network.py index acd056fdd268..677aa1e3e0e3 100644 --- a/api/core/network.py +++ b/api/core/network.py @@ -16,11 +16,14 @@ def is_internal_address(hostname: str) -> bool: except socket.gaierror: return False + _SHARED_ADDRESS_SPACE = ipaddress.ip_network("100.64.0.0/10") + return any( ip.is_loopback or ip.is_private or ip.is_link_local or ip.is_reserved or ip.is_multicast + or ip in _SHARED_ADDRESS_SPACE for ip in ips ) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index e497eebd4c7b..8dbcfd240220 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -41,9 +41,9 @@ class WarehouseConnectionSerializer(serializers.ModelSerializer): # type: ignore[type-arg] name = serializers.CharField(max_length=255, required=False) - config = serializers.JSONField(default=None, required=False, allow_null=True) + config = serializers.JSONField(required=False, allow_null=True) credentials = serializers.JSONField( - default=None, required=False, allow_null=True, write_only=True + required=False, allow_null=True, write_only=True ) total_events_received = serializers.SerializerMethodField() unique_events_count = serializers.SerializerMethodField() diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 17ef0c06bccc..db14bb8e06b7 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -99,7 +99,7 @@ def get_throttles(self) -> list[BaseThrottle]: "test_warehouse_connection", ): self.throttle_scope = "warehouse_connection_write" - return [ScopedRateThrottle()] + return [*super().get_throttles(), ScopedRateThrottle()] return super().get_throttles() def perform_create(self, serializer: BaseSerializer[WarehouseConnection]) -> None: diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 1621da32abb3..ab553122a945 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -66,11 +66,20 @@ def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: 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): + 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 not isinstance(value, str): + raise serializers.ValidationError( + {"config": {key: "Must be a string."}} + ) merged: SnowflakeConfig = { **SNOWFLAKE_DEFAULTS, **config, # type: ignore[typeddict-item] diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 8486c94c1bb8..0dd82fb80b52 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1064,8 +1064,7 @@ def test_get_throttles__write_actions__returns_scoped_throttle(action: str) -> N throttles = view.get_throttles() # Then - assert len(throttles) == 1 - assert isinstance(throttles[0], ScopedRateThrottle) + assert any(isinstance(t, ScopedRateThrottle) for t in throttles) assert view.throttle_scope == "warehouse_connection_write" @@ -1348,6 +1347,31 @@ def test_post__clickhouse_minimal_payload__applies_defaults_and_generates_name( ("config",), id="snowflake_non_dict_config", ), + pytest.param( + { + "warehouse_type": "snowflake", + "config": {"account_identifier": "xy12345", "extra": "bad"}, + }, + ("config", "extra"), + id="snowflake_unknown_key", + ), + pytest.param( + { + "warehouse_type": "snowflake", + "config": {"account_identifier": "xy12345", "warehouse": 123}, + }, + ("config", "warehouse"), + id="snowflake_non_string_value", + ), + pytest.param( + { + "warehouse_type": "clickhouse", + "config": {"host": "100.64.0.1"}, + "credentials": {"password": "hunter2"}, + }, + ("config", "host"), + id="shared_address_space_host", + ), ], ) def test_post__invalid_payload__returns_400( @@ -1542,6 +1566,38 @@ def test_patch__clickhouse_name_only__does_not_reverify( mock_client.assert_not_called() +def test_put__clickhouse_name_only__preserves_config_and_credentials( + admin_client: APIClient, + clickhouse_connection: WarehouseConnection, + enable_features: EnableFeaturesFixture, + environment: Environment, + mocker: MockerFixture, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + mocker.patch("experimentation.services.Client") + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, clickhouse_connection.id], + ) + + # When + response = admin_client.put( + url, + data={ + "warehouse_type": "clickhouse", + "name": "Renamed", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + clickhouse_connection.refresh_from_db() + assert clickhouse_connection.config is not None + assert clickhouse_connection.credentials == {"password": "hunter2"} + + def test_test_warehouse_connection__clickhouse__reverifies_and_returns_status( admin_client: APIClient, clickhouse_connection: WarehouseConnection, From 8f007c350c83c4121e99af272ead9577e2499a43 Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 10:16:08 +0200 Subject: [PATCH 18/25] fix: pre-merge polish (squash migrations, error codes, RFC 6598 scope, test copy) --- api/core/network.py | 15 ++++++++++----- ...connection_credentials_and_status_detail.py} | 7 ++++++- .../0011_warehouse_connection_status_detail.py | 17 ----------------- api/experimentation/services.py | 5 +++-- api/experimentation/views.py | 2 +- api/experimentation/warehouse_validation.py | 2 +- 6 files changed, 21 insertions(+), 27 deletions(-) rename api/experimentation/migrations/{0010_warehouse_connection_credentials.py => 0010_warehouse_connection_credentials_and_status_detail.py} (60%) delete mode 100644 api/experimentation/migrations/0011_warehouse_connection_status_detail.py diff --git a/api/core/network.py b/api/core/network.py index 677aa1e3e0e3..40baad2facba 100644 --- a/api/core/network.py +++ b/api/core/network.py @@ -1,29 +1,34 @@ import ipaddress import socket +# RFC 6598 carrier-grade NAT; not caught by ipaddress.is_private. +_SHARED_ADDRESS_SPACE = ipaddress.ip_network("100.64.0.0/10") -def is_internal_address(hostname: str) -> bool: + +def is_internal_address( + hostname: str, + *, + include_shared: bool = False, +) -> bool: """Return True if the hostname is, or resolves to, an internal network address: loopback, RFC 1918 private, link-local, reserved, or multicast. + When *include_shared* is True, also blocks RFC 6598 (100.64.0.0/10). 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 - _SHARED_ADDRESS_SPACE = ipaddress.ip_network("100.64.0.0/10") - return any( ip.is_loopback or ip.is_private or ip.is_link_local or ip.is_reserved or ip.is_multicast - or ip in _SHARED_ADDRESS_SPACE + or (include_shared and ip in _SHARED_ADDRESS_SPACE) for ip in ips ) diff --git a/api/experimentation/migrations/0010_warehouse_connection_credentials.py b/api/experimentation/migrations/0010_warehouse_connection_credentials_and_status_detail.py similarity index 60% rename from api/experimentation/migrations/0010_warehouse_connection_credentials.py rename to api/experimentation/migrations/0010_warehouse_connection_credentials_and_status_detail.py index 2de6ccf8ce95..0f117529259a 100644 --- a/api/experimentation/migrations/0010_warehouse_connection_credentials.py +++ b/api/experimentation/migrations/0010_warehouse_connection_credentials_and_status_detail.py @@ -1,4 +1,4 @@ -from django.db import migrations +from django.db import migrations, models import core.fields @@ -14,4 +14,9 @@ class Migration(migrations.Migration): name="credentials", field=core.fields.EncryptedJSONField(blank=True, null=True), ), + migrations.AddField( + model_name="warehouseconnection", + name="status_detail", + field=models.CharField(blank=True, max_length=255, null=True), + ), ] diff --git a/api/experimentation/migrations/0011_warehouse_connection_status_detail.py b/api/experimentation/migrations/0011_warehouse_connection_status_detail.py deleted file mode 100644 index 579fc28f37b0..000000000000 --- a/api/experimentation/migrations/0011_warehouse_connection_status_detail.py +++ /dev/null @@ -1,17 +0,0 @@ -# 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), - ), - ] diff --git a/api/experimentation/services.py b/api/experimentation/services.py index 0a84a19649b6..49fdbdcf7890 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -801,9 +801,10 @@ class InternalAddressError(Exception): def _describe_verification_error(error: Exception) -> str: if isinstance(error, clickhouse_errors.ServerException): + # 516 = AUTHENTICATION_FAILED (not in clickhouse_driver.errors.ErrorCodes) if error.code == 516: return "Authentication failed." - if error.code == 81: + if error.code == clickhouse_errors.ErrorCodes.UNKNOWN_DATABASE: return "Database does not exist." return "The ClickHouse server rejected the request." if isinstance(error, (clickhouse_errors.SocketTimeoutError, TimeoutError)): @@ -827,7 +828,7 @@ def verify_clickhouse_connection(connection: WarehouseConnection) -> None: 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"]): + if is_internal_address(config["host"], include_shared=True): raise InternalAddressError(config["host"]) client = Client( config["host"], diff --git a/api/experimentation/views.py b/api/experimentation/views.py index db14bb8e06b7..56e49e3dd87c 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -155,7 +155,7 @@ def test_warehouse_connection(self, request: Request, **kwargs: object) -> Respo return Response(self.get_serializer(connection).data) if connection.warehouse_type != WarehouseType.FLAGSMITH: return Response( - {"detail": "Test events are only supported for Flagsmith warehouses."}, + {"detail": "Connection testing is not supported for this warehouse type."}, status=status.HTTP_400_BAD_REQUEST, ) mark_warehouse_pending_connection(connection) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index ab553122a945..86a367775d79 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -38,7 +38,7 @@ def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: raise serializers.ValidationError( {"config": {"host": "This field is required."}} ) - if is_internal_address(merged["host"]): + if is_internal_address(merged["host"], include_shared=True): raise serializers.ValidationError( { "config": { From 7b0049a4a454edd2adfb0478665d19fa3c60b20c Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 10:30:01 +0200 Subject: [PATCH 19/25] fix: PATCH config merges onto stored values instead of static defaults --- api/experimentation/serializers.py | 9 +++++++- api/experimentation/warehouse_validation.py | 22 ++++++++++++++------ api/tests/unit/experimentation/test_views.py | 2 ++ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 8dbcfd240220..d4c60a2d2fc5 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -82,7 +82,14 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: config: dict[str, Any] | None = attrs.get("config") if config_validator := CONFIG_VALIDATORS.get(warehouse_type): - attrs["config"] = config_validator(config or {}) + stored = ( + getattr(self.instance, "config", None) + if self.instance is not None + and not type_changed + and isinstance(getattr(self.instance, "config", None), dict) + else None + ) + attrs["config"] = config_validator(config or {}, stored=stored) elif warehouse_type == WarehouseType.FLAGSMITH: if config: raise serializers.ValidationError( diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 86a367775d79..66874a9f65d7 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -26,14 +26,19 @@ def validate_clickhouse_credentials( return {"password": password} -def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: +def validate_clickhouse_config( + config: dict[str, Any], + *, + stored: dict[str, Any] | None = None, +) -> ClickHouseConfig: if not isinstance(config, dict): raise serializers.ValidationError({"config": "Must be an object."}) if unknown_keys := set(config) - set(CLICKHOUSE_DEFAULTS): raise serializers.ValidationError( {"config": {key: "Unknown field." for key in sorted(unknown_keys)}} ) - merged: dict[str, Any] = {**CLICKHOUSE_DEFAULTS, **config} + base = stored if stored is not None else dict(CLICKHOUSE_DEFAULTS) + merged: dict[str, Any] = {**base, **config} if not merged["host"] or not isinstance(merged["host"], str): raise serializers.ValidationError( {"config": {"host": "This field is required."}} @@ -63,7 +68,11 @@ def validate_clickhouse_config(config: dict[str, Any]) -> ClickHouseConfig: return cast(ClickHouseConfig, merged) -def validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: +def validate_snowflake_config( + config: dict[str, Any], + *, + stored: dict[str, Any] | None = None, +) -> SnowflakeConfig: if not isinstance(config, dict): raise serializers.ValidationError({"config": "Must be an object."}) if unknown_keys := set(config) - set(SNOWFLAKE_DEFAULTS): @@ -80,14 +89,15 @@ def validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: raise serializers.ValidationError( {"config": {key: "Must be a string."}} ) + base = stored if stored is not None else dict(SNOWFLAKE_DEFAULTS) merged: SnowflakeConfig = { - **SNOWFLAKE_DEFAULTS, - **config, # type: ignore[typeddict-item] + **base, # type: ignore[typeddict-item] + **config, } return merged -CONFIG_VALIDATORS: dict[str, Callable[[dict[str, Any]], Any]] = { +CONFIG_VALIDATORS: dict[str, Callable[..., Any]] = { WarehouseType.SNOWFLAKE: validate_snowflake_config, WarehouseType.CLICKHOUSE: validate_clickhouse_config, } diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 0dd82fb80b52..b2cd35f79524 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1539,6 +1539,8 @@ def test_patch__clickhouse_config_without_credentials__keeps_stored_password( assert response.status_code == status.HTTP_200_OK clickhouse_connection.refresh_from_db() assert clickhouse_connection.credentials == {"password": "hunter2"} + assert clickhouse_connection.config["database"] == "acme_dwh" + assert clickhouse_connection.config["port"] == 9000 assert mock_client.call_args.kwargs["password"] == "hunter2" assert mock_client.call_args.kwargs["port"] == 9000 From db5c29b000b702921d07528733586a1563b5f2e9 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 20 Jul 2026 08:30:25 +0000 Subject: [PATCH 20/25] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/experimentation/views.py | 4 +++- api/experimentation/warehouse_validation.py | 4 +--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 56e49e3dd87c..36fbdf244e93 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -155,7 +155,9 @@ def test_warehouse_connection(self, request: Request, **kwargs: object) -> Respo return Response(self.get_serializer(connection).data) if connection.warehouse_type != WarehouseType.FLAGSMITH: return Response( - {"detail": "Connection testing is not supported for this warehouse type."}, + { + "detail": "Connection testing is not supported for this warehouse type." + }, status=status.HTTP_400_BAD_REQUEST, ) mark_warehouse_pending_connection(connection) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 66874a9f65d7..7c1b9cbb7449 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -86,9 +86,7 @@ def validate_snowflake_config( ) for key, value in config.items(): if not isinstance(value, str): - raise serializers.ValidationError( - {"config": {key: "Must be a string."}} - ) + raise serializers.ValidationError({"config": {key: "Must be a string."}}) base = stored if stored is not None else dict(SNOWFLAKE_DEFAULTS) merged: SnowflakeConfig = { **base, # type: ignore[typeddict-item] From dca9632aaf4b5fef49fd7338340a7c5470dd75a3 Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 10:34:04 +0200 Subject: [PATCH 21/25] fix: narrow config type before indexing in test assertion --- api/tests/unit/experimentation/test_views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index b2cd35f79524..790cc325b594 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -1539,8 +1539,9 @@ def test_patch__clickhouse_config_without_credentials__keeps_stored_password( assert response.status_code == status.HTTP_200_OK clickhouse_connection.refresh_from_db() assert clickhouse_connection.credentials == {"password": "hunter2"} - assert clickhouse_connection.config["database"] == "acme_dwh" - assert clickhouse_connection.config["port"] == 9000 + assert clickhouse_connection.config is not None + assert clickhouse_connection.config.get("database") == "acme_dwh" + assert clickhouse_connection.config.get("port") == 9000 assert mock_client.call_args.kwargs["password"] == "hunter2" assert mock_client.call_args.kwargs["port"] == 9000 From 68d2e67d160d4fad7f7f40e1d106aeece60472fa Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 11:22:38 +0200 Subject: [PATCH 22/25] fix: Snowflake PATCH validates account_identifier after merge 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. --- api/experimentation/warehouse_validation.py | 9 +++-- api/tests/unit/experimentation/test_views.py | 37 ++++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/api/experimentation/warehouse_validation.py b/api/experimentation/warehouse_validation.py index 7c1b9cbb7449..aa01e9450bf5 100644 --- a/api/experimentation/warehouse_validation.py +++ b/api/experimentation/warehouse_validation.py @@ -79,11 +79,6 @@ def validate_snowflake_config( 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 not isinstance(value, str): raise serializers.ValidationError({"config": {key: "Must be a string."}}) @@ -92,6 +87,10 @@ def validate_snowflake_config( **base, # type: ignore[typeddict-item] **config, } + if not merged.get("account_identifier"): + raise serializers.ValidationError( + {"config": {"account_identifier": "This field is required."}} + ) return merged diff --git a/api/tests/unit/experimentation/test_views.py b/api/tests/unit/experimentation/test_views.py index 790cc325b594..cbcb9aae5c6b 100644 --- a/api/tests/unit/experimentation/test_views.py +++ b/api/tests/unit/experimentation/test_views.py @@ -636,6 +636,43 @@ def test_patch__snowflake_update_config__returns_200( assert data["config"]["warehouse"] == "BIG_WH" +def test_patch__snowflake_partial_config__preserves_stored_account_identifier( + admin_client: APIClient, + environment: Environment, + enable_features: EnableFeaturesFixture, + warehouse_connection_url: str, +) -> None: + # Given + enable_features("experimentation_warehouse_connection") + create_response = admin_client.post( + warehouse_connection_url, + data={ + "warehouse_type": "snowflake", + "name": "My Snowflake", + "config": {"account_identifier": "xy12345.us-east-1"}, + }, + format="json", + ) + connection_id = create_response.json()["id"] + url = reverse( + "api-v1:environments:experimentation:warehouse-connections-detail", + args=[environment.api_key, connection_id], + ) + + # When + response = admin_client.patch( + url, + data={"config": {"warehouse": "BIG_WH"}}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + data = response.json() + assert data["config"]["account_identifier"] == "xy12345.us-east-1" + assert data["config"]["warehouse"] == "BIG_WH" + + def test_patch__snowflake_update_name__returns_200( admin_client: APIClient, environment: Environment, From 4b54dc7edf05522f0d37181c8bb245e77767f44a Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 14:00:20 +0200 Subject: [PATCH 23/25] feat: dedicated WAREHOUSE_CREDENTIALS_SECRET for encryption 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. --- api/app/settings/common.py | 5 +++++ api/core/fields.py | 3 ++- api/tests/unit/core/test_fields.py | 4 ++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/api/app/settings/common.py b/api/app/settings/common.py index 21cc5128193e..43e6e2da71f2 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -58,6 +58,11 @@ SECRET_KEY = env("DJANGO_SECRET_KEY", default=get_random_secret_key()) +WAREHOUSE_CREDENTIALS_SECRET = env( + "WAREHOUSE_CREDENTIALS_SECRET", + default=SECRET_KEY, +) + HOSTED_SEATS_LIMIT = env.int("HOSTED_SEATS_LIMIT", default=0) MAX_PROJECTS_IN_FREE_PLAN = 1 diff --git a/api/core/fields.py b/api/core/fields.py index 637d0c282865..ef07f7caae31 100644 --- a/api/core/fields.py +++ b/api/core/fields.py @@ -12,7 +12,8 @@ def _get_fernet() -> Fernet: - digest = hashlib.sha256(settings.SECRET_KEY.encode()).digest() + secret: str = settings.WAREHOUSE_CREDENTIALS_SECRET + digest = hashlib.sha256(secret.encode()).digest() return Fernet(base64.urlsafe_b64encode(digest)) diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py index cdef06839da6..d891fa667a60 100644 --- a/api/tests/unit/core/test_fields.py +++ b/api/tests/unit/core/test_fields.py @@ -33,10 +33,10 @@ def test_from_db_value__secret_key_changed__returns_none_and_logs( log: StructuredLogCapture, ) -> None: # Given - settings.SECRET_KEY = "old-secret" + settings.WAREHOUSE_CREDENTIALS_SECRET = "old-secret" field = EncryptedJSONField() stored = field.get_prep_value({"password": "hunter2"}) - settings.SECRET_KEY = "new-secret" + settings.WAREHOUSE_CREDENTIALS_SECRET = "new-secret" # When value = field.from_db_value(stored, None, None) From 2b3c242e83f848300e6a76aa7ffc7c8ccdd5576c Mon Sep 17 00:00:00 2001 From: "flagsmith-engineering[bot]" Date: Mon, 20 Jul 2026 12:20:15 +0000 Subject: [PATCH 24/25] chore: Update documentation artefacts --- .../observability/_events-catalogue.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 90a8a1af9546..9583b11095c3 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -74,7 +74,7 @@ Attributes: ### `core.encrypted_field.decrypt_failed` Logged at `warning` from: - - `api/core/fields.py:36` + - `api/core/fields.py:37` Attributes: - `exc_info` @@ -97,12 +97,13 @@ Attributes: - `environment.id` - `exc_info` - `experiment.id` + - `feature.id` - `organisation.id` ### `experimentation.results.compute_failed` Logged at `error` from: - - `api/experimentation/tasks.py:135` + - `api/experimentation/tasks.py:136` Attributes: - `environment.id` @@ -540,7 +541,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:876` + - `api/experimentation/services.py:878` Attributes: - `environment.id` @@ -549,7 +550,7 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:789` + - `api/experimentation/services.py:790` Attributes: - `environment.id` @@ -558,7 +559,7 @@ Attributes: ### `warehouse.connection.verification_failed` Logged at `warning` from: - - `api/experimentation/services.py:852` + - `api/experimentation/services.py:854` Attributes: - `environment.id` @@ -568,7 +569,7 @@ Attributes: ### `warehouse.connection.verification_succeeded` Logged at `info` from: - - `api/experimentation/services.py:861` + - `api/experimentation/services.py:863` Attributes: - `environment.id` @@ -577,7 +578,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:411` + - `api/experimentation/services.py:412` Attributes: - `environment.id` @@ -587,7 +588,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:397` + - `api/experimentation/services.py:398` Attributes: - `environment.id` From 90f5b6da87a46fc4dd0a72cb09edffc15443dd1d Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 20 Jul 2026 15:07:05 +0200 Subject: [PATCH 25/25] chore: add WAREHOUSE_CREDENTIALS_SECRET to ECS task definitions Points at the same Secrets Manager key as DJANGO_SECRET_KEY for now. Update the ARN when a dedicated secret is provisioned. --- .../aws/production/ecs-task-definition-admin-api.json | 4 ++++ .../aws/production/ecs-task-definition-sdk-api.json | 4 ++++ .../aws/production/ecs-task-definition-task-processor.json | 4 ++++ infrastructure/aws/staging/ecs-task-definition-admin-api.json | 4 ++++ infrastructure/aws/staging/ecs-task-definition-sdk-api.json | 4 ++++ .../aws/staging/ecs-task-definition-task-processor.json | 4 ++++ 6 files changed, 24 insertions(+) diff --git a/infrastructure/aws/production/ecs-task-definition-admin-api.json b/infrastructure/aws/production/ecs-task-definition-admin-api.json index 32605f19ac9c..08df7794c8e9 100644 --- a/infrastructure/aws/production/ecs-task-definition-admin-api.json +++ b/infrastructure/aws/production/ecs-task-definition-admin-api.json @@ -247,6 +247,10 @@ "name": "DJANGO_SECRET_KEY", "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:DJANGO_SECRET_KEY::" }, + { + "name": "WAREHOUSE_CREDENTIALS_SECRET", + "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:DJANGO_SECRET_KEY::" + }, { "name": "E2E_TEST_AUTH_TOKEN", "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:E2E_TEST_AUTH_TOKEN::" diff --git a/infrastructure/aws/production/ecs-task-definition-sdk-api.json b/infrastructure/aws/production/ecs-task-definition-sdk-api.json index 7e928544ac72..5ba0e4ee7aae 100644 --- a/infrastructure/aws/production/ecs-task-definition-sdk-api.json +++ b/infrastructure/aws/production/ecs-task-definition-sdk-api.json @@ -252,6 +252,10 @@ "name": "DJANGO_SECRET_KEY", "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:DJANGO_SECRET_KEY::" }, + { + "name": "WAREHOUSE_CREDENTIALS_SECRET", + "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:DJANGO_SECRET_KEY::" + }, { "name": "E2E_TEST_AUTH_TOKEN", "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:E2E_TEST_AUTH_TOKEN::" diff --git a/infrastructure/aws/production/ecs-task-definition-task-processor.json b/infrastructure/aws/production/ecs-task-definition-task-processor.json index a476a1bf12e4..bfc4ea7dcd56 100644 --- a/infrastructure/aws/production/ecs-task-definition-task-processor.json +++ b/infrastructure/aws/production/ecs-task-definition-task-processor.json @@ -212,6 +212,10 @@ "name": "DJANGO_SECRET_KEY", "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:DJANGO_SECRET_KEY::" }, + { + "name": "WAREHOUSE_CREDENTIALS_SECRET", + "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:DJANGO_SECRET_KEY::" + }, { "name": "E2E_TEST_AUTH_TOKEN", "valueFrom": "arn:aws:secretsmanager:eu-west-2:084060095745:secret:ECS-API-LxUiIQ:E2E_TEST_AUTH_TOKEN::" diff --git a/infrastructure/aws/staging/ecs-task-definition-admin-api.json b/infrastructure/aws/staging/ecs-task-definition-admin-api.json index 7afb69722b3b..9e5085cec168 100644 --- a/infrastructure/aws/staging/ecs-task-definition-admin-api.json +++ b/infrastructure/aws/staging/ecs-task-definition-admin-api.json @@ -248,6 +248,10 @@ "name": "DJANGO_SECRET_KEY", "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:DJANGO_SECRET_KEY::" }, + { + "name": "WAREHOUSE_CREDENTIALS_SECRET", + "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:DJANGO_SECRET_KEY::" + }, { "name": "E2E_TEST_AUTH_TOKEN", "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:E2E_TEST_AUTH_TOKEN::" diff --git a/infrastructure/aws/staging/ecs-task-definition-sdk-api.json b/infrastructure/aws/staging/ecs-task-definition-sdk-api.json index d3b609fb34dc..c37116f73c92 100644 --- a/infrastructure/aws/staging/ecs-task-definition-sdk-api.json +++ b/infrastructure/aws/staging/ecs-task-definition-sdk-api.json @@ -259,6 +259,10 @@ "name": "DJANGO_SECRET_KEY", "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:DJANGO_SECRET_KEY::" }, + { + "name": "WAREHOUSE_CREDENTIALS_SECRET", + "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:DJANGO_SECRET_KEY::" + }, { "name": "E2E_TEST_AUTH_TOKEN", "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:E2E_TEST_AUTH_TOKEN::" diff --git a/infrastructure/aws/staging/ecs-task-definition-task-processor.json b/infrastructure/aws/staging/ecs-task-definition-task-processor.json index 7c1ed3f07f94..8e3acf02fb88 100644 --- a/infrastructure/aws/staging/ecs-task-definition-task-processor.json +++ b/infrastructure/aws/staging/ecs-task-definition-task-processor.json @@ -202,6 +202,10 @@ "name": "DJANGO_SECRET_KEY", "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:DJANGO_SECRET_KEY::" }, + { + "name": "WAREHOUSE_CREDENTIALS_SECRET", + "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:DJANGO_SECRET_KEY::" + }, { "name": "GITHUB_CLIENT_SECRET", "valueFrom": "arn:aws:secretsmanager:eu-west-2:302456015006:secret:ECS-API-heAdoB:GITHUB_CLIENT_SECRET::"