feat: support management of multiple Istio VirtualService objects #1381

darshanhs09 · 2021-07-29T20:52:45Z

Currently, Argo Rollout spec only supports a single VirtualService (VS) in its Istio integrated canary.
Multiple VirtualService support is needed in Istio-based setups in which a service's routes are given in multiple VirtualService objects rather than just a single one.

Use cases:
This feature is required to enable canary/blue-green traffic shifting for services with multiple port delegate VirtualServices.
It's very common to have multiple VirtualService's pointing to a single backend service.

Signed-off-by: Darshan Hassan Shashikumar [email protected]

Note: Users are recommended to use "trafficRouting.istio.virtualServices" instead of "trafficRouting.istio.virtualService". Because it provides more flexibility in adding one or more virtualServices. I am still keeping virtualService is to provide the backward compatibility. Going forward, we can deprecate "trafficRouting.istio.virtualService".

Checklist:
Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

…ollouts Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

…Argo Rollouts fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

huikang

Thanks for the contributions. My review comments are added.

docs/getting-started/istio/index.md

docs/getting-started/istio/rollout.yaml

pkg/apis/rollouts/validation/validation_references.go

rollout/trafficrouting/istio/controller_test.go

rollout/trafficrouting/istio/istio.go

huikang · 2021-08-02T13:45:46Z

