control/{controlknobs,controlclient}: simplify knobs API, fix controlclient crash

From integration tests elsewhere:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x845c9b]

goroutine 226 [running]:
tailscale.com/control/controlclient.(*Direct).sendMapRequest(0xc00053e1e0, 0x16670f0, 0xc000353780, 0xffffffffffffffff, 0xc0003e5f10, 0x0, 0x0)
   /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/direct.go:803 +0x19bb
tailscale.com/control/controlclient.(*Direct).PollNetMap(...)
   /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/direct.go:574
tailscale.com/control/controlclient.(*Auto).mapRoutine(0xc00052a1e0)
   /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/auto.go:464 +0x571
created by tailscale.com/control/controlclient.(*Auto).Start
   /home/runner/go/pkg/mod/tailscale.com@v1.1.1-0.20210715222212-1bb6abc604c1/control/controlclient/auto.go:151 +0x65
exit status 2

Also remove types/opt.Bool API addition which is now unnecessary.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz/derp_flow
Brad Fitzpatrick 3 years ago
parent 1bb6abc604
commit 171ec9f8f4

@ -800,7 +800,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netm
hasDebug := resp.Debug != nil hasDebug := resp.Debug != nil
// being conservative here, if Debug not present set to False // being conservative here, if Debug not present set to False
controlknobs.SetDisableUPnP(resp.Debug.DisableUPnP.And(hasDebug)) controlknobs.SetDisableUPnP(hasDebug && resp.Debug.DisableUPnP.EqualBool(true))
if hasDebug { if hasDebug {
if resp.Debug.LogHeapPprof { if resp.Debug.LogHeapPprof {
go logheap.LogHeap(resp.Debug.LogHeapURL) go logheap.LogHeap(resp.Debug.LogHeapURL)

@ -9,33 +9,26 @@ package controlknobs
import ( import (
"os" "os"
"strconv" "strconv"
"sync/atomic"
"tailscale.com/types/opt" "tailscale.com/syncs"
) )
// disableUPnP indicates whether to attempt UPnP mapping. // disableUPnP indicates whether to attempt UPnP mapping.
var disableUPnP atomic.Value var disableUPnP syncs.AtomicBool
func init() { func init() {
v, _ := strconv.ParseBool(os.Getenv("TS_DISABLE_UPNP")) v, _ := strconv.ParseBool(os.Getenv("TS_DISABLE_UPNP"))
var toStore opt.Bool SetDisableUPnP(v)
toStore.Set(v)
disableUPnP.Store(toStore)
} }
// DisableUPnP reports the last reported value from control // DisableUPnP reports the last reported value from control
// whether UPnP portmapping should be disabled. // whether UPnP portmapping should be disabled.
func DisableUPnP() opt.Bool { func DisableUPnP() bool {
v, _ := disableUPnP.Load().(opt.Bool) return disableUPnP.Get()
return v
} }
// SetDisableUPnP will set whether UPnP connections are permitted or not, // SetDisableUPnP sets whether control says that UPnP should be
// intended to be set from control. // disabled.
func SetDisableUPnP(v opt.Bool) { func SetDisableUPnP(v bool) {
old, ok := disableUPnP.Load().(opt.Bool) disableUPnP.Set(v)
if !ok || old != v {
disableUPnP.Store(v)
}
} }

@ -125,7 +125,7 @@ func addAnyPortMapping(
// now. // now.
// Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md. // Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md.
func getUPnPClient(ctx context.Context, gw netaddr.IP) (upnpClient, error) { func getUPnPClient(ctx context.Context, gw netaddr.IP) (upnpClient, error) {
if dis, ok := controlknobs.DisableUPnP().Get(); ok && dis { if controlknobs.DisableUPnP() {
return nil, nil return nil, nil
} }
ctx, cancel := context.WithTimeout(ctx, 250*time.Millisecond) ctx, cancel := context.WithTimeout(ctx, 250*time.Millisecond)
@ -177,7 +177,7 @@ func (c *Client) getUPnPPortMapping(
internal netaddr.IPPort, internal netaddr.IPPort,
prevPort uint16, prevPort uint16,
) (external netaddr.IPPort, ok bool) { ) (external netaddr.IPPort, ok bool) {
if dis, ok := controlknobs.DisableUPnP().Get(); ok && dis { if controlknobs.DisableUPnP() {
return netaddr.IPPort{}, false return netaddr.IPPort{}, false
} }
now := time.Now() now := time.Now()

@ -29,13 +29,6 @@ func (b Bool) Get() (v bool, ok bool) {
return v, err == nil return v, err == nil
} }
func (b Bool) And(v bool) Bool {
if v {
return b
}
return "false"
}
// EqualBool reports whether b is equal to v. // EqualBool reports whether b is equal to v.
// If b is empty or not a valid bool, it reports false. // If b is empty or not a valid bool, it reports false.
func (b Bool) EqualBool(v bool) bool { func (b Bool) EqualBool(v bool) bool {

@ -87,26 +87,3 @@ func TestBoolEqualBool(t *testing.T) {
} }
} }
func TestBoolAnd(t *testing.T) {
tests := []struct {
lhs Bool
rhs bool
want Bool
}{
{"true", true, "true"},
{"true", false, "false"},
{"false", true, "false"},
{"false", false, "false"},
{"", true, ""},
{"", false, "false"},
}
for _, tt := range tests {
if got := tt.lhs.And(tt.rhs); got != tt.want {
t.Errorf("(%q).And(%v) = %v; want %v", string(tt.lhs), tt.rhs, got, tt.want)
}
}
}

Loading…
Cancel
Save