Add support for stateful sets by adleong · Pull Request #3113 · linkerd/linkerd2

adleong · 2019-07-20T00:41:49Z

We add support for looking up individual pods in a stateful set with the destination service. This allows Linkerd to correctly proxy requests which address individual pods. The authority structure for such a request is <pod-name>.<service>.<namespace>.svc.cluster.local:<port>.

Fixes #2266

Signed-off-by: Alex Leong [email protected]

Signed-off-by: Alex Leong <[email protected]>

l5d-bot · 2019-07-20T01:08:07Z

Integration test results for f1b851c: success 🎉
Log output: https://gist.github.com/8432df311fbe66597373ed683bea462a

Relates to linkerd/linkerd2#3113 Signed-off-by: Andrew Seigner <[email protected]>

siggy

Confirmed working with:
BuoyantIO/emojivoto#68

From linkerd-prometheus pod:

wget -O - web-0.web-svc.emojivoto.svc.cluster.local:80

nit: rather than hostname, can we name this podName, or podHostname? when i see hostname in the code it's not obvious to me that's it's referencing a specific pod. (i realize that the k8s core field is named EndpointAddress.Hostname).

siggy · 2019-07-22T21:08:40Z

controller/api/destination/watcher/k8s.go

 	suffix := []string{"svc", "cluster", "local"}
