Remove deprecated --require-kubeconfig flag, remove default --kubeconfig value #58367

zhangxiaoyu-zidif · 2018-01-17T03:47:20Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #41161

Special notes for your reviewer:

Release note:

Remove deprecated --require-kubeconfig flag, remove default --kubeconfig value

zhangxiaoyu-zidif · 2018-01-17T03:50:24Z

/assign @mtaufen

zhangxiaoyu-zidif · 2018-01-17T06:09:07Z

/test pull-kubernetes-e2e-kops-aws

zhangxiaoyu-zidif · 2018-01-17T08:41:30Z

/test pull-kubernetes-e2e-kops-aws

zhangxiaoyu-zidif · 2018-01-17T09:22:06Z

/test pull-kubernetes-e2e-kops-aws
flake maybe

zhangxiaoyu-zidif · 2018-01-17T12:51:44Z

/test pull-kubernetes-e2e-kops-aws

zhangxiaoyu-zidif · 2018-01-17T14:25:30Z

/retest

zhangxiaoyu-zidif · 2018-01-17T14:26:37Z

flake ref #58387

mtaufen · 2018-01-17T17:29:00Z

cmd/kubelet/app/options/options.go

@@ -310,9 +305,6 @@ func (f *KubeletFlags) AddFlags(fs *pflag.FlagSet) {
 	f.ContainerRuntimeOptions.AddFlags(fs)

 	fs.Var(&f.KubeConfig, "kubeconfig", "Path to a kubeconfig file, specifying how to connect to the API server.")


Please add the following to the end of the help text: Providing --kubeconfig enables API server mode, omitting --kubeconfig enables standalone mode. Please also annotate the field in the KubeletFlags struct with a comment that matches the help text.

mtaufen · 2018-01-17T17:33:02Z

cmd/kubelet/app/server.go

-			case s.RequireKubeConfig:
-				return fmt.Errorf("invalid kubeconfig: %v", err)
-			case s.KubeConfig.Provided():
+			if s.KubeConfig.Provided() {


Simplify this to else if, don't need the extra nesting.

I think you can drop the else block entirely

shouldn't we still log the warning somewhere?

mtaufen · 2018-01-17T17:33:27Z

cmd/kubelet/app/server.go

@@ -604,9 +597,7 @@ func kubeconfigClientConfig(s *options.KubeletServer) (*restclient.Config, error
 // --require-kubeconfig=true, we attempt to load the default kubeconfig file.
 func createClientConfig(s *options.KubeletServer) (*restclient.Config, error) {
 	// If --kubeconfig was not provided, it will have a default path set in cmd/kubelet/app/options/options.go.


This comment is no longer correct, now that we're removing the default path.

krzyzacy · 2018-01-17T18:22:35Z

/retest

liggitt · 2018-01-17T18:44:11Z

cmd/kubelet/app/options/options.go

@@ -310,9 +305,6 @@ func (f *KubeletFlags) AddFlags(fs *pflag.FlagSet) {
 	f.ContainerRuntimeOptions.AddFlags(fs)

 	fs.Var(&f.KubeConfig, "kubeconfig", "Path to a kubeconfig file, specifying how to connect to the API server.")


would also like to see:

switch KubeConfig to just be a string

use fs.StringVar(&f.KubeConfig, "kubeconfig", f.KubeConfig, ...

use len(s.KubeConfig) > 0 instead of s.KubeConfig.Provided()

use len(s.KubeConfig) == 0 instead of standaloneMode

that makes the options struct much more natural to work with

liggitt · 2018-01-17T18:53:00Z

cmd/kubelet/app/server.go

@@ -604,9 +597,7 @@ func kubeconfigClientConfig(s *options.KubeletServer) (*restclient.Config, error
 // --require-kubeconfig=true, we attempt to load the default kubeconfig file.
 func createClientConfig(s *options.KubeletServer) (*restclient.Config, error) {


createAPIServerClientConfig -> createClientConfig -> kubeconfigClientConfig are all only called from one code path, not in standalone mode. most of this error checking is moot at this point. I'd consider just flattening those in the main flow in run()

zhangxiaoyu-zidif · 2018-01-19T00:41:16Z

/test pull-kubernetes-e2e-kops-aws

krzyzacy · 2018-01-19T01:21:32Z

/retest

liggitt · 2018-01-19T01:39:37Z

cmd/kubelet/app/server.go

-			}
+		}
+
+		if err != nil && len(s.KubeConfig) > 0 {


no need to check s.KubeConfig... if we're in this block, it was provided

Thanks Jordan ! Done.

liggitt · 2018-01-19T01:40:30Z

one nit on dropping a KubeConfig check before logging, then LGTM. thanks for cleaning this up

liggitt · 2018-01-19T02:20:11Z

cmd/kubelet/app/server.go

-		} else {
-			switch {
-			case s.RequireKubeConfig:
-				return fmt.Errorf("invalid kubeconfig: %v", err)


actually just re-read this, and this isn't quite right

--kubeconfig=foo is now equivalent to --kubeconfig=foo --require-kubeconfig

if you provide --kubeconfig and createAPIServerClientConfig fails, we have to return an error. that means we need to hoist the return error case up to where we create the client config:

clientConfig, err := createAPIServerClientConfig(s) if err != nil { return fmt.Errorf("invalid kubeconfig: %v", err) }

and if we're returning on non-nil error, you can drop the if err == nil { check and unindent the rest

OK， I prefer to returning non-nil err when createAPIServerClientConfig fails.

liggitt · 2018-01-20T07:06:42Z

thanks, go ahead and squash, then lgtm

zhangxiaoyu-zidif · 2018-01-20T07:30:55Z

@liggitt done, thanks.

fejta-bot · 2018-01-28T13:38:13Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.