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

feat: add provider configuration for ofrep #3247

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

thepabloaguilar
Copy link
Contributor

This PR can be considered the very first step towards the implementation of OpenFeature Remote Evaluation Protocol!

It's the implementation of an optional endpoint used by the protocol, I've decided to implement this endpoint first instead of the others (which actually are mandatory) because its simplicity and we can discuss some things here better.

The first thing you may ask is about the endpoint route, which is: /ofrep/*/ofrep/v1/configuration
Why we have ofrep twice? The answer is, because OFREP defines all the routes as follow:

  • /ofrep/v1/evaluate/flags/{key}
  • /ofrep/v1/evaluate/flags
  • /ofrep/v1/configuration

And as you can see there's a important part missing in those, the namespace flipt supports! So the strategy we can use is always prefix with /ofrep/{namespace} and when instantiating the ofrep client just give the address as follow: http://my.flipt/ofrep/my-namespace

Refs #2980

Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.37%. Comparing base (9d1dfaa) to head (80f124e).

Files Patch % Lines
internal/cmd/http.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3247      +/-   ##
==========================================
- Coverage   63.38%   63.37%   -0.02%     
==========================================
  Files         164      166       +2     
  Lines       13317    13347      +30     
==========================================
+ Hits         8441     8458      +17     
- Misses       4222     4231       +9     
- Partials      654      658       +4     
Flag Coverage Δ
unittests 63.37% <86.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@erka erka changed the title feat: add provider configuration for oprep feat: add provider configuration for ofrep Jul 6, 2024
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

This is looking great @thepabloaguilar. Thanks for taking it on.
One thought on here regarding our SDK generator and that path.
Might be worth skipping the SDK generation for the OFREP service as we're probably never going to use it.

rpc/flipt/flipt.yaml Outdated Show resolved Hide resolved
sdk/go/http/ofrep.sdk.gen.go Outdated Show resolved Hide resolved
@markphelps
Copy link
Collaborator

Hey @thepabloaguilar thanks for getting this work started!! Very much appreciated!

I actually asked in the OpenFeature Slack channel a couple months ago about the best way to support namespaces in our case, and they suggested we use the custom header support that OFREP clients already provide.

Here's the link to the slack thread (you need to be in the CNFC slack first I think): https://cloud-native.slack.com/archives/C066A48LK35/p1714414882954439

So in our case we would do something like this (using Go OFREP provider as an example):

provider := ofrep.NewProvider(
    "http://localhost:8080",
    ofrep.WithHeaderProvider(func() (name string, value string) {
      return "X-Flipt-Namespace", "foo"
    })
)

Then in our server side impl we would need to pull out this header value to get the correct namespace

@thepabloaguilar
Copy link
Contributor Author

Thanks for the comment @markphelps! I thought about using the header strategy first but I was like: Idk if it's correct to put a specific thing in something which is trying to generalize the interactions. But I'd stick with that, will add the header handling

@thepabloaguilar
Copy link
Contributor Author

I've tested locally and it seems to be compliant with the OpenAPI Spec:

curl localhost:8080/ofrep/v1/configuration
{"name":"flipt","capabilities":{"cacheInvalidation":{"polling":{"enabled":false,"minPollingIntervalMs":"0"}},"flagEvaluation":{"supportedTypes":["string","boolean"]}}}

Prettyfied:

{
  "name": "flipt",
  "capabilities": {
    "cacheInvalidation": {
      "polling": {
        "enabled": false,
        "minPollingIntervalMs": "0"
      }
    },
    "flagEvaluation": {
      "supportedTypes": [
        "string",
        "boolean"
      ]
    }
  }
}

@erka
Copy link
Collaborator

erka commented Jul 7, 2024

I've tested locally and it seems to be compliant with the OpenAPI Spec:

curl localhost:8080/ofrep/v1/configuration
{"name":"flipt","capabilities":{"cacheInvalidation":{"polling":{"enabled":false,"minPollingIntervalMs":"0"}},"flagEvaluation":{"supportedTypes":["string","boolean"]}}}

Hi @thepabloaguilar. Great work of OFREP!

There is one small thing that isn't in complaint with OFREP OpenAPI Spec:

minPollingIntervalMs:
  type: number
     description: |
         Minimum polling interval (in millisecond) supported by the flag management system.
          The provider should ensure not to set any polling value under this minimum.
      examples:
          - 60000

In our case, minPollingIntervalMs is a string not a number. This discrepancy arises because in gRPC/protobuf, int64 is encoded as a string. To be fully compliant with the OFREP spec, we could change minPollingIntervalMs to something like uint32. WDYT?

@thepabloaguilar
Copy link
Contributor Author

Sure @erka, I've just pushed the changes! But AFAIK even as a string it's a valid number for the JSON parsers

@erka
Copy link
Collaborator

erka commented Jul 8, 2024

Sure @erka, I've just pushed the changes! But AFAIK even as a string it's a valid number for the JSON parsers

@thepabloaguilar Unfortunately, Go doesn't

var r map[string]int
err := json.Unmarshal([]byte(`{"a":"1"}`), &r)
fmt.Println(r, err)
// Output:
// map[a:0] json: cannot unmarshal string into Go value of type int

@erka
Copy link
Collaborator

erka commented Jul 8, 2024

@thepabloaguilar One downside with mage proto is that it doesn't remove previously generated files when proto files change. We have to removesdk/go/http/ofrep.sdk.gen.go and sdk/go/ofrep.sdk.gen.go manually :(

@thepabloaguilar
Copy link
Contributor Author

@erka done!

@markphelps markphelps requested review from erka and a team July 8, 2024 19:40
Copy link
Collaborator

@erka erka left a comment

Choose a reason for hiding this comment

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

@thepabloaguilar Excellent beginning on the new feature!

Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Signed-off-by: Pablo Aguilar <pablo.aguilar@outlook.com.br>
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looks great to me @thepabloaguilar !! Thank you for kicking off this work 🚀

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jul 9, 2024
@kodiakhq kodiakhq bot merged commit 297c98e into flipt-io:main Jul 9, 2024
33 of 34 checks passed
@thepabloaguilar thepabloaguilar deleted the issue-2980-config branch July 9, 2024 16:05
@thepabloaguilar
Copy link
Contributor Author

I'm already working on the evaluate endpoint!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants