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

[peternied]

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]

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]

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

[dblock]

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]

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

[dblock]

@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]

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]

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

[nknize]

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]

@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.
• When OpenSearch core has more recent version than the plugin, we could warn the user: at least, it is going to be her conscious decisions to make. But the installation won't fail.
• But as a guardrail, we could introduce a "minimum required plugin compatibility level", to be the minimal supported version of the plugin for this particular release (fe for 1.2.x it is going to be 1.2.0 by default). In case of CVEs or any force major situation, this compatibility level could be bumped and in this case, the plugin installation will fail (with the informative message "why", fe "CVE-XXX ...").

What do you think guys?

[dblock]

@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]

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]

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: [opensearch-plugin] Cannot install old patch version of plugins on newer opensearch builds  #1707 (comment))
2. Modify the logic of compatibility verifcation in OpenSearch to accept any plugins that built for the same OpenSearch major and minor version.
   (origin: [opensearch-plugin] Cannot install old patch version of plugins on newer opensearch builds  #1707 (comment))

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:

• Regarding @reta’s idea,
  It sounds reasonable for OpenSearch core to accept any OpenSearch plugins that built for the same major and minor version (such as 1.2.x), because according to the semver specification - if only backwards compatible bug fixes are introduced, patch version MUST be incremented. I have little idea what edge cases could cause incompatibility if the plugins are built for a lower patch of OpenSearch core.
  But I got a confusion, is the proposed “minimum required plugin compatibility level" will be a property of OpenSeach core that controls all the plugins?
  I assume it will check this property of a plugin:
  

  OpenSearch/server/src/main/java/org/opensearch/plugins/PluginInfo.java

  Line 346 in 3f210dc

  * The version of OpenSearch the plugin was built for.

  Looks like it’s not necessary, because a plugin don’t need to re-build and bump version number if it doesn’t impact by a CVE.

• Regarding @nknize’s idea,
  I support the idea to add logic in the script of building OpenSearch plugins to accept “semver range syntax”, It will be a exciting change, as it will speed up the plugin release process a lot. He has mentioned the 2 benefits:

1. The patch releases of the plugin and OpenSearch core can be proceed independently.
2. If a plugin has got a dependent plugin, it doesn’t need to wait for its dependency bumping the OpenSearch version, when supporting a new version of OpenSearch.

[AmiStrn]

+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]

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]

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

[Bukhtawar]

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]

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 - #2868

[Bukhtawar]

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]

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 #1707 (comment) .

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]

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]

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]

+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:

• 1.3 branch version increment of for jackson to match with core security#2286
• [1.3] Bump org.bouncycastle:bcprov-jdk15on from 1.67 to 1.70 security#2422

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

• Plugin able to define range of patch version compatibility i.e >2.5.1, <2.5.7
• Plugin devs have a way to test against matrix of OpenSearch versions for unit (compile time) and integration tests (runtime)

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]

@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]

Created - opensearch-project/documentation-website#6433