From b7e3894fd70dc1a7bd58ac574a1c2d1629fff91e Mon Sep 17 00:00:00 2001 From: Cole Snodgrass Date: Thu, 26 Jan 2023 08:59:48 -0800 Subject: [PATCH] add perf feature-flag; ensure compatability with LaunchDarkly (LDv5) users (#21534) * init commit of flags.yml * add PerfBackgroundJsonValidation flag * update Multi contexts to support LDv5 * add test for no context behavior * allow ConfigFileClient to support a null Path * add comment * remove test that no longer applies * change factory to always return one of the implementations * add flags.yml config; quote ports * replace @Requires annotations with hand-checking * make the code easier to read --- airbyte-featureflag/src/main/kotlin/Client.kt | 44 +++++++++---- .../src/main/kotlin/Context.kt | 20 +++++- airbyte-featureflag/src/main/kotlin/Flags.kt | 2 + .../src/main/kotlin/config/Factory.kt | 31 +++++---- .../src/test/kotlin/ClientTest.kt | 32 +++++----- .../src/test/kotlin/ContextTest.kt | 63 ++++++++++++++++++- configs/flags.yml | 3 + docker-compose.yaml | 21 +++++-- 8 files changed, 158 insertions(+), 58 deletions(-) create mode 100644 configs/flags.yml diff --git a/airbyte-featureflag/src/main/kotlin/Client.kt b/airbyte-featureflag/src/main/kotlin/Client.kt index ddf92f665fc3..c9a7758c7b20 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) } + } } } @@ -175,9 +179,23 @@ 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) { + // 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) + } + } + 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 490e32580422..236120aa8564 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/main/kotlin/Flags.kt b/airbyte-featureflag/src/main/kotlin/Flags.kt index bfc1dd0a6a82..db1f57184d02 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) diff --git a/airbyte-featureflag/src/main/kotlin/config/Factory.kt b/airbyte-featureflag/src/main/kotlin/config/Factory.kt index d80a4673a13e..8cf81ed5f673 100644 --- a/airbyte-featureflag/src/main/kotlin/config/Factory.kt +++ b/airbyte-featureflag/src/main/kotlin/config/Factory.kt @@ -10,8 +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 +18,23 @@ internal const val CONFIG_OSS_KEY = "airbyte.feature-flag.path" @Factory class Factory { - @Requirements( - Requires(property = CONFIG_LD_KEY), - Requires(missingProperty = CONFIG_OSS_KEY), - ) @Singleton - fun Cloud(@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) + } - @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) + val path: Path? = if (configPath.isNotBlank()) { + Path.of(configPath) + } else { + null + } return ConfigFileClient(path) } } diff --git a/airbyte-featureflag/src/test/kotlin/ClientTest.kt b/airbyte-featureflag/src/test/kotlin/ClientTest.kt index dbbfc8ad55d3..4a538da7d509 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 @@ -32,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) @@ -57,6 +56,19 @@ class ConfigFileClient { * * TODO: move this to a different test suite */ + @Test + 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 @Ignore fun `verify platform reload capabilities`() { @@ -161,22 +173,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() diff --git a/airbyte-featureflag/src/test/kotlin/ContextTest.kt b/airbyte-featureflag/src/test/kotlin/ContextTest.kt index 042104d9d220..d9f2c2ae9f92 100644 --- a/airbyte-featureflag/src/test/kotlin/ContextTest.kt +++ b/airbyte-featureflag/src/test/kotlin/ContextTest.kt @@ -9,18 +9,75 @@ 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) + } + } + + @Test + fun `no contexts is an exception`() { + assertFailsWith { + Multi(listOf()) + } + } + } class WorkspaceTest { diff --git a/configs/flags.yml b/configs/flags.yml new file mode 100644 index 000000000000..b5cc273ca270 --- /dev/null +++ b/configs/flags.yml @@ -0,0 +1,3 @@ +flags: + - name: performance.backgroundJsonSchemaValidation + enabled: true diff --git a/docker-compose.yaml b/docker-compose.yaml index f01aef60365f..bc26a9266838 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: