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 support for unix socket path and secure connection #8

Merged
merged 9 commits into from
Jan 23, 2023

Conversation

josecolella
Copy link
Collaborator

@josecolella josecolella commented Jan 6, 2023

⚠️ Best reviewed commit-by-commit

This PR

  • Add support for connection via unix socket path
  • Add support for connection via secure connection

Related Issues

Resolves #5
Resolves #4

Notes

  • Documentation for Channel Credentials
  • Looking at the test, it seems that the secure connection can be constructed via with server roots certs only

Follow-up Tasks

How to test

@beeme1mr @toddbaert How would I test unix socket path locally. I'm using

docker run -p 8013:8013 ghcr.io/open-feature/flagd-testbed:latest

for flagd testing

references #5
references #4

Signed-off-by: Jose Colella <jose.colella@gusto.com>
Signed-off-by: Jose Colella <jose.colella@gusto.com>
Signed-off-by: Jose Colella <jose.colella@gusto.com>
- Documentation on how to connect via unix sockets and secure connection

Signed-off-by: Jose Colella <jose.colella@gusto.com>
josecolella and others added 2 commits January 9, 2023 06:43
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Jose Miguel Colella <josecolella@yahoo.com>
…vider/client.rb

Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Jose Miguel Colella <josecolella@yahoo.com>
Copy link
Collaborator

@technicalpickles technicalpickles left a comment

Choose a reason for hiding this comment

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

Since this is integrating with another service, I think it'd be extremely helpful/important to be able to validate that it is able to connect and interact with it.

You mention being able to locally with docker. I'd suggest making a docker-compose.yml to be able to bring that up. With that, you'd be able to add some RSpec hooks for making sure it's running. For example:

RSpec.configure do |config|
  next if config.dry_run

  config.when_first_matching_example_defined(type: :system) do
    # Run FlagD for integration specs
    unless config.dry_run
      # code to start flagd here
    end
  end

You'd need to either add :integration to the specs, or configure an RSpec to infer that based on a path, ie spec/integration. I'm not positive how that is done, as my initial searching seems to be tied to rspec-rails specifically.

@toddbaert
Copy link
Member

toddbaert commented Jan 9, 2023

Since this is integrating with another service, I think it'd be extremely helpful/important to be able to validate that it is able to connect and interact with it.

You mention being able to locally with docker. I'd suggest making a docker-compose.yml to be able to bring that up. With that, you'd be able to add some RSpec hooks for making sure it's running. For example:

RSpec.configure do |config|
  next if config.dry_run

  config.when_first_matching_example_defined(type: :system) do
    # Run FlagD for integration specs
    unless config.dry_run
      # code to start flagd here
    end
  end

You'd need to either add :integration to the specs, or configure an RSpec to infer that based on a path, ie spec/integration. I'm not positive how that is done, as my initial searching seems to be tied to rspec-rails specifically.

@technicalpickles One of the things we've done in the other SDK repos is implement this integration test suite, which relies on flagd (running in a container in CI), and this flagd provider to test basic SDK functionality.

Basically, it runs exactly what you propose, but to validate the SDK behavior end-to-end, including provider registration and the flag evaluation API. We could run the same integration suite here, or just a subset.

@josecolella
Copy link
Collaborator Author

@technicalpickles Was there anything else you saw for this PR that needed modification?

@toddbaert
Copy link
Member

I see some changes around the config/ENV/default config merging, and I also see a (previously) skipped test here that might be impacted.

Co-authored-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Jose Miguel Colella <josecolella@yahoo.com>
@toddbaert
Copy link
Member

As we discussed in the CNCF slack, it seems like we can make the cert path optional, since Ruby will otherwise use system certs.

josecolella and others added 2 commits January 11, 2023 22:34
…vider/client.rb

Co-authored-by: Josh Nichols <josh@technicalpickles.com>
Signed-off-by: Jose Miguel Colella <josecolella@yahoo.com>
…vider/client.rb

Co-authored-by: Josh Nichols <josh@technicalpickles.com>
Signed-off-by: Jose Miguel Colella <josecolella@yahoo.com>
Copy link
Collaborator

@technicalpickles technicalpickles left a comment

Choose a reason for hiding this comment

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

Chatted with @josecolella, and the comments I've left can be followup improvements. So approving ✅

@josecolella josecolella merged commit 88436c7 into main Jan 23, 2023
@josecolella josecolella deleted the jc-add-socket-path-support branch January 23, 2023 21:34
beeme1mr pushed a commit that referenced this pull request May 16, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.1.0](openfeature-flagd-provider-v0.0.1...openfeature-flagd-provider/v0.1.0)
(2024-05-16)


### ⚠ BREAKING CHANGES

* update flagd name and grpc schema
([#30](#30))

### ✨ New Features

* Add flagd provider
([#2](#2))
([98b695b](98b695b))
* Add support for unix socket path and secure connection
([#8](#8))
([88436c7](88436c7))
* Flagd provider uses structs from sdk
([#24](#24))
([d437e7f](d437e7f))
* integrate flagd provider with OpenFeature SDK
([#18](#18))
([80d6d02](80d6d02))
* Return default value on error
([#25](#25))
([f365c6d](f365c6d))
* update flagd name and grpc schema
([#30](#30))
([ddd438a](ddd438a))


### 🧹 Chore

* Format with standard
([#20](#20))
([bf25043](bf25043))
* Make things work
([#13](#13))
([5968037](5968037))
* update link to use new doc domain
([#12](#12))
([9baff65](9baff65))
* upgrade grpc client
([#16](#16))
([23ed78a](23ed78a))


### 🔄 Refactoring

* OpenFeature::FlagD::Provider::Configuration
([#14](#14))
([3686eb5](3686eb5))

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

Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.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.

Add secure connection support for FlagD provider Add socket support for FlagD provider
4 participants