-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PortsForwarder.Expose()
proxy check
#441
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cpick The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
97b8f50
to
0a63f4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {
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 <[email protected]>
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 <[email protected]>
0a63f4e
to
8992370
Compare
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. |
This break the port forwarding test:
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 |
The check for existing
proxies
entries usedlocal
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.