Add bearer token reloading and reuse in multiple storage backends #7360
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7360 +/- ##
==========================================
- Coverage 96.47% 96.44% -0.04%
==========================================
Files 374 376 +2
Lines 22865 22957 +92
==========================================
+ Hits 22060 22140 +80
- Misses 609 618 +9
- Partials 196 199 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/auth/tokenloader_test.go
Outdated
| require.NoError(t, err) | ||
| assert.Equal(t, token, t1, "expected %q, got %q") | ||
|
|
||
| // Change the file, but within cache interval |
There was a problem hiding this comment.
there is no way to guarantee that in CI environment this will be within the cache interval. You need to test with a mock time.Now function and advance the time manually.
| @@ -219,6 +219,8 @@ type BearerTokenAuthentication struct { | |||
| FilePath string `mapstructure:"file_path"` | |||
| // AllowTokenFromContext, if set to true, enables reading bearer token from the context. | |||
| AllowFromContext bool `mapstructure:"from_context"` | |||
| // ReloadInterval contains the interval at which the bearer token is reloaded. | |||
| ReloadInterval configoptional.Optional[time.Duration] `mapstructure:"reload_interval"` | |||
There was a problem hiding this comment.
what is the meaning of this being None? No reload? Why can't we use 0 for that?
There was a problem hiding this comment.
@yurishkuro If its none , it default to 10 seconds . Setting Reload interval 0 mean no caching , file is checked for token on every request
There was a problem hiding this comment.
Defaulting is a different process - after defaulting applies this won't be None, so not what I asked. This field treats 0 as "don't reload", which is semantically the same as None. There's no need for Optional here.
| @@ -219,6 +219,8 @@ type BearerTokenAuthentication struct { | |||
| FilePath string `mapstructure:"file_path"` | |||
| // AllowTokenFromContext, if set to true, enables reading bearer token from the context. | |||
| AllowFromContext bool `mapstructure:"from_context"` | |||
| // ReloadInterval contains the interval at which the bearer token is reloaded. | |||
| ReloadInterval configoptional.Optional[time.Duration] `mapstructure:"reload_interval"` | |||
There was a problem hiding this comment.
Defaulting is a different process - after defaulting applies this won't be None, so not what I asked. This field treats 0 as "don't reload", which is semantically the same as None. There's no need for Optional here.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(authMethods) > 0 { |
There was a problem hiding this comment.
how is basic auth handled?
@yurishkuro basic authentication is currently being handled at the Elasticsearch client level using client options. You can see it here:
using the olivere/elastic client, which has built-in support for basic auth through the SetBasicAuth() method.
I’m wondering if it might make sense to move the basic auth logic to the HTTP layer as well. That way, if we ever decide to switch away from olivere/elastic, we won’t have to rewrite the auth logic it would already be handled at the transport layer. am i correct ?
There was a problem hiding this comment.
I think it does make sense to move it here, doesn't look like it will make much difference and we will have everything in on place. Separate PR please.
…Multi-Auth Framework + Dynamic Token Reloading Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
619e4a6 to
c8deb4f
Compare
<!-- !! Please DELETE this comment before posting. We appreciate your contribution to the Jaeger project! 👋🎉 --> ## Which problem is this PR solving? part of - #7225 ## Description of the changes - Refactors Elasticsearch basic authentication from client-level initialization to HTTP transport layer, enabling hot password reloading and consistent authentication patterns. #7360 (comment) - It is part of the planned work to add support for API authentication, as discussed in #7230 (comment) ### Chnages - Moved basic auth from` elastic.SetBasicAuth()` to HTTP transport layer - Pre-encodes credentials as base64 in `TokenFn `for performance - Reuses existing` TokenProvider `infrastructure for file-based password reloading - Added `ReloadInterval` field to `BasicAuthentication` struct - New CLI flag: ` --es.password-reload-interval ` (default: 10s) ## How was this change tested? - ## Checklist - [ ] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [ ] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Which problem is this PR solving?
part of
Description of the changes
It is part of the planned work to add support for API authentication, as discussed in #7230 (comment)
Key Changes
Generic Authentication Framework
StaticTokenandOverrideFromCtxfields inRoundTripperConfigstruct supporting multiple authentication schemesToken File Loading
TokenProviderwith interval-based caching and graceful error handlingContext-Based Token Override
Files Changes
New
internal/auth/tokenloader.go- Cached token file loadinginternal/auth/tokenloader_test.go- Token loader testsRefactored
internal/auth/transport.go- Generic multi-auth transportinternal/auth/transport_test.go- Transport tests (12 scenarios)internal/storage/elasticsearch/config/auth_helper.go- Auth config helperinternal/storage/metricstore/prometheus/reader.go- Prometheus integrationHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test