From b53c5061952fcf87e4416651539cf18dab68584d Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Tue, 17 Jan 2023 13:57:10 -0800 Subject: [PATCH 01/11] init commit of flags.yml --- configs/flags.yml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 configs/flags.yml diff --git a/configs/flags.yml b/configs/flags.yml new file mode 100644 index 0000000000000..b5cc273ca2706 --- /dev/null +++ b/configs/flags.yml @@ -0,0 +1,3 @@ +flags: + - name: performance.backgroundJsonSchemaValidation + enabled: true From f42cd3a40cdf435233548c9b9936746850a4c4fa Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Tue, 17 Jan 2023 15:45:02 -0800 Subject: [PATCH 02/11] add PerfBackgroundJsonValidation flag --- airbyte-featureflag/src/main/kotlin/Flags.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/airbyte-featureflag/src/main/kotlin/Flags.kt b/airbyte-featureflag/src/main/kotlin/Flags.kt index bfc1dd0a6a820..db1f57184d023 100644 --- a/airbyte-featureflag/src/main/kotlin/Flags.kt +++ b/airbyte-featureflag/src/main/kotlin/Flags.kt @@ -16,6 +16,8 @@ object AutoDetectSchema : EnvVar(envVar = "AUTO_DETECT_SCHEMA") object NeedStateValidation : EnvVar(envVar = "NEED_STATE_VALIDATION") object ApplyFieldSelection : EnvVar(envVar = "APPLY_FIELD_SELECTION") +object PerfBackgroundJsonValidation : Temporary(key = "performance.backgroundJsonSchemaValidation") + object FieldSelectionWorkspaces : EnvVar(envVar = "FIELD_SELECTION_WORKSPACES") { override fun enabled(ctx: Context): Boolean { val enabledWorkspaceIds: List = fetcher(key) From 6a108cf48226cfa601c07701e1e881148ef70c73 Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Tue, 17 Jan 2023 15:45:32 -0800 Subject: [PATCH 03/11] update Multi contexts to support LDv5 --- airbyte-featureflag/src/main/kotlin/Client.kt | 15 ++++- .../src/main/kotlin/Context.kt | 20 ++++++- .../src/test/kotlin/ContextTest.kt | 56 ++++++++++++++++++- 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/airbyte-featureflag/src/main/kotlin/Client.kt b/airbyte-featureflag/src/main/kotlin/Client.kt index ddf92f665fc3d..6ce2923925be7 100644 --- a/airbyte-featureflag/src/main/kotlin/Client.kt +++ b/airbyte-featureflag/src/main/kotlin/Client.kt @@ -175,9 +175,20 @@ private fun Path.onChange(block: () -> Unit) { * Once v6 is GA, this method would be removed and replaced with toLDContext. */ private fun Context.toLDUser(): LDUser = when (this) { - is Multi -> throw IllegalArgumentException("LDv5 does not support multiple contexts") - else -> { + is Multi -> { val builder = LDUser.Builder(key) + with(contexts) { + forEach { builder.custom(it.kind, it.key) } + if (all { it.key == ANONYMOUS.toString() }) { + builder.anonymous(true) + } + } + builder.build() + } + + else -> { + // for LDv5 Users, add the context type and valid as a custom attribute + val builder = LDUser.Builder(key).apply { custom(kind, key) } if (this.key == ANONYMOUS.toString()) { builder.anonymous(true) } diff --git a/airbyte-featureflag/src/main/kotlin/Context.kt b/airbyte-featureflag/src/main/kotlin/Context.kt index 490e32580422b..236120aa85646 100644 --- a/airbyte-featureflag/src/main/kotlin/Context.kt +++ b/airbyte-featureflag/src/main/kotlin/Context.kt @@ -39,8 +39,24 @@ data class Multi(val contexts: List) : Context { /** This value MUST be "multi" to properly sync with the LaunchDarkly client. */ override val kind = "multi" - /** Multi contexts don't have a key */ - override val key = "" + /** + * Multi contexts don't have a key, however for LDv5 reasons, a key must exist. + * + * Determine what the key should be based on the priority order of: + * workspace -> connection -> source -> destination -> user + * taking the first value + * + * When LDv5 support is dropped replace this with: override vale key = "" + */ + override val key = when { + /** Intellij is going to recommend replacing .sortedBy with .minByOrNull, ignore this recommendation. */ + fetchContexts().isNotEmpty() -> fetchContexts().sortedBy { it.key }.first().key + fetchContexts().isNotEmpty() -> fetchContexts().sortedBy { it.key }.first().key + fetchContexts().isNotEmpty() -> fetchContexts().sortedBy { it.key }.first().key + fetchContexts().isNotEmpty() -> fetchContexts().sortedBy { it.key }.first().key + fetchContexts().isNotEmpty() -> fetchContexts().sortedBy { it.key }.first().key + else -> throw IllegalArgumentException("unsupported context: ${contexts.joinToString { it.kind }}") + } init { // ensure there are no nested contexts (i.e. this Multi does not contain another Multi) diff --git a/airbyte-featureflag/src/test/kotlin/ContextTest.kt b/airbyte-featureflag/src/test/kotlin/ContextTest.kt index 042104d9d2207..ec1036af39e6c 100644 --- a/airbyte-featureflag/src/test/kotlin/ContextTest.kt +++ b/airbyte-featureflag/src/test/kotlin/ContextTest.kt @@ -9,18 +9,68 @@ import io.airbyte.featureflag.Source import io.airbyte.featureflag.User import io.airbyte.featureflag.Workspace import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith class MultiTest { @Test fun `verify data`() { - val user = User("user-id") + val source = Source("source") val workspace = Workspace("workspace") - Multi(listOf(user, workspace)).also { + Multi(listOf(source, workspace)).also { assert(it.kind == "multi") - assert(it.key == "") } } + + @Test + fun `Multi cannot contain another Multi`() { + val source = Source("source") + val workspace = Workspace("workspace") + val multi = Multi(listOf(source, workspace)) + + assertFailsWith { + Multi(listOf(source, workspace, multi)) + } + } + + @Test + fun `fetchContexts returns correct results`() { + val source1 = Source("source1") + val source2 = Source("source2") + val workspace = Workspace("workspace") + val multi = Multi(listOf(source1, source2, workspace)) + + assertEquals(listOf(source1, source2), multi.fetchContexts(), "two source contexts") + assertEquals(listOf(workspace), multi.fetchContexts(), "one workspace context") + assertEquals(listOf(), multi.fetchContexts(), "should be empty as no connection context was provided") + } + + @Test + fun `key set correctly based on contexts`() { + val workspace = Workspace("workspace") + val connection = Connection("connection") + val source = Source("source") + val destination = Destination("destination") + val user = User("user") + + Multi(listOf(user, destination, source, connection, workspace)).also { + assert(it.key == workspace.key) + } + Multi(listOf(user, destination, source, connection)).also { + assert(it.key == connection.key) + } + Multi(listOf(user, destination, source)).also { + assert(it.key == source.key) + } + Multi(listOf(user, destination)).also { + assert(it.key == destination.key) + } + Multi(listOf(user)).also { + assert(it.key == user.key) + } + } + } class WorkspaceTest { From 17429d7d799ed253b6ac055ef8b27d8d9c774dd9 Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Tue, 17 Jan 2023 16:05:15 -0800 Subject: [PATCH 04/11] add test for no context behavior --- airbyte-featureflag/src/test/kotlin/ContextTest.kt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/airbyte-featureflag/src/test/kotlin/ContextTest.kt b/airbyte-featureflag/src/test/kotlin/ContextTest.kt index ec1036af39e6c..d9f2c2ae9f928 100644 --- a/airbyte-featureflag/src/test/kotlin/ContextTest.kt +++ b/airbyte-featureflag/src/test/kotlin/ContextTest.kt @@ -71,6 +71,13 @@ class MultiTest { } } + @Test + fun `no contexts is an exception`() { + assertFailsWith { + Multi(listOf()) + } + } + } class WorkspaceTest { From 445cae0a2b4130b0d11ce75b36ddc5320e7f29bf Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 18 Jan 2023 09:58:03 -0800 Subject: [PATCH 05/11] allow ConfigFileClient to support a null Path --- airbyte-featureflag/src/main/kotlin/Client.kt | 26 +++++++++++-------- .../src/test/kotlin/ClientTest.kt | 17 ++++++++++-- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/airbyte-featureflag/src/main/kotlin/Client.kt b/airbyte-featureflag/src/main/kotlin/Client.kt index 6ce2923925be7..66c4ceb94be07 100644 --- a/airbyte-featureflag/src/main/kotlin/Client.kt +++ b/airbyte-featureflag/src/main/kotlin/Client.kt @@ -33,26 +33,30 @@ sealed interface FeatureFlagClient { } /** - * Config file based feature-flag client. Feature-flag are derived from a yaml config file. - * Also supports flags defined via environment-variables via the [EnvVar] class. + * Config file based feature-flag client. + * + * If no [config] is provided, will return the default state for each [Flag] requested. + * Supports [EnvVar] flags as well. * - * @param [config] the location of the yaml config file that contains the feature-flag definitions. - * The [config] will be watched for changes and the internal representation of the [config] will be updated to match. + * @param [config] optional location of the yaml config file that contains the feature-flag definitions. + * If the [config] is provided, it will be watched for changes and the internal representation of the [config] will be updated to match. */ -class ConfigFileClient(config: Path) : FeatureFlagClient { +class ConfigFileClient(config: Path?) : FeatureFlagClient { /** [flags] holds the mappings of the flag-name to the flag properties */ - private var flags: Map = readConfig(config) + private var flags: Map = config?.let { readConfig(it) } ?: mapOf() /** lock is used for ensuring access to the flags map is handled correctly when the map is being updated. */ private val lock = ReentrantReadWriteLock() init { - if (!config.isRegularFile()) { - throw IllegalArgumentException("config must reference a file") - } + config?.also { + if (!it.isRegularFile()) { + throw IllegalArgumentException("config must reference a file") + } - config.onChange { - lock.write { flags = readConfig(config) } + it.onChange { + lock.write { flags = readConfig(config) } + } } } diff --git a/airbyte-featureflag/src/test/kotlin/ClientTest.kt b/airbyte-featureflag/src/test/kotlin/ClientTest.kt index 5adf6f562f9cb..bb2a6dc3a4d88 100644 --- a/airbyte-featureflag/src/test/kotlin/ClientTest.kt +++ b/airbyte-featureflag/src/test/kotlin/ClientTest.kt @@ -31,7 +31,7 @@ import kotlin.test.assertTrue class ConfigFileClient { @Test - fun `verify platform functionality`() { + fun `verify config-file functionality`() { val cfg = Path.of("src", "test", "resources", "feature-flags.yml") val client: FeatureFlagClient = ConfigFileClient(cfg) @@ -49,7 +49,20 @@ class ConfigFileClient { } @Test - fun `verify platform reload capabilities`() { + fun `verify no-config file returns default flag state`() { + val client: FeatureFlagClient = ConfigFileClient(null) + val defaultFalse = Temporary(key = "default-false") + val defaultTrue = Temporary(key = "default-true", default = true) + + val ctx = Workspace("workspace") + with(client) { + assertTrue { enabled(defaultTrue, ctx) } + assertFalse { enabled(defaultFalse, ctx) } + } + } + + @Test + fun `verify config-file reload capabilities`() { val contents0 = """flags: | - name: reload-test-true | enabled: true From c800338234c80c0fb368015c2cc997e452321bdf Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 18 Jan 2023 11:03:12 -0800 Subject: [PATCH 06/11] add comment --- airbyte-featureflag/src/main/kotlin/Client.kt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/airbyte-featureflag/src/main/kotlin/Client.kt b/airbyte-featureflag/src/main/kotlin/Client.kt index 66c4ceb94be07..c9a7758c7b20c 100644 --- a/airbyte-featureflag/src/main/kotlin/Client.kt +++ b/airbyte-featureflag/src/main/kotlin/Client.kt @@ -182,7 +182,10 @@ private fun Context.toLDUser(): LDUser = when (this) { is Multi -> { val builder = LDUser.Builder(key) with(contexts) { + // Add each individual context's value as an attribute on the LDUser. + // This allows for more granular targeting of feature-flag rules that target LDUser types. forEach { builder.custom(it.kind, it.key) } + if (all { it.key == ANONYMOUS.toString() }) { builder.anonymous(true) } From 68d8439fe3c39036b720432cc7d5ac2e8d1bc0b3 Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 18 Jan 2023 12:15:39 -0800 Subject: [PATCH 07/11] remove test that no longer applies --- .../src/test/kotlin/ClientTest.kt | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/airbyte-featureflag/src/test/kotlin/ClientTest.kt b/airbyte-featureflag/src/test/kotlin/ClientTest.kt index bb2a6dc3a4d88..d1b14d3493217 100644 --- a/airbyte-featureflag/src/test/kotlin/ClientTest.kt +++ b/airbyte-featureflag/src/test/kotlin/ClientTest.kt @@ -10,7 +10,6 @@ import io.airbyte.featureflag.EnvVar import io.airbyte.featureflag.FeatureFlagClient import io.airbyte.featureflag.Flag import io.airbyte.featureflag.LaunchDarklyClient -import io.airbyte.featureflag.Multi import io.airbyte.featureflag.Temporary import io.airbyte.featureflag.TestClient import io.airbyte.featureflag.User @@ -25,7 +24,6 @@ import java.nio.file.Path import java.util.concurrent.TimeUnit import kotlin.io.path.createTempFile import kotlin.io.path.writeText -import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -164,22 +162,6 @@ class LaunchDarklyClient { } } - @Test - fun `verify multi-context is not supported`() { - /** - * TODO replace this test once LDv6 is being used and Context.toLDUser no longer exists, to verify Multi support - */ - val ldClient: LDClient = mockk() - every { ldClient.boolVariation(any(), any(), any()) } returns false - - val client: FeatureFlagClient = LaunchDarklyClient(ldClient) - val multiCtx = Multi(listOf(User("test"))) - - assertFailsWith { - client.enabled(Temporary(key = "test"), multiCtx) - } - } - @Test fun `verify env-var flag support`() { val ldClient: LDClient = mockk() From 14b76766c3b98c62b66601ff94b397b00c22fca4 Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 18 Jan 2023 16:13:43 -0800 Subject: [PATCH 08/11] change factory to always return one of the implementations --- .../src/main/kotlin/config/Factory.kt | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/airbyte-featureflag/src/main/kotlin/config/Factory.kt b/airbyte-featureflag/src/main/kotlin/config/Factory.kt index d80a4673a13e7..55aec34bbc1d8 100644 --- a/airbyte-featureflag/src/main/kotlin/config/Factory.kt +++ b/airbyte-featureflag/src/main/kotlin/config/Factory.kt @@ -10,7 +10,6 @@ import io.airbyte.featureflag.FeatureFlagClient import io.airbyte.featureflag.LaunchDarklyClient import io.micronaut.context.annotation.Factory import io.micronaut.context.annotation.Property -import io.micronaut.context.annotation.Requirements import io.micronaut.context.annotation.Requires import jakarta.inject.Singleton import java.nio.file.Path @@ -20,22 +19,22 @@ internal const val CONFIG_OSS_KEY = "airbyte.feature-flag.path" @Factory class Factory { - @Requirements( - Requires(property = CONFIG_LD_KEY), - Requires(missingProperty = CONFIG_OSS_KEY), - ) + + @Requires(property = CONFIG_LD_KEY) @Singleton - fun Cloud(@Property(name = CONFIG_LD_KEY) apiKey: String): FeatureFlagClient { + fun LaunchDarklyClient(@Property(name = CONFIG_LD_KEY) apiKey: String): FeatureFlagClient { val client = LDClient(apiKey) return LaunchDarklyClient(client) } - @Requirements( - Requires(property = CONFIG_OSS_KEY), - Requires(missingProperty = CONFIG_LD_KEY), - ) - fun Platform(@Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient { - val path = Path.of(configPath) + @Requires(missingProperty = CONFIG_LD_KEY) + @Singleton + fun ConfigFileClient(@Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient { + val path: Path? = if (configPath.isNotBlank()) { + Path.of(configPath) + } else { + null + } return ConfigFileClient(path) } } From 94d64d46048c45e6984d2f349292af284bcbb66a Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 25 Jan 2023 11:15:40 -0800 Subject: [PATCH 09/11] add flags.yml config; quote ports --- docker-compose.yaml | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index f01aef60365f9..bc26a92668389 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -109,12 +109,14 @@ services: - MICRONAUT_ENVIRONMENTS=${WORKERS_MICRONAUT_ENVIRONMENTS} - APPLY_FIELD_SELECTION=${APPLY_FIELD_SELECTION} - FIELD_SELECTION_WORKSPACES=${FIELD_SELECTION_WORKSPACES} + configs: + - flags volumes: - /var/run/docker.sock:/var/run/docker.sock - workspace:${WORKSPACE_ROOT} - ${LOCAL_ROOT}:${LOCAL_ROOT} ports: - - 9000 + - "9000" networks: - airbyte_internal depends_on: @@ -155,7 +157,9 @@ services: - MICRONAUT_ENVIRONMENTS=${WORKERS_MICRONAUT_ENVIRONMENTS} - AUTO_DETECT_SCHEMA=${AUTO_DETECT_SCHEMA} ports: - - 8001 + - "8001" + configs: + - flags volumes: - workspace:${WORKSPACE_ROOT} - data:${CONFIG_ROOT} @@ -171,7 +175,7 @@ services: container_name: airbyte-webapp restart: unless-stopped ports: - - 80 + - "80" environment: - AIRBYTE_ROLE=${AIRBYTE_ROLE:-} - AIRBYTE_VERSION=${VERSION} @@ -222,6 +226,8 @@ services: - UPDATE_DEFINITIONS_CRON_ENABLED=${UPDATE_DEFINITIONS_CRON_ENABLED} - WORKSPACE_ROOT=${WORKSPACE_ROOT} - MICRONAUT_ENVIRONMENTS=${CRON_MICRONAUT_ENVIRONMENTS} + configs: + - flags volumes: - workspace:${WORKSPACE_ROOT} networks: @@ -248,9 +254,9 @@ services: container_name: airbyte-proxy restart: unless-stopped ports: - - 8000:8000 - - 8001:8001 - - 8003:8003 + - "8000:8000" + - "8001:8001" + - "8003:8003" environment: - BASIC_AUTH_USERNAME=${BASIC_AUTH_USERNAME} - BASIC_AUTH_PASSWORD=${BASIC_AUTH_PASSWORD} @@ -271,6 +277,9 @@ volumes: name: ${DATA_DOCKER_MOUNT} db: name: ${DB_DOCKER_MOUNT} +configs: + flags: + file: ./configs/flags.yml networks: airbyte_public: airbyte_internal: From 46bb5fe82a9c2f11b82556e4498d924318c0c093 Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 25 Jan 2023 16:33:52 -0800 Subject: [PATCH 10/11] replace @Requires annotations with hand-checking --- .../src/main/kotlin/config/Factory.kt | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/airbyte-featureflag/src/main/kotlin/config/Factory.kt b/airbyte-featureflag/src/main/kotlin/config/Factory.kt index 55aec34bbc1d8..ec47097bdf7c5 100644 --- a/airbyte-featureflag/src/main/kotlin/config/Factory.kt +++ b/airbyte-featureflag/src/main/kotlin/config/Factory.kt @@ -10,7 +10,6 @@ import io.airbyte.featureflag.FeatureFlagClient import io.airbyte.featureflag.LaunchDarklyClient import io.micronaut.context.annotation.Factory import io.micronaut.context.annotation.Property -import io.micronaut.context.annotation.Requires import jakarta.inject.Singleton import java.nio.file.Path @@ -19,17 +18,15 @@ internal const val CONFIG_OSS_KEY = "airbyte.feature-flag.path" @Factory class Factory { - - @Requires(property = CONFIG_LD_KEY) @Singleton - fun LaunchDarklyClient(@Property(name = CONFIG_LD_KEY) apiKey: String): FeatureFlagClient { - val client = LDClient(apiKey) - return LaunchDarklyClient(client) - } + fun featureFlagClient(@Property(name = CONFIG_LD_KEY) apiKey: String, @Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient { + // I cannot get the @Requires annotation to work to load one instance if a property is set and another instance if unset. + // Combined both cases together here instead resulting to manually doing the is-set check via the isNotBlank function. + if (apiKey.isNotBlank()) { + val client = LDClient(apiKey) + return LaunchDarklyClient(client) + } - @Requires(missingProperty = CONFIG_LD_KEY) - @Singleton - fun ConfigFileClient(@Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient { val path: Path? = if (configPath.isNotBlank()) { Path.of(configPath) } else { From 4dc4eccdc99115da1310d94cfd6297efbed4ec5e Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Wed, 25 Jan 2023 16:41:11 -0800 Subject: [PATCH 11/11] make the code easier to read --- airbyte-featureflag/src/main/kotlin/config/Factory.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/airbyte-featureflag/src/main/kotlin/config/Factory.kt b/airbyte-featureflag/src/main/kotlin/config/Factory.kt index ec47097bdf7c5..8cf81ed5f6734 100644 --- a/airbyte-featureflag/src/main/kotlin/config/Factory.kt +++ b/airbyte-featureflag/src/main/kotlin/config/Factory.kt @@ -19,7 +19,10 @@ internal const val CONFIG_OSS_KEY = "airbyte.feature-flag.path" @Factory class Factory { @Singleton - fun featureFlagClient(@Property(name = CONFIG_LD_KEY) apiKey: String, @Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient { + fun featureFlagClient( + @Property(name = CONFIG_LD_KEY) apiKey: String, + @Property(name = CONFIG_OSS_KEY) configPath: String, + ): FeatureFlagClient { // I cannot get the @Requires annotation to work to load one instance if a property is set and another instance if unset. // Combined both cases together here instead resulting to manually doing the is-set check via the isNotBlank function. if (apiKey.isNotBlank()) {