Support Docker mode #63
Conversation
| @@ -53,26 +57,65 @@ func init() { | |||
| runCmd.Flags().BoolVarP(¶ms.debug, "debug", "d", false, "Print debugging information to the standard error") | |||
| runCmd.Flags().IntVarP(¶ms.reportingInterval, "interval", "i", 1, "Interval, in seconds, between stats updates") | |||
| runCmd.Flags().BoolVarP(¶ms.assumeyes, "assume-yes", "y", false, "Force execution without asking for confirmation") | |||
| runCmd.Flags().BoolVarP(¶ms.docker, "docker", "", false, "Running against a docker image. " + | |||
There was a problem hiding this comment.
re-using things like the "host" to inject the image name is a bit hacky. But that is a minor thing
| @@ -53,26 +57,65 @@ func init() { | |||
| runCmd.Flags().BoolVarP(¶ms.debug, "debug", "d", false, "Print debugging information to the standard error") | |||
| runCmd.Flags().IntVarP(¶ms.reportingInterval, "interval", "i", 1, "Interval, in seconds, between stats updates") | |||
| runCmd.Flags().BoolVarP(¶ms.assumeyes, "assume-yes", "y", false, "Force execution without asking for confirmation") | |||
| runCmd.Flags().BoolVarP(¶ms.docker, "docker", "", false, "Running against a docker image. " + | |||
| "Use the image reference instead of the <host>, and a custom mapped port if the first exposed one is not ok (optional") | |||
| } | |||
|
|
|||
| func validateRequiredArgs(params *tcpgoonParams, args []string) error { | |||
There was a problem hiding this comment.
we must redo how we are validating parms. Relying on a max/min num of required params looks ugly: we should just check which parm is required and which are not in each execution mode. If execution modes parameters differ a lot in the end, maybe we should recover your idea of using different commands
There was a problem hiding this comment.
yes, the execution mode is the first question to answer
| } | ||
|
|
||
| params.targetip = addrs[0].String() | ||
| fmt.Fprintln(debugging.DebugOut, "TCPGOON target: Hostname(", params.target, "), IP (", params.targetip, ")") | ||
| if params.docker { |
There was a problem hiding this comment.
this massive if/else is... :) . That smells to two different validation functions depending on the "mode", but again, we should rethink the parms in general
| Port int | ||
| } | ||
|
|
||
| func lookForAvailableLocalPort() (availablePort int){ |
There was a problem hiding this comment.
up to you, but i would try the same the docker is exposing, and if that fails, require the user to be the one that chooses one
| return availablePort | ||
| } | ||
|
|
||
| func DownloadAndRun(image string, port int) (targetinfo IPandPort, containerID string) { |
There was a problem hiding this comment.
i guess this requires refactor / better encapsulation... long and with too much statements inside if/elses. I would also not panic in this function, but return an error and make the caller to manage the errors accordingly
There was a problem hiding this comment.
sure, do not take this PR as ready to review. The point here is, having a first working version, understand if this docker mode should be part of the regular command or have a custom one.
| fmt.Println("Internal Docker port binding:", mapperProto, mappedPort) | ||
| } | ||
|
|
||
| // hack https://stackoverflow.com/questions/47395973/issue-while-using-docker-api-for-go-cannot-import-nat |
There was a problem hiding this comment.
do we need to nat / expose the port in the host 0.0.0.0? Maybe we can target the container IP if an internal subnet/bridge network is being used
There was a problem hiding this comment.
changing the network defaults is another option, but I considered this simpler
Initial proposal to verify the approach to the docker mode.
Use it:
$ go run main.go run --docker nginxHints: