WorkloadSelector support for DestinationRule #2207

kfaseela · 2022-01-14T14:56:04Z

Adding support for workloadSelector in DR
as per RFC Simplify Sidecar Egress For MTLS

Signed-off-by: Faseela K faseela.k@est.tech

istio-policy-bot · 2022-01-14T14:56:06Z

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

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

kfaseela · 2022-01-14T15:07:06Z

/test release-notes_api

howardjohn

I am +1 to the idea

howardjohn · 2022-01-14T16:00:03Z

networking/v1alpha3/destination_rule.proto

+  // Criteria used to select the specific set of pods/VMs on which this
+  // `Sidecar` configuration should be applied. If omitted, the `Sidecar`
+  // configuration will be applied to all workload instances in the same namespace.
+  WorkloadSelector workload_selector = 5;


I think we should use istio.type.v1beta1.WorkloadSelector. Through unfortunate situation we ended up with two different ones to handle the same case, but all modern CRDs are using that type.

Also the comments are copy+pasted from Sidecar, need to be updated.

bianpengyuan · 2022-01-14T18:11:10Z

Curious, how does this interact with subset? Is it that the unmatched proxy won't have subset configured? If so should we also have workload selector at virtual service to make it consistent. There are definitely demand for this istio/istio#36240 and istio/istio#29434

howardjohn · 2022-01-14T18:18:48Z

VirtualService already basically has workload selector with sourceLabels? I *think* its functionally equivalent

…

On Fri, Jan 14, 2022 at 10:11 AM Pengyuan Bian ***@***.***> wrote: Curious, how does this interact with subset? Is it that the unmatched proxy won't have subset configured? If so should we also have workload selector at virtual service to make it consistent. There are definitely demand for this istio/istio#36240 <istio/istio#36240> and istio/istio#29434 <istio/istio#29434> — Reply to this email directly, view it on GitHub <#2207 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXODKSNVKYXKOEINQSTUWBRMVANCNFSM5L7AGQRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because your review was requested.Message ID: ***@***.***>

hzxuzhonghu · 2022-01-17T01:45:48Z

First, i wanna say i am totally for this proposal.
But i think this will break backward compatibility. Before a DR without wokloadSelector, it would be applied to all namespaces.

But with this, without workloadSelector set, it will be applied to the same namespace

linsun · 2022-01-19T04:11:21Z

This is an interesting concept. Could we think through what impact it will have for DR priority ordering: client side, server side, namespace, global.

BTW, is this workload selector for client side or server side?

kfaseela · 2022-01-19T15:26:43Z

First, i wanna say i am totally for this proposal. But i think this will break backward compatibility. Before a DR without wokloadSelector, it would be applied to all namespaces.

But with this, without workloadSelector set, it will be applied to the same namespace

I think the confusion came due to my wrong description of the attribute that it will be namespace specific. will correct it

kfaseela · 2022-01-19T15:31:42Z

This is an interesting concept. Could we think through what impact it will have for DR priority ordering: client side, server side, namespace, global.

BTW, is this workload selector for client side or server side?

workSelector by default is global, and we can use "exportTo" to limit namespace list.
DR is for both client and server, and we want to limit client side in this PR.

DR will impact server side in TLS case, TLS mode in DR can be changed to override global TLS mode (e.g.STRICT), from this perspective, we can limit the "workSelector" to client side only, and keep server side behaviour as before.

howardjohn · 2022-01-19T15:34:35Z

workSelector by default is global, and we can use "exportTo" to limit namespace list.

I disagree, if workloadSelector is present it should only match current namespace. This matches all of our resources (except Gateway, and its been replaced by another mechanism in gateway-api and was a mistake IMO).

DR will impact server side in TLS case, TLS mode in DR can be changed to override global TLS mode (e.g.STRICT), from this perspective, we can limit the "workSelector" to client side only, and keep server side behaviour as before.

DR only impacts server for circuit breakers, not TLS. If we want similar for this behavior we should do #1754 imo

bianpengyuan · 2022-01-20T00:02:06Z

DR will impact server side in TLS case, TLS mode in DR can be changed to override global TLS mode (e.g.STRICT), from this perspective, we can limit the "workSelector" to client side only, and keep server side behaviour as before.

istio/istio#36731 (comment) as an example of why it would also benefit server side.

howardjohn · 2022-01-20T00:10:34Z

istio/istio#36731 (comment) as an example of why it would also benefit server side.

I think its still somewhat ambiguous and #1754 would be preferred. For example, say I have a DR for httpbin.default.svc.cluster.local and selector app=httpbin. We would apply config on inbound and outbound side - use may have actually only wanted it on one or the other. Sidecar makes it very explicit

bianpengyuan · 2022-01-20T00:34:32Z

I think its still somewhat ambiguous and #1754 would be preferred

Yeah I agree. Adding selector to DR is more like a workaround for the inbound configuration issue, which might complicate the configuration or not even cover some of the use cases.

As per https://github.com/istio/api#updating `make proto-lock` has to be run as part of new api changes. I was trying to run the same for my PR istio#2207 and saw some additional changes added in proto.lock which was not introduced by my change. Hence putting them in a separate PR here to check if these changes are needed or not. Signed-off-by: Faseela K <faseela.k@est.tech>

As per https://github.com/istio/api#updating `make proto-lock` has to be run as part of new api changes. I was trying to run the same for my PR #2207 and saw some additional changes added in proto.lock which was not introduced by my change. Hence putting them in a separate PR here to check if these changes are needed or not. Signed-off-by: Faseela K <faseela.k@est.tech>

howardjohn

Code LGTM, just needs some docs improvements

howardjohn · 2022-03-17T22:09:49Z

networking/v1alpha3/destination_rule.proto

@@ -162,6 +163,28 @@ import "gogoproto/gogo.proto";
 //         simple: ROUND_ROBIN
 // ```
 // {{</tab>}}
+//
+// {{<tab name="v1alpha3" category-value="v1alpha3">}}


This is adding a third tab to a tab group of {v1alpha3,v1beta1}.

If we want to provide and example with workloadSelector, we should look like:

You can also use a workload selector .. blah blah... {{<tabset category-name="selector-example">}} {{tab }} ....

Not sure, if I fixed it properly. Could you please take another look now?

Looks great

howardjohn · 2022-03-17T22:11:07Z

networking/v1alpha3/destination_rule.proto

+  //
+  // Criteria used to select the specific set of pods/VMs on which this
+  // `DestinationRule` configuration should be applied. If omitted, the `DestinationRule`
+  // configuration will be applied to all workload instances in the same namespace.


"Same namespace" is not require accurate, since it can apply cross namespace,

We should also note that with workload selector set it is NOT cross namespace.

It should be clearly documented, whether this is selecting cross namespaces or within the same namespace.

Also move this field up to first position.

I have tried to rephrase the comment.

howardjohn · 2022-03-18T15:21:05Z

networking/v1alpha3/destination_rule.proto

@@ -162,6 +163,28 @@ import "gogoproto/gogo.proto";
 //         simple: ROUND_ROBIN
 // ```
 // {{</tab>}}
+//
+// {{<tab name="v1alpha3" category-value="v1alpha3">}}


Looks great

Adding support for workloadSelector in DR as per RFC Simplify Sidecar Egress For MTLS Signed-off-by: Faseela K <faseela.k@est.tech>

Signed-off-by: Faseela K <faseela.k@est.tech>

kfaseela · 2022-03-18T21:20:13Z

/retest

ramaraochavali · 2022-03-19T07:29:01Z

networking/v1alpha3/destination_rule.proto

+  // configuration will be applied only to the workload instances matching the workload selector
+  // label in the same namespace. Workload selectors do not apply across namespace boundaries.
+  // If omitted, the `DestinationRule` falls back to its default behavior.
+  istio.type.v1beta1.WorkloadSelector workload_selector = 5;


May be we should explain when this will be used vs. default behaviour? That is not clear to me while reading this comment.

