Implement egress proxy for external services #463

GregHanson · 2017-04-06T20:16:17Z

Assumptions:

external services can have only one port, and that port will be either 80 or 443
Support for external services of type https will be addressed in a later pull request

See issue #67 for discussion

rshriram · 2017-04-06T20:42:53Z

proxy/envoy/config.go

+				if service.External {
+					for _ , route := range routes {
+						for _ , cluster := range route.clusters {
+							cluster.ServiceName = ""


Why empty the service name in the cluster?

ServiceName is only for clusters of type sds

I wonder if we should use SDS for egress proxies. Probably not in this PR.

Yes I was debating about whether to use a default URL from MeshConfig or pointing it to SDS. I chose the meshconfig option because I figured otherwise I would have to either store the egress model.Service or cache it somehow so I could reliably generate a ServiceName for the cluster

We cannot use SDS for egress proxies. Because SDS cannot serve DNS names. Only IP/ports are allowed. If we convince Matt that SDS could allow logical_dns and strict_dns, then we might use SDS for egress as well.

@rshriram this is for the sidecar proxy config gen. SDS in this case would return the IP of the egress service. Egress proxy config gen would NOT use SDS because it will have to statically define the cluster with hosts (i.e. google.com, etc)

kyessenov · 2017-04-07T00:36:33Z

proxy/envoy/config.go

+				if service.External {
+					for _ , route := range routes {
+						for _ , cluster := range route.clusters {
+							cluster.ServiceName = ""


I wonder if we should use SDS for egress proxies. Probably not in this PR.

kyessenov · 2017-04-07T00:38:21Z

proxy/envoy/resources.go

@@ -73,6 +76,7 @@ type MeshConfig struct {
 var (
 	// DefaultMeshConfig configuration
 	DefaultMeshConfig = &MeshConfig{
+		EgressAddress:             "istio-egress:80",


EgressProxyAddress (we'll converge with ingress eventually)

kyessenov · 2017-04-07T00:38:49Z

proxy/envoy/resources.go

@@ -210,6 +214,8 @@ type HTTPRoute struct {
 	// faults contains the set of referenced faults in the route; the field is special
 	// and used only to aggregate fault filter information after composing routes
 	faults []*HTTPFilter
+
+	AutoHostRewrite bool `json:"auto_host_rewrite,omitempty"`


can you move it up above private fields?

kyessenov · 2017-04-07T00:41:00Z

platform/kube/conversion.go

 	if svc.Spec.ClusterIP != "" && svc.Spec.ClusterIP != v1.ClusterIPNone {
 		addr = svc.Spec.ClusterIP
 	}

+	if svc.Spec.ExternalName != "" {


You should compare svc.Spec service type, like in https://github.com/istio/manager/blob/master/platform/kube/controller.go#L520. There is some dead code there that needs to be updated as well.

istio-testing · 2017-04-07T19:28:06Z

Jenkins job manager/presubmit passed

codecov · 2017-04-07T20:56:10Z

Codecov Report

Merging #463 into master will decrease coverage by 0.28%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
- Coverage    76.5%   76.21%   -0.29%     
==========================================
  Files          26       26              
  Lines        3413     3376      -37     
==========================================
- Hits         2611     2573      -38     
- Misses        608      612       +4     
+ Partials      194      191       -3

Impacted Files	Coverage Δ
proxy/envoy/resources.go	`78.81% <ø> (-8.93%)`	⬇️
proxy/envoy/config.go	`91.28% <100%> (+0.46%)`	⬆️
test/mock/service.go	`91.2% <100%> (+0.99%)`	⬆️
proxy/envoy/route.go	`91.61% <100%> (ø)`	⬆️
platform/kube/conversion.go	`81.6% <100%> (+0.32%)`	⬆️
proxy/envoy/egress.go	`78.82% <78.82%> (ø)`
test/mock/secret.go	`0% <0%> (-50%)`	⬇️
proxy/envoy/ingress.go	`64.6% <0%> (-6.83%)`	⬇️
proxy/envoy/discovery.go	`73.65% <0%> (-5.17%)`	⬇️
... and 7 more

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 bd53495...9576d0b. Read the comment docs.

istio-testing · 2017-04-07T21:17:31Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-07T21:21:58Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-07T21:32:44Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-07T21:38:44Z

Jenkins job istio/manager-pr-master passed

rshriram · 2017-04-07T22:36:29Z

proxy/envoy/egress.go

+	rConfig := &HTTPRouteConfig{VirtualHosts: vhosts}
+
+	listener := &Listener{
+		Address:    fmt.Sprintf("tcp://%s:80", WildcardAddress),


Hard coding port here? Isn't istio egress service port configurable ?

This is egress proxy port, not the service port. @GregHanson please add a comment saying that service must have exactly one port or they get conflated.

rshriram · 2017-04-07T22:37:50Z

proxy/envoy/egress.go

+		},
+	}
+
+	//TODO


Doesn't apply to egress right? Please remove comment. But add a comment related to tcp service

istio-testing · 2017-04-11T15:01:42Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-11T15:18:22Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-11T15:22:22Z

Jenkins job manager/e2e-smoketest passed

istio-testing · 2017-04-11T15:38:24Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-11T15:42:19Z

Jenkins job manager/e2e-smoketest passed

istio-testing · 2017-04-11T19:44:51Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-11T19:48:39Z

Jenkins job manager/e2e-smoketest passed

kyessenov

Overall LGTM. I suggest we add a field to hold the external name instead of overloading Address. This will be needed for TCP external services.

kyessenov · 2017-04-11T21:55:45Z

proxy/envoy/config.go

@@ -269,6 +270,22 @@ func buildOutboundHTTPRoutes(instances []*model.ServiceInstance, services []*mod
 					routes = append(routes, buildDefaultRoute(cluster))
 				}

+				if service.External {
+					for _, route := range routes {
+						route.HostRewrite = service.Hostname


Is this needed? Isn't pod already using service hostname to address an external service?

it was a design choice. For example the proxy in the pod matches on this list of domains:

"httpbin:80", "httpbin", "httpbin.default:80", "httpbin.default", "httpbin.default.svc:80", "httpbin.default.svc", "httpbin.default.svc.cluster:80", "httpbin.default.svc.cluster", "httpbin.default.svc.cluster.local:80", "httpbin.default.svc.cluster.local", "httpbin.org:80", "httpbin.org"

By setting route.HostRewrite to a known value, the egress proxy explicitly only listens/matches for one of those domains:

"httpbin.org"

OK, I see. I don't think "httpbin.org" should be in that list. This doesn't sound right @rshriram

what do you mean? The application addresses the external service directly by the name and not via the kubernetes version of the service (httpbin.default.svc.cluster.local).

kyessenov · 2017-04-11T21:58:13Z

proxy/envoy/egress.go

+
+// EgressConfig defines information for engress
+type EgressConfig struct {
+	Namespace string


This struct is still used as the input struct for generateEgress()

kyessenov · 2017-04-11T21:58:48Z

proxy/envoy/egress.go

+}
+
+func generateEgress(conf *EgressConfig) *Config {
+


unnecessary white space

kyessenov · 2017-04-11T22:06:38Z

proxy/envoy/egress.go

+	rConfig := &HTTPRouteConfig{VirtualHosts: vhosts}
+
+	listener := &Listener{
+		Address:    fmt.Sprintf("tcp://%s:80", WildcardAddress),


This is egress proxy port, not the service port. @GregHanson please add a comment saying that service must have exactly one port or they get conflated.

kyessenov · 2017-04-11T22:08:04Z

proxy/envoy/egress.go

+			}
+
+		case model.ProtocolTCP, model.ProtocolHTTPS:
+		// TODO


drop this clause, so that default case will print a warning.

kyessenov · 2017-04-11T22:09:53Z

proxy/envoy/egress.go

+			route.clusters = append(route.clusters, cluster)
+
+			host = &VirtualHost{
+				Name:    buildEgressClusterName(svc.Address, servicePort.Port),


reuse cluster.Name

kyessenov · 2017-04-11T22:10:42Z

proxy/envoy/egress.go

+			route := &HTTPRoute{
+				Prefix:          "/",
+				Cluster:         buildEgressClusterName(svc.Address, servicePort.Port),
+				AutoHostRewrite: true,


Should we just put HostRewrite to the external name? It's known ahead of time based on the cluster definition.

In CloudFoundry case: catalog-1.mystore.com, catalog-2.mystore.com, catalog-3.mystore.com are all under under catalog.mystore.com
Although the current egress implementation does not support this yet, I tried to model the envoy config with this in mind. With AutoHostRewrite Host header will be set to the value of the URL from the host chosen in the cluster

kyessenov · 2017-04-11T22:13:02Z

test/mock/service.go

+	WorldService   = MakeService("world.default.svc.cluster.local", "10.2.0.0")
+	ExtHTTPService = MakeExternalHTTPService("httpbin.default.svc.cluster.local",
+		"httpbin.org", "10.3.0.0")
+	ExtHTTPSService = MakeExternalHTTPSService("httpsbin.default.svc.cluster.local",


please remove this until it is used.

I left it in there originally because until external https services are supported - it should not appear in RDS/CDS/egress outputs. And until they are supported, if someone pushes a change to the config generation logic - I wanted there to be something in place to catch it

OK, so it's a negative test. That's fine then.

kyessenov · 2017-04-11T22:17:17Z

platform/kube/conversion.go

 	if svc.Spec.ClusterIP != "" && svc.Spec.ClusterIP != v1.ClusterIPNone {
 		addr = svc.Spec.ClusterIP
 	}

+	if svc.Spec.Type == v1.ServiceTypeExternalName && svc.Spec.ExternalName != "" {
+		addr = svc.Spec.ExternalName


I think you should add a field to Service called ExternalName to hold this value. Address is needed for TCP/HTTPS routing for the in-cluster IP address.

Do you mean adding it to model.Service ? If so, then it simply conflates the model. Services already have a hostname in the model and there is nothing in the model that says a service is in cluster or not. So adding an ExternalName makes little sense from a generic model perspective.

If anything, I would add a type field to the model.Service, that can qualify a service as either "internal/external", deployment mode as a boolean "user-facing or not". This would let ingress/gateway Envoy pick up only user-facing services of internal Type, non-k8s environments), egress Envoy pick external services while other envoys can pickup internal services and external service (with a host rewrite to egress envoy).

I can also use service.Hostname to store the ExternalName value and just not touch service.Address in the external case . I originally chose to overwrite service.Address with ExternalName because I wanted to preserve the service.default.local.svc value because the logic for generating it was not exposed.

kyessenov · 2017-04-11T22:17:33Z

test/mock/service.go

@@ -79,6 +115,9 @@ func MakeInstance(service *model.Service, port *model.Port, version int) *model.
 // MakeIP creates a fake IP address for a service and instance version
 func MakeIP(service *model.Service, version int) string {
 	ip := net.ParseIP(service.Address).To4()
+	if ip == nil {


This isn't needed if you add a field for the external name.

as a general istio user wishing to use an external service - are they required to know the hostname (google.com) and the IP it resolves to? Or from a coding standpoint then, if an external service is specified how do we populate the IP field, or would we just leave IP/Address as an empty string in this case? And if left empty, we would still need this ip == nil check in the code or else the next few lines trigger an index out of bounds exception

I think this will be the case until we start using Endpoints for external services

rshriram

I don't think adding ExternalName to the service is a good idea as it conflates the model.

rshriram

.

kyessenov · 2017-04-12T17:02:33Z

The way I see it, ExternalName service is still a regular Istio service with a hostname (cluster-local), an IP address, but no actual pods. Instead, the service is redirected to out-of-cluster DNS name. Logically, this means external name service is a subtype of a service, so we need to add fields to express this in golang.

rshriram · 2017-04-12T18:30:26Z

ExternalName service is a platform specific implementation detail - with respect to kubernetes. The way I see it, external service is just an extension of the Istio mesh, allowing services outside the mesh to be represented in Istio. Doing the external name exposes the internal implementation detail of having an explicit istio-egress service. If we move towards a model where the sidecars directly route to external services (after talking to mixer), then this whole thing becomes redundant.

istio-testing · 2017-04-12T19:47:12Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-12T19:55:51Z

Jenkins job manager/e2e-smoketest passed

istio-testing · 2017-04-13T15:05:06Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-13T15:11:00Z

Jenkins job manager/e2e-smoketest passed

istio-testing · 2017-04-13T15:57:39Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-13T16:07:50Z

Jenkins job manager/e2e-smoketest passed

istio-testing · 2017-04-13T16:31:11Z

Jenkins job manager/presubmit passed

istio-testing · 2017-04-13T16:37:31Z

Jenkins job manager/e2e-smoketest passed

first pass at egress service implementation

b546669

rshriram reviewed Apr 6, 2017

View reviewed changes

kyessenov reviewed Apr 7, 2017

View reviewed changes

GregHanson added 2 commits April 7, 2017 13:16

added integration test

c2d2033

golint fixes

4db5735

GregHanson mentioned this pull request Apr 7, 2017

added egress proxy service to proxyconfig istio/api#70

Merged

merged with master

d817468

GregHanson added 2 commits April 7, 2017 16:02

removed unnecessary files

bfcb517

removed unnecessary files

e8fecfa

rshriram reviewed Apr 7, 2017

View reviewed changes

GregHanson added 5 commits April 10, 2017 17:01

improved test coverage. egress cluster name generation mod

063afb9

Merge branch 'master' into egress_proxy

7766a4f

golint fix

ab6b0ab

attempted bug fix for codecov tool

7e40828

merge with master

dde8349

removed short-circuited logic

d739f59

GregHanson changed the title ~~[WIP] first pass at egress service implementation~~ Implement egress proxy for external services Apr 11, 2017

bug fix for egress e2e testing. only match on hostname for egress routes

52f46cc

kyessenov suggested changes Apr 11, 2017

View reviewed changes

rshriram reviewed Apr 12, 2017

View reviewed changes

external name now stored in Service.External instead of .Address

158cb0b

GregHanson added 2 commits April 12, 2017 15:01

merged with master

b242f8c

disable e2e auth tests for egress

a71f5a9

ayj mentioned this pull request Apr 13, 2017

Allow external traffic from Istio pods #515

Closed

GregHanson added 2 commits April 13, 2017 07:45

Merge branch 'master' into egress_proxy

08fe7a4

updated test all cds/rds/sds unit tests for external services

2107bf2

rshriram approved these changes Apr 13, 2017

View reviewed changes

go fmt fixes

adfd5d1

Merge branch 'master' into egress_proxy

9576d0b

rshriram merged commit 43218f1 into istio:master Apr 13, 2017

rshriram mentioned this pull request Apr 13, 2017

Egress proxy for external services #67

Closed

GregHanson deleted the egress_proxy branch April 13, 2017 18:10

vadimeisenbergibm mentioned this pull request Jul 25, 2017

Implement external (from the mesh to the outside) traffic control istio/istio#496

Closed

+              		},
+              	}
+              	//TODO

Implement egress proxy for external services #463

Implement egress proxy for external services #463

Uh oh!

Conversation

GregHanson commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

istio-testing commented Apr 7, 2017

Uh oh!

codecov bot commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

istio-testing commented Apr 7, 2017

Uh oh!

istio-testing commented Apr 7, 2017

Uh oh!

istio-testing commented Apr 7, 2017

Uh oh!

istio-testing commented Apr 7, 2017

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!

istio-testing commented Apr 11, 2017

Uh oh!

istio-testing commented Apr 11, 2017

Uh oh!

istio-testing commented Apr 11, 2017

Uh oh!

istio-testing commented Apr 11, 2017

Uh oh!

istio-testing commented Apr 11, 2017

Uh oh!

istio-testing commented Apr 11, 2017

Uh oh!

istio-testing commented Apr 11, 2017

Uh oh!

kyessenov 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!

GregHanson Apr 12, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

GregHanson commented Apr 6, 2017 •

edited

Loading

codecov bot commented Apr 7, 2017 •

edited

Loading

GregHanson Apr 12, 2017 •

edited

Loading

GregHanson Apr 12, 2017 •

edited

Loading

GregHanson Apr 12, 2017 •

edited

Loading