tech debt: enforce agreement of engine return codes (#966)

Description: Introduces extern consts to enforce agreement between return codes produced by the engine. Note the JNI is currently an exception, but could be runtime enforced with an additional one-time validation step. Risk: Low Testing: Local Signed-off-by: Mike Schore <[email protected]>
envoyproxy · Jul 22, 2020 · 2a1b534 · 2a1b534
1 parent c6e1af6
commit 2a1b534
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 7 deletions.
diff --git a/STYLE.md b/STYLE.md
@@ -34,3 +34,11 @@ Additionally it can be installed as a commit hook with `pre-commit install`.
 - Swift code style is validated using [SwiftLint](https://github.com/realm/swiftlint)
 - The rules enforced are available in the repo's [.swiftlint.yml file](./.swiftlint.yml)
 - The linter may be run locally using `swiftlint` or auto-corrected with `swiftlint autocorrect`
+
+## Shared constructs
+
+- There's no directly supported way to universally share an enumeration across platforms. In order
+to provide some enforced consistency, we've adopted the convention of defining the enum at the
+lowest applicable layer (core/bridge) of the library, and then declaring public `extern const`
+values defined in terms of the enumeration, to be shared across bridge and platform code. See,
+for example: https://github.com/lyft/envoy-mobile/blob/main/library/common/types/c_types.h#L25
diff --git a/library/common/types/c_types.cc b/library/common/types/c_types.cc
@@ -5,6 +5,9 @@
 
 #include "common/common/assert.h"
 
+const int kEnvoySuccess = ENVOY_SUCCESS;
+const int kEnvoyFailure = ENVOY_FAILURE;
+
 void* safe_malloc(size_t size) {
   void* ptr = malloc(size);
   if (size > 0) {

diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h
@@ -22,7 +22,16 @@ typedef intptr_t envoy_stream_t;
 /**
  * Result codes returned by all calls made to this interface.
  */
-typedef enum { ENVOY_SUCCESS, ENVOY_FAILURE } envoy_status_t;
+typedef enum {
+  ENVOY_SUCCESS = 0,
+  ENVOY_FAILURE = 1,
+} envoy_status_t;
+
+/**
+ * Equivalent constants to envoy_status_t, for contexts where the enum may not be usable.
+ */
+extern const int kEnvoySuccess;
+extern const int kEnvoyFailure;
 
 /**
  * Error code associated with terminal status of a HTTP stream.

diff --git a/library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java b/library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java
@@ -4,6 +4,10 @@
 
 /* Concrete implementation of the `EnvoyEngine` interface. */
 public class EnvoyEngineImpl implements EnvoyEngine {
+  // TODO(goaway): enforce agreement values in /library/common/types/c_types.h.
+  private static final int ENVOY_SUCCESS = 0;
+  private static final int ENVOY_FAILURE = 1;
+
   private final long engineHandle;
 
   public EnvoyEngineImpl() {
@@ -37,7 +41,7 @@ public int runWithConfig(String configurationYAML, String logLevel) {
       return JniLibrary.runEngine(this.engineHandle, configurationYAML, logLevel);
     } catch (Throwable throwable) {
       // TODO: Need to have a way to log the exception somewhere.
-      return 1;
+      return ENVOY_FAILURE;
     }
   }
 

diff --git a/library/objective-c/EnvoyEngine.h b/library/objective-c/EnvoyEngine.h
@@ -175,6 +175,10 @@ extern const int kEnvoyFilterHeadersStatusStopAllIterationAndBuffer;
 
 #pragma mark - EnvoyEngine
 
+/// Return codes for Engine interface. @see /library/common/types/c_types.h
+extern const int kEnvoySuccess;
+extern const int kEnvoyFailure;
+
 /// Wrapper layer for calling into Envoy's C/++ API.
 @protocol EnvoyEngine
 

diff --git a/library/objective-c/EnvoyEngineImpl.m b/library/objective-c/EnvoyEngineImpl.m
@@ -132,14 +132,14 @@ - (int)registerFilter:(EnvoyHTTPFilter *)filter {
   api->on_response_data = NULL;
   api->context = context;
   register_platform_api(filter.name.UTF8String, api);
-  return 0;
+  return kEnvoySuccess;
 }
 
 - (int)runWithConfig:(EnvoyConfiguration *)config logLevel:(NSString *)logLevel {
   NSString *templateYAML = [[NSString alloc] initWithUTF8String:config_template];
   NSString *resolvedYAML = [config resolveTemplate:templateYAML];
   if (resolvedYAML == nil) {
-    return 1;
+    return kEnvoyFailure;
   }
 
   for (EnvoyHTTPFilter *filter in config.httpFilters) {
@@ -162,7 +162,7 @@ - (int)runWithConfigYAML:(NSString *)configYAML logLevel:(NSString *)logLevel {
   } @catch (NSException *exception) {
     NSLog(@"[Envoy] exception caught: %@", exception);
     [NSNotificationCenter.defaultCenter postNotificationName:@"EnvoyError" object:self];
-    return 1;
+    return kEnvoyFailure;
   }
 }
 

diff --git a/library/swift/src/mocks/MockEnvoyEngine.swift b/library/swift/src/mocks/MockEnvoyEngine.swift
@@ -12,12 +12,12 @@ final class MockEnvoyEngine: NSObject {
 extension MockEnvoyEngine: EnvoyEngine {
   func run(withConfig config: EnvoyConfiguration, logLevel: String) -> Int32 {
     MockEnvoyEngine.onRunWithConfig?(config, logLevel)
-    return 0
+    return kEnvoySuccess
   }
 
   func run(withConfigYAML configYAML: String, logLevel: String) -> Int32 {
     MockEnvoyEngine.onRunWithYAML?(configYAML, logLevel)
-    return 0
+    return kEnvoySuccess
   }
 
   func startStream(with callbacks: EnvoyHTTPCallbacks) -> EnvoyHTTPStream {