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: support proxy routing via gRPC default_authority #1202

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

raflFaisal
Copy link
Contributor

@raflFaisal raflFaisal commented Feb 4, 2025

This PR

To provide a :authority header to request route to target backend services using default_authority parameter by the JS clients. This enable better integration with service proxies like Envoy, facilitating more flexible and robust gRPC client configurations.

Related Issues

#1187
open-feature/flagd#1532

How to test

Use defaultAuthority parameter to pass down the target authority when sending a grpc request from client side.

@raflFaisal raflFaisal requested a review from a team as a code owner February 4, 2025 00:13
@raflFaisal raflFaisal force-pushed the support-custom-name-resolver branch from f27db24 to 0497bd8 Compare February 4, 2025 00:16
@raflFaisal raflFaisal force-pushed the support-custom-name-resolver branch from 0497bd8 to cfc030f Compare February 4, 2025 00:20
libs/providers/flagd/README.md Outdated Show resolved Hide resolved
libs/providers/flagd/README.md Show resolved Hide resolved
libs/providers/flagd/src/lib/configuration.ts Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

I'm OK with this solution, but it's distinctly different from the solution in other providers as you noted here:

Unfortunately, for Node.js, such support for Custom Name Resolution is not available.

While I'm OK with this solution, the title of this PR (which is what will show up in the release notes) doesn't seem accurate. It can address a subset of the same problems of a custom name resolver, but unless I'm very confused, it's not a custom name resolver.

@toddbaert toddbaert requested a review from aepfli February 6, 2025 19:00
@raflFaisal raflFaisal changed the title feat: support custom name resolver feat: support proxy routing via gRPC default_authority Feb 6, 2025
@raflFaisal
Copy link
Contributor Author

@toddbaert You are correct, let me change the PR title that matches with the code change.

"feat: support proxy routing via gRPC default_authority"

Let me know if it looks good to you

@toddbaert
Copy link
Member

toddbaert commented Feb 6, 2025

@toddbaert You are correct, let me change the PR title that matches with the code change.

"feat: support proxy routing via gRPC default_authority"

Let me know if it looks good to you

Yep, that sounds good to me. Just make sure to update any readme or inline docs as well to say the same sort of thing.

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.

Approved with one minor nit here: https://github.com/open-feature/js-sdk-contrib/pull/1202/files#r1945277742

Looks really good overall. Thanks for the good README and explanations!

@beeme1mr
Copy link
Member

beeme1mr commented Feb 6, 2025

I'll merge and release tomorrow. Please consider Todd's comment but I don't think it's a blocker.

@beeme1mr beeme1mr merged commit 4e0db2a into open-feature:main Feb 7, 2025
7 checks passed
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