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

docs(specification): Flag set specification proposal #2734

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

thomaspoignant
Copy link
Owner

@thomaspoignant thomaspoignant commented Nov 27, 2024

Description

Important

This PR aims to be collaborative, all proposals are welcome to design the best solution.

This pull request contains a proposal specification for the flag set capability in GO Feature Flag (#2752).
Based on recent discussions in the community, this is the most requested features we had recently.

In this PR there is a description of the problem space, plus some proposals on how it can look from a user perspective.
All feedbacks are more than welcome 🙏.

Note

If you want a more readable version of the document, go directly here.


For reference, a flag set is:

A collection of related flags. This grouping helps organize feature flags based on their intended use, facilitating easier management and deployment.
Source: Openfeature glossary.

Copy link

netlify bot commented Nov 27, 2024

Deploy Preview for go-feature-flag-doc-preview ready!

Name Link
🔨 Latest commit c99b587
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/6763e2014be7cf00080be3c5
😎 Deploy Preview https://deploy-preview-2734--go-feature-flag-doc-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines +133 to +138
authorizedKeys:
evaluation:
- apikey1 # owner: userID1
- apikey2 # owner: userID2
admin:
- apikey3
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I am not sure how to configure the relation between the API keys and the flag sets, all proposals are welcome on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went through the API keys configuration docs and it looks like it accept any string. Since it's an array it can be organized so that the available flag sets for a specific api key are declared as well in the config file.

Another approach would be to generate something like JWTs to be used as API keys and encode flag set access data as claims. This path would be much more complex

Copy link
Owner Author

@thomaspoignant thomaspoignant Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went through the API keys configuration docs and it looks like it accept any string. Since it's an array it can be organized so that the available flag sets for a specific api key are declared as well in the config file.

Yes it can be anything here.
I am just not sure how to represent the configuration without breaking changes for existing config 🤔.

Another approach would be to generate something like JWTs to be used as API keys and encode flag set access data as claims. This path would be much more complex

I was also thinking about that, but it could be an evolution for a later stage.

Comment on lines +186 to +191
authorizedKeys:
evaluation:
- apikey1 # owner: userID1
- apikey2 # owner: userID2
admin:
- apikey3
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I am not sure how to configure the relation between the API keys and the flag sets, all proposals are welcome on this.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.77%. Comparing base (b37837d) to head (c99b587).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2734   +/-   ##
=======================================
  Coverage   84.77%   84.77%           
=======================================
  Files         111      111           
  Lines        5207     5207           
=======================================
  Hits         4414     4414           
  Misses        628      628           
  Partials      165      165           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomaspoignant thomaspoignant changed the title doc(specification): Flag set specification proposal docs(specification): Flag set specification proposal Nov 27, 2024
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
retrievers:
- kind: file
path: config-file1.goff.yaml
flagSet: flagset-teamA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by doing it this way it ends up more flexible.

Since it becomes part of the retriever config and not the flag definition itself it won't be necessary to come up with different approaches to retrievers like Redis and MongoDB.

Does it make sense?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, it makes more sense to put it outside of the configuration and move it to the retriever configuration.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused how this would work in practice though? If tomorrow a new team comes in and they need their own flag set, then GOFF would need to be redeployed again with a new configuration for another retriever. In our companies' deployment/FF solution, we're hoping each development project in each team is an individual flag set to avoid possible conflict, which means every new project on every team that's created would require a production redeployment of go-feature-flag on our side. Obviously this isn't really maintainable, which is what our feature request aimed to solve.

Disregarding the feature request previously, would another solution such as having the flagset key defined per flag work? Each flag could have a flagset key that if not provided would be default (keeping original functionality), otherwise would set the flag as being owned by that flagset. The issue here is flags still have the same name in their source (S3, MongoDB, Redis, etc.), but this doesn't have to be an issue for goff as when it loads flags into its memory it can use the flagset to distinguish flags internally.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused how this would work in practice though? If tomorrow a new team comes in and they need their own flag set, then GOFF would need to be redeployed again with a new configuration for another retriever. In our companies' deployment/FF solution, we're hoping each development project in each team is an individual flag set to avoid possible conflict, which means every new project on every team that's created would require a production redeployment of go-feature-flag on our side. Obviously this isn't really maintainable, which is what our #2314 aimed to solve.

@iisharankov your use case is still in my mind but I want to first introduce the notion of flag set and later address your usecase.
All the proposals are still valid with your use case, it will just need to have some kind of meta retriever that will be able to automatically create a flag set.

I place your need for now in the Out of scope section, but as soon as we have the flag set ready we will be able to work on this too.

Disregarding the feature request previously, would another solution such as having the flagset key defined per flag work? Each flag could have a flagset key that if not provided would be default (keeping original functionality), otherwise would set the flag as being owned by that flagset. The issue here is flags still have the same name in their source (S3, MongoDB, Redis, etc.), but this doesn't have to be an issue for goff as when it loads flags into its memory it can use the flagset to distinguish flags internally.

Yes that was my original idea, but we will have limits because of unicity of flag name here.
See the example in my comments here: #2734 (comment)

Comment on lines +133 to +138
authorizedKeys:
evaluation:
- apikey1 # owner: userID1
- apikey2 # owner: userID2
admin:
- apikey3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went through the API keys configuration docs and it looks like it accept any string. Since it's an array it can be organized so that the available flag sets for a specific api key are declared as well in the config file.

Another approach would be to generate something like JWTs to be used as API keys and encode flag set access data as claims. This path would be much more complex

@gogbog
Copy link

gogbog commented Nov 29, 2024

Proposal:

Solution: In this solution, we configure each flag individually to determine whether it should be included in a set. This approach allows us to maintain a single retriever in the GOFF configuration with multiple FlagSets. Whenever a new flag is introduced, we can simply check the parameters in the JSON object to detect whether the user intends to include it in a FlagSet.

Example:

featureA-enabled:
    flagSet: flagset-teamA
    variations:
        enabled: true
        disabled: false
    defaultRule:
        variation: enabled

PROS:

  • My opinion is that this would be the most flexible solution, as the FlagSets will become highly dynamic and specific to individual flags.
  • We will be able to control the FlagSets, without redeploying GOFF or adding new receivers. (Solving @iisharankov problem mentioned above)
  • MongoDB would still work, as the best design would currently involve placing each flag within a single document (single JSON object).

CONS:

  • This could be the hardest solution to implement yet, I'm not sure.
  • Harder to keep up which flag is in which set
  • Every time when a new flag gets introduced we would have to check if a new FlagSet comes up and modify the cache accordingly

@thomaspoignant looking forward to seeing what you and the community think about this solution.

@thomaspoignant
Copy link
Owner Author

@gogbog your proposal was one of my original idea but we will not be able to have this statement:

We want to allow 2 flags with the same name if they are used by different teams or platforms.

Why ?
Because the YAML file will be invalid if we try to redefine 2 flags with the same name.

Example:

featureA-enabled:
    flagSet: flagset-teamA
    variations:
        enabled: true
        disabled: false
    defaultRule:
        variation: enabled

featureA-enabled:
    flagSet: flagset-teamB
    variations:
        enabled: true
        disabled: false
    defaultRule:
        variation: enabled

Also it does not really allow a team to edit their file autonomously without impacting each others.

@gogbog
Copy link

gogbog commented Dec 2, 2024

@thomaspoignant , you're right, I completely forgot about the requirement for having two flags with the same name. Unfortunately, if we want to support this, we would be forced to use multiple retrievers for each file, which leads to the challenge with dynamic receivers and redeployment. Currently, we're using MongoDB as a retriever, and I'm curious how would the JSON representation look like if we choose to implement Solution 1 and would it be possible?

@thomaspoignant
Copy link
Owner Author

Currently, we're using MongoDB as a retriever, and I'm curious how would the JSON representation look like if we choose to implement Solution 1 and would it be possible?

Yes you're right this solution since not the best one with MongoDB and other DBs.
The more I look at it, the more solution 2 seems to be the way to go.

@tomflenner
Copy link
Contributor

Hey there !

I am bringing our use case here to see if we can find an elegant solution with this flagSet feature to answer the problem !

We're using GOFF from relayproxy with a HTTP Retriever that is a simple endpoint which build a JSON payload based on stuff in database. The feature flag data are modelized in database by feature and group up by stuff like userId, clientId etc.

The problem we're facing is that we are multitenant and we might dont want mix up all our client feature flag in a single configuration when the relayproxy fetch data from the HTTP Retriever.

Do you have more details on how this flagSet will be used with HTTP Retriever ? Do we have a way to specify for example per instance or per client the set of data that is needed to being retrieved from the HTTP Retriever ?

@thomaspoignant
Copy link
Owner Author

Do you have more details on how this flagSet will be used with HTTP Retriever ? Do we have a way to specify for example per instance or per client the set of data that is needed to being retrieved from the HTTP Retriever ?

Yes your point is similar to the one of @iisharankov and I get it completely.
I want to proceed in 2 steps for the flag sets

  1. Enable flag sets in GO Feature Flag (in a static way).
  2. Be able to dynamically generate flag sets from a single provider.

Signed-off-by: Thomas Poignant <[email protected]>
@thomaspoignant
Copy link
Owner Author

Based on your feedback, I have proposed a new solution that seems more aligned with the concerns that some of you have raised.

I'll be happy to get your feedback on this one.
cc @tomflenner @gogbog @luizgribeiro @iisharankov

@thomaspoignant
Copy link
Owner Author

I also have an extra question, how do you think the exporter and notifier should work with a flag set?

  • Should we export everything to the same destination?
  • Should we notify all the flag changes or notify only per flag set?

Repository owner deleted a comment from sonarqubecloud bot Dec 18, 2024
Repository owner deleted a comment from sonarqubecloud bot Dec 18, 2024
@thomaspoignant
Copy link
Owner Author

thomaspoignant commented Dec 18, 2024

Another idea of configuration that came to my mind (and that is probably the best one) is something like this:

Where type: static means that the retriever will return only 1 flagset and type: dynamic can return multiple flagset.

listen: 1031
pollingInterval: 10000
startWithRetrieverError: false

# default flag set (no need to specify anything)
retrievers:
  - kind: file
    path: /xxx/flags.goff.json
exporter:
  kind: log
notifier:
  - kind: discord
    webhookUrl: https://xxx.xxx
  - kind: microsoftteams
    webhookUrl: https://xxx.xxx


# static flag set 
flagSets:
  - type: static
    name: teamA
    retrievers:
      - kind: file
        path: /xxx/flags2.goff.json 
    notifier:
      - kind: discord
        webhookUrl: https://xxx.xxx
      - kind: microsoftteams
        webhookUrl: https://xxx.xxx
    exporter:
      kind: log
  
  - type: dynamic
    retrievers:
      - kind: file
        path: /xxx/flags3.goff.json
    notifier:
      - kind: discord
        webhookUrl: https://xxx.xxx
      - kind: microsoftteams
        webhookUrl: https://xxx.xxx
    exporter:
      kind: log

  - type: dynamic
    retrievers:
      - kind: file
        path: /xxx/flags4.goff.json
    notifier:
      - kind: discord
        webhookUrl: https://xxx.xxx
      - kind: microsoftteams
        webhookUrl: https://xxx.xxx
    exporter:
      kind: log

I am not 100% sure if we should support dynamic flagsets for now, because it seems reasonable to me to change the configuration if you add one more team / tenant to the solution.
But I am happy to hear your feedback about this.

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

Successfully merging this pull request may close these issues.

5 participants