The linkerd proxy does not work with headless services#3470
The linkerd proxy does not work with headless services#3470ihcsim merged 2 commits intolinkerd:masterfrom JohannesEH:fix-3308
Conversation
|
I have to admit. I'm pretty unsure if this change can have unforeseen consequences. Specifically regarding the test case I altered. Is it a normal thing that endpoints keep ip addresses around if a pod goes down? What will the linkerd proxy do in this case? Secondly it seems "dirty" to return a podset from the function if no pods exists. Maybe the code should use another abstraction. Also if I need to write more tests please let me know. I have had very limited time to setup the project, code and test. So I accept if this change needs to be scrapped after review due to my lack of understanding the codebase. |
|
Thanks @JohannesEH. We really appreciate you taking the time to submit a fix and for providing helpful context about the change! |
adleong
left a comment
adleong
left a comment
There was a problem hiding this comment.
Thanks @JohannesEH! This change looks like the correct behavior to me. We trust the Kubernetes endpoints API to be the authoritative source of truth on what endpoints exist. So whenever an endpoint (be it a pod, or otherwise) is deleted, the Linkerd proxy will remove that IP from its load balancer.
I'll do a bit of manual testing to be sure, but this all looks good to me.
|
I'm getting this stack trace in the destination controller when I try this: |
|
I've tried getting a linkerd test environment up and running with minikube on my windows machine through wsl, but had no luck getting it to work. So I'm not sure if I can take this change any further as I would essentially be coding "in the dark". |
|
Looks like the panic is caused by an It seems to me that we shouldn't be calling the @JohannesEH The following diff works for me, when tested with a headless service. Can you verify if it works for you? diff --git a/controller/api/destination/endpoint_translator.go b/controller/api/destination/endpoint_translator.go
index 9ed2087e..5e337163 100644
--- a/controller/api/destination/endpoint_translator.go
+++ b/controller/api/destination/endpoint_translator.go
@@ -47,7 +47,23 @@ func newEndpointTranslator(
func (et *endpointTranslator) Add(set watcher.PodSet) {
addrs := []*pb.WeightedAddr{}
for _, address := range set {
- wa, err := et.toWeightedAddr(address)
+ var (
+ wa *pb.WeightedAddr
+ err error
+ )
+ if address.Pod != nil {
+ wa, err = et.toWeightedAddr(address)
+ } else {
+ // handling address with no associated pod, such as those
+ // belonging to headless services
+ var addr *net.TcpAddress
+ addr, err = et.toAddr(address)
+ wa = &pb.WeightedAddr{
+ Addr: addr,
+ Weight: defaultWeight,
+ }
+ }
+
if err != nil {
et.log.Errorf("Failed to translate endpoints to weighted addr: %s", err)
continue
diff --git a/controller/api/destination/watcher/endpoints_watcher.go b/controller/api/destination/watcher/endpoints_watcher.go
index c3095fd1..c145fd12 100644
--- a/controller/api/destination/watcher/endpoints_watcher.go
+++ b/controller/api/destination/watcher/endpoints_watcher.go
@@ -421,7 +421,7 @@ func (pp *portPublisher) endpointsToAddresses(endpoints *corev1.Endpoints) PodSe
continue
}
if endpoint.TargetRef == nil {
- id := PodID{
+ id := ServiceID{ // I think using ServiceID here makes it clearer that we aren't dealing with pods
Name: strings.Join([]string{
endpoints.ObjectMeta.Name,
endpoint.IP,YAML I used for testing: Using a |
…s not referencing a pod). Changed endpoints_watcher to also return endpoints with no targetref. Fixes #3308 Signed-off-by: Johannes Hansen <johannesh1980@gmail.com>
|
@ihcsim thanks! I finally got the test environment working on my windows machine. Basically I had some problems because I fetched the source on windows leading to some bash scripts having CRLF line endings instead of LF (Maybe I should add a note contributing.md to clarify the gotchas on windows). Also, the hyper-v minikube setup script is running kubernetes version 1.8.0 and should be updated to 1.12+. Finally the mkube script is looking for minikube in "Program Files (x86)" instead of "Program Files" which was the minikube install folder on my machine. With all that out of the way I was able to build and run the proxy. I applied your diff and ran some tests. It works in the simple case with port & targetPort being the same, but not with port remapping (i.e. port != targetPort). I'm not sure how (or if) this should be handled by the linkerd proxy? Maybe it's handled by kube_proxy? |
|
Sorry, got it to work now. With and without the linkerd proxy. I'll merge the changes. |
Signed-off-by: Johannes Hansen <johannesh1980@gmail.com>
|
@JohannesEH Have you tried Docker Desktop for Windows? I don't have much experience developing on Windows, but wonder if you will have better luck with Docker Desktop. |
|
@ihcsim yup I'm using that. However all bash build scripts needs something like wsl to execute. But it wasn't that bad once I got I working, it just took some time. But honestly my plan is to redo my work machine and setup dual boot with windows and some Linux distro. I would much rather do k8s/docker/go development on Linux. |
|
@JohannesEH Yeah, this PR will go into 2.7. btw, I haven't had a chance to look into the port != targetPort case you mentioned. Have you had a chance to investigate further. |
|
@ihcsim Cool. :) Regarding the error I reported with port remapping I wasn't able to repro the problem in my following tests. So I wrote of the initial test result as a fluke. |
adleong
left a comment
adleong
left a comment
There was a problem hiding this comment.
⭐ 🚫 💇♂
This looks great to me! Thanks!
ihcsim
left a comment
ihcsim
left a comment
There was a problem hiding this comment.
@JohannesEH LGTM! Thanks again for the PR 👍!
If you haven't done so, please fill up this form so that we can send you some awesome Linkerd swags.
4 participants
The linkerd proxy does not work with headless services (i.e. endpoints not referencing a pod).
Changed endpoints_watcher to also return endpoints with no targetref. Changed endpoint_translator to handle addresses with no associated pod.
Fixes #3308
Ran tests in minikube verifying that the proxy handles headless services correctly both in cases with and without port-remapping.
Signed-of-by: Johannes Hansen johannesh1980@gmail.com