Fix PortsForwarder.Expose() proxy check

[cpick]

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cpick
Once this PR has been reviewed and has the lgtm label, please assign baude for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[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]

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]

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]

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]

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]

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

[cfergeau]

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