Sidecar config resource

[rshriram]

Based on discussions and past networking meetings around
https://docs.google.com/document/d/1jmH8N0OdyJb1W6hB6xdLZXzdGUKs6GknI5Ayb6wD430

Renamed and revamped ServiceDependency and made some gateway edits as well.

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

[costinm]

Few inline comments - can be handled in separate PRs ( I assume we'll iterate few times, and not all will be implemented in 1.1 - what we can't implement well enough will need to be hidden or documented ).

The bind to port is the trickiest, in particular since 'bind=false' option in envoy is deprecated and a sidecar config may apply to workloads with different modes.

[costinm]

In any case - the IP should not be in the selector. It is a clear major problem in the current implementation. Using stuff from the cert can't be used at selection time - since this info is not known except at connect. Let's remove this until we have a clear need for an expansion - and then we can figure out if it needs to replace selector or where to add the fields.

…

On Thu, Dec 27, 2018 at 3:58 PM Shriram Rajagopalan < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In networking/v1alpha3/gateway.proto <#742 (comment)>: > @@ -350,3 +359,23 @@ message Port { // Label assigned to the port. string name = 3; } + +// WorkloadSelector specifies the criteria used to determine if the Gateway +// or Sidecar resource can be applied to a given workload. If multiple +// conditions are specified, all conditions need to match in order for the +// workload to be selected. +message WorkloadSelector { See the doc. The selector should not be just labels. We need to be able to use stuff from X509 certs, or other stuff that proxies provide as a way of identification. So putting them in a container leaves room for future expansion (like adding subject name or some jwt stuff in the workloadSelector) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6gnNtCkRVHTEtyEcIybn5ny3ds-Lks5u9V68gaJpZM4ZjMbE> .

[costinm]

It would be atrocious to ask users to configure individual VMs by IP or know ahead of time how the configs will be deployed. And the naming 'bind to port' is very confusing for a user - the doc doesn't say anything that the port where it'll be bound is 15000 or whatever else we'll do at implementation level to capture traffic. I'm not saying it can't be implemented - just that it's a horrible model to introduce the option to not bind. It should be part of the node metadata - i.e. environment based - not something a user should configure in Sidecar.

…

On Thu, Dec 27, 2018 at 4:05 PM Shriram Rajagopalan < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In networking/v1alpha3/sidecar.proto <#742 (comment)>: > + // The ip:port or the unix domain socket to which the listener should be + // bound to. Format: tcp://x.x.x.x:yyyy or unix:///path/to/uds or + // ***@***.*** (Linux abstract namespace). To bind to any IP with + // specific port, use tcp://0.0.0.0:<port> + string bind_address = 2; + + // When the bind address is an IP:port, the bindToPort option dictates + // whether or not the sidecar should bind its listener socket to the + // specified port. Set bindToPort to false (default) if application + // traffic entering/leaving a pod/VM is captured automatically through + // iptables redirection and forwarded to the sidecar on a specific port + // (see proxyListenPort in the global MeshConfig). When not using + // iptables for traffic capture, set bindToPort to true to force the + // sidecar to bind to the specified port. Note that the binding might + // fail if the application workload is already bound to the same port. + bool bind_to_port = 3; This is simply determining if we will add this listener to the existing 15000 listener or create a standalone listener, irrespective of the interception mode (ip tables/tproxy). Its in fact very easy to implement and super useful as you can now define one or more listeners on a sidecar in pod/VM that are not directly related to a service port. Its especially useful in VM world because you can now choose to install listeners for a specific set of ports and allow the application to explicitly communicate with the sidecar over the specified port. By default its false, i.e. it gets added to the 15000 listener. Whether or not we use the listener-level filter or the deprecated bind_to_port in envoy, the net effect is the same. Remember that this is an array of servers/listeners. And once we implement workload selector, users can apply this to a specific workload (a VM in a namespace). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6uuPwYbs0DA8mp_kAWvYaktCg6BEks5u9WBegaJpZM4ZjMbE> .

[costinm]

In other words - you should be able to deploy a config, using normal labels, and then connect machines from different environments using different capture modes, including no iptables, without having to edit the configs. And how capture works can be very dependent on version, deployment, etc - it is possible the admin will install a CNI that captures using different network namespace, and some workloads will run in this mode while others will be on older versions of the node. This should be completely transparent to the user.

…

On Thu, Dec 27, 2018 at 4:14 PM Costin Manolache ***@***.***> wrote: In any case - the IP should not be in the selector. It is a clear major problem in the current implementation. Using stuff from the cert can't be used at selection time - since this info is not known except at connect. Let's remove this until we have a clear need for an expansion - and then we can figure out if it needs to replace selector or where to add the fields. On Thu, Dec 27, 2018 at 3:58 PM Shriram Rajagopalan < ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In networking/v1alpha3/gateway.proto > <#742 (comment)>: > > > @@ -350,3 +359,23 @@ message Port { > // Label assigned to the port. > string name = 3; > } > + > +// WorkloadSelector specifies the criteria used to determine if the Gateway > +// or Sidecar resource can be applied to a given workload. If multiple > +// conditions are specified, all conditions need to match in order for the > +// workload to be selected. > +message WorkloadSelector { > > See the doc. The selector should not be just labels. We need to be able > to use stuff from X509 certs, or other stuff that proxies provide as a way > of identification. So putting them in a container leaves room for future > expansion (like adding subject name or some jwt stuff in the > workloadSelector) > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#742 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAFI6gnNtCkRVHTEtyEcIybn5ny3ds-Lks5u9V68gaJpZM4ZjMbE> > . >

[costinm]

The doc doesn't seem to have any 'workloadSelector' - and I'm pretty sure such a thing was not discussed. Let's remove it until we have a clear doc and agreement.

…

On Thu, Dec 27, 2018 at 1:09 PM Shriram Rajagopalan < ***@***.***> wrote: Based on discussions and past networking meetings around https://docs.google.com/document/d/1jmH8N0OdyJb1W6hB6xdLZXzdGUKs6GknI5Ayb6wD430 Renamed and revamped ServiceDependency and made some gateway edits as well. Signed-off-by: Shriram Rajagopalan ***@***.*** ------------------------------ You can view, comment on, or merge this pull request online at: #742 Commit Summary - renaming - Sidecar - Merge branch 'release-1.1' of https://github.com/istio/api into sidecars File Changes - *M* mesh/v1alpha1/config.pb.go <https://github.com/istio/api/pull/742/files#diff-0> (437) - *M* mesh/v1alpha1/config.proto <https://github.com/istio/api/pull/742/files#diff-1> (33) - *M* mesh/v1alpha1/istio.mesh.v1alpha1.pb.html <https://github.com/istio/api/pull/742/files#diff-2> (82) - *M* networking/v1alpha3/destination_rule.pb.go <https://github.com/istio/api/pull/742/files#diff-3> (172) - *M* networking/v1alpha3/destination_rule.proto <https://github.com/istio/api/pull/742/files#diff-4> (2) - *M* networking/v1alpha3/gateway.pb.go <https://github.com/istio/api/pull/742/files#diff-5> (472) - *M* networking/v1alpha3/gateway.proto <https://github.com/istio/api/pull/742/files#diff-6> (57) - *M* networking/v1alpha3/istio.networking.v1alpha3.pb.html <https://github.com/istio/api/pull/742/files#diff-7> (476) - *D* networking/v1alpha3/service_dependency.pb.go <https://github.com/istio/api/pull/742/files#diff-8> (938) - *D* networking/v1alpha3/service_dependency.proto <https://github.com/istio/api/pull/742/files#diff-9> (168) - *M* networking/v1alpha3/service_entry.pb.go <https://github.com/istio/api/pull/742/files#diff-10> (68) - *M* networking/v1alpha3/service_entry.proto <https://github.com/istio/api/pull/742/files#diff-11> (2) - *A* networking/v1alpha3/sidecar.pb.go <https://github.com/istio/api/pull/742/files#diff-12> (949) - *A* networking/v1alpha3/sidecar.proto <https://github.com/istio/api/pull/742/files#diff-13> (180) - *M* networking/v1alpha3/virtual_service.pb.go <https://github.com/istio/api/pull/742/files#diff-14> (230) - *M* networking/v1alpha3/virtual_service.proto <https://github.com/istio/api/pull/742/files#diff-15> (2) - *M* proto.lock <https://github.com/istio/api/pull/742/files#diff-16> (197) - *M* python/istio_api/mesh/v1alpha1/config_pb2.py <https://github.com/istio/api/pull/742/files#diff-17> (109) - *M* python/istio_api/networking/v1alpha3/destination_rule_pb2.py <https://github.com/istio/api/pull/742/files#diff-18> (72) - *M* python/istio_api/networking/v1alpha3/gateway_pb2.py <https://github.com/istio/api/pull/742/files#diff-19> (133) - *D* python/istio_api/networking/v1alpha3/service_dependency_pb2.py <https://github.com/istio/api/pull/742/files#diff-20> (241) - *M* python/istio_api/networking/v1alpha3/service_entry_pb2.py <https://github.com/istio/api/pull/742/files#diff-21> (32) - *A* python/istio_api/networking/v1alpha3/sidecar_pb2.py <https://github.com/istio/api/pull/742/files#diff-22> (191) - *M* python/istio_api/networking/v1alpha3/virtual_service_pb2.py <https://github.com/istio/api/pull/742/files#diff-23> (140) Patch Links: - https://github.com/istio/api/pull/742.patch - https://github.com/istio/api/pull/742.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#742>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6k4YukdAa0P0j2RpKocBPTMaku1Zks5u9TcJgaJpZM4ZjMbE> .

[rshriram]

.

[rshriram]

workloadSelector in the doc is in first example

 selector:
    labels:
      app: bookstore

in terms of the workloadSelector, right now we only have labels, but I have created room for this to grow once we have the ability to identify the workload by other information (from certs/etc.).

[rshriram]

cc @louiscryan

[costinm]

Yes, this 'room to grow' is what I'm saying is not agreed on, we can't add APIs without a clear use case and knowing where we'll go. Label-based selection is what we had and I'm not aware of any plan to deprecate and replace this. We can't identify from certs except at connect time, it is not known from discovery - and even there the cert is just used to verify the info passed in labels and the namespace. IP is clearly a dead end.

…

On Fri, Dec 28, 2018 at 7:50 PM Shriram Rajagopalan < ***@***.***> wrote: workloadSelector in the doc is in first example selector: labels: app: bookstore in terms of the workloadSelector, right now we only have labels, but I have created room for this to grow once we have the ability to identify the workload by other information (from certs/etc.). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6lZ8pIdIEPAvBG9xe90YirdJ6pl6ks5u9uaKgaJpZM4ZjMbE> .

[costinm]

Please revert the selector -> workloadSelector to avoid confusions. The rest is fine. We can't just rename fields and change structure without very strong reasons, the API is supposed to be stable and backward compatible. I understand you 'deprecated' selector - but there is absolutely no reason to do this.

…

On Fri, Dec 28, 2018 at 9:31 PM Costin Manolache ***@***.***> wrote: Yes, this 'room to grow' is what I'm saying is not agreed on, we can't add APIs without a clear use case and knowing where we'll go. Label-based selection is what we had and I'm not aware of any plan to deprecate and replace this. We can't identify from certs except at connect time, it is not known from discovery - and even there the cert is just used to verify the info passed in labels and the namespace. IP is clearly a dead end. On Fri, Dec 28, 2018 at 7:50 PM Shriram Rajagopalan < ***@***.***> wrote: > workloadSelector in the doc is in first example > > selector: > labels: > app: bookstore > > in terms of the workloadSelector, right now we only have labels, but I > have created room for this to grow once we have the ability to identify the > workload by other information (from certs/etc.). > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#742 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAFI6lZ8pIdIEPAvBG9xe90YirdJ6pl6ks5u9uaKgaJpZM4ZjMbE> > . >

[hzxuzhonghu]

Looks very good after this round.

We can't just rename fields and change structure without very strong
reasons, the API is supposed to be stable
and backward compatible

Agree, unless one day selector can not satisfy users.

[rshriram]

We can't identify from certs except at connect time, it is not known from
discovery - and even there the cert is just used to verify the info passed
in labels and the namespace. IP is clearly a dead end.

What do you mean? The sidecar resource to use will be selected after the proxy connects (we search through the list of sidecar resources on the namespace that matches the proxy's labels and apply it - this is how we do it for the gateway). So, instead of searching through the proxy labels (which requires GetProxyServiceInstances call for the updated set of labels), we can search through the proxy's cert info such as subject names/domains/other metadata that people traditionally embed on the workload's cert. Something like this:

workloadSelector:
  x509:
       Organization: sales
       OrganizationalUnit: department1

and we can try to match this with the info in the x509 cert when trying to pick the right sidecar for the resource. The x509 infrastructure is common among large vm installations and several folks such as eBay use the x509 fields such as the above for their access control, etc. Treat these fields as the equivalent of a namespace or some such division. Come to think of it, the entire CF infrastructure has x509 certs for every CF app instance (pod equivalent) and they dont have the concept of labels.

Note that this is not added in the API yet.

[rshriram]

Also, deprecating the selector is the only way to transition people using Gateways to have per namespace label selector instead of "all namespace" mode that we currently have. Otherwise, it will be a big breaking change - take a look at all the example gateway configs in the mailing list. Most of them will break if we simply change the gateway selector to be namespace only.

[ZackButcher]

+1 on selectors other than labels - cert fields (or SPIFFE identities) are a clear use case for non-label selection. CIDR ranges are also a valuable selector (and supporting a single IP address falls out of supporting CIDR ranges).

[costinm]

Please start a doc on selectors other than labels - and how they'll work. No problem with that - I would love to see cert info used, but this is not the right way to introduce this change, sneaking it in an unrelated PR. We agreed to add Sidecar, and we agreed to deprecate Gateway selecting across namespaces. CIDR ranges are also interesting - but again it's a complex subject that needs to be properly reviewed. And we can introduce additional ways to identify workloads without deprecating 'selector' - they can be added in a backward compatible way. The doc on adding Sidecar has nothing on deprecating 'selector' - and the plan to use the rename as a convoluted way to drive the migration makes no sense, even if someone finds an obscure use case for Gateways still existing in namespaces that don't run Gateway workloads. It can be done by adding an extra field to indicate the migration ( since we still can't rev the API version ). For 1.1 - the story on selecting by IP on non-flat namespace is still not clear, and using the certs is even less clear ( fine for using the cert on connect - but selector is also used to identify workloads that are not directly connected ). My proposal was to start with using the cert to validate the labels, and also to use it for the data plane connections. I can see a case for stopping the use of selectors in Gateway - for example if the 1.2 operator takes care of starting the envoy gateway workloads based on the Gateway config itself.

…

On Wed, Jan 2, 2019 at 1:39 PM Zack Butcher ***@***.***> wrote: +1 on selectors other than labels - cert fields (or SPIFFE identities) are a clear use case for non-label selection. CIDR ranges are also a valuable selector (and supporting a single IP address falls out of supporting CIDR ranges). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6lLw9FJLDokVVerU5EkbAj5uSz-oks5u_ScngaJpZM4ZjMbE> .

[rshriram]

https://docs.google.com/document/d/15p1PcYIUE18q5dbWJoYY5teyNgnnjBg73597-Tw6dzY is the doc restating what I noted earlier on the extensibility

[frankbu]

What about backward compatibility with label istio: ingressgateway? Should that be a special case, or maybe istio-system namespace is a special namespace whose workloads are virtually present in every other namespace?

[costinm]

We'll need a 'migration' section - explaining that the behavior in 1.0 had a bug, and how to move away.
But it can be in the release notes or separate doc - the text in this PR is correct.

[frankbu]

How can a service be privately scoped? What happens if a service is private but the corresponding virtualservice is public?

[louiscryan]

Then the VirtualService would have to be defined in the same namespace as the private service

[costinm]

Still looking good but not perfect - we can address comments and improve it in separate PRs before the release. I suspect while implementing testing we'll find more improvements.

/lgtm

[costinm]

Should we use the same format for the 'bind' ?

Do we allow default_endpoint to be a non-local IP or a DNS entry ? If not, would be good to be explicit ( and why ).
If we do - an example.

[costinm]

Let's introduce workload_selector consistently, for all configs - it's confusing to have Sidecar use one style
and the rest use a different one, and the discussion on how to use non-label selection is still not resolved.

[costinm]

I see it is hidden - so maybe it's ok to leave it there if in 1.1 we just implement the default Sidecar.

[istio-testing]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, rshriram

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [costinm,rshriram]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment