diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index f022be3910..6309494b71 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -393,69 +393,79 @@ 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::()) .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::()) .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::()) .transpose() .context("Failed to parse 'compaction_threshold' as an integer")?, gc_horizon: settings - .get("gc_horizon") + .remove("gc_horizon") .map(|x| x.parse::()) .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::()) .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::()) .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::()) .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::()) .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()), } }; + 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()? diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 0bcdb3c3a8..3bfedd14ea 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -131,13 +131,14 @@ pub struct TimelineCreateRequest { } #[serde_as] -#[derive(Serialize, Deserialize, Default)] +#[derive(Serialize, Deserialize, Debug, Default)] +#[serde(deny_unknown_fields)] pub struct TenantCreateRequest { #[serde(default)] #[serde_as(as = "Option")] pub new_tenant_id: Option, #[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 { @@ -148,7 +149,7 @@ impl std::ops::Deref for TenantCreateRequest { } } -#[derive(Serialize, Deserialize, Default)] +#[derive(Serialize, Deserialize, Debug, Default)] pub struct TenantConfig { pub checkpoint_distance: Option, pub checkpoint_timeout: Option, @@ -192,12 +193,13 @@ impl TenantCreateRequest { } #[serde_as] -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] +#[serde(deny_unknown_fields)] pub struct TenantConfigRequest { #[serde_as(as = "DisplayFromStr")] pub tenant_id: 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 TenantConfigRequest { @@ -768,4 +770,31 @@ mod tests { assert!(format!("{:?}", &original_broken.state).contains("reason")); assert!(format!("{:?}", &original_broken.state).contains("backtrace info")); } + + #[test] + fn test_reject_unknown_field() { + let id = TenantId::generate(); + let create_request = json!({ + "new_tenant_id": id.to_string(), + "unknown_field": "unknown_value".to_string(), + }); + let err = serde_json::from_value::(create_request).unwrap_err(); + assert!( + err.to_string().contains("unknown field `unknown_field`"), + "expect unknown field `unknown_field` error, got: {}", + err + ); + + let id = TenantId::generate(); + let config_request = json!({ + "tenant_id": id.to_string(), + "unknown_field": "unknown_value".to_string(), + }); + let err = serde_json::from_value::(config_request).unwrap_err(); + assert!( + err.to_string().contains("unknown field `unknown_field`"), + "expect unknown field `unknown_field` error, got: {}", + err + ); + } } diff --git a/pageserver/src/http/openapi_spec.yml b/pageserver/src/http/openapi_spec.yml index 62664733ea..0d09603650 100644 --- a/pageserver/src/http/openapi_spec.yml +++ b/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: diff --git a/test_runner/fixtures/pageserver/http.py b/test_runner/fixtures/pageserver/http.py index 1ff057fae2..1349923cc4 100644 --- a/test_runner/fixtures/pageserver/http.py +++ b/test_runner/fixtures/pageserver/http.py @@ -149,11 +149,16 @@ class PageserverHttpClient(requests.Session): 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)