[WOR-1560] Upgrade to TCL 1.0.10 for MDC-aware Stairway #424
Conversation
MdcHook is greatly reduced and renamed to StairwayLoggingHook: it now only logs at notable state transitions.
DataBiosphere/terra-common-lib#149 pulled in a version of the SamClient which now formats its ApiException message instead of returning the raw message. We needed to update our mocks accordingly, so that TCL's SamExceptionFactory could extract the message as expected.
MockMvc doesn't provide easy ways of comparing two OffsetDateTimes. This test was failing in CI due to mismatched granularity in our datetime strings, despite the datetimes being equal.
buildSrc/src/main/groovy/bio.terra.landingzone.java-common-conventions.gradle
Outdated
Show resolved
Hide resolved
This TCL upgrade includes an update to a vulnerable transitive dependency. We discovered when working in WSM that the vast majority of our defined OTEL deps are unnecessary. But, in getting rid of them I found that I needed to explicitly add in the guava dependency for TCL to work, we previously got it transitively through an otherwise unnecessary OTEL dependency.
okotsopoulos
changed the title
okotsopoulos
changed the title
The bump to 1.1.0 will be TCL's k8s client upgrade, and we want to upgrade LZS' k8s client at the same time. Reorganized dependency conventions so that Spring Boot dependency pins are reused when we bring in the Spring Boot and Spring dependency manager plug-ins, which is necessary so that integration tests running out of the library project start up. Note that these changes still upgrade LZS' vulnerable transitive dependency jose4j past its vulnerability, but this is accomplished by TCL 1.0.10 having pinned it. Later version 1.1.0 will remove the pin and address the vulnerability at the root by upgrading k8s client.
We were using Spring Boot and Spring dependency management plug-ins already, without benefitting from our Spring Boot dependency pins. If we're addressing a vulnerability in a Spring Boot dependency by pinning it, we should make sure it's pinned everywhere (e.g. postgresql).
This reverts commit b90f589.
okotsopoulos
requested review from
blakery,
aherbst-broad,
sergiygetlin and
cahrens
and removed request for
a team,
blakery and
aherbst-broad
library/src/main/java/bio/terra/landingzone/stairway/common/utils/StairwayLoggingHook.java
Show resolved
Hide resolved
library/src/main/java/bio/terra/landingzone/stairway/common/exception/MDCHandlingException.java
Show resolved
Hide resolved
| implementation "com.google.cloud.opentelemetry:exporter-trace:0.25.2" | ||
|
|
||
| implementation ('bio.terra:terra-common-lib:0.1.6-SNAPSHOT'){ | ||
| implementation 'com.google.guava:guava:33.1.0-jre' // Needed for TCL |
There was a problem hiding this comment.
If TCL needs guava, why doesn't it include it?
There was a problem hiding this comment.
Thanks for asking. My theory (and I wish I were more solid on this) is that TCL exposes guava as an api dependency but does not explicitly version it, because TCL derives its version from Google's libraries-bom:
But LZS does not pull in Google's libraries-bom (TCL does not expose it as an api dependency) so it can't find guava:
I am wondering if TCL should instead expose Google's libraries-bom as an api dependency, or if not, if LZS should instead pull in Google's libraries-bom itself.
This has been a question that's come up in other ways for me recently (exposing an api dependency whose version is derived from a BOM without exposing the BOM itself) so I'm going to raise it to a wider audience in #dsp-engineering.
There was a problem hiding this comment.
There was a problem hiding this comment.
For now, I'm inclined to pull in Google's libraries-bom into LZS instead with a more expansive comment. Hopefully we can change this upstream in TCL but I want to discuss that strategy more widely.
There was a problem hiding this comment.
Flip-flopped a bit: I updated the comment, but I ended up keeping the direct versioned guava dependency for now. We talked in stand-up about excluding it when pulling in TCL, but turns out that LZS does use it after all, such as for @VisibleForTesting annotations.
Longer term, we (the wider we) will need to consciously decide what we want TCL to be, and how opinionated it should be about dependency definition and exposure to consumers.
And, I don't think we need library and testharness' unversioned deps on TCL: both of them pull in java-common-conventions, which itself defines this dependency.
|
https://broadworkbench.atlassian.net/browse/WOR-1560
Upgraded
terra-common-libfrom 0.1.6 -> 1.0.10 mainly for the benefit of Stairway 1.0.0's native MDC-awareness.As a result, MdcHook is greatly reduced and renamed to StairwayLoggingHook: it now only logs at notable state transitions.
LZS now also gets more context than it previously had. Previously, it only logged the context of the calling thread for logs emitted during the execution of each step. Now, it…
This matches what's been live in WSM for awhile.
json4j vulnerability
This upgrade also addresses https://broadworkbench.atlassian.net/browse/WOR-1565 though not in the way we ultimately would like:
terra-common-lib1.0.10 pinsjson4jpast its vulnerability. Gradle dependencyInsights run on client, library, and service projects (compileClasspath and runtimeClasspath) shows thatjson4jis at 0.9.4 or 0.9.5 (vulnerability fixed in 0.9.4)Other dependency changes
OpenTelemetry direct dependencies could be drastically reduced, but we do have to make sure that Spring Boot dependency manager is bringing in a version of
opentelemetry-bomcompatible with what TCL is using.Related to this, I reorganized Gradle conventions related to Spring Boot and Spring dependency manager plug-ins, so that when we pin a Spring Boot dependency (as I did recently to mitigate a
postgresqlvulnerability) it is also pulled into thelibraryproject.Further attention in this space, specifically to make sure that the client build is not bringing in vulnerable dependencies, along with an upgrade to latest Spring Boot 3.2.3 (and resulting dependency clean-up) will be follow-on opportunities.