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

Support pod ip and service cluster ip lookups in the destination service #3595

Open
wants to merge 10 commits into
base: master
from

Conversation

@adleong
Copy link
Member

adleong commented Oct 17, 2019

Fixes #3444
Fixes #3443

Background and Behavior

This change adds support for the destination service to resolve Get requests which contain a service clusterIP or pod ip as the Path parameter. It returns the stream of endpoints, just as if Get had been called with the service's authority. This lays the groundwork for allowing the proxy to TLS TCP connections by allowing the proxy to do destination lookups for the SO_ORIG_DST of tcp connections. When that ip address corresponds to a service cluster ip or pod ip, the destination service will return the endpoints stream, including the pod metadata required to establish identity.

Prior to this change, attempting to look up an ip address in the destination service would result in a InvalidArgument error.

Updating the GetProfile method to support ip address lookups is out of scope and attempts to look up an ip address with the GetProfile method will result in InvalidArgument.

Implementation

We do this by creating a IPWatcher which wraps the EndpointsWatcher and supports lookups by ip. IPWatcher maintains a mapping up clusterIPs to service ids and translates subscriptions to an IP address into a subscription to the service id using the underlying EndpointsWatcher.

Since the service name is no longer always infer-able directly from the input parameters, we restructure EndpointTranslator and PodSet so that we propagate the service name from the endpoints API response.

Testing

This can be tested by running the destination service locally, using the current kube context to connect to a Kubernetes cluster:

go run controller/cmd/main.go destination -kubeconfig ~/.kube/config

Then lookups can be issued using the destination client:

go run controller/script/destination-client/main.go -path 192.168.54.78:80 -method get -addr localhost:8086

Service cluster ips and pod ips can be used as the path argument.

for listener, port := range ss.listeners {
ss.endpoints.Unsubscribe(ss.id, port, "", listener)
ss.endpoints.Unsubscribe(ss.service, port, "", listener)
ss.endpoints.Subscribe(id, port, "", listener)

This comment has been minimized.

Copy link
@zaharidichev

zaharidichev Oct 23, 2019

Member

Maybe this questions is quite silly, but just trying to understand things a little better. Why is this error not handled here. Is it because the Svc().Informer() is guaranteed to not return services of type ExternalName? What happens if this service changes to an external type right before you hit the k8s API?

This comment has been minimized.

Copy link
@adleong

adleong Oct 23, 2019

Author Member

This is not a silly question, it's actually a great point.

We should probably handle errors here by sending a NoEndpoints(exists = false) message to the subscribers.

Copy link
Member

alpeb left a comment

Looking good 👍

I was wondering about support for pods with hostNetwork: true... Calling Get with an IP for such a pod will return nothing because the pod indexer in getOrNewServiceSubscriptions might return more than one pod.
Would it make sense to map things not only by clusterIP but also by port?

Besides that, I just found a couple of nits described below, plus the comment of the NewServer func in server.go that needs to be updated.

@alpeb

This comment has been minimized.

Copy link
Member

alpeb commented Oct 29, 2019

To clarify over my previous comment, the log shows the "Pod IP conflict" at startup, and the Get call does actually return an answer, but not necessarily corresponding to the right pod.

Note that hostNetwork: true pods are never meshed, which I guess no mTLS can be established. Not sure if that would require a special treatment.

@adleong

This comment has been minimized.

Copy link
Member Author

adleong commented Oct 29, 2019

Nice catch, @alpeb. I think the simplest solution would be to omit pods which have hostNetwork true.

@olix0r

This comment has been minimized.

Copy link
Member

olix0r commented Nov 4, 2019

I've built and pushed an image from this branch, including the required proxy changes under git-9f78d7e6

@olix0r

This comment has been minimized.

Copy link
Member

olix0r commented Nov 4, 2019

Appears to work as expected; however it seemed that the emojivoto scrapes weren't discovered properly until emojivoto was reinjected...

:; linkerd metrics -n linkerd deploy/linkerd-prometheus | grep ^response_total
response_total{direction="outbound",tls="no_identity",no_tls_reason="not_provided_by_service_discovery",classification="failure",error="unclassified"} 45
response_total{direction="outbound",tls="no_identity",no_tls_reason="not_provided_by_service_discovery",status_code="200",classification="success"} 1303
response_total{direction="outbound",tls="no_identity",no_tls_reason="not_provided_by_service_discovery",status_code="502",classification="failure"} 2
response_total{direction="inbound",tls="no_identity",no_tls_reason="not_provided_by_remote",status_code="200",classification="success"} 93
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-destination",dst_pod="linkerd-destination-67fc4995d4-5b6z8",dst_pod_template_hash="67fc4995d4",dst_serviceaccount="linkerd-destination",tls="true",server_id="linkerd-destination.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 92
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-tap",dst_pod="linkerd-tap-9d56f9d78-bdbm4",dst_pod_template_hash="9d56f9d78",dst_serviceaccount="linkerd-tap",tls="true",server_id="linkerd-tap.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 92
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-controller",dst_pod="linkerd-controller-59b98cd9f-xh6fw",dst_pod_template_hash="59b98cd9f",dst_serviceaccount="linkerd-controller",tls="true",server_id="linkerd-controller.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 138
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-sp-validator",dst_pod="linkerd-sp-validator-6d6d66f959-jlhgf",dst_pod_template_hash="6d6d66f959",dst_serviceaccount="linkerd-sp-validator",tls="true",server_id="linkerd-sp-validator.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 92
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-proxy-injector",dst_pod="linkerd-proxy-injector-665676f8f4-s6kcv",dst_pod_template_hash="665676f8f4",dst_serviceaccount="linkerd-proxy-injector",tls="true",server_id="linkerd-proxy-injector.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 46
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-destination",dst_pod="linkerd-destination-67fc4995d4-2dg4r",dst_pod_template_hash="67fc4995d4",dst_serviceaccount="linkerd-destination",tls="true",server_id="linkerd-destination.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 91
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-controller",dst_pod="linkerd-controller-59b98cd9f-h45g6",dst_pod_template_hash="59b98cd9f",dst_serviceaccount="linkerd-controller",tls="true",server_id="linkerd-controller.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 135
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-proxy-injector",dst_pod="linkerd-proxy-injector-665676f8f4-c98z8",dst_pod_template_hash="665676f8f4",dst_serviceaccount="linkerd-proxy-injector",tls="true",server_id="linkerd-proxy-injector.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 89
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-sp-validator",dst_pod="linkerd-sp-validator-6d6d66f959-c49hn",dst_pod_template_hash="6d6d66f959",dst_serviceaccount="linkerd-sp-validator",tls="true",server_id="linkerd-sp-validator.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 88
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-proxy-injector",dst_pod="linkerd-proxy-injector-665676f8f4-75ql9",dst_pod_template_hash="665676f8f4",dst_serviceaccount="linkerd-proxy-injector",tls="true",server_id="linkerd-proxy-injector.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 87
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-tap",dst_pod="linkerd-tap-9d56f9d78-hr4vl",dst_pod_template_hash="9d56f9d78",dst_serviceaccount="linkerd-tap",tls="true",server_id="linkerd-tap.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 86
response_total{authority="linkerd-prometheus.linkerd.svc.cluster.local:9090",direction="inbound",tls="true",client_id="linkerd-controller.linkerd.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 6317
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="vote-bot",dst_pod="vote-bot-55fb746c54-4kntg",dst_pod_template_hash="55fb746c54",dst_serviceaccount="default",tls="true",server_id="default.emojivoto.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 6
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="emoji",dst_pod="emoji-847d6d58d4-6j2jm",dst_pod_template_hash="847d6d58d4",dst_serviceaccount="emoji",tls="true",server_id="emoji.emojivoto.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 6
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="voting",dst_pod="voting-7c8c96b45c-b2kvq",dst_pod_template_hash="7c8c96b45c",dst_serviceaccount="voting",tls="true",server_id="voting.emojivoto.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 6
response_total{direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="web",dst_pod="web-bf8469c6b-4dvdb",dst_pod_template_hash="bf8469c6b",dst_serviceaccount="web",tls="true",server_id="web.emojivoto.serviceaccount.identity.linkerd.cluster.local",status_code="200",classification="success"} 6
@adleong

This comment has been minimized.

Copy link
Member Author

adleong commented Nov 4, 2019

Appears to work as expected; however it seemed that the emojivoto scrapes weren't discovered properly until emojivoto was reinjected...

Perhaps prometheus was reusing long lived connections? I would expect that restarting prometheus would also cause everything to get picked up properly.

@olix0r

This comment has been minimized.

Copy link
Member

olix0r commented Nov 5, 2019

olix0r added a commit to linkerd/linkerd2-proxy that referenced this pull request Nov 7, 2019
This enables discovery (and mTLS) for HTTP traffic to private-network
IP addresses, on the assumption that they are more than likely part of
the local cluster. The set of discoverable networks may be configured
via the environment.

This depends on linkerd/linkerd2#3595 returning
resolutions for addresses.
@adleong

This comment has been minimized.

Copy link
Member Author

adleong commented Nov 8, 2019

To see this in action:

  • Install or upgrade to this verison
  • kubectl -n linkerd port-forward deploy/linkerd-prometheus 9090
  • Browse metrics like request_total{deployment="linkerd-prometheus",direction="outbound"}

This will show metrics for requests from Prometheus to other pods in the cluster. Notice that these metrics have full dst metadata including pod name, etc, and have tls=true. This is true even though Prometheus addresses requests directly to pod IPs.

author Alex Leong <alex@buoyant.io> 1569519892 -0700
committer Alex Leong <alex@buoyant.io> 1573578432 -0800

checkpoint

Signed-off-by: Alex Leong <alex@buoyant.io>
@adleong adleong force-pushed the alex/pod-ip branch from eba8da6 to 6c0e538 Nov 12, 2019
adleong added 8 commits Nov 12, 2019
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Bump CI
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Bump CI
Signed-off-by: Alex Leong <alex@buoyant.io>
Copy link
Member

ihcsim left a comment

I added some comments and questions regarding the new IP watcher. I'll take look at the unit test and do more testing tomorrow.

return []string{""}, fmt.Errorf("object is not a service")
}})

k8sAPI.Svc().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{

This comment has been minimized.

Copy link
@ihcsim

ihcsim Nov 26, 2019

Member

How does this work with the AddEventHandler in the endpoint_watcher.go? Will one override the other? Or is this a separate Informer instance?

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

it's the same imformer with multiple handler funcs registered.

AddFunc: iw.addPod,
DeleteFunc: iw.deletePod,
UpdateFunc: func(_ interface{}, obj interface{}) {
iw.addPod(obj)

This comment has been minimized.

Copy link
@ihcsim

ihcsim Nov 26, 2019

Member

Do we need to perform a iw.deletePod(obj) before the iw.addPod(obj)?

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

since pod IP addresses are immutable once assigned, I don't think so.

}
}
}
objs, err = iw.k8sAPI.Pod().Informer().GetIndexer().ByIndex(podIPIndex, clusterIP)

This comment has been minimized.

Copy link
@ihcsim

ihcsim Nov 26, 2019

Member

Do we need to add the corresponding k8sAPI.Pod().Informer().AddIndexers() to the NewAPIWatcher() constructor, like what you did for Svc?

Also, how come the pod informer knows about clusterIP?

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

the endpoints watcher has already added the pod ip indexer so we don't need to do it.

I'm using the name clusterIP a bit more broadly to mean the ip address of a resource in the cluster. This can be the ip address of a pod, or the cluster ip of a service.


// If the service doesn't yet exist, create a stub for it so the listener can
// be registered.
ss, ok := iw.publishers[clusterIP]

This comment has been minimized.

Copy link
@ihcsim

ihcsim Nov 26, 2019

Member

IIUC, the writer lock is only needed when if !ok, right? Is this the same as:

ss, ok := iw.getServiceSubscription()
if ! ok {
  iw.Lock()
  defer iw.Unlock()
  ss = &serviceSubscriptions{
    clusterIP: clusterIP,
    listeners: make(map[EndpointUpdateListener]Port),
    endpoints: iw.endpoints,
    log:       iw.log.WithField("clusterIP", clusterIP),
  }
...
}
return ss

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

I'm not sure, but we might need a read lock even in the ok case. The way we have it is slightly more conservative, but as far as I can tell it's idiomatic and makes things easier to reason about.

}
ss := iw.getOrNewServiceSubscriptions(pod.Status.PodIP)

ownerKind, ownerName := iw.k8sAPI.GetOwnerKindAndName(pod, true)

This comment has been minimized.

Copy link
@ihcsim

ihcsim Nov 26, 2019

Member

Won't ss.pod return the same thing? This seems like a repetition of what iw.getOrNewServiceSubscriptions() did to initialize the PodSet in the serviceSubscription, with the exception that iw.getOrNewServiceSubscriptions() also checks for pod.spec.HostNetwork.

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

good call

expectedNoEndpointsServiceExists bool
expectedError bool
}{
{

This comment has been minimized.

Copy link
@ihcsim

ihcsim Nov 26, 2019

Member

These use test cases seem identical(-ish) to those defined in endpoints_watcher_test.go. Any reasons why we need to re-run them here? Do the behaviour of the endpoints_watcher changes after it's being wrapped?

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

the way the lookups works is different, with lookups by ip instead of by service name. I think it's worth testing these conditions.

@ihcsim

This comment has been minimized.

Copy link
Member

ihcsim commented Nov 26, 2019

This works great! Previously, if I used an injected curl pod to curl a pod IP, tap will display the requests with TLS not provided. With this change, requests with IP authority is now TLS'ed 👍.

⚡  linkerd tap po                        
req id=2:0 proxy=out src=10.244.0.17:57658 dst=10.244.0.16:80 tls=true :method=GET :authority=10.244.0.16 :path=/api/list
rsp id=2:0 proxy=out src=10.244.0.17:57658 dst=10.244.0.16:80 tls=true :status=200 latency=7609µs
end id=2:0 proxy=out src=10.244.0.17:57658 dst=10.244.0.16:80 tls=true duration=374µs response-length=4513B
...
req id=2:5 proxy=out src=10.244.0.17:57972 dst=10.244.0.16:80 tls=true :method=GET :authority=web-sts-svc.emojivoto :path=/api/list
rsp id=2:5 proxy=out src=10.244.0.17:57972 dst=10.244.0.16:80 tls=true :status=200 latency=3653µs
end id=2:5 proxy=out src=10.244.0.17:57972 dst=10.244.0.16:80 tls=true duration=64µs response-length=4513B

Also, can confirm using Wireshark to show that the traffic is truly encrypted.
Screenshot_20191126_114446

ss = &serviceSubscriptions{
clusterIP: clusterIP,
listeners: make(map[EndpointUpdateListener]Port),
endpoints: iw.endpoints,
log: iw.log.WithField("clusterIP", clusterIP),
}
Comment on lines +183 to +188

This comment has been minimized.

Copy link
@alpeb

alpeb Dec 5, 2019

Member

It seems there's still an issue with pods with hostNetwork: true. If clusterIP corresponds to one or more than one such pods, even though they'll be ignored below, ss still gets set and will get returned at the end of this method.

This comment has been minimized.

Copy link
@adleong

adleong Dec 7, 2019

Author Member

hmmm, what do you think the correct behavior should be here?

getOrNewServiceSubscriptions should always return a serviceSubscriptions even if the ip address doesn't correspond to any pods or services in the cluster. (or it only corresponds to hostNetwork pods). In either of these cases, the returned serviceSubscriptions should have ss.pod = PodSet{}.

This causes updatePod to send a listener.Add with a size 0 pod set. Perhaps we simply need a check here to skip the add if the size of the pod set is 0.

Is there another issue that I'm missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.