From 1b0ba920feb4cc34b53f69a1df2a8a5cd34f6fe1 Mon Sep 17 00:00:00 2001 From: "Lubomir I. Ivanov" Date: Sat, 26 Jan 2019 15:36:04 +0200 Subject: [PATCH] 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 | 62 +++++++++---------- cmd/kubeadm/app/cmd/init_test.go | 4 +- cmd/kubeadm/app/cmd/join.go | 32 +++++----- cmd/kubeadm/app/cmd/join_test.go | 5 +- cmd/kubeadm/app/cmd/phases/certs_test.go | 4 +- .../app/cmd/phases/workflow/doc_test.go | 6 +- cmd/kubeadm/app/cmd/phases/workflow/runner.go | 16 ++--- .../app/cmd/phases/workflow/runner_test.go | 4 +- 8 files changed, 65 insertions(+), 68 deletions(-) diff --git a/cmd/kubeadm/app/cmd/init.go b/cmd/kubeadm/app/cmd/init.go index c38f37f55e..0a0f941374 100644 --- a/cmd/kubeadm/app/cmd/init.go +++ b/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) { - return newInitData(cmd, initOptions, out) + initRunner.SetDataInitializer(func(cmd *cobra.Command, args []string) (workflow.RunData, error) { + 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()) diff --git a/cmd/kubeadm/app/cmd/init_test.go b/cmd/kubeadm/app/cmd/init_test.go index 4c9e72dfa2..a6bc40ec69 100644 --- a/cmd/kubeadm/app/cmd/init_test.go +++ b/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) } }) } diff --git a/cmd/kubeadm/app/cmd/join.go b/cmd/kubeadm/app/cmd/join.go index d2c181513f..2706484f14 100644 --- a/cmd/kubeadm/app/cmd/join.go +++ b/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) { - return newJoinData(cmd, joinOptions, out) + joinRunner.SetDataInitializer(func(cmd *cobra.Command, args []string) (workflow.RunData, error) { + 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 { - klog.Warningf("[join] WARNING: More than one API server endpoint supplied on command line %v. Using the first one.", *options.args) + 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.", 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, diff --git a/cmd/kubeadm/app/cmd/join_test.go b/cmd/kubeadm/app/cmd/join_test.go index 5daca6ecba..9affd30cfd 100644 --- a/cmd/kubeadm/app/cmd/join_test.go +++ b/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) } }) } diff --git a/cmd/kubeadm/app/cmd/phases/certs_test.go b/cmd/kubeadm/app/cmd/phases/certs_test.go index f72208c5f3..dda406b9a2 100644 --- a/cmd/kubeadm/app/cmd/phases/certs_test.go +++ b/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) } }) diff --git a/cmd/kubeadm/app/cmd/phases/workflow/doc_test.go b/cmd/kubeadm/app/cmd/phases/workflow/doc_test.go index 47fb88b8f0..88123c4f1f 100644 --- a/cmd/kubeadm/app/cmd/phases/workflow/doc_test.go +++ b/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 - myWorkflowRunner.Run() + // Runs the workflow by passing a list of arguments + myWorkflowRunner.Run([]string{}) } diff --git a/cmd/kubeadm/app/cmd/phases/workflow/runner.go b/cmd/kubeadm/app/cmd/phases/workflow/runner.go index 255b32030d..cd4026d5d0 100644 --- a/cmd/kubeadm/app/cmd/phases/workflow/runner.go +++ b/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) } diff --git a/cmd/kubeadm/app/cmd/phases/workflow/runner_test.go b/cmd/kubeadm/app/cmd/phases/workflow/runner_test.go index 15ad369d07..1b65eb717f 100644 --- a/cmd/kubeadm/app/cmd/phases/workflow/runner_test.go +++ b/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) }