Skip to content

Commit ce6d65f

Browse files
authored
Merge pull request kubernetes#77400 from Klaven/arg_validation
updated phase runner to enable custom arg validation
2 parents ace60e8 + 2b69699 commit ce6d65f

File tree

6 files changed

+165
-15
lines changed

6 files changed

+165
-15
lines changed

cmd/kubeadm/app/cmd/phases/join/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ go_library(
3737
"//staging/src/k8s.io/client-go/util/cert:go_default_library",
3838
"//vendor/github.com/lithammer/dedent:go_default_library",
3939
"//vendor/github.com/pkg/errors:go_default_library",
40+
"//vendor/github.com/spf13/cobra:go_default_library",
4041
"//vendor/k8s.io/klog:go_default_library",
4142
"//vendor/k8s.io/utils/exec:go_default_library",
4243
],

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import (
2020
"fmt"
2121

2222
"github.com/pkg/errors"
23+
"github.com/spf13/cobra"
24+
2325
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
2426
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow"
2527
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
@@ -58,6 +60,7 @@ func NewControlPlaneJoinPhase() workflow.Phase {
5860
Short: "Join a machine as a control plane instance",
5961
InheritFlags: getControlPlaneJoinPhaseFlags("all"),
6062
RunAllSiblings: true,
63+
ArgsValidator: cobra.NoArgs,
6164
},
6265
newEtcdLocalSubphase(),
6366
newUpdateStatusSubphase(),
@@ -68,10 +71,11 @@ func NewControlPlaneJoinPhase() workflow.Phase {
6871

6972
func newEtcdLocalSubphase() workflow.Phase {
7073
return workflow.Phase{
71-
Name: "etcd",
72-
Short: "Add a new local etcd member",
73-
Run: runEtcdPhase,
74-
InheritFlags: getControlPlaneJoinPhaseFlags("etcd"),
74+
Name: "etcd",
75+
Short: "Add a new local etcd member",
76+
Run: runEtcdPhase,
77+
InheritFlags: getControlPlaneJoinPhaseFlags("etcd"),
78+
ArgsValidator: cobra.NoArgs,
7579
}
7680
}
7781

@@ -83,17 +87,19 @@ func newUpdateStatusSubphase() workflow.Phase {
8387
kubeadmconstants.ClusterStatusConfigMapKey,
8488
kubeadmconstants.KubeadmConfigConfigMap,
8589
),
86-
Run: runUpdateStatusPhase,
87-
InheritFlags: getControlPlaneJoinPhaseFlags("update-status"),
90+
Run: runUpdateStatusPhase,
91+
InheritFlags: getControlPlaneJoinPhaseFlags("update-status"),
92+
ArgsValidator: cobra.NoArgs,
8893
}
8994
}
9095

9196
func newMarkControlPlaneSubphase() workflow.Phase {
9297
return workflow.Phase{
93-
Name: "mark-control-plane",
94-
Short: "Mark a node as a control-plane",
95-
Run: runMarkControlPlanePhase,
96-
InheritFlags: getControlPlaneJoinPhaseFlags("mark-control-plane"),
98+
Name: "mark-control-plane",
99+
Short: "Mark a node as a control-plane",
100+
Run: runMarkControlPlanePhase,
101+
InheritFlags: getControlPlaneJoinPhaseFlags("mark-control-plane"),
102+
ArgsValidator: cobra.NoArgs,
97103
}
98104
}
99105

cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"fmt"
2121

2222
"github.com/pkg/errors"
23+
"github.com/spf13/cobra"
2324

