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: introduce grpc sync for flagd #297

Merged
merged 26 commits into from
Feb 15, 2023

Conversation

Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan commented Jan 19, 2023

This PR

Attempts to resolve #249 by introducing grpc sync provider to flagd.

OFEP [approved] - https://github.com/open-feature/ofep/blob/main/OFEP-flagd-grpc-sync.md

How to test/run ?

Flagd acts as the grpc client, hence you need at least a minimal mock server. For this, you can utilize this [1] server implementation.

Startup arguments of flagd now support grpc target uri. This can be provided with grpc:// , for example,

./flagd start --uri grpc://localhost:8090

Technical highlights

  • GRPC protobuf definitions are available in buf [2] and are backed by the schema repository (https://github.com/open-feature/schemas)
  • Initial connection must be successful (i.e- grpc server/target must be accepting connections)
  • Subsequent server connection losses will not result in a runtime failure and connection re-establishment attempts will be performed

What is not included (follow up improvements)

  • Connection security: This version does not enforce connection security. This will be addressed with follow-up improvements (ex:- TLS enabled connections). Hence, strongly recommends not using this version in production scenarios (fixed by feat: tls connectivity for grpc syncs  #398)
  • Server implementations: This sync provider was designed to be open and connects to any server implementation. Hence there is no default server implementation. You may create your own server implementation based on grpc schemas.

[1] - https://github.com/Kavindu-Dodan/flagd-grpc-sync-server
[2] - https://buf.build/open-feature/flagd

@xvzf
Copy link

xvzf commented Feb 2, 2023

That's amazing!

@toddbaert
Copy link
Member

I'm going to give this a try and a thorough review on Monday!

@toddbaert
Copy link
Member

toddbaert commented Feb 6, 2023

Works for me! I couple things I noted in testing:

  • There's no way to change flag values in server, is there? I know this is just a toy demo server, so that makes sense, but I guess without that there's no way to test that the grpc sync gets updates appropriately? Or am I missing something in regards to how to test that?
  • I see 1.6757003525209875e+09 warn grpc/grpc_sync.go:111 receivied unknown state: 41 {"component": "sync", "sync": "grpc"} in logs - not sure, but this could be a gRPC keepalive - we might not want to log every message received as a warning.
  • If the server does, the connection fails and the client flagd crashes. I think we need to add elegant re-connection logic here and make this pretty resilient.

@Kavindu-Dodan
Copy link
Contributor Author

Works for me! I couple things I noted in testing:

  • There's no way to change flag values in server, is there? I know this is just a toy demo server, so that makes sense, but I guess without that there's no way to test that the grpc sync gets updates appropriately? Or am I missing something in regards to how to test that?
  • I see 1.6757003525209875e+09 warn grpc/grpc_sync.go:111 receivied unknown state: 41 {"component": "sync", "sync": "grpc"} in logs - not sure, but this could be a gRPC keepalive - we might not want to log every message received as a warning.
  • If the server does, the connection fails and the client flagd crashes. I think we need to add elegant re-connection logic here and make this pretty resilient.

Yeah, unfortunately, that's the downside of grpc. We need the server impl and currently, it's my toy server 😿 And the reason is simple, unlike file, crds, the server impl can be anything. It could be triggered from DB change, github workflow or anything. But maybe I can expand my server with few more use-cases 🤔

I addressed other concerns in respective comments :)

toddbaert added a commit to open-feature/flagd-schemas that referenced this pull request Feb 7, 2023
## This PR

Is related to open-feature/ofep#45 &
open-feature/flagd#297

Introduce the service contract of grpc sync. 

Consider checking my personal buf repository -
https://buf.build/kavindudodan/flagd for a preview

---------

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Co-authored-by: Todd Baert <toddbaert@gmail.com>
@Kavindu-Dodan Kavindu-Dodan force-pushed the feature/grpc-poc branch 2 times, most recently from 4bd7848 to 5058b62 Compare February 8, 2023 00:03
@Kavindu-Dodan
Copy link
Contributor Author

@toddbaert I have added connection retry logic with the latest change set :) if you can do another round of review, and make sure this fits flagd standard, thne I will convert this draft to a normal PR 🙌

@Kavindu-Dodan Kavindu-Dodan force-pushed the feature/grpc-poc branch 2 times, most recently from 0d5a588 to 8afee0e Compare February 8, 2023 17:40
@Kavindu-Dodan Kavindu-Dodan marked this pull request as ready for review February 8, 2023 18:04
Kavindu-Dodan and others added 16 commits February 14, 2023 07:30
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Co-authored-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Co-authored-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Copy link
Contributor

