search

opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.77k stars 1.82k forks source link

[opensearch-plugin] Cannot install old patch version of plugins on newer opensearch builds #1707

Closed peternied closed 9 months ago

peternied commented 2 years ago 
2021-12-10 21:54:54 INFO     Executing "/tmp/tmp3_3h_wbz/opensearch-1.2.1/bin/opensearch-plugin install --batch file:/tmp/tmp3_3h_wbz/opensearch-job-scheduler-1.2.0.0.zip" in /tmp/tmp3_3h_wbz/opensearch-1.2.1
-> Installing file:/tmp/tmp3_3h_wbz/opensearch-job-scheduler-1.2.0.0.zip
-> Downloading file:/tmp/tmp3_3h_wbz/opensearch-job-scheduler-1.2.0.0.zip
-> Failed installing file:/tmp/tmp3_3h_wbz/opensearch-job-scheduler-1.2.0.0.zip
-> Rolling back file:/tmp/tmp3_3h_wbz/opensearch-job-scheduler-1.2.0.0.zip
-> Rolled back file:/tmp/tmp3_3h_wbz/opensearch-job-scheduler-1.2.0.0.zip
Exception in thread "main" java.lang.IllegalArgumentException: Plugin [opensearch-job-scheduler] was built for OpenSearch version 1.2.0 but version 1.2.1 is running
    at org.opensearch.plugins.PluginsService.verifyCompatibility(PluginsService.java:390)
    at org.opensearch.plugins.InstallPluginCommand.loadPluginInfo(InstallPluginCommand.java:803)
    at org.opensearch.plugins.InstallPluginCommand.installPlugin(InstallPluginCommand.java:858)
    at org.opensearch.plugins.InstallPluginCommand.execute(InstallPluginCommand.java:263)
    at org.opensearch.plugins.InstallPluginCommand.execute(InstallPluginCommand.java:237)
    at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:100)
    at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
    at org.opensearch.cli.MultiCommand.execute(MultiCommand.java:104)
    at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
    at org.opensearch.cli.Command.main(Command.java:101)
    at org.opensearch.plugins.PluginCli.main(PluginCli.java:60)

This is blocking us from consuming the redistribution artifacts for patch releases.

davidlago commented 2 years ago

Is this intentional or accidental? i.e. would we want to at least allow for patch version variability and have plugins work with any 1.2.x version of core?

davidlago commented 2 years ago

Ah I see... I misunderstood, so this issue is to actually address this so that we can install old patch version of plugins 👍

dblock commented 2 years ago

A plugin version 2.1.0 will not start on OpenSearch 2.1.1 by design. Let's revisit this design during/at 2.0?

peternied commented 2 years ago

Removing v1.2.1 tag, we had to do a full rebuild for 1.2.1 to work around this issue.

dblock commented 2 years ago

@nknize can you please comment on what you think the intended design here is? Can we relax this limitation given semver? Could we do it in a patch release like 1.2.3?

nknize commented 2 years ago

can you please comment on what you think the intended design here is?

The inherited implementation is based on the inherited (legacy?) "requirement" that plugins are bundled in the monolithic repo and there's always a 1:1 core to plugin version relationship.

In theory I agree it should not be a requirement OpenSearch wants/should carry forward and (on the surface) the project desires to move forward supporting something like semver range syntax (e.g., ~1.2.3 is >=1.2.3 < 1.3.0); but the code isn't there yet. (I know I'm preaching to the choir that we should think this through before making quick reactive changes to the build logic).

Thinking this through a little more, relaxing the strict full version check makes life easier where we can independently release patched plugins and/or min core. On the other hand it also introduces a many to many relationship between patched components. (e.g., a job-scheduler 1.2.3 can be installed alongside notifications v1.2.5 which is installed alongside knn v1.2.2). Having to check all patched versions is a difficult world for support engineers.

This leniency also permits different patched plugins to run w/ a potentially CVE vulnerable core like v1.2.0. But as we've just seen, in a critical CVE apocalypse we should likely be more thoughtful about never allowing a current released plugin to be compatible w/ a vulnerable core release; we should probably bite the bullet and rebuild the entire stack w/ a one line build.gradle version change like: ~1.2.1 to ensure current releases are only compatible w/ the patched core (or greater). Even if it "takes so long" to re-spin and release the stack, users that "forget" to upgrade to the patched core will be reminded when they upgrade any patched plugin.

