Skip to content
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.

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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rdettai
Copy link
Collaborator

@rdettai rdettai commented Mar 11, 2025

Description

Users can set the the index_uri to be the root of a bucket, and this panics when calling the index update endpoint. This PR:

  • replaces the panic with an Internal Server error
  • makes it possible to update an index even if the index_uri is the root of a bucket

How was this PR tested?

Describe how you tested this PR.

@rdettai rdettai self-assigned this Mar 11, 2025
@rdettai rdettai requested a review from guilload March 11, 2025 10:19
@rdettai rdettai force-pushed the fix-bucket-root-index-update branch from a3117da to 9a96456 Compare March 11, 2025 10:31
@rdettai rdettai force-pushed the fix-bucket-root-index-update branch from 6b51f6b to 18ab970 Compare March 11, 2025 13:57
@guilload
Copy link
Member

guilload commented Apr 9, 2025

The benefit of your fix only lies in not panicking in load_index_config_update. The new Uri::parent_unchecked function is only used in update_index in quickwit-serve and its logic is irrelevant. In your unit test, the value returned by parent_unchecked is ignored because the update carries an index URI. If you remove it, parent_unchecked returns s3:// to which the index ID is joined. Now we have s3://<index ID> instead of s3://<bucket name> for the index URI and we still have an error. So parent_unchecked is useless.

The best fix I can come up with is to deserialize the new config as a JsonValue, and extract index_uri from it. If it exists, verify that we have current_index_config.index_uri == new_index_config.index_uri, if not override new_index_config.index_uri with the old one, passing a dummy default_index_root_uri to load_index_config_from_user_config which will be ignored.

@rdettai rdettai force-pushed the fix-bucket-root-index-update branch from 8c2e32a to fa83c63 Compare April 10, 2025 08:30
@rdettai
Copy link
Collaborator Author

rdettai commented Apr 10, 2025

Thanks for your comment. I think the root of the problem here is that I initially over-complexified the behavior of the update endpoint regarding the index_uri. I wanted to avoid that an update of the default_index_root_uri breaks updates that use that default. I think it was over engineering. I propose to simplify that to let the update endpoint use the same default URI as the create endpoint.

It think it's fine to make this change as the update endpoint isn't released yet and anyway, this has no consequence unless the default_index_root_uri root URI is used along with the update endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants