trimmed ingress spec with tons of docs #198
Conversation
googlebot
added
the
cla: yes
| // Optional: Indicates whether connections to this port should be secured using TLS. | ||
| // If set to true, Envoy will use the certficates specified in the secrets section. Default | ||
| // is false. | ||
| bool secure = 2; |
There was a problem hiding this comment.
I think there is some subtlety here that a simple boolean does not convey. For example see
where we might require a TLS handshake but not actually terminate TLS in the ingress proxy itself.
There was a problem hiding this comment.
Are you thinking of something like allowing users to specify if Envoy should use TLS or not, and if TLS, terminate TLS or just route to backend?
If SNI-based routing materializes soon, then we might actually augment our route rules to do host based routing, with weighted routing (no rewrites/ header matches, etc.).
There was a problem hiding this comment.
Right, this is useful for the 'mesh extender' use case where all services in the mesh are exposed by their local names through an ingress proxy and simply routed by SNI name when no routing rules apply to the backend.
This has limitations if there are routing rules which apply to things at L7 (headers etc) but the benefit is that you get end-to-end security and possibly better performance.
You could also in theory play tricks with SNI where the client encodes routing labels into the presented name as long as that didn't cause naming conflicts. Creating alternate service names would largely achieve the equivalent effect so this may be overkill
| // CA certificate to validate the client's certificate. It is the responsibility of the | ||
| // underlying platform to mount the secret as a file under /etc/istio/ingress-certs with | ||
| // the same name as the secret. | ||
| string client_secret = 3; |
There was a problem hiding this comment.
technically this is not a 'client secret' it's a list of CA certs.
Consider terminology from
https://envoyproxy.github.io/envoy/configuration/listeners/ssl.html
I don't think we want K8S 'secret' terminology bleeding in here
There was a problem hiding this comment.
How about using server_cert, client_ca_bundle ?
There was a problem hiding this comment.
Seems reasonable.
Note that we also have to account for whether we want to accept or reject non-mutual TLS as part of this config.
apiVersion: config.istio.io/v1alpha2
kind: IngressRule
metadata:
name: my-ingress
spec:
servers:
- port: 80
protocol: HTTP
ruleSelector:
- labels:
istio: ingress-http # only rules that have this label in metadata will be applied.
- port: 443
protocol: HTTP
secure: true #enables HTTPS on this port
ruleSelector:
- labels:
istio: ingress-tls
- port: 2379 #to expose internal service via external port 2379
protocol: TCP
backend:
name: myRedis #service name
port: 1234
secrets:
- host: foo.bar.com
serverSecret: server.crt
clientSecret: client.ca-bundle
- host: example.com
serverSecret: server.crt |
| // spec: | ||
| // servers: | ||
| // - port: 443 | ||
| // protocol: HTTP |
There was a problem hiding this comment.
Do we intend to control protocol advertisement via ALPN with these names as an option here?
There was a problem hiding this comment.
Oh good point!.. May be make this a list ? protocols: http,http2 ? Or have a separate ALPN field.
| // INTERNAL USE ONLY. Map from K8S ingress into a partial Istio rule | ||
| // which is then used to map into actual route rules. This is an incomplete | ||
| // implementation. Bare minimal Kubernetes ingress specifications are supported. | ||
| message K8SIngressRule { |
There was a problem hiding this comment.
Does this message need to continue to exist?
There was a problem hiding this comment.
You mean as artifact or whether we even need support for ingress translation ? @kyessenov created the first version of ingress rule message. I believe it was to keep the code base simple, by importing protos into proxy/* code, while platform specific code dealt with transformation.
There was a problem hiding this comment.
We should be able to remove it eventually, if we want. There isn't a need to translate the k8s ingress resource into our own K8SIngressRule, since we can just use the k8s ingress resource instead. But in the meantime, to leverage existing code, we can leave it in place.
There was a problem hiding this comment.
I think the problem with removing the K8s Ingress support is that most (almost all) users are coming from K8s, and will expect to be able to use existing Ingresses. Removing it may be a barrier to Istio adoption. If we leave it in, then I think we still need to fix all (most of?) the problems that it has in some reasonable way, which means the new rules-based design doesn't really get us much further ahead, other than to give us a better non-kube solution.
Unless we can improve it, I think this new reference-to-rules Ingress design is significantly more complicated. It splits a simple ingress config out into many rules and requires replication of config in default rules and higher precedence overrides (e.g. canary rule). I think this may also negatively impact adoption.
|
TLS handling stuff should largely be copied from
https://github.com/envoyproxy/data-plane-api/blob/ea20cc35d1e762c8f0f35d22a7fb6cca867673f0/api/sds.proto#L134
where a decent amount of effort went in to qualifying the requirements
…On Wed, Oct 11, 2017 at 7:56 PM, Shriram Rajagopalan < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In proxy/v1/config/ingress_rule.proto
<#198 (comment)>:
> -// ingress proxes serve as the receiving edge proxy for the entire mesh, but
-// can also be addressed from inside the mesh. Each ingress rule defines a
-// destination service and port. Rules that do not resolve to a service or a
-// port in the mesh should be ignored.
-//
-// The routing rules for the destination service are applied at the ingress
-// proxy. That means the routing rule match conditions are composed and its
-// actions are enforced. The traffic splitting for the destination service is
-// also effective.
-//
-// WARNING: This API is experimental and under active development
-message IngressRule {
+// INTERNAL USE ONLY. Map from K8S ingress into a partial Istio rule
+// which is then used to map into actual route rules. This is an incomplete
+// implementation. Bare minimal Kubernetes ingress specifications are supported.
+message K8SIngressRule {
You mean as artifact or whether we even need support for ingress
translation ? @kyessenov <https://github.com/kyessenov> created the first
version of ingress rule message. I believe it was to keep the code base
simple, by importing protos into proxy/* code, while platform specific code
dealt with transformation.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#198 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIoKPHVBiK9756gTYJcw51QheV7Usw-Fks5srX_1gaJpZM4P2Xse>
.
|
| // protocol: HTTP | ||
| // ruleSelector: | ||
| // - labels: | ||
| // istio: ingress-http # only rules that have this label in metadata will be applied. |
There was a problem hiding this comment.
I assume there will be rules with this label for multiple destinations. How is the destination service decided for calls via the ingress? Seems to be missing some kind of path-to-destination or host-to-destination mapping that a K8s ingress provides.
There was a problem hiding this comment.
That is in the route rule. If a user wants to match on specific destination, then she provides host header match. If there is no host header match, then we end up getting 404s? Man, I totally missed this piece.
There was a problem hiding this comment.
How does precedence come into play? For rules with the same destination, I assume it's just the standard algorithm (first match). But, what if there are multiple rules with the same host header (or path/uri) match, but with different destination services. Which one(s) apply?
| // INTERNAL USE ONLY. Map from K8S ingress into a partial Istio rule | ||
| // which is then used to map into actual route rules. This is an incomplete | ||
| // implementation. Bare minimal Kubernetes ingress specifications are supported. | ||
| message K8SIngressRule { |
There was a problem hiding this comment.
We should be able to remove it eventually, if we want. There isn't a need to translate the k8s ingress resource into our own K8SIngressRule, since we can just use the k8s ingress resource instead. But in the meantime, to leverage existing code, we can leave it in place.
| // istio: ingress-http # only rules that have this label in metadata will be applied. | ||
| // - port: 443 | ||
| // protocol: HTTP | ||
| // secure: true #enables HTTPS on this port |
There was a problem hiding this comment.
Why not protocol: HTTPS here?
This is a follow up from #192 .. This rule is basically a trimmed version of the internal ingress rule that we were carrying. The idea is that we would expose this as a first class resource, rather than mucking with automatic conversions.
Most of the code also exists.
The benefit of this model is that we can now deploy ingress envoys in non kubernetes environments and also as a standalone LB in kubernetes. Currently, we are using kubectl to deploy ingress spec in nomad, and then using istioctl route rules. Which is clunky. We are also doing implicit route rule to ingress mappings by comparing ingress path/prefix to route rule path/prefix, and picking the most restrictive one. This model has suffered a series of flaws including inability to match regex, or mix/match regex/prefix rules, users requesting support for nginx specific annotations, and the list goes on.
This is not meant to replace Kubernetes Ingress. If user provides K8S ingress, we will pick that and function as usual. But if she chooses to use Istio ingress (enabled through a global mesh config), then we will no longer watch the ingress resources (but we will run as ingress controller). This allows both to co-exist .