Rename linkerd-proxy-api to linkerd-destination by olix0r · Pull Request #2281 · linkerd/linkerd2

olix0r · 2019-02-14T00:42:21Z

Up until now, the proxy-api controller service has been the sole service
that the proxy communicates with, implementing the majoriry of the API
defined in the linkerd2-proxy-api repo. But this is about to change:
linkerd/linkerd2-proxy-api#25 introduces a new Identity service; and
this service must be served outside of the existing proxy-api service
in the linkerd-controller deployment (so that it may run under a
distinct service account).

With this change, the "proxy-api" name becomes less descriptive. It's no
longer "the service that serves the API for the proxy," it's "the
service that serves the Destination API to the proxy." Therefore, it
seems best to bite the bullet and rename this to be the "destination"
service.

olix0r · 2019-02-14T00:55:31Z

Note that this shouldn't even be a backwards incompatible change when upgrading an existing cluster: as long as the linkerd-proxy-api service isn't actively deleted (which it shouldn't be in most workflows), then extant running proxies should Just Work, because the linkerd-proxy-api and linkerd-destination service will both point at linkerd-controller pods on the same port. @adleong

klingerf

Cool, works for me, but I have a bunch of nit-picky feedback. I'm happy to submit a PR against your branch to address this, if that's easier. I've already implemented some of it in my local checkout anyway.

For all the files that you're moving from controller/api/proxy to controller/api/destination, we should update their package name to be "destination" instead of "proxy", to match the directory structure.

Our install templates still reference the ProxyAPIPort field in the installConfig struct, and the proxyAPIPort field in the proxyConfigOptions struct. For consistency I'd like to rename those to DestinationAPIPort and destinationAPIPort.

In the web UI on the service mesh page, we're still calling this container "Proxy API", here:

klingerf · 2019-02-15T00:52:00Z

cli/cmd/logs.go

 	}

-	cmd.PersistentFlags().StringVarP(&options.container, "container", "c", options.container, "Tail logs from the specified container. Options are 'public-api', 'proxy-api', 'tap', 'destination', 'prometheus', 'grafana' or 'linkerd-proxy'")
+	cmd.PersistentFlags().StringVarP(&options.container, "container", "c", options.container, "Tail logs from the specified container. Options are 'public-api', 'destination', 'tap', 'destination', 'prometheus', 'grafana' or 'linkerd-proxy'")


Not your fault, but 'destination' was already included in the list of containers here, so now it shows up twice.

klingerf · 2019-02-15T01:02:58Z

controller/api/destination/server.go


 	// this server satisfies 2 gRPC interfaces:
-	// 1) linkerd2-proxy-api/destination.Destination (proxy-facing)
+	// 1) linkerd2-destination/destination.Destination (proxy-facing)


Maybe this is an inadvertent rename? Am pretty sure "linkerd2-proxy-api" refers to the name of the repo here, which isn't changing.

klingerf · 2019-02-15T01:10:07Z

controller/api/public/grpc_server_test.go

 	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes/duration"
-	"github.com/linkerd/linkerd2/controller/api/proxy"
+	"github.com/linkerd/linkerd2/controller/api/destination"


Oof, why does this test depend on the proxy package? Looks like this dependency was introduced in #2195, when we added the linkerd endpoints command. My preference would be to move it to a separate discovery package and have both destination and public depend on that. But I'm happy to do that in a follow-up branch. In the meantime, once you change the destination package name as suggested above, you'll need to use the new package name in this file, like so:

diff --git a/controller/api/public/grpc_server_test.go b/controller/api/public/grpc_server_test.go index f30030cc..cf3c8b6c 100644 --- a/controller/api/public/grpc_server_test.go +++ b/controller/api/public/grpc_server_test.go @@ -549,7 +549,7 @@ func TestEndpoints(t *testing.T) { } k8sAPI.Sync() - discoveryClient, gRPCServer, proxyAPIConn := proxy.InitFakeDiscoveryServer(t, k8sAPI) + discoveryClient, gRPCServer, proxyAPIConn := destination.InitFakeDiscoveryServer(t, k8sAPI) defer gRPCServer.GracefulStop() defer proxyAPIConn.Close()

klingerf · 2019-02-15T01:11:32Z

controller/cmd/public-api/main.go

 	prometheusURL := flag.String("prometheus-url", "http://127.0.0.1:9090", "prometheus url")
 	metricsAddr := flag.String("metrics-addr", ":9995", "address to serve scrapable metrics on")
