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

[Bug]: OFREP Bulk Evaluation #3412

Closed
1 task done
lukas-reining opened this issue Aug 27, 2024 · 8 comments · Fixed by #3556
Closed
1 task done

[Bug]: OFREP Bulk Evaluation #3412

lukas-reining opened this issue Aug 27, 2024 · 8 comments · Fixed by #3556
Labels

Comments

@lukas-reining
Copy link

Bug Description

Hey, I tried to use the OFREP client provider [1] with flipt.
The implementation of OFREP in flipt looks great, but there is one thing that does not fit to how we intended the bulk evaluation endpoint to be used.

When I try to use the OFREP client provider I get the following error: {"errorCode":"INVALID_CONTEXT","errorDetails":"flags were not provided in context"} and I can find the line in the code that returns this. [2]

This behavior does not fit to the intended use of the endpoint as we described in the following ADR: [3]
As this describes, for the client we expect all the flags to be loaded for synchronous evaluation.

Do you have a reason for adding the flags context key as mandatory? If not can we make it optional or remove it?

[1] https://github.com/open-feature/js-sdk-contrib/tree/main/libs/providers/ofrep-web
[2] https://github.com/open-feature/protocol/blob/main/service/adrs/0002-two-evaluation-endpoints.md#context
[3] https://github.com/open-feature/protocol/blob/main/service/adrs/0002-two-evaluation-endpoints.md#2-two-evaluation-endpoints

Version Info

v1.48.1

Search

  • I searched for other open and closed issues before opening this

Steps to Reproduce

Try this:

curl --request POST \
  --url https://try.flipt.io/ofrep/v1/evaluate/flags \
  --header 'Content-Type: application/json' \
  --header 'Accept: application/json' \
  --header 'X-Flipt-Namespace: default' \
  --data '{
  "context": {
    "targetingKey": "targetingKey1"
  }
}'

or try to use the OFREP client provider with flipt: https://github.com/open-feature/js-sdk-contrib/tree/main/libs/providers/ofrep-web#configurations-and-usage

Expected Behavior

No response

Additional Context

No response

@GeorgeMac
Copy link
Contributor

Thanks for reporting this @lukas-reining
Bare with me while I report this phishing nonsense 😓

@flipt-io flipt-io deleted a comment Aug 27, 2024
@flipt-io flipt-io deleted a comment Aug 27, 2024
@GeorgeMac
Copy link
Contributor

GeorgeMac commented Aug 27, 2024

Just for posterity I reported and then deleted a couple of phising comments.
They contained some funky looking links, which I can only assume contained malicious content.

Update: GitHub acknowledged they were malicious and I think the accounts have been removed.

@erka
Copy link
Collaborator

erka commented Aug 27, 2024

Thank you for your report @lukas-reining

I'd really like to discuss this https://github.com/open-feature/protocol/blob/main/service/adrs/0004-no-flag-filter-in-bulk-evaluation-request.md as well. How do you interpret the phrase For systems that rely on a list of flags? The definition seems a little unclear on how those systems should handle requests that don’t include flags.

@lukas-reining
Copy link
Author

lukas-reining commented Aug 27, 2024

I think you found a problem in the ADR there @erka.
In my opinion, this is just a proposal for all those who want to optionally have the option to filter for specific flags and I think we should fix that wording in the ADR.
To be able to support the static context paradigm, in our case the OFREP Web SDK, a service needs to support loading all the flags I think. Or the provider implementation has to get an option for specifying that, but I would prefer the first option.
What do you think @erka?
cc @beemer @toddbaert @Kavindu-Dodan @thomaspoignant

@erka
Copy link
Collaborator

erka commented Aug 27, 2024

@lukas-reining Your question is quite challenging. I understand the concept, and it's a great solution for a small number of flags. However, in practice, a flag provider and an enterprise customer might deal with dozens or even hundreds of flags at once. Moreover, not all of those flags will be web or UI-related or client-related. It's similar to the REST vs GraphQL debate.

This is why I would prefer to see the specification with clearly defined expectations for bulk evaluation.

@lukas-reining
Copy link
Author

lukas-reining commented Aug 27, 2024

Mh I get that this is quite challenging and know the cases of enterprises with hundreds of flags @erka.
But this is what really many of the provider SDKs are doing and you are also sending the whole ruleset for a namespace to the client, so what differentiates this is that the evaluation is taking place in the client instead of the server. The amount of transferred data is more in that approach, while the server processing is less [1][2].

I am completely with you that our spec should be more precise on the reason for and expected behavior of the bulk evaluation endpoint. But please also keep in mind that OFREP is really experimental right now and we are really happy to get feedback from the industry here.

So what do you think could we make it possible to have the bulk evaluation without a filter, or do you see any point missing about the spec to make it more compatible with how you are doing the bulk/client evaluation?
One thing that comes to my mind, is that you could expect the namespace as optional context property and return the flags of default if no namespace was given. But maybe you have a better idea.

[1] https://github.com/flipt-io/flipt-client-sdks/blob/4821cb227c6c8b10419b96674d44ad1d6668a647/flipt-client-browser/src/index.ts#L41
[2]

@erka
Copy link
Collaborator

erka commented Aug 28, 2024

@lukas-reining I’m afraid I’m not in a position to discuss the internal decisions of any flag providers.

Back to the OFREP bulk evaluation, it seems like you’re aiming for a design based on a static context paradigm. One challenge I encountered is that this approach isn’t well documented in the OpenAPI specification and the ofrep-web provider. As alien I personally had some difficulty with ofrep-web. Even after following the examples in the repository, I only received the default value, and my flag evaluation context didn’t seem to impact the flag evaluation. It was only after digging into the source and setting OpenFeature.setContext({...}); ...; await OpenFeature.setProviderAndWait(...); that I was able to achieve the expected result.
Additionally, I was wondering about the best approach for a scenario where a service needs to evaluate 5 flags for each of 1,000 users during a batch or cron job execution using the OFREP API. What your guidance could be for this case?

As you said that OFREP is experimental the Flipt implementation is the same. We are open to making adjustments as we gain a better understanding of the design and goals.

@erka
Copy link
Collaborator

erka commented Aug 29, 2024

@GeorgeMac @markphelps @lukas-reining I have created the issue. Please add the details which I may have missed

erka added a commit that referenced this issue Oct 26, 2024
Rework bulk evaluation logic to fetch namespace flags
if ones are not provided in the evaluation context. As result
all boolean and enabled variant flags will be evaluated.

fixes #3412

Signed-off-by: Roman Dmytrenko <[email protected]>
erka added a commit that referenced this issue Oct 26, 2024
Rework bulk evaluation logic to fetch namespace flags
if ones are not provided in the evaluation context. As result
all boolean and enabled variant flags will be evaluated.

fixes #3412

Signed-off-by: Roman Dmytrenko <[email protected]>
erka added a commit that referenced this issue Oct 27, 2024
Rework bulk evaluation logic to fetch namespace flags
if ones are not provided in the evaluation context. As result
all boolean and enabled variant flags will be evaluated.

fixes #3412

Signed-off-by: Roman Dmytrenko <[email protected]>
@erka erka closed this as completed in 3b2c25e Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants
@erka @GeorgeMac @lukas-reining and others