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

Research the best way to support advanced configuration per sync provider #405

Closed
toddbaert opened this issue Feb 13, 2023 · 6 comments
Closed
Assignees

Comments

@toddbaert
Copy link
Member

toddbaert commented Feb 13, 2023

flagd supports various sync providers (filepath, kubernetes, http, grpc), and also supports the usage of multiple providers simultaneously (just as an example, flagd can be started to listen to an HTTP endpoint, watch a file, and connect to 2 gRPC servers).

Each of these providers may require individual configuration. For instance, each gRPC stream might need different certs, or http syncs may each require different auth headers. We currently do not have a solution to handle this sort of configuration from the command line.

We need to decide on a way to support this. It should also work well with CRs we define in the OpenFeature operator.

Ideas:

  • the use of multiple syncProvider args, supporting key/value encoding of some kind
  • defining a configuration file structure and passing that to flagd
  • ?

Edit: See Jame's research/suggestions below. I'm somewhat partial to this one.

Implementation issue: open-feature/open-feature-operator#382

@toddbaert toddbaert changed the title [research] How to support advanced configuration per sync provider [research] How best to support advanced configuration per sync provider Feb 13, 2023
@james-milligan
Copy link
Contributor

I like the idea of a config file, its a pretty common pattern that would allow for developers to very easily interpret configurations (compared to a multi line list of startup flags), we can also create a schema definition for it to allow for linting + validation

@james-milligan
Copy link
Contributor

Ive done some more investigation into a clean method for handling per-sync configurations:

Currently we are using viper to handle our flag values, out of the box viper allows for the parsing of a config file (.json/.yaml), we already have this configuration set up within flagd under the --config flag.
This means we can provide a config file such as:

port: 8080
sync-provider-args:
    food: bars
    cat: dog

resulting in each sync provider receiving the following map[string]string: map[cat:dog food:bars]

This flag structure feels inappropriate for our use case, instead we can update the sync.ProviderArgs type from map[string]string to map[string]interface{} and decode the --sync-provider-args flag value as map[string]sync.ProviderArgs

conf := &map[string]sync.ProviderArgs{}
err = viper.UnmarshalKey(providerArgsFlagName, conf)

this means we can now pass the following configuration file:

sync-provider-args:
    sync-uri-1:
        food: 5
        cat: dog
        fizz: 
            buzz: 7

this allows us to define configuration on a per-sync basis, using the sync URI as the map key, which we can filter for on startup, with each configuration defined as map[string]interface{}, to be handled internally by each sync as required.

IMO this implementation keeps out configurations extremely flexible, while ensuring each syncs configuration is kept separate. It also allows us to use both flag arguments and config files (if not are used the configuration from flag arguments overwrites any duplicated keys in the configuration file)

This will also make it easy to introduce the watching of these configurations: https://github.com/spf13/viper#watching-and-re-reading-config-files

@james-milligan
Copy link
Contributor

Another option is to extend the functionality of the '--uri' flag to take optionally take a json configuration object.

We can identify any of the existing uri flags using the regex found in pkg/runtime/from_config.go, if none are matched then we can attempt to parse the argument into a struct similar to the following:

type UriConfiguration struct {
	URI             string `json:"uri"`
	ProviderID      string `json:"providerID,omitempty"`
	HTTPBearerToken string `json:"httpBearerToken,omitempty"`
}

If the string argument provided matches the regex, then we can create an 'empty configuration'.

We then pass this configuration to each of the providers, deprecating the existing '--sync-provider-args' flag and maintaining the backwards compatibility of the --uri flag.

Ive looked into implementing this with a viper.DecodeHook, and while possible, it will likely be far simpler to implement this using a helper function in the start command definition.

@james-milligan
Copy link
Contributor

james-milligan commented Feb 24, 2023

Im going to implement and open a PR for the string-ified object approach:

you could either pass:

--uri file://my-file.json

or:

--uri '{"uri":"my-file.json","arg":1}'

internally we would parse the response as a map[string]interface{} configuration object if the argument fails all existing sync provider regex. if they pass, a new object will be created, and the uri field set.

@beeme1mr beeme1mr changed the title [research] How best to support advanced configuration per sync provider Research the best way to support advanced configuration per sync provider Feb 24, 2023
@beeme1mr
Copy link
Member

Hey @james-milligan, I think the config object makes sense but I'm not sure if repurposing the --uri flag makes sense. Perhaps we should consider leaving uri as is and reintroducing --sync-provider and have that take the config object. It would also be a good opportunity to deprecate sync-provider-args.

What do you think @toddbaert?

@toddbaert toddbaert moved this from Todo to In Progress in ⚡ Realtime flag updates via gRPC in K8s Feb 27, 2023
@toddbaert
Copy link
Member Author

Hey @james-milligan, I think the config object makes sense but I'm not sure if repurposing the --uri flag makes sense. Perhaps we should consider leaving uri as is and reintroducing --sync-provider and have that take the config object. It would also be a good opportunity to deprecate sync-provider-args.

What do you think @toddbaert?

I agree with this idea. --sources might be better, but that's up for debate.

james-milligan added a commit that referenced this issue Mar 9, 2023
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

- introduces the `SyncProviderConfig` object and associated start flags
to pass configuration to specific sync providers

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

#405

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->

---------

Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Co-authored-by: Skye Gill <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants