You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "bonampak (via GitHub)" <gi...@apache.org> on 2025/02/19 13:38:06 UTC
[PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
bonampak opened a new pull request, #992:
URL: https://github.com/apache/knox/pull/992
## What changes were proposed in this pull request?
Upgrade com.cloudera.api.swagger:cloudera-manager-api-swagger to 7.13.1.
Update code for okhttp3. Resolve dependency convergence issues.
## How was this patch tested?
Unit and integration tests, deployed to a dev cluster and restarted Knox to verify discovery and knox ui works.
Please review [Knox Contributing Process](https://cwiki.apache.org/confluence/display/KNOX/Contribution+Process#ContributionProcess-GithubWorkflow) before opening a pull request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #992:
URL: https://github.com/apache/knox/pull/992#issuecomment-2687419546
> because of dependency convergence issues
Isn't it possible to exclude them everywhere? I'd like to avoid pulling in more 3rd party dependencies that are not essential for Knox to work. They are expensive to maintain in the future (CVEs, possible incompatibility, etc...).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "pzampino (via GitHub)" <gi...@apache.org>.
pzampino commented on code in PR #992:
URL: https://github.com/apache/knox/pull/992#discussion_r1961766505
##########
gateway-spi/src/main/java/org/apache/knox/gateway/util/TruststoreSSLContextUtils.java:
##########
@@ -48,4 +52,25 @@ public static SSLContext getTruststoreSSLContext(KeyStore truststore) {
return sslContext;
}
+ public static X509TrustManager getTrustManager(KeyStore truststore) {
+ try {
+ if (truststore != null) {
+ TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(truststore);
+ TrustManager[] trustManagers = trustManagerFactory.getTrustManagers();
+ if (trustManagers != null) {
+ for (TrustManager tm : trustManagers) {
+ if (tm instanceof X509TrustManager) {
+ return (X509TrustManager) tm;
+ }
+ }
+ }
+ throw new IllegalStateException("Unexpected default trust managers:" + Arrays.toString(trustManagers));
Review Comment:
What is the consequence of throwing this exception? The DiscoveryApiClient#configureSsl() method does not catch it, so it will bubble up. Do you know where it will eventually be handled and how?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "bonampak (via GitHub)" <gi...@apache.org>.
bonampak commented on PR #992:
URL: https://github.com/apache/knox/pull/992#issuecomment-3057377381
Closing this as it is part of https://github.com/apache/knox/pull/1059
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "bonampak (via GitHub)" <gi...@apache.org>.
bonampak commented on code in PR #992:
URL: https://github.com/apache/knox/pull/992#discussion_r1964009173
##########
gateway-spi/src/main/java/org/apache/knox/gateway/util/TruststoreSSLContextUtils.java:
##########
@@ -48,4 +52,25 @@ public static SSLContext getTruststoreSSLContext(KeyStore truststore) {
return sslContext;
}
+ public static X509TrustManager getTrustManager(KeyStore truststore) {
+ try {
+ if (truststore != null) {
+ TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(truststore);
+ TrustManager[] trustManagers = trustManagerFactory.getTrustManagers();
+ if (trustManagers != null) {
+ for (TrustManager tm : trustManagers) {
+ if (tm instanceof X509TrustManager) {
+ return (X509TrustManager) tm;
+ }
+ }
+ }
+ throw new IllegalStateException("Unexpected default trust managers:" + Arrays.toString(trustManagers));
Review Comment:
I took this part of the migration from here:
https://square.github.io/okhttp/5.x/okhttp/okhttp3/-ok-http-client/-builder/ssl-socket-factory.html
I would expect this to work in a standard setup (and it's the case with JDK 8-17).
I have not tested it on a FIPS-enabled cluster though.
I throw and catch IllegalStateException and return null if the trust manager for the default algorithm and trust store is not an instance of X509TrustManager. The `getTruststoreSSLContext()` method also uses the same methods by calling SSLContextBuilder.loadTrustMaterial(): uses the default TrustManagerFactory algorithm, initializes and gets the trust managers; it does not check the instance type. But the okhttp API expects an implementation of javax.net.ssl.X509TrustManager to validate the server's certificates.
The deprecated method variant does not require it, okhttp would use reflection to get one from `sun.security.ssl.SSLContextImpl`:
https://github.com/square/okhttp/blob/4984568367caaf359b82c452bd28b5e192824d1c/okhttp/src/main/kotlin/okhttp3/internal/platform/Platform.kt#L88
But this was removed in Okhttp 5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #992:
URL: https://github.com/apache/knox/pull/992#issuecomment-3057377588
Closing this one w/o merging because #1059 contains these changes too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "bonampak (via GitHub)" <gi...@apache.org>.
bonampak closed pull request #992: KNOX-3094 - Update CM API swagger to 7.13.1
URL: https://github.com/apache/knox/pull/992
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "bonampak (via GitHub)" <gi...@apache.org>.
bonampak commented on PR #992:
URL: https://github.com/apache/knox/pull/992#issuecomment-2672185075
Okhttp 4.x is written in Kotlin and needs the kotlin-stdlib artifacts.
The com.cloudera.api.swagger:cloudera-manager-api-swagger:7.13.1 brings in
```
<gson-fire-version>1.8.5</gson-fire-version>
<swagger-core-version>2.0.0</swagger-core-version>
<okhttp-version>4.10.0</okhttp-version>
<gson-version>2.10.1</gson-version>
```
Okhttp 4.10 is not the latest, but the swagger-codegen library only has this option.
From the CM part, they would have liked to set the okhttp3 version to 4.12. but swagger-codegen is bringing internally version 4.10
https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java#L89
https://github.com/swagger-api/swagger-codegen/issues/12337
(It seems that openapi-generator with okhttp 4.12 - gson or with httpclient 5 could be used instead of swagger-codegen.)
So we need to bump okhttp to 4.12.0, and I was also asked to upgrade gson-fire to 1.9.0; the latter has a gson:2.10.1 dependency (we had 2.8.9).
The kotlin libraries are upgraded to 1.9.10 because of dependency convergence issues:
```
Dependency convergence error for org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 paths to dependency are:
+-org.apache.knox:gateway-discovery-cm:2.1.0-SNAPSHOT
+-com.cloudera.api.swagger:cloudera-manager-api-swagger:7.13.1
+-com.squareup.okhttp3:logging-interceptor:4.12.0
+-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21
and
+-org.apache.knox:gateway-discovery-cm:2.1.0-SNAPSHOT
+-com.squareup.okhttp3:okhttp:4.12.0
+-com.squareup.okio:okio:3.6.0
+-com.squareup.okio:okio-jvm:3.6.0
+-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.9.10
and
+-org.apache.knox:gateway-discovery-cm:2.1.0-SNAPSHOT
+-com.squareup.okhttp3:okhttp:4.12.0
+-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21
```
https://github.com/square/okhttp/issues/8288
"OkHttp 4.12.0 expects you to use any version of kotlin greater than or equal to 1.8.21. We don't rush out a release each time okio releases with a newer Kotlin. This isn't a thing we worry about at all." ... "It's already fixed in the 5.0 alphas.".
Due to these issues, teams either remain on the latest non-kotlin version or remove okhttp:
https://issues.apache.org/jira/browse/HADOOP-18496 upgrade kotlin-stdlib due to CVEs
https://issues.apache.org/jira/browse/HADOOP-18890 Remove use of okhttp in runtime code.
org.jetbrains:annotations also needed to be resolved:
```
Dependency convergence error for org.jetbrains:annotations:16.0.2 paths to dependency are:
+-org.apache.knox:gateway-discovery-cm:2.1.0-SNAPSHOT
+-org.apache.knox:gateway-server:2.1.0-SNAPSHOT
+-org.jetbrains.pty4j:pty4j:0.11.4
+-org.jetbrains:annotations:16.0.2
and
+-org.apache.knox:gateway-discovery-cm:2.1.0-SNAPSHOT
+-com.squareup.okhttp3:okhttp:4.12.0
+-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.9.10
+-org.jetbrains.kotlin:kotlin-stdlib:1.9.10
+-org.jetbrains:annotations:13.0
```
I excluded it from kotlin-stdlib because by simply adding to dependencyManagement would cause it to become a compile-time dependency on pty4j.
The okio dependency was pinned to 3.6.0 because of this:
```
Dependency convergence error for com.squareup.okio:okio:1.6.0 paths to dependency are:
+-org.apache.knox:gateway-test-release:2.1.0-SNAPSHOT
+-org.apache.hadoop:hadoop-hdfs-client:3.2.4
+-com.squareup.okhttp:okhttp:2.7.5
+-com.squareup.okio:okio:1.6.0
and
+-org.apache.knox:gateway-test-release:2.1.0-SNAPSHOT
+-org.apache.knox:gateway-release:2.1.0-SNAPSHOT
+-org.apache.knox:gateway-discovery-cm:2.1.0-SNAPSHOT
+-com.squareup.okhttp3:okhttp:4.12.0
+-com.squareup.okio:okio:3.6.0
```
hadoop-hdfs-client is a test dependency (so the old okhttp:2.7.5 will not be in our release artifact dependencies) and okio is backwards-compatible:
https://github.com/square/okio/issues/1323
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KNOX-3094 - Update CM API swagger to 7.13.1 [knox]
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on PR #992:
URL: https://github.com/apache/knox/pull/992#issuecomment-2670784314
According to the PR description, this change is supposed to upgrade CM and OkHttp versions. However, I see that many others were touched:
Why are these new dependencies needed (particularly the Kotlin one)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org