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: grpc tls connectivity (grpcs) #477

Merged
merged 36 commits into from
Mar 9, 2023

Conversation

Kavindu-Dodan
Copy link
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 <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Co-authored-by: Skye Gill <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@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 <[email protected]>
@Kavindu-Dodan Kavindu-Dodan marked this pull request as ready for review March 9, 2023 16:22
@codecov
Copy link

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

@beeme1mr beeme1mr requested a review from skyerus March 9, 2023 16:41
@beeme1mr
Copy link
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
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 <[email protected]>
@Kavindu-Dodan
Copy link
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 :)

@toddbaert
Copy link
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
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