Skip to content
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

Create stable API for accessing safe Elasticsearch config values in plugins #119862

Open
3 tasks
joshdover opened this issue Nov 29, 2021 · 9 comments
Open
3 tasks
Labels
impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture

Comments

@joshdover
Copy link
Contributor

joshdover commented Nov 29, 2021

Blocked by #117496

In #117496, we added the ability for plugins to read CA fingerprints from the Elasticsearch configuration, a feature we plan to support for the long-term to allow plugins to integrate directly with the new security-on-by-default feature.

In order to minimize scope for the 8.0 release, this change was added to the existing legacy.globalConfig$() API. We should move this and any other necessary ES config values to a stable, undeprecated API directly on the Elasticsearch service.

It should still be accessible on the PluginInitializerContext interface so that it can be accessed in the same way as all other configuration values.

Scope:

  • Identify config values in use that are in legacy & should be available on a stable API
  • Determine whether they definitely need to be on PluginInitializerContext or whether providing on the ES API is sufficient
  • Remove legacy config
@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture labels Nov 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@exalate-issue-sync exalate-issue-sync bot added impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed loe:small Small Level of Effort impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Nov 29, 2021
@pgayvallet
Copy link
Contributor

It should still be accessible on the PluginInitializerContext interface so that it can be accessed in the same way as all other configuration values.

@joshdover any reason to want this exposed via the initializer context instead of an API from the elasticsearch service's setup contract?

@joshdover
Copy link
Contributor Author

It should still be accessible on the PluginInitializerContext interface so that it can be accessed in the same way as all other configuration values.

This was my only reason: consistency with other config values. Though tbh, I've always felt that this constructor being separate from setup was really unnecessary and just added complexity rather than made anything simpler. Maybe we should move away from PluginInitializerContext entirely?

@pgayvallet
Copy link
Contributor

This was my only reason: consistency with other config values.

Ok, thanks, just wanted to confirm using a core service's contract wouldn't be an issue for you.

FWIW, the config exposed via PluginInitializerContext was only meant to expose a plugin's specific config properties (and then we added this ugly legacyConfig workaround). If a plugin was to share parts of its config to some consumers, it would do so with a setup and/or start contract API. I think following the same logic here would make sense, therefor exposing this via the elasticsearch setup contract.

Identify config values in use that are in legacy & should be available on a stable API

Note that in addition to the legacyConfig we're exposing to plugins

export type SharedGlobalConfig = RecursiveReadonly<{
elasticsearch: Pick<ElasticsearchConfigType, typeof SharedGlobalConfigKeys.elasticsearch[number]>;

We're also exposing the whole config via ElasticsearchServiceSetup.legacy.config$:

legacy: {
config$: this.config$,
},

which is (at least) used by the console proxy:

this.esLegacyConfigService.setup(elasticsearch.legacy.config$);

Not sure if this one should be considered as part of the scope of this issue?

@joshdover
Copy link
Contributor Author

We're also exposing the whole config via ElasticsearchServiceSetup.legacy.config$:

Ah interesting, didn't realize we had that as well. FWIW our main use case for this has been eliminated and we no longer plan to need to read any ES config values from the Fleet UI side of things. I could see this changing in the future, but no plans currently.

Other than Console, I'm not sure what the requirements are for other plugins for config values. We definitely should remove legacy anything though and have a long-term option.

@lukeelmers lukeelmers added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Mar 1, 2022
@exalate-issue-sync exalate-issue-sync bot added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Mar 1, 2022
@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Mar 15, 2022
@TinaHeiligers
Copy link
Contributor

I'm enabling Jira sync to get this issue on the team's roadmap. We're long past the platform migration initiative and should be removing anything legacy-related.

@richkuz
Copy link
Contributor

richkuz commented Aug 16, 2022

Enterprise Search has a use case for fetching the external Elasticsearch URL: #77464 (comment)

@lukeelmers
Copy link
Member

@richkuz Based on Alex's comment (#77464 (comment)), it sounds like simply exposing the ES URL that Kibana is configured with isn't going to solve the problem for Enterprise Search since y'all need the external URL.

Would you mind opening a separate issue for that use case (exposing external ES URL to Kibana plugins?). It would be a blocker for #77464 and for the Enterprise Search use case.

@richkuz
Copy link
Contributor

richkuz commented Aug 24, 2022

👍 Submitted #139405

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

7 participants