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: added custom grpc resolver #1008

Merged

Conversation

pradeepbbl
Copy link
Member

This PR

Added custom gRPC resolver to support envoy proxy

  • support gRPC custom resolver using new config options tragetUri

Related Issues

Fixes #995

How to test

Unit test are already added for the custom resolver for integration test you need a working envoy proxy support or any of the supported core resolver mentioned here

FlagdOptions.builder()
      .targetUri("envoy://localhost:9211/b-features-api.service")
      .selector("test-project")
      .resolverType(Config.Resolver.IN_PROCESS)
      .build();
DEBUG [2024-10-08 00:00:37,661] io.grpc.netty.NettyClientHandler: [id: 0xef9bc3d3, L:/127.0.0.1:35368 - R:localhost/127.0.0.1:9211] OUTBOUND HEADERS: streamId=3 headers=GrpcHttp2OutboundHeaders[:authority: b-features-api.service, :path: /flagd.sync.v1.FlagSyncService/SyncFlags, :method: POST, :scheme: http, content-type: application/grpc, te: trailers, user-agent: grpc-java-netty/1.58.0, grpc-accept-encoding: gzip, grpc-timeout: 599837m] streamDependency=0 weight=16 exclusive=false padding=0 endStream=false
DEBUG [2024-10-08 00:00:37,664] io.grpc.netty.NettyClientHandler: [id: 0xef9bc3d3, L:/127.0.0.1:35368 - R:localhost/127.0.0.1:9211] OUTBOUND HEADERS: streamId=5 headers=GrpcHttp2OutboundHeaders[:authority: b-features-api.service, :path: /flagd.sync.v1.FlagSyncService/GetMetadata, :method: POST, :scheme: http, content-type: application/grpc, te: trailers, user-agent: grpc-java-netty/1.58.0, grpc-accept-encoding: gzip, grpc-timeout: 9969998u] streamDependency=0 weight=16 exclusive=false padding=0 endStream=false
DEBUG [2024-10-08 00:00:37,666] io.grpc.netty.NettyClientHandler: [id: 0xef9bc3d3, L:/127.0.0.1:35368 - R:localhost/127.0.0.1:9211] OUTBOUND DATA: streamId=5 padding=0 endStream=true length=5 bytes=0000000000
DEBUG [2024-10-08 00:00:37,667] io.grpc.netty.NettyClientHandler: [id: 0xef9bc3d3, L:/127.0.0.1:35368 - R:localhost/127.0.0.1:9211] OUTBOUND DATA: streamId=3 padding=0 endStream=true length=15 bytes=000000000a12086665617475726573
DEBUG [2024-10-08 00:00:37,719] io.grpc.netty.NettyClientHandler: [id: 0xef9bc3d3, L:/127.0.0.1:35368 - R:localhost/127.0.0.1:9211] INBOUND HEADERS: streamId=5 headers=GrpcHttp2ResponseHeaders[:status: 200, server: envoy, date: Tue, 08 Oct 2024 00:00:37 GMT, content-type: application/grpc, content-length: 0, grpc-status: 12, grpc-message: Method flagd.sync.v1.FlagSyncService/GetMetadata is unimplemented, x-envoy-upstream-service-time: 43] padding=0 endStream=true
DEBUG [2024-10-08 00:00:37,722] io.grpc.netty.NettyClientHandler: [id: 0xef9bc3d3, L:/127.0.0.1:35368 - R:localhost/127.0.0.1:9211] INBOUND HEADERS: streamId=3 headers=GrpcHttp2ResponseHeaders[:status: 200, server: envoy, date: Tue, 08 Oct 2024 00:00:37 GMT, content-type: application/grpc, grpc-encoding: gzip, grpc-accept-encoding: gzip, x-envoy-upstream-service-time: 44] padding=0 endStream=false

@pradeepbbl pradeepbbl requested a review from a team as a code owner October 8, 2024 00:38
@pradeepbbl pradeepbbl marked this pull request as draft October 8, 2024 00:39
@pradeepbbl pradeepbbl force-pushed the pmishra/995_support_grpc_custom_scheme branch from fb0f789 to 0da27d8 Compare October 8, 2024 00:41
@pradeepbbl pradeepbbl marked this pull request as ready for review October 9, 2024 10:21
providers/flagd/README.md Outdated Show resolved Hide resolved
providers/flagd/README.md Outdated Show resolved Hide resolved
providers/flagd/README.md Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

