[kn-admin] Avoid to print runtime error if kubectl context is not set

[chaozbj]

If the kubectl context is not set or run kubectl config unset current-context to unset it, we will get runtime error when run kn admin, for example:

☕10:37 ➜ ~/ [chao ✘] k config current-context
error: current-context is not set

☕10:36 ➜ ~/ [chao ✘] ./cmd autoscaling update --scale-to-zero
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xb0 pc=0x1da8c10]

goroutine 1 [running]:
knative.dev/client-contrib/plugins/admin/pkg/command/autoscaling.NewAutoscalingUpdateCommand.func2(0xc0002e4f00, 0xc0002c7ff0, 0x0, 0x1, 0x0, 0x0)
	/Users/chao/Workspace/Code/github.com/client-contrib/plugins/admin/pkg/command/autoscaling/update.go:66 +0xa0
github.com/spf13/cobra.(*Command).execute(0xc0002e4f00, 0xc0002c7fe0, 0x1, 0x1, 0xc0002e4f00, 0xc0002c7fe0)
	/Users/chao/Workspace/Code/go/pkg/mod/github.com/spf13/[email protected]/command.go:826 +0x460
github.com/spf13/cobra.(*Command).ExecuteC(0xc000197400, 0x0, 0x0, 0xc000197400)
	/Users/chao/Workspace/Code/go/pkg/mod/github.com/spf13/[email protected]/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/chao/Workspace/Code/go/pkg/mod/github.com/spf13/[email protected]/command.go:864
main.main()
	/Users/chao/Workspace/Code/github.com/client-contrib/plugins/admin/cmd/kn-admin.go:24 +0x40

I checked the codes in AdminParams:

// Initialize generate the clientset for params
func (params *AdminParams) Initialize() error {
	if params.ClientSet == nil {
		restConfig, err := params.RestConfig()
		if err != nil {
			return err
		}

		params.ClientSet, err = kubernetes.NewForConfig(restConfig)
		if err != nil {
			fmt.Println("failed to create client:", err)
			os.Exit(1)
		}
	}
	return nil
}

// rootCmd represents the base command when called without any subcommands
func NewAdminCommand(params ...pkg.AdminParams) *cobra.Command {
	p := &pkg.AdminParams{}
	p.Initialize()

	rootCmd := &cobra.Command{
		Use:   "kn\u00A0admin",
		Short: "A plugin of kn client to manage Knative",
		Long:  `kn admin: a plugin of kn client to manage Knative for administrators`,

...

I think we can do some improvements for the above functions so that we can avoid outputting runtime error if kubectl context is not set:

1. In Initialize(), we'd better return error when fail to create ClientSet
2. In NewAdminCommand(), we should check the error from p.Initialize(), if error is returned, we can print the error message and exit to run command.

[chaozbj]

@zhanggbj Have you any comments on that? If no objections, I will assign it to myself to do. Thanks!

[zhanggbj]

@chaozbj good finding! Thanks!

• about the title, I guess it should be kubectl context instead of kubelet context, could you please update it and the description.

• Please also help to verify your refine PR works with IKS cluster and minikube, thanks!

[chaozbj]

/assign

[chaozbj]

@zhanggbj After I improved the codes to:

// rootCmd represents the base command when called without any subcommands
func NewAdminCommand(params ...pkg.AdminParams) *cobra.Command {
	p := &pkg.AdminParams{}
	if err := p.Initialize(); err != nil {
		fmt.Printf("Failed to create kubernetes client, %+v\n", err)
		os.Exit(1)
	}
...

I found it will break the existing UT codes of root command since no kube config is available during testing. I checked our codes and knative client codes, got two options:

1. The params in function NewAdminCommand(params ...pkg.AdminParams) actually is not used, we can change it and pass a fake pkg.AdminParams for UT codes, this function will be improved like:

// rootCmd represents the base command when called without any subcommands
func NewAdminCommand(p *pkg.AdminParams) *cobra.Command {
        if p == nil {
	    p = &pkg.AdminParams{}
        }
	if err := p.Initialize(); err != nil {
		fmt.Printf("Failed to create kubernetes client, %+v\n", err)
		os.Exit(1)
	}
...

• Pros:
  • Less codes to change
  • Won't impact other command codes and UT codes
• Cons:
  • The parameter p pkg.AdminParams is declared only for UT codes, we don't use it in other cases.
  • Some subcommands like version without a kubenets client still needs user to set a kubeconfig first.

2. Another option is like knative client: NewServingClient func(namespace string) (clientservingv1.KnServingClient, error)
   We can declare a member function of AdminParams to delay creating kubenets client interface till subcommand running. If we use this option:

• Pros:
  • We can remove p *pkg.AdminParams from NewAdminCommand function
  • It won't break existing UT codes of root command
  • Some subcommands like version, help can run successfully even if the kubectl context is not set
• Cons:
  • More codes to change
  • Almost all commands and their UT codes will be impacted since p.ClientSet is replaced by a function and won't be created in advance.

Which one do you prefer? Thanks!
BTW, you can use kubectl config unset current-context to reproduce the runtime error

[zhanggbj]

@chaozbj Thanks for the investigation. Briefly, option 2 sounds good to me.
For option 1, I think it doesn't make sense to ask the end-user to specify a Kubeconfig for version or help command.
For option 2, it is more reasonable to specify a Kubeconfig when it is really required, and it good to be consistent with kn client itself.
Even though there will be more code change, but I think it does worth it!
Thanks again for the nice summary 👍

[chaozbj]

@zhanggbj Ok, I also like option 2, I will do it, thanks!