-	proxyAPIAddr := flag.String("proxy-api-addr", "127.0.0.1:8086", "address of proxy-api service")
+	proxyAPIAddr := flag.String("destination-addr", "127.0.0.1:8086", "address of destination service")


Can we also rename this variable to destinationAPIAddr? And then for completeness later in this file, proxyAPIConn to destinationAPIConn?

dadjeibaah

This is looking great! I left one comment but it's more of a nit than anything else.

dadjeibaah · 2019-02-14T18:50:10Z

BUILD.md

  plane.
 - [`controller`](controller)
-  - [`proxy-api`](controller/api/proxy): Accepts requests from `proxy`
+  - [`destination`](controller/api/destination): Accepts requests from `proxy`


Do we need to reword the description for this to remove the implication that this API could serve other information to the proxy? i.e. "Accepts requests from proxy instances and serves service discovery information"

Up until now, the proxy-api controller service has been the sole service that the proxy communicates with, implementing the majoriry of the API defined in the `linkerd2-proxy-api` repo. But this is about to change: linkerd/linkerd2-proxy-api#25 introduces a new Identity service; and this service must be served outside of the existing proxy-api service in the linkerd-controller deployment (so that it may run under a distinct service account). With this change, the "proxy-api" name becomes less descriptive. It's no longer "the service that serves the API for the proxy," it's "the service that serves the Destination API to the proxy." Therefore, it seems best to bite the bullet and rename this to be the "destination" service (i.e. because it only serves the `io.linkerd.proxy.destination.Destination` service).

Signed-off-by: Kevin Lingerfelt <[email protected]>

klingerf

@olix0r Ok, I pushed some commits addressing my feedback and @dadjeibaah's feedback, and I rebased to latest master. I think we freaked the DCO bot out though...

Anyway, once you've reviewed my changes, feel free to merge. Looks good to me.

olix0r · 2019-02-15T23:06:50Z

thank you, @klingerf !

dadjeibaah

Thanks for updating this @klingerf! 📦

olix0r self-assigned this Feb 14, 2019

olix0r requested review from adleong and klingerf February 14, 2019 00:42

olix0r added area/identity Automatic transport identity area/controller labels Feb 14, 2019

klingerf reviewed Feb 15, 2019

View reviewed changes

dadjeibaah reviewed Feb 15, 2019

View reviewed changes

olix0r and others added 4 commits February 15, 2019 13:30

Rename proxy package to destination package

e44ee84

Signed-off-by: Kevin Lingerfelt <[email protected]>

Additional renames in the cli/cmd package

ddcb14d

Signed-off-by: Kevin Lingerfelt <[email protected]>

Additional misc renames

762a4b5

Signed-off-by: Kevin Lingerfelt <[email protected]>

klingerf force-pushed the ver/destination-service branch from ad11905 to 762a4b5 Compare February 15, 2019 23:01

klingerf approved these changes Feb 15, 2019

View reviewed changes

dadjeibaah approved these changes Feb 15, 2019

View reviewed changes

olix0r merged commit 71ce786 into master Feb 15, 2019

olix0r deleted the ver/destination-service branch February 15, 2019 23:11

klingerf mentioned this pull request Feb 18, 2019

Add discovery package for managing discovery API #2317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename linkerd-proxy-api to linkerd-destination#2281

Rename linkerd-proxy-api to linkerd-destination#2281
olix0r merged 4 commits intomasterfrom
ver/destination-service

olix0r commented Feb 14, 2019 •

edited

Loading

Uh oh!

olix0r commented Feb 14, 2019

Uh oh!

klingerf left a comment

Uh oh!

klingerf Feb 15, 2019

Uh oh!

klingerf Feb 15, 2019

Uh oh!

klingerf Feb 15, 2019

Uh oh!

klingerf Feb 15, 2019

Uh oh!

dadjeibaah left a comment

Uh oh!

dadjeibaah Feb 14, 2019

Uh oh!

klingerf left a comment

Uh oh!

olix0r commented Feb 15, 2019

Uh oh!

dadjeibaah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

olix0r commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olix0r commented Feb 14, 2019

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

klingerf Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

dadjeibaah left a comment

Choose a reason for hiding this comment

Uh oh!

dadjeibaah Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

olix0r commented Feb 15, 2019

Uh oh!

dadjeibaah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olix0r commented Feb 14, 2019 •

edited

Loading