@howardjohn WDYT?

Clarified in the comment

Signed-off-by: Faseela K <faseela.k@est.tech>

ramaraochavali

Thank you

kfaseela · 2022-03-19T08:07:28Z

Thank you!

hzxuzhonghu · 2022-03-21T02:02:17Z

networking/v1alpha3/destination_rule.proto

+  // Criteria used to select the specific set of pods/VMs on which this
+  // `DestinationRule` configuration should be applied. If specified, the `DestinationRule`
+  // configuration will be applied only to the workload instances matching the workload selector
+  // label in the same namespace. Workload selectors do not apply across namespace boundaries.


I donot think this is consistent with other API @howardjohn

Not a good idea to be inconsistent

Can you explain what about it is inconsistent? My expectation is that this behaves the same way as every other Istio API that has a workload selector?

Workload selectors do not apply across namespace boundaries.

For gateway, the selector works across namespaces.

Another point it from implement view: currently dr is used both fro outbound and inbound setting. Then if this only applies to the same namespace, does that mean we need to create two DRs for a service if the caller resides in different namespace as server?

@hzxuzhonghu : implementation can be seen at : istio/istio#37174

I donot see how to deal with the inconsistency

it is different from not only gateway. but also authorization policy

If the authorization policy is in the root namespace, the selector // will additionally match with workloads in all namespaces.

But not EnvoyFilter, Sidecar, Telemetry, ProxyConfig, WasmPlugin, PeerAuthentication, or Request Authentication.

Gateway was a mistake IMO - we do not do this cross namespace in the gateway-api and we have a flag to disable cross namespace for Istio gateway.

AuthzPolicy is the odd one out, not DR, IMO.

It is very confusing and error-prone. What's the long-term plan? After upgrading API version, we should remove all this bad inconsistency

There are two inconsistencies - Gateway and AuthzPolicy.

Gateway is mostly addressed as noted. The new API does not have these issues and the current one has a flag.

AuthzPolicy - we have started looking into changing it but there has been some push back https://docs.google.com/document/u/0/d/1sCA6ReTnAR5tyKwuc7KPMygv2UPOlxL6IEU8Og0nQm0/mobilebasic.

So we have a plan. In the mean time, this change is consistent with 7 other Istio APIs - including the 3 most recently added ones. It seems inconsistent and unfair to block this change for inconsistency when factoring this in.

What do you think? Is it ok to move forward with this? If not, do you have alternative ideas?

I am highly motivated to move forward with this as we are stretching @kfaseela 's - a new contributor who could become a major regular contributor - motivation very thin after months of work to put a hold on the last minute.

ramaraochavali · 2022-03-21T11:30:23Z

But i think this will break backward compatibility. Before a DR without wokloadSelector, it would be applied to all namespaces.

But with this, without workloadSelector set, it will be applied to the same namespace

@kfaseela Is this correct? I thought there is no change in behaviour for the case of DR with no workload selector?

kfaseela · 2022-03-21T12:07:00Z

But i think this will break backward compatibility. Before a DR without wokloadSelector, it would be applied to all namespaces.

But with this, without workloadSelector set, it will be applied to the same namespace

@kfaseela Is this correct? I thought there is no change in behaviour for the case of DR with no workload selector?

This is not correct. The namespace restriction is there only for workloadSelector set cases.

pkgulati · 2022-12-26T10:34:10Z

How to achieve workloadSelector for incoming traffic. I want to apply max connections limit using Destination Rule only on SERVER side of the service using istio proxy (side car).

kfaseela · 2023-01-09T08:23:00Z

@pkgulati : This was discussed under: istio/istio#41235

kfaseela requested review from ericvn, howardjohn, linsun, louiscryan, nrjpoddar and smawson as code owners January 14, 2022 14:56

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 14, 2022

kfaseela force-pushed the egress-mtls-sidecar branch from a5f27c7 to 0f4c64d Compare January 14, 2022 15:04

howardjohn reviewed Jan 14, 2022

View reviewed changes

