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

Basic ADS client #604

Merged
merged 9 commits into from
Jan 27, 2023
Merged

Basic ADS client #604

merged 9 commits into from
Jan 27, 2023

Conversation

renuka-fernando
Copy link
Contributor

@renuka-fernando renuka-fernando commented Oct 27, 2022

Add basic ADS client

Related issue: envoyproxy/ratelimit#201
Related PR: envoyproxy/ratelimit#373

Signed-off-by: Renuka Fernando renukapiyumal@gmail.com

@renuka-fernando
Copy link
Contributor Author

Hey @alecholmez,

I found this xDS client sample, https://github.com/grpc/grpc-go/tree/master/examples/features/xds
But I am not sure that we can use this, in its sample client, it is just a call to the method "SayHello". It is not to send the Discovery Request.

Client: https://github.com/grpc/grpc-go/blob/master/examples/features/xds/client/main.go#L68

@alecholmez
Copy link
Contributor

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
Is rather old and only works for V2 and hasn't been touched in years haha, but it's a similar concept.

@renuka-fernando
Copy link
Contributor Author

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>
@renuka-fernando
Copy link
Contributor Author

Hey, @alecholmez,

When you get a chance, could you please review this PR?

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

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!

@github-actions github-actions bot added the stale label Dec 1, 2022
@renuka-fernando
Copy link
Contributor Author

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.

@github-actions github-actions bot removed the stale label Dec 6, 2022
@alecholmez alecholmez self-assigned this Dec 15, 2022
@renuka-fernando
Copy link
Contributor Author

Hi @alecholmez,

Could you please have a look? Please let me know if any changes are required for the basic ADS client.

Copy link
Contributor

@alecholmez alecholmez left a 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...

pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
pkg/adsclient/sotw/v3/client.go Outdated Show resolved Hide resolved
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@renuka-fernando
Copy link
Contributor Author

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...

Hi @alecholmez,

Thank you for your comments.

Moved the client.go file as requested to pkg/client/sotw/v3/client.go.
So later we can add delta client as pkg/client/delta/v3/client.go.

Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@alecholmez
Copy link
Contributor

Great! I'm letting the build run and hopefully we can get this in shortly.

@alecholmez
Copy link
Contributor

@renuka-fernando looks like the build failed. Can you look into this?

Signed-off-by: Renuka Fernando <renukapiyumal@gmail.com>
@renuka-fernando
Copy link
Contributor Author

@alecholmez could you please check now?

@renuka-fernando
Copy link
Contributor Author

Hi @alecholmez,

Could you please run the failed build again and please review the PR? The issue with the build is fixed now: #625

@alecholmez
Copy link
Contributor

@renuka-fernando looks like this one still failed can you check it out?

Copy link
Contributor

@alecholmez alecholmez left a 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!

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.

None yet

2 participants