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.

Sign up for GitHub

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

Jump to bottom

veneur-proxy doesn't work on Kubernetes via GRPC #762

Open
rafaelgaspar opened this issue Nov 11, 2019 · 2 comments
Open

veneur-proxy doesn't work on Kubernetes via GRPC #762

rafaelgaspar opened this issue Nov 11, 2019 · 2 comments

Comments

@rafaelgaspar
Copy link

rafaelgaspar commented Nov 11, 2019 •
edited
Loading

I've been trying to use veneur-proxy on our setup that only has one veneur-global and a bunch of veneur-local's running as sidecars on each pod that needs to send metrics to Datadog.

The setup with just one veneur-global works fine, but after fiddling around with the proxy, the farthest I've got was:

time="2019-11-11T15:29:14Z" level=debug msg="Found TCP port" port=8128
time="2019-11-11T15:29:14Z" level=debug msg="Got destinations" destinations="[http://172.21.153.68:8128]" service=veneur-global
time="2019-11-11T15:29:41Z" level=error msg="Proxying failed" duration=2.684632ms error="failed to forward to the host 'http://172.21.153.68:8128' (cause=forward, metrics=1): failed to send 1 metrics over gRPC: rpc error: code = Unavailable desc = all SubConns are in TransientFailure, latest connection error: connection error: desc = \"transport: Error while dialing dial tcp: address http://172.21.153.68:8128: too many colons in address\"" protocol=grpc
time="2019-11-11T15:29:44Z" level=debug msg="About to refresh destinations" acceptingForwards=false consulForwardGRPCService=veneur-global consulForwardService= consulTraceService=

That error is similar to when the forward GRPC address is added with the http:// prefix and looking at the code it seams that the "http://" is hardcoded added by the KubernetesDiscoverer while it's not added by Consul.

What is the reasoning for that? I don't want to fallback to sending this over HTTP just because the discoverer can't handle this.

Thanks for your time.
singron added a commit to clockwisehq/veneur that referenced this issue May 27, 2020
@singron
Don't add http:// prefix for kubernetes services
978b4da 
The host:port will become a http url if necessary in doPost in proxy.go.
However, adding the http:// here prevents GRPC from working.

Fixes stripe#762
@bshelton229
Copy link

We ran into this a while ago and ended up doing a quick patch to unblock ourselves - roverdotcom/veneur@master...roverdotcom:v13.1.0-rover

When I started to look into doing something that could possibly be merged upstream, I realized there probably needs to be some discussion about the possibility of adding some kind of pluggable discovery pattern and allowing real kubernetes_* configuration for this particular discovery rather than detecting being in kubernetes and using the consul configuration. I then got busy for a couple years, but just ran across this today :). We'd love to help think about how to get the kubernetes discovery uplifted a bit and merged in if possible so we could get back upstream as well.

Our quick patch just looks for the service name to contain the substring grpc, and if it does, it looks for a port definition on the pod named exactly grpc and configures the returned ip list accordingly, without the leading http://. This has worked for us, but is obviously continuing the quick and dirty way of getting k8s discovery to work.

@singron
Copy link

singron commented Jul 9, 2020

FYI if you are running veneur-proxy in kubernetes with grpc, you might also run into #788

AndrooHan pushed a commit to AndrooHan/veneur that referenced this issue Mar 18, 2021
@andrewhan-square
Allow kubernetes discoverer to use gRPC destinations
471112a 
Summary
I updated the kubnetes discoverer to look for veneur global containers with a grpc port name. This will allow us to specify that we are using gRPC.
Additionally, I removed the http:// prefix in the saved podIp because it hits a "too many colons in address" error with gRPC. This will still work with forwarding with http because in proxy.go, the doPost method will check and append it if its missing.
This PR is related to issue stripe#762 as it removes the http hard coding.

Motivation
In Kubernetes we want to proxy metrics to veneur global with gRPC. This edit in our fork fixed the issues we were hitting.

Test plan
Ran this in our veneur-proxy pods that utilize this Kubernetes discoverer code with gRPC to verify everything works.

Rollout/monitoring/revert plan
This change should be backwards compatible as the doPost function for http communication prepends the necessary prefix. This only affects gRPC destinations used by proxysrv which shouldn't have been available with kubernetes.
AndrooHan pushed a commit to AndrooHan/veneur that referenced this issue Mar 18, 2021
@andrewhan-square
Allow kubernetes discoverer to use gRPC destinations
72a83df 
Summary
I updated the kubnetes discoverer to look for veneur global containers with a grpc port name. This will allow us to specify that we are using gRPC.
Additionally, I removed the http:// prefix in the saved podIp because it hits a "too many colons in address" error with gRPC. This will still work with forwarding with http because in proxy.go, the doPost method will check and append it if its missing.
This PR is related to issue stripe#762 as it removes the http hard coding.

