Add API Key Authentication Support for Elasticsearch Storage #7402
Conversation
Signed-off-by: danish9039 <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7402 +/- ##
==========================================
+ Coverage 96.46% 96.47% +0.01%
==========================================
Files 375 376 +1
Lines 22958 23025 +67
==========================================
+ Hits 22147 22214 +67
Misses 613 613
Partials 198 198
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:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Signed-off-by: danish9039 <[email protected]>
|
@yurishkuro can you please review this ,This PR finalizes our effort to add API authentication support for the Elasticsearch storage backend. #7230 (comment) |
| @@ -227,6 +228,17 @@ type BearerTokenAuthentication struct { | |||
| ReloadInterval time.Duration `mapstructure:"reload_interval"` | |||
| } | |||
|
|
|||
| // APIKeyAuthentication contains the configuration for attaching API keys | |||
| type APIKeyAuthentication struct { | |||
There was a problem hiding this comment.
this struct is identical to the previous one. Please merge into one, e.g. TokenAuthentication. And consolidate init functions, there's no need to repeat so much boiler plate and tests
There was a problem hiding this comment.
@yurishkuro We can definitely merge BearerAuthAuthentication and APIKeyAuthentication structs, but doing so would remove the ability to use both bearer token and API key simultaneously , authentication framework was refactored to support multiple authentication methods together, allowing both API key and bearer token authentication to be used concurrently.
#7230 (comment)
If we're okay with limiting the system to use either bearer token or API key at a time (but not both), then we can go ahead with merging the two structs .
Let me know your take on this .
There was a problem hiding this comment.
You can still have two fields in the auth config for bearer and api tokens, but they can reuse the same struct type to represent their data
There was a problem hiding this comment.
You can still have two fields in the auth config for bearer and api tokens, but they can reuse the same struct type to represent their data
got it !
Signed-off-by: danish9039 <[email protected]>
Signed-off-by: danish9039 <[email protected]>
| @@ -193,9 +193,21 @@ type BulkProcessing struct { | |||
| Workers int `mapstructure:"workers"` | |||
| } | |||
|
|
|||
| // TokenAuthBase contains the common fields shared by all token-based authentication methods | |||
| type TokenAuthBase struct { | |||
There was a problem hiding this comment.
you don't need the other two types, I would just get rid of them, keep the code simpler
There was a problem hiding this comment.
Simplified as per your suggestion.
Signed-off-by: danish9039 <[email protected]>
| func initBearerAuth(tokenAuth *TokenAuthentication, logger *zap.Logger) (*auth.Method, error) { | ||
| if tokenAuth == nil { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
There's an extra tab character at the end of this line that should be removed for consistent formatting.
| } | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Signed-off-by: danish9039 <[email protected]>
Which problem is this PR solving?
part of
Description of the changes
Changes
Added new authentication method -
APIKeyAuthenticationstruct to support API key-based auth alongside existing basic/bearer token methodsDual source support - API keys can be loaded from files OR extracted from request context
Hot reloading - File-based API keys automatically reload at configurable intervals (default 10s, 0 disables)
New command flags - Added
--es.api-key-file,--es.api-key-allow-from-context,--es.api-key-reload-intervalNew context module - Created
apikey-context.gowithGetAPIKey()andContextWithAPIKey()functionsConditional logic - Health check disabled when
AllowFromContext=trueANDFilePath=""for both bearer tokens and API keysHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test