tenant create / update-config: reject unknown fields

control_plane/src/pageserver.rs

@@ -369,6 +369,7 @@ impl PageServerNode {
             evictions_low_residence_duration_metric_threshold: settings
                 .remove("evictions_low_residence_duration_metric_threshold")
                 .map(|x| x.to_string()),
+            other: Default::default(),
         };
         let request = models::TenantCreateRequest {
             new_tenant_id,
@@ -393,69 +394,80 @@ impl PageServerNode {
             })
     }
 
-    pub fn tenant_config(&self, tenant_id: TenantId, settings: HashMap<&str, &str>) -> Result<()> {
+    pub fn tenant_config(
+        &self,
+        tenant_id: TenantId,
+        mut settings: HashMap<&str, &str>,
+    ) -> anyhow::Result<()> {
         let config = {
             // Braces to make the diff easier to read
             models::TenantConfig {
                 checkpoint_distance: settings
-                    .get("checkpoint_distance")
+                    .remove("checkpoint_distance")
                     .map(|x| x.parse::<u64>())
                     .transpose()
                     .context("Failed to parse 'checkpoint_distance' as an integer")?,
-                checkpoint_timeout: settings.get("checkpoint_timeout").map(|x| x.to_string()),
+                checkpoint_timeout: settings.remove("checkpoint_timeout").map(|x| x.to_string()),
                 compaction_target_size: settings
-                    .get("compaction_target_size")
+                    .remove("compaction_target_size")
                     .map(|x| x.parse::<u64>())
                     .transpose()
                     .context("Failed to parse 'compaction_target_size' as an integer")?,
-                compaction_period: settings.get("compaction_period").map(|x| x.to_string()),
+                compaction_period: settings.remove("compaction_period").map(|x| x.to_string()),
                 compaction_threshold: settings
-                    .get("compaction_threshold")
+                    .remove("compaction_threshold")
                     .map(|x| x.parse::<usize>())
                     .transpose()
                     .context("Failed to parse 'compaction_threshold' as an integer")?,
                 gc_horizon: settings
-                    .get("gc_horizon")
+                    .remove("gc_horizon")
                     .map(|x| x.parse::<u64>())
                     .transpose()
                     .context("Failed to parse 'gc_horizon' as an integer")?,
-                gc_period: settings.get("gc_period").map(|x| x.to_string()),
+                gc_period: settings.remove("gc_period").map(|x| x.to_string()),
                 image_creation_threshold: settings
-                    .get("image_creation_threshold")
+                    .remove("image_creation_threshold")
                     .map(|x| x.parse::<usize>())
                     .transpose()
                     .context("Failed to parse 'image_creation_threshold' as non zero integer")?,
-                pitr_interval: settings.get("pitr_interval").map(|x| x.to_string()),
+                pitr_interval: settings.remove("pitr_interval").map(|x| x.to_string()),
                 walreceiver_connect_timeout: settings
-                    .get("walreceiver_connect_timeout")
+                    .remove("walreceiver_connect_timeout")
+                    .map(|x| x.to_string()),
+                lagging_wal_timeout: settings
+                    .remove("lagging_wal_timeout")
                     .map(|x| x.to_string()),
-                lagging_wal_timeout: settings.get("lagging_wal_timeout").map(|x| x.to_string()),
                 max_lsn_wal_lag: settings
-                    .get("max_lsn_wal_lag")
+                    .remove("max_lsn_wal_lag")
                     .map(|x| x.parse::<NonZeroU64>())
                     .transpose()
                     .context("Failed to parse 'max_lsn_wal_lag' as non zero integer")?,
                 trace_read_requests: settings
-                    .get("trace_read_requests")
+                    .remove("trace_read_requests")
                     .map(|x| x.parse::<bool>())
                     .transpose()
                     .context("Failed to parse 'trace_read_requests' as bool")?,
                 eviction_policy: settings
-                    .get("eviction_policy")
-                    .map(|x| serde_json::from_str(x))
+                    .remove("eviction_policy")
+                    .map(serde_json::from_str)
                     .transpose()
                     .context("Failed to parse 'eviction_policy' json")?,
                 min_resident_size_override: settings
-                    .get("min_resident_size_override")
+                    .remove("min_resident_size_override")
                     .map(|x| x.parse::<u64>())
                     .transpose()
                     .context("Failed to parse 'min_resident_size_override' as an integer")?,
                 evictions_low_residence_duration_metric_threshold: settings
-                    .get("evictions_low_residence_duration_metric_threshold")
+                    .remove("evictions_low_residence_duration_metric_threshold")
                     .map(|x| x.to_string()),
+                other: Default::default(),
             }
         };
 
+        if !settings.is_empty() {
+            bail!("Unrecognized tenant settings: {settings:?}")
+        }
+
         self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))?
             .json(&models::TenantConfigRequest { tenant_id, config })
             .send()?

libs/pageserver_api/src/models.rs

@@ -1,7 +1,9 @@
 use std::{
     collections::HashMap,
+    marker::PhantomData,
     num::{NonZeroU64, NonZeroUsize},
     time::SystemTime,
+    unreachable,
 };
 
 use byteorder::{BigEndian, ReadBytesExt};
