From 6fac4e9ec81863012ad94d68dabc714e0a4576ec Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 13 Nov 2024 12:23:14 +1100 Subject: [PATCH 1/2] fix GzipHandler issues with HttpConnector mode enabled on Jetty 9.4 Signed-off-by: Lachlan Roberts --- .../runtime/jetty9/JettyRequestAPIData.java | 21 ++- .../jetty9/JettyServletEngineAdapter.java | 3 +- .../runtime/jetty9/GzipHandlerTest.java | 147 ++++++++++++++++++ runtime/testapps/pom.xml | 4 + .../jetty9/gzipapp/EE10EchoServlet.java | 45 ++++++ .../jetty9/gzipapp/EE8EchoServlet.java | 45 ++++++ .../gzipapp/ee10/WEB-INF/appengine-web.xml | 24 +++ .../jetty9/gzipapp/ee10/WEB-INF/web.xml | 32 ++++ .../gzipapp/ee8/WEB-INF/appengine-web.xml | 24 +++ .../jetty9/gzipapp/ee8/WEB-INF/web.xml | 32 ++++ .../gzipapp/jetty94/WEB-INF/appengine-web.xml | 25 +++ .../jetty9/gzipapp/jetty94/WEB-INF/web.xml | 32 ++++ 12 files changed, 425 insertions(+), 9 deletions(-) create mode 100644 runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/GzipHandlerTest.java create mode 100644 runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE10EchoServlet.java create mode 100644 runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE8EchoServlet.java create mode 100644 runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/appengine-web.xml create mode 100644 runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/web.xml create mode 100644 runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/appengine-web.xml create mode 100644 runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/web.xml create mode 100644 runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/appengine-web.xml create mode 100644 runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/web.xml diff --git a/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java b/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java index f14811a03..cb217fd1d 100644 --- a/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java +++ b/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java @@ -68,6 +68,8 @@ import java.util.stream.Stream; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequestWrapper; + +import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpURI; @@ -126,15 +128,20 @@ public JettyRequestAPIData( this.securityTicket = DEFAULT_SECRET_KEY; HttpFields fields = new HttpFields(); - List headerNames = Collections.list(request.getHeaderNames()); - for (String headerName : headerNames) { - String name = headerName.toLowerCase(Locale.ROOT); - String value = request.getHeader(headerName); + for (HttpField field : request.getHttpFields()) { + // If it has a HttpHeader it is one of the standard headers so won't match any appengine specific header. + if (field.getHeader() != null) { + fields.add(field); + continue; + } + + String lowerCaseName = field.getName().toLowerCase(Locale.ROOT); + String value = field.getValue(); if (Strings.isNullOrEmpty(value)) { continue; } - switch (name) { + switch (lowerCaseName) { case X_APPENGINE_TRUSTED_IP_REQUEST: // If there is a value, then the application is trusted // If the value is IS_TRUSTED, then the user is trusted @@ -236,9 +243,9 @@ public JettyRequestAPIData( break; } - if (passThroughPrivateHeaders || !PRIVATE_APPENGINE_HEADERS.contains(name)) { + if (passThroughPrivateHeaders || !PRIVATE_APPENGINE_HEADERS.contains(lowerCaseName)) { // Only non AppEngine specific headers are passed to the application. - fields.add(name, value); + fields.add(field); } } diff --git a/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyServletEngineAdapter.java b/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyServletEngineAdapter.java index d39d9bbe7..86644f4e7 100644 --- a/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyServletEngineAdapter.java +++ b/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyServletEngineAdapter.java @@ -43,7 +43,6 @@ import java.util.Optional; import javax.servlet.ServletException; import org.eclipse.jetty.server.Connector; -import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.SizeLimitHandler; @@ -141,10 +140,10 @@ public void start(String serverInfo, ServletEngineAdapter.Config runtimeOptions) if (Boolean.getBoolean(HTTP_CONNECTOR_MODE)) { logger.atInfo().log("Using HTTP_CONNECTOR_MODE to bypass RPC"); appVersionKey = AppVersionKey.fromAppInfo(appinfo); - JettyHttpProxy.insertHandlers(server); AppVersion appVersion = appVersionHandlerMap.getAppVersion(appVersionKey); server.insertHandler( new JettyHttpHandler(runtimeOptions, appVersion, appVersionKey, appInfoFactory)); + JettyHttpProxy.insertHandlers(server); ServerConnector connector = JettyHttpProxy.newConnector(server, runtimeOptions); server.addConnector(connector); } else { diff --git a/runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/GzipHandlerTest.java b/runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/GzipHandlerTest.java new file mode 100644 index 000000000..a9d1dcf18 --- /dev/null +++ b/runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/GzipHandlerTest.java @@ -0,0 +1,147 @@ +/* + * Copyright 2021 Google LLC + * + * Licensed 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 + * + * https://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 com.google.apphosting.runtime.jetty9; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.util.Arrays; +import java.util.Collection; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.zip.GZIPOutputStream; +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentProvider; +import org.eclipse.jetty.client.api.Result; +import org.eclipse.jetty.client.util.InputStreamContentProvider; +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.util.Utf8StringBuilder; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class GzipHandlerTest extends JavaRuntimeViaHttpBase { + + @Parameterized.Parameters + public static Collection data() { + return Arrays.asList( + new Object[][] { + {"jetty94", false}, + {"jetty94", true}, + {"ee8", false}, + {"ee8", true}, + {"ee10", false}, + {"ee10", true}, + }); + } + + private static final int MAX_SIZE = 32 * 1024 * 1024; + + @Rule public TemporaryFolder temp = new TemporaryFolder(); + private final HttpClient httpClient = new HttpClient(); + private final boolean httpMode; + private final String environment; + private RuntimeContext runtime; + + public GzipHandlerTest(String environment, boolean httpMode) { + this.environment = environment; + this.httpMode = httpMode; + System.setProperty("appengine.use.HttpConnector", Boolean.toString(httpMode)); + } + + @Before + public void before() throws Exception { + String app = "com/google/apphosting/runtime/jetty9/gzipapp/" + environment; + copyAppToDir(app, temp.getRoot().toPath()); + httpClient.start(); + runtime = runtimeContext(); + System.err.println("==== Using Environment: " + environment + " " + httpMode + " ===="); + } + + @After + public void after() throws Exception { + httpClient.stop(); + runtime.close(); + } + + @Test + public void testRequestGzipContent() throws Exception { + int contentLength = 1024; + + CompletableFuture completionListener = new CompletableFuture<>(); + byte[] data = new byte[contentLength]; + Arrays.fill(data, (byte) 'X'); + Utf8StringBuilder received = new Utf8StringBuilder(); + ContentProvider content = new InputStreamContentProvider(gzip(data)); + + String url = runtime.jettyUrl("/"); + httpClient + .newRequest(url) + .content(content) + .onResponseContentAsync( + (response, content1, callback) -> { + received.append(content1); + callback.succeeded(); + }) + .header(HttpHeader.CONTENT_ENCODING, "gzip") + .send(completionListener::complete); + + // The request was successfully decoded by the GzipHandler. + Result response = completionListener.get(5, TimeUnit.SECONDS); + assertThat(response.getResponse().getStatus(), equalTo(HttpStatus.OK_200)); + String contentReceived = received.toString(); + assertThat(contentReceived, containsString("\nX-Content-Encoding: gzip\n")); + assertThat(contentReceived, not(containsString("\nContent-Encoding: gzip\n"))); + assertThat(contentReceived, containsString("\nAccept-Encoding: gzip\n")); + + // Server correctly echoed content of request. + String expectedData = new String(data); + String actualData = contentReceived.substring(contentReceived.length() - contentLength); + assertThat(actualData, equalTo(expectedData)); + + // Response was gzip encoded. + HttpFields responseHeaders = response.getResponse().getHeaders(); + assertThat(responseHeaders.get(HttpHeader.CONTENT_ENCODING), equalTo("gzip")); + } + + private RuntimeContext runtimeContext() throws Exception { + RuntimeContext.Config config = + RuntimeContext.Config.builder().setApplicationPath(temp.getRoot().toString()).build(); + return RuntimeContext.create(config); + } + + private static InputStream gzip(byte[] data) throws IOException { + ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); + try (GZIPOutputStream gzipOutputStream = new GZIPOutputStream(byteArrayOutputStream)) { + gzipOutputStream.write(data); + } + return new ByteArrayInputStream(byteArrayOutputStream.toByteArray()); + } +} diff --git a/runtime/testapps/pom.xml b/runtime/testapps/pom.xml index 30e66d6c4..98ebe5ef5 100644 --- a/runtime/testapps/pom.xml +++ b/runtime/testapps/pom.xml @@ -36,6 +36,10 @@ com.google.appengine appengine-api-1.0-sdk + + org.eclipse.jetty + jetty-util + com.google.guava guava diff --git a/runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE10EchoServlet.java b/runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE10EchoServlet.java new file mode 100644 index 000000000..9e8385011 --- /dev/null +++ b/runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE10EchoServlet.java @@ -0,0 +1,45 @@ +/* + * Copyright 2021 Google LLC + * + * Licensed 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 + * + * https://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 com.google.apphosting.runtime.jetty9.gzipapp; + +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import java.io.PrintWriter; +import java.util.Enumeration; +import org.eclipse.jetty.util.IO; + +/** Servlet that prints all the system properties. */ +public class EE10EchoServlet extends HttpServlet { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException { + resp.setContentType("text/plain"); + + PrintWriter writer = resp.getWriter(); + writer.println(); + Enumeration headerNames = req.getHeaderNames(); + while (headerNames.hasMoreElements()) { + String headerName = headerNames.nextElement(); + writer.println(headerName + ": " + req.getHeader(headerName)); + } + writer.println(); + + String string = IO.toString(req.getInputStream()); + writer.print(string); + } +} diff --git a/runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE8EchoServlet.java b/runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE8EchoServlet.java new file mode 100644 index 000000000..e9a68ac83 --- /dev/null +++ b/runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/gzipapp/EE8EchoServlet.java @@ -0,0 +1,45 @@ +/* + * Copyright 2021 Google LLC + * + * Licensed 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 + * + * https://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 com.google.apphosting.runtime.jetty9.gzipapp; + +import java.io.IOException; +import java.io.PrintWriter; +import java.util.Enumeration; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.util.IO; + +/** Servlet that prints all the system properties. */ +public class EE8EchoServlet extends HttpServlet { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException { + resp.setContentType("text/plain"); + + PrintWriter writer = resp.getWriter(); + writer.println(); + Enumeration headerNames = req.getHeaderNames(); + while (headerNames.hasMoreElements()) { + String headerName = headerNames.nextElement(); + writer.println(headerName + ": " + req.getHeader(headerName)); + } + writer.println(); + + String string = IO.toString(req.getInputStream()); + writer.print(string); + } +} diff --git a/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/appengine-web.xml b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/appengine-web.xml new file mode 100644 index 000000000..7c3e813ff --- /dev/null +++ b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/appengine-web.xml @@ -0,0 +1,24 @@ + + + + + java21 + gzip + + + + diff --git a/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/web.xml b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/web.xml new file mode 100644 index 000000000..ca78e1d9f --- /dev/null +++ b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee10/WEB-INF/web.xml @@ -0,0 +1,32 @@ + + + + + + Main + com.google.apphosting.runtime.jetty9.gzipapp.EE10EchoServlet + + + Main + /* + + diff --git a/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/appengine-web.xml b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/appengine-web.xml new file mode 100644 index 000000000..c5e365f0f --- /dev/null +++ b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/appengine-web.xml @@ -0,0 +1,24 @@ + + + + + java21 + gzip + + + + diff --git a/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/web.xml b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/web.xml new file mode 100644 index 000000000..8c47c7d67 --- /dev/null +++ b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/ee8/WEB-INF/web.xml @@ -0,0 +1,32 @@ + + + + + + Main + com.google.apphosting.runtime.jetty9.gzipapp.EE8EchoServlet + + + Main + /* + + diff --git a/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/appengine-web.xml b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/appengine-web.xml new file mode 100644 index 000000000..d2766777d --- /dev/null +++ b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/appengine-web.xml @@ -0,0 +1,25 @@ + + + + + java8 + gzip + true + + + + diff --git a/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/web.xml b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/web.xml new file mode 100644 index 000000000..8c47c7d67 --- /dev/null +++ b/runtime/testapps/src/main/resources/com/google/apphosting/runtime/jetty9/gzipapp/jetty94/WEB-INF/web.xml @@ -0,0 +1,32 @@ + + + + + + Main + com.google.apphosting.runtime.jetty9.gzipapp.EE8EchoServlet + + + Main + /* + + From 274ad9f74390abff89621846856705586ac6ce05 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 13 Nov 2024 16:44:23 +1100 Subject: [PATCH 2/2] optimization for the Jetty 12 JettyRequestAPIData Signed-off-by: Lachlan Roberts --- .../apphosting/runtime/jetty/http/JettyRequestAPIData.java | 6 ++++++ .../apphosting/runtime/jetty9/JettyRequestAPIData.java | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/http/JettyRequestAPIData.java b/runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/http/JettyRequestAPIData.java index 8bc17f80e..8a3d79625 100644 --- a/runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/http/JettyRequestAPIData.java +++ b/runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/http/JettyRequestAPIData.java @@ -119,6 +119,12 @@ public JettyRequestAPIData( HttpFields.Mutable fields = HttpFields.build(); for (HttpField field : request.getHeaders()) { + // If it has a HttpHeader it is one of the standard headers so won't match any appengine specific header. + if (field.getHeader() != null) { + fields.add(field); + continue; + } + String name = field.getLowerCaseName(); String value = field.getValue(); if (Strings.isNullOrEmpty(value)) { diff --git a/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java b/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java index cb217fd1d..3766b8938 100644 --- a/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java +++ b/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java @@ -135,13 +135,13 @@ public JettyRequestAPIData( continue; } - String lowerCaseName = field.getName().toLowerCase(Locale.ROOT); + String name = field.getName().toLowerCase(Locale.ROOT); String value = field.getValue(); if (Strings.isNullOrEmpty(value)) { continue; } - switch (lowerCaseName) { + switch (name) { case X_APPENGINE_TRUSTED_IP_REQUEST: // If there is a value, then the application is trusted // If the value is IS_TRUSTED, then the user is trusted @@ -243,7 +243,7 @@ public JettyRequestAPIData( break; } - if (passThroughPrivateHeaders || !PRIVATE_APPENGINE_HEADERS.contains(lowerCaseName)) { + if (passThroughPrivateHeaders || !PRIVATE_APPENGINE_HEADERS.contains(name)) { // Only non AppEngine specific headers are passed to the application. fields.add(field); }