feat: Add support for Istio multicluster setup. Fixes #1191. #1274

shakti-das · 2021-06-13T10:27:11Z

Fixes #1191

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.

shakti-das · 2021-06-13T11:07:36Z

Verified the change in our multicluster setup. I will update the docs on how to set it up next. In brief:

Use istioctl x create-remote-secret to generate a secret for the Istio primary cluster.
Label the secret with label istio.argoproj.io/primary-cluster: true.
Rollout controller during startup checks for the existence of this secret.
If a secret with this label exists, then configure Istio Controller kube client to read/watch/update from the primary/config cluster instead of the current cluster.

shakti-das · 2021-06-13T12:18:04Z

@jessesuen Please take a look whenever you find the time. Thanks.

shakti-das · 2021-06-17T06:52:44Z

@jessesuen could you please take a look and provide any feedback for the approach? Thanks.

codecov · 2021-07-01T18:43:38Z

Codecov Report

Merging #1274 (ba80df2) into master (503c520) will decrease coverage by 0.11%.
The diff coverage is 48.83%.

@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
- Coverage   81.40%   81.28%   -0.12%     
==========================================
  Files         106      107       +1     
  Lines        9778     9818      +40     
==========================================
+ Hits         7960     7981      +21     
- Misses       1278     1295      +17     
- Partials      540      542       +2

Impacted Files	Coverage Δ
utils/istio/multicluster.go	`45.00% <45.00%> (ø)`
rollout/controller.go	`78.35% <100.00%> (ø)`
rollout/trafficrouting.go	`88.46% <100.00%> (ø)`
rollout/trafficrouting/istio/controller.go	`45.73% <0.00%> (+2.32%)`	⬆️

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 503c520...ba80df2. Read the comment docs.

huikang

Looks good. Just some minor questions. Thanks.

huikang · 2021-07-01T18:20:49Z

docs/features/traffic-management/istio.md

+  name: argo-rollouts-istio-primary
+  namespace: <any-namespace-preferrably-config-namespace>
+```
+2. Create a `ClusterRole` that provides access to Rollout controller in the Istio primary cluster.


The default role in the manifest folder should be good

argo-rollouts/manifests/role/argo-rollouts-clusterrole.yaml

Lines 139 to 150 in 460262e

# virtualservice/destinationrule access needed for using the Istio provider

- apiGroups:

- networking.istio.io

resources:

- virtualservices

- destinationrules

verbs:

- watch

- get

- update

- patch

- list

Could you put a note there so that user doesn't need to create extra clusterrole.

Good point. If Argo controller is also installed in the primary cluster (not mandatory), then we could just use the default argo-rollouts-clusterrole instead of creating another in the primary cluster. I will add a note to mention the same. Thanks.

huikang · 2021-07-01T18:24:03Z

utils/istio/multicluster.go

+	PrimaryClusterSecretLabel = "istio.argoproj.io/primary-cluster"
+)
+
+func GetPrimaryClusterDynamicClient(kubeClient kubernetes.Interface, namespace string) (string, dynamic.Interface) {


Shall we return an error here? If a user sets up the PrimaryClusterSecretLabel, the intention is to use the multi-cluster feature. Without returning an error, user may miss the error message in the caller.

It depends on do we want to stop the controller from starting up if there is an error in this setup? The current behavior is that the error related to the primary cluster setup will be logged as an error, but it won't stop the argo controller from starting up. The controller will just continue doing what it does now, i.e., assume the cluster it's running on as the cluster where Istio config is available.

huikang · 2021-07-01T19:12:01Z

utils/istio/multicluster.go

+		primaryClusterConfig, err := buildKubeClientConfig(kubeConfig)
+		if err != nil {
+			log.Errorf("Error building kubeconfig for primary cluster %s: %v", clusterId, err)
+			return clusterId, nil, err


can we include the clusterID in the returned error message, fmt.Errorf("Error building kubeconfig for primary cluster %s: %v", clusterId, err)

huikang · 2021-07-01T19:14:46Z

utils/istio/multicluster.go

+}
+
+func getKubeClientConfig(secret *corev1.Secret) (string, clientcmd.ClientConfig, error) {
+	for clusterId, kubeConfig := range secret.Data {


Could you elaborate a bit on how to determine the clusterid to return its config? Thanks.

The Secret passed to this method is the secret generated via istioctl x create-remote-secret and labeled istio.argoproj.io/primary-cluster: true. The istioctl x create-remote-secret command generates a Secret with the data as: clusterId: encoded-cluster-config. Here we read that data and build the kube config for the primary cluster.

jessesuen

Great work!

jessesuen · 2021-07-07T12:14:33Z

Change LGTM. @shakti-das - can you fix linting errors and I can merge it in

Signed-off-by: Shakti <[email protected]>

sonarcloud · 2021-07-07T14:47:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

shakti-das · 2021-07-07T15:07:49Z

@jessesuen did a gofmt -s -w but don't know if it fixed the CI error, still see:

4 workflows awaiting approval
First-time contributors need a maintainer to approve running workflows

Could you please re-trigger the workflows? Thanks.

shakti-das · 2021-07-08T05:55:00Z

The log in Lint Go job:

Current runner version: '2.278.0'
Operating System
Virtual Environment
GITHUB_TOKEN Permissions
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v2'
Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: The operation was canceled.
Warning: Back off 26.085 seconds before retry.
Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: The operation was canceled.
Warning: Back off 18.385 seconds before retry.
Error: The operation was canceled.

jessesuen · 2021-07-08T18:20:08Z

Yep this was a hiccup in the github action. Reran and linting has passed

shakti-das force-pushed the shakti/istio-multicluster branch from e731ccc to 4004f62 Compare June 13, 2021 10:28

shakti-das changed the title ~~feat (Istio multicluster): read/watch/update VS/DR in primary cluster in an Istio multicluster setup. Fixes #1191.~~ feat: Watch/update VS/DR in primary cluster in an Istio multicluster setup. Fixes #1191. Jun 13, 2021

shakti-das changed the title ~~feat: Watch/update VS/DR in primary cluster in an Istio multicluster setup. Fixes #1191.~~ feat: Add support for Istio multicluster setup. Fixes #1191. Jun 13, 2021

shakti-das force-pushed the shakti/istio-multicluster branch from 4004f62 to 631bdd9 Compare June 13, 2021 10:46

shakti-das force-pushed the shakti/istio-multicluster branch from 7cff585 to 0c7cc07 Compare June 19, 2021 10:27

shakti-das force-pushed the shakti/istio-multicluster branch from 0c7cc07 to 3fd8284 Compare June 28, 2021 06:02

huikang reviewed Jul 1, 2021

View reviewed changes

shakti-das force-pushed the shakti/istio-multicluster branch from 3fd8284 to 68a2408 Compare July 2, 2021 09:09

jessesuen approved these changes Jul 7, 2021

View reviewed changes

shakti-das added 7 commits July 7, 2021 20:16

Initial working code

dd74ab0

Signed-off-by: Shakti <[email protected]>

Refactor, add tests

016e103

Signed-off-by: Shakti <[email protected]>

Add organization to users.md

e060855

Signed-off-by: Shakti <[email protected]>

Refactor test

20d1aa2

Signed-off-by: Shakti <[email protected]>

Update docs for enabling Argo Rollouts in Istio multicluster setup

a76373d

Signed-off-by: Shakti <[email protected]>

Address review comments

a39e1b5

Signed-off-by: Shakti <[email protected]>

gofmt -s -w new files

ba80df2

Signed-off-by: Shakti <[email protected]>

shakti-das force-pushed the shakti/istio-multicluster branch from feb4b9a to ba80df2 Compare July 7, 2021 14:46

jessesuen merged commit ba21c6c into argoproj:master Jul 8, 2021

shakti-das deleted the shakti/istio-multicluster branch July 10, 2021 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for Istio multicluster setup. Fixes #1191. #1274

feat: Add support for Istio multicluster setup. Fixes #1191. #1274

shakti-das commented Jun 13, 2021 •

edited

Loading

shakti-das commented Jun 13, 2021 •

edited

Loading

shakti-das commented Jun 13, 2021

shakti-das commented Jun 17, 2021

codecov bot commented Jul 1, 2021 •

edited

Loading

huikang left a comment

huikang Jul 1, 2021

shakti-das Jul 2, 2021

huikang Jul 1, 2021

shakti-das Jul 2, 2021

huikang Jul 1, 2021

huikang Jul 1, 2021

shakti-das Jul 2, 2021

jessesuen left a comment

jessesuen commented Jul 7, 2021 •

edited

Loading

sonarcloud bot commented Jul 7, 2021

shakti-das commented Jul 7, 2021

shakti-das commented Jul 8, 2021

jessesuen commented Jul 8, 2021

	# virtualservice/destinationrule access needed for using the Istio provider
	- apiGroups:
	- networking.istio.io
	resources:
	- virtualservices
	- destinationrules
	verbs:
	- watch
	- get
	- update
	- patch
	- list

feat: Add support for Istio multicluster setup. Fixes #1191. #1274

feat: Add support for Istio multicluster setup. Fixes #1191. #1274

Conversation

shakti-das commented Jun 13, 2021 • edited Loading

shakti-das commented Jun 13, 2021 • edited Loading

shakti-das commented Jun 13, 2021

shakti-das commented Jun 17, 2021

codecov bot commented Jul 1, 2021 • edited Loading

Codecov Report

huikang left a comment

Choose a reason for hiding this comment

huikang Jul 1, 2021

Choose a reason for hiding this comment

shakti-das Jul 2, 2021

Choose a reason for hiding this comment

huikang Jul 1, 2021

Choose a reason for hiding this comment

shakti-das Jul 2, 2021

Choose a reason for hiding this comment

huikang Jul 1, 2021

Choose a reason for hiding this comment

huikang Jul 1, 2021

Choose a reason for hiding this comment

shakti-das Jul 2, 2021

Choose a reason for hiding this comment

jessesuen left a comment

Choose a reason for hiding this comment

jessesuen commented Jul 7, 2021 • edited Loading

sonarcloud bot commented Jul 7, 2021

shakti-das commented Jul 7, 2021

shakti-das commented Jul 8, 2021

jessesuen commented Jul 8, 2021

shakti-das commented Jun 13, 2021 •

edited

Loading

shakti-das commented Jun 13, 2021 •

edited

Loading

codecov bot commented Jul 1, 2021 •

edited

Loading

jessesuen commented Jul 7, 2021 •

edited

Loading