[Feature] JSON merge function for multiple files

[AlexsJones]

In order to combine multiple feature flag json files ( in the case of aggregation etc ) Then we should have a function to merge multiple json files....

e.g.

MergeJSON(jsonPayload [][]byte) ([]byte, error)

[moshebe]

I can work on this one if it's not assigned yet 🤚

[AlexsJones]

Awesome thanks, we need the ability to combine files with a couple of different strategies e..g.

A overwrite B A merge B A additive to B

Does that make sense?

[moshebe]

Sure. Can you please elaborate more about the use cases you think about? I would like to understand the problem better before approaching the solution (e.g. do we need to merge only the top level JSON keys or a deeper merge is needed)

[AlexsJones]

My thought is that we might want to support multiple input files in flagD, for example ./flagd start -f /var/temp/flags-default.json -f /var/temp/flags-new.json

In this case, we'd need to look at a form of flag merging, which could be based on the precedence of files. You could use the spec for guidance on which and how to merge keys.

I think with this knowledge you could walk through the top-level keys and do a hash on the object to detect if you'd need to step in and then compare the next level keys.

[beeme1mr]

I would advise against attempting to merge flags with the same key. In my opinion, if a duplicate flag key is detected, the first value would be used. Attempting to merge these values would likely lead to unexpected behavior.

[AlexsJones]

Perhaps a proposal in the research repo would be a good place to start @moshebe ?

[toddbaert]

I'm personally not sure we need a research item. The problem here as I understand it is pretty simple. Because we want to support multiple CR instances per deployment, flagd needs to be able to read and merge multiple files when using the json_evaluator implementation of the IEvaluator interface . The JSON files used may have conflicting keys. I think this would be a pretty unreasonable configuration to support beyond simply overwriting the duplicated flag key (why would I configure to CRDs, both containing a flag called new-welcome-message?).

I think all we need to do is have the "last key win"... so that if I have to JSON files with the same flag key, the one configured last overwrites the previous duplicate.

cc @AlexsJones @beeme1mr

[thomaspoignant]

I am not 100% sure to understand the value of merging files? Do you see any reason why you would split your flags into multiple files?

[AlexsJones]

I'm personally not sure we need a research item. The problem here as I understand it is pretty simple. Because we want to support multiple CR instances per deployment, flagd needs to be able to read and merge multiple files when using the json_evaluator implementation of the IEvaluator interface . The JSON files used may have conflicting keys. I think this would be a pretty unreasonable configuration to support beyond simply overwriting the duplicated flag key (why would I configure to CRDs, both containing a flag called new-welcome-message?).

I think all we need to do is have the "last key win"... so that if I have to JSON files with the same flag key, the one configured last overwrites the previous duplicate.

cc @AlexsJones @beeme1mr

Agree with last key wins to keep it simple

[toddbaert]

@thomaspoignant

Do you see any reason why you would split your flags into multiple files?

In the K8s situation, it makes a lot of since. A user might have one CR instance that defines flags shared by many services, and another that only applies to a single service. For example:

common-flags.yaml

apiVersion: core.openfeature.dev/v1alpha1
kind: FeatureFlagConfiguration
metadata:
  name: common-flags
spec:
  featureFlagSpec: |
    {
      "flags": {
        "common-flag": {...}
    }

specific-flags.yaml

apiVersion: core.openfeature.dev/v1alpha1
kind: FeatureFlagConfiguration
metadata:
  name: specific-flags
spec:
  featureFlagSpec: |
    {
      "flags": {
        "flag-for-this-service": {...}
    }

deployment.yaml:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: my-app-deployment
  labels:
    app: my-app
spec:
  replicas: 1
  selector:
    matchLabels:
      app: my-app
  template:
    metadata:
      labels:
        app: my-app
      annotations:
        openfeature.dev: "enabled"
        openfeature.dev/featureflagconfiguration: ["common-flags", "specific-flags"]

Flagd would mount both of these, and merge them. With the last winning.

[moshebe]

Sorry for the delay wasn't available in the past few days. I tend to agree with the use-case of handling multiple files with the first-come-first-served policy for the keys collision. I'll start working on the JSON merge function & tests.

Question regarding the files watch: The json_evaluator has state field contains the current configuration - would we want to perform the merge under the SetState method? (currently it replaces the current state)

(Maybe I'm missing something, still new to the code - but it seems like the notifier is watching a single file right now, not sure where do we plan do make the merge)

[toddbaert]

Question regarding the files watch: The json_evaluator has state field contains the current configuration - would we want to perform the merge under the SetState method? (currently it replaces the current state)

Yep, I think this is the right way to go. The eval package is the part that "knows" how to deal with the actual structure of the flags (only mentioning this to make the purpose clear, but at some point we might implement an IEvaluator that supports flags defined in a format other than JSON). So the merge should happen there, I think.

(Maybe I'm missing something, still new to the code - but it seems like the notifier is watching a single file right now, not sure where do we plan do make the merge)

The sync package (which has the notifier) handles picking up the data, but NOT it's format/structure, it's agnostic to that. I think you just need to make the notifier support multiple files, but the merging won't be its concern.

I hope that helps!

[moshebe]

Thanks @toddbaert, sounds good.

I created a draft PR for whom is interested. Will update here when it will be ready to review.

[moshebe]

Hi guys, just marked my PR as ready for review - looking forward for your feedback 🙏

[AlexsJones]

I'm on PTO till 20th but will try to take a look at some Point