wangzhigang1999 commented on PR #7451:
URL: https://github.com/apache/kyuubi/pull/7451#issuecomment-4587409731
Thanks for tracking this down!
I'd like to talk about how the fix is done before it lands. Not because the
direction is wrong — I'm just not sure that passing the pattern into every
`ProcBuilder` is the best long-term approach. I might be missing something, so
feel free to disagree.
**What I agree with:** redaction is done on the server side, so the pattern
has to be read from the server config, not from the config that goes to the
engine. That part is correct and necessary.
**My concern is the mechanism.** `conf.get(...)` returned `None` because
`serverOnly` is implemented by *removing* the entry from the per-session config
in `getUserDefaults`. `ProcBuilder` runs on the server but holds that stripped
config, so redaction quietly turned off. Passing `Option[Regex]` down fixes
redaction, but it only fixes this one symptom:
- It doesn't scale. The next `serverOnly` config that some server-side code
needs while launching an engine will hit the same problem, and we'd add another
parameter for it.
- An earlier version of this PR passed `serverConf: KyuubiConf` instead. I
think that's worse: now there are two configs of the same type, told apart only
by their variable name — which is exactly the kind of mix-up that caused this
bug.
So the real problem is that `serverOnly` mixes two unrelated things into one
"remove": "don't send this to the engine" and "users can't change this".
Removing the entry does both, but it also means any server-side code that
legitimately needs the value can no longer read it.
**There's a related problem with the same cause**, which is why I'd rather
fix the model than patch it: the launch path does `conf.getAll` with no
per-engine filtering (`SparkProcessBuilder`), prefixes everything with
`spark.`, and ships it. So a Flink tuning key like
`kyuubi.engine.flink.memory`, set in a shared `kyuubi-defaults.conf`, ends up
on the Spark engine's command line as `spark.kyuubi.engine.flink.memory=...` —
harmless noise for tuning keys, but any sensitive value in there rides along
too. Both problems are "send everything, then remove a few".
**The direction I'd suggest** is to split `serverOnly` into two separate,
independent flags, and stop using "remove" to control where a config goes:
- `audience` — which engines a config is allowed to reach. Instead of
removing it from the config, we filter it out when building the engine's
startup command. The value is figured out from the key prefix by default
(`kyuubi.engine.spark.*` → Spark, `kyuubi.server.*` → server-only), **and** can
also be set by hand on the entry when the prefix isn't enough. So most configs
need no marking at all, but you can still set it manually when you need to.
- `immutable` — users can't override it. This is checked when Kyuubi merges
the user's config. It's set by hand, on purpose, because "can a user change
this?" is a security decision worth deciding per config.
Then `serverOnly` is just `audience(server-only) + immutable`, and we can
drop it. Two benefits: (1) the value stays in the server config and is fixed,
so redaction goes back to a plain `conf.get` and the `Option[Regex]` passing
here goes away completely; (2) filtering by `audience` also stops sending one
engine's configs to other engines. One model, both problems, no per-config
wiring.
```mermaid
flowchart TB
MASTER["server config (loaded on server, the source of truth)"]
UO["user config: connection / SET / user-defaults"]
UO --> IMM{"immutable key?"}
IMM -->|yes| PIN["user value ignored, server value kept"]
IMM -->|no| APPLY["user value applied"]
MASTER --> CONF
PIN --> CONF
APPLY --> CONF
CONF["full config on the server — nothing removed"]
CONF -->|"conf.get(...)"| READ["server-side reads — redaction sees the
real value"]
CONF --> PROJ{"build config for target"}
PROJ -->|"engine = Spark"| SPARK["send to Spark: audience is Spark or
all-engines"]
PROJ -->|"engine = Flink"| FLINK["send to Flink: audience is Flink or
all-engines"]
PROJ -->|"logs / REST"| LOG["sent out, masked if sensitive"]
```
In short: `immutable` is checked when a user config comes in, `audience` is
applied when a config goes out, and because nothing is removed from the server
config, server-side reads (including redaction) always see the real value —
which is exactly what broke here.
Curious to hear your thoughts!
CC @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
