Skip to content

feat: grpc tls connectivity (grpcs)#477

Merged
beeme1mr merged 36 commits intoopen-feature:mainfrom
Kavindu-Dodan:feat/grpc-tlc-con
Mar 9, 2023
Merged

feat: grpc tls connectivity (grpcs)#477
beeme1mr merged 36 commits intoopen-feature:mainfrom
Kavindu-Dodan:feat/grpc-tlc-con

Conversation

@Kavindu-Dodan
Copy link
Copy Markdown
Contributor

This PR

Introduce TLS connectivity for GRPC sync provider.

TLS can be enabled using schema grpcs://. For example,

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

Further, a self-sign certificate can be provided for TLS connectivity using configuration source field certPath

ex:- ./flagd start --sources='[{"uri":"grpcs://localhost:9090","provider":"grpc", "certPath":"<CA_CERT>"}]'

How to test

Start mock server impl - https://github.com/Kavindu-Dodan/flagd-grpc-sync-server & then run flagd with grpc tls mode

james-milligan and others added 30 commits February 28, 2023 10:46
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan Kavindu-Dodan changed the title Feat/grpc tlc con feat: grpc tls connectivity (grpcs) Mar 8, 2023
# Conflicts:
#	cmd/start.go
#	docs/configuration/configuration.md
#	docs/configuration/flagd_start.md
#	pkg/runtime/from_config.go
#	pkg/runtime/runtime.go
#	pkg/runtime/runtime_test.go
#	pkg/sync/file/filepath_sync.go
#	pkg/sync/grpc/grpc_sync.go
#	pkg/sync/grpc/grpc_sync_test.go
#	pkg/sync/http/http_sync.go
#	pkg/sync/isync.go
#	pkg/sync/kubernetes/kubernetes_sync.go
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan Kavindu-Dodan marked this pull request as ready for review March 9, 2023 16:22
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 9, 2023

Codecov Report

Merging #477 (8a69ba8) into main (1762503) will increase coverage by 1.30%.
The diff coverage is 73.13%.

@@            Coverage Diff             @@
##             main     #477      +/-   ##
==========================================
+ Coverage   62.08%   63.39%   +1.30%     
==========================================
  Files          15       15              
  Lines        1891     1923      +32     
==========================================
+ Hits         1174     1219      +45     
+ Misses        654      639      -15     
- Partials       63       65       +2     
Impacted Files Coverage Δ
pkg/runtime/from_config.go 25.60% <50.00%> (+0.30%) ⬆️
pkg/sync/grpc/grpc_sync.go 77.24% <74.60%> (+15.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@beeme1mr beeme1mr requested a review from skyerus March 9, 2023 16:41
@beeme1mr
Copy link
Copy Markdown
Member

beeme1mr commented Mar 9, 2023

This can be tested using the test server [1] @Kavindu-Dodan created and generating self signed certs.

Generate CA cert

  • CA Private Key: openssl ecparam -name prime256v1 -genkey -noout -out ca.key
  • CA Certificate: openssl req -new -x509 -sha256 -key ca.key -out ca.cert

Generate Server certificate

  • Create OpenSSL config: echo "subjectAltName = DNS:localhost" > openssl.conf
  • Server private key: openssl ecparam -name prime256v1 -genkey -noout -out server.key
  • Server signing request: openssl req -new -sha256 -addext "subjectAltName=DNS:localhost" -key server.key -out server.csr
  • Server cert: openssl x509 -req -in server.csr -CA ca.cert -CAkey ca.key -out server.crt -days 1000 -sha256 -extfile openssl.conf

Where the file opnessl.conf contains following,

subjectAltName = DNS:localhost

Once certificates & keys are ready,

  • Running grpc server with certificates - go run main.go -s=true -certPath=server.crt -keyPath=server.key

  • Running flagd with certificates - go run main.go start --sources='[{"uri":"grpcs://localhost:9090","provider":"grpc", "certPath":"ca.cert"}]'

[1] https://github.com/Kavindu-Dodan/flagd-grpc-sync-server

Copy link
Copy Markdown
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Largely this is working, but I believe it caused a regression in our reconnect which causes flagd to crash when the server connection is lost. This tells me we need a test, e2e or otherwise, for this... I will create a new issue for that. I think the crash itself needs to be resolved in this PR though.

Additionally, grpcs:// is not really a valid scheme. I think it's fine to keep for the uri style configuration, but I think a boolean for tls might make more sense for the --sources style configuration. This more a matter of opinion though... I just feel weird about relying on a non-standard scheme wherever we don't have to.

Edit: I created this issue for testing.

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Copy Markdown
Contributor Author

Largely this is working, but I believe it caused a regression in our reconnect which causes flagd to crash when the server connection is lost. This tells me we need a test, e2e or otherwise, for this... I will create a new issue for that. I think the crash itself needs to be resolved in this PR though.

Additionally, grpcs:// is not really a valid scheme. I think it's fine to keep for the uri style configuration, but I think a boolean for tls might make more sense for the --sources style configuration. This more a matter of opinion though... I just feel weird about relying on a non-standard scheme wherever we don't have to.

Edit: I created this issue for testing.

Good catch. I fixed this and added a unit test to validate this scenario

I also have no hard opinion on the grpcs:// schema usage. But if there are no other alternatives, this is fine :)

Comment thread pkg/sync/grpc/grpc_sync.go
Comment thread pkg/sync/grpc/grpc_sync_test.go
@toddbaert
Copy link
Copy Markdown
Member

Confirmed reconnect works now.

@beeme1mr beeme1mr merged commit 228f430 into open-feature:main Mar 9, 2023
beeme1mr pushed a commit that referenced this pull request Mar 9, 2023
🤖 I have created a release *beep* *boop*
---


##
[0.4.2](v0.4.1...v0.4.2)
(2023-03-09)


### 🧹 Chore

* Add targeted Flag to example config
([#467](#467))
([6a039ce](6a039ce))
* **deps:** pin dependencies
([#473](#473))
([679e860](679e860))
* **deps:** update google-github-actions/release-please-action digest to
e0b9d18 ([#474](#474))
([5b85b2a](5b85b2a))
* refactoring and improve coverage for K8s Sync
([#466](#466))
([6dc441e](6dc441e))


### 🐛 Bug Fixes

* add registry login
([#476](#476))
([99de755](99de755))
* **deps:** update module golang.org/x/crypto to v0.7.0
([#472](#472))
([f53f6c8](f53f6c8))
* **deps:** update module google.golang.org/protobuf to v1.29.0
([#478](#478))
([f9adc8e](f9adc8e))


### ✨ New Features

* grpc tls connectivity (grpcs)
([#477](#477))
([228f430](228f430))
* introduce per-sync configurations
([#448](#448))
([1d80039](1d80039))

---
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>
@Kavindu-Dodan Kavindu-Dodan deleted the feat/grpc-tlc-con branch March 9, 2023 22:13
@github-actions github-actions Bot mentioned this pull request Dec 2, 2023
raphael-wigoutschnigg-dt pushed a commit to open-feature-forking/flagd that referenced this pull request Mar 11, 2025
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
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.

4 participants