Additional cleanup, more defaults for easier setup. #4923

costinm · 2018-04-12T22:58:00Z

As long as secret name matches the cluster name (which I think is reasonable default) - it will work with no custom annotations.

The pilot address would confuse users, we're not supporting that mode yet.

istio-merge-robot · 2018-04-13T08:05:33Z

@costinm PR needs rebase

john-a-joyce · 2018-04-13T14:39:21Z

After looking closer at this PR and the previous one #4706. I notice the
kubeconfig for the cluster pilot is running in never comes from the cluster registry. These were changes introduced in 4706 more that changes in the PR. The original intent was that this file based approach could be converted to reading an actual clusterregistry via API calls. If we ever want to support this we would need to either add back the logic to ensure we get the local kubeconfig from the registry or ensure that when we add clusters from the registry we skip the local cluster.

sbezverk · 2018-04-13T14:58:10Z

@john-a-joyce before cluster registry gets init, we need to get a client to read configmap and secrtet, that is why initial kube client is from local server, once cluster registry is populated, local client should not be used and all subsequent calls should use one from registry.
One solution I could see is once cluster registry gets initialized, replace Server's kubeClient kubernetes.Interface with the one from the cluster registry for the corresponding server. Having local server on the cluster registry probably is not an issue, since request for client is based on a cluster name, so if you do not want to use registry for local cluster, do not call for getting a client with local cluster name.

codecov · 2018-04-13T20:21:32Z

Codecov Report

Merging #4923 into master will increase coverage by 1%.
The diff coverage is 32%.

@@           Coverage Diff           @@
##           master   #4923    +/-   ##
=======================================
+ Coverage      74%     74%    +1%     
=======================================
  Files         307     307            
  Lines       25770   25861    +91     
=======================================
+ Hits        18879   19024   +145     
+ Misses       6128    6088    -40     
+ Partials      763     749    -14

Impacted Files	Coverage Δ
pilot/pkg/config/clusterregistry/conversion.go	`38% <32%> (-1%)`	⬇️
mixer/adapter/rbac/controller.go	`39% <0%> (-15%)`	⬇️
mixer/adapter/solarwinds/log_handler.go	`58% <0%> (-6%)`	⬇️
mixer/adapter/circonus/circonus.go	`72% <0%> (-4%)`	⬇️
pilot/pkg/serviceregistry/kube/controller.go	`64% <0%> (-3%)`	⬇️
mixer/adapter/servicecontrol/quotaprocessor.go	`80% <0%> (-2%)`	⬇️
mixer/adapter/fluentd/fluentd.go	`75% <0%> (-1%)`	⬇️
mixer/adapter/denier/denier.go	`100% <0%> (ø)`	⬆️
mixer/adapter/stdio/stdio.go	`100% <0%> (ø)`	⬆️
pilot/pkg/model/controller.go	`100% <0%> (ø)`	⬆️
... and 9 more

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 2a3acc7...d2311ca. Read the comment docs.

googlebot · 2018-04-13T20:21:32Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

sdake

/lgtm

sdake · 2018-04-14T04:31:39Z

from linting gate:

+ bin/fmt.sh -c
File ./pilot/pkg/config/clusterregistry/conversion.go needs formatting. Please run bin/fmt.sh
20a21
> 	"github.com/hashicorp/go-multierror"

baodongli · 2018-04-16T15:06:34Z

retest ci/circleci:lint

istio-testing · 2018-04-16T16:16:23Z

New changes are detected. LGTM label has been removed.

istio-testing · 2018-04-16T19:49:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sdake
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vadimeisenbergibm

Assign the PR to them by writing /assign @vadimeisenbergibm in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

pilot/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

baodongli · 2018-04-16T20:07:51Z

pilot/pkg/config/clusterregistry/conversion.go

@@ -182,13 +133,15 @@ func getClustersConfigs(k8s kubernetes.Interface, configMapName, configMapNamesp
 				secretName, secretNamespace, key, err)
 			return &ClusterStore{}, nil
 		}
+		cs.clusters = append(cs.clusters, &cluster)


remove this line which is a duplicate of line 138

baodongli · 2018-04-16T20:07:55Z

pilot/pkg/config/clusterregistry/conversion.go

 	"strings"

-	multierror "github.com/hashicorp/go-multierror"
-	"go.uber.org/multierr"
+	"github.com/hashicorp/go-multierror"


lint doesn't like it, but the following seems to make it happy

multierror "github.com/hashicorp/go-multierror"

baodongli

@costinm I actually rebased and fixed those two comments before you pushed up this commit.

sbezverk · 2018-04-16T20:27:02Z

pilot/pkg/config/clusterregistry/conversion.go

-			cluster.ObjectMeta.Annotations[ClusterAccessConfigSecretNamespace] = "istio-system"
-		}
+		// by default, expect a secret with the same name as the cluster
+		cluster.ObjectMeta.Annotations[ClusterAccessConfigSecretNamespace] = cluster.Name


I think using just secret name is dangerous as it could cause unpleasant collisions. I would still suggest to use labels as an idnicator that secret carries cluster config. It does not have to be done in this PR, once it is merged I will push a change to address it.

That's going away with the config changes.

sbezverk

Other than duplicate line, looks good.

sdake · 2018-04-17T04:19:30Z

Thanks @costinm.

costinm added 6 commits April 12, 2018 13:45

Fix mc init and get rid of unused code

edfc560

Merge branch 'master' of github.com:istio/istio into costin-mc1

5cb0efc

format

241fe73

Fix the test

9272d40

Additional cleanup and defaults

e0820eb

Merge branch 'master' of github.com:istio/istio into costin-mc1

95c52a2

costinm requested review from sdake, baodongli and sbezverk April 12, 2018 22:58

googlebot added the cla: yes label Apr 12, 2018

Add default for registry namespace

0e1e909

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 13, 2018

baodongli force-pushed the costin-mc1 branch from 0e1e909 to 30fd084 Compare April 13, 2018 20:21

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Apr 13, 2018

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 13, 2018

baodongli approved these changes Apr 13, 2018

View reviewed changes

sdake approved these changes Apr 14, 2018

View reviewed changes

istio-testing assigned sdake Apr 14, 2018

istio-testing added the lgtm label Apr 14, 2018

sdake mentioned this pull request Apr 14, 2018

Tracker for caveats and existing problems with Multicluster #4822

Closed

costinm added 3 commits April 16, 2018 10:54

Fix mc init and get rid of unused code

c92efdc

Fix the test

6d2dd3b

Additional cleanup and defaults

c945357

baodongli force-pushed the costin-mc1 branch from 30fd084 to 429a076 Compare April 16, 2018 16:16

istio-testing removed the lgtm label Apr 16, 2018

chxchx added the cla: human-approved label Apr 16, 2018

baodongli force-pushed the costin-mc1 branch from 429a076 to 4b2306b Compare April 16, 2018 18:56

chxchx removed the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 16, 2018

baodongli force-pushed the costin-mc1 branch from 4b2306b to 975fc59 Compare April 16, 2018 19:25

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 16, 2018

Baodong (Robert) Li and others added 4 commits April 16, 2018 15:26

fix merge conflict and cleanup multierr import

975fc59

Merge branch 'master' of github.com:istio/istio into costin-mc1

31d5c92

Merge branch 'costin-mc1' of github.com:istio/istio into costin-mc1

2e81864

Missed conflicts

216e729

costinm removed the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 16, 2018

baodongli reviewed Apr 16, 2018

View reviewed changes

baodongli suggested changes Apr 16, 2018

View reviewed changes

Lint fix

cdda154

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 16, 2018

costinm removed the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 16, 2018

sbezverk reviewed Apr 16, 2018

View reviewed changes

costinm added 2 commits April 16, 2018 13:47

Duplicated line

3b2b872

Merge branch 'master' of github.com:istio/istio into costin-mc1

d2311ca

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 16, 2018

baodongli approved these changes Apr 16, 2018

View reviewed changes

costinm merged commit 55a1397 into master Apr 16, 2018

costinm deleted the costin-mc1 branch April 16, 2018 21:31

Additional cleanup, more defaults for easier setup. #4923

Additional cleanup, more defaults for easier setup. #4923

Uh oh!

Conversation

costinm commented Apr 12, 2018

Uh oh!

istio-merge-robot commented Apr 13, 2018

Uh oh!

john-a-joyce commented Apr 13, 2018

Uh oh!

sbezverk commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

googlebot commented Apr 13, 2018

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

sdake commented Apr 14, 2018

Uh oh!

baodongli commented Apr 16, 2018

Uh oh!

istio-testing commented Apr 16, 2018

Uh oh!

istio-testing commented Apr 16, 2018

Uh oh!

baodongli Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

baodongli Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

baodongli left a comment

Choose a reason for hiding this comment

Uh oh!

sbezverk Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

costinm Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

sbezverk left a comment

Choose a reason for hiding this comment

Uh oh!

sdake commented Apr 17, 2018

Uh oh!

Uh oh!

sbezverk commented Apr 13, 2018 •

edited

Loading

codecov bot commented Apr 13, 2018 •

edited

Loading