Pilot param clusterRegistriesNamespace should default to pilot namespace by tiswanso · Pull Request #6446 · istio/istio

tiswanso · 2018-06-20T21:24:05Z

use pilot ns if set
use "istio-system" as last resort

tiswanso · 2018-06-20T21:25:00Z

This helps with multicluster tests--see Issue #6072

tiswanso · 2018-06-21T13:54:10Z

/cc @baodongli @gyliu513 @sbezverk

sbezverk · 2018-06-21T14:00:53Z

pilot/pkg/bootstrap/server.go

hm, what if pilot runs out of cluster mode and namespace parameter is not specified?

in that case args.Namespace will be the pilot pod's namespace (os.Getenv("POD_NAMESPACE")). See the code section right above this one.

@tiswanso that was exactly why I asked, if pilot runs OUT of cluster then line 199 will return nil as this variable is not defined.

@sbezverk -- sorry, I misunderstood. hmm... in that case perhaps we shouldn't start the secret controller since there's no args to direct us to watch something. OR alternatively, we could revert to istio-system if args.Namespace == nil.

I think having a istio-system namespace as a last resort is fine.

- use pilot ns as first default and "istio-system" as last resort

codecov · 2018-06-21T14:38:40Z

Codecov Report

Merging #6446 into master will decrease coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #6446     +/-   ##
========================================
- Coverage      68%     67%    -<1%     
========================================
  Files         368     345     -23     
  Lines       31687   30407   -1280     
========================================
- Hits        21489   20317   -1172     
+ Misses       9346    9280     -66     
+ Partials      852     810     -42

Impacted Files	Coverage Δ
pilot/cmd/pilot-discovery/main.go	`77% <100%> (ø)`
istioctl/cmd/istioctl/authn.go	`50% <0%> (-23%)`	⬇️
...olarwinds/internal/papertrail/papertrail_logger.go	`59% <0%> (-21%)`	⬇️
istioctl/cmd/istioctl/inject.go	`27% <0%> (-16%)`	⬇️
pilot/pkg/proxy/envoy/v2/debug.go	`27% <0%> (-2%)`	⬇️
pilot/pkg/serviceregistry/consul/controller.go	`71% <0%> (-2%)`	⬇️
mixer/adapter/bypass/bypass.go	`62% <0%> (-2%)`	⬇️
pilot/pkg/networking/plugin/authz/rbac.go	`79% <0%> (-1%)`	⬇️
...t/pkg/serviceregistry/external/servicediscovery.go	`80% <0%> (-1%)`	⬇️
istioctl/cmd/istioctl/config.go	`2% <0%> (-1%)`	⬇️
... and 116 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 5b1c7bc...fc2657b. Read the comment docs.

gyliu513 · 2018-06-21T15:08:10Z

pilot/cmd/pilot-discovery/main.go

 	discoveryCmd.PersistentFlags().StringVar(&serverArgs.Config.ClusterRegistriesConfigmap, "clusterRegistriesConfigMap", "",
 		"ConfigMap map for clusters config store")
