istio / old_pilot_repo

Deprecated home of Istio's Pilot, now in istio/istio's pilot dir
Apache License 2.0
137 stars 91 forks source link

Distribute new protobuf representation of mixerclient configuration #1644

Closed ayj closed 6 years ago

ayj commented 6 years ago

What this PR does / why we need it:

1) Generate mixerclient protobuf code. This will likely be simplified (or go away) once we start storing generated artifacts as part of repo consolidation.

2) Distribute the new protobuf mixerclient configuration as part of the existing mixer filter configuration. The protobuf is encoded as a JSON map value under "v2" key in the filter config struct map alongside the legacy ad-hoc configuration. This is a backwards compatible change to allow proxies to incrementally adopt the new configuration. The legacy ad-hoc config is deprecated and will eventually be removed.

3) annotate inbound route's opaque_config with the destination.service of the server instance. Mixerclient can use this to look-up per-service quota/api in the filter configuration.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

NOTE #1): this depends on https://github.com/istio/api/pull/220 for corresponding protobuf changes to make gogoslick_proto_library happy.

NOTE #2): posted for early review. I'll rebase+merge after repo consolidation is finished.

Release note:

istio-testing commented 6 years ago

@ayj: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none". Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed. Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
codecov[bot] commented 6 years ago

Codecov Report

Merging #1644 into master will increase coverage by 0.04%. The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
+ Coverage   83.28%   83.33%   +0.04%     
==========================================
  Files          52       52              
  Lines        6546     6599      +53     
==========================================
+ Hits         5452     5499      +47     
- Misses        890      892       +2     
- Partials      204      208       +4
Impacted Files Coverage Δ
proxy/envoy/ingress.go 75.37% <100%> (ø) :arrow_up:
proxy/envoy/config.go 92.12% <100%> (-0.02%) :arrow_down:
proxy/envoy/mixer.go 90.29% <90.32%> (-1.55%) :arrow_down:

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 43f959c...bc17c64. Read the comment docs.

kyessenov commented 6 years ago

Merging prior to repo unification. @mandarjog please update ISTIO_API to head, the matching PR got merged.

kyessenov commented 6 years ago

A follow-up question to @ayj : would it make more sense to set control service key in the forwarded attributes instead of mixer attributes? The client can pass the normalized host name to the destination. I assume there's some logic in mixerfilter to normalize the host header at the moment?

ayj commented 6 years ago

Forwarded attributes aren't present on ingress. It also marginally increases the request size for something that could be derived on the local server-side proxy. Is there a functional advantage to including destination.service in the forwarded attributes?

Host header normalization introduces platform specific naming complexity to the mixerclient, e.g. mapping of name to name.namespace.svc.cluster.local requires mixerclient know the destination is a k8s service and which namespace its running in. This logic isn't present in mixerclient today. +@qiwzhang for additional comments.

kyessenov commented 6 years ago

We may want to preserve the address that the client used to address the server. Think of two services hosted on the same server. At TCP level, and potentially at HTTP level, all you have is IP address (hostnames are optional). I see that as a strong advantage of carrying the address from the client-side balancer to the server. Of course, we still need to do something when the client it outside of the mesh (e.g. ingress or non-istio pods).

ayj commented 6 years ago

In your example, do HTTP and TCP share the same address:port?

Forwarding attributes is a function of the mixerclient which, for now, means Envoy is making the client request. Don't we already assume Envoy includes the hostname? Otherwise our shared port HTTP services wouldn't work.

kyessenov commented 6 years ago

Right, client envoy requires using hostname. We still need to normalize the hostname, and that needs to be done at the client. For example, if a request sent from one namespace to another, and there are two identical services in the two namespaces, we cannot resolve hostname correctly in the destination namespace.

ayj commented 6 years ago

How does envoy know which service instance to route to in that case? My understanding is that the app would need to either use (a) namespace qualified service name or (b) service or endpoint IP address. Both cases should be recognized by server's virtual_host domain list.

kyessenov commented 6 years ago

It depends on the DNS context of the proxy. If the client and server are in different namespaces, then the hostname extension is different. The client can still use a short name. This is mostly a problem with multiple services sharing a pod.

ayj commented 6 years ago

Two functionally identical services in different namespaces would be different services for the purposes of service naming and distributing mixerclient/quota/apispec configuration. In this case, a client could use the short short name (i.e. no namespace qualification) and it be routed to the service in the same namespace which the server-side proxy would map to the correct FQ destination service name.

kyessenov commented 6 years ago

That's not necessarily the case. If my pod is reachable through names "a.ns1" and "a.ns2", and the client in "ns1" calls it through "a", then how does the server on the pod know which name was used? The server sees two options for services "a.ns1" and "a.ns2", and the host header "a". What is even worse is that if the request does not come from Istio, then host name is completely unreliable. It could be a random external IP if it's load balanced through some middle proxies.