pjfanning commented on code in PR #8699: URL: https://github.com/apache/hadoop/pull/8699#discussion_r4401017514
########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestJettyClasspath.java: ########## @@ -0,0 +1,40 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.http; + +import org.junit.jupiter.api.Test; + +import static org.apache.hadoop.test.ClasspathTestUtils.assertSingleJettyRelease; +import static org.apache.hadoop.test.ClasspathTestUtils.assertSingleServletApi; + +/** + * HADOOP-19970: this module's classpath resolves one servlet API and one + * Jetty release. + */ +public class TestJettyClasspath { Review Comment: There are six near-identical copies of this class (hadoop-common, shuffle, yarn-common, AHS, nodemanager, router), and the nodemanager one has already gained an extra test. Since `ClasspathTestUtils` already lives in the hadoop-common test-jar, an `AbstractJettyClasspathTest` there with an empty subclass per module would keep them in sync. JUnit 5 runs inherited `@Test` methods. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/ClasspathTestUtils.java: ########## @@ -0,0 +1,98 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.test; + +import java.io.IOException; +import java.net.URL; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Assertions on the test classpath of a module, so a module can check that + * it resolves one servlet API and one Jetty release (HADOOP-19970). + */ +public final class ClasspathTestUtils { + + /** The servlet API every module resolves. */ + public static final String SERVLET_API_JAR = "/jetty-servlet-api-"; + + /** + * Jetty jars by their Maven repository path, + * .../org/eclipse/jetty/[websocket/]artifact/version/. Toolchain + * artifacts such as jetty-servlet-api have their own versions and + * do not match. + */ + private static final Pattern JETTY_JAR = Pattern.compile( Review Comment: This only allows an optional `websocket/` segment, so jars from the other Jetty groups never match. For example, `.../org/eclipse/jetty/http2/http2-server/9.4.44.v20210927/http2-server-9.4.44.v20210927.jar!` gives `group(1)=http2`, `group(2)=http2-server`, and then `/[^/]+\.jar!` can't match the rest. Stale `http2-*`, `alpn`, or `fcgi` jars next to `${jetty.version}` would therefore pass `assertSingleJettyRelease`, which is the drift this is meant to catch (the old Solr stack carried some of these). Maybe `(?:(?:websocket|http2|alpn|fcgi)/)?`, or anchor on the version directory instead? ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-applications-catalog/hadoop-yarn-applications-catalog-webapp/pom.xml: ########## @@ -242,6 +289,53 @@ <groupId>org.eclipse.jetty</groupId> <artifactId>jetty-alpn-client</artifactId> </exclusion> + <!-- Review Comment: This block of Solr Jetty and servlet exclusions, including the comment, is repeated word for word on the other solr dependency (around line 192). The next Solr bump that adds a Jetty artifact has to be fixed in both places, and missing one brings back two Jetty releases. Could the exclusions live once on the solr artifacts in `hadoop-project` `dependencyManagement`? ########## hadoop-client-modules/hadoop-client-runtime/pom.xml: ########## @@ -166,11 +166,22 @@ <!-- Excluding Jetty dependencies to resolve `BanDuplicateClasses` issues caused by duplicate Jetty classes in `hadoop-client-minicluster` and `hadoop-client-runtime`. so keeping it here leads to conflicts. + + jetty-util is not named here. It is this jar's to ship, as Review Comment: Thanks for the explanation. One consequence: every `hadoop-client-runtime` consumer now gets relocated `org.eclipse.jetty.util.*` (and `jetty-continuation`) in a client-facing jar again. That's server-side code and CVE surface that YARN-11793 had removed. And if a later minicluster path brings in `jetty-util` without the per-dependency exclusions, `BanDuplicateClasses` breaks between the two shaded jars again. Would it be narrower to ship `jetty-util` in `hadoop-client-minicluster` (stop excluding it there) and keep it out of the runtime jar? ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/ClasspathTestUtils.java: ########## @@ -0,0 +1,98 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.test; + +import java.io.IOException; +import java.net.URL; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Assertions on the test classpath of a module, so a module can check that + * it resolves one servlet API and one Jetty release (HADOOP-19970). + */ +public final class ClasspathTestUtils { + + /** The servlet API every module resolves. */ + public static final String SERVLET_API_JAR = "/jetty-servlet-api-"; + + /** + * Jetty jars by their Maven repository path, + * .../org/eclipse/jetty/[websocket/]artifact/version/. Toolchain + * artifacts such as jetty-servlet-api have their own versions and + * do not match. + */ + private static final Pattern JETTY_JAR = Pattern.compile( + "/org/eclipse/jetty/(?:websocket/)?([^/]+)/([^/]+)/[^/]+\\.jar!"); + + private ClasspathTestUtils() { + } + + /** + * Assert that exactly one classpath entry provides a resource, + * and that the entry is the expected jar. + * @param resource resource name, such as "javax/servlet/Servlet.class". + * @param jar a fragment of the URL of the jar that should provide it. + * @throws IOException if the classpath cannot be read. + */ + public static void assertSingleProvider(String resource, String jar) + throws IOException { + List<URL> urls = Collections.list( + ClasspathTestUtils.class.getClassLoader().getResources(resource)); + assertEquals(1, urls.size(), "providers of " + resource + ": " + urls); + assertTrue(urls.get(0).toString().contains(jar), + "unexpected provider of " + resource + ": " + urls.get(0)); + } + + /** + * Assert that one servlet API, jetty-servlet-api, is on the classpath. + * @throws IOException if the classpath cannot be read. + */ + public static void assertSingleServletApi() throws IOException { + assertSingleProvider("javax/servlet/Servlet.class", SERVLET_API_JAR); + } + + /** + * Assert that the classpath carries Jetty jars, all from one release. + * @return the Jetty jars found, as artifact-version. + * @throws IOException if the classpath cannot be read. + */ + public static Set<String> assertSingleJettyRelease() throws IOException { + Set<String> jars = new TreeSet<>(); + Set<String> versions = new TreeSet<>(); + for (URL url : Collections.list(ClasspathTestUtils.class.getClassLoader() + .getResources("META-INF/MANIFEST.MF"))) { + Matcher m = JETTY_JAR.matcher(url.toString()); + if (m.find()) { + jars.add(m.group(1) + "-" + m.group(2)); + versions.add(m.group(2)); + } + } + assertFalse(jars.isEmpty(), "no Jetty jars found on the classpath"); Review Comment: Since jars are only recognised by their Maven-repository path, this depends on classpath layout rather than on the classes present. Under an IDE using Gradle's cache (`.../files-2.1/org.eclipse.jetty/jetty-server/<ver>/<hash>/...`) or with shaded or flattened jars, nothing matches and this fails with "no Jetty jars found" even though Jetty is there. A Jetty jar in a different layout next to repository jars also isn't counted. An alternative is to read `Implementation-Version` from each jar's manifest, or look up a known class per artifact via `getResource`, which would make it layout-independent. ########## hadoop-common-project/hadoop-common/pom.xml: ########## @@ -126,6 +126,17 @@ <artifactId>jakarta.activation-api</artifactId> <scope>runtime</scope> </dependency> + <!-- + HttpServer2 and the filters around it are written against javax.servlet. + The API used to arrive only as a transitive of jetty-server; it is declared + here so the module states which servlet API it compiles against instead of + depending on which jar happened to come first on the classpath. + --> + <dependency> + <groupId>org.eclipse.jetty.toolchain</groupId> + <artifactId>jetty-servlet-api</artifactId> Review Comment: I realise moving to this artifact is the point of the PR, but it changes things for downstream projects. Before, they got `javax.servlet:javax.servlet-api` via `jetty-server`, and Maven could pick one version if they also declared it (Spark, Hive and HBase all do). `jetty-servlet-api` has different coordinates, so those projects will now get both jars providing `javax.servlet.*`, and which one wins depends on classpath order. That's exactly what the PR has to work around in `timelineservice-hbase-tests` via `hbase-server`. At minimum this deserves a release note telling downstream projects to exclude one or the other. Is there a reason not to keep `javax.servlet:javax.servlet-api` as the single managed API instead? ########## hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-shuffle/pom.xml: ########## @@ -55,6 +55,11 @@ <groupId>${leveldbjni.group}</groupId> <artifactId>leveldbjni-all</artifactId> </dependency> + <!-- ShuffleChannelHandler uses org.eclipse.jetty.http.HttpHeader --> + <dependency> + <groupId>org.eclipse.jetty</groupId> + <artifactId>jetty-http</artifactId> Review Comment: `hadoop-common` (which already brings `jetty-http`) is `provided` in this module tree (`hadoop-mapreduce-client/pom.xml`), but this declaration uses the default compile scope. As a result, `hadoop-mapreduce-client-shuffle` now publishes `jetty-http` as a compile dependency to its consumers, and it gets copied into the mapreduce lib layout. The applicationhistoryservice change in this PR uses `provided` for the same situation. Should this be `<scope>provided</scope>` too? ########## hadoop-project/pom.xml: ########## @@ -909,8 +915,25 @@ <groupId>org.eclipse.jetty</groupId> <artifactId>javax.servlet-api</artifactId> Review Comment: Since the comment below says this exclusion never matched anything, could it be replaced by the `javax.servlet` one rather than kept next to it? Leaving the dead coordinates in makes it likely they'll be copied again. Related (line 2235): the `jakarta.servlet-api` exclusion is declared on both the managed `jersey-test-framework-core` and `jersey-test-framework-provider-jdk-http` (and again in `hadoop-client-minicluster`). If the managed one on `-core` already applies when `-jdk-http` pulls it in transitively, the extra declarations could go. If it doesn't, a short note saying so would help the next reader. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
