Fix index update endpoint panic when index_uri is the bucket root (#5711)

* Add failing unit test

* Fix index update endpoint panic when index_uri is the bucket root

* Simplify update endpoint to use same default URI as create

Author: rdettai

docs/reference/rest-api.md

@@ -348,7 +348,7 @@ Updating the doc mapping doesn't reindex existing data. Queries and results are
 |---------------------|--------------------|-----------------------------------------------------------------------------------------------------------------------|---------------------------------------|
 | `version`           | `String`           | Config format version, use the same as your Quickwit version.                                                         | _required_                            |
 | `index_id`          | `String`           | Index ID, must be the same index as in the request URI.                                                               | _required_                            |
-| `index_uri`         | `String`           | Defines where the index files are stored. Cannot be updated.                                                          | `{current_index_uri}`                 |
+| `index_uri`         | `String`           | Defines where the index files are stored. Cannot be updated.                                                          | `{default_index_root_uri}/{index_id}`                 |
 | `doc_mapping`       | `DocMapping`       | Doc mapping object as specified in the [index config docs](../configuration/index-config.md#doc-mapping).             | _required_                            |
 | `indexing_settings` | `IndexingSettings` | Indexing settings object as specified in the [index config docs](../configuration/index-config.md#indexing-settings). |                                       |
 | `search_settings`   | `SearchSettings`   | Search settings object as specified in the [index config docs](../configuration/index-config.md#search-settings).     |                                       |

quickwit/quickwit-config/src/index_config/serialize.rs

@@ -68,21 +68,17 @@ pub fn load_index_config_from_user_config(
 ///
 /// Ensures that the new configuration is valid in itself and compared to the
 /// current index config. If the new configuration omits some fields, the
-/// default values will be used, not those of the current index config. The only
-/// exception is the index_uri because it cannot be updated.
+/// default values will be used, not those of the current index config.
 pub fn load_index_config_update(
     config_format: ConfigFormat,
     index_config_bytes: &[u8],
+    default_index_root_uri: &Uri,
     current_index_config: &IndexConfig,
 ) -> anyhow::Result<IndexConfig> {
-    let current_index_parent_dir = &current_index_config
-        .index_uri
-        .parent()
-        .expect("index URI should have a parent");
     let mut new_index_config = load_index_config_from_user_config(
         config_format,
         index_config_bytes,
-        current_index_parent_dir,
+        default_index_root_uri,
     )?;
     ensure!(
         current_index_config.index_id == new_index_config.index_id,
@@ -355,10 +351,11 @@ mod test {
             index_id: hdfs-logs
             doc_mapping: {}
         "#;
+        let default_root = Uri::for_test("s3://mybucket");
         let original_config: IndexConfig = load_index_config_from_user_config(
             ConfigFormat::Yaml,
             original_config_yaml.as_bytes(),
-            &Uri::for_test("s3://mybucket"),
+            &default_root,
         )
         .unwrap();
         {
@@ -371,6 +368,7 @@ mod test {
             let updated_config = load_index_config_update(
                 ConfigFormat::Yaml,
                 updated_config_yaml.as_bytes(),
+                &default_root,
                 &original_config,
             )
             .unwrap();
@@ -387,6 +385,7 @@ mod test {
             let updated_config = load_index_config_update(
                 ConfigFormat::Yaml,
                 updated_config_yaml.as_bytes(),
+                &default_root,
                 &original_config,
             )
             .unwrap();
@@ -403,6 +402,7 @@ mod test {
             let load_error = load_index_config_update(
                 ConfigFormat::Yaml,
                 updated_config_yaml.as_bytes(),
+                &default_root,
                 &original_config,
             )
             .unwrap_err();
@@ -432,10 +432,11 @@ mod test {
                 period: 90 days
                 schedule: daily
         "#;
+        let default_root = Uri::for_test("s3://mybucket");
         let original_config: IndexConfig = load_index_config_from_user_config(
             ConfigFormat::Yaml,
             original_config_yaml.as_bytes(),
-            &Uri::for_test("s3://mybucket"),
+            &default_root,
         )
         .unwrap();
 
@@ -452,6 +453,7 @@ mod test {
         let updated_config = load_index_config_update(
             ConfigFormat::Yaml,
             updated_config_yaml.as_bytes(),
+            &default_root,
             &original_config,
         )
         .unwrap();
@@ -473,10 +475,11 @@ mod test {
             index_id: hdfs-logs
             doc_mapping: {}
         "#;
+        let default_root = Uri::for_test("s3://mybucket");
         let original_config: IndexConfig = load_index_config_from_user_config(
             ConfigFormat::Yaml,
             original_config_yaml.as_bytes(),
-            &Uri::for_test("s3://mybucket"),
+            &default_root,
         )
         .unwrap();
 
@@ -493,6 +496,7 @@ mod test {
         let updated_config = load_index_config_update(
             ConfigFormat::Yaml,
             updated_config_yaml.as_bytes(),
+            &default_root,
             &original_config,
         )
         .unwrap();
@@ -513,10 +517,11 @@ mod test {
                       type: datetime
                       fast: true
         "#;
+        let default_root = Uri::for_test("s3://mybucket");
         let original_config: IndexConfig = load_index_config_from_user_config(
             ConfigFormat::Yaml,
             original_config_yaml.as_bytes(),
-            &Uri::for_test("s3://mybucket"),
+            &default_root,
         )
         .unwrap();
 
@@ -539,6 +544,7 @@ mod test {
         load_index_config_update(
             ConfigFormat::Yaml,
             updated_config_yaml.as_bytes(),
+            &default_root,
             &original_config,
         )
         .expect_err("mapping changed but uid fixed should error");
@@ -556,6 +562,7 @@ mod test {
         load_index_config_update(
             ConfigFormat::Yaml,
             updated_config_yaml.as_bytes(),
+            &default_root,
             &original_config,
         )
         .expect_err("timestamp field removed should error");
@@ -575,6 +582,7 @@ mod test {
         load_index_config_update(
             ConfigFormat::Yaml,
             updated_config_yaml.as_bytes(),
+            &default_root,
             &original_config,
         )
         .expect_err("field required for timestamp is absent");
@@ -595,6 +603,7 @@ mod test {
         load_index_config_update(
             ConfigFormat::Yaml,
             updated_config_yaml.as_bytes(),
+            &default_root,
             &original_config,
         )
         .expect_err("field required for default search is absent");

quickwit/quickwit-serve/src/index_api/index_resource.rs

@@ -286,13 +286,15 @@ pub async fn create_index(
 
 pub fn update_index_handler(
     metastore: MetastoreServiceClient,
+    node_config: Arc<NodeConfig>,
 ) -> impl Filter<Extract = (impl warp::Reply,), Error = Rejection> + Clone {
     warp::path!("indexes" / String)
         .and(warp::put())
         .and(extract_config_format())
         .and(warp::body::content_length_limit(1024 * 1024))
         .and(warp::filters::body::bytes())
         .and(with_arg(metastore))
+        .and(with_arg(node_config))
         .then(update_index)
         .map(log_failure("failed to update index"))
         .and(extract_format_from_qs())
@@ -325,6 +327,7 @@ pub async fn update_index(
     config_format: ConfigFormat,
     index_config_bytes: Bytes,
     metastore: MetastoreServiceClient,
+    node_config: Arc<NodeConfig>,
 ) -> Result<IndexMetadata, IndexServiceError> {
     info!(index_id = %target_index_id, "update-index");
 
@@ -336,9 +339,13 @@ pub async fn update_index(
     let index_uid = current_index_metadata.index_uid.clone();
     let current_index_config = current_index_metadata.into_index_config();
 
-    let new_index_config =
-        load_index_config_update(config_format, &index_config_bytes, &current_index_config)
-            .map_err(IndexServiceError::InvalidConfig)?;
+    let new_index_config = load_index_config_update(
+        config_format,
+        &index_config_bytes,
+        &node_config.default_index_root_uri,
+        &current_index_config,
+    )
+    .map_err(IndexServiceError::InvalidConfig)?;
 
     let update_request = UpdateIndexRequest::try_from_updates(
         index_uid,

quickwit/quickwit-serve/src/index_api/rest_handler.rs

@@ -89,8 +89,11 @@ pub fn index_management_handlers(
     // Indexes handlers.
     get_index_metadata_handler(index_service.metastore())
         .or(list_indexes_metadata_handler(index_service.metastore()))
-        .or(create_index_handler(index_service.clone(), node_config))
-        .or(update_index_handler(index_service.metastore()))
+        .or(create_index_handler(
+            index_service.clone(),
+            node_config.clone(),
+        ))
+        .or(update_index_handler(index_service.metastore(), node_config))
         .or(clear_index_handler(index_service.clone()))
         .or(delete_index_handler(index_service.clone()))
         .boxed()
@@ -1034,6 +1037,29 @@ mod tests {
                 .default_search_fields,
             ["severity_text", "body"]
         );
+        // test with index_uri at the root of a bucket
+        {
+            let resp = warp::test::request()
+                .path("/indexes")
+                .method("POST")
+                .json(&true)
+                .body(r#"{"version": "0.7", "index_id": "hdfs-logs2", "index_uri": "s3://my-bucket", "doc_mapping": {"field_mappings":[{"name": "timestamp", "type": "i64", "fast": true, "indexed": true}]},"search_settings":{"default_search_fields":["body"]}}"#)
+                .reply(&index_management_handler)
+                .await;
+            let body = std::str::from_utf8(resp.body()).unwrap();
+            assert_eq!(resp.status(), 200, "{body}",);
+        }
+        {
+            let resp = warp::test::request()
+                .path("/indexes/hdfs-logs2")
+                .method("PUT")
+                .json(&true)
+                .body(r#"{"version": "0.7", "index_id": "hdfs-logs2", "index_uri": "s3://my-bucket", "doc_mapping": {"field_mappings":[{"name": "timestamp", "type": "i64", "fast": true, "indexed": true}]},"search_settings":{"default_search_fields":["severity_text", "body"]}}"#)
+                .reply(&index_management_handler)
+                .await;
+            let body = std::str::from_utf8(resp.body()).unwrap();
+            assert_eq!(resp.status(), 200, "{body}",);
+        }
     }
 
     #[tokio::test]