dblock commented 2 years ago

@nknize so you don't recommend we make this change for 1.2.3 if I read this correctly?

nknize commented 2 years ago

I don't recommend a change to introduce uncontrolled leniency on patched version checks, period. I'd be happy to discuss / review a change to introduce semver range checks on patched versions if someone wants to churn out that PR in time for 1.2.3.

reta commented 2 years ago

@nknize @dblock my five cents, given the recent issues with plugins and CVEs, all at the same time:

What do you think guys?

dblock commented 2 years ago

@nknize @dblock my five cents, given the recent issues with plugins and CVEs, all at the same time:

  • In general, I think it would make sense for OpenSearch core 1.2.x to accept any OpenSearch 1.2.x plugin (in more general sense, differences in patch releases should not be rejected). The reasoning behind it: no major or breaking changes are expected, so in general, it should be safe. Also, it is a viable fallback path in case more recent plugin versions have bugs or issues.

This does make sense, but it also means we may not be able to make interface additions (e.g. new field) in minor releases, doesn't it?

reta commented 2 years ago

This does make sense, but it also means we may not be able to make interface additions (e.g. new field) in minor releases, doesn't it?

Not necessarily, introducing the default interface methods won't break it, but could be difficult to catch though. Along with warning the user, we highlight the version mismatch and consequences, but surely we cannot guarantee it is going to work.

tlfeng commented 2 years ago

