android: move common yaml configuration down to java layer by buildbreaker · Pull Request #385 · envoyproxy/envoy-mobile

buildbreaker · 2019-08-26T22:01:06Z

A small structural change to the configuration and builder relationship since we are unable to read the default configuration provided by the native layer without initializing the JNI library.

The change here is to move most of the configuration resolution down to the Engine layer since it is responsible for the JNI library interaction.

Signed-off-by: Alan Chiu achiu@lyft.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: move common yaml configuration down to java layer
Risk Level: low
Testing: unit
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

goaway · 2019-08-27T21:31:25Z

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

+  public final int dnsRefreshSeconds;
+  public final int statsFlushSeconds;
+
+  public EnvoyConfiguration(String configYAML, int connectTimeoutSeconds, int dnsRefreshSeconds,


I would make two constructors - one which takes just a yaml and another which takes the other parameters. The one which takes only yaml, should produce a configuration that is not interpolated, but rather is passed through directly. In other words, resolve should just pass-through a configYAML iff it's present.

Talked offline with @buildbreaker about this and I agree with the 2-initializer approach. A couple other things to note though:

The initializer that takes the configYAML (pre-resolved) will not be used today, but will act as a convenience

The motivation for this PR/change is to defer calling into the JNI to get the template string until after the JNI is loaded (i.e., when resolve is called). Can we inline some documentation for this here @buildbreaker?

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

rebello95

I can make the equivalent change on iOS as well. Some comments.

rebello95 · 2019-09-03T17:29:33Z

library/kotlin/src/io/envoyproxy/envoymobile/EnvoyBuilder.kt

   */
  fun build(): Envoy {
-    return Envoy(engineType(), resolvedYAML(), logLevel)
+    return Envoy(engineType(), EnvoyConfiguration(configYAML, connectTimeoutSeconds, dnsRefreshSeconds, statsFlushSeconds), logLevel)


Should we abstract the EnvoyConfiguration into a private/internal function so we can test that it's properly initialized?

I don't quite understand what you have in mind here. Do you mean to have a way for us to create an EnvoyConfiguration with the appropriate values from the builder: initialize EnvoyConfiguration with a custom configuration versus with the connectTimeoutSeconds, dnsRefreshSeconds, statsFlushSeconds values?

Discussed further offline and we will:

Have EnvoyConfiguration only be used for template-based configurations and avoid conflating custom YAML configs with the template resolution

Add a constructor to Envoy Engine so we have one that takes an EnvoyConfiguration (which resolves properties to the default template) or a custom string YAML file

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

rebello95 · 2019-09-03T18:41:25Z

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

+  public final int dnsRefreshSeconds;
+  public final int statsFlushSeconds;
+
+  public EnvoyConfiguration(String configYAML, int connectTimeoutSeconds, int dnsRefreshSeconds,


Talked offline with @buildbreaker about this and I agree with the 2-initializer approach. A couple other things to note though:

The initializer that takes the configYAML (pre-resolved) will not be used today, but will act as a convenience

The motivation for this PR/change is to defer calling into the JNI to get the template string until after the JNI is loaded (i.e., when resolve is called). Can we inline some documentation for this here @buildbreaker?

Signed-off-by: Alan Chiu <achiu@lyft.com>

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

Signed-off-by: Alan Chiu <achiu@lyft.com>

rebello95

LGTM overall, some final comments

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngine.java

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java

rebello95 · 2019-09-04T16:17:47Z

library/java/test/io/envoyproxy/envoymobile/engine/EnvoyConfigurationTest.kt

+    val resolvedTemplate = envoyConfiguration.resolveTemplate(TEST_CONFIG)
+    assertThat(resolvedTemplate).contains("connect_timeout: 123s")
+    assertThat(resolvedTemplate).contains("dns_refresh_rate: 234s")
+    assertThat(resolvedTemplate).contains("stats_flush_interval: 345s")


Maybe also assert that the template doesn't contain any {{?

The behavior that is baked in the method is that if it contains any unresolved templates, we'll throw an exception so that test is: resolve templates with invalid templates will throw on build

rebello95 · 2019-09-04T16:19:34Z

library/kotlin/src/io/envoyproxy/envoymobile/EnvoyBuilder.kt

   */
  fun build(): Envoy {
-    return Envoy(engineType(), resolvedYAML(), logLevel)
+    val configurationYAML = configYAML


Not sure this really provides any benefit 🤷‍♂

It's a variable and the class isn't thread safe so the field could be changed between the lines of code. This way at the start of the method call we are guaranteed to have the correct configuration yaml...

Basically, I get it's silly but the IDE was complaining to me so I changed it :(

Signed-off-by: Alan Chiu <achiu@lyft.com>

goaway · 2019-09-04T20:37:30Z

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java

+   * @return String, the resolved template.
+   * @throws ConfigurationException, when the template provided is not fully resolved.
+   */
+  String resolveTemplate(String templateYAML) {


Thoughts on moving the logic in this function into EnvoyConfigurationImpl (could be a helper method called out of runWithConfig)? That way the configuration really doesn't need to know or care about any implementation details of how its attributes are used.

I can create another file which holds this method and just port the unit tests there -- it'll make this class a pure data class then. Wdyt?

cc: @rebello95

Sure, I'm good with that

Actually, let's revisit at a later date on this 😬

goaway

Overall this looks great. One suggestion above.

This is a mirror of the Android change made in #385. Signed-off-by: Michael Rebello <me@michaelrebello.com>

This is a mirror of the Android change made in #385. The change was originally motivated by the fact that Android was unable to obtain the configuration template within C++ before the JNI was loaded. Moving the template resolution logic to Java fixed this issue by allowing the JNI to be initialized before loading the template. Key changes in this PR: - Moved template resolution to Objective-C - Custom YAML files are no longer treated as templates, and are not resolved (but are passed through as-is). This matches Android - Updated initializers as necessary to match Android Signed-off-by: Michael Rebello <me@michaelrebello.com>

This is a mirror of the Android change made in envoyproxy/envoy-mobile#385. The change was originally motivated by the fact that Android was unable to obtain the configuration template within C++ before the JNI was loaded. Moving the template resolution logic to Java fixed this issue by allowing the JNI to be initialized before loading the template. Key changes in this PR: - Moved template resolution to Objective-C - Custom YAML files are no longer treated as templates, and are not resolved (but are passed through as-is). This matches Android - Updated initializers as necessary to match Android Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

goaway reviewed Aug 27, 2019

View reviewed changes

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java Outdated Show resolved Hide resolved

rebello95 reviewed Sep 3, 2019

View reviewed changes

buildbreaker marked this pull request as ready for review September 3, 2019 20:07

Alan Chiu added 12 commits September 3, 2019 13:13

android: move yaml configuration down to java layer

1c57036

Signed-off-by: Alan Chiu <achiu@lyft.com>

clean up

57606e9

Signed-off-by: Alan Chiu <achiu@lyft.com>

new line

ee7b4ab

Signed-off-by: Alan Chiu <achiu@lyft.com>

push

50da5fb

Signed-off-by: Alan Chiu <achiu@lyft.com>

fix

cfed93a

Signed-off-by: Alan Chiu <achiu@lyft.com>

format

b008e8e

Signed-off-by: Alan Chiu <achiu@lyft.com>

clena up and add tests

ecdf0fd

Signed-off-by: Alan Chiu <achiu@lyft.com>

fix

1c37fb1

Signed-off-by: Alan Chiu <achiu@lyft.com>

OOOOPS

3272f7e

Signed-off-by: Alan Chiu <achiu@lyft.com>

period

f94cb71

Signed-off-by: Alan Chiu <achiu@lyft.com>

spellcheck

bf2bca6

Signed-off-by: Alan Chiu <achiu@lyft.com>

fix things

af59361

Signed-off-by: Alan Chiu <achiu@lyft.com>

buildbreaker force-pushed the ac/move-configuration branch from b077bd1 to af59361 Compare September 3, 2019 20:30

buildbreaker changed the title ~~android: move yaml configuration down to java layer~~ android: move common yaml configuration down to java layer Sep 4, 2019

rebello95 reviewed Sep 4, 2019

View reviewed changes

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java Outdated Show resolved Hide resolved

rebello95 reviewed Sep 4, 2019

View reviewed changes

library/java/src/io/envoyproxy/envoymobile/engine/EnvoyConfiguration.java Outdated Show resolved Hide resolved

missed deletion

075ddea

Signed-off-by: Alan Chiu <achiu@lyft.com>

rebello95 reviewed Sep 4, 2019

View reviewed changes

pr feedback

fdf2442

Signed-off-by: Alan Chiu <achiu@lyft.com>

rebello95 previously approved these changes Sep 4, 2019

View reviewed changes

cool

9e326dd

Signed-off-by: Alan Chiu <achiu@lyft.com>

buildbreaker dismissed rebello95’s stale review via 9e326dd September 4, 2019 17:46

rebello95 previously approved these changes Sep 4, 2019

View reviewed changes

fix

1dddd8e

Signed-off-by: Alan Chiu <achiu@lyft.com>

buildbreaker dismissed rebello95’s stale review via 1dddd8e September 4, 2019 20:24

fix

3cdc7d5

Signed-off-by: Alan Chiu <achiu@lyft.com>

goaway reviewed Sep 4, 2019

View reviewed changes

rebello95 approved these changes Sep 4, 2019

View reviewed changes

buildbreaker merged commit b9b1370 into master Sep 4, 2019

buildbreaker deleted the ac/move-configuration branch September 4, 2019 22:39

rebello95 added a commit that referenced this pull request Sep 6, 2019

ios: move configuration to Objective-C

4496b33

This is a mirror of the Android change made in #385. Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 mentioned this pull request Sep 6, 2019

ios: move configuration to Objective-C #420

Merged

Conversation

buildbreaker commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goaway left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

buildbreaker commented Aug 26, 2019 •

edited

Loading