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

Always use POST for queries; disable readonly setting by default #184

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

Conversation

slvrtrn
Copy link
Contributor

@slvrtrn slvrtrn commented Dec 3, 2024

Summary

See #181; it is better not to enforce readonly by default at all.

From the docs:

Setting readonly = 1 prohibits the user from changing settings.,

This might be a huge deal for ClickHouse users since ClickHouse has 700+ configurable settings. Currently, a query with length > 8192 that fetches data will not be allowed to change settings because of forced readonly=1 in that code branch.

Additionally, it is possibly safer to just always send queries via the POST body, as long URLs could be truncated by the proxies.

This is a breaking change (readonly setting is no longer set by default for queries that fetch data); however, it is still possible to attach readonly setting when needed using with_option.

Checklist

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided so that we can include it in CHANGELOG later
  • For significant changes, documentation in README and https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@slvrtrn slvrtrn marked this pull request as ready for review December 4, 2024 00:31
src/query.rs Outdated Show resolved Hide resolved
serprex
serprex previously approved these changes Dec 4, 2024
@mshustov
Copy link
Member

mshustov commented Dec 4, 2024

perhaps it is better not to enforce readonly by default at all.

We should clarify why: Setting readonly = 1 prohibits the user from changing settings., which might be a huge deal for ClickHouse users since ClickHouse has 700+ configurable settings. see https://clickhouse.com/docs/en/operations/settings/permissions-for-queries#readonly

Co-authored-by: Philip Dubé <[email protected]>
@loyd
Copy link
Collaborator

loyd commented Dec 5, 2024

As far as I remember, (older?) CH version rejected some SELECT-over-POST queries without readonly=1 if a user is restricted. But it could be my false memory or a bug in older versions.

as long URLs could be truncated by the proxies.

On the other hand, providers can log these queries or even produce metrics, but this is usually done only for query params. This is why, initially, this crate used the SELECT-over-GET approach.
Not sure whether someone uses it.

This is a breaking change

Why (if CH accepts such queries even for restricted users)?

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Dec 5, 2024

@loyd

This is a breaking change

Why (if CH accepts such queries even for restricted users)?

Perhaps not breaking, indeed, only for the very outdated ClickHouse versions (which we don't officially support anyway)...
Shall we proceed with merging this? WDYT?

@loyd
Copy link
Collaborator

loyd commented Dec 7, 2024

Shall we proceed with merging this? WDYT?

Yes, but I'm not sure we won't break something unintentionally and will force people to set read_only=1 for most queries.

To be sure, we should update our CI tests to run against restricted (RO) users.

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Dec 10, 2024

@mshustov

We should clarify why: Setting readonly = 1 prohibits the user from changing settings., which might be a huge deal for ClickHouse users since ClickHouse has 700+ configurable settings. see https://clickhouse.com/docs/en/operations/settings/permissions-for-queries#readonly

I double-checked

When set to 1, allows:

  • All types of read queries (like SELECT and equivalent queries).
  • Queries that modify only session context (like USE).

So, queries like this will work:

curl -XPOST "http://localhost:8123?readonly=1&default_format=JSONEachRow&max_execution_time=0.5" --data-binary "select * from foo" 

{"i":42}
{"i":144}

See that it has

default_format=JSONEachRow
max_execution_time=0.5

Still need to clarify what could be a potential corner case with readonly=1...

@loyd
Copy link
Collaborator

loyd commented Dec 16, 2024

@slvrtrn, ok, it's fine.

Let's release it as 0.14 after merging neighboring obviously non-breaking PRs (chrono, written_bytes, row(crate))

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.

4 participants