Fixes #11818. Select Sidecars by Workload/Pod labels instead of Servi… by robertpanzer · Pull Request #12620 · istio/istio

robertpanzer · 2019-03-20T10:31:34Z

…ce labels

This PR is an attempt to fix #11818.

I don't know if I missed some parts, but however I am trying:)

The problem reported in the issue is that the workload selectors of the Sidecar instances match against the labels of the Services which in turn lead to the proxies, but that doesn't work for consumer only services.

This PR changes this by getting the workload labels from the DiscoveryServer for the current proxy and uses these labels as the basis for the selection of the Sidecar instance.

There's just a few open questions:

The member holding the workload labels is called WorkloadsByID, suggesting that its keys would be the IDs of the proxies. Instead the only place I could find where this map is filled is the kube serviceregistry which feeds the pod ip as the id:

istio/pilot/pkg/serviceregistry/kube/pod.go

Line 86 in 190d4c1

pc.c.XDSUpdater.WorkloadUpdate(ip, pod.ObjectMeta.Labels, pod.ObjectMeta.Annotations)
I didn't find any specific tests that would cover Sidecars right now, and most of what I ran locally succeeded despite the change. Running the tests locally isn't easy for me, so let's see what the CI says. Local manual testing worked for me and also picked up dynamic changes to Sidecars and Pod labels (I know that doesn't count much without automated tests :P)
This is a breaking change for existing users, is it still possible to fix this issue on 1.1.x assuming that Istio somehow follows semver?

googlebot · 2019-03-20T10:31:37Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

istio-testing · 2019-03-20T10:31:50Z

Hi @robertpanzer. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

golangcibot · 2019-03-20T10:34:03Z

pilot/pkg/proxy/envoy/v2/ads.go

File is not goimports-ed (from goimports)

…Service labels

googlebot · 2019-03-20T10:39:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

robertpanzer · 2019-03-20T11:37:00Z

I also checked using Proxy.Metadata as the source for the workload labels.
Unfortunately these don't seem to be dynamic and react on label updates made with kubectl.

hzxuzhonghu · 2019-03-20T12:03:32Z

/ok-to-test

robertpanzer · 2019-03-20T14:22:11Z

I wonder though how sidecars that don't run inside a K8s cluster could be supported.
My guess is that such applications have to be registered in another Service Registry like Consul.
However the Consul Registry doesn't seem to feed the WorkloadsById.
Maybe that needs to be added as well.

howardjohn · 2019-03-20T14:41:22Z

Thanks for picking this up, looks pretty good. I believe the failing test are known flakes.

I'll pull this down and test it out a bit as well

howardjohn · 2019-03-20T16:29:48Z

pilot/pkg/proxy/envoy/v2/ads.go


+func (s *DiscoveryServer) workloadLabels(proxy *model.Proxy) model.Labels {
+	var workloadLabels model.Labels
+	if workload := s.WorkloadsByID[proxy.IPAddresses[0]]; workload != nil {


I think getting IPAdresses[0] is safe but not 100% sure if someone could verify

That's a good point.
I found code that also simply accesses Proxy.IPAddresses[0], but now found that this in the kube registry, where it makes sense to make this assumption.
I am going to change this, even if this removes the precious lgtm label 😢

howardjohn · 2019-03-20T16:44:36Z

/lgtm

I think we can get this in for 1.1.x but up to @duderino / others

…t ip addresses

howardjohn · 2019-03-20T17:27:35Z

pilot/pkg/proxy/envoy/v2/ads.go

+func (s *DiscoveryServer) workloadLabels(proxy *model.Proxy) model.Labels {
+	var workloadLabels model.Labels
+	ipAddresses := proxy.IPAddresses
+	if len(ipAddresses) == 0 {


My thoughts were more if we have multiple ipAddress rather than none - I haven't had a chance to dig into how the addresses get set though so I am not sure what happens outside of k8s. @rshriram any ideas? I don't think its a major issue but I am only familiar with kubernetes

I see.
Currently afaict only the kube registry feeds workloadLabels with the podIP.
However there could probably be more than 1 address in Proxy.IPAdresses and assuming that this PodIP is always in the first slot might be brittle.
Maybe it would make sense then to make workloadLabels return a LabelsCollection and require the workloadSelector to be a superset of every item in that collection.

What do you think?

I think looking at all of them makes sense, the question I would have is do we need the workloadSelector to match all or any of the labels we get. I suspect we would want to do all. I think this is basically how it worked before right?

Yes, that's how it worked before, just using the services labels.
I think it also makes sense to match against all Labels, with the current code I would assume that even with different IPAddresses only one set should be non-empty.
Thanks for the discussion! 👍

I just upgraded the PR to get the labels associated with all IP addresses.
The selector will match if it is a subset of any set of labels associated with a workloadId.
This is basically the same behavior as before, just that it tests against the labels of the Pods instead of the Services.

cc @zhaohuabing any thoughts since you added the multi-ip support for each proxy, for consul environments

I started working on an alternative fix today that would add a method ServiceDiscovery.GetProxyWorkloadLabels() which returns the workload labels for a proxy.
Maybe this works better for proxies not running in Kubernetes.
The change set will certainly be much, much larger.

howardjohn · 2019-03-20T20:36:47Z

/lgtm thanks for working on this!

istio-testing · 2019-03-20T20:57:30Z

@robertpanzer: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-integ-k8s-tests.sh	`416b7fd`	link	`/test istio-integ-k8s-tests`
prow/istio-pilot-multicluster-e2e.sh	`416b7fd`	link	`/test istio-pilot-multicluster-e2e`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

hzxuzhonghu · 2019-03-21T01:41:38Z

/lgtm

@rshriram WDYT

hzxuzhonghu · 2019-03-21T02:12:39Z

Sorry after another thought, this is not right.

/lgtm cancel

istio-testing · 2019-03-21T02:12:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: howardjohn, robertpanzer
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: hzxuzhonghu

If they are not already assigned, you can assign the PR to them by writing /assign @hzxuzhonghu in a comment when ready.

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:

pilot/OWNERS

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

hzxuzhonghu · 2019-03-21T02:13:25Z

pilot/pkg/proxy/envoy/v2/ads.go

+	ipAddresses := proxy.IPAddresses
+	labels := make([]model.Labels, 0, len(ipAddresses))
+	for _, ip := range ipAddresses {
+		if workload := s.WorkloadsByID[ip]; workload != nil {


WorkloadsByID only set in k8s env

That was one of my questions: How would proxies identify for non-K8s applications?
Is it required to register MESH_INTERNAL ServiceEntries for these?
Or use Consul?

Proxy.Metadata and the current approach using Proxy.ServiceInstances don't work, as the metadata is fix and doesn't follow Pod labels, and ServiceInstances doesn't exist for consumer only applications.
Consumer only non-K8s applications should suffer from the same problem.

WorkloadsByID is only used by incremental EDS feature.

Which only supported in k8s

I understand that.
Maybe it would be possible to store the labels in another location at the DiscoveryServer where other Registries can store and update the labels too.
That shouldn't be too hard actually and could probably also use the IPAddress as key, the Consul Registry already uses this for matching a Proxy to a ServiceInstance.

Maybe another approach could be to add a method GetProxyWorkloadLabels to ServiceRegistry.
On Kubernetes this could still get dynamic updates.
I can have a try for this approach today.

rshriram · 2019-03-21T14:56:37Z

I am reluctant to do a partial fix like this.. while you might be able to get the Sidecar logic flowing, other plugins might fail if they encounter a workload that has no service instances (notably authN and such). A simple litmus test is to try to add a test under lds_test.go (there are tests for sidecar, follow the same logic, but create a pseudo workload without any service). If my hunch is right, you will find that a couple of plugins will fail and seg fault.

Alternative fix is to create a dummy internal service for workloads that do not have a backing service, in the getProxyServiceInstances call. that might solve your problem. Once again, I would validate with the tests in lds_test.go at the very least to see what else might fail.

robertpanzer · 2019-03-21T15:11:21Z

Thanks for the pointer where to add tests, I was already searching for that!
I only wonder why these segfaults should occur there given that my manual tests with this PR already worked for a K8s Deployment without a K8s Service.
Actually, wouldn't that mean that even right now with 1.1.0 such applications should make Pilot trap?
I don't really see the difference in taking the reference labels just from another location.

I see the point though that it's problematic for proxies corresponding to services from other ServiceRegistries, e.g. Consul, that they won't be able to find any workload labels anymore.
This is additionally made worse by the fact that all services from Consul result in ServiceInstances belonging to the default namespace, so that it's not even possible to apply different Sidecars by namespace.

Adding a fake ServiceEntry doesn't sound like a good idea to me.
There's probably a lot of places where these have to be sorted out again like the health checks that were mentioned above.

robertpanzer · 2019-03-21T16:45:28Z

Created an alternative PR to fix the problem at #12666.
Maybe this finds more acceptance.

robertpanzer · 2019-03-22T14:58:33Z

Closing this in favor of #12666

istio-testing requested review from GregHanson and rshriram March 20, 2019 10:31

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Mar 20, 2019

istio-testing added the needs-ok-to-test label Mar 20, 2019

golangcibot reviewed Mar 20, 2019

View reviewed changes

pilot/pkg/proxy/envoy/v2/ads.go Outdated

Copy link
Copy Markdown

Contributor

golangcibot Mar 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File is not goimports-ed (from goimports)

robertpanzer force-pushed the fix-11818-sidecar-selector branch from 76edc92 to 63374d5 Compare March 20, 2019 10:36

Fixes istio#11818. Select Sidecars by Workload/Pod labels instead of …

7187d8f

…Service labels

robertpanzer force-pushed the fix-11818-sidecar-selector branch from 63374d5 to 7187d8f Compare March 20, 2019 10:39

googlebot added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Mar 20, 2019

robertpanzer mentioned this pull request Mar 20, 2019

Sidecar resource with workloadSelector only match Services #11818

Closed

robertpanzer force-pushed the fix-11818-sidecar-selector branch from 8eb7a1a to 7187d8f Compare March 20, 2019 11:29

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Mar 20, 2019

howardjohn reviewed Mar 20, 2019

View reviewed changes

istio-testing assigned howardjohn Mar 20, 2019

istio-testing added the lgtm label Mar 20, 2019

Avoid out of bounds access getting workload labels for proxies withou…

aa37d31

…t ip addresses

istio-testing removed the lgtm label Mar 20, 2019

howardjohn reviewed Mar 20, 2019

View reviewed changes

Handle workload labels associated with all Proxy IPAddresses

416b7fd

istio-testing assigned hzxuzhonghu Mar 21, 2019

istio-testing added lgtm labels Mar 21, 2019

istio-testing removed the lgtm label Mar 21, 2019

istio-testing removed the approved label Mar 21, 2019

hzxuzhonghu reviewed Mar 21, 2019

View reviewed changes

robertpanzer mentioned this pull request Mar 21, 2019

Fix #11818 fix workloadSelector for Sidecars #12666

Merged

robertpanzer closed this Mar 22, 2019

Conversation

robertpanzer commented Mar 20, 2019

Uh oh!

googlebot commented Mar 20, 2019

Uh oh!

istio-testing commented Mar 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 20, 2019

Uh oh!

robertpanzer commented Mar 20, 2019

Uh oh!

hzxuzhonghu commented Mar 20, 2019

Uh oh!

robertpanzer commented Mar 20, 2019

Uh oh!

howardjohn commented Mar 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Mar 20, 2019

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!

howardjohn commented Mar 20, 2019

Uh oh!

istio-testing commented Mar 20, 2019

Uh oh!

hzxuzhonghu commented Mar 21, 2019

Uh oh!

hzxuzhonghu commented Mar 21, 2019

Uh oh!

istio-testing commented Mar 21, 2019

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!

rshriram commented Mar 21, 2019

Uh oh!

robertpanzer commented Mar 21, 2019

Uh oh!

robertpanzer commented Mar 21, 2019

Uh oh!

robertpanzer commented Mar 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants