[DR-3398] [ID-948] Upgrade to Spring Boot 3.1.6, TCL 0.1.9 #161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll just publish a Jakarta-based library under the original name.
And other upgrades too. Take advantage of ugraded dependency management plug-in: don't need to specify Spring Boot dependency versions. Removed pins applied previously for security vulnerabilities which are no longer needed. Generators now use jakarta.
@Traced -> @WithSpan Added needed application configuration changes, and disabled OTEL for tests
From release notes (version 9.0, 2020-09-06): Replaces all net.minidev.json.JSONObject method arguments and return types with java.util.Map<String,Object>
Without it, the test tries to instantiate OTEL beans (unsuccessfully)
okotsopoulos
changed the title
Due to the client library no longer being compatible with Spring 2.
dvoet
approved these changes
Dec 14, 2023
Comment on lines
26
to
+28
| invokerPackage: "${artifactGroup}.client", | ||
| dateLibrary : 'java11' | ||
| dateLibrary : 'java11', | ||
| jakarta : true |
There was a problem hiding this comment.
I didn't say this in the last review but it has been nagging me. It might be worth switching from library = 'resttemplate' above to library = 'jersey2' to be like other service. Then cross service tracing can be setup like this. Alternatively we could implement a ClientHttpRequestInterceptor that does tracing for this kind of client lib.
There was a problem hiding this comment.
Good shout, I can play with this.
Do you have a sense of what changes would be required to the services interacting with the client library?
There was a problem hiding this comment.
I think I'd like to work on this separately in its own ticket, so will merge this PR in without it… I couldn't get it to work out of the box but I may be missing something simple.
Ghost-in-a-Jar
approved these changes
Dec 14, 2023
service/build.gradle
Outdated
| @@ -53,6 +53,18 @@ dependencies { | |||
| } | |||
| // https://mvnrepository.com/artifact/org.mockito/mockito-core | |||
| testImplementation group: 'org.mockito', name: 'mockito-core', version: '4.4.0' | |||
|
|
|||
| // OpenTelemetry | |||
| var openTelemetryVersion = '1.31.0' | |||
There was a problem hiding this comment.
After seeing this in different PRs I'm wondering if we should expose this dependency from tcl, with the idea that all terra services should be using tracing and opentelemetry.
There was a problem hiding this comment.
I'm glad you brought this up, I was wondering about this too. @dvoet , is there a reason why this block isn't handled by default in TCL?
Allow google-cloud-pubsub, google-cloud-kms to get their versions from the parent libraries-bom Upgraded libraries-bom to latest version Found that org.springframework.cloud:spring-cloud-gcp-starter-logging has been usurped by a dep of the same name in com.google.cloud
This was causing tests to fail in CI, but not locally: that's because I override the log appender locally to use Console-Standard.
okotsopoulos
changed the title
|
okotsopoulos
added a commit
to DataBiosphere/terra-drs-hub
that referenced
this pull request
Dec 14, 2023
Created now that ECM is fully on Spring 3 in DataBiosphere/terra-external-credentials-manager#161
okotsopoulos
added a commit
to DataBiosphere/terra-drs-hub
that referenced
this pull request
Dec 15, 2023
…75 -> 0.1.9 (#97) * DR-3393 Upgrade Spring 2.7.4 -> 3.1.2, TCL 0.0.75 -> 0.1.7 Which upgrades transitive deps needed to resolve security tickets DR-3385, DR-3391 (and remove previous version pins for past security upgrades). * In Spring 3, javax -> jakarta * Migrate Apache HttpClient 4->5 Using https://hc.apache.org/httpcomponents-client-5.3.x/migration-guide/preparation.html as a helper. Most old interfaces / classes resolved automatically after swapping org.apache.http imports with org.apache.hc.httpclient5, but SSL settings required extra attention. We now set the SSL socket factory on the PoolingConnectionManager which is passed to our CloseableHttpClient. * Logback >1.3 no longer allows nesting if elts within appender, logger, root elts Fixed our configuration according to their guide: https://logback.qos.ch/codes.html#nested_if_element * Needed OTEL set-up for DrsHub to start Looking at recent PRs for other services who also added these imports and similar config * Spotbugs NP_NULL_PARAM_DEREF I don't think that Spotbugs is being very smart here: DrsMetadata.Builder() can accept a null DrsResponse, but there's no harm in moving our call to the setter within our existing null check. * Disable OTEL in tests Without this, tests extending BaseTest were failing with errors like: Invalid bean definition with name 'otelWebMvcFilter' defined in class path resource * DR-3385 CVE-2023-6378 Upgrade logback-classic, logback-core to 1.4.14 It was not enough that we updated TCL, since we exclude its spring dependency. Spring boot hasn't yet upgraded its logback versions, so for the time being we can pull in non-vulnerable versions ourselves. Added a note to check back on this following the next Spring boot release. While here, realized that I wasn't pulling in the latest 3.1.x Spring boot version, so brought it up to 3.1.6. * Delegate Spring Boot dependency management to plugin Upgraded io.spring.dependency-management plugin to the latest version. Happily, we now only set our Spring Boot version in one place. * Gradle convention: only use double quotes for interpolated strings * Update GlobalExceptionHandler.buildErrorReport signature This simplifies our call to it. * Upgrade Sam client library to get Jakarta client While here, upgraded to latest TCL and ECM Client... But sadly, ECM does not currently publish a Jakarta client. Until they do, DrsHub cannot upgrade to Spring 3. Right now, I suspect this incompatibility is driving errors like the following when attempting to fetch a user's passport from ECM: Caused by: java.lang.NoSuchMethodError: 'org.springframework.http.HttpStatus org.springframework.http.ResponseEntity.getStatusCode()' at bio.terra.externalcreds.client.ApiClient.invokeAPI(ApiClient.java:500) at bio.terra.externalcreds.api.OidcApi.getProviderPassportWithHttpInfo(OidcApi.java:314) * Use new Jakarta-based ECM client library Added in DataBiosphere/terra-external-credentials-manager#160 * Update google dependencies Where possible, allow dependencies to get their versions from the parent libraries-bom Found that org.springframework.cloud:spring-cloud-gcp-starter-logging has been usurped by a dep of the same name in com.google.cloud * Upgrade OTEL, correct its configuration * Pull in new ECM Jakarta-based library Created now that ECM is fully on Spring 3 in DataBiosphere/terra-external-credentials-manager#161 * Designate this release as #major While DrsHub does not publish a client library, the significance of these upgrades warrants a major version bump rather than the usual minor version bump. * Swagger UI drive-by config: show request duration, enable 'try it out' by default
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira:
https://broadworkbench.atlassian.net/browse/DR-3398
https://broadworkbench.atlassian.net/browse/ID-948
What:
In a major version release:
bio.terra:externalcreds-client-resttemplateWhy:
Yesterday I merged a PR to make ECM publish a Jakarta-based client library in addition to the existing Javax-based client library. At that point I didn't pursue a full upgrade to Spring Boot 3: #160
Initially, testing the new client library with DrsHub on Spring Boot 3 seemed to work, but I was only able to test DRS URI resolution for a passportable snapshot when the caller held no passport.
After merging, I was able to re-link my RAS account in Terra Dev and verified that I now held a passport. When retesting DRS URI resolution for a passportable snapshot, I was running into client incompatibility errors similar to the ones I saw before.
Instead of figuring out how to resolve issues specific to the second client library, I instead decided that it would be more worthwhile to pursue the upgrade and move back to a single client library. Publishing the new library locally and pulling it into my local DrsHub now allows DRS URI resolution for a passportable snapshot to proceed when the caller holds a passport.
Side benefits: these upgrades allowed me to clean up package pinning related to past security vulnerabilities, and more completely leverage the dependency management plug-in. Identiteam may like to cross-check any open security vulnerability tickets on their board for ECM to see if this PR resolves them.
logback-classic,logback-coreto 1.4.14How:
Following precedence set by prior Spring 3 upgrades and https://broadworkbench.atlassian.net/wiki/spaces/DSPBLOG/blog/2023/11/16/2906521657/The+Great+Spring+Boot+3+Yak+Shaving+Expedition (thanks, @dvoet!)
To review, you may prefer to go commit-by-commit: I endeavored to split them up logically (e.g. the "javax -> jakarta" commit touches many files but they're all package name changes).