Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NaturezzZ
Copy link

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.

@NaturezzZ NaturezzZ requested a review from a team as a code owner February 15, 2024 07:53
@istio-policy-bot
Copy link

😊 Welcome @NaturezzZ! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test labels Feb 15, 2024
@istio-testing
Copy link
Collaborator

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 /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.

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.

@NaturezzZ
Copy link
Author

/ok-to-test

@istio-testing
Copy link
Collaborator

@NaturezzZ: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

@zirain
Copy link
Member

zirain commented Feb 15, 2024

/ok-to-test

@istio-testing 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 Feb 15, 2024
@NaturezzZ
Copy link
Author

NaturezzZ commented Feb 15, 2024

A /release-notes-none label is required. I don't think this PR needs a release note.

@kfaseela kfaseela added the release-notes-none Indicates a PR that does not require release notes. label Feb 15, 2024
@kfaseela
Copy link
Member

/test release-notes

Copy link

@ilrudie ilrudie left a 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.

networking/v1alpha3/destination_rule.proto Outdated Show resolved Hide resolved
networking/v1alpha3/destination_rule.proto Outdated Show resolved Hide resolved
networking/v1alpha3/destination_rule.proto Outdated Show resolved Hide resolved
networking/v1alpha3/destination_rule.proto Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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

Copy link
Member

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

Copy link
Author

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.

@NaturezzZ
Copy link
Author

Thank you @hzxuzhonghu, I removed the comparison with SE and clarified use case of workloadSelector and subset instead.

Comment on lines 174 to 175
// [Subsets](https://istio.io/latest/docs/reference/config/networking/destination-rule/#Subset)
// can be used to select server workloads in fine granularity.
Copy link
Member

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.

Copy link
Author

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.
Copy link
Member

Choose a reason for hiding this comment

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

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 6, 2024
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 7, 2024
@NaturezzZ
Copy link
Author

Sorry for the late reply. I revised some wording to eliminate ambiguity. Would you mind take a look? Thank you. : )

@NaturezzZ NaturezzZ requested a review from ilrudie March 7, 2024 05:08
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 20, 2024
@istio-testing
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR needs to be rebased before being merged ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. release-notes-none Indicates a PR that does not require release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants