dongjoon-hyun commented on PR #58599: URL: https://github.com/apache/spark/pull/58599#issuecomment-5584138666
Thanks for the update - the rename, the conditional init parameter, the docs entry and the test move all look good. One thing on the new documentation. It now says: > unless another authentication filter is configured **ahead of this one** in > `spark.ui.filters` to establish the request's user ... Only set it to `false` > when such an authentication filter is in place. I don't think that precondition can actually be met in either deploy mode. **cluster mode** - `ApplicationMaster.addAmIpFilter` does ```scala System.setProperty(UI_FILTERS.key, amFilter) ``` which replaces `spark.ui.filters` wholesale. `ApplicationMaster.main` has already pushed the properties-file values into `sys.props` by then, and `startUserApplication()` runs afterwards, so the user application's `new SparkConf()` sees `spark.ui.filters` containing `AmIpFilter` alone. A user-configured authentication filter is silently dropped, so there is no way to satisfy the precondition. **client mode** - `YarnSchedulerBackend.addWebUIFilter` builds ```scala val allFilters = Seq(filterName) ++ conf.get(UI_FILTERS) ``` and uses `prependFilterMapping` for already-installed handlers, so `AmIpFilter` is always first. Nothing can be configured "ahead of" it. That ordering is actually the safe one, though: a real authentication filter runs *after* `AmIpFilter` and its own request wrapper overrides the cookie principal, since the last wrapper in the chain determines `getRemoteUser()`. Which raises the question the doc doesn't answer: in client mode, if a real authentication filter is installed it already wins over the cookie, so what does setting this to `false` add? And in cluster mode the documented safe condition is unreachable, so the doc is effectively telling operators never to enable it. (The filter ordering itself is pre-existing, not something this PR introduces.) Could you reword the doc to match the actual filter ordering, and say which deploy mode(s) this option is meant for? -- 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]
