Add user principal tag in metrics #2445
Conversation
There was a problem hiding this comment.
LGTM 👍 Thanks for your contribution, @fivetran-kostaszoumpatianos !
| * even crash the server. | ||
| */ | ||
| @WithDefault("false") | ||
| boolean enableInApiMetrics(); |
There was a problem hiding this comment.
Defaulting to false here looks reasonable to me. Still, since this may expose sensitive information, please open a dev email discussion for visibility.
There was a problem hiding this comment.
Thanks @dimas-b I sent an email to dev.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
Thanks @dimas-b I will send a |
|
@dimas-b I had to re-format the code, and your approval got removed. Could you please take another look? Thanks! |
|
FYI here is the ML discussion thread: https://lists.apache.org/thread/o7of5dpmglmkjosftqsyr54x3dcfo1o4 I replied there but to summarize my opinion here: I'm not against the idea but there are security risks. Imo this feature should be opt-in (which is the case) and guarded by a production readiness check. |
Thank you @adutra I have added a production readiness check. |
| if (config.userPrincipalTag().enableInApiMetrics()) { | ||
| return ProductionReadinessCheck.of( | ||
| Error.of( | ||
| "Metrics configuration includes user principal name in tags.", |
There was a problem hiding this comment.
This will effectively always produce WARN log messages in servers where principal tags are enabled, even if the admin user configured that explicitly. From my POV this is not ideal user experience, but having this WARN is probably better that accidentally exposing principal names.
I think we may want to add per-message suppression for production checks at some point.
There was a problem hiding this comment.
Yes, I agree. We should have that at some point. In the interim we could get away with just having the warning there. I can open an issue and work on it.
| @@ -113,6 +114,17 @@ public void warnOnFailedChecks( | |||
| } | |||
| } | |||
|
|
|||
| @Produces | |||
| public ProductionReadinessCheck checkMetricTags(MetricsConfiguration config) { | |||
| if (config.userPrincipalTag().enableInApiMetrics()) { | |||
There was a problem hiding this comment.
I think we are missing a readiness check when both tags are enabled, since this is the most dangerous situation.
There was a problem hiding this comment.
Indeed, this is a great point. I added one.
| @@ -39,7 +39,9 @@ public Map<String, String> getConfigOverrides() { | |||
| "polaris.metrics.realm-id-tag.enable-in-api-metrics", | |||
| "true", | |||
| "polaris.metrics.realm-id-tag.enable-in-http-metrics", | |||
| "true"); | |||
| "true", | |||
There was a problem hiding this comment.
At some point I think we'll need to refactor these tests and create a sort of "parameterized Quarkus test" that tests all the combinations of (realm tag on/off) x (principal tag on/off). But we can look into that later.
There was a problem hiding this comment.
Yes, good point. Thanks!
fivetran-kostaszoumpatianos
dismissed stale reviews from eric-maynard and dimas-b
via
b0e2cd7
fivetran-kostaszoumpatianos
requested review from
adutra,
dimas-b and
eric-maynard
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Fixes #2444.
This PR adds a user principal tag in metrics and an associated configuration option that turns it on.
This is the following:
polaris.metrics.user-principal-tag.enable-in-api-metrics, and by default this is set tofalse.To retrieve the user principal ID it uses the SecurityContext and annotates it with a
MeterTagannotation under the keyprincipal.