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

Upgrade to latest Guava (v30) and Solr (v8) to resolve security alerts #3020

Merged
merged 14 commits into from
Feb 25, 2021

Conversation

tdonohue
Copy link
Member

@tdonohue tdonohue commented Oct 21, 2020

References

This PR resolves 3 security alerts by upgrading Guava:

This PR also moves us off an EOL version of Solr, resolving https://jira.lyrasis.org/browse/DS-4497

Description

Upgrades the following dependencies:

  • Upgrades guava from v19.0 to v30.0-jre. This resolves the security alerts listed above.
  • Upgrades Solr from v7.3.1 to v8.7.0. This was necessitated by the guava upgrade, as I was unable to get the latest version of guava to work with Solr v7 (because of Java API changes in guava).
  • Minor upgrade of Jetty from v9.4.17.v20190418 to 9.4.35.v20201120 to align with the above upgrades and fix GitHub security alert
  • Other minor dependency realignment in POMs to resolve dependency convergence issues.

Code changes required by Guava upgrade:

  • Removes & replaces outdated/obsolete Lyncode builder-commons & test-support dependencies (from dspace-oai module classes). These old Lyncode dependencies brought in a very old version of guava & were causing a headache in terms of dependency convergence. It was easiest to completely remove them & replace them with updated code that did the same thing.
    • In order to determine what the Lyncode builders were doing, I looked at the source code at https://github.com/lyncode/builder-commons and found a modern replacement (in some cases using Guava directly & in others just using standard Java 11 code).
    • The Lyncode test-support classes were only used in PipelineTest, and I found replacement code in our existing QDCXslTest class which does a similar test without using Lyncode's test-support.

Code changes required by Solr upgrade:

  • Updated SolrSearchCore and SolrServiceImpl to use GET requests when the EmbeddedSolrServer is in use (during integration test execution).
  • Updated IndexFactoryImpl to no longer use ContentStreamUpdateRequest for indexing full text. Instead, it has been refactored to use Apache Tika directly. Also introduced a new discovery.solr.fulltext.charLimit configuration to allow for more control over Tika's default character limit indexing settings.
  • Minor updates to existing solrconfig.xml files to ensure they are aligned with Solr v8.7.0 default settings. (Some whitespace changes occurred here too that were automated by my IDE)
  • Updated Docker to support Solr v8 (and the new directory structure in Solr v8 docker images). Also required moving to using the official Solr Docker image with Docker Compose, instead of creating our own custom image.

Additional minor changes:

  • Renamed DatabaseRegistryUpdater to RegistryUpdater. This ensures it comes alphabetically after the GroupServiceInitializer and therefore is always run after Groups are created (as Flyway defaults to running Callbacks alphabetically). I stumbled on this problem when starting with a fresh database...as the RegistryUpdater depends on the GroupServiceInitializer.

Instructions for Reviewers

  • Review code
  • Verify tests still succeed (they do)
  • Test OAI-PMH interface & reindexing.
  • Test Solr indexing (./dspace index-discovery -b)
  • Test search/browse functionality.

(NOTE: I've tested & verified all of the above and found no remaining issues.)

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
  • My PR passes Checkstyle validation based on the Code Style Guide.
  • My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
  • My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

@tdonohue tdonohue added bug code task Code cleanup task dependencies Pull requests that update a dependency file labels Oct 21, 2020
@tdonohue tdonohue added this to the 7.0beta5 milestone Oct 21, 2020
@tdonohue tdonohue self-assigned this Oct 21, 2020
@tdonohue tdonohue added the work in progress PR is still being worked on & is not currently ready for review label Oct 21, 2020
@tdonohue
Copy link
Member Author

tdonohue commented Oct 21, 2020

Strangely, in this (draft) PR, the ITs are failing because of this error: #2989 (at least that's the error logged in the test logs over and over again for each failing test). The changes in this PR are entirely unrelated to that issue, but perhaps that error was somehow being "swallowed" prior to the changes in this PR. Either that, or maybe a different underlying error is being hidden/swallowed while only the ClassCastException is shown.

In any case, this PR might be dependent on first finding a fix for #2989.

This PR works in manual testing. Server webapp works. OAI-PMH works, and ./dspace oai commands work. It's just the ITs which are having odd issues.

@tdonohue tdonohue changed the title Upgrade to latest guava to resolve security alerts Upgrade to latest Guava (v30) and Solr (v8) to resolve security alerts Jan 22, 2021
@tdonohue tdonohue added component: Discovery Related to Discovery search or browse system interface: OAI-PMH Related to the OAI-PMH interface (dspace-oai module) and removed work in progress PR is still being worked on & is not currently ready for review labels Jan 22, 2021
@tdonohue tdonohue marked this pull request as ready for review January 22, 2021 19:23
@tdonohue
Copy link
Member Author

This PR is now ready for review/testing. I've scheduled it for Beta 5 in order to ensure it can be added prior to Testathon & 7.0 final.

@tdonohue tdonohue added the backend: Solr Related to the Solr index label Jan 25, 2021
@tdonohue
Copy link
Member Author

tdonohue commented Feb 1, 2021

@mwoodiupui : Would you be interested in giving this a review and/or test? I'd appreciate your feedback here since you did the last Solr upgrade. In addition, your feedback would be useful in the follow-up PR #3126 (where I've attempted to cleanup/remove old Solr configs that seem unused/unnecessary)

Copy link
Member

@mwoodiupui mwoodiupui left a comment

Choose a reason for hiding this comment

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

+1 by inspection. This all looks reasonable to me. (I did not give much attention to the Docker portions since I have so little contact with Docker.)

@tdonohue tdonohue force-pushed the upgrade_guava branch 2 times, most recently from fa7f446 to 48f628f Compare February 4, 2021 22:33
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

Hi @tdonohue thanks for this important work. Everything looks good to me so I'm +1 with merging it.
I would suggest to update to version 8.8.1 that is the latest stable and it seems perfectly compatible with what we already have (I have tested this PR against a local 8.8.1 installation without noting major issues). The switch to 8.8.1 can be done in the second PR #3126

@github-actions github-actions bot added the merge conflict PR has a merge conflict that needs resolution label Feb 24, 2021
…upServiceInitializer as Flyway runs callbacks alphabetically
…ng EmbeddedSolrServer always uses GET instead of POST
… Use Tika directly for parsing instead of ContentStreamUpdateRequest (which results in "URI is too large >8192" errors in Solr v8)
…Exception warnings from Spring Boot during startup
…r v8, and all the examples show to use docker-compose directly with official image.
@tdonohue tdonohue removed the merge conflict PR has a merge conflict that needs resolution label Feb 25, 2021
@tdonohue tdonohue merged commit 5d8285d into DSpace:main Feb 25, 2021
@tdonohue tdonohue deleted the upgrade_guava branch February 25, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Solr Related to the Solr index bug code task Code cleanup task component: Discovery Related to Discovery search or browse system dependencies Pull requests that update a dependency file interface: OAI-PMH Related to the OAI-PMH interface (dspace-oai module)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants