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

Prevent OSS plugins from having required dependencies on optional plugins #106485

Closed
kobelb opened this issue Jul 21, 2021 · 10 comments · Fixed by #107432
Closed

Prevent OSS plugins from having required dependencies on optional plugins #106485

kobelb opened this issue Jul 21, 2021 · 10 comments · Fixed by #107432
Assignees
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@kobelb
Copy link
Contributor

kobelb commented Jul 21, 2021

When we stopped building the OSS distributable, we removed the only systemic protection that we had to prevent OSS plugins from having dependencies on X-Pack plugins. After further discussion, we've come to the conclusion that we should allow OSS plugins to have optional dependencies on X-Pack plugins. However, we should not allow OSS plugins to have required dependencies on X-Pack plugins. We should enforce this as part of CI or the build itself to prevent required dependencies from OSS to X-Pack.

@kobelb kobelb added the Team:Operations Team label for Operations Team label Jul 21, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@jportner
Copy link
Contributor

jportner commented Jul 29, 2021

I was playing with this today. It appears that we don't have any controls in place today to prevent dependencies from OSS to X-pack. However, we do have an ESLint rule to prevent imports:

kibana/.eslintrc.js

Lines 473 to 477 in 3612a7a

{
target: ['src/**/*'],
from: ['x-pack/**/*'],
errorMessage: 'OSS cannot import x-pack files.',
},

@elastic/kibana-operations Do we have any options to enforce this differently? Unless I'm missing something, this ESLint rule is the only thing preventing an OSS plugin from depending on an X-pack plugin. But I don't see a way that we can modify this rule to allow for optional dependencies and only block required dependencies. Perhaps that needs to be done differently at the build level?

@tylersmalley
Copy link
Contributor

By optional dependencies, I assume you mean with the plugin contract. For that, it seems the best way to enforce that would be in core.

@jportner
Copy link
Contributor

Yes, via the kibana.json file's optionalPlugins field. I guess that does sound like a Core concern. @elastic/kibana-core thoughts?

@joshdover
Copy link
Contributor

Yeah I agree this seems easiest to solve from the PluginsService in Core. We'll need to start tracking where the plugin discovery process found a plugin (eg oss, x-pack, or external) and do the enforcement based on that. We have other use cases for limiting features based on whether plugins are a 1st party or 3rd party plugin (eg #106857), so I imagine this would be a broadly useful thing to add.

@jportner jportner added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed Team:Operations Team label for Operations Team labels Jul 30, 2021
@jportner jportner self-assigned this Aug 2, 2021
@mshustov
Copy link
Contributor

mshustov commented Aug 4, 2021

@kobelb Should we allow external plugins to declare a dependency on x-pack ones? IMO it's a valid case. I think we need to call it out and test this scenario explicitly.

@jportner
Copy link
Contributor

jportner commented Aug 4, 2021

I don’t see why we wouldn’t allow this.

The only reason we want to restrict dependencies at all is because if OSS plugins require X-Pack plugins, then the OSS build would be broken. And we want to avoid breaking the OSS build.
External plugins should be allowed to require X-Pack plugins IMO.

@stacey-gammon
Copy link
Contributor

To clarify some things, we no longer create a separate OSS and x-pack build, so the reason we didn't want to support a required dependency longer exists. If there is a reason for a required dependency, we should re-open this discussion and consider supporting it.

@lukeelmers
Copy link
Member

the reason we didn't want to support a required dependency longer exists

I'm struggling to find a reference to the original internal discussion, but I believe the reason we wanted to only allow optional dependencies from oss to x-pack was less a technical one, and more of a pragmatic one -- in case there were a time in the future where we did end up maintaining separate builds again, it felt "safer" to not start introducing a bunch of hard dependencies between oss & x-pack. cc @kobelb as he was advocating for this approach IIRC

@kobelb
Copy link
Contributor Author

kobelb commented Jan 20, 2022

@lukeelmers copying my thoughts from the private issue about this here:

If we were to do this, we would be preventing ourselves and all others from ever being able to produce an OSS build of Kibana. If we want to go in this direction, we'll likely need to get the input of others, as this wasn't discussed during our original re-licensing effort. We did agree to no longer produce the OSS distributables for download, but we didn't decide that we should put ourselves in a position where this isn't possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants