Eureka service discovery adapter #994

ijsnellf · 2017-08-05T01:02:30Z

Basic Eureka support. Forthcoming:

More documentation describing how Eureka data is consumed
Integration tests (after config store work becomes more settled)

Note that TCP services will not work with Eureka until headless TCP services are supported in our proxy generation logic.

codecov · 2017-08-05T02:24:10Z

Codecov Report

Merging #994 into master will increase coverage by 0.41%.
The diff coverage is 77.08%.

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
+ Coverage   71.65%   72.07%   +0.41%     
==========================================
  Files          43       47       +4     
  Lines        5067     5307     +240     
==========================================
+ Hits         3631     3825     +194     
- Misses       1278     1304      +26     
- Partials      158      178      +20

Impacted Files	Coverage Δ
platform/eureka/client.go	`56.81% <56.81%> (ø)`
platform/eureka/servicediscovery.go	`70.49% <70.49%> (ø)`
platform/eureka/conversion.go	`84.21% <84.21%> (ø)`
platform/eureka/controller.go	`92.5% <92.5%> (ø)`
platform/kube/controller.go	`61.45% <0%> (+3.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de4446f...5b27021. Read the comment docs.

rshriram

@ijsnellf can you add a readme under platform/eureka documenting how you map the eureka fields to our service model, limitations and any todos, requirements expected from end users?

@GregHanson can you do the same for Consul ?

We don't need elaborate docs. Just enough to get other devs started, if they want to contribute.

cmluciano

One comment on health

I'd like to see this use a client library like fargo in the future.

cmluciano · 2017-08-07T18:15:34Z

platform/eureka/testdata/eureka-apps.json

+                  "hostName": "foo.bar.local",
+                  "app": "FOO_BAR_LOCAL",
+                  "ipAddr": "10.0.0.2",
+                  "status": "UP",


This normally indicates health as posted by local healthchecks or those embedded in clients like ribbon. Since this is updated by each application instance, I think we should filter out instances with status: "down".

Yes, that sounds reasonable.

@cmluciano: filter out everything except for "UP", I assume? Possible values: UP, DOWN, STARTING, OUT_OF_SERVICE, UNKNOWN

Yes everything except for up

GregHanson · 2017-08-08T20:05:53Z

platform/eureka/servicediscovery_test.go

@@ -0,0 +1,161 @@
+package eureka


missing copyright

GregHanson · 2017-08-08T20:09:59Z

platform/eureka/controller.go

+	"time"
+
+	"reflect"
+


kind of surprised this passed this goimports test, I believe "time" and "reflect" should be sorted alphabetically

Please remove extra line between the imports. Lines between imports are used to group imports together.

Yep, they got added randomly. Surprised that goimports doesn't remove them automatically.

GregHanson · 2017-08-08T20:11:14Z

platform/eureka/client.go

+
+type application struct {
+	Name      string      `json:"name"`
+	Instances []*instance `json:"instance"`


json:"instance" or json:"instances"

Eureka idiosyncrasies. instance is correct.

cmluciano · 2017-08-09T15:02:13Z

platform/consul/conversion_test.go

@@ -191,7 +191,7 @@ func TestConvertService(t *testing.T) {
 	}

 	if len(out.Ports) != 1 {
-		t.Errorf("covnertService() incorrect # of ports => %v, want %v",
+		t.Errorf("convertService() incorrect # of ports => %v, want %v",


Nit: should this be moved to a new PR?

Technically, but if it has to be it won't happen.

cmluciano · 2017-08-09T15:03:19Z

platform/eureka/client.go

+	Hostname   string   `json:"hostName"`
+	App        string   `json:"app"`
+	IPAddress  string   `json:"ipAddr"`
+	Status     string   `json:"status"`


I forget, does status mean health? If so do we already drop non-"UP" instances?

Yes. We filter out the non-"UP" instances in the conversion logic (in conversion.go).

cmluciano · 2017-08-09T15:05:51Z

platform/eureka/client.go

+const statusUp = "UP"
+
+const (
+	basePath = "/eureka/v2"


Should note in docs that this is hardcoded to Eureka v2

Can this be made configurable in the future at least?

+1. Can't we use a config file or something to take in all these params?

Yes. We might move to a Eureka library in the future as well.

I was wrong in my original interpretation of this. The v2 endpoint has no relation to the v2 release of Eureka.

cmluciano · 2017-08-09T15:07:53Z

platform/eureka/client_test.go

+			data:    readFile(t, "testdata/eureka-apps.json"),
+			apps: []*application{
+				{
+					Name: appName("foo.bar.local"),


Nit: This is fine for testing, but I don't think it is common to place a full DNS entry in the Eureka application name.

Yeah, makes sense. The app name shouldn't matter at all for any of the actual logic, but I can change it so it is less confusing.

cmluciano · 2017-08-09T15:09:09Z

platform/eureka/controller.go

+			if !reflect.DeepEqual(apps, cachedApps) {
+				cachedApps = apps
+				// TODO: feed with real events.
+				// The handlers are being feed dummy events. This is sufficient with simplistic


spencergibb · 2017-08-11T14:43:11Z

Can I ask how heartbeats are being handled (haven't had a chance to review yet)?

spencergibb · 2017-08-11T14:44:11Z

I probably just answered my own question in my mind, clients still handle registration and heartbeats, this is just another client, is that right?

cmluciano · 2017-08-11T17:28:20Z

@spencergibb Yes client libraries embedded in the application will normally do this. Is pilot responsible for handling health-checks on each application?

rshriram · 2017-08-11T17:48:00Z

No. We just do discovery. Registration and heartbeats are application/platform's responsibility

ijsnellf · 2017-08-14T16:01:33Z

Thanks for the comments everyone, I think I addressed them all. Should be ready to merge, pending an approval.

rshriram

This looks good for a first cut. Bunch of nits and a few important TODOs (esp the reading port protocols from a file).

rshriram · 2017-08-14T15:36:47Z

platform/eureka/controller.go

+				continue
+			}
+
+			if !reflect.DeepEqual(apps, cachedApps) {


+1. Since we don't control how the Eureka server handles the order of registered instances, we should make this comparison deterministic on our end by sorting the apps before caching/comparison.

rshriram · 2017-08-14T15:39:19Z

platform/eureka/conversion.go

+
+// Convert Eureka applications to services. If provided, only convert applications in the hostnames whitelist,
+// otherwise convert all.
+func convertServices(apps []*application, hostnames map[string]bool) map[string]*model.Service {


Is there a reason to have this special case for filtering hostnames in the white list?

Only for convenience. There are several places where we filter hostnames.

rshriram · 2017-08-14T15:47:49Z

platform/eureka/conversion.go

+}
+
+func convertPorts(instance *instance) model.PortList {
+	out := make(model.PortList, 0, 2)


add a comment stating that eureka instances have only two ports

rshriram · 2017-08-14T15:50:11Z

platform/eureka/conversion.go

+
+				service.Ports = append(service.Ports, port)
+			}
+		}


don't you need to delete the service from the map if it has no ports attached to it? Is this possible in Eureka?

rshriram · 2017-08-14T15:57:55Z

platform/eureka/conversion_test.go

+			},
+			services: map[string]*model.Service{
+				"foo.bar.local": makeService("foo.bar.local", []int{5000}, nil),
+				"foo.biz.local": makeService("foo.biz.local", []int{5000}, nil),


may be have the protocol also here, for completeness ?

rshriram · 2017-08-14T15:59:13Z

platform/eureka/servicediscovery.go

+	}
+	services := convertServices(apps, nil)
+
+	// TODO: canonical ordering?


I think this is a requirement else Envoy will go crazy. Use stable sort or whatever and make sure that the return value is deterministic.

Thanks, missed that I hadn't dealt with that TODO

rshriram · 2017-08-14T16:29:01Z

platform/eureka/conversion.go

+	return out
+}
+
+const protocolMetadata = "istio.protocol" // metadata key for port protocol


I understand that Eureka does not support protocols. But this would require changes to existing eureka clients or require people to update instance info out of band.

As part of the effort to read Eureka configuration from a file (e.g., the server's IPs, API base URL, etc., one can also imagine that the user provides a list of ports and their protocols - which can be used during runtime to lookup the protocols associated with a port). I leave it up to you to do this in this PR or in a follow up PR to add the ability to read this info from a statically supplied config file.

The metadata can be supplied via a config file for the Eureka clients. Do you think that is reasonable? It would have to be a config file change on each Eureka client in your env.

See: https://github.com/Netflix/eureka/wiki/Overriding-Default-Configurations#statically-set-via-configuration

For the alternative that you are suggesting, are you imagining a config file that the Istio discovery server would read?

Yes.. Because odds are, someone already has bunch of apps running on VMs or wherever. For these folks, do you think its easy to update the Eureka server with metadata info out of band and will this info remain in tact despite heartbeats etc. from apps?

rshriram · 2017-08-14T16:30:58Z

platform/eureka/client.go

+}
+
+type instance struct {
+	Hostname   string   `json:"hostName"`


According to the docs, hostName is used during access (not sure if its AWS specific), while there is also a virtualHostName and secureVirtualHostName(). Question is, when I say http://reviews.foo.local/getReviews, is reviews.foo.local coming from hostName or virtualHostName ?
@cmluciano ?

Talked to @seh about these:

hostName should refer to the machine where the application resides.

virtualHostName is similar to a VIP (Virtual IP) that a client could add

rshriram · 2017-08-14T16:32:07Z

platform/eureka/client.go

+	Status     string   `json:"status"`
+	Port       *port    `json:"port,omitempty"`
+	SecurePort *port    `json:"securePort,omitempty"`
+	Metadata   metadata `json:"metadata,omitempty"`


According to docs, https://github.com/Netflix/eureka/wiki/Eureka-REST-operations, the instance also has a vipAddress. Is this equivalent to our service VIP address in K8S ? If so, may be worthwhile to include it?

Eureka's VIPs aren't the same as k8s service addresses. VIP address can be used to obtain hostname/port pairs using the Eureka client, but the requests themselves are made to the hostname, not the VIP address.

See: https://github.com/Netflix/eureka/wiki/Understanding-eureka-client-server-communication#eureka-client-operations

We may have it in the future, but until then our linters get mad because it is an unused variable, so I've left it out. :)

rshriram · 2017-08-14T16:33:32Z

platform/eureka/client.go

+	Hostname   string   `json:"hostName"`
+	App        string   `json:"app"`
+	IPAddress  string   `json:"ipAddr"`
+	Status     string   `json:"status"`


Please add a TODO to parse the datacenter info from instance (https://github.com/Netflix/eureka/wiki/Eureka-REST-operations) - this is needed to populate Envoy's AZ info per instance.

rshriram · 2017-08-14T16:36:55Z

@jhspaybar would appreciate any feedback you may have :).

kyessenov

Code-wise I think it's alright. We need a short explanation of how we map Eureka model into our data model (things like two-ports, hostnames, labels). Also, an integration test for a dummy eureka setup would be great.

seh · 2017-08-15T23:50:25Z

platform/eureka/client.go

+	Metadata metadata `json:"metadata,omitempty"`
+}
+
+type port struct {


This isn't going to work with older Eureka servers.
See Preserve an instance's enabled status for its two ports hudl/fargo#59 for precedent in handling the differences.

FWIW, pivotal doesn't support the older eureka servers. Maybe a documentation not on minimum supported eureka version?

Thanks @seh. I created an issue: #1378

Isaiah Snell-Feikema added 7 commits August 4, 2017 19:46

eureka platform

d44f252

gofmt

922141a

rename

faf5b6e

format + basic docs

9c77d01

added more doc

76faa71

fix confusing variable names + fmt

0f32ff2

update bazel

683204b

istio deleted a comment from istio-testing Aug 5, 2017

fix checks

559c540

istio deleted a comment from istio-testing Aug 5, 2017

rshriram reviewed Aug 5, 2017

View reviewed changes

Isaiah Snell-Feikema added 6 commits August 5, 2017 20:17

basic conversion tests

24dc8f8

basic client tests

7a654d2

minor refactor of client

22554c3

fixed typo

6888f07

full client tests

1fb885a

removed unused tests

52897e1

ijsnellf changed the title ~~[WIP] Eureka support~~ Eureka support Aug 7, 2017

Isaiah Snell-Feikema added 3 commits August 7, 2017 11:28

Merge branch 'master' of github.com:istio/pilot into eureka

686160f

readded accidentally removed file

b2b9136

checks

99ba6ac

ijsnellf requested review from kyessenov and GregHanson August 7, 2017 16:41

lint

df955d2

cmluciano reviewed Aug 7, 2017

View reviewed changes

Isaiah Snell-Feikema added 4 commits August 7, 2017 14:21

sd tests

62618a6

Merge branch 'master' of github.com:istio/pilot into eureka

c25c520

filter eureka instances based on status

08c38f3

added status filtering to service conversion logic

75058c4

GregHanson reviewed Aug 8, 2017

View reviewed changes

platform/eureka/servicediscovery_test.go

@@ -0,0 +1,161 @@

package eureka

Copy link

Member

GregHanson Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing copyright

GregHanson reviewed Aug 8, 2017

View reviewed changes

cmluciano reviewed Aug 9, 2017

View reviewed changes

Isaiah Snell-Feikema added 6 commits August 14, 2017 08:06

Merge branch 'master' of github.com:istio/pilot into eureka

4b27738

copyrights

f0393ab

remove random newlines

fd395fe

typo

47282c6

docs

c4d07e3

goimports

878f5af

rshriram approved these changes Aug 14, 2017

View reviewed changes

rshriram changed the title ~~Eureka support~~ Eureka adapter Aug 14, 2017

kyessenov approved these changes Aug 14, 2017

View reviewed changes

Isaiah Snell-Feikema added 4 commits August 15, 2017 16:34

address shriram comments

69648cb

Merge branch 'master' of github.com:istio/pilot into eureka

62efe67

no aligncheck

79c9320

Merge branch 'master' of github.com:istio/pilot into eureka

5b27021

rshriram merged commit 3dccc73 into istio:master Aug 15, 2017

rshriram changed the title ~~Eureka adapter~~ Eureka service discovery adapter Aug 15, 2017

seh reviewed Aug 15, 2017

View reviewed changes

rshriram mentioned this pull request Aug 17, 2017

Platform support for CloudFoundry #333

Closed

ijsnellf deleted the eureka branch October 2, 2017 16:58

ijsnellf mentioned this pull request Oct 2, 2017

Eureka adapter support for Eureka pre-v1.2.2 #1378

Closed

kyessenov mentioned this pull request Nov 3, 2017

Eureka adapter support for Eureka pre-v1.2.2 istio/istio#1374

Closed

Eureka service discovery adapter #994

Eureka service discovery adapter #994

Conversation

ijsnellf commented Aug 5, 2017 • edited by rshriram

codecov bot commented Aug 5, 2017 • edited

Codecov Report

rshriram left a comment

Choose a reason for hiding this comment

cmluciano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencergibb commented Aug 11, 2017

spencergibb commented Aug 11, 2017

cmluciano commented Aug 11, 2017

rshriram commented Aug 11, 2017

ijsnellf commented Aug 14, 2017

rshriram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmluciano Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijsnellf Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Aug 14, 2017

kyessenov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijsnellf commented Aug 5, 2017 •

edited by rshriram

codecov bot commented Aug 5, 2017 •

edited

cmluciano Aug 14, 2017 •

edited

ijsnellf Aug 14, 2017 •

edited