-
Notifications
You must be signed in to change notification settings - Fork 558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Doc: Add note on DR workloadSelector #3088
base: master
Are you sure you want to change the base?
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. |
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. 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 |
@NaturezzZ: Cannot trigger testing until a trusted user reviews the PR and leaves an In 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 |
A /release-notes-none label is required. I don't think this PR needs a release note. |
/test release-notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -168,6 +168,12 @@ import "type/v1beta1/selector.proto"; | |||
// The following example shows how a destination rule can be applied to a | |||
// specific workload using the workloadSelector configuration. | |||
// | |||
// **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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to compare with SE here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subsets are used to control which servers the traffic is routed to. I'll improve the wording here to eliminate ambiguity.
@@ -168,6 +168,12 @@ import "type/v1beta1/selector.proto"; | |||
// The following example shows how a destination rule can be applied to a | |||
// specific workload using the workloadSelector configuration. | |||
// | |||
// **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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/istio/istio/blob/388d5d1b9f37e7c3d15cf72967fad22f0b8e70fb/pilot/pkg/networking/core/v1alpha3/cluster_builder.go#L380 - SidecarScope.DestinationRule does control the server workloads?
Sorry for the late reply. I revised some wording to eliminate ambiguity. Would you mind take a look? Thank you. : ) |
PR needs rebase. 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-sigs/prow repository. |
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.