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

Fix hyperkube flag parsing #25512

Merged
merged 1 commit into from
May 22, 2016
Merged

Fix hyperkube flag parsing #25512

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/hyperkube/hyperkube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ func (hk *HyperKube) Run(args []string) error {
command := args[0]
baseCommand := path.Base(command)
serverName := baseCommand
args = args[1:]
if serverName == hk.Name {
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as you run hyperkube kubectl, this should not matter. Why movin 2 lines higher?

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to perhaps break things when you softlink kubectl to the hyperkube binary. Have you tested that?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

Ironically enough, this was actually a fix to make softlinking work better :) I've tested it.

There's no reason to pass the command arg to any of the Server Run blocks . In particular, since kubectl is now a Cobra command, it requires that if args[0] is not a flag, it match the command name exactly. In the case of a softlink, it's possible thats args[0] will be for example /usr/bin/kubectl, and the Cobra command will complain. This removes that possibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Looking at the documentation for pflags.FlagSet.Parse it looks like the args should not contain the command name. That would imply that we were doing it wrong before when the command name was being included. interspersed defaulting to true must have covered our sins.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, that's very true!

args = args[1:]

baseFlags := hk.Flags()
baseFlags.SetInterspersed(false) // Only parse flags up to the next real command
Expand Down
80 changes: 80 additions & 0 deletions cmd/hyperkube/hyperkube_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"strings"
"testing"

"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -54,6 +55,55 @@ func testServerError(n string) *Server {
}
}

const defaultCobraMessage = "default message from cobra command"
const defaultCobraSubMessage = "default sub-message from cobra command"
const cobraMessageDesc = "message to print"
const cobraSubMessageDesc = "sub-message to print"

func testCobraCommand(n string) *Server {

var cobraServer *Server
var msg string
cmd := &cobra.Command{
Use: n,
Long: n,
Short: n,
Run: func(cmd *cobra.Command, args []string) {
cobraServer.hk.Printf("msg: %s\n", msg)
},
}
cmd.PersistentFlags().StringVar(&msg, "msg", defaultCobraMessage, cobraMessageDesc)

var subMsg string
subCmdName := "subcommand"
subCmd := &cobra.Command{
Use: subCmdName,
Long: subCmdName,
Short: subCmdName,
Run: func(cmd *cobra.Command, args []string) {
cobraServer.hk.Printf("submsg: %s", subMsg)
},
}
subCmd.PersistentFlags().StringVar(&subMsg, "submsg", defaultCobraSubMessage, cobraSubMessageDesc)

cmd.AddCommand(subCmd)

localFlags := cmd.LocalFlags()
localFlags.SetInterspersed(false)
s := &Server{
SimpleUsage: n,
Long: fmt.Sprintf("A server named %s which uses a cobra command", n),
Run: func(s *Server, args []string) error {
cobraServer = s
cmd.SetOutput(s.hk.Out())
cmd.SetArgs(args)
return cmd.Execute()
},
flags: localFlags,
}

return s
}
func runFull(t *testing.T, args string) *result {
buf := new(bytes.Buffer)
hk := HyperKube{
Expand All @@ -66,6 +116,7 @@ func runFull(t *testing.T, args string) *result {
hk.AddServer(testServer("test2"))
hk.AddServer(testServer("test3"))
hk.AddServer(testServerError("test-error"))
hk.AddServer(testCobraCommand("test-cobra-command"))

a := strings.Split(args, " ")
t.Logf("Running full with args: %q", a)
Expand Down Expand Up @@ -143,3 +194,32 @@ func TestServerError(t *testing.T) {
assert.Contains(t, x.output, "test-error Run")
assert.EqualError(t, x.err, "Server returning error")
}

func TestCobraCommandHelp(t *testing.T) {
x := runFull(t, "hyperkube test-cobra-command --help")
assert.NoError(t, x.err)
assert.Contains(t, x.output, "A server named test-cobra-command which uses a cobra command")
assert.Contains(t, x.output, cobraMessageDesc)
}
func TestCobraCommandDefaultMessage(t *testing.T) {
x := runFull(t, "hyperkube test-cobra-command")
assert.Contains(t, x.output, fmt.Sprintf("msg: %s", defaultCobraMessage))
}
func TestCobraCommandMessage(t *testing.T) {
x := runFull(t, "hyperkube test-cobra-command --msg foobar")
assert.Contains(t, x.output, "msg: foobar")
}

func TestCobraSubCommandHelp(t *testing.T) {
x := runFull(t, "hyperkube test-cobra-command subcommand --help")
assert.NoError(t, x.err)
assert.Contains(t, x.output, cobraSubMessageDesc)
}
func TestCobraSubCommandDefaultMessage(t *testing.T) {
x := runFull(t, "hyperkube test-cobra-command subcommand")
assert.Contains(t, x.output, fmt.Sprintf("submsg: %s", defaultCobraSubMessage))
}
func TestCobraSubCommandMessage(t *testing.T) {
x := runFull(t, "hyperkube test-cobra-command subcommand --submsg foobar")
assert.Contains(t, x.output, "submsg: foobar")
}
16 changes: 9 additions & 7 deletions cmd/hyperkube/kubectl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,23 @@ package main
import (
"os"

"k8s.io/kubernetes/cmd/kubectl/app"
"k8s.io/kubernetes/pkg/kubectl/cmd"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
)

func NewKubectlServer() *Server {
cmd := cmd.NewKubectlCommand(cmdutil.NewFactory(nil), os.Stdin, os.Stdout, os.Stderr)
Copy link
Contributor

Choose a reason for hiding this comment

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

So far hyperkube has always collected cmds from k8s.io/kubernetes/cmd as it adds another layer between k8s.io/kubernetes/pkg and k8s.io/kubernetes/cmd/hyperkube. Calling cmd.NewKubectlCommand right here is against that. Not sure, if it really matters. Still, I see the additional layer as a useful assumption when debugging hyperkube. At the same time the common pattern of defining Server.Run (as bellow) is to delegate the call to another app.Run.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with skipping k8s.io/kubernetes/cmd/kubectl as long as we put a big comment in there that any significant changes need to be duplicated here. At some point hopefully we can finish the hyperkube transition and hyperkube will be the only way to get at this stuff. Until the, we need to keep stuff in sync.

Copy link
Author

Choose a reason for hiding this comment

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

addressed in followup commit.

localFlags := cmd.LocalFlags()
localFlags.SetInterspersed(false)

return &Server{
name: "kubectl",
SimpleUsage: "Kubernetes command line client",
Long: "Kubernetes command line client",
Run: func(s *Server, args []string) error {
os.Args = os.Args[1:]
if err := app.Run(); err != nil {
os.Exit(1)
}
os.Exit(0)
return nil
cmd.SetArgs(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

if we don't muck with args above, this should be args[1:]

Copy link
Author

Choose a reason for hiding this comment

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

Depending on whether or not it's a softlink (os.Args[0] == "hyperkube"), it's either os.Args[1:] or os.Args[2:]. It's kind of nice to let the hyperkube server deal with that logic and sub-servers can just assume that their args list starts with the first relevant flag. This would easily support, for example, nesting servers in servers.

return cmd.Execute()
},
flags: localFlags,
}
}
4 changes: 4 additions & 0 deletions cmd/kubectl/app/kubectl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ import (
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
)

/*
WARNING: this logic is duplicated, with minor changes, in cmd/hyperkube/kubectl.go
Any salient changes here will need to be manually reflected in that file.
*/
func Run() error {
cmd := cmd.NewKubectlCommand(cmdutil.NewFactory(nil), os.Stdin, os.Stdout, os.Stderr)
return cmd.Execute()
Expand Down