From 15821d6a4dda2a6f02841f4d0d68e8536b3e6b9f Mon Sep 17 00:00:00 2001 From: GatewayJ <18332154+GatewayJ@users.noreply.github.com> Date: Sun, 19 Jul 2026 01:47:27 +0800 Subject: [PATCH 1/4] fix(console): enforce JSON request contracts --- console-web/types/api.ts | 2 +- src/console/error.rs | 148 +++++++- src/console/handlers/auth.rs | 3 +- src/console/handlers/cluster.rs | 3 +- src/console/handlers/encryption.rs | 9 +- src/console/handlers/pods.rs | 3 +- src/console/handlers/pools.rs | 7 +- src/console/handlers/security_context.rs | 14 +- src/console/handlers/tenants.rs | 9 +- src/console/json.rs | 41 +++ src/console/mod.rs | 1 + src/console/models/encryption.rs | 75 +++- src/console/openapi.rs | 433 ++++++++++++++++++++++- src/console/routes/mod.rs | 160 +++++++++ 14 files changed, 868 insertions(+), 40 deletions(-) create mode 100644 src/console/json.rs diff --git a/console-web/types/api.ts b/console-web/types/api.ts index 4e48ed5..5d6ee1e 100644 --- a/console-web/types/api.ts +++ b/console-web/types/api.ts @@ -428,7 +428,7 @@ export interface EncryptionInfoResponse { export interface UpdateEncryptionRequest { enabled: boolean - backend?: string + backend?: "local" | "vault" vault?: { endpoint: string } diff --git a/src/console/error.rs b/src/console/error.rs index 49d9d45..5d4e156 100755 --- a/src/console/error.rs +++ b/src/console/error.rs @@ -14,6 +14,7 @@ use axum::{ Json, + extract::rejection::JsonRejection, http::StatusCode, response::{IntoResponse, Response}, }; @@ -37,6 +38,18 @@ pub enum Error { #[snafu(display("Bad request: {}", message))] BadRequest { message: String }, + #[snafu(display("Invalid JSON syntax: {}", message))] + JsonSyntax { message: String }, + + #[snafu(display("Invalid JSON data: {}", message))] + JsonData { message: String }, + + #[snafu(display("Unsupported JSON media type: {}", message))] + UnsupportedMediaType { message: String }, + + #[snafu(display("Request body rejected with status {}: {}", status, message))] + RequestBody { status: StatusCode, message: String }, + #[snafu(display("Conflict: {}", message))] Conflict { message: String }, @@ -65,9 +78,23 @@ pub enum Error { Json { source: serde_json::Error }, } -/// Map `kube::Error` to a console error (403 -> Forbidden, 404 -> NotFound, 409 -> Conflict). +/// Map Kubernetes API status errors to the corresponding Console error contract. pub fn map_kube_error(e: kube::Error, not_found_resource: impl Into) -> Error { match &e { + kube::Error::Api(ae) if ae.code == 401 => Error::Unauthorized { + message: if ae.message.trim().is_empty() { + "Kubernetes API authentication required".to_string() + } else { + ae.message.clone() + }, + }, + kube::Error::Api(ae) if matches!(ae.code, 400 | 422) => Error::BadRequest { + message: if ae.message.trim().is_empty() { + "Kubernetes API rejected the request".to_string() + } else { + ae.message.clone() + }, + }, kube::Error::Api(ae) if ae.code == 403 => Error::Forbidden { message: if ae.message.is_empty() { "Kubernetes API access denied".to_string() @@ -79,13 +106,36 @@ pub fn map_kube_error(e: kube::Error, not_found_resource: impl Into) -> resource: not_found_resource.into(), }, kube::Error::Api(ae) if ae.code == 409 => Error::Conflict { - message: "Resource was modified by another request, please retry".to_string(), + message: if ae.message.trim().is_empty() { + "Kubernetes API reported a resource conflict".to_string() + } else { + ae.message.clone() + }, }, _ => Error::KubeApi { source: e }, } } impl Error { + /// Convert Axum's JSON extraction failures into the Console error contract. + pub(crate) fn from_json_rejection(rejection: JsonRejection) -> Self { + match rejection { + JsonRejection::JsonSyntaxError(error) => Self::JsonSyntax { + message: error.body_text(), + }, + JsonRejection::JsonDataError(error) => Self::JsonData { + message: error.body_text(), + }, + JsonRejection::MissingJsonContentType(error) => Self::UnsupportedMediaType { + message: error.body_text(), + }, + rejection => Self::RequestBody { + status: rejection.status(), + message: rejection.body_text(), + }, + } + } + fn log_if_server_error(&self) { match self { Error::InternalServer { message } => { @@ -173,6 +223,38 @@ impl Error { Vec::new(), None, ), + Error::JsonSyntax { message } => ( + StatusCode::BAD_REQUEST, + "BadRequest".to_string(), + "InvalidJsonSyntax".to_string(), + message, + Vec::new(), + None, + ), + Error::JsonData { message } => ( + StatusCode::UNPROCESSABLE_ENTITY, + "UnprocessableEntity".to_string(), + "InvalidJsonData".to_string(), + message, + Vec::new(), + None, + ), + Error::UnsupportedMediaType { message } => ( + StatusCode::UNSUPPORTED_MEDIA_TYPE, + "UnsupportedMediaType".to_string(), + "UnsupportedJsonContentType".to_string(), + message, + Vec::new(), + None, + ), + Error::RequestBody { status, message } => ( + status, + "RequestBodyError".to_string(), + "InvalidRequestBody".to_string(), + message, + Vec::new(), + None, + ), Error::Conflict { message } => ( StatusCode::CONFLICT, "Conflict".to_string(), @@ -261,6 +343,15 @@ mod tests { use crate::console::models::common::ConsoleErrorDetails; use serde_json::json; + fn kube_api_error(code: u16, message: &str) -> kube::Error { + kube::Error::Api(kube::error::ErrorResponse { + status: "Failure".to_string(), + message: message.to_string(), + reason: String::new(), + code, + }) + } + #[test] fn bad_request_maps_to_stable_error_contract() -> std::result::Result<(), serde_json::Error> { let (status, response) = Error::BadRequest { @@ -331,4 +422,57 @@ mod tests { ); Ok(()) } + + #[test] + fn kubernetes_unprocessable_entity_maps_to_bad_request() { + let error = map_kube_error( + kube_api_error(422, "metadata.labels: Invalid value"), + "Tenant", + ); + + assert!(matches!( + error, + Error::BadRequest { message } if message == "metadata.labels: Invalid value" + )); + } + + #[test] + fn kubernetes_unauthorized_maps_to_unauthorized() { + let server_message = map_kube_error( + kube_api_error(401, "Unauthorized: bearer token has expired"), + "Tenant", + ); + assert!(matches!( + server_message, + Error::Unauthorized { message } + if message == "Unauthorized: bearer token has expired" + )); + + let fallback = map_kube_error(kube_api_error(401, ""), "Tenant"); + assert!(matches!( + fallback, + Error::Unauthorized { message } + if message == "Kubernetes API authentication required" + )); + } + + #[test] + fn kubernetes_conflict_preserves_server_message_or_uses_fallback() { + let server_message = map_kube_error( + kube_api_error(409, "tenants.rustfs.com \"logs\" already exists"), + "Tenant", + ); + assert!(matches!( + server_message, + Error::Conflict { message } + if message == "tenants.rustfs.com \"logs\" already exists" + )); + + let fallback = map_kube_error(kube_api_error(409, ""), "Tenant"); + assert!(matches!( + fallback, + Error::Conflict { message } + if message == "Kubernetes API reported a resource conflict" + )); + } } diff --git a/src/console/handlers/auth.rs b/src/console/handlers/auth.rs index a51a14d..7d3c093 100755 --- a/src/console/handlers/auth.rs +++ b/src/console/handlers/auth.rs @@ -18,6 +18,7 @@ use snafu::ResultExt; use crate::console::{ error::{self, Error, Result}, + json::ConsoleJson, models::auth::{LoginRequest, LoginResponse, SessionResponse}, state::{AppState, Claims, SESSION_TTL_SECONDS}, }; @@ -30,7 +31,7 @@ use crate::types::v1alpha1::tenant::Tenant; // -d "{\"token\": \"$TOKEN\"}" pub async fn login( State(state): State, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result { tracing::info!("Console login attempt"); diff --git a/src/console/handlers/cluster.rs b/src/console/handlers/cluster.rs index e252de7..3310d7e 100755 --- a/src/console/handlers/cluster.rs +++ b/src/console/handlers/cluster.rs @@ -14,6 +14,7 @@ use crate::console::{ error::{self, Error, Result}, + json::ConsoleJson, models::cluster::*, state::Claims, }; @@ -146,7 +147,7 @@ pub async fn list_namespaces( /// Create a namespace by name. pub async fn create_namespace( Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; let api: Api = Api::all(client); diff --git a/src/console/handlers/encryption.rs b/src/console/handlers/encryption.rs index 266654f..ba73de6 100644 --- a/src/console/handlers/encryption.rs +++ b/src/console/handlers/encryption.rs @@ -14,6 +14,7 @@ use crate::console::{ error::{self, Error, Result}, + json::ConsoleJson, models::encryption::*, state::Claims, }; @@ -110,7 +111,7 @@ pub async fn get_encryption( pub async fn update_encryption( Path((namespace, name)): Path<(String, String)>, Extension(claims): Extension, - Json(body): Json, + ConsoleJson(body): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; let api: Api = Api::namespaced(client, &namespace); @@ -121,9 +122,9 @@ pub async fn update_encryption( .map_err(|e| error::map_kube_error(e, format!("Tenant '{}'", name)))?; let encryption = if body.enabled { - let backend = match body.backend.as_deref() { - Some("vault") => KmsBackendType::Vault, - _ => KmsBackendType::Local, + let backend = match body.backend { + Some(UpdateEncryptionBackend::Vault) => KmsBackendType::Vault, + Some(UpdateEncryptionBackend::Local) | None => KmsBackendType::Local, }; let vault_endpoint = body .vault diff --git a/src/console/handlers/pods.rs b/src/console/handlers/pods.rs index 2116701..0579e8b 100755 --- a/src/console/handlers/pods.rs +++ b/src/console/handlers/pods.rs @@ -14,6 +14,7 @@ use crate::console::{ error::{self, Error, Result}, + json::ConsoleJson, models::pod::*, state::Claims, }; @@ -234,7 +235,7 @@ pub async fn delete_pod( pub async fn restart_pod( Path((namespace, tenant_name, pod_name)): Path<(String, String, String)>, Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; let api: Api = Api::namespaced(client, &namespace); diff --git a/src/console/handlers/pools.rs b/src/console/handlers/pools.rs index a413ceb..0ff4aea 100755 --- a/src/console/handlers/pools.rs +++ b/src/console/handlers/pools.rs @@ -19,6 +19,7 @@ use kube::{Api, Client, ResourceExt, api::ListParams}; use crate::console::{ error::{self, Error, Result}, + json::ConsoleJson, models::{common::ConsoleErrorDetails, pool::*}, state::Claims, }; @@ -554,7 +555,7 @@ pub async fn list_pools( pub async fn add_pool( Path((namespace, tenant_name)): Path<(String, String)>, Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; let tenant_api: Api = Api::namespaced(client, &namespace); @@ -825,7 +826,7 @@ async fn write_pool_decommission_request( pub async fn start_pool_decommission( Path((namespace, tenant_name, pool_name)): Path<(String, String, String)>, Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { write_pool_decommission_request( namespace, @@ -843,7 +844,7 @@ pub async fn start_pool_decommission( pub async fn cancel_pool_decommission( Path((namespace, tenant_name, pool_name)): Path<(String, String, String)>, Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { write_pool_decommission_request( namespace, diff --git a/src/console/handlers/security_context.rs b/src/console/handlers/security_context.rs index 258ed55..6544b12 100644 --- a/src/console/handlers/security_context.rs +++ b/src/console/handlers/security_context.rs @@ -15,7 +15,11 @@ use super::validate_tenant_for_write; use crate::console::{ error::{self, Error, Result}, - models::encryption::{PatchField, SecurityContextInfo, UpdateSecurityContextRequest}, + json::ConsoleJson, + models::encryption::{ + PatchField, SecurityContextInfo, SecurityContextUpdateResponse, + UpdateSecurityContextRequest, + }, state::Claims, }; use crate::types::v1alpha1::security_context::PodSecurityContextOverride; @@ -85,7 +89,7 @@ fn apply_validated_security_context_update( pub async fn update_security_context( Path((namespace, name)): Path<(String, String)>, Extension(claims): Extension, - Json(body): Json, + ConsoleJson(body): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; let api: Api = Api::namespaced(client, &namespace); @@ -130,12 +134,6 @@ pub async fn update_security_context( })) } -#[derive(Debug, serde::Serialize)] -pub struct SecurityContextUpdateResponse { - pub success: bool, - pub message: String, -} - async fn create_client(claims: &Claims) -> Result { let mut config = kube::Config::infer() .await diff --git a/src/console/handlers/tenants.rs b/src/console/handlers/tenants.rs index 8ee74d9..b86dc0d 100755 --- a/src/console/handlers/tenants.rs +++ b/src/console/handlers/tenants.rs @@ -15,6 +15,7 @@ use super::validate_tenant_for_write; use crate::console::{ error::{self, Error, Result}, + json::ConsoleJson, models::tenant::*, state::Claims, }; @@ -232,7 +233,7 @@ pub async fn get_tenant_details( /// Create a Tenant CR (and namespace if missing). pub async fn create_tenant( Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let tenant = tenant_from_create_request(req)?; let (name, namespace) = tenant_identity(&tenant)?; @@ -338,7 +339,7 @@ fn tenant_from_create_request(req: CreateTenantRequest) -> Result { /// Create a Tenant from its complete YAML representation. pub async fn create_tenant_from_yaml( Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let tenant = parse_tenant_yaml_for_create(&req.yaml)?; let (name, namespace) = tenant_identity(&tenant)?; @@ -379,7 +380,7 @@ pub async fn delete_tenant( pub async fn update_tenant( Path((namespace, name)): Path<(String, String)>, Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; let api: Api = Api::namespaced(client, &namespace); @@ -549,7 +550,7 @@ pub async fn get_tenant_yaml( pub async fn put_tenant_yaml( Path((namespace, name)): Path<(String, String)>, Extension(claims): Extension, - Json(req): Json, + ConsoleJson(req): ConsoleJson, ) -> Result> { let in_tenant = parse_tenant_yaml(&req.yaml)?; diff --git a/src/console/json.rs b/src/console/json.rs new file mode 100644 index 0000000..e30b6b5 --- /dev/null +++ b/src/console/json.rs @@ -0,0 +1,41 @@ +// Copyright 2025 RustFS Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use axum::{ + Json, async_trait, + extract::{FromRequest, Request}, +}; +use serde::de::DeserializeOwned; + +use crate::console::error::Error; + +/// JSON request extractor that preserves the Console API error envelope. +#[derive(Debug)] +pub struct ConsoleJson(pub T); + +#[async_trait] +impl FromRequest for ConsoleJson +where + S: Send + Sync, + T: DeserializeOwned, +{ + type Rejection = Error; + + async fn from_request(request: Request, state: &S) -> Result { + Json::::from_request(request, state) + .await + .map(|Json(value)| Self(value)) + .map_err(Error::from_json_rejection) + } +} diff --git a/src/console/mod.rs b/src/console/mod.rs index f0ee932..115896b 100755 --- a/src/console/mod.rs +++ b/src/console/mod.rs @@ -18,6 +18,7 @@ pub mod error; pub mod handlers; +pub mod json; pub mod middleware; pub mod models; #[allow(dead_code)] diff --git a/src/console/models/encryption.rs b/src/console/models/encryption.rs index e6e2684..affe5be 100644 --- a/src/console/models/encryption.rs +++ b/src/console/models/encryption.rs @@ -48,7 +48,7 @@ pub struct LocalInfo { } #[derive(Debug, Serialize, Deserialize, ToSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct LocalMasterKeySecretRefInfo { pub name: String, pub key: String, @@ -85,24 +85,31 @@ impl SecurityContextInfo { /// PUT request – update encryption configuration. #[derive(Debug, Deserialize, ToSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct UpdateEncryptionRequest { pub enabled: bool, - pub backend: Option, + pub backend: Option, pub vault: Option, pub local: Option, pub kms_secret_name: Option, pub default_key_id: Option, } +#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Eq, ToSchema)] +#[serde(rename_all = "lowercase")] +pub enum UpdateEncryptionBackend { + Local, + Vault, +} + #[derive(Debug, Deserialize, ToSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct UpdateVaultRequest { pub endpoint: String, } #[derive(Debug, Deserialize, ToSchema)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct UpdateLocalRequest { pub key_directory: Option, pub master_key_secret_ref: Option, @@ -156,6 +163,13 @@ where } } +/// SecurityContext update result. +#[derive(Debug, Serialize, ToSchema)] +pub struct SecurityContextUpdateResponse { + pub success: bool, + pub message: String, +} + /// Generic success response. #[derive(Debug, Serialize, ToSchema)] pub struct EncryptionUpdateResponse { @@ -165,7 +179,9 @@ pub struct EncryptionUpdateResponse { #[cfg(test)] mod tests { - use super::{PatchField, SecurityContextInfo, UpdateSecurityContextRequest}; + use super::{ + PatchField, SecurityContextInfo, UpdateEncryptionRequest, UpdateSecurityContextRequest, + }; use crate::types::v1alpha1::security_context::PodSecurityContextOverride; #[test] @@ -222,4 +238,51 @@ mod tests { }; assert!(error.to_string().contains("unknown field `runAsUsr`")); } + + #[test] + fn encryption_update_accepts_supported_wire_contracts() { + for value in [ + serde_json::json!({ "enabled": false }), + serde_json::json!({ + "enabled": true, + "backend": "vault", + "vault": { "endpoint": "https://vault.example.com" }, + "kmsSecretName": "vault-token", + "defaultKeyId": "tenant-key" + }), + serde_json::json!({ + "enabled": true, + "backend": "local", + "local": { + "keyDirectory": "/var/lib/rustfs/kms", + "masterKeySecretRef": { "name": "local-kms", "key": "master-key" }, + "allowInsecureDevDefaults": false + } + }), + ] { + serde_json::from_value::(value) + .expect("supported encryption request should deserialize"); + } + } + + #[test] + fn encryption_update_rejects_unknown_backends_and_fields() { + for value in [ + serde_json::json!({ "enabled": true, "backend": "valut" }), + serde_json::json!({ "enabled": true, "enabeld": true }), + serde_json::json!({ + "enabled": true, + "backend": "vault", + "vault": { "endpoint": "https://vault.example.com", "endpont": "typo" } + }), + serde_json::json!({ + "enabled": true, + "backend": "local", + "local": { "allowInsecureDevDefault": true } + }), + ] { + serde_json::from_value::(value) + .expect_err("unknown encryption values and fields must be rejected"); + } + } } diff --git a/src/console/openapi.rs b/src/console/openapi.rs index f8008b6..938e7b7 100644 --- a/src/console/openapi.rs +++ b/src/console/openapi.rs @@ -28,6 +28,12 @@ use crate::console::models::cluster::{ use crate::console::models::common::{ ConsoleActionResponse, ConsoleErrorDetails, ConsoleErrorResponse, }; +use crate::console::models::encryption::{ + EncryptionInfoResponse, EncryptionUpdateResponse, LocalInfo, LocalMasterKeySecretRefInfo, + SecurityContextInfo, SecurityContextUpdateResponse, UpdateEncryptionBackend, + UpdateEncryptionRequest, UpdateLocalRequest, UpdateSecurityContextRequest, UpdateVaultRequest, + VaultInfo, +}; use crate::console::models::event::{EventItem, EventListResponse}; use crate::console::models::pod::{ ContainerInfo, ContainerState, DeletePodResponse, LogsQuery, PodCondition, PodDetails, @@ -73,6 +79,10 @@ use crate::types::v1alpha1::status::provisioning::{ api_delete_tenant, api_get_tenant_yaml, api_put_tenant_yaml, + api_get_encryption, + api_update_encryption, + api_get_security_context, + api_update_security_context, api_list_pools, api_add_pool, api_delete_pool, @@ -126,6 +136,18 @@ use crate::types::v1alpha1::status::provisioning::{ UpdateTenantResponse, DeleteTenantResponse, TenantYAML, + EncryptionInfoResponse, + EncryptionUpdateResponse, + VaultInfo, + LocalInfo, + LocalMasterKeySecretRefInfo, + UpdateEncryptionRequest, + UpdateEncryptionBackend, + UpdateVaultRequest, + UpdateLocalRequest, + SecurityContextInfo, + UpdateSecurityContextRequest, + SecurityContextUpdateResponse, PoolDetails, PoolListResponse, AddPoolRequest, @@ -167,6 +189,8 @@ use crate::types::v1alpha1::status::provisioning::{ tags( (name = "auth", description = "Authentication"), (name = "tenants", description = "Tenant management"), + (name = "encryption", description = "Tenant encryption configuration"), + (name = "security-context", description = "Tenant pod security context"), (name = "pools", description = "Pool management"), (name = "pods", description = "Pod management"), (name = "events", description = "Event management"), @@ -182,7 +206,21 @@ use crate::types::v1alpha1::status::provisioning::{ pub struct ApiDoc; // --- Auth --- -#[utoipa::path(post, path = "/api/v1/login", request_body = LoginRequest, responses((status = 200, body = LoginResponse)), tag = "auth")] +#[utoipa::path( + post, + path = "/api/v1/login", + request_body = LoginRequest, + responses( + (status = 200, body = LoginResponse), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "auth" +)] fn api_login(_body: Json) -> Json { unimplemented!("Documentation only") } @@ -212,7 +250,23 @@ fn api_get_tenant_state_counts() -> Json { unimplemented!("Documentation only") } -#[utoipa::path(post, path = "/api/v1/tenants", request_body = CreateTenantRequest, responses((status = 200, body = TenantListItem)), tag = "tenants")] +#[utoipa::path( + post, + path = "/api/v1/tenants", + request_body = CreateTenantRequest, + responses( + (status = 200, body = TenantListItem), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "tenants" +)] fn api_create_tenant(_body: Json) -> Json { unimplemented!("Documentation only") } @@ -224,6 +278,9 @@ fn api_create_tenant(_body: Json) -> Json { responses( (status = 200, body = TenantListItem), (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), (status = 401, body = ConsoleErrorResponse), (status = 403, body = ConsoleErrorResponse), (status = 409, body = ConsoleErrorResponse), @@ -259,7 +316,25 @@ fn api_get_tenant() -> Json { unimplemented!("Documentation only") } -#[utoipa::path(put, path = "/api/v1/namespaces/{namespace}/tenants/{name}", params(("namespace" = String, Path), ("name" = String, Path)), request_body = UpdateTenantRequest, responses((status = 200, body = UpdateTenantResponse)), tag = "tenants")] +#[utoipa::path( + put, + path = "/api/v1/namespaces/{namespace}/tenants/{name}", + params(("namespace" = String, Path), ("name" = String, Path)), + request_body = UpdateTenantRequest, + responses( + (status = 200, body = UpdateTenantResponse), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "tenants" +)] fn api_update_tenant(_body: Json) -> Json { unimplemented!("Documentation only") } @@ -274,18 +349,153 @@ fn api_get_tenant_yaml() -> Json { unimplemented!("Documentation only") } -#[utoipa::path(put, path = "/api/v1/namespaces/{namespace}/tenants/{name}/yaml", params(("namespace" = String, Path), ("name" = String, Path)), request_body = TenantYAML, responses((status = 200, body = TenantYAML)), tag = "tenants")] +#[utoipa::path( + put, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/yaml", + params( + ("namespace" = String, Path), + ("name" = String, Path) + ), + request_body = TenantYAML, + responses( + (status = 200, body = TenantYAML), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "tenants" +)] fn api_put_tenant_yaml(_body: Json) -> Json { unimplemented!("Documentation only") } +// --- Encryption --- +#[utoipa::path( + get, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/encryption", + params( + ("namespace" = String, Path, description = "Tenant namespace"), + ("name" = String, Path, description = "Tenant name") + ), + responses( + (status = 200, body = EncryptionInfoResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "encryption" +)] +fn api_get_encryption() -> Json { + unimplemented!("Documentation only") +} + +#[utoipa::path( + put, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/encryption", + params( + ("namespace" = String, Path, description = "Tenant namespace"), + ("name" = String, Path, description = "Tenant name") + ), + request_body = UpdateEncryptionRequest, + responses( + (status = 200, body = EncryptionUpdateResponse), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "encryption" +)] +fn api_update_encryption(_body: Json) -> Json { + unimplemented!("Documentation only") +} + +// --- Security Context --- +#[utoipa::path( + get, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/security-context", + params( + ("namespace" = String, Path, description = "Tenant namespace"), + ("name" = String, Path, description = "Tenant name") + ), + responses( + (status = 200, body = SecurityContextInfo), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "security-context" +)] +fn api_get_security_context() -> Json { + unimplemented!("Documentation only") +} + +#[utoipa::path( + put, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/security-context", + params( + ("namespace" = String, Path, description = "Tenant namespace"), + ("name" = String, Path, description = "Tenant name") + ), + request_body = UpdateSecurityContextRequest, + responses( + (status = 200, body = SecurityContextUpdateResponse), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "security-context" +)] +fn api_update_security_context( + _body: Json, +) -> Json { + unimplemented!("Documentation only") +} + // --- Pools --- #[utoipa::path(get, path = "/api/v1/namespaces/{namespace}/tenants/{name}/pools", params(("namespace" = String, Path), ("name" = String, Path)), responses((status = 200, body = PoolListResponse)), tag = "pools")] fn api_list_pools() -> Json { unimplemented!("Documentation only") } -#[utoipa::path(post, path = "/api/v1/namespaces/{namespace}/tenants/{name}/pools", params(("namespace" = String, Path), ("name" = String, Path)), request_body = AddPoolRequest, responses((status = 200, body = AddPoolResponse)), tag = "pools")] +#[utoipa::path( + post, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/pools", + params(("namespace" = String, Path), ("name" = String, Path)), + request_body = AddPoolRequest, + responses( + (status = 200, body = AddPoolResponse), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "pools" +)] fn api_add_pool(_body: Json) -> Json { unimplemented!("Documentation only") } @@ -325,6 +535,9 @@ fn api_delete_pool() -> Json { responses( (status = 200, body = PoolDecommissionRequestResponse), (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), (status = 401, body = ConsoleErrorResponse), (status = 403, body = ConsoleErrorResponse), (status = 404, body = ConsoleErrorResponse), @@ -351,6 +564,9 @@ fn api_start_pool_decommission( responses( (status = 200, body = PoolDecommissionRequestResponse), (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), (status = 401, body = ConsoleErrorResponse), (status = 403, body = ConsoleErrorResponse), (status = 404, body = ConsoleErrorResponse), @@ -381,7 +597,28 @@ fn api_delete_pod() -> Json { unimplemented!("Documentation only") } -#[utoipa::path(post, path = "/api/v1/namespaces/{namespace}/tenants/{name}/pods/{pod}/restart", params(("namespace" = String, Path), ("name" = String, Path), ("pod" = String, Path)), request_body = RestartPodRequest, responses((status = 200, body = DeletePodResponse)), tag = "pods")] +#[utoipa::path( + post, + path = "/api/v1/namespaces/{namespace}/tenants/{name}/pods/{pod}/restart", + params( + ("namespace" = String, Path), + ("name" = String, Path), + ("pod" = String, Path) + ), + request_body = RestartPodRequest, + responses( + (status = 200, body = DeletePodResponse), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 404, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "pods" +)] fn api_restart_pod(_body: Json) -> Json { unimplemented!("Documentation only") } @@ -411,7 +648,23 @@ fn api_list_namespaces() -> Json { unimplemented!("Documentation only") } -#[utoipa::path(post, path = "/api/v1/namespaces", request_body = CreateNamespaceRequest, responses((status = 200, body = NamespaceItem)), tag = "cluster")] +#[utoipa::path( + post, + path = "/api/v1/namespaces", + request_body = CreateNamespaceRequest, + responses( + (status = 200, body = NamespaceItem), + (status = 400, body = ConsoleErrorResponse), + (status = 413, body = ConsoleErrorResponse), + (status = 415, body = ConsoleErrorResponse), + (status = 422, body = ConsoleErrorResponse), + (status = 401, body = ConsoleErrorResponse), + (status = 403, body = ConsoleErrorResponse), + (status = 409, body = ConsoleErrorResponse), + (status = 500, body = ConsoleErrorResponse) + ), + tag = "cluster" +)] fn api_create_namespace(_body: Json) -> Json { unimplemented!("Documentation only") } @@ -428,6 +681,42 @@ mod tests { use serde_json::Value; use utoipa::OpenApi; + #[test] + fn every_json_request_documents_the_console_rejection_envelope() { + let spec = serde_json::to_value(ApiDoc::openapi()).expect("OpenAPI spec serializes"); + let paths = spec + .pointer("/paths") + .and_then(Value::as_object) + .expect("OpenAPI paths exist"); + let mut request_operations = 0; + + for (path, path_item) in paths { + for method in ["post", "put", "patch"] { + let Some(operation) = path_item.get(method) else { + continue; + }; + if operation.get("requestBody").is_none() { + continue; + } + request_operations += 1; + + for status in ["400", "413", "415", "422"] { + assert_eq!( + operation + .pointer(&format!( + "/responses/{status}/content/application~1json/schema/$ref" + )) + .and_then(Value::as_str), + Some("#/components/schemas/ConsoleErrorResponse"), + "{method} {path} status {status} should use ConsoleErrorResponse" + ); + } + } + } + + assert_eq!(request_operations, 12); + } + #[test] fn delete_pool_documents_standard_error_responses() { let spec = serde_json::to_value(ApiDoc::openapi()).expect("OpenAPI spec serializes"); @@ -498,11 +787,137 @@ mod tests { .and_then(Value::as_str), Some("#/components/schemas/TenantListItem") ); + for status in ["400", "413", "415", "422"] { + assert_eq!( + operation + .pointer(&format!( + "/responses/{status}/content/application~1json/schema/$ref" + )) + .and_then(Value::as_str), + Some("#/components/schemas/ConsoleErrorResponse"), + "status {status} should use ConsoleErrorResponse" + ); + } + } + + #[test] + fn encryption_routes_and_strict_request_schema_are_documented() { + let spec = serde_json::to_value(ApiDoc::openapi()).expect("OpenAPI spec serializes"); + let operation = spec + .pointer("/paths/~1api~1v1~1namespaces~1{namespace}~1tenants~1{name}~1encryption") + .expect("encryption path should exist"); + + assert_eq!( + operation + .pointer("/get/responses/200/content/application~1json/schema/$ref") + .and_then(Value::as_str), + Some("#/components/schemas/EncryptionInfoResponse") + ); + assert!( + spec.pointer("/components/schemas/SecurityContextInfo") + .is_some(), + "nested encryption response schemas should be registered" + ); + assert_eq!( + operation + .pointer("/put/requestBody/content/application~1json/schema/$ref") + .and_then(Value::as_str), + Some("#/components/schemas/UpdateEncryptionRequest") + ); + assert_eq!( + operation + .pointer("/put/responses/200/content/application~1json/schema/$ref") + .and_then(Value::as_str), + Some("#/components/schemas/EncryptionUpdateResponse") + ); + assert_eq!( + operation.pointer("/put/tags/0").and_then(Value::as_str), + Some("encryption") + ); + for status in ["400", "413", "415", "422"] { + assert_eq!( + operation + .pointer(&format!( + "/put/responses/{status}/content/application~1json/schema/$ref" + )) + .and_then(Value::as_str), + Some("#/components/schemas/ConsoleErrorResponse"), + "status {status} should use ConsoleErrorResponse" + ); + } + + assert_eq!( + spec.pointer("/components/schemas/UpdateEncryptionBackend/enum") + .and_then(Value::as_array), + Some(&vec![ + Value::String("local".to_string()), + Value::String("vault".to_string()), + ]) + ); + for schema in [ + "UpdateEncryptionRequest", + "UpdateVaultRequest", + "UpdateLocalRequest", + "LocalMasterKeySecretRefInfo", + ] { + assert_eq!( + spec.pointer(&format!( + "/components/schemas/{schema}/additionalProperties" + )) + .and_then(Value::as_bool), + Some(false), + "{schema} should reject unknown fields" + ); + } + } + + #[test] + fn security_context_routes_are_documented() { + let spec = serde_json::to_value(ApiDoc::openapi()).expect("OpenAPI spec serializes"); + let operation = spec + .pointer("/paths/~1api~1v1~1namespaces~1{namespace}~1tenants~1{name}~1security-context") + .expect("security-context path should exist"); + + assert_eq!( + operation + .pointer("/get/responses/200/content/application~1json/schema/$ref") + .and_then(Value::as_str), + Some("#/components/schemas/SecurityContextInfo") + ); assert_eq!( operation - .pointer("/responses/400/content/application~1json/schema/$ref") + .pointer("/put/requestBody/content/application~1json/schema/$ref") .and_then(Value::as_str), - Some("#/components/schemas/ConsoleErrorResponse") + Some("#/components/schemas/UpdateSecurityContextRequest") + ); + assert_eq!( + operation + .pointer("/put/responses/200/content/application~1json/schema/$ref") + .and_then(Value::as_str), + Some("#/components/schemas/SecurityContextUpdateResponse") + ); + assert_eq!( + operation.pointer("/put/tags/0").and_then(Value::as_str), + Some("security-context") ); + assert!( + spec.pointer("/tags") + .and_then(Value::as_array) + .is_some_and(|tags| tags.iter().any(|tag| { + tag.get("name").and_then(Value::as_str) == Some("security-context") + })), + "security-context tag metadata should be registered" + ); + for status in ["400", "413", "415", "422"] { + assert_eq!( + operation + .pointer(&format!( + "/put/responses/{status}/content/application~1json/schema/$ref" + )) + .and_then(Value::as_str), + Some("#/components/schemas/ConsoleErrorResponse"), + "status {status} should use ConsoleErrorResponse" + ); + } } } diff --git a/src/console/routes/mod.rs b/src/console/routes/mod.rs index adba902..5a6f4cd 100755 --- a/src/console/routes/mod.rs +++ b/src/console/routes/mod.rs @@ -163,3 +163,163 @@ pub fn topology_routes() -> Router { get(handlers::topology::get_topology_overview), ) } + +#[cfg(test)] +mod tests { + use super::{auth_routes, cluster_routes, pod_routes, pool_routes, tenant_routes}; + use crate::console::state::{AppState, Claims}; + use axum::{ + Extension, Router, + body::{Body, to_bytes}, + http::{Method, Request, StatusCode, header}, + }; + use serde_json::Value; + use tower::ServiceExt; + + type TestResult = Result>; + + fn test_app() -> Router { + tenant_routes() + .merge(pool_routes()) + .merge(pod_routes()) + .merge(cluster_routes()) + .merge(auth_routes()) + .layer(Extension(Claims { + k8s_token: "test-token".to_string(), + exp: usize::MAX, + iat: 0, + })) + .with_state(AppState::new("test-secret".to_string())) + } + + async fn send_json_request( + method: Method, + path: &str, + content_type: Option<&str>, + body: &str, + ) -> TestResult<(StatusCode, Value)> { + let mut request = Request::builder().method(method).uri(path); + if let Some(content_type) = content_type { + request = request.header(header::CONTENT_TYPE, content_type); + } + let response = test_app() + .oneshot(request.body(Body::from(body.to_string()))?) + .await?; + let status = response.status(); + let body = to_bytes(response.into_body(), usize::MAX).await?; + Ok((status, serde_json::from_slice(&body)?)) + } + + fn assert_error_envelope(body: &Value, code: &str, reason: &str) { + assert_eq!(body.get("code").and_then(Value::as_str), Some(code)); + assert_eq!(body.get("reason").and_then(Value::as_str), Some(reason)); + assert!(body.get("message").and_then(Value::as_str).is_some()); + } + + #[tokio::test] + async fn every_json_write_route_maps_syntax_errors_to_console_envelope() -> TestResult { + let cases = [ + (Method::POST, "/login"), + (Method::POST, "/tenants"), + (Method::POST, "/tenants/yaml"), + (Method::PUT, "/namespaces/storage/tenants/tenant-a"), + (Method::PUT, "/namespaces/storage/tenants/tenant-a/yaml"), + ( + Method::PUT, + "/namespaces/storage/tenants/tenant-a/encryption", + ), + ( + Method::PUT, + "/namespaces/storage/tenants/tenant-a/security-context", + ), + (Method::POST, "/namespaces/storage/tenants/tenant-a/pools"), + ( + Method::POST, + "/namespaces/storage/tenants/tenant-a/pools/primary/decommission", + ), + ( + Method::POST, + "/namespaces/storage/tenants/tenant-a/pools/primary/decommission/cancel", + ), + ( + Method::POST, + "/namespaces/storage/tenants/tenant-a/pods/tenant-a-0/restart", + ), + (Method::POST, "/namespaces"), + ]; + + for (method, path) in cases { + let (status, body) = + send_json_request(method, path, Some("application/json"), "{").await?; + assert_eq!(status, StatusCode::BAD_REQUEST, "{path}"); + assert_error_envelope(&body, "BadRequest", "InvalidJsonSyntax"); + } + Ok(()) + } + + #[tokio::test] + async fn json_write_route_maps_unsupported_content_type_to_console_envelope() -> TestResult { + for content_type in [None, Some("text/plain")] { + let (status, body) = + send_json_request(Method::POST, "/login", content_type, r#"{"token":"test"}"#) + .await?; + + assert_eq!(status, StatusCode::UNSUPPORTED_MEDIA_TYPE); + assert_error_envelope(&body, "UnsupportedMediaType", "UnsupportedJsonContentType"); + } + Ok(()) + } + + #[tokio::test] + async fn json_write_route_maps_invalid_data_to_console_envelope() -> TestResult { + let (status, body) = send_json_request( + Method::POST, + "/login", + Some("application/json"), + r#"{"token":42}"#, + ) + .await?; + + assert_eq!(status, StatusCode::UNPROCESSABLE_ENTITY); + assert_error_envelope(&body, "UnprocessableEntity", "InvalidJsonData"); + Ok(()) + } + + #[tokio::test] + async fn oversized_json_body_maps_to_console_envelope() -> TestResult { + const DEFAULT_JSON_BODY_LIMIT: usize = 2 * 1024 * 1024; + let body = serde_json::json!({ + "token": "x".repeat(DEFAULT_JSON_BODY_LIMIT), + }) + .to_string(); + assert!(body.len() > DEFAULT_JSON_BODY_LIMIT); + + let (status, body) = + send_json_request(Method::POST, "/login", Some("application/json"), &body).await?; + + assert_eq!(status, StatusCode::PAYLOAD_TOO_LARGE); + assert_error_envelope(&body, "RequestBodyError", "InvalidRequestBody"); + Ok(()) + } + + #[tokio::test] + async fn encryption_route_maps_invalid_data_to_console_envelope() -> TestResult { + for body in [ + r#"{"enabled":"true"}"#, + r#"{"enabled":true,"backend":"valut"}"#, + r#"{"enabled":true,"enabeld":true}"#, + ] { + let (status, body) = send_json_request( + Method::PUT, + "/namespaces/storage/tenants/tenant-a/encryption", + Some("application/json"), + body, + ) + .await?; + + assert_eq!(status, StatusCode::UNPROCESSABLE_ENTITY); + assert_error_envelope(&body, "UnprocessableEntity", "InvalidJsonData"); + } + Ok(()) + } +} From 9b3e2f16d1dd0079bd0041052198683d17857557 Mon Sep 17 00:00:00 2001 From: GatewayJ <18332154+GatewayJ@users.noreply.github.com> Date: Sun, 19 Jul 2026 12:10:24 +0800 Subject: [PATCH 2/4] fix(console): bound JSON rejection messages --- src/console/error.rs | 54 ++++++++++++++++++++++++++++++++++++--- src/console/routes/mod.rs | 24 +++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/src/console/error.rs b/src/console/error.rs index 5d4e156..13232cf 100755 --- a/src/console/error.rs +++ b/src/console/error.rs @@ -22,6 +22,24 @@ use snafu::Snafu; use crate::console::models::common::{ConsoleErrorDetails, ConsoleErrorResponse}; +pub(crate) const JSON_REJECTION_MESSAGE_MAX_BYTES: usize = 1024; +const JSON_REJECTION_TRUNCATION_SUFFIX: &str = "... [truncated]"; + +fn bounded_json_rejection_message(mut message: String) -> String { + if message.len() <= JSON_REJECTION_MESSAGE_MAX_BYTES { + return message; + } + + let mut truncate_at = + JSON_REJECTION_MESSAGE_MAX_BYTES.saturating_sub(JSON_REJECTION_TRUNCATION_SUFFIX.len()); + while !message.is_char_boundary(truncate_at) { + truncate_at -= 1; + } + message.truncate(truncate_at); + message.push_str(JSON_REJECTION_TRUNCATION_SUFFIX); + message +} + /// Console HTTP API error type #[derive(Debug, Snafu)] #[snafu(visibility(pub))] @@ -121,17 +139,17 @@ impl Error { pub(crate) fn from_json_rejection(rejection: JsonRejection) -> Self { match rejection { JsonRejection::JsonSyntaxError(error) => Self::JsonSyntax { - message: error.body_text(), + message: bounded_json_rejection_message(error.body_text()), }, JsonRejection::JsonDataError(error) => Self::JsonData { - message: error.body_text(), + message: bounded_json_rejection_message(error.body_text()), }, JsonRejection::MissingJsonContentType(error) => Self::UnsupportedMediaType { - message: error.body_text(), + message: bounded_json_rejection_message(error.body_text()), }, rejection => Self::RequestBody { status: rejection.status(), - message: rejection.body_text(), + message: bounded_json_rejection_message(rejection.body_text()), }, } } @@ -352,6 +370,34 @@ mod tests { }) } + #[test] + fn json_rejection_message_preserves_text_at_the_limit() { + let message = "x".repeat(JSON_REJECTION_MESSAGE_MAX_BYTES); + + assert_eq!(bounded_json_rejection_message(message.clone()), message); + } + + #[test] + fn json_rejection_message_is_truncated_to_the_byte_limit() { + let message = "x".repeat(JSON_REJECTION_MESSAGE_MAX_BYTES + 1); + + let bounded = bounded_json_rejection_message(message); + + assert_eq!(bounded.len(), JSON_REJECTION_MESSAGE_MAX_BYTES); + assert!(bounded.ends_with(JSON_REJECTION_TRUNCATION_SUFFIX)); + } + + #[test] + fn json_rejection_message_truncates_on_a_utf8_boundary() { + let message = "界".repeat(JSON_REJECTION_MESSAGE_MAX_BYTES); + + let bounded = bounded_json_rejection_message(message); + + assert!(bounded.len() <= JSON_REJECTION_MESSAGE_MAX_BYTES); + assert!(bounded.starts_with('界')); + assert!(bounded.ends_with(JSON_REJECTION_TRUNCATION_SUFFIX)); + } + #[test] fn bad_request_maps_to_stable_error_contract() -> std::result::Result<(), serde_json::Error> { let (status, response) = Error::BadRequest { diff --git a/src/console/routes/mod.rs b/src/console/routes/mod.rs index 5a6f4cd..1ed65b1 100755 --- a/src/console/routes/mod.rs +++ b/src/console/routes/mod.rs @@ -167,6 +167,7 @@ pub fn topology_routes() -> Router { #[cfg(test)] mod tests { use super::{auth_routes, cluster_routes, pod_routes, pool_routes, tenant_routes}; + use crate::console::error::JSON_REJECTION_MESSAGE_MAX_BYTES; use crate::console::state::{AppState, Claims}; use axum::{ Extension, Router, @@ -285,6 +286,29 @@ mod tests { Ok(()) } + #[tokio::test] + async fn json_rejection_message_is_bounded_for_large_unknown_fields() -> TestResult { + let unknown_field = "x".repeat(JSON_REJECTION_MESSAGE_MAX_BYTES * 4); + let request_body = format!(r#"{{"{unknown_field}":true,"token":"test"}}"#); + let (status, body) = send_json_request( + Method::POST, + "/login", + Some("application/json"), + &request_body, + ) + .await?; + + assert_eq!(status, StatusCode::UNPROCESSABLE_ENTITY); + assert_error_envelope(&body, "UnprocessableEntity", "InvalidJsonData"); + let message = body + .get("message") + .and_then(Value::as_str) + .expect("error message should be present"); + assert!(message.len() <= JSON_REJECTION_MESSAGE_MAX_BYTES); + assert!(message.ends_with("... [truncated]")); + Ok(()) + } + #[tokio::test] async fn oversized_json_body_maps_to_console_envelope() -> TestResult { const DEFAULT_JSON_BODY_LIMIT: usize = 2 * 1024 * 1024; From 011bbefcd34fe8b2e12baee0cc076cd388c2055b Mon Sep 17 00:00:00 2001 From: GatewayJ <18332154+GatewayJ@users.noreply.github.com> Date: Sun, 19 Jul 2026 12:10:55 +0800 Subject: [PATCH 3/4] fix(console): replace encryption settings atomically --- src/console/handlers/encryption.rs | 353 +++++++++++++++++++++-------- 1 file changed, 258 insertions(+), 95 deletions(-) diff --git a/src/console/handlers/encryption.rs b/src/console/handlers/encryption.rs index ba73de6..79151de 100644 --- a/src/console/handlers/encryption.rs +++ b/src/console/handlers/encryption.rs @@ -25,7 +25,6 @@ use crate::types::v1alpha1::security_context::effective_run_as_non_root; use crate::types::v1alpha1::tenant::Tenant; use axum::{Extension, Json, extract::Path}; use k8s_openapi::api::core::v1 as corev1; -use kube::api::{Patch, PatchParams}; use kube::{Api, Client}; fn trim_to_non_empty(value: Option) -> Option { @@ -34,6 +33,97 @@ fn trim_to_non_empty(value: Option) -> Option { .filter(|value| !value.is_empty()) } +fn encryption_config_from_request(body: &UpdateEncryptionRequest) -> Result { + let encryption = if body.enabled { + let backend = match body.backend { + Some(UpdateEncryptionBackend::Vault) => KmsBackendType::Vault, + Some(UpdateEncryptionBackend::Local) | None => KmsBackendType::Local, + }; + let vault_endpoint = body + .vault + .as_ref() + .and_then(|vault| trim_to_non_empty(Some(vault.endpoint.clone()))); + let kms_secret_name = trim_to_non_empty(body.kms_secret_name.clone()); + let default_key_id = trim_to_non_empty(body.default_key_id.clone()); + + if backend == KmsBackendType::Vault { + if vault_endpoint.is_none() { + return Err(Error::BadRequest { + message: "Vault backend requires vault.endpoint to be non-empty".to_string(), + }); + } + if kms_secret_name.is_none() { + return Err(Error::BadRequest { + message: "Vault backend requires kmsSecretName".to_string(), + }); + } + } else { + let local = body.local.as_ref(); + let allow_insecure_dev_defaults = local + .and_then(|local| local.allow_insecure_dev_defaults) + .unwrap_or(false); + let master_key_ref_ok = local + .and_then(|local| local.master_key_secret_ref.as_ref()) + .is_some_and(|secret| { + !secret.name.trim().is_empty() && !secret.key.trim().is_empty() + }); + if !allow_insecure_dev_defaults && !master_key_ref_ok { + return Err(Error::BadRequest { + message: "Local backend requires local.masterKeySecretRef.name/key unless local.allowInsecureDevDefaults is true".to_string(), + }); + } + } + + let vault = if backend == KmsBackendType::Vault { + vault_endpoint.map(|endpoint| VaultKmsConfig { endpoint }) + } else { + None + }; + let local = if backend == KmsBackendType::Local { + body.local.as_ref().map(|local| LocalKmsConfig { + key_directory: trim_to_non_empty(local.key_directory.clone()), + master_key_secret_ref: local.master_key_secret_ref.as_ref().map(|secret| { + LocalKmsMasterKeySecretRef { + name: secret.name.trim().to_string(), + key: secret.key.trim().to_string(), + } + }), + allow_insecure_dev_defaults: local.allow_insecure_dev_defaults.unwrap_or(false), + }) + } else { + None + }; + let kms_secret = if backend == KmsBackendType::Vault { + kms_secret_name.map(|name| corev1::LocalObjectReference { name }) + } else { + None + }; + + EncryptionConfig { + enabled: true, + backend, + vault, + local, + kms_secret, + default_key_id, + } + } else { + EncryptionConfig { + enabled: false, + ..Default::default() + } + }; + + Ok(encryption) +} + +fn apply_encryption_update(tenant: &mut Tenant, body: &UpdateEncryptionRequest) -> Result<()> { + // Replace the complete encryption configuration in memory. The caller persists the Tenant with + // resourceVersion optimistic concurrency so fields omitted from the request are truly removed. + tenant.spec.encryption = Some(encryption_config_from_request(body)?); + Ok(()) +} + /// GET /namespaces/:namespace/tenants/:name/encryption pub async fn get_encryption( Path((namespace, name)): Path<(String, String)>, @@ -116,104 +206,38 @@ pub async fn update_encryption( let client = create_client(&claims).await?; let api: Api = Api::namespaced(client, &namespace); - let _tenant = api - .get(&name) - .await - .map_err(|e| error::map_kube_error(e, format!("Tenant '{}'", name)))?; - - let encryption = if body.enabled { - let backend = match body.backend { - Some(UpdateEncryptionBackend::Vault) => KmsBackendType::Vault, - Some(UpdateEncryptionBackend::Local) | None => KmsBackendType::Local, - }; - let vault_endpoint = body - .vault - .as_ref() - .and_then(|v| trim_to_non_empty(Some(v.endpoint.clone()))); - let kms_secret_name = trim_to_non_empty(body.kms_secret_name.clone()); - let default_key_id = trim_to_non_empty(body.default_key_id.clone()); + const MAX_RETRIES: u32 = 3; + let mut last_conflict = None; + for _ in 0..MAX_RETRIES { + let mut tenant = api + .get(&name) + .await + .map_err(|e| error::map_kube_error(e, format!("Tenant '{}'", name)))?; + apply_encryption_update(&mut tenant, &body)?; - if backend == KmsBackendType::Vault { - if vault_endpoint.is_none() { - return Err(Error::BadRequest { - message: "Vault backend requires vault.endpoint to be non-empty".to_string(), - }); - } - if kms_secret_name.is_none() { - return Err(Error::BadRequest { - message: "Vault backend requires kmsSecretName".to_string(), - }); + match api.replace(&name, &Default::default(), &tenant).await { + Ok(_) => { + return Ok(Json(EncryptionUpdateResponse { + success: true, + message: if body.enabled { + "Encryption configuration updated".to_string() + } else { + "Encryption disabled".to_string() + }, + })); } - } else { - let local = body.local.as_ref(); - let allow_insecure_dev_defaults = local - .and_then(|l| l.allow_insecure_dev_defaults) - .unwrap_or(false); - let master_key_ref_ok = local - .and_then(|l| l.master_key_secret_ref.as_ref()) - .is_some_and(|s| !s.name.trim().is_empty() && !s.key.trim().is_empty()); - if !allow_insecure_dev_defaults && !master_key_ref_ok { - return Err(Error::BadRequest { - message: "Local backend requires local.masterKeySecretRef.name/key unless local.allowInsecureDevDefaults is true".to_string(), - }); + Err(error) => { + let mapped = error::map_kube_error(error, format!("Tenant '{}'", name)); + if !matches!(&mapped, Error::Conflict { .. }) { + return Err(mapped); + } + last_conflict = Some(mapped); } } + } - let vault = if backend == KmsBackendType::Vault { - vault_endpoint.map(|endpoint| VaultKmsConfig { endpoint }) - } else { - None - }; - - let local = if backend == KmsBackendType::Local { - body.local.map(|l| LocalKmsConfig { - key_directory: trim_to_non_empty(l.key_directory), - master_key_secret_ref: l.master_key_secret_ref.map(|s| { - LocalKmsMasterKeySecretRef { - name: s.name.trim().to_string(), - key: s.key.trim().to_string(), - } - }), - allow_insecure_dev_defaults: l.allow_insecure_dev_defaults.unwrap_or(false), - }) - } else { - None - }; - - let kms_secret = if backend == KmsBackendType::Vault { - kms_secret_name.map(|s| corev1::LocalObjectReference { name: s }) - } else { - None - }; - - Some(EncryptionConfig { - enabled: true, - backend, - vault, - local, - kms_secret, - default_key_id, - }) - } else { - Some(EncryptionConfig { - enabled: false, - ..Default::default() - }) - }; - - let patch = serde_json::json!({ "spec": { "encryption": encryption } }); - - api.patch(&name, &PatchParams::default(), &Patch::Merge(&patch)) - .await - .map_err(|e| error::map_kube_error(e, format!("Tenant '{}'", name)))?; - - Ok(Json(EncryptionUpdateResponse { - success: true, - message: if body.enabled { - "Encryption configuration updated".to_string() - } else { - "Encryption disabled".to_string() - }, + Err(last_conflict.unwrap_or_else(|| Error::Conflict { + message: "Resource was modified by another request, please retry".to_string(), })) } @@ -233,7 +257,16 @@ async fn create_client(claims: &Claims) -> Result { #[cfg(test)] mod tests { - use super::trim_to_non_empty; + use super::{apply_encryption_update, trim_to_non_empty}; + use crate::console::models::encryption::{ + LocalMasterKeySecretRefInfo, UpdateEncryptionBackend, UpdateEncryptionRequest, + UpdateLocalRequest, UpdateVaultRequest, + }; + use crate::types::v1alpha1::encryption::{ + EncryptionConfig, KmsBackendType, LocalKmsConfig, LocalKmsMasterKeySecretRef, + VaultKmsConfig, + }; + use k8s_openapi::api::core::v1::LocalObjectReference; #[test] fn trim_to_non_empty_drops_blank_strings() { @@ -244,4 +277,134 @@ mod tests { Some("/data/rustfs0/.kms-keys".to_string()) ); } + + #[test] + fn disabling_encryption_removes_all_previous_backend_fields() { + let mut tenant = crate::tests::create_test_tenant(None, None); + tenant.spec.encryption = Some(EncryptionConfig { + enabled: true, + backend: KmsBackendType::Vault, + vault: Some(VaultKmsConfig { + endpoint: "https://vault.example.com".to_string(), + }), + local: Some(LocalKmsConfig { + key_directory: Some("/data/old-keys".to_string()), + master_key_secret_ref: Some(LocalKmsMasterKeySecretRef { + name: "old-local-key".to_string(), + key: "master-key".to_string(), + }), + allow_insecure_dev_defaults: false, + }), + kms_secret: Some(LocalObjectReference { + name: "old-vault-token".to_string(), + }), + default_key_id: Some("old-default-key".to_string()), + }); + let request = UpdateEncryptionRequest { + enabled: false, + backend: None, + vault: None, + local: None, + kms_secret_name: None, + default_key_id: None, + }; + + apply_encryption_update(&mut tenant, &request).expect("disable should be valid"); + + let encryption = tenant.spec.encryption.expect("disabled configuration"); + assert!(!encryption.enabled); + assert_eq!(encryption.backend, KmsBackendType::Local); + assert!(encryption.vault.is_none()); + assert!(encryption.local.is_none()); + assert!(encryption.kms_secret.is_none()); + assert!(encryption.default_key_id.is_none()); + } + + #[test] + fn switching_vault_to_local_removes_vault_fields_and_clears_omitted_values() { + let mut tenant = crate::tests::create_test_tenant(None, None); + tenant.spec.encryption = Some(EncryptionConfig { + enabled: true, + backend: KmsBackendType::Vault, + vault: Some(VaultKmsConfig { + endpoint: "https://vault.example.com".to_string(), + }), + kms_secret: Some(LocalObjectReference { + name: "vault-token".to_string(), + }), + default_key_id: Some("old-default-key".to_string()), + ..Default::default() + }); + let request = UpdateEncryptionRequest { + enabled: true, + backend: Some(UpdateEncryptionBackend::Local), + vault: None, + local: Some(UpdateLocalRequest { + key_directory: None, + master_key_secret_ref: Some(LocalMasterKeySecretRefInfo { + name: " local-master ".to_string(), + key: " master-key ".to_string(), + }), + allow_insecure_dev_defaults: Some(false), + }), + kms_secret_name: None, + default_key_id: None, + }; + + apply_encryption_update(&mut tenant, &request).expect("local update should be valid"); + + let encryption = tenant.spec.encryption.expect("local configuration"); + assert_eq!(encryption.backend, KmsBackendType::Local); + assert!(encryption.vault.is_none()); + assert!(encryption.kms_secret.is_none()); + assert!(encryption.default_key_id.is_none()); + let local = encryption.local.expect("local settings"); + assert!(local.key_directory.is_none()); + let secret = local.master_key_secret_ref.expect("master key reference"); + assert_eq!(secret.name, "local-master"); + assert_eq!(secret.key, "master-key"); + } + + #[test] + fn switching_local_to_vault_removes_local_fields() { + let mut tenant = crate::tests::create_test_tenant(None, None); + tenant.spec.encryption = Some(EncryptionConfig { + enabled: true, + backend: KmsBackendType::Local, + local: Some(LocalKmsConfig { + key_directory: Some("/data/old-keys".to_string()), + master_key_secret_ref: Some(LocalKmsMasterKeySecretRef { + name: "old-local-key".to_string(), + key: "master-key".to_string(), + }), + allow_insecure_dev_defaults: false, + }), + ..Default::default() + }); + let request = UpdateEncryptionRequest { + enabled: true, + backend: Some(UpdateEncryptionBackend::Vault), + vault: Some(UpdateVaultRequest { + endpoint: " https://vault.example.com ".to_string(), + }), + local: None, + kms_secret_name: Some(" vault-token ".to_string()), + default_key_id: Some(" tenant-key ".to_string()), + }; + + apply_encryption_update(&mut tenant, &request).expect("vault update should be valid"); + + let encryption = tenant.spec.encryption.expect("vault configuration"); + assert_eq!(encryption.backend, KmsBackendType::Vault); + assert!(encryption.local.is_none()); + assert_eq!( + encryption.vault.expect("vault settings").endpoint, + "https://vault.example.com" + ); + assert_eq!( + encryption.kms_secret.expect("vault secret").name, + "vault-token" + ); + assert_eq!(encryption.default_key_id.as_deref(), Some("tenant-key")); + } } From 62bdfc65dc65484620df4253a71a232e9eb9b9c0 Mon Sep 17 00:00:00 2001 From: GatewayJ <18332154+GatewayJ@users.noreply.github.com> Date: Sun, 19 Jul 2026 13:43:23 +0800 Subject: [PATCH 4/4] test(console): cover encryption update retries --- src/console/handlers/encryption.rs | 230 +++++++++++++++++++++++++++-- 1 file changed, 219 insertions(+), 11 deletions(-) diff --git a/src/console/handlers/encryption.rs b/src/console/handlers/encryption.rs index 79151de..18dfbc0 100644 --- a/src/console/handlers/encryption.rs +++ b/src/console/handlers/encryption.rs @@ -204,30 +204,40 @@ pub async fn update_encryption( ConsoleJson(body): ConsoleJson, ) -> Result> { let client = create_client(&claims).await?; - let api: Api = Api::namespaced(client, &namespace); + let response = update_encryption_with_client(&client, &namespace, &name, &body).await?; + Ok(Json(response)) +} + +async fn update_encryption_with_client( + client: &Client, + namespace: &str, + name: &str, + body: &UpdateEncryptionRequest, +) -> Result { + let api: Api = Api::namespaced(client.clone(), namespace); - const MAX_RETRIES: u32 = 3; + const MAX_ATTEMPTS: u32 = 3; let mut last_conflict = None; - for _ in 0..MAX_RETRIES { + for _ in 0..MAX_ATTEMPTS { let mut tenant = api - .get(&name) + .get(name) .await - .map_err(|e| error::map_kube_error(e, format!("Tenant '{}'", name)))?; - apply_encryption_update(&mut tenant, &body)?; + .map_err(|e| error::map_kube_error(e, format!("Tenant '{name}'")))?; + apply_encryption_update(&mut tenant, body)?; - match api.replace(&name, &Default::default(), &tenant).await { + match api.replace(name, &Default::default(), &tenant).await { Ok(_) => { - return Ok(Json(EncryptionUpdateResponse { + return Ok(EncryptionUpdateResponse { success: true, message: if body.enabled { "Encryption configuration updated".to_string() } else { "Encryption disabled".to_string() }, - })); + }); } Err(error) => { - let mapped = error::map_kube_error(error, format!("Tenant '{}'", name)); + let mapped = error::map_kube_error(error, format!("Tenant '{name}'")); if !matches!(&mapped, Error::Conflict { .. }) { return Err(mapped); } @@ -257,7 +267,8 @@ async fn create_client(claims: &Claims) -> Result { #[cfg(test)] mod tests { - use super::{apply_encryption_update, trim_to_non_empty}; + use super::{apply_encryption_update, trim_to_non_empty, update_encryption_with_client}; + use crate::console::error::Error; use crate::console::models::encryption::{ LocalMasterKeySecretRefInfo, UpdateEncryptionBackend, UpdateEncryptionRequest, UpdateLocalRequest, UpdateVaultRequest, @@ -266,7 +277,51 @@ mod tests { EncryptionConfig, KmsBackendType, LocalKmsConfig, LocalKmsMasterKeySecretRef, VaultKmsConfig, }; + use http::{Request, Response, StatusCode}; use k8s_openapi::api::core::v1::LocalObjectReference; + use kube::{Client, client::Body}; + use serde_json::{Value, json}; + use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }; + use tower::service_fn; + + fn tenant_response( + resource_version: &str, + image: &str, + annotation: Option<(&str, &str)>, + ) -> Value { + let mut tenant = crate::tests::create_test_tenant(None, None); + tenant.metadata.name = Some("tenant-a".to_string()); + tenant.metadata.namespace = Some("storage".to_string()); + tenant.metadata.resource_version = Some(resource_version.to_string()); + tenant.metadata.annotations = annotation.map(|(key, value)| { + std::collections::BTreeMap::from([(key.to_string(), value.to_string())]) + }); + tenant.spec.image = Some(image.to_string()); + serde_json::to_value(tenant).expect("Tenant response should serialize") + } + + fn json_response(status: StatusCode, body: Value) -> Response { + Response::builder() + .status(status) + .body(Body::from( + serde_json::to_vec(&body).expect("response body should serialize"), + )) + .expect("response should build") + } + + fn disabled_encryption_request() -> UpdateEncryptionRequest { + UpdateEncryptionRequest { + enabled: false, + backend: None, + vault: None, + local: None, + kms_secret_name: None, + default_key_id: None, + } + } #[test] fn trim_to_non_empty_drops_blank_strings() { @@ -407,4 +462,157 @@ mod tests { ); assert_eq!(encryption.default_key_id.as_deref(), Some("tenant-key")); } + + #[tokio::test] + async fn encryption_update_retries_conflict_against_latest_tenant() { + let request_count = Arc::new(AtomicUsize::new(0)); + let initial = tenant_response("1", "rustfs:initial", None); + let concurrent = tenant_response( + "2", + "rustfs:concurrent", + Some(("example.com/concurrent", "preserved")), + ); + let service = service_fn({ + let request_count = Arc::clone(&request_count); + move |request: Request| { + let request_number = request_count.fetch_add(1, Ordering::SeqCst); + let initial = initial.clone(); + let concurrent = concurrent.clone(); + async move { + let method = request.method().clone(); + let path = request.uri().path().to_string(); + let request_body = request + .into_body() + .collect_bytes() + .await + .expect("request body should be readable"); + assert_eq!( + path, + "/apis/rustfs.com/v1alpha1/namespaces/storage/tenants/tenant-a" + ); + + let response = match request_number { + 0 => { + assert_eq!(method, http::Method::GET); + json_response(StatusCode::OK, initial) + } + 1 => { + assert_eq!(method, http::Method::PUT); + let body: Value = serde_json::from_slice(&request_body) + .expect("replacement should be JSON"); + assert_eq!( + body.pointer("/metadata/resourceVersion"), + Some(&json!("1")) + ); + assert_eq!(body.pointer("/spec/image"), Some(&json!("rustfs:initial"))); + assert_eq!( + body.pointer("/spec/encryption/enabled"), + Some(&json!(false)) + ); + json_response( + StatusCode::CONFLICT, + json!({ + "status": "Failure", + "message": "tenant changed concurrently", + "reason": "Conflict", + "code": 409 + }), + ) + } + 2 => { + assert_eq!(method, http::Method::GET); + json_response(StatusCode::OK, concurrent) + } + 3 => { + assert_eq!(method, http::Method::PUT); + let body: Value = serde_json::from_slice(&request_body) + .expect("replacement should be JSON"); + assert_eq!( + body.pointer("/metadata/resourceVersion"), + Some(&json!("2")) + ); + assert_eq!( + body.pointer("/metadata/annotations/example.com~1concurrent"), + Some(&json!("preserved")) + ); + assert_eq!( + body.pointer("/spec/image"), + Some(&json!("rustfs:concurrent")) + ); + assert_eq!( + body.pointer("/spec/encryption/enabled"), + Some(&json!(false)) + ); + json_response(StatusCode::OK, body) + } + other => panic!("unexpected request number {other}"), + }; + + Ok::<_, std::convert::Infallible>(response) + } + } + }); + let client = Client::new(service, "default"); + + let response = update_encryption_with_client( + &client, + "storage", + "tenant-a", + &disabled_encryption_request(), + ) + .await + .expect("the retry should succeed"); + + assert!(response.success); + assert_eq!(request_count.load(Ordering::SeqCst), 4); + } + + #[tokio::test] + async fn encryption_update_returns_last_conflict_after_attempts_are_exhausted() { + let request_count = Arc::new(AtomicUsize::new(0)); + let service = service_fn({ + let request_count = Arc::clone(&request_count); + move |request: Request| { + let request_number = request_count.fetch_add(1, Ordering::SeqCst); + async move { + let attempt = request_number / 2 + 1; + let response = if request_number % 2 == 0 { + assert_eq!(request.method(), http::Method::GET); + json_response( + StatusCode::OK, + tenant_response(&attempt.to_string(), "rustfs:latest", None), + ) + } else { + assert_eq!(request.method(), http::Method::PUT); + json_response( + StatusCode::CONFLICT, + json!({ + "status": "Failure", + "message": format!("conflict attempt {attempt}"), + "reason": "Conflict", + "code": 409 + }), + ) + }; + Ok::<_, std::convert::Infallible>(response) + } + } + }); + let client = Client::new(service, "default"); + + let error = update_encryption_with_client( + &client, + "storage", + "tenant-a", + &disabled_encryption_request(), + ) + .await + .expect_err("three conflicts should exhaust the update attempts"); + + assert!(matches!( + error, + Error::Conflict { message } if message == "conflict attempt 3" + )); + assert_eq!(request_count.load(Ordering::SeqCst), 6); + } }