Cronvoy: Map EM Errors to Cronet API Errors (#1594)

bazel/envoy_mobile_test_extensions.bzl

@@ -6,4 +6,5 @@ TEST_EXTENSIONS = [
     "//library/common/extensions/filters/http/test_accessor:config",
     "//library/common/extensions/filters/http/test_event_tracker:config",
     "//library/common/extensions/filters/http/test_kv_store:config",
+    "//library/common/extensions/filters/http/test_read:config",
 ]

library/common/extensions/filters/http/test_read/BUILD

@@ -0,0 +1,42 @@
+load(
+    "@envoy//bazel:envoy_build_system.bzl",
+    "envoy_cc_extension",
+    "envoy_extension_package",
+    "envoy_proto_library",
+)
+
+licenses(["notice"])  # Apache 2
+
+envoy_extension_package()
+
+envoy_proto_library(
+    name = "filter",
+    srcs = ["filter.proto"],
+    deps = [
+        "@envoy_api//envoy/config/common/matcher/v3:pkg",
+    ],
+)
+
+envoy_cc_extension(
+    name = "test_read_filter_lib",
+    srcs = ["filter.cc"],
+    hdrs = ["filter.h"],
+    repository = "@envoy",
+    deps = [
+        "filter_cc_proto",
+        "@envoy//source/common/http:utility_lib",
+        "@envoy//source/common/stream_info:stream_info_lib",
+        "@envoy//source/extensions/filters/http/common:pass_through_filter_lib",
+    ],
+)
+
+envoy_cc_extension(
+    name = "config",
+    srcs = ["config.cc"],
+    hdrs = ["config.h"],
+    repository = "@envoy",
+    deps = [
+        ":test_read_filter_lib",
+        "@envoy//source/extensions/filters/http/common:factory_base_lib",
+    ],
+)

library/common/extensions/filters/http/test_read/config.cc

@@ -0,0 +1,26 @@
+#include "library/common/extensions/filters/http/test_read/config.h"
+
+#include "library/common/extensions/filters/http/test_read/filter.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace HttpFilters {
+namespace TestRead {
+
+Http::FilterFactoryCb TestReadFilterFactory::createFilterFactoryFromProtoTyped(
+    const envoymobile::extensions::filters::http::test_read::TestRead& /*config*/,
+    const std::string&, Server::Configuration::FactoryContext& /*context*/) {
+  return [](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+    callbacks.addStreamDecoderFilter(std::make_shared<TestReadFilter>());
+  };
+}
+
+/**
+ * Static registration for the TestRead filter. @see NamedHttpFilterConfigFactory.
+ */
+REGISTER_FACTORY(TestReadFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory);
+
+} // namespace TestRead
+} // namespace HttpFilters
+} // namespace Extensions
+} // namespace Envoy

library/common/extensions/filters/http/test_read/config.h

@@ -0,0 +1,34 @@
+#pragma once
+
+#include <string>
+
+#include "source/extensions/filters/http/common/factory_base.h"
+
+#include "library/common/extensions/filters/http/test_read/filter.pb.h"
+#include "library/common/extensions/filters/http/test_read/filter.pb.validate.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace HttpFilters {
+namespace TestRead {
+
+/**
+ * Config registration for the TestRead filter. @see NamedHttpFilterConfigFactory.
+ */
+class TestReadFilterFactory
+    : public Common::FactoryBase<envoymobile::extensions::filters::http::test_read::TestRead> {
+public:
+  TestReadFilterFactory() : FactoryBase("test_read") {}
+
+private:
+  ::Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
+      const envoymobile::extensions::filters::http::test_read::TestRead& config,
+      const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;
+};
+
+DECLARE_FACTORY(TestReadFilterFactory);
+
+} // namespace TestRead
+} // namespace HttpFilters
+} // namespace Extensions
+} // namespace Envoy

library/common/extensions/filters/http/test_read/filter.cc

@@ -0,0 +1,46 @@
+#include "library/common/extensions/filters/http/test_read/filter.h"
+
+#include "envoy/server/filter_config.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace HttpFilters {
+namespace TestRead {
+
+Http::FilterHeadersStatus TestReadFilter::decodeHeaders(Http::RequestHeaderMap& request_headers,
+                                                        bool) {
+  // sample path is /failed?start=0x10000
+  Http::Utility::QueryParams query_parameters =
+      Http::Utility::parseQueryString(request_headers.Path()->value().getStringView());
+  uint64_t response_flag;
+  if (absl::SimpleAtoi(query_parameters.at("start"), &response_flag)) {
+    decoder_callbacks_->streamInfo().setResponseFlag(
+        TestReadFilter::mapErrorToResponseFlag(response_flag));
+    decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, "test_read filter threw: ", nullptr,
+                                       absl::nullopt, "");
+  }
+  return Http::FilterHeadersStatus::StopIteration;
+}
+
+StreamInfo::ResponseFlag TestReadFilter::mapErrorToResponseFlag(uint64_t errorCode) {
+  switch (errorCode) {
+  case 0x4000000:
+    return StreamInfo::DnsResolutionFailed;
+  case 0x40:
+    return StreamInfo::UpstreamConnectionTermination;
+  case 0x20:
+    return StreamInfo::UpstreamConnectionFailure;
+  case 0x10:
+    return StreamInfo::UpstreamRemoteReset;
+  case 0x10000:
+    return StreamInfo::StreamIdleTimeout;
+  default:
+    // Any other error that we aren't interested in. I picked a random error.
+    return StreamInfo::RateLimitServiceError;
+  }
+}
+
+} // namespace TestRead
+} // namespace HttpFilters
+} // namespace Extensions
+} // namespace Envoy