2425
clientset "k8s.io/client-go/kubernetes"
2526
"k8s.io/klog"
@@ -157,10 +158,11 @@ func newControlPlanePrepareKubeconfigSubphase() workflow.Phase {
157158

158159
func newControlPlanePrepareControlPlaneSubphase() workflow.Phase {
159160
return workflow.Phase{
160-
Name: "control-plane",
161-
Short: "Generate the manifests for the new control plane components",
162-
Run: runControlPlanePrepareControlPlaneSubphase, //NB. eventually in future we would like to break down this in sub phases for each component
163-
InheritFlags: getControlPlanePreparePhaseFlags("control-plane"),
161+
Name: "control-plane",
162+
Short: "Generate the manifests for the new control plane components",
163+
Run: runControlPlanePrepareControlPlaneSubphase, //NB. eventually in future we would like to break down this in sub phases for each component
164+
InheritFlags: getControlPlanePreparePhaseFlags("control-plane"),
165+
ArgsValidator: cobra.NoArgs,
164166
}
165167
}
166168

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ limitations under the License.
1616

1717
package workflow
1818

19-
import "github.com/spf13/pflag"
19+
import (
20+
"github.com/spf13/cobra"
21+
"github.com/spf13/pflag"
22+
)
2023

2124
// Phase provides an implementation of a workflow phase that allows
2225
// creation of new phases by simply instantiating a variable of this type.
@@ -71,6 +74,10 @@ type Phase struct {
7174
// Nb. if two or phases have the same local flags, please consider using local flags in the parent command
7275
// or additional flags defined in the phase runner.
7376
LocalFlags *pflag.FlagSet
77+
78+
// ArgsValidator defines the positional arg function to be used for validating args for this phase
79+
// If not set a phase will adopt the args of the top level command.
80+
ArgsValidator cobra.PositionalArgs
7481
}
7582

7683
// AppendPhase adds the given phase to the nested, ordered sequence of phases.

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,12 @@ func (e *Runner) BindToCommand(cmd *cobra.Command) {
372372
// if this phase has children (not a leaf) it doesn't accept any args
373373
if len(p.Phases) > 0 {
374374
phaseCmd.Args = cobra.NoArgs
375+
} else {
376+
if p.ArgsValidator == nil {
377+
phaseCmd.Args = cmd.Args
378+
} else {
379+
phaseCmd.Args = p.ArgsValidator
380+
}
375381
}
376382

377383
// adds the command to parent

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

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,134 @@ func phaseBuilder5(name string, flags *pflag.FlagSet) Phase {
297297
}
298298
}
299299

300+
type argTest struct {
301+
args cobra.PositionalArgs
302+
pass []string
303+
fail []string
304+
}
305+
306+
func phaseBuilder6(name string, args cobra.PositionalArgs, phases ...Phase) Phase {
307+
return Phase{
308+
Name: name,
309+
Short: fmt.Sprintf("long description for %s ...", name),
310+
Phases: phases,
311+
ArgsValidator: args,
312+
}
313+
}
314+
315+
// customArgs is a custom cobra.PositionArgs function
316+
func customArgs(cmd *cobra.Command, args []string) error {
317+
for _, a := range args {
318+
if a != "qux" {
319+
return fmt.Errorf("arg %s does not equal qux", a)
320+
}
321+
}
322+
return nil
323+
}
324+
325+
func TestBindToCommandArgRequirements(t *testing.T) {
326+
327+
// because cobra.ExactArgs(1) == cobra.ExactArgs(3), it is needed
328+
// to run test argument sets that both pass and fail to ensure the correct function was set.
329+
var usecases = []struct {
330+
name string
331+
runner Runner
332+
testCases map[string]argTest
333+
cmd *cobra.Command
334+
}{
335+
{
336+
name: "leaf command, no defined args, follow parent",
337+
runner: Runner{
338+
Phases: []Phase{phaseBuilder("foo")},
339+
},
340+
testCases: map[string]argTest{
341+
"phase foo": {
342+
pass: []string{"one", "two", "three"},
343+
fail: []string{"one", "two"},
344+
args: cobra.ExactArgs(3),
345+
},
346+
},
347+
cmd: &cobra.Command{
348+
Use: "init",
349+
Args: cobra.ExactArgs(3),
350+
},
351+
},
352+
{
353+
name: "container cmd expect none, custom arg check for leaf",
354+
runner: Runner{
355+
Phases: []Phase{phaseBuilder6("foo", cobra.NoArgs,
356+
phaseBuilder6("bar", cobra.ExactArgs(1)),
357+
phaseBuilder6("baz", customArgs),
358+
)},
359+
},
360+
testCases: map[string]argTest{
361+
"phase foo": {
362+
pass: []string{},
363+
fail: []string{"one"},
364+
args: cobra.NoArgs,
365+
},
366+
"phase foo bar": {
367+
pass: []string{"one"},
368+
fail: []string{"one", "two"},
369+
args: cobra.ExactArgs(1),
370+
},
371+
"phase foo baz": {
372+
pass: []string{"qux"},
373+
fail: []string{"one"},
374+
args: customArgs,
375+
},
376+
},
377+
cmd: &cobra.Command{
378+
Use: "init",
379+
Args: cobra.NoArgs,
380+
},
381+
},
382+
}
383+
384+
for _, rt := range usecases {
385+
t.Run(rt.name, func(t *testing.T) {
386+
387+
rt.runner.BindToCommand(rt.cmd)
388+
389+
// Checks that cmd gets a new phase subcommand
390+
phaseCmd := getCmd(rt.cmd, "phase")
391+
if phaseCmd == nil {
392+
t.Error("cmd didn't have phase subcommand\n")
393+
return
394+
}
395+
396+
for c, args := range rt.testCases {
397+
398+
cCmd := getCmd(rt.cmd, c)
399+
if cCmd == nil {
400+
t.Errorf("cmd didn't have %s subcommand\n", c)
401+
continue
402+
}
403+
404+
// Ensure it is the expected function
405+
if reflect.ValueOf(cCmd.Args).Pointer() != reflect.ValueOf(args.args).Pointer() {
406+
t.Error("The function poiners where not equal.")
407+
}
408+
409+
// Test passing argument set
410+
err := cCmd.Args(cCmd, args.pass)
411+
412+
if err != nil {
413+
t.Errorf("command %s should validate the args: %v\n %v", cCmd.Name(), args.pass, err)
414+
}
415+
416+
// Test failing argument set
417+
err = cCmd.Args(cCmd, args.fail)
418+
419+
if err == nil {
420+
t.Errorf("command %s should fail to validate the args: %v\n %v", cCmd.Name(), args.pass, err)
421+
}
422+
}
423+
424+
})
425+
}
426+
}
427+
300428
func TestBindToCommand(t *testing.T) {
301429

302430
var dummy string

0 commit comments

Comments
 (0)