@skyerus skyerus left a comment

Choose a reason for hiding this comment

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

Minor concerns, nice work :)

pkg/sync/grpc/grpc_sync.go Outdated Show resolved Hide resolved
pkg/sync/grpc/grpc_sync.go Outdated Show resolved Hide resolved
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
pkg/sync/grpc/grpc_sync.go Outdated Show resolved Hide resolved
pkg/sync/grpc/grpc_sync.go Outdated Show resolved Hide resolved
Kavindu-Dodan and others added 4 commits February 15, 2023 07:19
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan Kavindu-Dodan merged commit 33413f2 into open-feature:main Feb 15, 2023
@Kavindu-Dodan Kavindu-Dodan deleted the feature/grpc-poc branch February 15, 2023 15:58
beeme1mr pushed a commit that referenced this pull request Mar 2, 2023
🤖 I have created a release *beep* *boop*
---


##
[0.4.0](v0.3.7...v0.4.0)
(2023-03-02)


### ⚠ BREAKING CHANGES

* Use OTel to export metrics (metric name changes)
([#419](#419))

### 🧹 Chore

* add additional sections to the release notes
([#449](#449))
([798f71a](798f71a))
* attach image sbom to release artefacts
([#407](#407))
([fb4ee50](fb4ee50))
* **deps:** update actions/configure-pages digest to fc89b04
([#417](#417))
([04014e7](04014e7))
* **deps:** update amannn/action-semantic-pull-request digest to b6bca70
([#441](#441))
([ce0ebe1](ce0ebe1))
* **deps:** update docker/login-action digest to ec9cdf0
([#437](#437))
([2650670](2650670))
* **deps:** update docker/metadata-action digest to 3343011
([#438](#438))
([e7ebf32](e7ebf32))
* **deps:** update github/codeql-action digest to 32dc499
([#439](#439))
([f91d91b](f91d91b))
* **deps:** update google-github-actions/release-please-action digest to
d3c71f9 ([#406](#406))
([6e1ffb2](6e1ffb2))
* disable caching tests in CI
([#442](#442))
([28a35f6](28a35f6))
* fix race condition on init read
([#409](#409))
([0c9eb23](0c9eb23))
* integration test stability
([#432](#432))
([5a6a5d5](5a6a5d5))
* integration tests
([#312](#312))
([6192ac8](6192ac8))
* reorder release note sections
([df7bfce](df7bfce))
* use -short flag in benchmark tests
([#431](#431))
([e68a6aa](e68a6aa))


### 🐛 Bug Fixes

* **deps:** update kubernetes packages to v0.26.2
([#450](#450))
([2885227](2885227))
* **deps:** update module github.com/bufbuild/connect-go to v1.5.2
([#416](#416))
([feb7f04](feb7f04))
* **deps:** update module
github.com/open-feature/go-sdk-contrib/providers/flagd to v0.1.9
([#427](#427))
([42d2705](42d2705))
* **deps:** update module github.com/open-feature/open-feature-operator
to v0.2.29 ([#429](#429))
([b7fae81](b7fae81))
* **deps:** update module github.com/stretchr/testify to v1.8.2
([#440](#440))
([ab3e674](ab3e674))
* **deps:** update module golang.org/x/net to v0.7.0
([#410](#410))
([c6133b6](c6133b6))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.14.5
([#454](#454))
([f907f11](f907f11))
* remove non-error error log from parseFractionalEvaluationData
([#446](#446))
([34aca79](34aca79))


### ✨ New Features

* add debug logging for merge behaviour
([#456](#456))
([dc71e84](dc71e84))
* add Health and Readiness probes
([#418](#418))
([7f2358c](7f2358c))
* Add version to startup message
([#430](#430))
([8daf613](8daf613))
* introduce flag merge behaviour
([#414](#414))
([524f65e](524f65e))
* introduce grpc sync for flagd
([#297](#297))
([33413f2](33413f2))
* refactor and improve K8s sync provider
([#443](#443))
([4c03bfc](4c03bfc))
* Use OTel to export metrics (metric name changes)
([#419](#419))
([eb3982a](eb3982a))


### 📚 Documentation

* add .net flagd provider
([73d7840](73d7840))
* configuration merge docs
([#455](#455))
([6cb66b1](6cb66b1))
* documentation for creating a provider
([#413](#413))
([d0c099d](d0c099d))
* updated filepaths for schema store regex
([#344](#344))
([2d0e9d9](2d0e9d9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Dec 2, 2023
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.

Real-time Flag Updates Beyond Kubernetes
7 participants