utils/istio/istio.go

 // GetRolloutVirtualServiceKeys gets the referenced VirtualService and its namespace from a Rollout
 func GetRolloutVirtualServiceKeys(ro *v1alpha1.Rollout) []string {
+	var virtualServices []v1alpha1.IstioVirtualService
+	var virtualServiceKeys []string


Are we getting keys or just namespace+name? The variable name is kind of confusing.

Basically, we get the list of keys. each key will be in the format of namespace / virtualService Name

so how about vsvsNamespaceName? Key seems have different meanings in k8s.

not resolved yet

The only way to identify the rollout is with namespace and name. So, having the key as the variable name would make more sense.
I didn't change this in my latest update. Let me know your thoughts.

utils/istio/istio.go

huikang · 2021-08-02T13:49:37Z

utils/istio/istio.go

-	if namespace == "" {
-		namespace = ro.Namespace
+	if MultipleVirtualServiceConfigured(ro) {
+		if canary.TrafficRouting.Istio.VirtualService.Name != "" {


this should not happen, right? Because the validation logic will not allow to reach this place. If so, this is a panic.
Or we can remove line 86 - 88

Let's say both virtualService and virtualServices are configured then MultipleVirtualServiceConfigured method return true. Since both are configured which is an invalid configuration. so, i am having this check.

Such scenario should be detected earlier in the validationVirtualServiceConfig, I believe this if and return empty string array may introduce a subtle bug in the future.

please resolve this comment

Yes, this check is already taken care. I have removed it.
Please check, let me know if you are okay with this change then I will resolve it.

…Argo Rollouts. Addressed all the review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

shakti-das · 2021-08-04T04:31:59Z

Hi @jessesuen, could you please review this PR when you find the time. We want to get Argo Rollouts to production soon and this issue is a blocker right now. Thank you.

huikang

Thanks for the update. Looks much better.

docs/getting-started/istio/index.md

rollout/trafficrouting/istio/controller.go

docs/getting-started/istio/index.md

…Argo Rollouts. Addressed the minor review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

huikang · 2021-08-05T14:03:32Z

utils/istio/istio.go

-	if namespace == "" {
-		namespace = ro.Namespace
+	if MultipleVirtualServiceConfigured(ro) {
+		if canary.TrafficRouting.Istio.VirtualService.Name != "" {


Such scenario should be detected earlier in the validationVirtualServiceConfig, I believe this if and return empty string array may introduce a subtle bug in the future.

rollout/trafficrouting/istio/istio_test.go

huikang · 2021-08-05T14:10:44Z

Hi, @jessesuen , I almost finished reviewing this PR. Do you mind kicking off the CI flow? Thanks.

codecov · 2021-08-05T20:51:09Z

Codecov Report

Merging #1381 (80bf601) into master (c72a354) will increase coverage by 0.07%.
The diff coverage is 89.10%.

@@            Coverage Diff             @@
##           master    #1381      +/-   ##
==========================================
+ Coverage   81.35%   81.43%   +0.07%     
==========================================
  Files         108      108              
  Lines       14505    14573      +68     
==========================================
+ Hits        11801    11867      +66     
  Misses       2089     2089              
- Partials      615      617       +2

Impacted Files	Coverage Δ
rollout/controller.go	`75.19% <40.00%> (-0.35%)`	⬇️
rollout/trafficrouting/istio/controller.go	`50.81% <76.00%> (+3.38%)`	⬆️
.../apis/rollouts/validation/validation_references.go	`84.58% <87.09%> (+2.10%)`	⬆️
rollout/trafficrouting/istio/istio.go	`80.96% <100.00%> (+0.24%)`	⬆️
utils/istio/istio.go	`95.00% <100.00%> (+1.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c72a354...80bf601. Read the comment docs.

…Argo Rollouts. Addressed few minor review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 · 2021-08-05T21:11:27Z

Hi @jessesuen , I have updated the PR.
Can you please kickoff the CI flow again?
Thanks

huikang

Please do not mark the comment resolved as you have not made any change.

rollout/trafficrouting/istio/istio_test.go

huikang · 2021-08-08T17:12:01Z

utils/istio/istio.go

 // GetRolloutVirtualServiceKeys gets the referenced VirtualService and its namespace from a Rollout
 func GetRolloutVirtualServiceKeys(ro *v1alpha1.Rollout) []string {
+	var virtualServices []v1alpha1.IstioVirtualService
+	var virtualServiceKeys []string


not resolved yet

huikang · 2021-08-08T17:12:30Z

utils/istio/istio.go

-	if namespace == "" {
-		namespace = ro.Namespace
+	if MultipleVirtualServiceConfigured(ro) {
+		if canary.TrafficRouting.Istio.VirtualService.Name != "" {


please resolve this comment

shakti-das · 2021-08-11T05:30:52Z

@darshanhs09 could you please address the open review comments. Thanks.

…Argo Rollouts. Addressed few minor review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 · 2021-08-12T11:42:21Z

@darshanhs09 could you please address the open review comments. Thanks.

Hi @shakti-das, I was out of town so couldn't check.
I have updated the changes now.
Thanks

…Argo Rollouts. fixing the lint issue. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

huikang · 2021-08-13T00:16:34Z

Could you resolve the conflict? Else LGTM.

sboobna · 2021-08-17T16:27:14Z

@darshanhs09

darshanhs09 · 2021-08-19T18:17:41Z

TlsRoutes is a new field introduced in virtualService.
Now, I cannot commit my changes directly not just because it is conflicting with some of my changes.
I have to also handle this new field in virtualServices.
So, I made the changes to handle this new field in multiple virtual services. But I am yet to test it.
Once I am done with my testing I will update the PR.

Thanks

jessesuen · 2021-08-12T08:23:03Z

pkg/apis/rollouts/v1alpha1/types.go

@@ -354,9 +354,11 @@ type NginxTrafficRouting struct {
 // IstioTrafficRouting configuration for Istio service mesh to enable fine grain configuration
 type IstioTrafficRouting struct {
 	// VirtualService references an Istio VirtualService to modify to shape traffic
-	VirtualService IstioVirtualService `json:"virtualService" protobuf:"bytes,1,opt,name=virtualService"`
+	VirtualService IstioVirtualService `json:"virtualService,omitempty" protobuf:"bytes,1,opt,name=virtualService"`


I think the VirtualService field will need to be changed to a *IstioVirtualService pointer, now that it is optional, since otherwise the virtualService field will be defaulted to an empty struct after json marshaling. e.g.:

spec: strategy: canary: trafficRouting: istio: virtualService: {}

Yeah! you are right!
I am thinking, I will take care of this separately. All the existing code of virtualService needs to changed for this. It would be better to take care of it in a separate issue.

…Argo Rollouts. fixing the unit test issue. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

…Argo Rollouts. Adding the TLSRoute details in the multiple virtualService spec. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 · 2021-08-27T18:51:00Z

@huikang I have resolved the conflicts and update the PR.
Can you please check and approve?
@jessesuen Can you please kickoff the CI flow?

huikang · 2021-08-27T20:06:31Z

Hi, @darshanhs09, thanks for the update. Could you resolve the lint issue and re-run codegen?

huikang

some minor suggestions in the test file

rollout/trafficrouting/istio/controller_test.go

pkg/apis/rollouts/validation/validation_references_test.go

pkg/apis/rollouts/validation/validation_references.go

pkg/apis/rollouts/validation/validation_references_test.go

…Argo Rollouts. Addressing few minor comments and lint issues. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 · 2021-08-27T21:25:29Z

I have addressed Hui comments and lint issues.
@jessesuen Can you please kickoff the CI flow again?

huikang · 2021-08-31T01:27:08Z

LGTM

…Argo Rollouts. Addressing UT issue. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

sonarqubecloud · 2021-08-31T07:48:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

No Coverage information
13.7% Duplication

darshanhs09 · 2021-08-31T07:48:59Z

@huikang Thanks for reviewing.
@jessesuen Can you please run the CI flow ?

jessesuen · 2021-09-08T17:12:46Z

Great work!

Multiple VirtualService objects support for canary strategy of Argo R…

3f3462f

…ollouts Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from d2c0b6a to 3f3462f Compare July 29, 2021 21:12

darshanhs09 changed the title ~~feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts. Fixes #1100~~ feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts. Fixes #1100 Jul 29, 2021

darshanhs09 changed the title ~~feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts. Fixes #1100~~ feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts Jul 29, 2021

feat: Multiple VirtualService objects support for canary strategy of …

ec64953

…Argo Rollouts fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 810a451 to ec64953 Compare July 31, 2021 22:56

huikang reviewed Aug 2, 2021

View reviewed changes

feat: Multiple VirtualService objects support for canary strategy of …

415edd8

…Argo Rollouts. Addressed all the review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 9f0125f to 415edd8 Compare August 3, 2021 20:38

huikang reviewed Aug 4, 2021

View reviewed changes

feat: Multiple VirtualService objects support for canary strategy of …

b355ffb

…Argo Rollouts. Addressed the minor review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from bc187bb to b355ffb Compare August 4, 2021 18:41

huikang reviewed Aug 5, 2021

View reviewed changes

feat: Multiple VirtualService objects support for canary strategy of …

58d3595

…Argo Rollouts. Addressed few minor review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from a611c0b to 58d3595 Compare August 5, 2021 21:06

huikang suggested changes Aug 8, 2021

View reviewed changes

jessesuen changed the title ~~feat: Multiple VirtualService objects support for canary strategy of Argo Rollouts~~ feat: support management of multiple Istio VirtualService objects Aug 12, 2021

feat: Multiple VirtualService objects support for canary strategy of …

2222dc8

…Argo Rollouts. Addressed few minor review comments fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 6061e68 to 2222dc8 Compare August 12, 2021 11:31

feat: Multiple VirtualService objects support for canary strategy of …

f627404

…Argo Rollouts. fixing the lint issue. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 415576c to f627404 Compare August 12, 2021 12:03

jessesuen reviewed Aug 20, 2021

View reviewed changes

feat: Multiple VirtualService objects support for canary strategy of …

5870f2e

…Argo Rollouts. fixing the unit test issue. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 34ddd1d to 5870f2e Compare August 27, 2021 17:42

feat: Multiple VirtualService objects support for canary strategy of …

0c0871c

…Argo Rollouts. Adding the TLSRoute details in the multiple virtualService spec. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 61c0ace to 0c0871c Compare August 27, 2021 17:55

Merge branch 'master' into multiple-virtual-services-support

5cdb12e

huikang suggested changes Aug 27, 2021

View reviewed changes

feat: Multiple VirtualService objects support for canary strategy of …

bb1afbe

…Argo Rollouts. Addressing few minor comments and lint issues. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 8d9cffd to bb1afbe Compare August 27, 2021 21:22

darshanhs09 added 2 commits August 31, 2021 03:40

Merge branch 'master' into multiple-virtual-services-support

1917388

feat: Multiple VirtualService objects support for canary strategy of …

80bf601

…Argo Rollouts. Addressing UT issue. fixes argoproj#1100 Signed-off-by: Darshan Hassan Shashikumar <[email protected]>

darshanhs09 force-pushed the multiple-virtual-services-support branch from 6c2f60e to 80bf601 Compare August 31, 2021 07:46

jessesuen approved these changes Sep 8, 2021

View reviewed changes

jessesuen merged commit 8c3e1a2 into argoproj:master Sep 8, 2021

perenesenko mentioned this pull request Sep 28, 2021

Istio does not switch the traffic #1552

Closed

feat: support management of multiple Istio VirtualService objects #1381

feat: support management of multiple Istio VirtualService objects #1381

Conversation

darshanhs09 commented Jul 29, 2021 • edited Loading

huikang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darshanhs09 Aug 3, 2021 • 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

Choose a reason for hiding this comment

shakti-das commented Aug 4, 2021

huikang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huikang commented Aug 5, 2021

codecov bot commented Aug 5, 2021 • edited Loading

Codecov Report

darshanhs09 commented Aug 5, 2021 • edited Loading

huikang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakti-das commented Aug 11, 2021

darshanhs09 commented Aug 12, 2021

huikang commented Aug 13, 2021

sboobna commented Aug 17, 2021

darshanhs09 commented Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darshanhs09 commented Aug 27, 2021

huikang commented Aug 27, 2021

huikang left a comment

Choose a reason for hiding this comment

darshanhs09 commented Aug 27, 2021

huikang commented Aug 31, 2021

sonarqubecloud bot commented Aug 31, 2021

darshanhs09 commented Aug 31, 2021

jessesuen commented Sep 8, 2021

darshanhs09 commented Jul 29, 2021 •

edited

Loading

darshanhs09 Aug 3, 2021 •

edited

Loading

codecov bot commented Aug 5, 2021 •

edited

Loading

darshanhs09 commented Aug 5, 2021 •

edited

Loading