I think now we’ve got 2 proposals:

  1. Add logic in the script of building OpenSearch plugins, which allows plugins to choose the supported OpenSearch version range in the plugins’ Gradle build scripts. (origin: https://github.com/opensearch-project/opensearch/issues/1707#issuecomment-996078415)
  2. Modify the logic of compatibility verifcation in OpenSearch to accept any plugins that built for the same OpenSearch major and minor version. (origin: https://github.com/opensearch-project/opensearch/issues/1707#issuecomment-996903315)

Looks like they either can resolve the issue. In my opinion, the only conflict is, the 1st proposal allows the plugin to choose the supported patch OpenSearch version, while the 2nd "introduce uncontrolled leniency on patched version checks". I’d like to hear more opinions on the solutions.

I've got some opinions:

AmiStrn commented 2 years ago

+1 for @nknize's suggestion. Version range makes things very clear and flexible. However, we should provide tooling to test the versions in the range in my opinion, to help plugin developers create a good CI.

dblock commented 2 years ago

One reason why we were reluctant to relax the plugin version matrix fully by introducing a feature that lets plugins declare a compatible version range is precisely the amount of testing that would be required to release anything, and the exploding amount of potential combinations that administrators would be encountering. But I think we should still build this, and then start by using the feature to enable patch-level semver compatibility.

AmiStrn commented 2 years ago

One reason why we were reluctant to relax the plugin version matrix fully by introducing a feature that lets plugins declare a compatible version range is precisely the amount of testing that would be required to release anything, and the exploding amount of potential combinations that administrators would be encountering. But I think we should still build this, and then start by using the feature to enable patch-level semver compatibility.

The more I thought about this move the less I like the idea. Testing would be horrible in this situation, and I cant find a good way around it.

I agree with the following statement the most as it is also this exact case that has brought this issue to light:

This leniency also permits different patched plugins to run w/ a potentially CVE vulnerable core like v1.2.0. But as we've just seen, in a critical CVE apocalypse we should likely be more thoughtful about never allowing a current released plugin to be compatible w/ a vulnerable core release

jcgraybill commented 2 years ago

I agree with the conclusion being reached here. OpenSearch plugins are semi-independent of the rest of OpenSearch, but there aren't currently good mechanisms to prevent test-matrix explosion aside strict version-locking. Relaxing the version-locking requirement in a meaningful way is more than a change to the build process.

IMO this would be a great problem to tackle: how could we really decouple plugin deployment in a way that is user-friendly and developer-friendly.

rursprung commented 2 years ago

coming from opensearch-project/opensearch-plugins#122: IMHO plugin and core versioning should be completely de-coupled. to achieve this it'd be required to ensure that semver is being followed strictly.

as for testing, here's my suggestion:

this would of course not cover all aspects, but it'd cover the most important ones. and, thanks to semver, if somebody runs into trouble, you can always tell them to just upgrade to the next minor/patch version of a plugin and test again with that without incurring a big risk (presuming the plugin itself has been properly tested).

(not sure on which ticket the discussion should be continued - probably here and mine should be closed as a duplicate? i didn't see this one here before)

anasalkouz commented 2 years ago

+1 for for semver range. This will limit test combinations and reduce blast radius . In this case we need only to test the min & max value of the version range.

tlfeng commented 2 years ago

I got some concerns when dealing with the situation that a plugin depend on another plugin (extends another plugin through SPI). I’m thinking is it needed to require a plugin to declare the version range for its required plugin, however no matter needed or not, there will be issues.

When users prompt to install anomaly-detection plugin, they have to follow complex steps.

  1. Uninstall both of the job-scheduler and index-management plugin. (Because currently extensible plugin can't be uninstalled alone when there are plugins depend on it installed in the cluster.)
  2. Install a newer version of job-scheduler plugin.
  3. Install anomaly-detection plugin, and put Index management plugin back.
rursprung commented 2 years ago

IMHO in your example the two plugins shouldn't restrict to a specific minor version. if the plugins use proper semver they can just rely on e.g. >= 1.2.2 which then accepts anything <2.0.0.

but i think your example with plugins depending on each other shows something else: it should be possible to do an in-place upgrade of a plugin rather than having to uninstall the old version and then install the new one again. then you don't need to uninstall dependent plugins first (unless they're incompatible with the new version). i guess that's another feature request to spin out of this discussion?

IMHO managing such dependencies is a solved problem - all package managers (on os-level like apt, software development dependency managers like npm, etc.) deal exactly with this. esp. the os-level ones (e.g. apt) need to deal with it in a similar way opensearch is doing it: they need to be able to replace existing packages with new ones while others are depending on them and ensure that upgrades are done in the right order (and all of which while the system is running, which is more complicated than in the opensearch use-case where the opensearch node is not running during the installation).

dblock commented 2 years ago

hot-swap of plugins is on the radar of @saratvemulapalli

saratvemulapalli commented 2 years ago

I've opened up a meta issue which will solve this for the longer term: #2283 and enable hot-swap of plugins/extensions.

abseth-amzn commented 2 years ago

Hello all, I am working on supporting custom plugins with Amazon's managed OpenSearch service. It would be desirable to allow customers to upgrade their domain's engine version without requiring them to provide updated custom plugins every time, especially for patch version upgrades for the core. I have read through the above mentioned comments and see following options for the path forward:

  1. Relax patch version check for plugins. This would allow for smooth patch version upgrades that can happen due to a variety of reasons in the managed service.
  2. Introduce range checks on patch and minor versions. Apart from patch version upgrades, this would also allow for a smooth UpgradeDomain experience for customers.
  3. Allow for plugins to declare what versions of OpenSearch they are compatible with (across patch and minor versions). The core will validate if its version is in that range.

I wanted to explore the possibility of starting with #1 to allow for seamless version upgrade experience for customers using custom plugins. I understand that it would require additional testing effort but asking customers to provide custom plugins with updated versions (when most of the time there is not much plugin code change) may create significant friction in feature adoption. Looking for thoughts from the community on this.

dblock commented 2 years ago

Thanks for a thoughtful proposal @abseth-amzn! Tl;dr I would prefer (3) + (1) and I think it's a similar amount of work as just (1).

Longer story:

My preference would be for the longest term solution, aka (3) where plugins specify, for example, ~> 2.x by default (compatible with any 2.x version) and lock that down only if they find an incompatibility for whatever reason (e.g. >= 2.0, <= 2.3).

Because OpenSearch does follow semver, I think you could default to (1) if a plugin does not specify its compatibility, and introduce (3) later. It's weird that the dependency (server) would specify version compatibility for its dependent (plugin), but I think it's OK as a default when the dependent has no opinion.

Finally, I would not assume that (1) is less work than (3). I think that 99% of the work is on the server anyway, the only difference is that the plugin needs an additional configuration field for which version(s) of the server it's compatible with.

abseth-amzn commented 2 years ago

Thank you @dblock! To clarify, following could be the path forward based on the examples above:

  1. Relax the patch version check for plugins as OpenSearch follows SemVer.
  2. Enable plugins to declare the semver range of core versions they are compatible with.
  3. If a plugin does not specify the compatibility range then default to (1)
dblock commented 2 years ago

Yes. I think (1) is an independent change from (2), and (1) is a breaking change that would need to be introduced in a major version of OpenSearch, while (2) is a new feature and could be added to any version.

rursprung commented 2 years ago

2. Enable plugins to declare the range of core versions

with this you mean a semver definition like > 2.0.0, < 2.8, right? then this would be great news as it'd solve 99% of the problems encountered as plugins can then start using that once they upgrade to the opensearch version which supports this and then don't have to deal with it again until the next major release

Bukhtawar commented 2 years ago

Security patches are common and requires a faster deployment cycle to mitigate the security threats. I wanted to gather thoughts on relaxing patch version compatibility checks across the board, so that deploying security patches become seamless. With providing explicit compatible semver I feel we should keep the patch version out and have ranges defined up to the minor versions only >2.0, <2.8

Thoughts?

shwetathareja commented 2 years ago

Yes, there shouldn't be any breaking changes in plugin interfaces in minor version releases. But there are lot of internal classes/ objects exposed through these interfaces e.g. DiscoveryPlugin which has bunch of methods and expose other internal classes like TransportService/ NetworkService etc. and similarly ClusterService is exposed in a different ClusterPlugin interface. These internal classes are not bound by any plugin interface contract. And, if a plugin depends on their methods (which in turn can return other internal classes) and these are changed, then it can cause potential incompatibility or failures. This can be obvious if there is a compile time failure or just a behavior change, would figure out during runtime testing. So providing configuration to users to specify the plugin version range compatibility is one but real responsibility lies with testing. I found similar discussion for core extension points - https://github.com/opensearch-project/OpenSearch/issues/2868

Bukhtawar commented 2 years ago

Good point! The way forward I see with this is a clear documentation on opensearch.api/opensearch.internal on Java docs and strong BWC checks to ensure these contracts are honoured at patch and minor version bump ups. Any plugin user before they can specify semVer ranges for compatibility need to ensure via some gradle checks that they don't strictly depend on anything that is not opensearch.api and show them warnings on usages if any such dependency is found.

Having these checks on the core and the plugin package can definitely help avoid surprises.

krishna-ggk commented 2 years ago

Security patches are common and requires a faster deployment cycle to mitigate the security threats. I wanted to gather thoughts on relaxing patch version compatibility checks across the board, so that deploying security patches become seamless. With providing explicit compatible semver I feel we should keep the patch version out and have ranges defined up to the minor versions only >2.0, <2.8

Thoughts?

@Bukhtawar The main reason to also include patch version is to avoid uncontrolled leniency as discussed in https://github.com/opensearch-project/OpenSearch/issues/1707#issuecomment-996078415 .

However we probably just need min-bound for that scenario. I'm not able to think of reasons where a max-bound on patch version would be desirable.

dblock commented 2 years ago

However we probably just need min-bound for that scenario. I'm not able to think of reasons where a max-bound on patch version would be desirable.

Consider the case where we have version 1.3.3 out there and mistakenly commit a (breaking) change in 1.3.4. Plugin update relies on that feature, which then gets reverted in 1.3.5. Plugin may choose to first lock down its max and quickly release, before addressing the problem at the root.

What I am saying is that developers need control over compatibility from the plugin POV, leave it to them to figure out how they use it. We should introduce restrictions only where it's obviously harmful.

abseth-amzn commented 1 year ago

As a first step towards supporting semVer range of compatible versions for plugins, we could start with relaxing the patch version check (for default compatibility range).

This would require building the ability to test across patch versions of the core for every patch release of a plugin and vice versa. This would include testing a plugin with compiled version of newer patch of OpenSearch (and vice versa) along with running integration tests to catch runtime issues.

saratvemulapalli commented 1 year ago

+1 @abseth-amzn I like the proposal, it makes sense to relax patch versions for plugins and longer term extensions will support running across versions including major and minor.

We have seen examples in the past where patch versions have caused problems:

As most of the comments here already put an outline, we'd like to see:

We'd like to start making changes to make this happen. Mostly everybody looks aligned with the discussion, let us know if there is any additional feedback while we start moving the needle for patch versions. cc: @dblock @reta @Bukhtawar @nknize @shwetathareja @rursprung @krishna-ggk @peternied

reta commented 9 months ago

@abseth-amzn could you please create documentation issue here [1] for 2.13.0 so we could share this new feature with plugin developers, thank you.

[1] https://github.com/opensearch-project/documentation-website

abseth-amzn commented 8 months ago

Created - https://github.com/opensearch-project/documentation-website/issues/6433