Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

updated phase runner to enable custom arg validation #77400

Merged
merged 1 commit into from
May 12, 2019
Merged

updated phase runner to enable custom arg validation #77400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/kubeadm/app/cmd/phases/join/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ go_library(
"//staging/src/k8s.io/client-go/util/cert:go_default_library",
"//vendor/github.com/lithammer/dedent:go_default_library",
"//vendor/github.com/pkg/errors:go_default_library",
"//vendor/github.com/spf13/cobra:go_default_library",
"//vendor/k8s.io/klog:go_default_library",
"//vendor/k8s.io/utils/exec:go_default_library",
],
Expand Down
26 changes: 16 additions & 10 deletions cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"fmt"

"github.com/pkg/errors"
"github.com/spf13/cobra"

"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
Expand Down Expand Up @@ -58,6 +60,7 @@ func NewControlPlaneJoinPhase() workflow.Phase {
Short: "Join a machine as a control plane instance",
InheritFlags: getControlPlaneJoinPhaseFlags("all"),
RunAllSiblings: true,
ArgsValidator: cobra.NoArgs,
},
newEtcdLocalSubphase(),
newUpdateStatusSubphase(),
Expand All @@ -68,10 +71,11 @@ func NewControlPlaneJoinPhase() workflow.Phase {

func newEtcdLocalSubphase() workflow.Phase {
return workflow.Phase{
Name: "etcd",
Short: "Add a new local etcd member",
Run: runEtcdPhase,
InheritFlags: getControlPlaneJoinPhaseFlags("etcd"),
Name: "etcd",
Short: "Add a new local etcd member",
Run: runEtcdPhase,
InheritFlags: getControlPlaneJoinPhaseFlags("etcd"),
ArgsValidator: cobra.NoArgs,
}
}

Expand All @@ -83,17 +87,19 @@ func newUpdateStatusSubphase() workflow.Phase {
kubeadmconstants.ClusterStatusConfigMapKey,
kubeadmconstants.KubeadmConfigConfigMap,
),
Run: runUpdateStatusPhase,
InheritFlags: getControlPlaneJoinPhaseFlags("update-status"),
Run: runUpdateStatusPhase,
InheritFlags: getControlPlaneJoinPhaseFlags("update-status"),
ArgsValidator: cobra.NoArgs,
}
}

func newMarkControlPlaneSubphase() workflow.Phase {
return workflow.Phase{
Name: "mark-control-plane",
Short: "Mark a node as a control-plane",
Run: runMarkControlPlanePhase,
InheritFlags: getControlPlaneJoinPhaseFlags("mark-control-plane"),
Name: "mark-control-plane",
Short: "Mark a node as a control-plane",
Run: runMarkControlPlanePhase,
InheritFlags: getControlPlaneJoinPhaseFlags("mark-control-plane"),
ArgsValidator: cobra.NoArgs,
}
}

Expand Down
10 changes: 6 additions & 4 deletions cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"

"github.com/pkg/errors"
"github.com/spf13/cobra"

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

func newControlPlanePrepareControlPlaneSubphase() workflow.Phase {
return workflow.Phase{
Name: "control-plane",
Short: "Generate the manifests for the new control plane components",
Run: runControlPlanePrepareControlPlaneSubphase, //NB. eventually in future we would like to break down this in sub phases for each component
InheritFlags: getControlPlanePreparePhaseFlags("control-plane"),
Name: "control-plane",
Short: "Generate the manifests for the new control plane components",
Run: runControlPlanePrepareControlPlaneSubphase, //NB. eventually in future we would like to break down this in sub phases for each component
InheritFlags: getControlPlanePreparePhaseFlags("control-plane"),
ArgsValidator: cobra.NoArgs,
}
}

Expand Down
9 changes: 8 additions & 1 deletion cmd/kubeadm/app/cmd/phases/workflow/phase.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ limitations under the License.

package workflow

import "github.com/spf13/pflag"
import (
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

// Phase provides an implementation of a workflow phase that allows
// creation of new phases by simply instantiating a variable of this type.
Expand Down Expand Up @@ -71,6 +74,10 @@ type Phase struct {
// Nb. if two or phases have the same local flags, please consider using local flags in the parent command
// or additional flags defined in the phase runner.
LocalFlags *pflag.FlagSet

// ArgsValidator defines the positional arg function to be used for validating args for this phase
// If not set a phase will adopt the args of the top level command.
ArgsValidator cobra.PositionalArgs
}

// AppendPhase adds the given phase to the nested, ordered sequence of phases.
Expand Down
6 changes: 6 additions & 0 deletions cmd/kubeadm/app/cmd/phases/workflow/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,12 @@ func (e *Runner) BindToCommand(cmd *cobra.Command) {
// if this phase has children (not a leaf) it doesn't accept any args
if len(p.Phases) > 0 {
phaseCmd.Args = cobra.NoArgs
} else {
if p.ArgsValidator == nil {
phaseCmd.Args = cmd.Args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to do this? I am not a particular fan of imposing cobra.MaximumNArgs(1) in this way.

Copy link
Contributor Author

@Klaven Klaven May 9, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? I'm not sure I understand this. What I mean to accomplish here is if you did not set any Args in the phase, to just use the parent commands arg checker. right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I believe this is what @fabriziopandini wanted kubernetes/kubeadm#1375 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just took the join case, for init this would be cobra.NoArgs. This means that we need to test all the phases to check for CLI breakages (especially the init phases).

The original proposal by @fabriziopandini (in kubernetes/kubeadm#1375) states:

More specifically, we would like to have leaf phases without discovery flags using Args: cobra.NoArgs (while the top-level command/all the other leaf phases will use Args: cobra.MaximumNArgs(1))

I agree with this, except for the fact, that inits top level cmd should use cobra.NoArgs too (which it does ATM).

The way you do it gets the job done, but I distaste the implicit behavior of this. Without full test coverage for all possible phases it can can cause problems in the future.
What I think is a bit more appropriate with respect to the above concern is to make stuff explicit - don't inherit anything from the parent command, but explicitly add the positional args verifier per phase. This will make the things more verbose though and the patch larger in size.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand @rosti point that implicit behavior is ... not explicit when you read the code

I considered three options
a) to apply by default the parent arg validation of parent phase to leaf phases (current implementation)
b) to apply a predefined default to all phases e.g. NoArgs
c) to not apply any default for leaf phases, but to set args validation only if requested

The option c) goes in the direction of being explicit, but in the end, you have to set args for all phases, and I don't like the idea to add more knobs to all the phases (we already have eeetooomuch flags)

I discarded b) because choosing a default is tricky (why NoArgs instead of something else?)