library/common/extensions/filters/http/test_read/filter.h

@@ -0,0 +1,35 @@
+#pragma once
+
+#include "envoy/http/filter.h"
+
+#include "source/common/common/logger.h"
+#include "source/common/http/utility.h"
+#include "source/common/stream_info/stream_info_impl.h"
+#include "source/extensions/filters/http/common/pass_through_filter.h"
+
+#include "library/common/extensions/filters/http/test_read/filter.pb.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace HttpFilters {
+namespace TestRead {
+
+/**
+ * Filter to return specified error code based on a request header.
+ */
+class TestReadFilter final : public Http::PassThroughFilter,
+                             public Logger::Loggable<Logger::Id::filter> {
+public:
+  Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& request_headers, bool) override;
+
+private:
+  /* A mapping of the envoymobile errors we care about for testing
+   * From https://github.com/envoyproxy/envoy/blob/main/envoy/stream_info/stream_info.h
+   */
+  StreamInfo::ResponseFlag mapErrorToResponseFlag(uint64_t errorCode);
+};
+
+} // namespace TestRead
+} // namespace HttpFilters
+} // namespace Extensions
+} // namespace Envoy

library/common/extensions/filters/http/test_read/filter.proto

@@ -0,0 +1,8 @@
+syntax = "proto3";
+
+package envoymobile.extensions.filters.http.test_read;
+
+import "validate/validate.proto";
+
+message TestRead {
+}

library/java/org/chromium/net/impl/Annotations.java

@@ -19,16 +19,6 @@ public final class Annotations {
     int HIGHEST = 5;
   }
 
-  /** Subset of errors defined in chromium/src/net/base/net_error_list.h */
-  @IntDef({NetError.ERR_HTTP2_PING_FAILED, NetError.ERR_QUIC_HANDSHAKE_FAILED})
-  @Retention(RetentionPolicy.SOURCE)
-  public @interface NetError {
-    int ERR_NETWORK_CHANGED = -21;
-    int ERR_HTTP2_PING_FAILED = -352;
-    int ERR_QUIC_PROTOCOL_ERROR = -356;
-    int ERR_QUIC_HANDSHAKE_FAILED = -358;
-  }
-
   /** Enum defined here: chromium/src/components/cronet/url_request_context_config.h, line 37 */
   @IntDef({HttpCacheType.DISABLED, HttpCacheType.DISK, HttpCacheType.MEMORY})
   @Retention(RetentionPolicy.SOURCE)

library/java/org/chromium/net/impl/BUILD

@@ -24,6 +24,7 @@ android_library(
         "CronetUploadDataStream.java",
         "CronetUrlRequest.java",
         "CronetUrlRequestContext.java",
+        "Errors.java",
         "Executors.java",
         "HttpReason.java",
         "ImplVersion.java",

library/java/org/chromium/net/impl/BidirectionalStreamNetworkException.java

@@ -1,6 +1,6 @@
 package org.chromium.net.impl;
 
-import org.chromium.net.impl.Annotations.NetError;
+import org.chromium.net.impl.Errors.NetError;
 
 /**
  * Used in {@link CronetBidirectionalStream}. Implements {@link NetworkExceptionImpl}.
@@ -13,13 +13,11 @@ public BidirectionalStreamNetworkException(String message, int errorCode,
 
   @Override
   public boolean immediatelyRetryable() {
-    switch (mCronetInternalErrorCode) {
-    case NetError.ERR_HTTP2_PING_FAILED:
-    case NetError.ERR_QUIC_HANDSHAKE_FAILED:
+    if (mCronetInternalErrorCode == NetError.ERR_HTTP2_PING_FAILED.getErrorCode() ||
+        mCronetInternalErrorCode == NetError.ERR_QUIC_HANDSHAKE_FAILED.getErrorCode()) {
       assert mErrorCode == ERROR_OTHER;
       return true;
-    default:
-      return super.immediatelyRetryable();
     }
+    return super.immediatelyRetryable();
   }
 }

library/java/org/chromium/net/impl/CronetBidirectionalStream.java

@@ -1,5 +1,9 @@
 package org.chromium.net.impl;
 
+import static org.chromium.net.impl.Errors.isQuicException;
+import static org.chromium.net.impl.Errors.mapEnvoyMobileErrorToNetError;
+import static org.chromium.net.impl.Errors.mapNetErrorToCronetApiErrorCode;
+
 import android.util.Log;
 
 import androidx.annotation.Nullable;
@@ -15,6 +19,7 @@
 import org.chromium.net.impl.Annotations.RequestPriority;
 import org.chromium.net.impl.CronetBidirectionalState.Event;
 import org.chromium.net.impl.CronetBidirectionalState.NextAction;
+import org.chromium.net.impl.Errors.NetError;
 import org.chromium.net.impl.UrlResponseInfoImpl.HeaderBlockImpl;
 
 import java.net.MalformedURLException;
@@ -643,21 +648,23 @@ public void run() {
     });
   }
 
-  private void onErrorReceived(int errorCode, int nativeError, int nativeQuicError,
-                               String errorString, long receivedByteCount) {
+  private void onErrorReceived(int errorCode, EnvoyFinalStreamIntel finalStreamIntel) {
     if (mResponseInfo != null) {
-      mResponseInfo.setReceivedByteCount(receivedByteCount);
+      mResponseInfo.setReceivedByteCount(finalStreamIntel.getReceivedByteCount());
     }
-    CronetException exception;
-    if (errorCode == NetworkException.ERROR_QUIC_PROTOCOL_FAILED ||
-        errorCode == NetworkException.ERROR_NETWORK_CHANGED) {
-      exception = new QuicExceptionImpl("Exception in BidirectionalStream: " + errorString,
-                                        errorCode, nativeError, nativeQuicError);
+
+    NetError netError = mapEnvoyMobileErrorToNetError(finalStreamIntel.getResponseFlags());
+    int javaError = mapNetErrorToCronetApiErrorCode(netError);
+
+    if (isQuicException(javaError)) {
+      mException.set(new QuicExceptionImpl("Exception in BidirectionalStream: " + netError,
+                                           javaError, netError.getErrorCode(),
+                                           /*nativeQuicError*/ 0));
     } else {
-      exception = new BidirectionalStreamNetworkException(
-          "Exception in BidirectionalStream: " + errorString, errorCode, nativeError);
+      mException.set(new BidirectionalStreamNetworkException(
+          "Exception in BidirectionalStream: " + netError, javaError, netError.getErrorCode()));
     }