Hey @pradeepbbl ! Thanks a lot for this. I was hoping to get to it this week, but I won't be able to. I will review next week. Sorry about that.

@pradeepbbl
Copy link
Member Author

Hey @pradeepbbl ! Thanks a lot for this. I was hoping to get to it this week, but I won't be able to. I will review next week. Sorry about that.

No worries take your time meanwhile I'm exploring way to have e2e test using local envoy container

@pradeepbbl pradeepbbl force-pushed the pmishra/995_support_grpc_custom_scheme branch from 745b423 to 8328fee Compare October 11, 2024 23:47
@pradeepbbl
Copy link
Member Author

added a new e2e test coverage for envoy custom resolver

Test Name: RunFlagdInProcessEnvoyCucumberTest

Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

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

nice, love the e2e tests, just added a suggestion regarding enums.

@toddbaert
Copy link
Member

toddbaert commented Oct 15, 2024

I only see one small issue: the way GLUE_PROPERTY_NAME name works, for example here, it resolves all subclasses under that package: so your RunFlagdInProcessEnvoyCucumberTest class is running BOTH the dev.openfeature.contrib.providers.flagd.e2e.process.envoy.FlagdInProcessSetup AND the dev.openfeature.contrib.providers.flagd.e2e.process.FlagdInProcessSetup. To fix this, just make these classes/packages siblings (and probably rename your setup class to something like FlagdInProcessEnvoySetup or something like that).

@toddbaert toddbaert self-requested a review October 15, 2024 17:24
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.

Excellent work, thanks so much. This is my only blocking change (which is small and I would do myself, but I can't edit your branch)

@toddbaert
Copy link
Member

@pradeepbbl I merged another PR that results in some (I believe minor) conflicts here, mostly with the setup of the providers for e2e tests. Just be sure to use:

.deadline(1000)
.streamDeadlineMs(0) // this makes reconnect tests more predictable

for the options in those.

I think that's the only thing to note, but if you have issues with the conflicts or e2e tests not working, please let me know and I will do what I can to help.

added custom gRPC resolver to support envoy proxy

Signed-off-by: Pradeep Mishra <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
pradeepbbl and others added 6 commits October 15, 2024 23:40
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Pradeep Mishra <pradeepbbl@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Pradeep Mishra <pradeepbbl@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Pradeep Mishra <pradeepbbl@users.noreply.github.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
@pradeepbbl pradeepbbl force-pushed the pmishra/995_support_grpc_custom_scheme branch from 9405359 to 8d7cee6 Compare October 15, 2024 21:42
Signed-off-by: Pradeep <pradeepbbl@gmail.com>
@pradeepbbl
Copy link
Member Author

I only see one small issue: the way GLUE_PROPERTY_NAME name works, for example here, it resolves all subclasses under that package: so your RunFlagdInProcessEnvoyCucumberTest class is running BOTH the dev.openfeature.contrib.providers.flagd.e2e.process.envoy.FlagdInProcessSetup AND the dev.openfeature.contrib.providers.flagd.e2e.process.FlagdInProcessSetup. To fix this, just make these classes/packages siblings (and probably rename your setup class to something like FlagdInProcessEnvoySetup or something like that).

@toddbaert thanks as suggested fixed the GLUE_PROPERTY_NAME and also the merge conflict, please have a look when you have free time.

Signed-off-by: Pradeep <pradeepbbl@gmail.com>
@pradeepbbl
Copy link
Member Author

Excellent work, thanks so much. This is my only blocking change (which is small and I would do myself, but I can't edit your branch)

yeah sorry for that as per the company policy all our open-source contribution need to be done through company github org

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.

Nothing else to say but amazing work!

I'll merge this today unless anyone objects, and then release.

@toddbaert toddbaert merged commit 85403b7 into open-feature:main Oct 16, 2024
4 checks passed
@toddbaert
Copy link
Member

#1022 (comment)

@pradeepbbl pradeepbbl deleted the pmishra/995_support_grpc_custom_scheme branch October 16, 2024 18:29
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.

[flagd] Support custom schemes (name resolvers)
6 participants