So I'm back again to a), the current implementation, that IMO makes sense because phases are pieces of the main command, and so it is very likely that the args value of the parent command will make sense for leaf phases as well. In practices:

  • init has noArgs, and all the init phases will get NoArgs.
  • join has MaximumNArgs(1), this makes sense for many join phases as well, but you can set NoArgs by exception where this does not make sense: control-plane-prepare/control-plane, control-plane-join/*

That means that with this approach we are changing only 4 phases over 20/30 phases currently in place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 4 select phases have been updated to NoArg I will squash and push when you all give the word. (if you like it that is.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's go with inheriting then.
+1 for some of the control-plane join phases to use NoArgs.

} else {
phaseCmd.Args = p.ArgsValidator
}
}

// adds the command to parent
Expand Down
128 changes: 128 additions & 0 deletions cmd/kubeadm/app/cmd/phases/workflow/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,134 @@ func phaseBuilder5(name string, flags *pflag.FlagSet) Phase {
}
}

type argTest struct {
args cobra.PositionalArgs
pass []string
fail []string
}

func phaseBuilder6(name string, args cobra.PositionalArgs, phases ...Phase) Phase {
return Phase{
Name: name,
Short: fmt.Sprintf("long description for %s ...", name),
Phases: phases,
ArgsValidator: args,
}
}

// customArgs is a custom cobra.PositionArgs function
func customArgs(cmd *cobra.Command, args []string) error {
for _, a := range args {
if a != "qux" {
return fmt.Errorf("arg %s does not equal qux", a)
}
}
return nil
}

func TestBindToCommandArgRequirements(t *testing.T) {

// because cobra.ExactArgs(1) == cobra.ExactArgs(3), it is needed
// to run test argument sets that both pass and fail to ensure the correct function was set.
var usecases = []struct {
name string
runner Runner
testCases map[string]argTest
cmd *cobra.Command
}{
{
name: "leaf command, no defined args, follow parent",
runner: Runner{
Phases: []Phase{phaseBuilder("foo")},
},
testCases: map[string]argTest{
"phase foo": {
pass: []string{"one", "two", "three"},
fail: []string{"one", "two"},
args: cobra.ExactArgs(3),
},
},
cmd: &cobra.Command{
Use: "init",
Args: cobra.ExactArgs(3),
},
},
{
name: "container cmd expect none, custom arg check for leaf",
runner: Runner{
Phases: []Phase{phaseBuilder6("foo", cobra.NoArgs,
phaseBuilder6("bar", cobra.ExactArgs(1)),
phaseBuilder6("baz", customArgs),
)},
},
testCases: map[string]argTest{
"phase foo": {
pass: []string{},
fail: []string{"one"},
args: cobra.NoArgs,
},
"phase foo bar": {
pass: []string{"one"},
fail: []string{"one", "two"},
args: cobra.ExactArgs(1),
},
"phase foo baz": {
pass: []string{"qux"},
fail: []string{"one"},
args: customArgs,
},
},
cmd: &cobra.Command{
Use: "init",
Args: cobra.NoArgs,
},
},
}

for _, rt := range usecases {
t.Run(rt.name, func(t *testing.T) {

rt.runner.BindToCommand(rt.cmd)

// Checks that cmd gets a new phase subcommand
phaseCmd := getCmd(rt.cmd, "phase")
if phaseCmd == nil {
t.Error("cmd didn't have phase subcommand\n")
return
}

for c, args := range rt.testCases {

cCmd := getCmd(rt.cmd, c)
if cCmd == nil {
t.Errorf("cmd didn't have %s subcommand\n", c)
continue
}

// Ensure it is the expected function
if reflect.ValueOf(cCmd.Args).Pointer() != reflect.ValueOf(args.args).Pointer() {
t.Error("The function poiners where not equal.")
}

// Test passing argument set
err := cCmd.Args(cCmd, args.pass)

if err != nil {
t.Errorf("command %s should validate the args: %v\n %v", cCmd.Name(), args.pass, err)
}

// Test failing argument set
err = cCmd.Args(cCmd, args.fail)

if err == nil {
t.Errorf("command %s should fail to validate the args: %v\n %v", cCmd.Name(), args.pass, err)
}
}

})
}
}

func TestBindToCommand(t *testing.T) {

var dummy string
Expand Down