kubeadm: fix a couple of problems related to initData/joinData

Fix a couple of problems related to data used by the phases runners
of `init` and `join`.

1) make `newInitData()` and `newJoinData()` return pointers.

Methods of the data objects returned by these functions should
be able to modify fields in the data objects - e.g.
`func (d initData) Client()`. This allows us to store a state and
not execute the same logic multiple times - e.g. obtaining a client.

A side effect of this change is that the `new...` functions must return
pointers, so that casting the data object in a phase, from
`workflow.RunData` to a locally defined interface, works.

2) Make it possible to pass arguments from a parent command
to a sub-phase with regards to data initialization.

cmd/kubeadm/app/cmd/init.go

@@ -136,16 +136,16 @@ func NewCmdInit(out io.Writer, initOptions *initOptions) *cobra.Command {
 		Use:   "init",
 		Short: "Run this command in order to set up the Kubernetes control plane.",
 		Run: func(cmd *cobra.Command, args []string) {
-			c, err := initRunner.InitData()
+			c, err := initRunner.InitData(args)
 			kubeadmutil.CheckErr(err)
 
-			data := c.(initData)
+			data := c.(*initData)
 			fmt.Printf("[init] Using Kubernetes version: %s\n", data.cfg.KubernetesVersion)
 
-			err = initRunner.Run()
+			err = initRunner.Run(args)
 			kubeadmutil.CheckErr(err)
 
-			err = showJoinCommand(&data, out)
+			err = showJoinCommand(data, out)
 			kubeadmutil.CheckErr(err)
 		},
 	}
