Fix `PortsForwarder.Expose()` proxy check by cpick · Pull Request #441 · containers/gvisor-tap-vsock

cpick · 2024-12-21T19:16:44Z

The check for existing proxies entries used local as a key, but they're actually inserted using keys of: key(protocol, local).

This meant that, previously, the check was ineffective and wouldn't reject duplicate forwards.

cfergeau

Ouch, good catch.
I'd add this on top of this commit to let the compiler catch such bugs in the future:

diff --git a/pkg/services/forwarder/ports.go b/pkg/services/forwarder/ports.go
index 6b92a005..a1de5401 100644
--- a/pkg/services/forwarder/ports.go
+++ b/pkg/services/forwarder/ports.go
@@ -25,11 +25,13 @@ import (
 	"gvisor.dev/gvisor/pkg/tcpip/stack"
 )
 
+type ProxyKey string
+
 type PortsForwarder struct {
 	stack *stack.Stack
 
 	proxiesLock sync.Mutex
-	proxies     map[string]proxy
+	proxies     map[ProxyKey]proxy
 }
 
 type proxy struct {
@@ -61,7 +63,7 @@ func (w CloseWrapper) Close() error {
 func NewPortsForwarder(s *stack.Stack) *PortsForwarder {
 	return &PortsForwarder{
 		stack:   s,
-		proxies: make(map[string]proxy),
+		proxies: make(map[ProxyKey]proxy),
 	}
 }
 
@@ -256,8 +258,8 @@ func (f *PortsForwarder) Expose(protocol types.TransportProtocol, local, remote
 	return nil
 }
 
-func key(protocol types.TransportProtocol, local string) string {
-	return fmt.Sprintf("%s/%s", protocol, local)
+func key(protocol types.TransportProtocol, local string) ProxyKey {
+	return ProxyKey(fmt.Sprintf("%s/%s", protocol, local))
 }
 
 func (f *PortsForwarder) Unexpose(protocol types.TransportProtocol, local string) error {

cfergeau · 2025-01-08T11:27:51Z

I've added the suggested change as a new commit, rebased to get a fix for CI failures, and force-pushed this to your branch.

cfergeau · 2025-01-08T15:43:44Z

This break the port forwarding test:

port forwarding
/home/runner/work/gvisor-tap-vsock/gvisor-tap-vsock/test/port_forwarding_test.go:23
  should expose and reach rootless podman API using unix to unix forwarding over ssh [It]
  /home/runner/work/gvisor-tap-vsock/gvisor-tap-vsock/test/port_forwarding_test.go:231

it's reproducible locally with only your commit on top of git main, so this is not related to the github environment. The main branch is green: https://github.com/containers/gvisor-tap-vsock/actions/runs/12671768387/job/35314238072

vyasgun · 2025-01-09T15:24:33Z

The test is failing because unix/local is being exposed in an earlier test and not being unexposed. This should fix it:

diff --git a/test/port_forwarding_test.go b/test/port_forwarding_test.go
index 2e32e8f1..8e031934 100644
--- a/test/port_forwarding_test.go
+++ b/test/port_forwarding_test.go
@@ -226,6 +226,9 @@ var _ = ginkgo.Describe("port forwarding", func() {
                        g.Expect(err).ShouldNot(gomega.HaveOccurred())
                        g.Expect(resp.StatusCode).To(gomega.Equal(http.StatusOK))
                }).Should(gomega.Succeed())
+
+               out, err = sshExec(`curl http://gateway.containers.internal/services/forwarder/unexpose -X POST -d'{"protocol":"unix","local":"` + unix2tcpfwdsock + `","remote":"tcp://192.168.127.2:8080"}'`)
+               gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
        })
 
        ginkgo.It("should expose and reach rootless podman API using unix to unix forwarding over ssh", func() {

cfergeau · 2025-01-09T15:41:26Z

The test is failing because unix/local is being exposed in an earlier test and not being unexposed. This should fix it:

diff --git a/test/port_forwarding_test.go b/test/port_forwarding_test.go
index 2e32e8f1..8e031934 100644
--- a/test/port_forwarding_test.go
+++ b/test/port_forwarding_test.go
@@ -226,6 +226,9 @@ var _ = ginkgo.Describe("port forwarding", func() {
                        g.Expect(err).ShouldNot(gomega.HaveOccurred())
                        g.Expect(resp.StatusCode).To(gomega.Equal(http.StatusOK))
                }).Should(gomega.Succeed())
+
+               out, err = sshExec(`curl http://gateway.containers.internal/services/forwarder/unexpose -X POST -d'{"protocol":"unix","local":"` + unix2tcpfwdsock + `","remote":"tcp://192.168.127.2:8080"}'`)
+               gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
        })
 
        ginkgo.It("should expose and reach rootless podman API using unix to unix forwarding over ssh", func() {

Ah tests are not independent from each other :-/
Can you add a commit fixing this to this fix-proxy-check branch?

vyasgun · 2025-01-10T04:42:18Z

@cfergeau I don't have the permission to do so :/

The check for existing `proxies` entries used `local` as a key, but they're actually inserted using keys of: `key(protocol, local)`. This meant that, previously, the check was ineffective and wouldn't reject duplicate forwards. Signed-off-by: Chris Pick <chris@chrispick.com>

This allows the go compiler to catch the bug fixed in the previous commit: ``` GOARCH=amd64 GOOS=freebsd go build -ldflags "-s -w -X github.com/containers/gvisor-tap-vsock/pkg/types.gitVersion=v0.8.1-5-g9df1d587" -o bin/gvproxy-freebsd-amd64 ./cmd/gvproxy # github.com/containers/gvisor-tap-vsock/pkg/services/forwarder pkg/services/forwarder/ports.go:73:24: cannot use local (variable of type string) as ProxyKey value in map index make: *** [Makefile:57: cross] Error 1 ``` Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

…gain This also fixes a pre-existing issue with this test file, an exposed port was not unexposed before attempting to expose it again, which caused a test failure after the PortsForwarder.Expose() fix. Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>

cfergeau · 2025-01-10T15:10:22Z

CI is green now. I'll merge this next week if you don't have objections to these follow-ups commits @cpick

cfergeau · 2025-01-14T10:53:23Z

/lgtm
/approve

openshift-ci · 2025-01-14T10:53:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, cpick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cpick force-pushed the fix-proxy-check branch from 97b8f50 to 0a63f4e Compare December 21, 2024 21:04

cfergeau reviewed Jan 8, 2025

View reviewed changes

cfergeau force-pushed the fix-proxy-check branch from 0a63f4e to 8992370 Compare January 8, 2025 11:26

vyasgun mentioned this pull request Jan 10, 2025

Add a test to check an already exposed port cannot be exposed again #454

Closed

openshift-merge-robot added the needs-rebase label Jan 10, 2025

Chris Pick and others added 3 commits January 10, 2025 15:13

cfergeau force-pushed the fix-proxy-check branch from 434e487 to 233ecd8 Compare January 10, 2025 14:14

openshift-merge-robot removed the needs-rebase label Jan 10, 2025

openshift-ci bot assigned cfergeau Jan 14, 2025

openshift-ci bot added the lgtm label Jan 14, 2025

openshift-ci bot added the approved label Jan 14, 2025

openshift-merge-bot bot merged commit 5086dbc into containers:main Jan 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `PortsForwarder.Expose()` proxy check#441

Fix `PortsForwarder.Expose()` proxy check#441
openshift-merge-bot[bot] merged 3 commits intocontainers:mainfrom
cpick:fix-proxy-check

cpick commented Dec 21, 2024

Uh oh!

cfergeau left a comment

Uh oh!

cfergeau commented Jan 8, 2025

Uh oh!

cfergeau commented Jan 8, 2025 •

edited

Loading

Uh oh!

vyasgun commented Jan 9, 2025

Uh oh!

cfergeau commented Jan 9, 2025

Uh oh!

vyasgun commented Jan 10, 2025

Uh oh!

cfergeau commented Jan 10, 2025

Uh oh!

cfergeau commented Jan 14, 2025

Uh oh!

openshift-ci bot commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cpick commented Dec 21, 2024

Uh oh!

cfergeau left a comment

Choose a reason for hiding this comment

Uh oh!

cfergeau commented Jan 8, 2025

Uh oh!

cfergeau commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vyasgun commented Jan 9, 2025

Uh oh!

cfergeau commented Jan 9, 2025

Uh oh!

vyasgun commented Jan 10, 2025

Uh oh!

cfergeau commented Jan 10, 2025

Uh oh!

cfergeau commented Jan 14, 2025

Uh oh!

openshift-ci bot commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cfergeau commented Jan 8, 2025 •

edited

Loading