-
Notifications
You must be signed in to change notification settings - Fork 85
Test for Brotli decompressor #2066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
f6a6245
d7108fd
b9d9c10
c14bd73
db45ff3
1dde551
81128d8
dc3331f
3bbdc9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| import static io.envoyproxy.envoymobile.engine.EnvoyConfiguration.TrustChainVerification.VERIFY_TRUST_CHAIN; | ||
|
|
||
| import android.content.Context; | ||
| import androidx.annotation.VisibleForTesting; | ||
| import io.envoyproxy.envoymobile.engine.AndroidEngineImpl; | ||
| import io.envoyproxy.envoymobile.engine.AndroidJniLibrary; | ||
| import io.envoyproxy.envoymobile.engine.AndroidNetworkMonitor; | ||
|
|
@@ -15,6 +16,7 @@ | |
| import io.envoyproxy.envoymobile.engine.types.EnvoyLogger; | ||
| import io.envoyproxy.envoymobile.engine.types.EnvoyOnEngineRunning; | ||
| import io.envoyproxy.envoymobile.engine.types.EnvoyStringAccessor; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -26,6 +28,24 @@ | |
| */ | ||
| public class NativeCronetEngineBuilderImpl extends CronetEngineBuilderImpl { | ||
|
|
||
| private static final String BROTLI_CONFIG = | ||
| "\n" | ||
| + | ||
| " \"@type\": type.googleapis.com/envoy.extensions.filters.http.decompressor.v3.Decompressor\n" | ||
| + " decompressor_library:\n" | ||
| + " name: text_optimized\n" | ||
| + " typed_config:\n" | ||
| + | ||
| " \"@type\": type.googleapis.com/envoy.extensions.compression.brotli.decompressor.v3.Brotli\n" | ||
| + " request_direction_config:\n" | ||
| + " common_config:\n" | ||
| + " enabled:\n" | ||
| + " default_value: false\n" | ||
| + " runtime_key: request_decompressor_enabled\n" | ||
| + " response_direction_config:\n" | ||
| + " common_config:\n" | ||
| + " ignore_no_transform_header: true\n"; | ||
|
|
||
| private final EnvoyLogger mEnvoyLogger = null; | ||
| private final EnvoyEventTracker mEnvoyEventTracker = null; | ||
| private boolean mAdminInterfaceEnabled = false; | ||
|
|
@@ -50,9 +70,6 @@ public class NativeCronetEngineBuilderImpl extends CronetEngineBuilderImpl { | |
| private String mAppId = "unspecified"; | ||
| private TrustChainVerification mTrustChainVerification = VERIFY_TRUST_CHAIN; | ||
| private String mVirtualClusters = "[]"; | ||
| private List<EnvoyHTTPFilterFactory> mPlatformFilterChain = Collections.emptyList(); | ||
| private List<EnvoyNativeFilterConfig> mNativeFilterChain = Collections.emptyList(); | ||
| private Map<String, EnvoyStringAccessor> mStringAccessors = Collections.emptyMap(); | ||
|
|
||
| /** | ||
| * Builder for Native Cronet Engine. Default config enables SPDY, disables QUIC and HTTP cache. | ||
|
|
@@ -61,6 +78,17 @@ public class NativeCronetEngineBuilderImpl extends CronetEngineBuilderImpl { | |
| */ | ||
| public NativeCronetEngineBuilderImpl(Context context) { super(context); } | ||
|
|
||
| /** | ||
| * Indicates to skip the TLS certificate verification. | ||
| * | ||
| * @return the builder to facilitate chaining. | ||
| */ | ||
| @VisibleForTesting | ||
| public CronetEngineBuilderImpl setMockCertVerifierForTesting() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this logic is (roughly) moving from CronetEngineBuilder to NativeCronetEngineBuilderImpl. Out of curiosity, what's that motivation?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved this here because this is an EnvoyMobile enum that is being use, and this is the only class bridging EM and Cronet for the Configuration. The second weaker reason is that CronetEngineBuilderImpl (where that method was before this PR) is also used by the Java implementation of Cronet, where it makes no sense. |
||
| mTrustChainVerification = TrustChainVerification.ACCEPT_UNTRUSTED; | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public ExperimentalCronetEngine build() { | ||
| if (getUserAgent() == null) { | ||
|
|
@@ -79,14 +107,21 @@ EnvoyEngine createEngine(EnvoyOnEngineRunning onEngineRunning) { | |
| } | ||
|
|
||
| private EnvoyConfiguration createEnvoyConfiguration() { | ||
| List<EnvoyHTTPFilterFactory> platformFilterChain = Collections.emptyList(); | ||
| List<EnvoyNativeFilterConfig> nativeFilterChain = new ArrayList<>(); | ||
| Map<String, EnvoyStringAccessor> stringAccessors = Collections.emptyMap(); | ||
| if (brotliEnabled()) { | ||
| nativeFilterChain.add( | ||
| new EnvoyNativeFilterConfig("envoy.filters.http.decompressor", BROTLI_CONFIG)); | ||
| } | ||
| return new EnvoyConfiguration( | ||
| mAdminInterfaceEnabled, mGrpcStatsDomain, mStatsDPort, mConnectTimeoutSeconds, | ||
| mDnsRefreshSeconds, mDnsFailureRefreshSecondsBase, mDnsFailureRefreshSecondsMax, | ||
| mDnsQueryTimeoutSeconds, mDnsPreresolveHostnames, mDnsFallbackNameservers, | ||
| mEnableDnsFilterUnroutableFamilies, mEnableHappyEyeballs, mEnableInterfaceBinding, | ||
| mH2ConnectionKeepaliveIdleIntervalMilliseconds, mH2ConnectionKeepaliveTimeoutSeconds, | ||
| mStatsFlushSeconds, mStreamIdleTimeoutSeconds, mPerTryIdleTimeoutSeconds, mAppVersion, | ||
| mAppId, mTrustChainVerification, mVirtualClusters, mNativeFilterChain, mPlatformFilterChain, | ||
| mStringAccessors); | ||
| mAppId, mTrustChainVerification, mVirtualClusters, nativeFilterChain, platformFilterChain, | ||
| stringAccessors); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| package org.chromium.net; | ||
|
|
||
| import static org.chromium.net.testing.CronetTestRule.SERVER_CERT_PEM; | ||
| import static org.chromium.net.testing.CronetTestRule.SERVER_KEY_PKCS8_PEM; | ||
| import static org.chromium.net.testing.CronetTestRule.getContext; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| import androidx.test.filters.SmallTest; | ||
| import org.chromium.net.testing.CronetTestRule; | ||
| import org.chromium.net.testing.CronetTestRule.OnlyRunNativeCronet; | ||
| import org.chromium.net.testing.CronetTestUtil; | ||
| import org.chromium.net.testing.Feature; | ||
| import org.chromium.net.testing.Http2TestServer; | ||
| import org.chromium.net.testing.TestFilesInstaller; | ||
| import org.chromium.net.testing.TestUrlRequestCallback; | ||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Rule; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.robolectric.RobolectricTestRunner; | ||
|
|
||
| /** | ||
| * Simple test for Brotli support. | ||
| */ | ||
| @RunWith(RobolectricTestRunner.class) | ||
| public class BrotliTest { | ||
| @Rule public final CronetTestRule mTestRule = new CronetTestRule(); | ||
|
|
||
| private CronetEngine mCronetEngine; | ||
|
|
||
| @Before | ||
| public void setUp() throws Exception { | ||
| TestFilesInstaller.installIfNeeded(getContext()); | ||
| assertTrue( | ||
| Http2TestServer.startHttp2TestServer(getContext(), SERVER_CERT_PEM, SERVER_KEY_PKCS8_PEM)); | ||
| } | ||
|
|
||
| @After | ||
| public void tearDown() throws Exception { | ||
| assertTrue(Http2TestServer.shutdownHttp2TestServer()); | ||
| if (mCronetEngine != null) { | ||
| mCronetEngine.shutdown(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| @SmallTest | ||
| @Feature({"Cronet"}) | ||
| @OnlyRunNativeCronet | ||
| public void testBrotliAdvertised() throws Exception { | ||
| ExperimentalCronetEngine.Builder builder = new ExperimentalCronetEngine.Builder(getContext()); | ||
| builder.enableBrotli(true); | ||
| CronetTestUtil.setMockCertVerifierForTesting(builder); | ||
| mCronetEngine = builder.build(); | ||
| String url = Http2TestServer.getEchoAllHeadersUrl(); | ||
| TestUrlRequestCallback callback = startAndWaitForComplete(url); | ||
| assertEquals(200, callback.mResponseInfo.getHttpStatusCode()); | ||
| // TODO(carloseltuerto): also support "deflate" decompressor - Cronet does. | ||
| assertTrue(callback.mResponseAsString.contains("accept-encoding: br,gzip")); | ||
| } | ||
|
|
||
| @Test | ||
| @SmallTest | ||
| @Feature({"Cronet"}) | ||
| @OnlyRunNativeCronet | ||
| public void testBrotliNotAdvertised() throws Exception { | ||
| ExperimentalCronetEngine.Builder builder = new ExperimentalCronetEngine.Builder(getContext()); | ||
| CronetTestUtil.setMockCertVerifierForTesting(builder); | ||
| mCronetEngine = builder.build(); | ||
| String url = Http2TestServer.getEchoAllHeadersUrl(); | ||
| TestUrlRequestCallback callback = startAndWaitForComplete(url); | ||
| assertEquals(200, callback.mResponseInfo.getHttpStatusCode()); | ||
| assertFalse(callback.mResponseAsString.contains("br")); | ||
| } | ||
|
|
||
| @Test | ||
| @SmallTest | ||
| @Feature({"Cronet"}) | ||
| @OnlyRunNativeCronet | ||
| public void testBrotliDecoded() throws Exception { | ||
| ExperimentalCronetEngine.Builder builder = new ExperimentalCronetEngine.Builder(getContext()); | ||
| builder.enableBrotli(true); | ||
| CronetTestUtil.setMockCertVerifierForTesting(builder); | ||
| mCronetEngine = builder.build(); | ||
| String url = Http2TestServer.getServeSimpleBrotliResponse(); | ||
| TestUrlRequestCallback callback = startAndWaitForComplete(url); | ||
| assertEquals(200, callback.mResponseInfo.getHttpStatusCode()); | ||
| String expectedResponse = "The quick brown fox jumps over the lazy dog"; | ||
| assertEquals(expectedResponse, callback.mResponseAsString); | ||
| // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2086): uncomment this line. | ||
| // assertEquals(callback.mResponseInfo.getAllHeaders().get("content-encoding").get(0),"br"); | ||
| } | ||
|
|
||
| private TestUrlRequestCallback startAndWaitForComplete(String url) { | ||
| TestUrlRequestCallback callback = new TestUrlRequestCallback(); | ||
| UrlRequest.Builder builder = | ||
| mCronetEngine.newUrlRequestBuilder(url, callback, callback.getExecutor()); | ||
| builder.build().start(); | ||
| callback.blockForDone(); | ||
| return callback; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package org.chromium.net.testing; | ||
|
|
||
| import org.chromium.net.ExperimentalCronetEngine; | ||
| import org.chromium.net.impl.NativeCronetEngineBuilderImpl; | ||
|
|
||
| /** | ||
| * Utilities for Cronet testing | ||
| */ | ||
| public final class CronetTestUtil { | ||
|
|
||
| public static void setMockCertVerifierForTesting(ExperimentalCronetEngine.Builder builder) { | ||
| getCronetEngineBuilderImpl(builder).setMockCertVerifierForTesting(); | ||
| } | ||
|
|
||
| public static NativeCronetEngineBuilderImpl | ||
| getCronetEngineBuilderImpl(ExperimentalCronetEngine.Builder builder) { | ||
| return (NativeCronetEngineBuilderImpl)builder.getBuilderDelegate(); | ||
| } | ||
|
|
||
| private CronetTestUtil() {} | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you, but I think I'd be inclined to add this to the .cc file, not the .h file. (Though many similar includes seem to be in the .h file already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly, I'm going with the "monkey see, monkey do" strategy - my sense of where a
.hshould be included is not developed yet. That strategy has one virtue, though: it is consistent (good or bad).If you can tell me what is the strategy here, I can rather move all the offending includes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a great strategy :) My general philosophy is to only put an include in the .h file if it's needed for the .h file itself. In this case, it appears that the only "code" in the .h file is a declaration of a function with no arguments that returns void. So I would think that it shouldn't need any includes. Does it still build if you move all of 'em?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved them all - thanks!