Implement gvproxy networking using cmdline wrapper #19723
Conversation
jakecorrenti
changed the title
jakecorrenti
force-pushed
the
implement-gvproxy-cmdline
branch
3 times, most recently
from
6c79466
to
84fee33
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, jakecorrenti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| cmd = append(cmd, []string{"-forward-dest", destSock}...) | ||
| cmd = append(cmd, []string{"-forward-user", forwardUser}...) | ||
| cmd = append(cmd, []string{"-forward-identity", m.IdentityPath}...) | ||
| cmd.AddForwardSock(socket.GetPath()) |
There was a problem hiding this comment.
i wonder ... should we follow a pattern here like:
cmd := gvproxy.Command().AddForwardSock(...).AddForwardDest(...)...
and then if we must afterwards:
cmd.foo = "bar"
There was a problem hiding this comment.
We can definitely do that if you don't mind that it blocks this PR
There was a problem hiding this comment.
I was giving this a go and, this is just me, but when we start chaining 2-3 functions the line gets really long. I think this is a little difficult to read when you have to go really far to the edge of the screen versus just reading straight down. Regardless, I can still implement it if it's what we want.
afaik go doesn't like it when we do something like this (which works in something like Rust):
gvproxy.NewGvproxyCommand().AddForwardSock()
.AddForwardDest()
.AddForwardIdentity()
|
Do we want to ensure that we use tagged releases of gvisor in Podman releases, or is that not something we should be concerned about? |
|
containers/gvisor-tap-vsock#266 should fix the failing tests |
jakecorrenti
force-pushed
the
implement-gvproxy-cmdline
branch
from
84fee33
to
86e40a8
Compare
openshift-merge-robot
added
the
needs-rebase
jakecorrenti
force-pushed
the
implement-gvproxy-cmdline
branch
from
86e40a8
to
f19c992
Compare
openshift-merge-robot
removed
the
needs-rebase
Converts the host networking code in `podman machine` to use the `GvproxyCommand` type introduced in containers/gvisor-tap-vsock#258 [NO NEW TESTS NEEDED] Signed-off-by: Jake Correnti <[email protected]>
jakecorrenti
force-pushed
the
implement-gvproxy-cmdline
branch
2 times, most recently
from
d2c0bf2
to
289e59e
Compare
|
LGTM |
|
/lgtm |
Removes the line in applehv and qemu `machine.go` file. These are remnants from containers#19723. This lines was written to add stdin, stdout, stderr as extra files, but that is not how `c.ExtraFiles` works (unlike `os.ProcAttr`). go source: https://cs.opensource.google/go/go/+/go1.21.1:src/os/exec/exec.go;l=147 Signed-off-by: Jake Correnti <[email protected]>
Removes the line in applehv and qemu `machine.go` file. These are remnants from containers#19723. This lines was written to add stdin, stdout, stderr as extra files, but that is not how `c.ExtraFiles` works (unlike `os.ProcAttr`). go source: https://cs.opensource.google/go/go/+/go1.21.1:src/os/exec/exec.go;l=147 [NO NEW TESTS NEEDED] Signed-off-by: Jake Correnti <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Converts the host networking code in
podman machineto use theCommandtype introduced in containers/gvisor-tap-vsock#258[NO NEW TESTS NEEDED]
Does this PR introduce a user-facing change?