Foreign Services - followup #307
Conversation
googlebot
added
the
cla: yes
|
@frankbu PTAL. I have folded the entire thing into one single atomic foreign services. I think the namespace issue can be tackled across all three resources uniformly |
| // that services in the mesh are allowed to access. | ||
| // Foreign service describes the endpoints, ports and protocols of a | ||
| // white-listed set of mesh-external domains and IP blocks that services in | ||
| // the mesh are allowed to access. |
There was a problem hiding this comment.
This doesn't seem to be a description of a "service", so the resource name "ForeignService" seems odd.
|
I'm not following the new idea. Can there now be multiple ForeignService resources, i.e., one for |
| // the mesh are allowed to access. | ||
| // | ||
| // NOTE: If a foreign service has the same name as a service in the | ||
| // service registry, the foreign service's declaration will be given |
There was a problem hiding this comment.
What is the name of a foreign service? The metadata.name (i.e. foreign-svc in your example) doesn't seem to be the name of any service
|
@frankbu you can now have multiple foreign service resources, just like egress rules. Removed the confusing wording around the registry overwrite. |
| message PortMap { | ||
| // REQUIRED: A valid non-negative integer port number on which | ||
| // connections to the external service would be received. | ||
| uint32 in_port = 1; |
There was a problem hiding this comment.
Are we sure we always have a port number here? Should we reuse the Port message?
There was a problem hiding this comment.
Tried and dumped.. the port message in foreign service has to accomodate the tls upgrade, internal and external ports (when app wants to do http://google and wants sidecar to do https://google). The generic port message in gateway does not accomodate this. Because the port in the routing rule serves as a way in the gateway to route to a particular upstream on specific port.
There was a problem hiding this comment.
Given that we've agreed to haev port declarations in dest-rule I think this mapping should be declared there.
Also I think it might be more readable to have something like this in dest-rule....
port :
- number : 80
name: http
forwardTo: https #forward reference by port name
- number : 443
name: https
Can also then use circuit-breaking to shut-down direct access to 443 when Envoy not originating traffic. Seems like something which belongs in traffic-policy
There was a problem hiding this comment.
Destination rule does not declare ports.. It has port specific policies. Especially, with teh wildcard and other stuff, we cannot declare ports. The above format straddles route and cluster.. pushign that into destination rule is going to add complexity. Besides, routing already has this port rerouting thing.
| // PortMap describes the properties of a specific port of the external | ||
| // service, the port by which the service will be accessed inside the | ||
| // mesh and TLS upgrade options if any. | ||
| message PortMap { |
There was a problem hiding this comment.
I think this should be part ofSubset's TrafficPolicy:
api/routing/v1alpha2/destination_rule.proto
Lines 97 to 109 in ba8e7ca
Then we can do similar things inside of the mesh too.
There was a problem hiding this comment.
Yeah I toyed with it..The problem is the dang inheritance. DestinationRules support inheritance. It is not exactly clear where the port will fit in, as it becomes a problem of multiple inheritance.
Also, why push the port into just the subset? why not push it into the main destination rule as well ?
There was a problem hiding this comment.
and I couldn't do that, because there is also a port in the routing rule. The existing flow is gatewayPort->routeRule(decidePort)->destinationRule(no port business here)->out.
Adding a new inbound path (foreign services) is causing this dilemma.
| // mesh and TLS upgrade options if any. | ||
| message PortMap { | ||
| // REQUIRED: A valid non-negative integer port number on which | ||
| // connections to the external service would be received. |
There was a problem hiding this comment.
This seems a little unclear to me, what about:
The port number on which the application in the mesh will call the external service.
I don't think we need to say "valid non-negative integer."
| // service must be made over a different port (e.g., https over port | ||
| // 443). | ||
| // | ||
| // NOTE: outPort cannot be specified when the discovery mode is set to |
There was a problem hiding this comment.
Why? Even with static addresses, seems reasonable to want offload TLS to mesh and upgrade to a different port w/ TLS when egressing.
There was a problem hiding this comment.
outPort applies only with static and DNS. but not with "none" which is the equivalent of original_dst_cluster.. The reason (and its a temporary restriction) is that you cannot rewrite ports in original_dst_clusters.
There was a problem hiding this comment.
Ah, it wasn't clear to me if this was an artifact of Envoy or not.
| // HTTP|HTTPS|GRPC|HTTP2|MONGO|TCP. When protocol is set to HTTPS, the | ||
| // traffic will be treated as opaque TCP traffic, and TCP level | ||
| // constraints will apply. | ||
| string protocol = 3; |
There was a problem hiding this comment.
We really need to make this an enum, we're reusing the same thing all over the place.
There was a problem hiding this comment.
Yes, let's upstream model service from istio to here. If the intention is to define egress as
an external service, then we should start from an internal service definition.
There was a problem hiding this comment.
hard to make this an enum when we'll have 3rd-party protocol support :/
| } | ||
|
|
||
| // REQUIRED: The Ports associated with the external services. | ||
| repeated PortMap ports = 2; |
There was a problem hiding this comment.
Is a PortMap really required? Can't we just blindly forward the app's request if we have no PortMap configured? Service discovery ensures that in the mesh requests to the egress proxies implementing this behavior will hit the port its listening on, and if the side-car is the egress proxy then there's no problem there either.
There was a problem hiding this comment.
You need some set of ports for the service right? Otherwise, we would have to assume that default port is 80, which seems wonky. PortMap may be badly named, but essentially, it is the equivalent of ports, with some extra sauce to forward to a different outbound port.
|
I still think naming is causing confusion and making this harder to understand than necessary.
@rshriram It sounds like you've already considered, but discarded, the idea of using
|
|
I was actually pro using the Gateway. But @louiscryan is of the opinion that this is overloading the functionality in the wrong manner. This is essentially adding some more entries to the service discovery system, when its a simple matter of *.foo.com -> foo.bar.com.. But when we have something like foo.bar.com -> 1.1.1.1, 2.2.2.2, 3.3.3.3, then we are describing a pseudo service reg spec. Either way, the gateway is a good place to stick these in. |
|
I think that the naming confusion/disagreement stems from the fact that
So both are describing some of the same kinds of things: ports, protocols, etc., of services that cross the mesh internal/external boundry. What's different between the two is that The way I see it, there are 3 kinds of requests that need to be considered in traffic routing:
#1 is handled by the RouteRules and DestinationRules. #2 and #3 require some additional config. Standing back and looking at this from a high level I think a typical user with little or no implementation knowledge would think of #2 and #3 as gateway config, not a gateway and a (foreign) service. That would argue for using If we don't want to use the names I suggested above The bottom line is that this new v1alpha2 spec solves a lot of problems but IMO a few bad names are preventing this from being something that could be relatively simple to understand. |
|
Per discussion today, gateway is just a proxy. Internal/external/in-between. ForeignServices is a way to add more entries to the service registry (kube's external services wont suffice, because we have wildcard destinations). By default, foreign services will be exposed via sidecars directly. But you can implement a dedicated egress proxy using the gateway. When you do the latter, you declare foreign services as exposable only at gateway (this is missing today), then you write a route rule that will forward traffic from sidecar to the gateway host. |
|
|
||
| // Endpoint defines a network address (IP or hostname) associated with | ||
| // the foreign service. | ||
| message Endpoint { |
There was a problem hiding this comment.
Still doesn't seem to cover port variation per endpoint instance. Would have expected to see
map<string, int> ports = 3; // Mapping of port names to physical port numbers by endpoint instance
There was a problem hiding this comment.
Ah yes.. That was an intentional omit. The problem was it made no sense with inport and outport. It also made no sense when there was port remapping happening at the route level.
|
Updates: I removed the portMap thing completely. Its back to good old Ports. Its also back to the good old http://google.com:443 model, so that we can make some progress on the foreign services while we sort out the saner way of doing 80 to 443. People might most likely use https from the start, and not bother to use Istio for monitoring external APIs. As that ensures that the application works with and without Istio. So, in lieu of that, it seems its not that important to cater to this TLS upgrade use case. Other than that, the other nits are per previous PR. |
|
LGTM Envoy uses an Address proto to capture the concept of host and port. I wonder if we should do something similar. Just an idea for future consideration. |
|
@wora will do, in next iteration (just wanted to get this out of the door by 22nd) for initial feedback. After @kyessenov refactors the envoy apis, I hope it would make it easier to import/download these protos separately, reuse data structures. I wanted to import the TLS stuff as well, but it was too buried into the entire API, and I couldn't import easily. Hopefully, with the refactoring, I should be able to import all the envoy TLS things |
|
Okay, @ZackButcher @frankbu and @kyessenov prefer the term ExternalService. I have no preference. Don't mind renaming.. @louiscryan if you have any concern, now would be a good time to speak before I rename the files.. |
|
LGTM and fine with rename to ExternalService |
|
Is this related with defect： upstream connect error or disconnect/reset before headers? Is this fix merged in istio0.7.1? We still find istio/istio#4999 raise in istio0.7.1 |
Signed-off-by: Liam White <[email protected]>
This is mostly addressing nits to the previous PR #273 .. Fixing docs, and making the endpoint ports plural.