-
Notifications
You must be signed in to change notification settings - Fork 510
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
Basic ADS client #604
Basic ADS client #604
Conversation
Hey @alecholmez, I found this xDS client sample, https://github.com/grpc/grpc-go/tree/master/examples/features/xds Client: https://github.com/grpc/grpc-go/blob/master/examples/features/xds/client/main.go#L68 |
So we can actually do something similar, I've written an envoy xDS client before let me see if I can dig that up... This code: https://github.com/greymatter-io/gm-fabric-go/blob/main/discovery/xds.go |
Thank you for your response. But the link https://github.com/greymatter-io/gm-fabric-go/blob/main/discovery/xds.go is not visible to me, it gives me 404. |
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
f366d18
to
025da84
Compare
Hey, @alecholmez, When you get a chance, could you please review this PR? |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Hi @kyessenov @sunjayBhatia @alecholmez , Could you please have a look? This is related to the envoy rate limit service configuration update feature via xDS protocol. There, rate limit service works as the xDS client and GCP as xDS server. Since the rate limit service requires an xDS client in golang we wanted to have a generic golang xDS client in GCP repo. It is really grateful if you could have a look. Thanks. |
Hi @alecholmez, Could you please have a look? Please let me know if any changes are required for the basic ADS client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall this looks good. Just some cleanup here and there and we can get this in.
One recommendation I might have would be to change the folder structure:
pkg/client/sotw/v3/client.go
I imagine we might want more of these. Maybe a delta client, xDS client, delta xDS client, etc...
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Hi @alecholmez, Thank you for your comments. Moved the client.go file as requested to |
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Great! I'm letting the build run and hopefully we can get this in shortly. |
@renuka-fernando looks like the build failed. Can you look into this? |
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@alecholmez could you please check now? |
Hi @alecholmez, Could you please run the failed build again and please review the PR? The issue with the build is fixed now: #625 |
@renuka-fernando looks like this one still failed can you check it out? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good thanks for the contributions!
Add basic ADS client
Related issue: envoyproxy/ratelimit#201
Related PR: envoyproxy/ratelimit#373
Signed-off-by: Renuka Fernando renukapiyumal@gmail.com