-    mException.set(exception);
+
     failWithException();
   }
 
@@ -1031,9 +1038,7 @@ public void onError(int errorCode, String message, int attemptCount, EnvoyStream
     mEnvoyFinalStreamIntel = finalStreamIntel;
     switch (mState.nextAction(Event.ON_ERROR)) {
     case NextAction.NOTIFY_USER_NETWORK_ERROR:
-      // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1594): fix error scheme.
-      onErrorReceived(errorCode, /* nativeError= */ -1,
-                      /* nativeQuicError */ 0, message, finalStreamIntel.getReceivedByteCount());
+      onErrorReceived(errorCode, finalStreamIntel);
       break;
     case NextAction.NOTIFY_USER_FAILED:
       // There was already an error in-progress - the network error came too late and is ignored.

library/java/org/chromium/net/impl/CronetUrlRequest.java

@@ -1,5 +1,9 @@
 package org.chromium.net.impl;
 
+import static org.chromium.net.impl.Errors.isQuicException;
+import static org.chromium.net.impl.Errors.mapEnvoyMobileErrorToNetError;
+import static org.chromium.net.impl.Errors.mapNetErrorToCronetApiErrorCode;
+
 import android.os.ConditionVariable;
 import android.util.Log;
 import androidx.annotation.IntDef;
@@ -30,9 +34,11 @@
 import org.chromium.net.CallbackException;
 import org.chromium.net.CronetException;
 import org.chromium.net.InlineExecutionProhibitedException;
+import org.chromium.net.NetworkException;
 import org.chromium.net.RequestFinishedInfo;
 import org.chromium.net.RequestFinishedInfo.Metrics;
 import org.chromium.net.UploadDataProvider;
+import org.chromium.net.impl.Errors.NetError;
 
 /** UrlRequest, backed by Envoy-Mobile. */
 public final class CronetUrlRequest extends UrlRequestBase {
@@ -448,6 +454,7 @@ private static int determineNextErrorState(boolean streamEnded, @State int origi
     }
   }
 
+  // No-op if already in a terminal state.
   private void enterErrorState(CronetException error) {
     @State int originalState;
     @State int updatedState;
@@ -927,10 +934,18 @@ public void onError(int errorCode, String message, int attemptCount,
         return;
       }
 
-      String errorMessage = "failed with error after " + attemptCount + " attempts. Message=[" +
-                            message + "] Code=[" + errorCode + "]";
-      CronetException exception = new CronetExceptionImpl(errorMessage, /* cause= */ null);
-      enterErrorState(exception); // No-op if already in a terminal state.
+      NetError netError = mapEnvoyMobileErrorToNetError(finalStreamIntel.getResponseFlags());
+      int javaError = mapNetErrorToCronetApiErrorCode(netError);
+
+      if (isQuicException(javaError)) {
+        enterErrorState(new QuicExceptionImpl("Exception in CronetUrlRequest: " + netError,
+                                              javaError, netError.getErrorCode(),
+                                              /*nativeQuicError*/ 0));
+        return;
+      }
+
+      enterErrorState(new NetworkExceptionImpl("Exception in CronetUrlRequest: " + netError,
+                                               javaError, netError.getErrorCode()));
     }
 
     @Override