-	for i, subdomain := range domains[2:] {
+	n := len(domains)
+	if n < 5 {


should this also check for n > 6 ? that way we could ditch the the last return statement.

Yes, but I think we'd need to make the n == 6 case into the else block. I think the current way is clearer.

siggy · 2019-07-22T21:11:46Z

controller/api/destination/watcher/endpoints_watcher_test.go

+  podIP: 172.17.0.20`,
+			},
+			authority:                        "name1-3.name1.ns.svc.cluster.local:5959",
+			expectedAddresses:                []string{"172.17.0.20:5959"},


siggy · 2019-07-22T21:33:03Z

controller/api/destination/watcher/endpoints_watcher.go

 	for _, subset := range endpoints.Subsets {
 		resolvedPort := pp.resolveTargetPort(subset)
 		for _, endpoint := range subset.Addresses {
+			if pp.hostname != "" && pp.hostname != endpoint.Hostname {


why the pp.hostname != "" check? if pp.hostname == "" and endpoint.Hostname == "foo" this check will fail, is that the intent?

If the portPublisher's hostname is unset (empty string) that means that no hostname was specified. In other words, this is a regular service lookup and not a stable pod id lookup. Therefore, we do not want to skip any of the endpoints.

So does that mean pp.hostname == "" && endpoint.Hostname == "foo" will never happen? And if it does, then there is a bug elsewhere? Or can we just do if pp.hostname != endpoint.Hostname here?

pp.hostname corresponds to the subscriber's query whereas endpoint.Hostname corresponds to the hostname of the pod. So, for example, if we did a destination lookup for foo.default.svc.cluster.local where foo is a service which selects a stateful set, then pp.hostname would be empty (because we are querying the entire service, not a particular stable id) but each pod in the service would have a non-empty endpoint.Hostname.

In this case, we want to select all the pods, and therefore do not want to hit the continue.

Relates to linkerd/linkerd2#3113 Signed-off-by: Andrew Seigner <[email protected]>

adleong · 2019-07-23T00:07:10Z

They're referred to as hostnames in the Kubernetes documentation as well: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#stable-network-id

I agree that it's not the best name, but I'd prefer to match the kubernetes terminology.

ihcsim

👍 Can confirm that this works with the example in #2266.

Also, the new metrics label shows up correctly:

# HELP endpoints_exists A gauge which is 1 if the endpoints exists and 0 if it does not.
# TYPE endpoints_exists gauge
endpoints_exists{hostname="",namespace="default",port="80",service="my-external-nginx"} 0
endpoints_exists{hostname="",namespace="linkerd",port="9090",service="linkerd-prometheus"} 1
endpoints_exists{hostname="nginx-0",namespace="default",port="80",service="nginx"} 1
endpoints_exists{hostname="nginx-1",namespace="default",port="80",service="nginx"} 1
endpoints_exists{hostname="nginx-2",namespace="default",port="80",service="nginx"} 1
# HELP endpoints_pods A gauge for the current number of pods in a endpoints.
# TYPE endpoints_pods gauge
endpoints_pods{hostname="",namespace="default",port="80",service="my-external-nginx"} 0
endpoints_pods{hostname="",namespace="linkerd",port="9090",service="linkerd-prometheus"} 1
endpoints_pods{hostname="nginx-0",namespace="default",port="80",service="nginx"} 1
endpoints_pods{hostname="nginx-1",namespace="default",port="80",service="nginx"} 1
endpoints_pods{hostname="nginx-2",namespace="default",port="80",service="nginx"} 1
# HELP endpoints_subscribers A gauge for the current number of subscribers to a endpoints.
# TYPE endpoints_subscribers gauge
endpoints_subscribers{hostname="",namespace="default",port="80",service="my-external-nginx"} 2
endpoints_subscribers{hostname="",namespace="linkerd",port="9090",service="linkerd-prometheus"} 1
endpoints_subscribers{hostname="nginx-0",namespace="default",port="80",service="nginx"} 2
endpoints_subscribers{hostname="nginx-1",namespace="default",port="80",service="nginx"} 2
endpoints_subscribers{hostname="nginx-2",namespace="default",port="80",service="nginx"} 1
# HELP endpoints_updates A counter for number of updates to a endpoints.
# TYPE endpoints_updates counter
endpoints_updates{hostname="",namespace="default",port="80",service="my-external-nginx"} 0
endpoints_updates{hostname="",namespace="linkerd",port="9090",service="linkerd-prometheus"} 2
endpoints_updates{hostname="nginx-0",namespace="default",port="80",service="nginx"} 2
endpoints_updates{hostname="nginx-1",namespace="default",port="80",service="nginx"} 2
endpoints_updates{hostname="nginx-2",namespace="default",port="80",service="nginx"} 1

siggy

lgtm modulo ivan's comment and rebase. 👍 🚢

…n server Signed-off-by: Alex Leong <[email protected]>

adleong · 2019-07-23T21:35:42Z

This conflicted non-trivially with #2950. Since authority parsing is now more complex and depends on the cluster domain, I have pulled that logic up out of the watchers and into the destination server. This required changing the watcher interfaces to accept service and profile IDs rather than raw authorities.

@arminbuerkle: would you like to review this?

l5d-bot · 2019-07-23T21:56:04Z

Integration test results for 1ef8ed5: success 🎉
Log output: https://gist.github.com/d7446ce8610cba9a0fd02016109ccf28

arminbuerkle

Thanks @adleong for mentioning me.

I looked through the PR and only found a few minor nit-picks.

The implementations itself looks solid though 👍

arminbuerkle · 2019-07-24T11:01:18Z

controller/cmd/destination/main.go

 		log.Fatalf("Failed to listen on %s: %s", *addr, err)
 	}

+	global, err := config.Global(consts.MountPathGlobalConfig)


I'd probably move this into the destination package into the NewServer function.

We don't need to and don't want to set a different cluster domain than what's set in the config so calling it here give us no benefit.

By moving it you reduce the amount of parameters required for NewServer.

Since config.Global is needed for the trust domain too, i'd also move that into the destination package and pass *disableIdentity instead of trustDomain to NewServer

I think I prefer to keep the config reading in the main and pass the specific values into NewServer. This reduces coupling between the server implementation and the config, and makes the server easier to test.

arminbuerkle · 2019-07-24T11:15:28Z

controller/api/destination/watcher/k8s.go

+// the HTTP default (80) is returned as the port number.  If the authority
+// is a pod DNS name then the pod hostname is also returned as the 3rd return
+// value.  See https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/.
+func GetServiceAndPort(authority string, clusterDomain string) (ServiceID, Port, string, error) {


GetServiceAndPort now returns a ServiceID, Port, string and error.

It looks inconsistent that the pod hostname does not have a type while for example port does

In addition returning a hostname from a GetServiceAndPort function seems confusing. IMO it's time to rename the function. Maybe ParseAuthority? Similar to go's url.Parse?

Great suggestion, thanks.

arminbuerkle · 2019-07-24T11:32:43Z

controller/api/destination/watcher/k8s.go

-	// Needs to have `".svc."` to be a valid k8s svc
-	if domains[2] != "svc" {
-		return ServiceID{}, 0, fmt.Errorf("Invalid k8s service %s", host)
+	for i, subdomain := range domains[n-len(suffix):] {


If i'm not missing anything you could rewrite this loop into:

// svc.{clusterDomain} != 'host' suffix if !strings.HasSuffix(host, "svc."+clusterDomain) { return ServiceID{}, 0, "", fmt.Errorf("Invalid k8s service %s", host) }

which is easier to understand.

arminbuerkle · 2019-07-24T11:37:21Z

controller/api/destination/watcher/k8s.go

+			Namespace: domains[1],
+		}
+		return service, port, "", nil
+	} else if n == 3+len(suffix) {


There's no else if needed since you're returning in the previous if statement.

If you only use an if you can save on indentations and make it easier to understand.

Signed-off-by: Alex Leong <[email protected]>

l5d-bot · 2019-07-24T18:55:36Z

Integration test results for 4896cc4: success 🎉
Log output: https://gist.github.com/a843284b996dd476b4c75f5f44d2073a

Continue of #2950. I decided to check for the `clusterDomain` in the config map in web server main for the same reasons as as pointed out here #3113 (comment) It decouples the server implementations from the config. Signed-off-by: Armin Buerkle <[email protected]>

adleong added 2 commits July 19, 2019 16:34

Add support for stateful sets

f9ad12f

Signed-off-by: Alex Leong <[email protected]>

Drop hostname in profile lookups

f1b851c

Signed-off-by: Alex Leong <[email protected]>

adleong self-assigned this Jul 20, 2019

adleong mentioned this pull request Jul 20, 2019

Can't Reach StatefulSet Pods Via Stable Network ID #2266

Closed

siggy added a commit to BuoyantIO/emojivoto that referenced this pull request Jul 22, 2019

Fix serviceName fields for stateful set config

90e249d

Relates to linkerd/linkerd2#3113 Signed-off-by: Andrew Seigner <[email protected]>

siggy mentioned this pull request Jul 22, 2019

Fix serviceName fields for stateful set config BuoyantIO/emojivoto#68

Merged

siggy reviewed Jul 22, 2019

View reviewed changes

siggy added a commit to BuoyantIO/emojivoto that referenced this pull request Jul 22, 2019

Fix serviceName fields for stateful set config (#68)

44098df

Relates to linkerd/linkerd2#3113 Signed-off-by: Andrew Seigner <[email protected]>

ihcsim approved these changes Jul 23, 2019

View reviewed changes

siggy approved these changes Jul 23, 2019

View reviewed changes

Resolve merge conflicts and pull authority parsing up into destinatio…

1ef8ed5

…n server Signed-off-by: Alex Leong <[email protected]>

adleong mentioned this pull request Jul 23, 2019

Return invalid argument for external name services #3120

Merged

arminbuerkle reviewed Jul 24, 2019

View reviewed changes

adleong added 2 commits July 24, 2019 10:35

Merge branch 'master' into alex/stateful-set

bae4231

Review feedback

4896cc4

Signed-off-by: Alex Leong <[email protected]>

adleong merged commit e538a05 into master Jul 24, 2019

adleong deleted the alex/stateful-set branch July 24, 2019 21:09

arminbuerkle mentioned this pull request Jul 26, 2019

Allow setting custom cluster domain in service profiles #3148

Merged

Conversation

adleong commented Jul 20, 2019

Uh oh!

l5d-bot commented Jul 20, 2019

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adleong commented Jul 23, 2019

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

siggy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adleong commented Jul 23, 2019

Uh oh!

l5d-bot commented Jul 23, 2019

Uh oh!

arminbuerkle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Jul 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

siggy left a comment •

edited

Loading