Browse Source

feat(cardinality-limit): Improve validation (#70217)

Add upper limit for cardinality limit value.
Add tests that ensures that the generated limit is normalized.

Closes https://github.com/getsentry/sentry/issues/69750
ArthurKnaus 9 months ago
parent
commit
5b11583d1c

+ 11 - 1
src/sentry/api/endpoints/project_details.py

@@ -264,9 +264,19 @@ class ProjectAdminSerializer(ProjectMemberSerializer):
         return validate_pii_config_update(organization, value)
         return validate_pii_config_update(organization, value)
 
 
     def validate_relayCustomMetricCardinalityLimit(self, value):
     def validate_relayCustomMetricCardinalityLimit(self, value):
-        if value is not None and value < 0:
+        if value is None:
+            return value
+
+        if value < 0:
             raise serializers.ValidationError("Cardinality limit must be a non-negative integer.")
             raise serializers.ValidationError("Cardinality limit must be a non-negative integer.")
 
 
+        # Value is stored as uint32 in relay
+        # TODO: find a way to share this constant between relay and sentry
+        if value > 4_294_967_295:
+            raise serializers.ValidationError(
+                "Cardinality limit must be smaller or equal to 4,294,967,295."
+            )
+
         return value
         return value
 
 
     def validate_builtinSymbolSources(self, value):
     def validate_builtinSymbolSources(self, value):

+ 21 - 1
tests/sentry/api/endpoints/test_project_details.py

@@ -9,6 +9,7 @@ from unittest import mock
 import orjson
 import orjson
 from django.db import router
 from django.db import router
 from django.urls import reverse
 from django.urls import reverse
+from sentry_relay.processing import normalize_cardinality_limit_config
 
 
 from sentry import audit_log
 from sentry import audit_log
 from sentry.constants import RESERVED_PROJECT_SLUGS, ObjectStatus
 from sentry.constants import RESERVED_PROJECT_SLUGS, ObjectStatus
@@ -751,7 +752,10 @@ class ProjectUpdateTest(APITestCase):
             self.proj_slug,
             self.proj_slug,
             relayCustomMetricCardinalityLimit=1000,
             relayCustomMetricCardinalityLimit=1000,
         )
         )
-        assert self.project.get_option("relay.cardinality-limiter.limits") == [
+
+        config = self.project.get_option("relay.cardinality-limiter.limits")
+
+        assert config == [
             {
             {
                 "limit": {
                 "limit": {
                     "id": "project-override-custom",
                     "id": "project-override-custom",
@@ -762,6 +766,11 @@ class ProjectUpdateTest(APITestCase):
                 }
                 }
             }
             }
         ]
         ]
+
+        limit = config[0]["limit"]
+        normalized_limit = normalize_cardinality_limit_config(limit)
+        assert normalized_limit == limit
+
         assert resp.data["relayCustomMetricCardinalityLimit"] == 1000
         assert resp.data["relayCustomMetricCardinalityLimit"] == 1000
 
 
     def test_custom_metrics_cardinality_limit_invalid_text(self):
     def test_custom_metrics_cardinality_limit_invalid_text(self):
@@ -784,6 +793,17 @@ class ProjectUpdateTest(APITestCase):
             "Cardinality limit must be a non-negative integer."
             "Cardinality limit must be a non-negative integer."
         ]
         ]
 
 
+    def test_custom_metrics_cardinality_limit_invalid_too_high(self):
+        resp = self.get_error_response(
+            self.org_slug,
+            self.proj_slug,
+            relayCustomMetricCardinalityLimit=4_294_967_296,
+        )
+        assert self.project.get_option("replay.cardinality-limiter.limts", []) == []
+        assert resp.data["relayCustomMetricCardinalityLimit"] == [
+            "Cardinality limit must be smaller or equal to 4,294,967,295."
+        ]
+
     def test_custom_metrics_cardinality_limit_accepts_none(self):
     def test_custom_metrics_cardinality_limit_accepts_none(self):
         resp = self.get_success_response(
         resp = self.get_success_response(
             self.org_slug,
             self.org_slug,