es.createClient: improve config merging logic #129587
Conversation
pgayvallet
added
Feature:elasticsearch
Team:Core
| @@ -88,8 +88,9 @@ export function parseClientOptions( | |||
| password: config.password, | |||
| }; | |||
| } else if (config.serviceAccountToken) { | |||
| // TODO: change once ES client has native support for service account tokens: https://github.com/elastic/elasticsearch-js/issues/1477 | |||
| clientOptions.headers!.authorization = `Bearer ${config.serviceAccountToken}`; | |||
| clientOptions.auth = { | |||
There was a problem hiding this comment.
Not directly related to what this PR was supposed to address, but I saw this TODO and the client is now natively supporting bearer auth, so.
| import { Writable } from '@kbn/utility-types'; | ||
| import type { ElasticsearchClientConfig } from './client'; | ||
|
|
||
| type WritableConfig = Writable<ElasticsearchClientConfig>; |
There was a problem hiding this comment.
ElasticsearchClientConfig / ElasticsearchConfig defines their properties as readonly, so I had to use a writable version here.
|
Pinging @elastic/kibana-core (Team:Core) |
| delete writableBaseConfig.serviceAccountToken; | ||
| } | ||
|
|
||
| return merge({}, writableBaseConfig, configOverrides); |
There was a problem hiding this comment.
just a thought/NIT: since we're already shallow cloning the object on line 21 {} at the beginning is not needed. although having it might prevent future bugs if the code changes and then we're mutating objects.
There was a problem hiding this comment.
Yea, I agree that it's not strictly necessary, but it just felt safer to already add it. OTOH there is a unit test to check against mutating the source, so it may be fine
| clientConfig?: Partial<ElasticsearchClientConfig> | ||
| ) { | ||
| const config = clientConfig ? merge({}, baseConfig, clientConfig) : baseConfig; | ||
| const config = clientConfig ? mergeConfig(baseConfig, clientConfig) : baseConfig; |
There was a problem hiding this comment.
isnt this easier to read?
clientConfig: Partial<ElasticsearchClientConfig> = {}
) {
const config = mergeConfig(baseConfig, clientConfig);
I am also concerned that not using mergeConfig in both cases might cause unexpected bugs since the function mergeConfig has special logic that will only apply to 1 branch of this statement
There was a problem hiding this comment.
fair enough, that seems reasonable
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
pgayvallet
added
v7.17.3
auto-backport
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
* es.createClient: improve config merging logic * add additional test * fix test * address review comments Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit b822275) # Conflicts: # src/core/server/elasticsearch/elasticsearch_service.ts
* es.createClient: improve config merging logic (#129587) * es.createClient: improve config merging logic * add additional test * fix test * address review comments Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit b822275) # Conflicts: # src/core/server/elasticsearch/elasticsearch_service.ts * use 7.x config props
Summary
Fix #128661
username/passwordandserviceAccountTokenauth.bearerconfig instead of manual header for client configurationChecklist