Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 15, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e3d6dd
Add failing unit test
rdettai Mar 11, 2025
71fb390
Fix index update endpoint panic when index_uri is the bucket root
rdettai Mar 11, 2025
f4790ca
Simplify update endpoint to use same default URI as create
rdettai Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference/rest-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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). | |
Expand Down
31 changes: 20 additions & 11 deletions quickwit/quickwit-config/src/index_config/serialize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
{
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();

Expand All @@ -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();
Expand All @@ -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();

Expand All @@ -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();
Expand All @@ -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();

Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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");
Expand Down
13 changes: 10 additions & 3 deletions quickwit/quickwit-serve/src/index_api/index_resource.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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");

Expand All @@ -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,
Expand Down
30 changes: 28 additions & 2 deletions quickwit/quickwit-serve/src/index_api/rest_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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]
Expand Down