-	discoveryCmd.PersistentFlags().StringVar(&serverArgs.Config.ClusterRegistriesNamespace, "clusterRegistriesNamespace", "istio-system",
+	discoveryCmd.PersistentFlags().StringVar(&serverArgs.Config.ClusterRegistriesNamespace, "clusterRegistriesNamespace", "",


Why not define the default value as istio-system as before and set ClusterRegistriesNamespace to args.Namespace if ClusterRegistriesNamespace is empty?

if the default is declared here as before, we can't tell if the user didn't set it.

Ah, I see, thanks @tiswanso

istio-testing · 2018-06-21T15:12:50Z

@tiswanso: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-dashboard.sh	`fc2657b`	link	`/test e2e-dashboard`

Details

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. I understand the commands that are listed here.

baodongli

/lgtm

sbezverk · 2018-06-21T16:46:11Z

/lgtm

gyliu513

/lgtm

gyliu513 · 2018-06-21T22:50:24Z

pilot/cmd/pilot-discovery/main.go

 	discoveryCmd.PersistentFlags().StringVar(&serverArgs.Config.ClusterRegistriesConfigmap, "clusterRegistriesConfigMap", "",
 		"ConfigMap map for clusters config store")
-	discoveryCmd.PersistentFlags().StringVar(&serverArgs.Config.ClusterRegistriesNamespace, "clusterRegistriesNamespace", "istio-system",
+	discoveryCmd.PersistentFlags().StringVar(&serverArgs.Config.ClusterRegistriesNamespace, "clusterRegistriesNamespace", "",


Ah, I see, thanks @tiswanso

gyliu513 · 2018-06-21T22:53:07Z

pilot/pkg/bootstrap/server.go

+	if args.Config.ClusterRegistriesNamespace == "" {
+		if args.Namespace != "" {
+			args.Config.ClusterRegistriesNamespace = args.Namespace
+		} else {


One more question, I think we do not need the else now, as 198-199 can always guarantee there is a value for args.Namespace

I was thinking the same but @sbezverk reminded that there's deployment scenarios with pilot not in a k8s cluster & so it's possible there's no POD_NAMESPACE in env.

make sense, do you want to add a comment here to clarify?

john-a-joyce · 2018-06-22T14:26:52Z

/lgtm

istio-testing · 2018-06-22T14:26:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: baodongli, gyliu513, john-a-joyce, sbezverk, tiswanso
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: costinm

Assign the PR to them by writing /assign @costinm 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

Details

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

sdake · 2018-06-24T02:25:28Z

/assign @costinm

…t namespace (#6446)" This reverts commit b9294f7.

* Revert "Remove v2 transition commands since everything is now v2 (#6665)" This reverts commit 6339eb6. * Revert "Pilot param clusterRegistriesNamespace should default to pilot namespace (#6446)" This reverts commit b9294f7. * Revert "iptable just "return" by uid as the parameter u indicates (#6561)" This reverts commit 22a0b88. * Revert "Remove node agent service, residue from flexvolume driver. (#6651)" This reverts commit db3da82. * Revert "Continuously reapply galley CA bundle to prevent overwrite (#6599)" This reverts commit f9e8fd8. * Revert "Do not count typeConfigs if it is error. (#6527)" This reverts commit eb1de31. * Revert "Make racetest green - Fixed data races and flakiness (#6625)" This reverts commit 30b8ecb. * Revert "Improve push squashing (#6641)" This reverts commit 399cd2d.

…ace (istio#6446) - use pilot ns as first default and "istio-system" as last resort

* Revert "Remove v2 transition commands since everything is now v2 (istio#6665)" This reverts commit 6339eb6. * Revert "Pilot param clusterRegistriesNamespace should default to pilot namespace (istio#6446)" This reverts commit b9294f7. * Revert "iptable just "return" by uid as the parameter u indicates (istio#6561)" This reverts commit 22a0b88. * Revert "Remove node agent service, residue from flexvolume driver. (istio#6651)" This reverts commit db3da82. * Revert "Continuously reapply galley CA bundle to prevent overwrite (istio#6599)" This reverts commit f9e8fd8. * Revert "Do not count typeConfigs if it is error. (istio#6527)" This reverts commit eb1de31. * Revert "Make racetest green - Fixed data races and flakiness (istio#6625)" This reverts commit 30b8ecb. * Revert "Improve push squashing (istio#6641)" This reverts commit 399cd2d.

googlebot added the cla: yes label Jun 20, 2018

istio-testing requested review from baodongli, gyliu513 and sbezverk June 21, 2018 13:54

sbezverk reviewed Jun 21, 2018

View reviewed changes

Pilot param clusterRegistriesNamespace should default to pilot namespace

fc2657b

- use pilot ns as first default and "istio-system" as last resort

tiswanso force-pushed the fix_clusreg_ns_default branch from b30e16b to fc2657b Compare June 21, 2018 14:38

gyliu513 reviewed Jun 21, 2018

View reviewed changes

baodongli approved these changes Jun 21, 2018

View reviewed changes

istio-testing assigned baodongli Jun 21, 2018

istio-testing added the lgtm label Jun 21, 2018

istio-testing assigned sbezverk Jun 21, 2018

tiswanso added the area/environments label Jun 21, 2018

gyliu513 reviewed Jun 21, 2018

View reviewed changes

istio-testing assigned gyliu513 Jun 21, 2018

gyliu513 reviewed Jun 21, 2018

View reviewed changes

gyliu513 mentioned this pull request Jun 21, 2018

Added clusterRegistriesNamespace to istio chart. #6459

Closed

istio-testing assigned john-a-joyce Jun 22, 2018

istio-testing assigned costinm Jun 24, 2018

rshriram merged commit b9294f7 into istio:master Jun 27, 2018

hklai added a commit that referenced this pull request Jun 27, 2018

Revert "Pilot param clusterRegistriesNamespace should default to pilo…

fd249f2

…t namespace (#6446)" This reverts commit b9294f7.

quanjielin pushed a commit to quanjielin/istio that referenced this pull request Jul 2, 2018

Pilot param clusterRegistriesNamespace should default to pilot namesp…

0fbc2d8

…ace (istio#6446) - use pilot ns as first default and "istio-system" as last resort

Conversation

tiswanso commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiswanso commented Jun 20, 2018

Uh oh!

tiswanso commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 21, 2018

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baodongli left a comment

Choose a reason for hiding this comment

Uh oh!

sbezverk commented Jun 21, 2018

Uh oh!

gyliu513 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-a-joyce commented Jun 22, 2018

Uh oh!

istio-testing commented Jun 22, 2018

Uh oh!

sdake commented Jun 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tiswanso commented Jun 20, 2018 •

edited

Loading

istio-testing commented Jun 21, 2018 •

edited

Loading