-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POC] init impl to add datasource service/client lifecycle #19
Conversation
const url = dataSourceObj.endpoint.url; | ||
/** | ||
* TODO: | ||
* credential manager will provide "decrypt(authId: string)" to return auth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noCharger do we have action item for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Decrypt function
As discussed offline, we can use https://github.com/elastic-analytics/OpenSearch-Dashboards/blob/db8c74eacd7a78830f[…]c/plugins/credential_management/server/crypto/cli/crypto_cli.ts for the end-to-end PoC. Support decrypt(authId: string)
is a non-blocking follow-up.
Also from the principle of single responsibility, fetch credential using saved object cli and decrypt with cryto cli could be decoupled. Let's name the function as fetch_and_decrypt(id: string)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noCharger do we have action item for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noCharger Made the change to first fetch encrypted password in opensearch_data_service
, and then use existing CrptoCLI -> decrypt()
function for the end-to-end use case to run. Any subsequent change will be posted in separate PR. But I don't think a functionality of getCredentialById
is a multi-responsibility. We can discuss more
Please add one TODO: pick up one viz to demonstrate usage of the new data source client. |
Signed-off-by: Zhongnan Su <[email protected]>
@seraphjiang End to end use case verified. Steps added to the PR description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not commit yarn.lock
} | ||
async asDataSource(dataSourceId: string) { | ||
const { endpoint, credentialId } = await this.getDataSourceInfo(dataSourceId); | ||
const { username, password } = await this.getCredentialInfo(credentialId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to consider more auth types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's are the current defined auth type in data source data model or credential manager? @kristenTian @noCharger
} | ||
|
||
// close anything in pool | ||
public async close() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check the opensearch js client, see if it can keep the connection until being explicitly closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/elastic/elasticsearch-js/blob/4def742e4774dba44f31b8e1701345a8c29870ee/lib/Connection.js#L59-L63
keep-alive
is default to true in ClientOptions
private readonly log: Logger; | ||
private readonly config$: Observable<OpenSearchConfig>; | ||
private auditorFactory?: AuditorFactory; | ||
private stop$ = new Subject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this field being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used in the Compatibility check(that I commented out for now). Compatibility checks happen constantly until "this.stop" is called (which it will be called in stop()
stage in our case)
7c33c9d
to
7f0bfa3
Compare
7f0bfa3
to
2567a2e
Compare
removed |
Description
dataSourceClientsPool
osd.yml
asDataSource()
to find/create client, then return to calleropensearch_data_service
savedObjectClient
DataSourceClient
instanceopensearch_data_service
toCoreSetup
,CoreStart
,CoreRouteHandlerContext
search_source
to adddataSourceId
field, and add it toISearchOptions
to provide tosearch()
interfaceopensearchagg.ts
, to attach data source id if existsopensearch_seearch_strategy
to select between default cluster client and data source clientTODO
index pattern
anddataSource
, pending PR from @kristenTianTest
Only manual test for now
Steps:
dataSourceId
to ecommerce index pattern by something like:Issues Resolved
TBA
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr