Doc: Add note on DR workloadSelector#3088
Conversation
|
😊 Welcome @NaturezzZ! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
size/XS
|
Hi @NaturezzZ. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
|
@NaturezzZ: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
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. |
|
/ok-to-test |
istio-testing
added
ok-to-test
|
A /release-notes-none label is required. I don't think this PR needs a release note. |
kfaseela
added
the
release-notes-none
|
/test release-notes |
ilrudie
left a comment
ilrudie
left a comment
There was a problem hiding this comment.
I'm not mandating any of the suggested changes. The overall clarification you've written seems like a good one but I think we can workshop the wording a little bit to ensure the new note is clear and concise.
| // | ||
| // **Note:** The workloadSelector configuration in | ||
| // Destination Rules reveals which workloads the traffic emanating from | ||
| // uses this Destination Rule, unlike the workloadSelector configuration |
There was a problem hiding this comment.
why do we need to compare with SE here
There was a problem hiding this comment.
or why it is SE not other API that contains workloadSelector. IMO we donot have to do such explicit comparation
There was a problem hiding this comment.
removed and changed a better counter example in latest commit. thank you.
|
Thank you @hzxuzhonghu, I removed the comparison with SE and clarified use case of |
| // [Subsets](https://istio.io/latest/docs/reference/config/networking/destination-rule/#Subset) | ||
| // can be used to select server workloads in fine granularity. |
There was a problem hiding this comment.
I don't think this is accurate. Subsets control load balancing on the clinet-side, it doesn't impact which servers implemnent which policies.
https://github.com/istio/istio/blob/388d5d1b9f37e7c3d15cf72967fad22f0b8e70fb/pilot/pkg/networking/core/v1alpha3/cluster_builder.go#L380 - no subset involved.
There was a problem hiding this comment.
Subsets are used to control which servers the traffic is routed to. I'll improve the wording here to eliminate ambiguity.
| // | ||
| // **Note:** The workloadSelector configuration in | ||
| // destination rule controls which client workloads | ||
| // use this destination rule, rather than which server workloads do. |
There was a problem hiding this comment.
https://github.com/istio/istio/blob/388d5d1b9f37e7c3d15cf72967fad22f0b8e70fb/pilot/pkg/networking/core/v1alpha3/cluster_builder.go#L380 - SidecarScope.DestinationRule does control the server workloads?
istio-testing
added
the
needs-rebase
istio-testing
removed
the
needs-rebase
|
Sorry for the late reply. I revised some wording to eliminate ambiguity. Would you mind take a look? Thank you. : ) |
istio-testing
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
Phil1602
left a comment
Phil1602
left a comment
There was a problem hiding this comment.
Thanks for tackling this!
Maybe we can revive this PR - I linked an issue which basically describes this issue you are documenting.
| // **Note:** The workloadSelector configuration in | ||
| // destination rule controls which client workloads | ||
| // use this destination rule, rather than controlling | ||
| // which servers are selected as traffic endpoints. |
There was a problem hiding this comment.
I would argue vice-versa:
Most of the Istio resources (e.g. Sidecar, DestinationRule) use workloadSelector to target clients (==Envoys which apply/use this config).
ServiceEntry is IMO the only exception: It uses workloadSelector to select endpoints/targets for traffic flow.
Or did I miss something?
|
@hzxuzhonghu I just noticed that the issue istio/istio#55247 was closed due to inactivity. But this very PR here should be tackling the reported issue. Maybe they could be linked? @NaturezzZ May I also ask if you could update and rebase the PR based on the feedback and discussion here? |
|
@howardjohn sorry to bother you ... but I just ran into this issue again ... (just because the naming collision is so darn confusing when writing a a) in the documentation via this very PR |
10 participants
Supplementary explanation based on istio/istio#49111 (comment).
The same field name WorkloadSelector has a different effect/behavior in DR and SE. WorkloadSelector in ServiceEntry indicates destination workloads of traffic, while in DR workloadSelector reveals which workloads the traffic emanating from uses the DR.