Allow kubernetes discoverer to use gRPC destinations #829
Conversation
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.
|
|
|
Hi @aditya-stripe , I was able to get our management to fill out Stripe's corporate CLA instead of the one for individual contributions but I don't think that the CLAAssistant is picking it up. Could you help verify if the corporate form is filled and remove the CLA check in the PR if everything looks good? Thanks! cc: @csolidum |
|
|
Hm pinging maybe @eriwo-stripe or @rma-stripe to help check on the CLA |
|
Hey @andrewhan-square, I'm checking on this now. |
|
I got confirmation that Square has signed a Corporate CLA and it is okay to waive the CLA check for this PR. |
|
@eriwo-stripe I'm interested in helping push this forward. Do you have any guidance on what needs to be done to fix the failing circleci test_tip check and get a reviewer on this PR? |
|
@csolidum @andrewhan-square: The test_tip failure was just fixed the other day in this PR: #833. I believe rebasing on master and then rerunning the tests should fix it. Once that is done I can approve this. |
CHANGELOG.md
Outdated
| ## Bugfixes | ||
| * A fix for forwarding metrics with gRPC using the kubernetes discoverer. Thanks, [androohan](https://github.com/androohan)! | ||
|
|
There was a problem hiding this comment.
Can you create a new 14.2.0 section at the top of the readme and move this there?
There was a problem hiding this comment.
Updated, wasn't sure to put for the date so put unreleased
kubernetes.go
Outdated
| @@ -83,8 +89,7 @@ func (kd *KubernetesDiscoverer) GetDestinationsForService(serviceName string) ([ | |||
| continue | |||
| } | |||
|
|
|||
| // prepend with // so that it is a valid URL parseable by url.Parse | |||
| podIp := fmt.Sprintf("http://%s:%s", pod.Status.PodIP, forwardPort) | |||
| podIp := fmt.Sprintf("%s:%s", pod.Status.PodIP, forwardPort) | |||
There was a problem hiding this comment.
Can you also add a test to verify the assertion that "This will still work with forwarding with http because in proxy.go, the doPost method will check and append it if its missing."?
There was a problem hiding this comment.
added back the "http://" prefix for tcp and http ports. Pulled out some of this logic into another function and added some tests around that.
function to make it more testable and added some tests on generating these ips
I think I should be on the same CLA agreement as @andrewhan-square |
|
@eriwo-stripe let me know if there are any more changes needed before this PR is merged. |
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.
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.