jenkinsci/plugin-pom

published incremental poms have dependencyManagement stripped causing incorrect dependency resolution

Opened this issue · 23 comments

jtnord commented

Jenkins and plugins versions report

Environment ci.jenkins.io - N/A

A plugin that is using incrementals/CD publishing has its pom mangled by the maven-flatten-plugin

this flattening removes the dependencyManagement entries as can be seen between this repository file and this published artifact

As the dependencies on kotlin are transitive they are not included in the flattened pom, but are included in the hpi.

when the plugin is depended on by another plugin the result is the dependency manamgemtn is gone so you get the transitive version of the dependencies, not the version that the plugin was built with and bundled.

This not only causes errors for consumers - it also makes the behaviour of a build different in your IDE than when deployed - as in the IDE with workspace resolution the dependencies would be correct, but once released or in CI they would be different.

Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 paths to dependency are:
+-org.jenkins-ci.plugins:github-api:1.303-999999-SNAPSHOT
ohttp-api-plugin --->    +-io.jenkins.plugins:okhttp-api:4.10.0-125.v3593b_a_f8c97b_
  +-com.squareup.okhttp3:logging-interceptor:4.10.0
    +-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10
and
+-org.jenkins-ci.plugins:github-api:1.303-999999-SNAPSHOT
ohttp-api-plugin --->  +-io.jenkins.plugins:okhttp-api:4.10.0-125.v3593b_a_f8c97b_
  +-com.squareup.okio:okio:3.3.0
    +-com.squareup.okio:okio-jvm:3.3.0
      +-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.0
]

jenkinsci/github-api-plugin#200 (review)

What Operating System are you using (both controller, and any agents involved in the problem)?

N/A

Reproduction steps

jenkinsci/github-api-plugin#200 (review)

Expected Results

no enforcer error - dependency:tree would show the same versions of kotlin both in the okhttp-api plugin and in any other consumer of it that does not itself depend on kotlin

Actual Results

dependencyManagement is stripped, downstream consumers get incorrect library versions.

Anything else?

No response

jglick commented

As the dependencies on kotlin are transitive they are not included in the flattened pom, but are included in the hpi.

So this seems wrong. Why are the dependencies not in the flattened POM?

My suspicious eye falls on gradle-module-metadata-maven-plugin but deleting that does not change anything.

jtnord commented

why are the dependencies not in the flattened POM

because the flattened pom is not promoting them from transitive dependencies to direct dependencies.

you depend on say library X. you do not list all its dependencies - you just get them.

now the flatten plugin does have a way to do this - but this breaks the maven hierachy as mentioned in the PR - as the depth first wins changes as you promote something from Depth N to depth 1 and then in other plugins it will go from N+2 to 2.

(it will also promote lots of things that really should not be promoted too - as it promotes everything from plugins that you depend on!)

jglick commented

How widespread is the issue? This case is the first I can recall being a problem so I wondering what about this plugin is different. Can it simply declare all the Kotlin dependencies as direct in its POM? Since it is importing the BOM that would not really increase maintenance cost.

Or exclude kotlin-stdlib-jdk8 from logging-interceptor? Are excludes included in the flattened POM?

jtnord commented

How widespread is the issue?

Anything that uses dependencyManagement to manage transitive dependencies. Can not answer - but upstream BOMs are getting more common - jackson2-api is /probably/ fine as jackson tends not to depend on anything 3rd party but I have not checked.

This case is the first I can recall being a problem so I wondering what about this plugin is different. Can it simply declare all the Kotlin dependencies as direct in its POM? Since it is importing the BOM that would not really increase maintenance cost.

it does in so much as if new transitive deps are added (or removed) then they can go unnoticed. (very few people checked the maven-hpi log for the WARNING bundling transitive dependency - less still when the bump if via dependabot!)

Or exclude kotlin-stdlib-jdk8 from logging-interceptor? Are excludes included in the flattened POM?

excludes can be in the flattened pom, and according to the documentation they are by default - but given we are here because the documentation lies....

jglick commented

very few people checked the maven-hpi log for the WARNING bundling transitive dependency

Unfortunately true, and I have thought of upgrading this to an error, though that would require a lot of adaptation (and, as you point out, leave behind stray deps when removed upstream).

jtnord commented

very few people checked the maven-hpi log for the WARNING bundling transitive dependency

Unfortunately true, and I have thought of upgrading this to an error, though that would require a lot of adaptation (and, as you point out, leave behind stray deps when removed upstream).

I had in mind but had never done, adding an enforcer rule that checked the contents of WEB-INF/lib where you had to specify what you expected, or at least how many things you expected which fixes the issue with stray-deps but also would require migration (or at least some form of opt-in), but we are now off topic :)

jtnord commented

this probably also needs duplicating to the main jenkins parent pom which is used by things like jenkins-test-harness which imports the jetty-bom

jtnord commented

wwwait what... https://issues.apache.org/jira/browse/MNG-7003 (https://issues.apache.org/jira/browse/MNG-5761)

is the use of a bom in Maven useless when consuming a project that uses it for transitive dependencies 😱 so even if we did do this it would not work correctly

jglick commented

apache/maven#1000 is interesting but it would be more interesting if for a given packaging you could pick a dependency manager…especially one which behaves like what RequireUpperBoundsDeps enforces, rather than this crazy tracking of dependency depth.

jtnord commented

So reconsider https://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#flattenDependencyMode ?

yup. I guess this should be safe even with a different dependency tree when having multiple plugins in your IDE vs consuming a plugin dep from the repository as you would (should) have a failure from RequireUpperBoundsDeps if there where any conflicts. and if there where no conflicts then you get the same version at any rate.

But I will need to check that it behaves correctly with plugin dependencies - I bet 💵💵 it won't which would be an issue. (all transitive dependencies from plugin X leak into plugin Y when it depends on X.

jglick commented

Interesting, adds <exclusions> automatically. Looks right to me. Again, hard to predict what the effects would be on corner cases in the future.

jtnord commented

ass I thought - flattenMode all is no good as it promotes all transitive dependencies including those from other plugins which would completely mess with plugin BOM and hightly probably the PCT, as the plugins would be updated but you would get their old deps not the new ones from the transitive plugins

https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/github-api/1.303-419.vfca_9e79c9635/

note that github-api depends on several other plugins.

short of getting depedencyManagement fixed I think the only alternative is to not use flatten at all but replace it with an hpi aware alternative that adds the transitive dependencies that would get bundled into the hpi, but nothing that would come via another plugin.

jglick commented

Maybe just work around in okhttp-api for now (by declaring at least the offending lib dep directly) and sit on this unless and until it seems like a broader problem, or some clarity is reached regarding how Maven 4 will handle these issues? My understanding of the problem in this case is that it bundles an artifact which is available via two distinct dependency trails from other bundled artifacts, and which would be using conflicting versions were it not for the imported BOM. Certainly legal but seems like a relatively unusual case.

jtnord commented

Maybe just work around in okhttp-api for now (by declaring at least the offending lib dep directly) and sit on this unless and until it seems like a broader problem, or some clarity is reached regarding how Maven 4 will handle these issues? My understanding of the problem in this case is that it bundles an artifact which is available via two distinct dependency trails from other bundled artifacts, and which would be using conflicting versions were it not for the imported BOM. Certainly legal but seems like a relatively unusual case.

my understanding is different.

any plugin that bundles a transitive dependency (not declared inline) whose version is defined by an entry in dependencyManagement (either directly or via a bom) will when it is consumed / used by any other plugin not get the version that you would get from the plugin, but whatever version is defined without the bom.

There does not need to be a conflict or different paths to the dependency - which is the very scary part.

for example just add a dependency to okhttp from a plain project and compare the dependency:tree of them

e.g.

<?xml version="1.0" encoding="UTF-8"?>
<project>
  <modelVersion>4.0.0</modelVersion>
  <groupId>foo</groupId>
  <artifactId>bar</artifactId>
  <version>1.0-SNAPSHOT</version>
  <dependencies>
    <dependency>
      <groupId>io.jenkins.plugins</groupId>
      <artifactId>okhttp-api</artifactId>
      <version>4.10.0-125.v3593b_a_f8c97b_</version>
    </dependency>
  </dependencies>
</project>

produces

[INFO] foo:bar:jar:1.0-SNAPSHOT
[INFO] \- io.jenkins.plugins:okhttp-api:jar:4.10.0-125.v3593b_a_f8c97b_:compile
[INFO]    +- com.squareup.okio:okio:jar:3.3.0:compile
[INFO]    |  \- com.squareup.okio:okio-jvm:jar:3.3.0:compile
[INFO]    |     \- org.jetbrains.kotlin:kotlin-stdlib-common:jar:1.8.0:compile
[INFO]    +- com.squareup.okhttp3:okhttp:jar:4.10.0:compile
[INFO]    |  \- org.jetbrains.kotlin:kotlin-stdlib:jar:1.6.20:compile
[INFO]    |     \- org.jetbrains:annotations:jar:13.0:compile
[INFO]    \- com.squareup.okhttp3:logging-interceptor:jar:4.10.0:compile
[INFO]       \- org.jetbrains.kotlin:kotlin-stdlib-jdk8:jar:1.6.10:compile
[INFO]          \- org.jetbrains.kotlin:kotlin-stdlib-jdk7:jar:1.6.10:compile

notice the 1.6.10 versions of kotlin from logging-interceptor 4.10.0
also note that the cersion of the common-jar is 1.8.0 and not 1.8.10

this means even if there are no conflicts if you ran mvn test or compile you are getting the lower versions not what is shipped, and so you could be linking against some signatures that have disappeared in some incompatible way that works perfectly fine everywhere outside of a Jenkins instance.

jtnord commented

so irrespective of the maven bug I believe it would be beneficial to publish the dependencyManagement section today (expanded).

This would give parity with anything produced by maven-release-plugin so having these entries would not leave us in a worse position - as we would still need to deal with any fallout should it arise.

My take on any fallout is that if it does arise whilst it could be surprise it will actually fix things and break workarounds that people have used rather than break something that was well setup and working as expected.

flattening dependencies would lead to tedious occurrences where you need to exclude a tree of dependencies (one exclude becomes N and fragile)

jglick commented

There does not need to be […] different paths to the dependency

No? Is there a realistic example of a case where there is only a single path? I think you could construct one but it would be artificial—the normal reason to bother importing a BOM when not needed to align versions of direct dependencies would be that there are different paths to some transitive dep, with different versions.

jtnord commented

there are probably lots we do not know about - as upperBounds will only tells you when there is a conflict, not that a plugin version was "downgraded"

realistic example (you do not need a BOM - just a regular dep inside a dependencyManagement section) (this is constructed - but happens - esp when a transitive dep has a vulnerability)

plugin A depends on library B
library B is rarely released but relies on library C (old version 2.03)
plugin A uses dependencyManagement to bundle newer version of C to 2.76 (does not declare direct dependency - which is correct)

plugin X depends on plugin A.

inside Jenkins plugin X will see Library C at the newer version 2.76 when building and running tests it will see 2.03

There is a single path to the dependency and as dependencyManagement is ignored plugin X will get the dependency version of C that is declared in library B's pom.