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

syncProvider should be part of flagsourceconfiguration #318

Closed
toddbaert opened this issue Jan 27, 2023 · 5 comments
Closed

syncProvider should be part of flagsourceconfiguration #318

toddbaert opened this issue Jan 27, 2023 · 5 comments
Assignees

Comments

@toddbaert
Copy link
Member

toddbaert commented Jan 27, 2023

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
Copy link
Contributor

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
Copy link
Contributor

skyerus commented Jan 27, 2023

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
Copy link
Contributor

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
Copy link
Contributor

skyerus commented Jan 27, 2023

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
Copy link
Member Author

toddbaert commented Jan 27, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants