-
Notifications
You must be signed in to change notification settings - Fork 85
android: move common yaml configuration down to java layer #385
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 all commits
1c57036
57606e9
ee7b4ab
50da5fb
cfed93a
b008e8e
ecdf0fd
1c37fb1
3272f7e
f94cb71
bf2bca6
af59361
075ddea
fdf2442
9e326dd
1dddd8e
3cdc7d5
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 |
|---|---|---|
| @@ -1,11 +1,46 @@ | ||
| package io.envoyproxy.envoymobile.engine; | ||
|
|
||
| public interface EnvoyConfiguration { | ||
| public class EnvoyConfiguration { | ||
|
|
||
| public final Integer connectTimeoutSeconds; | ||
| public final Integer dnsRefreshSeconds; | ||
| public final Integer statsFlushSeconds; | ||
|
|
||
| /** | ||
| * Provides a default configuration template that may be used for starting Envoy. | ||
| * Create an EnvoyConfiguration with a user provided configuration values. | ||
| * | ||
| * @return A template that may be used as a starting point for constructing configurations. | ||
| * @param connectTimeoutSeconds timeout for new network connections to hosts in the cluster. | ||
| * @param dnsRefreshSeconds rate in seconds to refresh DNS. | ||
| * @param statsFlushSeconds interval at which to flush Envoy stats. | ||
| */ | ||
| String templateString(); | ||
| public EnvoyConfiguration(int connectTimeoutSeconds, int dnsRefreshSeconds, | ||
| int statsFlushSeconds) { | ||
| this.connectTimeoutSeconds = connectTimeoutSeconds; | ||
| this.dnsRefreshSeconds = dnsRefreshSeconds; | ||
| this.statsFlushSeconds = statsFlushSeconds; | ||
| } | ||
|
|
||
| /** | ||
| * Resolves the provided configuration template using properties on this configuration. | ||
| * This default configuration is provided by the native layer. | ||
| * | ||
| * @param templateYAML the default template configuration. | ||
| * @return String, the resolved template. | ||
| * @throws ConfigurationException, when the template provided is not fully resolved. | ||
| */ | ||
| String resolveTemplate(String templateYAML) { | ||
| String resolvedConfiguration = | ||
| templateYAML.replace("{{ connect_timeout }}", String.format("%ss", connectTimeoutSeconds)) | ||
| .replace("{{ dns_refresh_rate }}", String.format("%ss", dnsRefreshSeconds)) | ||
| .replace("{{ stats_flush_interval }}", String.format("%ss", statsFlushSeconds)); | ||
|
|
||
| if (resolvedConfiguration.contains("{{")) { | ||
| throw new ConfigurationException(); | ||
| } | ||
| return resolvedConfiguration; | ||
| } | ||
|
|
||
| static class ConfigurationException extends RuntimeException { | ||
| ConfigurationException() { super("Unresolved Template Key"); } | ||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load("//bazel:kotlin_test.bzl", "envoy_mobile_kt_test") | ||
|
|
||
| envoy_mobile_kt_test( | ||
| name = "envoy_configuration_test", | ||
| srcs = [ | ||
| "EnvoyConfigurationTest.kt", | ||
| ], | ||
| deps = [ | ||
| "//library/kotlin/src/io/envoyproxy/envoymobile:envoy_interfaces_lib", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package io.envoyproxy.envoymobile.engine | ||
|
|
||
| import org.junit.Test | ||
|
|
||
| private const val TEST_CONFIG = """ | ||
| mock_template: | ||
| - name: mock | ||
| connect_timeout: {{ connect_timeout }} | ||
| dns_refresh_rate: {{ dns_refresh_rate }} | ||
| stats_flush_interval: {{ stats_flush_interval }} | ||
| """ | ||
|
|
||
|
|
||
| class EnvoyConfigurationTest { | ||
|
|
||
| @Test | ||
| fun `resolving with default configuration resolves with values`() { | ||
| val envoyConfiguration = EnvoyConfiguration(123, 234, 345) | ||
|
|
||
| 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") | ||
|
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. Maybe also assert that the template doesn't contain any
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. 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: |
||
| } | ||
|
|
||
|
|
||
| @Test(expected = ConfigurationException::class) | ||
| fun `resolve templates with invalid templates will throw on build`() { | ||
| val envoyConfiguration = EnvoyConfiguration(123, 234, 345) | ||
|
|
||
| envoyConfiguration.resolveTemplate("{{ }}") | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,20 @@ | ||
| package io.envoyproxy.envoymobile | ||
|
|
||
| import io.envoyproxy.envoymobile.engine.EnvoyConfiguration | ||
| import io.envoyproxy.envoymobile.engine.EnvoyConfigurationImpl | ||
| import io.envoyproxy.envoymobile.engine.EnvoyEngine | ||
| import io.envoyproxy.envoymobile.engine.EnvoyEngineImpl | ||
|
|
||
| class ConfigurationException : Exception("Unresolved Template Key") | ||
|
|
||
| open class EnvoyBuilder internal constructor( | ||
| private val envoyConfiguration: EnvoyConfiguration | ||
| ) { | ||
| private var logLevel = LogLevel.INFO | ||
| private var configYAML: String | ||
| private var configYAML: String? = null | ||
| private var engineType: () -> EnvoyEngine = { EnvoyEngineImpl() } | ||
|
|
||
| private var connectTimeoutSeconds = 30 | ||
| private var dnsRefreshSeconds = 60 | ||
| private var statsFlushSeconds = 60 | ||
|
|
||
| constructor() : this(EnvoyConfigurationImpl()) | ||
|
|
||
| init { | ||
| configYAML = envoyConfiguration.templateString() | ||
| } | ||
|
|
||
| /** | ||
| * Add a log level to use with Envoy. | ||
| * @param logLevel the log level to use with Envoy. | ||
|
|
@@ -40,7 +31,7 @@ open class EnvoyBuilder internal constructor( | |
| * @param configYAML the YAML file to use as a configuration. | ||
| */ | ||
| fun addConfigYAML(configYAML: String?): EnvoyBuilder { | ||
| this.configYAML = configYAML ?: envoyConfiguration.templateString() | ||
| this.configYAML = configYAML | ||
| return this | ||
| } | ||
|
|
||
|
|
@@ -80,7 +71,12 @@ open class EnvoyBuilder internal constructor( | |
| * @return A new instance of Envoy. | ||
| */ | ||
| fun build(): Envoy { | ||
| return Envoy(engineType(), resolvedYAML(), logLevel) | ||
| val configurationYAML = configYAML | ||
|
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. Not sure this really provides any benefit 🤷♂
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. 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 :( |
||
| if (configurationYAML == null) { | ||
| return Envoy(engineType(), EnvoyConfiguration(connectTimeoutSeconds, dnsRefreshSeconds, statsFlushSeconds), logLevel) | ||
| } else { | ||
| return Envoy(engineType(), configurationYAML, logLevel) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -92,24 +88,4 @@ open class EnvoyBuilder internal constructor( | |
| this.engineType = engineType | ||
| return this | ||
| } | ||
|
|
||
|
|
||
| /** Processes the YAML template provided, replacing keys with values from the configuration. | ||
| * | ||
| * @parameter template: The template YAML file to use. | ||
| * @returns: A resolved YAML file with all template keys replaced. | ||
| * @throws ConfigurationException when the yaml configuration replacement is incomplete | ||
| */ | ||
| private fun resolvedYAML(): String { | ||
| val resolvedTemplate = configYAML | ||
| .replace("{{ connect_timeout }}", "${connectTimeoutSeconds}s") | ||
| .replace("{{ dns_refresh_rate }}", "${dnsRefreshSeconds}s") | ||
| .replace("{{ stats_flush_interval }}", "${statsFlushSeconds}s") | ||
|
|
||
| if (resolvedTemplate.contains("{{")) { | ||
| throw ConfigurationException() | ||
| } | ||
|
|
||
| return resolvedTemplate | ||
| } | ||
| } | ||
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.
Thoughts on moving the logic in this function into
EnvoyConfigurationImpl(could be a helper method called out ofrunWithConfig)? That way the configuration really doesn't need to know or care about any implementation details of how its attributes are used.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 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
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.
Sure, I'm good with that
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.
Actually, let's revisit at a later date on this 😬