bianpengyuan mentioned this pull request Jan 19, 2022

circuit_breakers.thresholds cannot be set on cluster using EnvoyFilter istio/istio#36731

Closed

kfaseela force-pushed the egress-mtls-sidecar branch from 6e07f6c to bf91ad8 Compare January 27, 2022 09:31

kfaseela force-pushed the egress-mtls-sidecar branch from bf91ad8 to 7ec7bdd Compare February 3, 2022 14:57

bianpengyuan mentioned this pull request Feb 4, 2022

Support terminating HTTPS on Sidecar's IstioEgressListener istio/istio#37160

Closed

This was referenced Feb 4, 2022

Running proto-commit #2230

Merged

workloadSelector support in DR istio/istio#37174

Merged

kfaseela changed the title ~~WIP: WorkloadSelector support for DestinationRule~~ WorkloadSelector support for DestinationRule Mar 10, 2022

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. labels Mar 10, 2022

kfaseela force-pushed the egress-mtls-sidecar branch from 95e82ca to b95db44 Compare March 17, 2022 08:25

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 17, 2022

howardjohn reviewed Mar 17, 2022

View reviewed changes

kfaseela force-pushed the egress-mtls-sidecar branch from cb5009b to c69b6ea Compare March 18, 2022 10:04

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 18, 2022

howardjohn approved these changes Mar 18, 2022

View reviewed changes

kfaseela added 6 commits March 18, 2022 21:29

WIP: WorkloadSelector support for DestinationRule

0846081

Adding support for workloadSelector in DR as per RFC Simplify Sidecar Egress For MTLS Signed-off-by: Faseela K <faseela.k@est.tech>

Fix release-notes

84b1ad5

Signed-off-by: Faseela K <faseela.k@est.tech>

Use istio.type.v1beta1.WorkloadSelector

93860c9

Signed-off-by: Faseela K <faseela.k@est.tech>

run proto-commit

b3810f2

Signed-off-by: Faseela K <faseela.k@est.tech>

Rebase

d3dd28b

Signed-off-by: Faseela K <faseela.k@est.tech>

incorporate review comments and rebase

bc7ed01

Signed-off-by: Faseela K <faseela.k@est.tech>

kfaseela force-pushed the egress-mtls-sidecar branch from c69b6ea to bc7ed01 Compare March 18, 2022 21:13

howardjohn assigned ericvn Mar 18, 2022

ramaraochavali reviewed Mar 19, 2022

View reviewed changes

Additional review comments for usecase explanation

3314502

Signed-off-by: Faseela K <faseela.k@est.tech>

ramaraochavali approved these changes Mar 19, 2022

View reviewed changes

istio-testing merged commit 7fbbe6f into istio:master Mar 19, 2022

hzxuzhonghu reviewed Mar 21, 2022

View reviewed changes

kfaseela deleted the egress-mtls-sidecar branch August 5, 2022 08:39

WorkloadSelector support for DestinationRule #2207

WorkloadSelector support for DestinationRule #2207

Conversation

kfaseela commented Jan 14, 2022

istio-policy-bot commented Jan 14, 2022

kfaseela commented Jan 14, 2022

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bianpengyuan commented Jan 14, 2022

howardjohn commented Jan 14, 2022 via email

hzxuzhonghu commented Jan 17, 2022

linsun commented Jan 19, 2022

kfaseela commented Jan 19, 2022

kfaseela commented Jan 19, 2022

howardjohn commented Jan 19, 2022 • edited Loading

bianpengyuan commented Jan 20, 2022

howardjohn commented Jan 20, 2022

bianpengyuan commented Jan 20, 2022

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaseela commented Mar 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali left a comment

Choose a reason for hiding this comment

kfaseela commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Mar 21, 2022

kfaseela commented Mar 21, 2022

pkgulati commented Dec 26, 2022

kfaseela commented Jan 9, 2023

howardjohn commented Jan 19, 2022 •

edited

Loading

hzxuzhonghu Mar 21, 2022 •

edited

Loading