Add support for profile lookups by IP address #4727

kleimkuhler · 2020-07-07T16:31:01Z

Motivation

This adds the ability to get profiles for services by IP address.

Change in behavior

When the destination server receives a GetProfile request with an IP address,
it now tries to map that IP address to a service.

If the IP address maps to an existing service, then the destination server
returns the profile stream subscribes for updates to the service--this is the
existing behavior. If the IP changes to a new service, the stream will still
send updates for the first service the IP address corresponded to since that is
what it is subscribed to.

If the IP address does not map to an existing service, then the destination
server returns the profile stream but does not subscribe for updates. The stream
will receive one update, the default profile.

Solution

This change uses the IPWatcher within the destination server to check for what
services an IP address correspond to. By adding a new method GetSvc to
IPWatcher, the server now calls this method if GetProfile receives a request
with an IP address.

Testing

Install linkerd on a cluster and get the cluster IP of any service:

❯ kubectl get -n linkerd svc/linkerd-tap -o wide
NAME          TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)            AGE   SELECTOR
linkerd-tap   ClusterIP   10.104.57.90   <none>        8088/TCP,443/TCP   16h   linkerd.io/control-plane-component=tap

Run the destination server:

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

Get the profile for the tap service by IP address:

❯ go run controller/script/destination-client/main.go -method getProfile -path 10.104.57.90:8088
INFO[0000] retry_budget:{retry_ratio:0.2  min_retries_per_second:10  ttl:{seconds:10}} 
INFO[0000]

Get the profile for an IP address that does not correspond to a service:

❯ go run controller/script/destination-client/main.go -method getProfile -path 10.256.0.1:8088
INFO[0000] retry_budget:{retry_ratio:0.2  min_retries_per_second:10  ttl:{seconds:10}} 
INFO[0000]

You can add and remove settings for the service profile for tap and get updates.

Signed-off-by: Kevin Leimkuhler kevin@kleimkuhler.com

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

adleong

Looks good. A few questions.

controller/api/destination/server.go

controller/api/destination/watcher/ip_watcher.go

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

zaharidichev

Looks good. Also tested it with the dst client.

zaharidichev · 2020-07-08T10:40:47Z

controller/api/destination/server.go

+					log.Warnf("Failed to subscribe to profile %s: %s", path, err)
+					return err
+				}
+				defer s.profiles.Unsubscribe(nil, primary)


I think the error that Unsubsribe can sometimes return will be muted when calling it via defer. Maybe better to leave the function without a return value and just log on error.

Yes you are correct here that errors that occur in Unsubscribe will fail silently.

I tested this by inserting return fmt.Errorf("this is an error") in Unsubscribe, and then subscribed to and quit a profile stream.

I expected to see this error in the server, but nothing occurred.

This occurs in the other places that we unsubscribe in this file, so I lean towards making this change separately.

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

…le-ip Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

adleong

Before merging, I'd like to get confirmation from @olix0r that this is desired behavior when looking up a ip that doesn't correspond to a service (sending the default profile and then leaving the stream open).

controller/api/destination/watcher/ip_watcher.go

controller/api/destination/watcher/profile_watcher.go

Remove additional changes from a previous simplification Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

controller/api/destination/server.go

olix0r · 2020-07-09T16:04:02Z

controller/api/destination/server.go

-	if err != nil {
-		log.Debugf("Invalid service %s", dest.GetPath())
-		return status.Errorf(codes.InvalidArgument, "invalid service: %s", err)
+			return nil


So, Thinking a bit more. I do think we're going to have to watch for updates. Just because a service isn't in our index doesn't mean it's not about to show up there. That doesn't have to be a hard blocker -- we can merge this and be incremental. But I think we should support the service being known by the app before it is known by the controller...

This is also jogging some ideas for things we may want to do in a followup re: supporting returning all profiles for a pod:ip target.

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

adleong

Cool, I think we can ship this as-is with a TODO or a follow up issue to make the mapping from ip to service dynamic.

kleimkuhler · 2020-07-10T16:28:39Z

Cool, I think we can ship this as-is with a TODO or a follow up issue to make the mapping from ip to service dynamic.

#4743 is the follow-up issue for this

## Motivation Closes linkerd#3916 This adds the ability to get profiles for services by IP address. ### Change in behavior When the destination server receives a `GetProfile` request with an IP address, it now tries to map that IP address to a service. If the IP address maps to an existing service, then the destination server returns the profile stream subscribes for updates to the _service_--this is the existing behavior. If the IP changes to a new service, the stream will still send updates for the first service the IP address corresponded to since that is what it is subscribed to. If the IP address does not map to an existing service, then the destination server returns the profile stream but does not subscribe for updates. The stream will receive one update, the default profile. ### Solution This change uses the `IPWatcher` within the destination server to check for what services an IP address correspond to. By adding a new method `GetSvc` to `IPWatcher`, the server now calls this method if `GetProfile` receives a request with an IP address. ## Testing Install linkerd on a cluster and get the cluster IP of any service: ```bash ❯ kubectl get -n linkerd svc/linkerd-tap -o wide NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE SELECTOR linkerd-tap ClusterIP 10.104.57.90 <none> 8088/TCP,443/TCP 16h linkerd.io/control-plane-component=tap ``` Run the destination server: ```bash ❯ go run controller/cmd/main.go destination -kubeconfig ~/.kube/config ``` Get the profile for the tap service by IP address: ```bash ❯ go run controller/script/destination-client/main.go -method getProfile -path 10.104.57.90:8088 INFO[0000] retry_budget:{retry_ratio:0.2 min_retries_per_second:10 ttl:{seconds:10}} INFO[0000] ``` Get the profile for an IP address that does not correspond to a service: ```bash ❯ go run controller/script/destination-client/main.go -method getProfile -path 10.256.0.1:8088 INFO[0000] retry_budget:{retry_ratio:0.2 min_retries_per_second:10 ttl:{seconds:10}} INFO[0000] ``` You can add and remove settings for the service profile for tap and get updates. Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com> Signed-off-by: Eric Solomon <errcsool@engineer.com>

The proxy performs endpoint discovery for unnamed services, but not service profiles. The destination controller and proxy have been updated to support lookups for unnamed services in #4727 and linkerd/linkerd2-proxy#626, respectively. This change modifies the injection template so that the `proxy.destinationGetNetworks` configuration enables profile discovery for all networks on which endpoint discovery is permitted.

…#4960) The proxy performs endpoint discovery for unnamed services, but not service profiles. The destination controller and proxy have been updated to support lookups for unnamed services in #4727 and linkerd/linkerd2-proxy#626, respectively. This change modifies the injection template so that the `proxy.destinationGetNetworks` configuration enables profile discovery for all networks on which endpoint discovery is permitted.

kleimkuhler added 5 commits July 7, 2020 00:36

Add ip lookup to GetProfile

7d4ec1f

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

wip test

c9ca449

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Use ip_watcher for getprofile

1489a99

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Fix test

4b8a8e0

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Remove print

d88a5bd

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

kleimkuhler requested a review from adleong July 7, 2020 16:31

kleimkuhler requested a review from a team as a code owner July 7, 2020 16:31

kleimkuhler changed the title ~~Kleimkuhler/get profile ip~~ Add support for profile lookups by IP address Jul 7, 2020

adleong reviewed Jul 7, 2020

View reviewed changes

controller/api/destination/server.go Outdated Show resolved Hide resolved

controller/api/destination/server.go Outdated Show resolved Hide resolved

controller/api/destination/watcher/ip_watcher.go Outdated Show resolved Hide resolved

kleimkuhler added 2 commits July 7, 2020 23:49

Add review feedback

ea239c2

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Return stream when profile does not exist for IP address

28d64fc

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

zaharidichev approved these changes Jul 8, 2020

View reviewed changes

kleimkuhler added 3 commits July 8, 2020 21:06

Simplify the no service case

95f76ba

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Merge remote-tracking branch 'origin/main' into kleimkuhler/get-profi…

39958df

…le-ip Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

Remove comment

b040ea6

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

adleong reviewed Jul 9, 2020

View reviewed changes

controller/api/destination/watcher/ip_watcher.go Outdated Show resolved Hide resolved

controller/api/destination/watcher/profile_watcher.go Outdated Show resolved Hide resolved

Add review feedback

cff106a

Remove additional changes from a previous simplification Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

olix0r self-requested a review July 9, 2020 15:44

olix0r reviewed Jul 9, 2020

View reviewed changes

Add comments

9a6aee2

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>

adleong approved these changes Jul 9, 2020

View reviewed changes

kleimkuhler merged commit f49b40c into main Jul 10, 2020

kleimkuhler deleted the kleimkuhler/get-profile-ip branch July 10, 2020 21:41

olix0r mentioned this pull request Sep 10, 2020

inject: Configure the proxy to discover profiles for unnamed services #4960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for profile lookups by IP address #4727

Add support for profile lookups by IP address #4727

kleimkuhler commented Jul 7, 2020 •

edited

Loading

adleong left a comment

zaharidichev left a comment

zaharidichev Jul 8, 2020

kleimkuhler Jul 8, 2020

adleong left a comment

olix0r Jul 9, 2020

adleong left a comment

kleimkuhler commented Jul 10, 2020

Add support for profile lookups by IP address #4727

Add support for profile lookups by IP address #4727

Conversation

kleimkuhler commented Jul 7, 2020 • edited Loading

Motivation

Change in behavior

Solution

Testing

adleong left a comment

Choose a reason for hiding this comment

zaharidichev left a comment

Choose a reason for hiding this comment

zaharidichev Jul 8, 2020

Choose a reason for hiding this comment

kleimkuhler Jul 8, 2020

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

olix0r Jul 9, 2020

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

kleimkuhler commented Jul 10, 2020

kleimkuhler commented Jul 7, 2020 •

edited

Loading