Skip to content
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

feat(flags): Add OpenFeature integration #1209

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions kork-core/kork-core.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ dependencies {
api project(":kork-api")
api project(":kork-annotations")
api project(":kork-exceptions")
api project(":kork-flags")
api "org.springframework.boot:spring-boot-autoconfigure"
api "org.springframework.boot:spring-boot-starter-aop"
api "org.springframework.boot:spring-boot-starter-actuator"
Expand All @@ -30,6 +31,7 @@ dependencies {
testImplementation "org.junit.jupiter:junit-jupiter-api"
testImplementation("org.mockito:mockito-core")
testImplementation "org.spockframework:spock-core"
testImplementation "org.springframework:spring-test"
testRuntimeOnly "cglib:cglib-nodep"
testRuntimeOnly "org.objenesis:objenesis"
testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright 2024 Apple, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.kork;

import com.netflix.spinnaker.kork.dynamicconfig.OpenFeatureImportBeanDefinitionRegistrar;
import com.netflix.spinnaker.kork.dynamicconfig.TransientConfigConfiguration;
import com.netflix.spinnaker.kork.version.ServiceVersion;
import com.netflix.spinnaker.kork.version.SpringPackageVersionResolver;
import com.netflix.spinnaker.kork.version.VersionResolver;
import java.util.List;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Import;

@Import({OpenFeatureImportBeanDefinitionRegistrar.class, TransientConfigConfiguration.class})
public class BootstrapComponents {
@Bean
@ConditionalOnMissingBean(VersionResolver.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say to use this only on auto-config classes. This isn't one of those, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a class imported by an auto-config class in one case and one named as an auto-config class in the other case. However, this has made me realize that there's a bit of a bean mess in the various Spinnaker services because PluginsAutoConfiguration imports PlatformComponents (which is an auto-config but is being directly imported as a regular config in this context), but PluginsAutoConfiguration is not itself an auto-config, and it is directly imported by different configuration classes in different services in such a way that the beans from there aren't directly visible by IntelliJ consistently through different modules. I want to look more closely at this.

And to be specific, this BootstrapComponents class is a de-duplication of the two beans defined exactly this way in both PluginsAutoConfiguration and PlatformComponents I found while doing something else at one point that got slightly mixed in here due to affected code (declarations of a default DynamicConfigService bean).

This whole area may need some deeper digging. My main purpose is to provide the beans for accessing OpenFeatureAPI directly, but I added the DynamicConfigService integration for a simpler way to get started using flagd (or whatever OpenFeature provider you want to use in theory) which can control all the existing features of Spinnaker that use DynamicConfigService. However, now this raises further questions about where to place the beans.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. This is no worse than it was before in PluginsAutoConfiguration and PlatformComponents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on how I've redone the bean config, these changes might not be relevant anymore, but I did put them in a separate commit.

public static VersionResolver versionResolver(ApplicationContext applicationContext) {
return new SpringPackageVersionResolver(applicationContext);
}

@Bean
@ConditionalOnMissingBean(ServiceVersion.class)
public static ServiceVersion serviceVersion(
ApplicationContext applicationContext, List<VersionResolver> versionResolvers) {
return new ServiceVersion(applicationContext, versionResolvers);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,43 +16,21 @@

package com.netflix.spinnaker.kork;

import com.netflix.spinnaker.kork.dynamicconfig.TransientConfigConfiguration;
import com.netflix.spinnaker.kork.metrics.SpectatorConfiguration;
import com.netflix.spinnaker.kork.version.ServiceVersion;
import com.netflix.spinnaker.kork.version.SpringPackageVersionResolver;
import com.netflix.spinnaker.kork.version.VersionResolver;
import io.github.resilience4j.circuitbreaker.autoconfigure.CircuitBreakersHealthIndicatorAutoConfiguration;
import io.github.resilience4j.ratelimiter.autoconfigure.RateLimitersHealthIndicatorAutoConfiguration;
import java.util.List;
import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;

@Configuration
@Import({
TransientConfigConfiguration.class,
BootstrapComponents.class,
SpectatorConfiguration.class,
})
@ImportAutoConfiguration(
exclude = {
CircuitBreakersHealthIndicatorAutoConfiguration.class,
RateLimitersHealthIndicatorAutoConfiguration.class
})
public class PlatformComponents {

@Bean
@ConditionalOnMissingBean(ServiceVersion.class)
ServiceVersion serviceVersion(
ApplicationContext applicationContext, List<VersionResolver> versionResolvers) {
return new ServiceVersion(applicationContext, versionResolvers);
}

@Bean
@ConditionalOnMissingBean(SpringPackageVersionResolver.class)
VersionResolver springPackageVersionResolver(ApplicationContext applicationContext) {
return new SpringPackageVersionResolver(applicationContext);
}
}
public class PlatformComponents {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
* Copyright 2024 Apple, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package com.netflix.spinnaker.kork.dynamicconfig;

import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
import dev.openfeature.sdk.ErrorCode;
import dev.openfeature.sdk.EvaluationContext;
import dev.openfeature.sdk.Features;
import dev.openfeature.sdk.FlagEvaluationDetails;
import dev.openfeature.sdk.MutableContext;
import dev.openfeature.sdk.Value;
import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;

/**
* Dynamic configuration service using {@linkplain dev.openfeature.sdk.OpenFeatureAPI OpenFeature}
* as the primary lookup mechanism with the ability to fall back to using another {@link
* DynamicConfigService}.
*/
@Log4j2
@NonnullByDefault
@RequiredArgsConstructor
public class OpenFeatureDynamicConfigService implements DynamicConfigService {
private final Features features;
private final DynamicConfigService fallbackDynamicConfigService;

@Override
@SuppressWarnings("unchecked")
public <T> T getConfig(Class<T> configType, String configName, T defaultValue) {
FlagEvaluationDetails<?> details;
T value;
if (configType == Boolean.class) {
details = features.getBooleanDetails(configName, (Boolean) defaultValue);
value = (T) details.getValue();
} else if (configType == Integer.class) {
details = features.getIntegerDetails(configName, (Integer) defaultValue);
value = (T) details.getValue();
} else if (configType == Long.class) {
// TODO(jvz): https://github.com/open-feature/java-sdk/issues/501
var intDetails = features.getIntegerDetails(configName, ((Long) defaultValue).intValue());
details = intDetails;
value = (T) Long.valueOf(intDetails.getValue().longValue());
Comment on lines +53 to +57
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I've found more issues with this part that I'm investigating. I'm considering adding a facade API for feature flags since OpenFeature is missing support for long (and any other Number type besides int and double), and I'm not trying to fork the library. While I'd normally criticize people for doing that, this seems to be a scenario where a facade is useful.

} else if (configType == Double.class) {
details = features.getDoubleDetails(configName, (Double) defaultValue);
value = (T) details.getValue();
} else if (configType == String.class) {
details = features.getStringDetails(configName, (String) defaultValue);
value = (T) details.getValue();
} else {
var objectDetails = features.getObjectDetails(configName, Value.objectToValue(defaultValue));
details = objectDetails;
value = (T) objectDetails.getValue().asObject();
}
var errorCode = details.getErrorCode();
if (errorCode == ErrorCode.FLAG_NOT_FOUND) {
return fallbackDynamicConfigService.getConfig(configType, configName, defaultValue);
}
if (errorCode != null) {
log.warn("Unable to resolve configuration key '{}': {}", configName, details);
}
return value;
}

@Override
public boolean isEnabled(String flagName, boolean defaultValue) {
var details = features.getBooleanDetails(flagPropertyName(flagName), defaultValue);
var errorCode = details.getErrorCode();
if (errorCode == ErrorCode.FLAG_NOT_FOUND) {
return fallbackDynamicConfigService.isEnabled(flagName, defaultValue);
}
if (errorCode != null) {
log.warn("Unable to resolve configuration flag '{}': {}", flagName, details);
}
return details.getValue();
}

@Override
public boolean isEnabled(String flagName, boolean defaultValue, ScopedCriteria criteria) {
var context = convertCriteria(criteria);
var details = features.getBooleanDetails(flagPropertyName(flagName), defaultValue, context);
var errorCode = details.getErrorCode();
if (errorCode == ErrorCode.FLAG_NOT_FOUND) {
return fallbackDynamicConfigService.isEnabled(flagName, defaultValue, criteria);
}
if (errorCode != null) {
log.warn(
"Unable to resolve configuration flag '{}' with {}: {}", flagName, criteria, details);
}
return details.getValue();
}

private static String flagPropertyName(String flagName) {
return flagName.endsWith(".enabled") ? flagName : flagName + ".enabled";
}

private static EvaluationContext convertCriteria(ScopedCriteria criteria) {
return new MutableContext()
.add("region", criteria.region)
.add("account", criteria.account)
.add("cloudProvider", criteria.cloudProvider)
.add("application", criteria.application);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright 2024 Apple, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package com.netflix.spinnaker.kork.dynamicconfig;

import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
import dev.openfeature.sdk.Features;
import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import org.springframework.beans.factory.BeanFactory;
import org.springframework.beans.factory.config.RuntimeBeanReference;
import org.springframework.beans.factory.support.BeanDefinitionRegistry;
import org.springframework.beans.factory.support.BeanNameGenerator;
import org.springframework.beans.factory.support.GenericBeanDefinition;
import org.springframework.context.annotation.ImportBeanDefinitionRegistrar;
import org.springframework.core.env.Environment;
import org.springframework.core.type.AnnotationMetadata;

/** Handles creation of an OpenFeature-based {@link DynamicConfigService} bean. */
@Log4j2
@NonnullByDefault
@RequiredArgsConstructor
public class OpenFeatureImportBeanDefinitionRegistrar implements ImportBeanDefinitionRegistrar {
private final Environment environment;
private final BeanFactory beanFactory;

@Override
public void registerBeanDefinitions(
AnnotationMetadata importingClassMetadata,
BeanDefinitionRegistry registry,
BeanNameGenerator importBeanNameGenerator) {
var enableOpenFeatureFlags =
environment.getProperty("flags.providers.openfeature.enabled", Boolean.class, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other note: I'd like to document all the properties supported by this PR. Some are documented, but it's not consistent.

if (!enableOpenFeatureFlags) {
// no need to configure anything; let default SpringDynamicConfigService bean be created
// elsewhere
return;
}
var features = beanFactory.getBeanProvider(Features.class).getIfUnique();
if (features == null) {
return;
}

var dynamicConfigServiceFallbackBean = new GenericBeanDefinition();
var enableSpringFlags =
environment.getProperty("flags.providers.spring.enabled", Boolean.class, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbyron-sf here's the feature flag to disable this dynamic property support from Spring when performance issues are a concern. It would be more efficient for dynamic flags to be handled by something like flagd which indexes all the feature flags by name in a hash map (something Spring doesn't do for whatever reason).

// not trying to auto-inject a dependent bean anywhere
dynamicConfigServiceFallbackBean.setAutowireCandidate(false);
if (enableSpringFlags) {
dynamicConfigServiceFallbackBean.setBeanClass(SpringDynamicConfigService.class);
} else {
dynamicConfigServiceFallbackBean.setInstanceSupplier(() -> DynamicConfigService.NOOP);
}

var fallbackBeanName =
importBeanNameGenerator.generateBeanName(dynamicConfigServiceFallbackBean, registry);
registry.registerBeanDefinition(fallbackBeanName, dynamicConfigServiceFallbackBean);
log.debug("Registered bean '{}': {}", fallbackBeanName, dynamicConfigServiceFallbackBean);

var dynamicConfigServiceBean = new GenericBeanDefinition();
dynamicConfigServiceBean.setPrimary(true);
dynamicConfigServiceBean.setBeanClass(OpenFeatureDynamicConfigService.class);
var args = dynamicConfigServiceBean.getConstructorArgumentValues();
args.addGenericArgumentValue(features);
args.addGenericArgumentValue(new RuntimeBeanReference(fallbackBeanName));
var beanName = importBeanNameGenerator.generateBeanName(dynamicConfigServiceBean, registry);
registry.registerBeanDefinition(beanName, dynamicConfigServiceBean);
log.debug("Registered bean '{}': {}", beanName, dynamicConfigServiceBean);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Copyright 2024 Apple, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package com.netflix.spinnaker.kork.dynamicconfig;

import static org.junit.jupiter.api.Assertions.*;

import dev.openfeature.sdk.OpenFeatureAPI;
import dev.openfeature.sdk.providers.memory.Flag;
import dev.openfeature.sdk.providers.memory.InMemoryProvider;
import java.util.HashMap;
import java.util.Map;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

class OpenFeatureDynamicConfigServiceTest {
OpenFeatureDynamicConfigService service;

@BeforeEach
void setUp() {
Map<String, Flag<?>> flags = new HashMap<>();
flags.put(
"flag.enabled",
Flag.<Boolean>builder()
.variant("on", true)
.variant("off", false)
.defaultVariant("on")
.build());
// TODO(jvz): https://github.com/open-feature/java-sdk/issues/501
// this should be encoded as a Flag<Long>, but there is no Long-based API available yet
flags.put(
"some.number",
Flag.builder()
.variant("default", 10000)
.variant("alternative", 20000)
.defaultVariant("default")
.build());
flags.put(
"config",
Flag.<String>builder()
.variant("alpha", "1.0")
.variant("beta", "2.0")
.variant("gamma", "3.0")
.defaultVariant("beta")
.build());
OpenFeatureAPI api = OpenFeatureAPI.getInstance();
api.setProviderAndWait(new InMemoryProvider(flags));
service = new OpenFeatureDynamicConfigService(api.getClient(), DynamicConfigService.NOOP);
}

@Test
void canResolveBooleanFlags() {
assertTrue(service.isEnabled("flag", false));
assertTrue(service.isEnabled("flag.enabled", false));
assertFalse(service.isEnabled("flag.", false));
}

@Test
void canResolveLongData() {
assertEquals(10000L, service.getConfig(Long.class, "some.number", 42L));
}

@Test
void canResolveStringData() {
assertEquals("2.0", service.getConfig(String.class, "config", "0.0"));
}
}
Loading
Loading