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

Update plugins related to 2022-10-19 security advisory; add ionicons-api #1507

Merged
merged 7 commits into from
Oct 26, 2022

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Oct 25, 2022

Replaces #1506, #1504, #1501, and #1496. We have to add ionicons-api, whose oldest version requires Jenkins 2.346.1, and I am not sure how we exclude it from bom-2.332.x and bom-2.319.x (if it is even possible (MNG-5600), and if we even care).

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

…2.x for compatibility with workflow-cps and pipeline-groovy-lib
Comment on lines 127 to 130
<dependency>
<groupId>io.jenkins.plugins</groupId>
<artifactId>ionicons-api</artifactId>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to fail prep in 2.332.x and 2.319.x lines. Maybe you can add it to the 2.346.x and 2.361.x profiles? But then we would also need to introduce a weekly profile and make sure prep.sh calls that, so a bit bigger change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at README.md, I think we can just delete the dependency here, since ionicons-api will be picked up transitively via workflow-cps on the appropriate LTS lines. Then we just need to tweak check.groovy to accept the fact that ionicons-api is part of dependencyManagement in 2.332.x and 2.319.x but is not used by the plugin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 1e8f8b6.

@jglick
Copy link
Member

jglick commented Oct 25, 2022

See background in #1159, #1160, etc. I need to think about whether the limitation can be solved. Ideas welcome obviously.

…check.groovy to allow it to be unused when testing BOM on old LTS lines
@@ -7,7 +7,8 @@ assert artifactMap['junit:junit'] == project.artifactMap['junit:junit']
def managedPluginDeps = managedDeps.collect {stripAllButGA(it)}.grep { ga ->
def art = artifactMap[ga]
if (art == null) {
if (ga.contains('.plugins')) { // TODO without an Artifact, we have no reliable way of checking whether it is actually a plugin
if (ga.contains('.plugins') // TODO without an Artifact, we have no reliable way of checking whether it is actually a plugin
&& !(ga == 'io.jenkins.plugins:ionicons-api' && settings.activeProfiles.any {it ==~ /^2[.](332|319)[.]x$/})) { // TODO: Remove once 2.332.x is no longer part of the BOM (or if MNG-5600 is fixed and we can exclude this dependency in the BOM for old LTS lines)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine for now. If we need to do this sort of thing regularly we should probably introduce a text file with a list of plugins limited to newer lines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And now I am curious whether we can do the same for example for instance-identity.)

@@ -7,7 +7,8 @@ assert artifactMap['junit:junit'] == project.artifactMap['junit:junit']
def managedPluginDeps = managedDeps.collect {stripAllButGA(it)}.grep { ga ->
def art = artifactMap[ga]
if (art == null) {
if (ga.contains('.plugins')) { // TODO without an Artifact, we have no reliable way of checking whether it is actually a plugin
if (ga.contains('.plugins') // TODO without an Artifact, we have no reliable way of checking whether it is actually a plugin
&& !(ga == 'io.jenkins.plugins:ionicons-api' && settings.activeProfiles.any {it ==~ /^2[.](332|319)[.]x$/})) { // TODO: Remove once 2.332.x is no longer part of the BOM (or if MNG-5600 is fixed and we can exclude this dependency in the BOM for old LTS lines)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or simply

Suggested change
&& !(ga == 'io.jenkins.plugins:ionicons-api' && settings.activeProfiles.any {it ==~ /^2[.](332|319)[.]x$/})) { // TODO: Remove once 2.332.x is no longer part of the BOM (or if MNG-5600 is fixed and we can exclude this dependency in the BOM for old LTS lines)
&& ga != 'io.jenkins.plugins:ionicons-api') { // TODO: Remove once 2.332.x is no longer part of the BOM (or if MNG-5600 is fixed and we can exclude this dependency in the BOM for old LTS lines)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I added this defensively before I realized that there was a transitive dependency on ionicons-api because I wanted to make sure that we still verified its inclusion in sample on the recent LTS lines. If you are confident that it will be a long-term dependency of various plugins used by sample, then simplifying this check is probably fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confident that it will be a long-term dependency

Not really, but at some point we will drop 2.332.x and be able to delete this exemption as well, so I just thought we could simplify a bit. OTOH the check as written here does textually mention the old line names, making it more likely to appear in a text search when dropping old lines.

@dwnusbaum
Copy link
Member Author

The pipeline-model-definition test failures look legitimately related to pipeline-input-step: "java.lang.IllegalArgumentException: InputStep id is required to be URL safe, but the provided id Matrix - AXIS_VALUE = 'A' is not safe". The reason is that pipeline-model-definition is locked to an old version in 2.319.x, so we'll need to do the same for pipeline-input-step.

…r compatibility with pipeline-model-definition
@jglick jglick changed the title Update plugins related to 2022-10-19 security advisory Update plugins related to 2022-10-19 security advisory; add ionicons-api Oct 25, 2022
@jglick jglick added the enhancement New feature or request label Oct 25, 2022
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be taken out of draft I think; is there any reason for reservation here?

@dwnusbaum
Copy link
Member Author

Can be taken out of draft I think; is there any reason for reservation here?

Should be fine at this point. Previously I was not sure what to do about ionicons-api.

@dwnusbaum dwnusbaum marked this pull request as ready for review October 25, 2022 17:42
@jglick jglick enabled auto-merge (squash) October 25, 2022 18:12
@dwnusbaum
Copy link
Member Author

dwnusbaum commented Oct 25, 2022

Looks like the PCT failed while trying to test data-tables-api with this error:

[2022-10-25T18:25:46.325Z] [ERROR] Error executing Maven.
[2022-10-25T18:25:46.325Z] org.apache.maven.cli.internal.ExtensionResolutionException: Extension io.jenkins.tools.incrementals:git-changelist-maven-extension:1.4 or one of its dependencies could not be resolved: Could not transfer artifact org.apache.maven:maven-builder-support:jar:3.5.0 from/to central (https://repo.maven.apache.org/maven2): transfer failed for https://repo.maven.apache.org/maven2/org/apache/maven/maven-builder-support/3.5.0/maven-builder-support-3.5.0.jar
...
[2022-10-25T18:25:46.326Z] Caused by: org.apache.maven.wagon.providers.http.httpclient.conn.ConnectTimeoutException: Connect to repo.maven.apache.org:443 [repo.maven.apache.org/146.75.116.215] failed: Connection timed out

At a glance, it does not seem related to my changes. Should we be connecting to https://repo.jenkins-ci.org/ instead?

@jglick
Copy link
Member

jglick commented Oct 25, 2022

We should, yes. That plugin’s POM does specify repositories so I am not sure why it would be contacting Central.

@dwnusbaum
Copy link
Member Author

Does Maven use a different context when looking up the dependencies for Maven plugins or something? Do we need repositories or pluginRepositories in https://github.com/jenkinsci/incrementals-tools somewhere?

@jglick
Copy link
Member

jglick commented Oct 25, 2022

Oh sorry I missed that it was referring to the Maven extension. Yes this is loaded prior to parsing the POM, so will go to Central. Probably we could override that with a custom settings file. #1508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants