stage0: add --expose flag on app level #3691
Conversation
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
|
Can one of the admins verify this patch? |
|
@squeed what do you think about this approach compared to the approach in the last comments of your PR? |
|
@ALL is there no interest in this flag? I can close this, but a solution to expose ports at runtime would be great. |
|
@trusch please leave this open. I was offline a bunch of days and I'm catching up with backlog, but I'll try to get to reviewing this soon. |
|
@lucab I hope your backlog is shrinking ;) I can rebase this if it makes your life easier. |
| @@ -895,3 +896,35 @@ func (au *appStderr) String() string { | |||
| func (au *appStderr) Type() string { | |||
| return "appStderr" | |||
| } | |||
|
|
|||
| // appExpose is for --expose flags in the form of: --expose=query,protocol=tcp,port=8080,count=1,socketActivated=true | |||
There was a problem hiding this comment.
I guess query here would be clearer as portname.
| @@ -103,6 +103,7 @@ func addAppFlags(cmd *cobra.Command) { | |||
| cmd.Flags().Var((*appWorkingDir)(&rktApps), "working-dir", "override the working directory of the preceding image") | |||
| cmd.Flags().Var((*appReadOnlyRootFS)(&rktApps), "readonly-rootfs", "if set, the app's rootfs will be mounted read-only") | |||
| cmd.Flags().Var((*appMount)(&rktApps), "mount", "mount point binding a volume to a path within an app") | |||
| cmd.Flags().Var((*appExpose)(&rktApps), "expose", "expose ports of the application (example: '--expose=http,protocol=tcp,port=8080')") | |||
There was a problem hiding this comment.
This new flag should be documented in run.md, and possibly also also to prepare / run-prepared (I didn't check, I think this also adds it to the command so maybe only the doc part is missing).
| @@ -188,6 +188,24 @@ func MergeMounts(mounts []schema.Mount, appMounts []schema.Mount) []schema.Mount | |||
| return deduplicateMPs(ml) | |||
| } | |||
|
|
|||
| // MergePorts merges two port lists | |||
| func MergePorts(a, b []types.Port) []types.Port { | |||
There was a problem hiding this comment.
I'm not sure how relevant this maybe in this context, but this is not a stable merge (i.e. it doesn't guarantee too keep the relative order). It may poses some issues when trying to answer questions like what is the precedence order between manifest ports and exposed ports.
|
Uops, I'm really sorry for the delay. I did a pass on this and left some inline comments. However I've higher level concerns with it, which I am detailing below. First, I like the split between expose / port as it follows the same logic of volume / mount; however I see this more as part of a larger re-design of the port syntax (getting rid of Then, I think this is a better proposal over the current In general, I'm still holding the line of reasoning of #3407 (comment). I would address the specific #2113 problem by documenting the current $port-$proto magic synatx and adding a default for un-exposed ports. Once that concern is gone, we can more throughfully discuss something like #3407 (comment) and come up with a proposed design (possibly based on expose / port) and implement it, taking care of all the re-design points above at once. |
|
@lucab regarding ipv6: look what's landed in cni: containernetworking/plugins#10 So bridge is ready and the PR for ptp is also there :) Sent from my HTC One_M8 using FastHub |
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/shThis fixes issue #2113 and is conflicting with PR #3407