[datafusion] add a new table for introspecting the cluster's config#4964
[datafusion] add a new table for introspecting the cluster's config#4964MohamedBassem wants to merge 7 commits into
Conversation
|
This is still not ready for review. Sent it out by mistake. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53bce7f974
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013f94473c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74516f8969
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b5a3469bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
AhmedSoliman
left a comment
There was a problem hiding this comment.
Very cool. Excited to get this merged. Left a few comments but overall it looks good. The most notable is to add some facility to disable this table via config.
| mod network_server; | ||
| mod roles; | ||
|
|
||
| use std::sync::Arc; |
| let schema = projection.clone(); | ||
| let mut stream_builder = RecordBatchReceiverStream::builder(projection, 2); | ||
| let tx = stream_builder.tx(); | ||
| let node_id = self.metadata.my_node_id(); |
There was a problem hiding this comment.
Is there risk of this scanner running before the node acquiring its own node_id? If so, can we use the fallible version my_node_id_opt to avoid crashing?
If other tables already have the same risk, then it's okay to leave as is.
There was a problem hiding this comment.
yeah, both loglet workers and bifrost read streams seam to be reading the node_id in the beginning of the scan function as well.
| self.remote_scanner_manager.clone(), | ||
| None, // local scanner is registered separately by the node | ||
| )?; | ||
| crate::configs::register_self( |
There was a problem hiding this comment.
I can imagine scenarios where we might want to disable access to this table. Can we let this be controlled by a config option? (enabled by default though).
There was a problem hiding this comment.
Where do you think would be a good section to place this knob? admin.query_engine?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61fa21823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// against the hyphenated forms here. | ||
| fn is_potentially_secret(key: &str) -> bool { | ||
| let key = key.to_ascii_lowercase(); | ||
| [ |
There was a problem hiding this comment.
Redact private-key config entries
Fresh evidence beyond the earlier redaction comments is that Kafka additional_options can carry arbitrary librdkafka keys (crates/types/src/config/kafka.rs:30-35), and librdkafka defines ssl.key.pem and sasl.oauthbearer.assertion.private.key.pem as client private-key strings (Confluent docs). Flattened keys such as ingress.kafka-clusters[0].ssl.key.pem contain none of the current needles, so the config table returns the PEM material unredacted; please include private-key/key-pem style names or redact these option maps more conservatively.
Useful? React with 👍 / 👎.
This PR introduces a datafusion based table for introspecting configs across nodes (available over restatectl). This is meant to be a more ergonomic way for debugging config values than the
USR1signal mechanism that we have today. The implementation heavily mimics other tables in error handling, code structure, etc.Note: Our configs can sometimes include secret tokens (e.g. S3 access tokens, etc). This PR has a best effort redaction for such secrets, however, it doesn't guarantee that it'll cover every secret. The premise here is that this table is privileged (you need access to the fabric port to query it), so it's ok to do so. If we want better guarantees, we can introduce a new
Redacted<T>which always serde serializes toRedactedand use it for all of the secrets in our configs.Example usage: