open-feature / open-feature-operator

A Kubernetes feature flag operator
https://openfeature.dev
Apache License 2.0
179 stars 34 forks source link

`syncProvider` should be part of `flagsourceconfiguration` #318

Closed toddbaert closed 1 year ago

toddbaert commented 1 year ago

It seems like the only way to configure flagd to run in the "mounted file mode" (with a filepath sync) is to add:

  syncProvider:
    name: filepath

to a FeatureFlagConfiguration spec.

This seems less than ideal. We want to keep the sidecar configuration in the fsc CRD, instead of mixing it with the flag definition CRD (ff).

It also seems like helm should allow us to configure this (currently it only allows custom providerArgs via sidecarConfiguration.providerArgs.

james-milligan commented 1 year ago

We need to keep the syncProvider.name configuration in the FeatureFlagConfiguration spec as the syncs are set up on a 'per configuration' basis, i.e. ffconfig-A could configure a file path sync whilst ffconfig-B configures a Kubernetes sync (or grpc down the line) and both can be passed to flagd to run simultaneously, if we remove this configuration then we lose this flexibility.

That being said I think we should have the option to set the default via the fsc crd, allowing for the configuration to be overwritten if required via the existing syncProvider.name property in the FeatureFlagConfiguration spec

skyerus commented 1 year ago

It does seem confusing to define the sync provider on a per FeatureFlagConfiguration basis, intuitively it feels like FlagSourceConfiguration ought to define where flags are sourced from. However, if doing this removes the flexibility of using multiple sync providers then I am onboard with James' suggestion to define the default in the FlagSourceConfiguration, with some documentation explaining the motivation.

james-milligan commented 1 year ago

Another option is to further extend the FlagSourceConfiguration, adding an array of sync providers with source and provider fields. We can then deprecate the openfeature.dev/featureflagconfiguration annotation, and would remove the requirement of creating a FeatureFlagConfiguration for all sync providers (currently to implement a remote sync you must create a FeatureFlagconfiguration with the appropriate sync provider configuration)

This change would be breaking

apiVersion: core.openfeature.dev/v1alpha2
kind: FlagSourceConfiguration
metadata:
  name: example
spec:
  syncProviders:
  - source: default/end-to-end
    provider: kubernetes
  - source: default/end-to-end
    provider: filepath
  - source: 192.168.10.1:8080
    provider: remote
  - source: 192.168.10.1:8080
    provider: grpc
skyerus commented 1 year ago

Are there downsides to creating a hard relation between FlagSourceConfiguration & FeatureFlagConfiguration? Having the FeatureFlagConfiguration hardcoded in the FlagSourceConfiguration removes the power of being able to use the same FlagSourceConfiguration but switching between FeatureFlagConfiguration.

toddbaert commented 1 year ago

@james-milligan You're right, the syncs are "set up on a 'per configuration' basis". Maybe there isn't much that could be done here with respect to ergonomics...

I am interested in this proposal but I also understand @skyerus 's concern about it. However, the downside if we don't create this hard relation, is that the same FeatureFlagConfiguration can't be mounted different ways (maybe that's not a big deal though).

We could also just keep things as they are, and perhaps create an operator-level "Default" mode of operation that mounts everything as configmaps vs k8s-syncs as specified, and allows override at the spec level.