Expose the --raw-port flag in the run and run-prepared subcommands
#3407
Conversation
| // along with the flagStringList, used everywhere | ||
|
|
||
| // flagStringList implements the flag.Value interface to contain a set of strings | ||
| type flagStringList []string |
There was a problem hiding this comment.
These belong to rkt/cli_apps.go I guess.
| Now, any traffic arriving on host's TCP port 8888 on any IP will be forwarded to the pod on port 80. | ||
|
|
||
|
|
||
| You can also forward a port that is not referenced at the app time with the `--raw-port` option. The syntax is Name, Protocol (`tcp` or `udp`), Pod port, Host IP (use 0.0.0.0 for all IPs), Host port. |
There was a problem hiding this comment.
"referenced in the app manifest" perhaps?
There was a problem hiding this comment.
What is "Name" here and what is the user supposed to pick for it? Can we do better and just generate one for the user? Or ask the user to name them port-proto so we can automatically pick them up without a new option?
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080` | | ||
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:<HOSTIP>:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080`. The HOSTIP is optional. | | ||
| | `--private-users` | `false` | `t 10000 rue` or `false` | Run within user namespaces | | ||
| | `--raw-port` | none | A port to forward from the host to the pod | Syntax: `--raw-port=name:proto:podPort:hostIP:hostPort`, e.g. `http:tcp:80:0.0.0.0:8080` | |
There was a problem hiding this comment.
IMHO this looks quite ugly from a UX point of view.
| | `--no-store` | `false` | `true` or `false` | Fetch images, ignoring the local store. See [image fetching behavior][img-fetch] | | ||
| | `--pod-manifest` | none | A path | The path to the pod manifest. If it's non-empty, then only `--net`, `--no-overlay` and `--interactive` will have effect. | | ||
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080` | | ||
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:<HOSTIP>:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080`. The HOSTIP is optional. | |
There was a problem hiding this comment.
use [:HOSTIP] to denote optionality
There was a problem hiding this comment.
(also HOSTIP could do with a brief description)
There was a problem hiding this comment.
Syntax should follow the OCI spec v1.0.1 and being adopted in other tools and documentation:
--port 80/tcp
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080` | | ||
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:<HOSTIP>:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080`. The HOSTIP is optional. | | ||
| | `--private-users` | `false` | `true` or `false` | Run within user namespaces | | ||
| | `--raw-port` | none | A port to forward from the host to the pod | Syntax: `--raw-port=name:proto:podPort:hostIP:hostPort`, e.g. `http:tcp:80:0.0.0.0:8080` | |
There was a problem hiding this comment.
NAME:PROTO:PODPORT:HOSTIP:HOSTPORT as elsewhere
| | `--no-store` | `false` | `true` or `false` | Fetch images, ignoring the local store. See [image fetching behavior][img-fetch]. | | ||
| | `--pod-manifest` | none | A path | The path to the pod manifest. If it's non-empty, then only `--net`, `--no-overlay` and `--interactive` will have effect. | | ||
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080`. | | ||
| | `--port` | none | A port name and number pair | Container port name to expose through host port number. Requires [contained network][contained]. Syntax: `--port=NAME:<HOSTIP>:HOSTPORT` The NAME is that given in the ACI. By convention, Docker containers' EXPOSEd ports are given a name formed from the port number, a hyphen, and the protocol, e.g., `80-tcp`, giving something like `--port=80-tcp:8080`. The HOSTIP is optional. | |
There was a problem hiding this comment.
As above. (Seems like we should have a shared section for the prepare/run options....)
| // flagStringList implements the flag.Value interface to contain a set of strings | ||
| type flagStringList []string | ||
|
|
||
| func (dns *flagStringList) Set(s string) error { |
| // argument. This is need to permit to correctly handle | ||
| // multiple "IMAGE -- imageargs ---" options | ||
| cmdPrepare.Flags().SetInterspersed(false) | ||
| flagPorts = portList{} |
There was a problem hiding this comment.
seems like there could be a more logical location for this (separate from this interspersed operation at least)
|
Sorry, my review was a little half-baked from yesterday before luca started his so some of his comments take precedence. |
|
This was supposed to be a small PR, exposing the The current question is: Should we have some way of overloading the "port name" designator currently used in This is made extra silly by our current synthesis of ports exposed by Docker images, since they don't have names. So the port name is just |
|
Afraid I don't quite follow - can you mock up a simple usage example for me?
…On 24 November 2016 at 12:02, Casey Callendrello ***@***.***> wrote:
This was supposed to be a small PR, exposing the app sandbox flag to the
run subcommand. However, I agree it could be better :-).
The current question is: Should we have some way of overloading the "port
name" designator currently used in --port to specify an arbitrary port?
Right now, you can only pass a value to --port that references something
exposed by a manifest. Luca suggested this - and by doing so, we could
remove a CLI flag at the loss of a bit of simplicity.
This is made extra silly by our current synthesis of ports exposed by
Docker images, since they don't have names. So the port name is just
tcp-80, for example. We could just always parse this. It's the simplest
case from a UX perspective, and that's always a good thing.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3407 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACewN7_Rvh45LikseLCy2ICCHG3jf24Jks5rBW6qgaJpZM4K6sKs>
.
|
|
So, Dockerfiles have the Therefore you might do something like However, the CRI specifies that port forwarding should be set up before any applications are added. So right now So, we can either add |
|
Are there any cases in which parsing would result in undesired/unexpected behaviour? (I mean in non-parse-failure cases) |
|
Not really. I suppose if someone was relying on something |
|
The only other source of confusion I see is if some aci specifies a |
|
SGTM |
|
So are we in agreement that we should:
right? |
|
This didn't quite make it in time for this release, bumping. |
|
This issue looks like a perfect candidate for bikeshedding. So, what we've got now is The information missing from the above is: 1) the container port number, 2) the protocol. That information right now is resolved via the name having already been associated with that info, but the point is to remove that requirement. The current suggestion is to make the name be acceptable as either a name of a port defined within the image, or as We have quite a few possible colours we can paint this particular bikeshed instead though, and I'd prefer taking a look at what the colours are before picking the above. CSVWe have various options which are comma-separated key-value pairs. We could introduce that to This would allow us to use something like: Heuristically parsingCurrently, by convention, docker2aci leaves ports of the form I think this has the downside of being le
6854
ss obvious (more "magic"). It's also easy for a user to typo (e.g. The upside is that backwards compatibility is free. More colonsWe could extend the existing This has the upside of being the most consistent with the current option of I think painting more colons onto our bikeshed is probably a better option that going with a heuristically coloured one. The CSV colour also looks quite nice to me, though I'm not really a fan of the amount of typing required for the |
|
So, right now we have:
If we go according to @euank's suggestion, then we have the additional valid cases:
After some thought, I want to just get rid of |
|
@squeed there are more valid cases than that as well: 2 colons can be any of:
proto, ips, and ports can all be distinguished from each other, so I don't think there are any ambiguous cases. |
|
Hence my desire to keep the |
|
I'm confused about the end of this discussion. Do we have a decision? |
|
Only if @euank can convince me not to parse "tcp-80" in all cases :-). |
|
@squeed I left my reasoning why I don't like it. I personally wouldn't have much problem typing Being able to omit tcp and have it assumed is nice. With that, I have a final option for this bikeshed Optional Name
The logic would be something like: "if the first part would be valid as a port name, interpret it as the current --port flag behaviour, otherwise the above". Pros:
Cons:
|
|
Just my two cents: Another option would be to introduce an "--expose [name=]http,proto=tcp,port=80" like option on app level. This would simply patch the image manifest (or the pod-manifest, not sure about the details). The --port option could then use the newly defined port-identifier. This would be similar to the --mount option which is used if the aci doesnt provide mount points. I think this would be more easy to understand, and you can memorize the syntax better than with a hundred optional positional arguments. Pros:
Cons:
|
|
Can one of the admins verify this patch? |
|
(github.com/appc/spec/schema/types).PortFromString() is already there and could do the parsing. Then I would simply add that port to the app in the pod-manifest in the end of the run call right before stage0.Run() is called (in rkt/run.go:404) |
This allows us to specify exposed ports at runtime in favor of patching the image manifests.
A typical call will look like this:
rkt run \
--port shell:11111 \
trusch.io/alpine \
--expose shell,port=12345,protocol=tcp \
--exec nc -- -lk -p 12345 -e /bin/sh
This fixes issue rkt#2113 and is conflicting with PR rkt#3407
Fixes #2113