@@ -181,8 +181,8 @@ func NewCmdInit(out io.Writer, initOptions *initOptions) *cobra.Command {
 
 	// sets the data builder function, that will be used by the runner
 	// both when running the entire workflow or single phases
-	initRunner.SetDataInitializer(func(cmd *cobra.Command) (workflow.RunData, error) {
+	initRunner.SetDataInitializer(func(cmd *cobra.Command, args []string) (workflow.RunData, error) {
-		return newInitData(cmd, initOptions, out)
+		return newInitData(cmd, args, initOptions, out)
 	})
 
 	// binds the Runner to kubeadm init command by altering
@@ -278,7 +278,7 @@ func newInitOptions() *initOptions {
 // newInitData returns a new initData struct to be used for the execution of the kubeadm init workflow.
 // This func takes care of validating initOptions passed to the command, and then it converts
 // options into the internal InitConfiguration type that is used as input all the phases in the kubeadm init workflow
-func newInitData(cmd *cobra.Command, options *initOptions, out io.Writer) (initData, error) {
+func newInitData(cmd *cobra.Command, args []string, options *initOptions, out io.Writer) (*initData, error) {
 	// Re-apply defaults to the public kubeadm API (this will set only values not exposed/not set as a flags)
 	kubeadmscheme.Scheme.Default(options.externalcfg)
 
@@ -286,27 +286,27 @@ func newInitData(cmd *cobra.Command, options *initOptions, out io.Writer) (initD
 	// validated values to the public kubeadm config API when applicable
 	var err error
 	if options.externalcfg.FeatureGates, err = features.NewFeatureGate(&features.InitFeatureGates, options.featureGatesString); err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 
 	ignorePreflightErrorsSet, err := validation.ValidateIgnorePreflightErrors(options.ignorePreflightErrors)
 	if err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 
 	if err = validation.ValidateMixedArguments(cmd.Flags()); err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 
 	if err = options.bto.ApplyTo(options.externalcfg); err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 
 	// Either use the config file if specified, or convert public kubeadm API to the internal InitConfiguration
 	// and validates InitConfiguration
 	cfg, err := configutil.ConfigFileAndDefaultsToInternalConfig(options.cfgPath, options.externalcfg)
 	if err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 
 	// override node name and CRI socket from the command line options
@@ -318,17 +318,17 @@ func newInitData(cmd *cobra.Command, options *initOptions, out io.Writer) (initD
 	}
 
 	if err := configutil.VerifyAPIServerBindAddress(cfg.LocalAPIEndpoint.AdvertiseAddress); err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 	if err := features.ValidateVersion(features.InitFeatureGates, cfg.FeatureGates, cfg.KubernetesVersion); err != nil {
-		return initData{}, err
+		return &initData{}, err
 	}
 
 	// if dry running creates a temporary folder for saving kubeadm generated files
 	dryRunDir := ""
 	if options.dryRun {
 		if dryRunDir, err = ioutil.TempDir("", "kubeadm-init-dryrun"); err != nil {
-			return initData{}, errors.Wrap(err, "couldn't create a temporary directory")
+			return &initData{}, errors.Wrap(err, "couldn't create a temporary directory")
 		}
 	}
 
@@ -340,11 +340,11 @@ func newInitData(cmd *cobra.Command, options *initOptions, out io.Writer) (initD
 			kubeconfigDir = dryRunDir
 		}
 		if err := kubeconfigphase.ValidateKubeconfigsForExternalCA(kubeconfigDir, cfg); err != nil {
-			return initData{}, err
+			return &initData{}, err
 		}
 	}
 
-	return initData{
+	return &initData{
 		cfg:                   cfg,
 		certificatesDir:       cfg.CertificatesDir,
 		skipTokenPrint:        options.skipTokenPrint,
@@ -359,27 +359,27 @@ func newInitData(cmd *cobra.Command, options *initOptions, out io.Writer) (initD
 }
 
 // Cfg returns initConfiguration.
-func (d initData) Cfg() *kubeadmapi.InitConfiguration {
+func (d *initData) Cfg() *kubeadmapi.InitConfiguration {
 	return d.cfg
 }
 
 // DryRun returns the DryRun flag.
-func (d initData) DryRun() bool {
+func (d *initData) DryRun() bool {
 	return d.dryRun
 }
 
 // SkipTokenPrint returns the SkipTokenPrint flag.
-func (d initData) SkipTokenPrint() bool {
+func (d *initData) SkipTokenPrint() bool {
 	return d.skipTokenPrint
 }
 
 // IgnorePreflightErrors returns the IgnorePreflightErrors flag.
-func (d initData) IgnorePreflightErrors() sets.String {
+func (d *initData) IgnorePreflightErrors() sets.String {
 	return d.ignorePreflightErrors
 }
 
 // CertificateWriteDir returns the path to the certificate folder or the temporary folder path in case of DryRun.
-func (d initData) CertificateWriteDir() string {
+func (d *initData) CertificateWriteDir() string {
 	if d.dryRun {
 		return d.dryRunDir
 	}
@@ -387,12 +387,12 @@ func (d initData) CertificateWriteDir() string {
 }
 
 // CertificateDir returns the CertificateDir as originally specified by the user.
-func (d initData) CertificateDir() string {
+func (d *initData) CertificateDir() string {
 	return d.certificatesDir
 }
 
 // KubeConfigDir returns the path of the Kubernetes configuration folder or the temporary folder path in case of DryRun.
-func (d initData) KubeConfigDir() string {
+func (d *initData) KubeConfigDir() string {
 	if d.dryRun {
 		return d.dryRunDir
 	}
@@ -400,7 +400,7 @@ func (d initData) KubeConfigDir() string {
 }
 
 // KubeConfigPath returns the path to the kubeconfig file to use for connecting to Kubernetes
-func (d initData) KubeConfigPath() string {
+func (d *initData) KubeConfigPath() string {
 	if d.dryRun {
 		d.kubeconfigPath = filepath.Join(d.dryRunDir, kubeadmconstants.AdminKubeConfigFileName)
 	}
@@ -408,7 +408,7 @@ func (d initData) KubeConfigPath() string {
 }
 
 // ManifestDir returns the path where manifest should be stored or the temporary folder path in case of DryRun.
-func (d initData) ManifestDir() string {
+func (d *initData) ManifestDir() string {
 	if d.dryRun {
 		return d.dryRunDir
 	}
@@ -416,7 +416,7 @@ func (d initData) ManifestDir() string {
 }
 
 // KubeletDir returns path of the kubelet configuration folder or the temporary folder in case of DryRun.
-func (d initData) KubeletDir() string {
+func (d *initData) KubeletDir() string {
 	if d.dryRun {
 		return d.dryRunDir
 	}
@@ -424,19 +424,19 @@ func (d initData) KubeletDir() string {
 }
 
 // ExternalCA returns true if an external CA is provided by the user.
-func (d initData) ExternalCA() bool {
+func (d *initData) ExternalCA() bool {
 	return d.externalCA
 }
 
 // OutputWriter returns the io.Writer used to write output to by this command.
-func (d initData) OutputWriter() io.Writer {
+func (d *initData) OutputWriter() io.Writer {
 	return d.outputWriter
 }
 
 // Client returns a Kubernetes client to be used by kubeadm.
 // This function is implemented as a singleton, thus avoiding to recreate the client when it is used by different phases.
 // Important. This function must be called after the admin.conf kubeconfig file is created.
-func (d initData) Client() (clientset.Interface, error) {
+func (d *initData) Client() (clientset.Interface, error) {
 	if d.client == nil {
 		if d.dryRun {
 			// If we're dry-running; we should create a faked client that answers some GETs in order to be able to do the full init flow and just logs the rest of requests
@@ -455,7 +455,7 @@ func (d initData) Client() (clientset.Interface, error) {
 }
 
 // Tokens returns an array of token strings.
-func (d initData) Tokens() []string {
+func (d *initData) Tokens() []string {
 	tokens := []string{}
 	for _, bt := range d.cfg.BootstrapTokens {
 		tokens = append(tokens, bt.Token.String())

cmd/kubeadm/app/cmd/init_test.go

@@ -142,7 +142,7 @@ func TestNewInitData(t *testing.T) {
 			}
 
 			// test newInitData method
-			data, err := newInitData(cmd, initOptions, nil)
+			data, err := newInitData(cmd, tc.args, initOptions, nil)
 			if err != nil && !tc.expectError {
 				t.Fatalf("newInitData returned unexpected error: %v", err)
 			}
@@ -152,7 +152,7 @@ func TestNewInitData(t *testing.T) {
 
 			// exec additional validation on the returned value
 			if tc.validate != nil {
-				tc.validate(t, &data)
+				tc.validate(t, data)
 			}
 		})
 	}

cmd/kubeadm/app/cmd/join.go

@@ -159,7 +159,6 @@ var (
 // Please note that this structure includes the public kubeadm config API, but only a subset of the options
 // supported by this api will be exposed as a flag.
 type joinOptions struct {
-	args                  *[]string
 	cfgPath               string
 	token                 string
 	controlPlane          bool
@@ -191,17 +190,16 @@ func NewCmdJoin(out io.Writer, joinOptions *joinOptions) *cobra.Command {
 		Short: "Run this on any machine you wish to join an existing cluster",
 		Long:  joinLongDescription,
 		Run: func(cmd *cobra.Command, args []string) {
-			joinOptions.args = &args
 
-			c, err := joinRunner.InitData()
+			c, err := joinRunner.InitData(args)
 			kubeadmutil.CheckErr(err)
 
-			err = joinRunner.Run()
+			err = joinRunner.Run(args)
 			kubeadmutil.CheckErr(err)
 
 			// TODO: remove this once we have all phases in place.
 			// the method joinData.Run() itself should be removed too.
-			data := c.(joinData)
+			data := c.(*joinData)
 			err = data.Run()
 			kubeadmutil.CheckErr(err)
 		},
@@ -216,8 +214,8 @@ func NewCmdJoin(out io.Writer, joinOptions *joinOptions) *cobra.Command {
 
 	// sets the data builder function, that will be used by the runner
 	// both when running the entire workflow or single phases
-	joinRunner.SetDataInitializer(func(cmd *cobra.Command) (workflow.RunData, error) {
+	joinRunner.SetDataInitializer(func(cmd *cobra.Command, args []string) (workflow.RunData, error) {
-		return newJoinData(cmd, joinOptions, out)
+		return newJoinData(cmd, args, joinOptions, out)
 	})
 
 	// binds the Runner to kubeadm join command by altering
@@ -321,7 +319,7 @@ func newJoinOptions() *joinOptions {
 // newJoinData returns a new joinData struct to be used for the execution of the kubeadm join workflow.
 // This func takes care of validating joinOptions passed to the command, and then it converts
 // options into the internal JoinConfiguration type that is used as input all the phases in the kubeadm join workflow
-func newJoinData(cmd *cobra.Command, options *joinOptions, out io.Writer) (joinData, error) {
+func newJoinData(cmd *cobra.Command, args []string, options *joinOptions, out io.Writer) (*joinData, error) {
 	// Re-apply defaults to the public kubeadm API (this will set only values not exposed/not set as a flags)
 	kubeadmscheme.Scheme.Default(options.externalcfg)
 
@@ -344,13 +342,13 @@ func newJoinData(cmd *cobra.Command, options *joinOptions, out io.Writer) (joinD
 	}
 
 	// if an APIServerEndpoint from which to retrive cluster information was not provided, unset the Discovery.BootstrapToken object
-	if len(*options.args) == 0 {
+	if len(args) == 0 {
 		options.externalcfg.Discovery.BootstrapToken = nil
 	} else {
-		if len(options.cfgPath) == 0 && len(*options.args) > 1 {
+		if len(options.cfgPath) == 0 && len(args) > 1 {
-			klog.Warningf("[join] WARNING: More than one API server endpoint supplied on command line %v. Using the first one.", *options.args)
+			klog.Warningf("[join] WARNING: More than one API server endpoint supplied on command line %v. Using the first one.", args)
 		}
-		options.externalcfg.Discovery.BootstrapToken.APIServerEndpoint = (*options.args)[0]
+		options.externalcfg.Discovery.BootstrapToken.APIServerEndpoint = args[0]
 	}
 
 	// if not joining a control plane, unset the ControlPlane object
@@ -360,11 +358,11 @@ func newJoinData(cmd *cobra.Command, options *joinOptions, out io.Writer) (joinD
 
 	ignorePreflightErrorsSet, err := validation.ValidateIgnorePreflightErrors(options.ignorePreflightErrors)
 	if err != nil {
-		return joinData{}, err
+		return &joinData{}, err
 	}
 
 	if err = validation.ValidateMixedArguments(cmd.Flags()); err != nil {
-		return joinData{}, err
+		return &joinData{}, err
 	}
 
 	// Either use the config file if specified, or convert public kubeadm API to the internal JoinConfiguration
@@ -379,7 +377,7 @@ func newJoinData(cmd *cobra.Command, options *joinOptions, out io.Writer) (joinD
 
 	cfg, err := configutil.JoinConfigFileAndDefaultsToInternalConfig(options.cfgPath, options.externalcfg)
 	if err != nil {
-		return joinData{}, err
+		return &joinData{}, err
 	}
 
 	// override node name and CRI socket from the command line options
@@ -392,11 +390,11 @@ func newJoinData(cmd *cobra.Command, options *joinOptions, out io.Writer) (joinD
 
 	if cfg.ControlPlane != nil {
 		if err := configutil.VerifyAPIServerBindAddress(cfg.ControlPlane.LocalAPIEndpoint.AdvertiseAddress); err != nil {
-			return joinData{}, err
+			return &joinData{}, err
 		}
 	}
 
-	return joinData{
+	return &joinData{
 		cfg:                   cfg,
 		ignorePreflightErrors: ignorePreflightErrorsSet,
 		outputWriter:          out,

cmd/kubeadm/app/cmd/join_test.go

@@ -220,7 +220,6 @@ func TestNewJoinData(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			// initialize an external join option and inject it to the join cmd
 			joinOptions := newJoinOptions()
-			joinOptions.args = &tc.args
 			cmd := NewCmdJoin(nil, joinOptions)
 
 			// sets cmd flags (that will be reflected on the join options)
@@ -229,7 +228,7 @@ func TestNewJoinData(t *testing.T) {
 			}
 
 			// test newJoinData method
-			data, err := newJoinData(cmd, joinOptions, nil)
+			data, err := newJoinData(cmd, tc.args, joinOptions, nil)
 			if err != nil && !tc.expectError {
 				t.Fatalf("newJoinData returned unexpected error: %v", err)
 			}
@@ -239,7 +238,7 @@ func TestNewJoinData(t *testing.T) {
 
 			// exec additional validation on the returned value
 			if tc.validate != nil {
-				tc.validate(t, &data)
+				tc.validate(t, data)
 			}
 		})
 	}

cmd/kubeadm/app/cmd/phases/certs_test.go

@@ -91,7 +91,7 @@ func TestCreateSparseCerts(t *testing.T) {
 
 			r := workflow.NewRunner()
 			r.AppendPhase(NewCertsPhase())
-			r.SetDataInitializer(func(*cobra.Command) (workflow.RunData, error) {
+			r.SetDataInitializer(func(*cobra.Command, []string) (workflow.RunData, error) {
 				certsData := &testCertsData{
 					cfg: testutil.GetDefaultInternalConfig(t),
 				}
@@ -99,7 +99,7 @@ func TestCreateSparseCerts(t *testing.T) {
 				return certsData, nil
 			})
 
-			if err := r.Run(); (err != nil) != test.ExpectError {
+			if err := r.Run([]string{}); (err != nil) != test.ExpectError {
 				t.Fatalf("expected error to be %t, got %t (%v)", test.ExpectError, (err != nil), err)
 			}
 		})

cmd/kubeadm/app/cmd/phases/workflow/doc_test.go

@@ -102,10 +102,10 @@ func ExampleRunner_Run() {
 
 	// Defines the method that creates the runtime data shared
 	// among all the phases included in the workflow
-	myWorkflowRunner.SetDataInitializer(func(cmd *cobra.Command) (RunData, error) {
+	myWorkflowRunner.SetDataInitializer(func(cmd *cobra.Command, args []string) (RunData, error) {
 		return myWorkflowData{data: "some data"}, nil
 	})
 
-	// Runs the workflow
+	// Runs the workflow by passing a list of arguments
-	myWorkflowRunner.Run()
+	myWorkflowRunner.Run([]string{})
 }

cmd/kubeadm/app/cmd/phases/workflow/runner.go

@@ -53,7 +53,7 @@ type Runner struct {
 
 	// runDataInitializer defines a function that creates the runtime data shared
 	// among all the phases included in the workflow
-	runDataInitializer func(*cobra.Command) (RunData, error)
+	runDataInitializer func(*cobra.Command, []string) (RunData, error)
 
 	// runData is part of the internal state of the runner and it is used for implementing
 	// a singleton in the InitData methods (thus avoiding to initialize data
@@ -171,17 +171,17 @@ func (e *Runner) computePhaseRunFlags() (map[string]bool, error) {
 // SetDataInitializer allows to setup a function that initialize the runtime data shared
 // among all the phases included in the workflow.
 // The method will receive in input the cmd that triggers the Runner (only if the runner is BindToCommand)
-func (e *Runner) SetDataInitializer(builder func(cmd *cobra.Command) (RunData, error)) {
+func (e *Runner) SetDataInitializer(builder func(cmd *cobra.Command, args []string) (RunData, error)) {
 	e.runDataInitializer = builder
 }
 
 // InitData triggers the creation of runtime data shared among all the phases included in the workflow.
 // This action can be executed explicitly out, when it is necessary to get the RunData
 // before actually executing Run, or implicitly when invoking Run.
-func (e *Runner) InitData() (RunData, error) {
+func (e *Runner) InitData(args []string) (RunData, error) {
 	if e.runData == nil && e.runDataInitializer != nil {
 		var err error
-		if e.runData, err = e.runDataInitializer(e.runCmd); err != nil {
+		if e.runData, err = e.runDataInitializer(e.runCmd, args); err != nil {
 			return nil, err
 		}
 	}
@@ -190,7 +190,7 @@ func (e *Runner) InitData() (RunData, error) {
 }
 
 // Run the kubeadm composable kubeadm workflows.
-func (e *Runner) Run() error {
+func (e *Runner) Run(args []string) error {
 	e.prepareForExecution()
 
 	// determine which phase should be run according to RunnerOptions
@@ -201,7 +201,7 @@ func (e *Runner) Run() error {
 
 	// builds the runner data
 	var data RunData
-	if data, err = e.InitData(); err != nil {
+	if data, err = e.InitData(args); err != nil {
 		return err
 	}
 
@@ -309,7 +309,7 @@ func (e *Runner) BindToCommand(cmd *cobra.Command) {
 	phaseCommand := &cobra.Command{
 		Use:   "phase",
 		Short: fmt.Sprintf("use this command to invoke single phase of the %s workflow", cmd.Name()),
-		Args:  cobra.NoArgs, // this forces cobra to fail if a wrong phase name is passed
+		// TODO: this logic is currently lacking verification if a suphase name is valid!
 	}
 
 	cmd.AddCommand(phaseCommand)
@@ -347,7 +347,7 @@ func (e *Runner) BindToCommand(cmd *cobra.Command) {
 				// overrides the command triggering the Runner using the phaseCmd
 				e.runCmd = cmd
 				e.Options.FilterPhases = []string{phaseSelector}
-				if err := e.Run(); err != nil {
+				if err := e.Run(args); err != nil {
 					fmt.Fprintln(os.Stderr, err)
 					os.Exit(1)
 				}

cmd/kubeadm/app/cmd/phases/workflow/runner_test.go

@@ -172,7 +172,7 @@ func TestRunOrderAndConditions(t *testing.T) {
 		t.Run(u.name, func(t *testing.T) {
 			callstack = []string{}
 			w.Options = u.options
-			err := w.Run()
+			err := w.Run([]string{})
 			if err != nil {
 				t.Errorf("Unexpected error: %v", err)
 			}
@@ -241,7 +241,7 @@ func TestRunHandleErrors(t *testing.T) {
 	for _, u := range usecases {
 		t.Run(u.name, func(t *testing.T) {
 			w.Options = u.options
-			err := w.Run()
+			err := w.Run([]string{})
 			if (err != nil) != u.expectedError {
 				t.Errorf("Unexpected error: %v", err)
 			}