should not create k8s client if config dir is provided#3779
should not create k8s client if config dir is provided#3779istio-merge-robot merged 4 commits intoistio:masterfrom
Conversation
pilot/pkg/bootstrap/server.go
Outdated
| // initKubeClient creates the k8s client if running in an k8s environment. | ||
| func (s *Server) initKubeClient(args *PilotArgs) error { | ||
| needToCreateClient := false | ||
| // FIXME: in the case of multiple registries, we are currently only checking needToCreateClient for the last one |
There was a problem hiding this comment.
add a break anyplace you set needToCreateClient = true?
There was a problem hiding this comment.
break the for loop with a label, otherwise it just breaks the switch statement.
pilot/pkg/bootstrap/server.go
Outdated
| case EurekaRegistry: | ||
| needToCreateClient = true | ||
| } | ||
| case CloudFoundryRegistry: |
There was a problem hiding this comment.
if you have multiple registries, you want to create a client if ANY of the registries need it, so we shouldn't explicitly set needToCreateClient = false here
0d26a42 to
397da52
Compare
pilot/pkg/bootstrap/server.go
Outdated
| //valid registry, but does not need to create client | ||
| continue | ||
| default: | ||
| errs = multierror.Append(errs, fmt.Errorf("Service registry %s is not supported.", r)) |
There was a problem hiding this comment.
Remove the errs and default case. Right now if you had k8s + Consul, it would fail because it won't accept ConsulRegistry.
pilot/pkg/bootstrap/server.go
Outdated
| needToCreateClient = true | ||
| case EurekaRegistry: | ||
| needToCreateClient = true | ||
| case CloudFoundryRegistry: |
|
This doesn't seem to be a P0 or even P1 - we currently don't support file based config, it's mostly for testing. |
|
We are trying to support it in Consul. And this bug is preventing it.. Either way, its a bug isn't it ? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rshriram The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
in response to istio-users question here: https://groups.google.com/forum/#!topic/istio-users/52SiQkkTq2A
pilot should not need to create a k8s client (and therefor provide a kubeconfig file) when the
--configDirflag is provided. This is currently blocking the use of pilot's filewatcher adapter. The way we are currently checking for whether a k8s client needs to be created has a bug and needs to be corrected - this will be addressed in an additional PR