@@ -137,7 +139,7 @@ pub struct TenantCreateRequest {
     #[serde_as(as = "Option<DisplayFromStr>")]
     pub new_tenant_id: Option<TenantId>,
     #[serde(flatten)]
-    pub config: TenantConfig,
+    pub config: TenantConfig, // as we have a flattened field, we should reject all unknown fields in it
 }
 
 impl std::ops::Deref for TenantCreateRequest {
@@ -170,6 +172,22 @@ pub struct TenantConfig {
     pub eviction_policy: Option<serde_json::Value>,
     pub min_resident_size_override: Option<u64>,
     pub evictions_low_residence_duration_metric_threshold: Option<String>,
+    #[serde(flatten)]
+    pub other: HashMap<String, RejectValue>,
+}
+
+pub struct RejectValue(PhantomData<()>);
+
+impl<'de> serde::Deserialize<'de> for RejectValue {
+    fn deserialize<D: serde::Deserializer<'de>>(_: D) -> Result<Self, D::Error> {
+        Err(serde::de::Error::custom("Unrecognized tenant settings"))
+    }
+}
+
+impl serde::Serialize for RejectValue {
+    fn serialize<S: serde::Serializer>(&self, _: S) -> Result<S::Ok, S::Error> {
+        unreachable!()
+    }
 }
 
 #[serde_as]
@@ -227,6 +245,7 @@ impl TenantConfigRequest {
             eviction_policy: None,
             min_resident_size_override: None,
             evictions_low_residence_duration_metric_threshold: None,
+            other: HashMap::new(),
         };
         TenantConfigRequest { tenant_id, config }
     }

pageserver/src/http/openapi_spec.yml

@@ -741,8 +741,11 @@ paths:
                 $ref: "#/components/schemas/Error"
     post:
       description: |
-        Create a tenant. Returns new tenant id on success.\
+        Create a tenant. Returns new tenant id on success.
+
         If no new tenant id is specified in parameters, it would be generated. It's an error to recreate the same tenant.
+
+        Invalid fields in the tenant config will cause the request to be rejected with status 400.
       requestBody:
         content:
           application/json:
@@ -790,6 +793,8 @@ paths:
     put:
       description: |
         Update tenant's config.
+
+        Invalid fields in the tenant config will cause the request to be rejected with status 400.
       requestBody:
         content:
           application/json:

test_runner/fixtures/pageserver/http.py

@@ -149,11 +149,16 @@ def tenant_list(self) -> List[Dict[Any, Any]]:
         assert isinstance(res_json, list)
         return res_json
 
-    def tenant_create(self, new_tenant_id: Optional[TenantId] = None) -> TenantId:
+    def tenant_create(
+        self, new_tenant_id: Optional[TenantId] = None, conf: Optional[Dict[str, Any]] = None
+    ) -> TenantId:
+        if conf is not None:
+            assert "new_tenant_id" not in conf.keys()
         res = self.post(
             f"http://localhost:{self.port}/v1/tenant",
             json={
                 "new_tenant_id": str(new_tenant_id) if new_tenant_id else None,
+                **(conf or {}),
             },
         )
         self.verbose_error(res)

test_runner/regress/test_tenant_conf.py

@@ -1,13 +1,17 @@
 import json
 from contextlib import closing
+from typing import Generator
 
 import psycopg2.extras
+import pytest
 from fixtures.log_helper import log
 from fixtures.neon_fixtures import (
     LocalFsStorage,
+    NeonEnv,
     NeonEnvBuilder,
     RemoteStorageKind,
 )
+from fixtures.pageserver.http import PageserverApiException
 from fixtures.pageserver.utils import assert_tenant_state, wait_for_upload
 from fixtures.types import Lsn
 from fixtures.utils import wait_until
@@ -403,3 +407,62 @@ def get_metric():
     metric = get_metric()
     assert int(metric.labels["low_threshold_secs"]) == 24 * 60 * 60, "label resets to default"
     assert int(metric.value) == 0, "value resets to default"
+
+
+@pytest.fixture
+def unknown_fields_env(neon_env_builder: NeonEnvBuilder) -> Generator[NeonEnv, None, None]:
+    env = neon_env_builder.init_start()
+    yield env
+    env.pageserver.allowed_errors.extend(
+        [
+            ".*/v1/tenant .*Error processing HTTP request: Bad request.*",
+            ".*/v1/tenant/config .*Error processing HTTP request: Bad request.*",
+        ]
+    )
+
+
+def test_unknown_fields_cli_create(unknown_fields_env: NeonEnv):
+    """
+    When specifying an invalid config field during tenant creation on the CLI, the CLI should fail with an error.
+    """
+
+    with pytest.raises(Exception, match="Unrecognized tenant settings"):
+        unknown_fields_env.neon_cli.create_tenant(conf={"unknown_field": "unknown_value"})
+
+
+def test_unknown_fields_http_create(unknown_fields_env: NeonEnv):
+    """
+    When specifying an invalid config field during tenant creation on the HTTP API, the API should fail with an error.
+    """
+
+    ps_http = unknown_fields_env.pageserver.http_client()
+
+    with pytest.raises(PageserverApiException) as excinfo:
+        ps_http.tenant_create(conf={"unknown_field": "unknown_value"})
+    assert excinfo.value.status_code == 400
+
+
+def test_unknown_fields_cli_config(unknown_fields_env: NeonEnv):
+    """
+    When specifying an invalid config field during tenant configuration on the CLI, the CLI should fail with an error.
+    """
+
+    (tenant_id, _) = unknown_fields_env.neon_cli.create_tenant()
+
+    with pytest.raises(Exception, match="Unrecognized tenant settings"):
+        unknown_fields_env.neon_cli.config_tenant(
+            tenant_id, conf={"unknown_field": "unknown_value"}
+        )
+
+
+def test_unknown_fields_http_config(unknown_fields_env: NeonEnv):
+    """
+    When specifying an invalid config field during tenant configuration on the HTTP API, the API should fail with an error.
+    """
+
+    (tenant_id, _) = unknown_fields_env.neon_cli.create_tenant()
+    ps_http = unknown_fields_env.pageserver.http_client()
+
+    with pytest.raises(PageserverApiException) as excinfo:
+        ps_http.set_tenant_config(tenant_id, {"unknown_field": "unknown_value"})
+    assert excinfo.value.status_code == 400