feat(flags): Add OpenFeature integration

[jvz]

This adds an OpenFeature-based feature flags module along with a built-in provider based on flagd.

As this implements DynamicConfigService, all existing dynamic config usage is supported out of the box here. This is primarily to get agreement with the rest of the project that we'll be using that API (along with the OpenFeature API) for feature flags. This should help a lot with experimental features.

[dbyron-sf]

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

[jvz]

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.

[dbyron-sf]

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

[jvz]

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.

[dbyron-sf]

Could you add some high-level motivation for this? What are the benefits of OpenFeature?

[jvz]

What I find to be a killer benefit is being able to use targeting for feature flag enablement. While DynamicConfigService offers basic support for a few orthogonal targeting criteria, OpenFeature defines a more general feature flag API.

Then, I combined that with flagd by using the Java provider as a reference implementation. What I like about flagd (besides being OSS and already well-integrated with OpenFeature) are the existing ways of defining feature flags which uses some JsonLogic thing for defining more complex feature flag targeting rules.

Benefits-wise, there are a few different targets. The impetus behind integrating with this is to help enable more continuous delivery of Spinnaker itself by flagging experimental and new stuff behind flags that can be selectively rolled out in different ways. For example, I've been researching how to go about extending Fiat to support external policy engines, but due to the existing mess in Fiat, I've found that very hard to do without affecting a lot of existing code, and I want to be able to selectively roll out changes in authorization based on context like the current user, application, resource type, and so on, and not just in a boolean fashion.

Another advantage is to develop things in a sort of default-on way rather than using conditional beans and other easy to break configuration methods for feature flags. While features are still in alpha, they're not active, but once they're stable, the feature flags can be removed.

[dbyron-sf]

Thanks for that @jvz . I need to digest it a bit. One thing we have noticed is that, at least with the spring implementation of DynamicConfigService is that it becomes super slow/expensive when used in combination with lots of (clouddriver) accounts if they're defined in yml files. More generally it slows down with lots of config properties, but lots of accounts is a way to get there. Basically, any access of one property causes a copy (and perhaps parsing...can't remember) of all properties. We've since moved accounts to the account API, but for folks who haven't, this could be a drag. The tl;dr was basically, don't use the DynamicConfigService. Does this implementation still use enough spring under the covers to suffer this way?

[jvz]

... Does this implementation still use enough spring under the covers to suffer this way?

It allows it as a backup, though perhaps I could make that configurable as well. Right now, it configures a DynamicConfigService to fallback to a Spring-based one for undefined feature flags from OpenFeature.

[dbyron-sf]

I think I see where you're coming from...or at least the ability to enable features per-user (or some other attribute) makes sense. I'm not quite sure I see the part about conditional beans. Can you give an example of how this would play out as a feature moves from alpha to stable?

[jvz]

The conditional beans aspect would be where various beans are annotated like @ConditionalOnProperty("some.feature.flag"). The more of these you have, the larger number of combinations of possible beans available explodes which makes it much easier to write beans in such a way that various configurations can cause startup failures due to missing beans and such. It also makes it harder to write error handling code to alert to misconfiguration (like via a FailureAnalyzer as those are based on the exception type, and missing beans always throw a NoSuchBeanDefinitionException which is hard to generically analyze). Similarly, when using @ConditionalOnMissingBean or other conditions, these become very hard to use for Spinnaker features without configuration errors that very few people would ever understand how to debug.

As for a move from alpha to stable, the idea would be that relevant beans configured for an alpha feature wouldn't necessarily be conditionally created; instead, the beans would use feature flags to enable or disable the code within the beans to actually do anything (comparable to how fiat-api works today). When a feature is alpha, it can be selectively enabled by admins using whatever feature flag conditions they want (e.g., maybe you have a group of users who want to try out new features early on like Firefox Nightly versus other users who only want to use stable features), and as a feature graduates to stable, the feature flags can change their default variant to enabled, and then the feature flag can be removed (or converted into a more basic property if it makes sense for the feature to remain optional but doesn't require dynamic feature flags).

If you think of this from an implementation point of view, the difference between feature flags and configuration properties would deal with how dynamic the input settings are expected to be. When dealing with Spring properties, for example, these are usually written with the assumption that the property is defined at startup and never changes (and only the limited @RefreshScope beans we have in Spinnaker assume otherwise, but that has performance issues as you mentioned when the number of Spring properties increases as property lookup is O(n) instead of anything sublinear due to the numerous extension points in Spring for handling properties). When dealing with a feature flag, the flag should be checked nearly every time (with potential caching involved).

[dbyron-sf]

OK, agreed. Seems like this is a bit of a separate thing from OpenFeature integration (i.e. always enable all the beans and check flags inside them). As in, I don't think this PR actually moves in this direction. And yes, as we get more flags, this will be more important.

[dbyron-sf]

There is something nice about beans not loading at all as far as reasoning about changes in behavior. It's worth the tradeoff though.

[jvz]

I think the bean loading bit should be limited to optional features rather than unstable features. Otherwise, the combinatorial explosion of possible configurations to consider is too large for us humans to handle (once you have, say, 10 optional features, you now have 1024 combinations of flags possible, and it's likely that ~1000 of them are invalid combinations).

[dbyron-sf]

You're saying conditional bean loading is for optional features only, or for unstable features only? I think you're saying unstable.

[jvz]

Conditional bean loading should be for optional features only, right. Unstable features that are meant to be part of the default system shouldn't bother creating conditional beans (but they should annotate their stability guarantees at least).

[jvz]

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.

[jvz]

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

[jvz]

@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).