Motivation
In Kubernetes we want to proxy metrics to veneur global with gRPC. This edit in our fork fixed the issues we were hitting.

Test plan
Ran this in our veneur-proxy pods that utilize this Kubernetes discoverer code with gRPC to verify everything works.

Rollout/monitoring/revert plan
This change should be backwards compatible as the doPost function for http communication prepends the necessary prefix. This only affects gRPC destinations used by proxysrv which shouldn't have been available with kubernetes.
AndrooHan pushed a commit to AndrooHan/veneur that referenced this issue Mar 18, 2021
@andrewhan-square
Allow kubernetes discoverer to use gRPC destinations
f016f9f 
Summary
I updated the kubnetes discoverer to look for veneur global containers with a grpc port name. This will allow us to specify that we are using gRPC.
Additionally, I removed the http:// prefix in the saved podIp because it hits a "too many colons in address" error with gRPC. This will still work with forwarding with http because in proxy.go, the doPost method will check and append it if its missing.
This PR is related to issue stripe#762 as it removes the http hard coding.

Motivation
In Kubernetes we want to proxy metrics to veneur global with gRPC. This edit in our fork fixed the issues we were hitting.

Test plan
Ran this in our veneur-proxy pods that utilize this Kubernetes discoverer code with gRPC to verify everything works.

Rollout/monitoring/revert plan
This change should be backwards compatible as the doPost function for http communication prepends the necessary prefix. This only affects gRPC destinations used by proxysrv which shouldn't have been available with kubernetes.
@AndrooHan AndrooHan mentioned this issue Mar 18, 2021
Closed
andrewhan-square added a commit to andrewhan-square/veneur that referenced this issue Apr 27, 2021
@andrewhan-square
Allow kubernetes discoverer to use gRPC destinations
7b0ae2f 
Summary
I updated the kubnetes discoverer to look for veneur global containers with a grpc port name. This will allow us to specify that we are using gRPC.
Additionally, I removed the http:// prefix in the saved podIp because it hits a "too many colons in address" error with gRPC. This will still work with forwarding with http because in proxy.go, the doPost method will check and append it if its missing.
This PR is related to issue stripe#762 as it removes the http hard coding.

Motivation
In Kubernetes we want to proxy metrics to veneur global with gRPC. This edit in our fork fixed the issues we were hitting.

Test plan
Ran this in our veneur-proxy pods that utilize this Kubernetes discoverer code with gRPC to verify everything works.

Rollout/monitoring/revert plan
This change should be backwards compatible as the doPost function for http communication prepends the necessary prefix. This only affects gRPC destinations used by proxysrv which shouldn't have been available with kubernetes.
eriwo-stripe pushed a commit that referenced this issue Jun 21, 2021
@andrewhan-square @csolidum
Allow kubernetes discoverer to use gRPC destinations (#829)
cf35312 
* Allow kubernetes discoverer to use gRPC destinations

Summary
I updated the kubnetes discoverer to look for veneur global containers with a grpc port name. This will allow us to specify that we are using gRPC.
Additionally, I removed the http:// prefix in the saved podIp because it hits a "too many colons in address" error with gRPC. This will still work with forwarding with http because in proxy.go, the doPost method will check and append it if its missing.
This PR is related to issue #762 as it removes the http hard coding.

Motivation
In Kubernetes we want to proxy metrics to veneur global with gRPC. This edit in our fork fixed the issues we were hitting.

Test plan
Ran this in our veneur-proxy pods that utilize this Kubernetes discoverer code with gRPC to verify everything works.

Rollout/monitoring/revert plan
This change should be backwards compatible as the doPost function for http communication prepends the necessary prefix. This only affects gRPC destinations used by proxysrv which shouldn't have been available with kubernetes.

* Update CHANGELOG for new release

* Pulled the logic that generates pod IPs from pod info into it's own
function to make it more testable and added some tests on generating
these ips

Co-authored-by: Chris Solidum <csolidum@squareup.com>
Co-authored-by: csolidum <chris2987@gmail.com>
singron added a commit to clockwisehq/veneur that referenced this issue Mar 29, 2022
@singron
Don't add http:// prefix for kubernetes services
32ef23d 
The host:port will become a http url if necessary in doPost in proxy.go.
However, adding the http:// here prevents GRPC from working.

Fixes stripe#762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rafaelgaspar @bshelton229 @singron