DR-3404: Spring Boot 3 upgrade + More #1581
Merged
Conversation
This file contains 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
snf2ye
force-pushed
the
sh/dr-3404-spring-boot-3-upgrade
branch
3 times, most recently
from
b3e335b to
e02378a
Compare
snf2ye
force-pushed
the
sh/dr-3404-spring-boot-3-upgrade
branch
8 times, most recently
from
dbd1b0a to
e7a3040
Compare
snf2ye
commented
Feb 6, 2024
Comment on lines
-181
to
-179
| // Fix for CVE-2022-25857 which identifies a vulnerability in snake-yaml 1.30 | ||
| // This override can likely be removed or bumped to 2.0 when we upgrade to spring boot 3 | ||
| ext['snakeyaml.version'] = '1.33' |
There was a problem hiding this comment.
snakeyaml is now at 2.2
| implementation 'jakarta.validation:jakarta.validation-api' | ||
| implementation 'jakarta.annotation:jakarta.annotation-api' | ||
|
|
||
| implementation 'org.apache.httpcomponents.client5:httpclient5' |
There was a problem hiding this comment.
Why did we need to add this?
There was a problem hiding this comment.
There is a note about this in the description:
Apache HttpClient in RestTemplate
Support for Apache HttpClient has been spring-projects/spring-framework#28925, immediately replaced by org.apache.httpcomponents.client5:httpclient5 (note: this dependency has a different groupId). If you are noticing issues with HTTP client behavior, it could be that RestTemplate is falling back to the JDK client. org.apache.httpcomponents:httpclient can be brought transitively by other dependencies, so your application might rely on this dependency without declaring it.
Comment on lines
+44
to
53
| var responseBody = body; | ||
| // Without specifically grabbing the exception message for validation errors, | ||
| // TDR will swallow the error and return a 400 with no error message. | ||
| if (responseBody == null || status.isSameCodeAs(HttpStatus.BAD_REQUEST)) { | ||
| responseBody = | ||
| new ErrorModel() | ||
| .message(status + " - see error details") | ||
| .addErrorDetailItem(ex.getMessage()); | ||
| } | ||
|
|
||
| return new ResponseEntity<>(responseBody, headers, status); |
There was a problem hiding this comment.
With these changes, a ProblemDetail is returned for the response body, making the responseBody no longer null for bad request exceptions. So, I've added a special case for BAD_REQUESTS to continue the work originally added in this PR: #869
snf2ye
force-pushed
the
sh/dr-3404-spring-boot-3-upgrade
branch
5 times, most recently
from
1e1bd77 to
9dd9e62
Compare
snf2ye
requested review from
nmalfroy,
samanehsan and
okotsopoulos
as code owners
snf2ye
changed the title
snf2ye
force-pushed
the
sh/dr-3404-spring-boot-3-upgrade
branch
5 times, most recently
from
1666a5b to
604677e
Compare
pshapiro4broad
approved these changes
Feb 9, 2024
| @@ -736,15 +735,13 @@ public static ErrorReportException convertSAMExToDataRepoEx(final ApiException s | |||
| logger.warn("SAM client exception details: {}", samEx.getResponseBody()); | |||
|
|
|||
| // Sometimes the sam message is buried several levels down inside of the error report object. | |||
| // If we find an empty message then we try to deserialize the error report and use that message. | |||
| String message = samEx.getMessage(); | |||
There was a problem hiding this comment.
Right now this is overwritten unless an exception is thrown, I think it would be clearer to only assign it once instead. Something like
final String message;
try {
...
} catch (...) {
message = Objects.requireNonNullElse(samEx.getMessage(), "SAM client exception");
}
okotsopoulos
approved these changes
Feb 9, 2024
| @@ -273,17 +257,7 @@ public static void deleteRandomPod() throws ApiException, IOException { | |||
| } | |||
|
|
|||
| public static void deletePod(String podNameToDelete) throws ApiException, IOException { | |||
| // known issue with java api "deleteNamespacedPod()" endpoint | |||
src/test/java/bio/terra/service/auth/iam/sam/SamApiServiceTest.java
Outdated
Show resolved
Hide resolved
okotsopoulos
reviewed
Feb 9, 2024
Slight change in status message from sam
Due to changes to gradle formatting
…requests The response body used to be null, but now it is returning a Problem detail that hides the actual exception message
… for datarepo-client
- Required upgrading to k8 20.0.0 to support snakeyaml 2.2 - We could now additionally upgrade the kubernetes client in the main service
tracing/stairway env variables Add otel instrumentation for SAM Following the example in BPM https://github.com/DataBiosphere/terra-billing-profile-manager/blob/9074f11e45187f18000b024208d2bb118874e187/service/src/main/java/bio/terra/profile/service/iam/SamService.java update to otel noop by default we'll turn off tracing with open telemetry
This appears to now implement strict stubbing
Turns out the python client publish GHA has been failing for 3+ weeks, so this change is not tested. A separate ticket has been added to handle this fix. https://broadworkbench.atlassian.net/browse/DR-3455
From @okotsopoulos - I'll flag that this latest version of ECM client includes this change: DataBiosphere/terra-external-credentials-manager#141 Which means that we can remove this 2 year old stopgap (a unit test in EcmServiceTest would need to be updated): String passport = oidcApiService.getOidcApi(userReq).getProviderPassport(RAS_PROVIDER); // Passports returned by OidcApi have a bug in their formatting: // double quotes must be stripped if passing back to PassportApi for validation, // otherwise the passport will not be considered valid JWT. // This stopgap can be removed when the client is fixed: // https://broadworkbench.atlassian.net/browse/ID-128 return StringUtils.strip(passport, "\"");
…boot 3 - Fixes swagger-ui wonkiness - Can remove swagger-ui-dist handlers - Can still keep the swagger-ui.html path - more details: https://springdoc.org/
upgrading causes ui bugs; switching to spring docs points to a different url (instead of swagger-ui.html, it's swagger-ui/index.html. There is a redirect but we'd also have to make proxy changes to allow the new url to pass through) Revert "Using springdoc seems to be the recommended path forward with spring boot 3"
snf2ye
force-pushed
the
sh/dr-3404-spring-boot-3-upgrade
branch
from
a2ad0d5 to
462ec39
Compare
|
This was referenced Mar 4, 2024
snf2ye
added a commit
to DataBiosphere/terra-data-catalog
that referenced
this pull request
Mar 4, 2024
) https://broadworkbench.atlassian.net/browse/DC-839 _______ With the DataBiosphere/jade-data-repo#1581 for data repo, we're switching back to only publishing to one Jakarta backed client. This PR moves your references to the jakarta client back to the main data repo client and bumps to the latest version. This is an effort to alleviate any confusion around upgrading past the last supported datarepo-jakarta-client version.
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.
https://broadworkbench.atlassian.net/browse/DC-816
Remaining TODOs
Upgrades
Spring Boot
2.7.18->3.2.2logback-core&logback-classic- Effective upgrade from1.2.13->1.4.11org.codehaus.janino:janino- stays3.1.11org.springframework:spring-jdbc- Effective upgrade from5.1.9->6.0.14servlet jstl- Effective upgrade from1.2->3.0.0org.glassfish.jersey- Effective upgrade from2.30.1->3.1.3jersey-hk2- Same version -4.10.0jackson-core&jackson-databind- Effective upgrade from2.13.2->2.14.3webjars-locator-core- Effective upgrade0.46->0.521.0.11-RELEASE->1.1.41.33->2.2implementation 'org.apache.httpcomponents.client5:httpclient5'(note as to why below)Gradle
7.3->8.5; Upgrade some of the formatting in our build.gradle2.1.1->2.2.10.8.7->0.8.9Swagger
2.4.27->3.0.522.1.12->2.2.203.0.47->3.0.51Spring cloud GCP starter logging
Spring cloud sleuth replaced by micrometer
Azure upgrades
This would be a very reasonable things to break into another PR - they're just listed as having CVE vulnerabilities. All of these are minor or patch level ugprades.
Upgraded:
Test dependencies
Terra upgrades
0.0.89->0.1.102.13:0.1-ae94a59->2.13:0.1-a91acd0- This git hash maps to upgrading from v0.0.119 to v0.0.186 (And it appears I could remove the exclusion of the sam reference from TCL)0.0.78->0.0.801.0.4->1.0.110.72.0->1.3.00.4.3->0.198.42Needed Changes
Update from javax to jakarta
Added
implementation 'org.apache.httpcomponents.client5:httpclient5'Move back to publishing single, jakarta-based client: datarepo-client
With this change, we will move back to only publishing a single client library, now Jakarta-based: datarepo-client. This change will be marked with a major version change - v2.0.0. This follows in the steps of ECM.
This means that any service who wants the latest datarepo client library will also need to move to Spring Boot 3. Currently, these services include: Terra CLI (no longer under our ownership), datarepo cli (appears to no longer be maintained), java template project (which will want to upgrade to jakarta to model best practice), cbas, and a number of POCs.
To help alleviate confusion, I'll put up PRs against the other repos in DataBiosphere that are referencing the datarepo-jakarta-client. They'll instead point to the datarepo-client:2.0.0 or later versions. [https://broadworkbench.atlassian.net/browse/DC-839]
Open Telemetry (otel)
By upgrading terra-common-lib, we automatically included the tracing work. It would error without adding the opentelemetry imports. So, why not get it working? I followed the instructions in Doug's DSP blog. I think it's helpful that integrate with the stairway trace hook. Otherwise, I added a sample of
@WithSpantags focusing on the snapshot dao, where we have found some high db load, and integration with other APIs (TPS) to see what was helpful. I support a more complete look into otel given an initial reading on what's helpful with these changes.Other required changes:
Known Issues:
Example warning message:
Example trace (with many more details if you click on one the dots) 🎉
Migrate datarepo-clienttests to use jakarta client
The main change required here was to upgrade the kubernetes client.
Changes in Error Messages
As you'll see in the commits, there were some changes in order to still correctly surface error messages. When we were expecting error details to be empty, they actually returned unhelpful ProblemDetail models. This is one thing for us to be look out for as we move forward with this change -- are our errors messages correct?
Resulting Tickets