Support publishing additional container ports in thv run#3892
Support publishing additional container ports in thv run#3892Sanskarzz wants to merge 4 commits intostacklok:mainfrom
Conversation
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
Sanskarzz
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
eleftherias,
jhrozek,
lujunsan,
rdimitrov and
yrobla
as code owners
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
github-actions
bot
added
size/S
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3892 +/- ##
==========================================
- Coverage 66.80% 66.76% -0.04%
==========================================
Files 444 444
Lines 44503 44550 +47
==========================================
+ Hits 29730 29744 +14
- Misses 12445 12474 +29
- Partials 2328 2332 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/S
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
github-actions
bot
added
size/S
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
Mostly LGTM to me. Can you manually verify this fixes the user's reported issue by running this locally? A series of screenshots or video will do.
| if tt.wantError { | ||
| if err == nil { | ||
| t.Errorf("ParsePortSpec(%s) expected error but got nil", tt.portSpec) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if err != nil { | ||
| t.Errorf("ParsePortSpec(%s) unexpected error: %v", tt.portSpec, err) | ||
| return | ||
| } | ||
|
|
||
| if tt.expectedHostPort != "" && hostPort != tt.expectedHostPort { | ||
| t.Errorf("ParsePortSpec(%s) hostPort = %s, want %s", tt.portSpec, hostPort, tt.expectedHostPort) | ||
| } | ||
|
|
||
| if tt.expectedHostPort == "" && hostPort == "" { | ||
| t.Errorf("ParsePortSpec(%s) hostPort is empty, want random port", tt.portSpec) | ||
| } | ||
|
|
||
| if containerPort != tt.expectedContainer { | ||
| t.Errorf("ParsePortSpec(%s) containerPort = %d, want %d", tt.portSpec, containerPort, tt.expectedContainer) | ||
| } |
There was a problem hiding this comment.
nit: can you rewrite these assertions to use the require package (e.g.require.Equal or require.True)?
This will make the tests less nested and easier to read.
| hostPortStr := bindings[0].HostPort | ||
| if hostPortStr == "" || hostPortStr == "0" { | ||
| hostPort = networking.FindAvailable() | ||
| if hostPort == 0 { | ||
| return nil, 0, fmt.Errorf("could not find an available port") | ||
| } | ||
| bindings[0].HostPort = fmt.Sprintf("%d", hostPort) | ||
| portBindings[key] = bindings | ||
| } else { | ||
| var err error | ||
| hostPort, err = strconv.Atoi(hostPortStr) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("failed to convert host port %s to int: %w", hostPortStr, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this the fix for:
It also fixes a bug in the Docker runtime client where explicitly requested host ports were being overwritten by random ports.
?
If so, can you find a way to add a regression test and some unit tests here? The generatePortBindings function seems like it should be amenable to unit testing without any refactoring.
2 participants
Fix: #3812
Summary
This PR adds support for the
--publish / -pflag to thethv runcommand, enabling users to expose arbitrary container ports to the host, similar todocker run -p. It also fixes a bug in the Docker runtime client where explicitly requested host ports were being overwritten by random ports.Key Changes
--publishflag tothv run.networking.ParsePortSpecfor robust port specification parsing.runtime.Setupto handle multiple port bindings efficiently.pkg/container/docker/client.goto respect existingHostPortassignments ingeneratePortBindings.