From 57a008a1e17438ffd7b30cd8d8f16e4097ab131f Mon Sep 17 00:00:00 2001 From: Will Norris Date: Thu, 23 Mar 2023 10:49:56 -0700 Subject: [PATCH] all: pass log IDs as the proper type rather than strings This change focuses on the backend log ID, which is the mostly commonly used in the client. Tests which don't seem to make use of the log ID just use the zero value. Signed-off-by: Will Norris --- cmd/tailscaled/tailscaled.go | 14 +++++++------- cmd/tailscaled/tailscaled_windows.go | 8 +++++--- cmd/tsconnect/wasm/wasm_js.go | 2 +- ipn/ipnlocal/local.go | 13 +++++++------ ipn/ipnlocal/local_test.go | 5 +++-- ipn/ipnlocal/loglines_test.go | 2 +- ipn/ipnlocal/state_test.go | 7 ++++--- ipn/ipnserver/server.go | 7 ++++--- ipn/localapi/localapi.go | 5 +++-- log/sockstatlog/logger.go | 10 +++++----- ssh/tailssh/tailssh_test.go | 3 ++- tsnet/tsnet.go | 6 +++--- tstest/integration/tailscaled_deps_test_darwin.go | 1 + tstest/integration/tailscaled_deps_test_freebsd.go | 1 + tstest/integration/tailscaled_deps_test_linux.go | 1 + tstest/integration/tailscaled_deps_test_openbsd.go | 1 + tstest/integration/tailscaled_deps_test_windows.go | 1 + wgengine/netstack/netstack_test.go | 5 +++-- 18 files changed, 53 insertions(+), 39 deletions(-) diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go index 88686ea50..b559dc74d 100644 --- a/cmd/tailscaled/tailscaled.go +++ b/cmd/tailscaled/tailscaled.go @@ -51,6 +51,7 @@ "tailscale.com/tsweb" "tailscale.com/types/flagtype" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/util/clientmetric" "tailscale.com/util/multierr" "tailscale.com/util/osshare" @@ -377,11 +378,10 @@ func run() error { debugMux = newDebugMux() } - logid := pol.PublicID.String() - return startIPNServer(context.Background(), logf, logid) + return startIPNServer(context.Background(), logf, pol.PublicID) } -func startIPNServer(ctx context.Context, logf logger.Logf, logid string) error { +func startIPNServer(ctx context.Context, logf logger.Logf, logID logid.PublicID) error { ln, err := safesocket.Listen(args.socketpath) if err != nil { return fmt.Errorf("safesocket.Listen: %v", err) @@ -407,7 +407,7 @@ func startIPNServer(ctx context.Context, logf logger.Logf, logid string) error { } }() - srv := ipnserver.New(logf, logid) + srv := ipnserver.New(logf, logID) if debugMux != nil { debugMux.HandleFunc("/debug/ipn", srv.ServeHTMLStatus) } @@ -425,7 +425,7 @@ func startIPNServer(ctx context.Context, logf logger.Logf, logid string) error { return } } - lb, err := getLocalBackend(ctx, logf, logid) + lb, err := getLocalBackend(ctx, logf, logID) if err == nil { logf("got LocalBackend in %v", time.Since(t0).Round(time.Millisecond)) srv.SetLocalBackend(lb) @@ -449,7 +449,7 @@ func startIPNServer(ctx context.Context, logf logger.Logf, logid string) error { return nil } -func getLocalBackend(ctx context.Context, logf logger.Logf, logid string) (_ *ipnlocal.LocalBackend, retErr error) { +func getLocalBackend(ctx context.Context, logf logger.Logf, logID logid.PublicID) (_ *ipnlocal.LocalBackend, retErr error) { linkMon, err := monitor.New(logf) if err != nil { return nil, fmt.Errorf("monitor.New: %w", err) @@ -520,7 +520,7 @@ func getLocalBackend(ctx context.Context, logf logger.Logf, logid string) (_ *ip return nil, fmt.Errorf("store.New: %w", err) } - lb, err := ipnlocal.NewLocalBackend(logf, logid, store, dialer, e, opts.LoginFlags) + lb, err := ipnlocal.NewLocalBackend(logf, logID, store, dialer, e, opts.LoginFlags) if err != nil { return nil, fmt.Errorf("ipnlocal.NewLocalBackend: %w", err) } diff --git a/cmd/tailscaled/tailscaled_windows.go b/cmd/tailscaled/tailscaled_windows.go index 46ca2b2f5..a654dadf4 100644 --- a/cmd/tailscaled/tailscaled_windows.go +++ b/cmd/tailscaled/tailscaled_windows.go @@ -47,6 +47,7 @@ "tailscale.com/net/dns" "tailscale.com/net/tstun" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/util/winutil" "tailscale.com/version" "tailscale.com/wf" @@ -262,13 +263,13 @@ func beWindowsSubprocess() bool { if len(os.Args) != 3 || os.Args[1] != "/subproc" { return false } - logid := os.Args[2] + logID := os.Args[2] // Remove the date/time prefix; the logtail + file loggers add it. log.SetFlags(0) log.Printf("Program starting: v%v: %#v", version.Long(), os.Args) - log.Printf("subproc mode: logid=%v", logid) + log.Printf("subproc mode: logid=%v", logID) if err := envknob.ApplyDiskConfigError(); err != nil { log.Printf("Error reading environment config: %v", err) } @@ -290,7 +291,8 @@ func beWindowsSubprocess() bool { } }() - err := startIPNServer(ctx, log.Printf, logid) + publicLogID, _ := logid.ParsePublicID(logID) + err := startIPNServer(ctx, log.Printf, publicLogID) if err != nil { log.Fatalf("ipnserver: %v", err) } diff --git a/cmd/tsconnect/wasm/wasm_js.go b/cmd/tsconnect/wasm/wasm_js.go index f1bd1c55d..154e3c2d0 100644 --- a/cmd/tsconnect/wasm/wasm_js.go +++ b/cmd/tsconnect/wasm/wasm_js.go @@ -122,7 +122,7 @@ func newIPN(jsConfig js.Value) map[string]any { return ns.DialContextTCP(ctx, dst) } - logid := lpc.PublicID.String() + logid := lpc.PublicID srv := ipnserver.New(logf, logid) lb, err := ipnlocal.NewLocalBackend(logf, logid, store, dialer, eng, controlclient.LoginEphemeral) if err != nil { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index cc00ece69..caf323064 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -63,6 +63,7 @@ "tailscale.com/types/empty" "tailscale.com/types/key" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/types/netmap" "tailscale.com/types/persist" "tailscale.com/types/preftype" @@ -139,7 +140,7 @@ type LocalBackend struct { pm *profileManager store ipn.StateStore dialer *tsdial.Dialer // non-nil - backendLogID string + backendLogID logid.PublicID unregisterLinkMon func() unregisterHealthWatch func() portpoll *portlist.Poller // may be nil @@ -265,7 +266,7 @@ type LocalBackend struct { // but is not actually running. // // If dialer is nil, a new one is made. -func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, dialer *tsdial.Dialer, e wgengine.Engine, loginFlags controlclient.LoginFlags) (*LocalBackend, error) { +func NewLocalBackend(logf logger.Logf, logID logid.PublicID, store ipn.StateStore, dialer *tsdial.Dialer, e wgengine.Engine, loginFlags controlclient.LoginFlags) (*LocalBackend, error) { if e == nil { panic("ipn.NewLocalBackend: engine must not be nil") } @@ -300,7 +301,7 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, diale pm: pm, store: pm.Store(), dialer: dialer, - backendLogID: logid, + backendLogID: logID, state: ipn.NoState, portpoll: portpoll, em: newExpiryManager(logf), @@ -310,7 +311,7 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, diale // for now, only log sockstats on unstable builds if version.IsUnstableBuild() { - b.sockstatLogger, err = sockstatlog.NewLogger(logpolicy.LogsDir(logf), logf, logid) + b.sockstatLogger, err = sockstatlog.NewLogger(logpolicy.LogsDir(logf), logf, logID) if err != nil { log.Printf("error setting up sockstat logger: %v", err) } @@ -1294,7 +1295,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { } hostinfo := hostinfo.New() - hostinfo.BackendLogID = b.backendLogID + hostinfo.BackendLogID = b.backendLogID.String() hostinfo.FrontendLogID = opts.FrontendLogID hostinfo.Userspace.Set(wgengine.IsNetstack(b.e)) hostinfo.UserspaceRouter.Set(wgengine.IsNetstackRouter(b.e)) @@ -1448,7 +1449,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { b.e.SetNetInfoCallback(b.setNetInfo) - blid := b.backendLogID + blid := b.backendLogID.String() b.logf("Backend: logs: be:%v fe:%v", blid, opts.FrontendLogID) b.send(ipn.Notify{BackendLogID: &blid}) b.send(ipn.Notify{Prefs: &prefs}) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index cc105f0a8..2a474e46d 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -23,6 +23,7 @@ "tailscale.com/tstest" "tailscale.com/types/key" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/types/netmap" "tailscale.com/wgengine" "tailscale.com/wgengine/filter" @@ -506,7 +507,7 @@ func TestLazyMachineKeyGeneration(t *testing.T) { t.Fatalf("NewFakeUserspaceEngine: %v", err) } t.Cleanup(eng.Close) - lb, err := NewLocalBackend(logf, "logid", store, nil, eng, 0) + lb, err := NewLocalBackend(logf, logid.PublicID{}, store, nil, eng, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) } @@ -770,7 +771,7 @@ func TestStatusWithoutPeers(t *testing.T) { } t.Cleanup(e.Close) - b, err := NewLocalBackend(logf, "logid", store, nil, e, 0) + b, err := NewLocalBackend(logf, logid.PublicID{}, store, nil, e, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) } diff --git a/ipn/ipnlocal/loglines_test.go b/ipn/ipnlocal/loglines_test.go index 55c403cc1..cde3f9198 100644 --- a/ipn/ipnlocal/loglines_test.go +++ b/ipn/ipnlocal/loglines_test.go @@ -54,7 +54,7 @@ func TestLocalLogLines(t *testing.T) { } t.Cleanup(e.Close) - lb, err := NewLocalBackend(logf, idA.String(), store, nil, e, 0) + lb, err := NewLocalBackend(logf, idA, store, nil, e, 0) if err != nil { t.Fatal(err) } diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index c89061b95..7a2c7132e 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -21,6 +21,7 @@ "tailscale.com/types/empty" "tailscale.com/types/key" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/types/netmap" "tailscale.com/types/persist" "tailscale.com/wgengine" @@ -303,7 +304,7 @@ func TestStateMachine(t *testing.T) { } t.Cleanup(e.Close) - b, err := NewLocalBackend(logf, "logid", store, nil, e, 0) + b, err := NewLocalBackend(logf, logid.PublicID{}, store, nil, e, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) } @@ -946,7 +947,7 @@ func TestEditPrefsHasNoKeys(t *testing.T) { } t.Cleanup(e.Close) - b, err := NewLocalBackend(logf, "logid", new(mem.Store), nil, e, 0) + b, err := NewLocalBackend(logf, logid.PublicID{}, new(mem.Store), nil, e, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) } @@ -1025,7 +1026,7 @@ func TestWGEngineStatusRace(t *testing.T) { eng, err := wgengine.NewFakeUserspaceEngine(logf, 0) c.Assert(err, qt.IsNil) t.Cleanup(eng.Close) - b, err := NewLocalBackend(logf, "logid", new(mem.Store), nil, eng, 0) + b, err := NewLocalBackend(logf, logid.PublicID{}, new(mem.Store), nil, eng, 0) c.Assert(err, qt.IsNil) var cc *mockControl diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 355c02193..0123d9e9e 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -25,6 +25,7 @@ "tailscale.com/ipn/ipnlocal" "tailscale.com/ipn/localapi" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/util/mak" "tailscale.com/util/set" "tailscale.com/util/systemd" @@ -35,7 +36,7 @@ type Server struct { lb atomic.Pointer[ipnlocal.LocalBackend] logf logger.Logf - backendLogID string + backendLogID logid.PublicID // resetOnZero is whether to call bs.Reset on transition from // 1->0 active HTTP requests. That is, this is whether the backend is // being run in "client mode" that requires an active GUI @@ -412,9 +413,9 @@ func (s *Server) addActiveHTTPRequest(req *http.Request, ci *ipnauth.ConnIdentit // // At some point, either before or after Run, the Server's SetLocalBackend // method must also be called before Server can do anything useful. -func New(logf logger.Logf, logid string) *Server { +func New(logf logger.Logf, logID logid.PublicID) *Server { return &Server{ - backendLogID: logid, + backendLogID: logID, logf: logf, resetOnZero: envknob.GOOS() == "windows", } diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 688b76290..5d7b36e9a 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -40,6 +40,7 @@ "tailscale.com/tka" "tailscale.com/types/key" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/types/ptr" "tailscale.com/util/clientmetric" "tailscale.com/util/httpm" @@ -124,7 +125,7 @@ func randHex(n int) string { metrics = map[string]*clientmetric.Metric{} ) -func NewHandler(b *ipnlocal.LocalBackend, logf logger.Logf, logID string) *Handler { +func NewHandler(b *ipnlocal.LocalBackend, logf logger.Logf, logID logid.PublicID) *Handler { return &Handler{b: b, logf: logf, backendLogID: logID} } @@ -149,7 +150,7 @@ type Handler struct { b *ipnlocal.LocalBackend logf logger.Logf - backendLogID string + backendLogID logid.PublicID } func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { diff --git a/log/sockstatlog/logger.go b/log/sockstatlog/logger.go index ace9ad946..4ac84c154 100644 --- a/log/sockstatlog/logger.go +++ b/log/sockstatlog/logger.go @@ -58,17 +58,17 @@ type event struct { } // SockstatLogID reproducibly derives a new logid.PrivateID for sockstat logging from a node's public backend log ID. -// The returned PrivateID is the sha256 sum of id + "sockstat". +// The returned PrivateID is the sha256 sum of logID + "sockstat". // If a node's public log ID becomes known, it is trivial to spoof sockstat logs for that node. // Given the this is just for debugging, we're not too concerned about that. -func SockstatLogID(id string) logid.PrivateID { - return logid.PrivateID(sha256.Sum256([]byte(id + "sockstat"))) +func SockstatLogID(logID logid.PublicID) logid.PrivateID { + return logid.PrivateID(sha256.Sum256([]byte(logID.String() + "sockstat"))) } // NewLogger returns a new Logger that will store stats in logdir. // On platforms that do not support sockstat logging, a nil Logger will be returned. // The returned Logger must be shut down with Shutdown when it is no longer needed. -func NewLogger(logdir string, logf logger.Logf, backendLogID string) (*Logger, error) { +func NewLogger(logdir string, logf logger.Logf, logID logid.PublicID) (*Logger, error) { if !sockstats.IsAvailable { return nil, nil } @@ -91,7 +91,7 @@ func NewLogger(logdir string, logf logger.Logf, backendLogID string) (*Logger, e } logger.logger = logtail.NewLogger(logtail.Config{ BaseURL: logpolicy.LogURL(), - PrivateID: SockstatLogID(backendLogID), + PrivateID: SockstatLogID(logID), Collection: "sockstats.log.tailscale.io", Buffer: filch, NewZstdEncoder: func() logtail.Encoder { diff --git a/ssh/tailssh/tailssh_test.go b/ssh/tailssh/tailssh_test.go index bdd237376..0fb580440 100644 --- a/ssh/tailssh/tailssh_test.go +++ b/ssh/tailssh/tailssh_test.go @@ -38,6 +38,7 @@ "tailscale.com/tempfork/gliderlabs/ssh" "tailscale.com/tstest" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/types/netmap" "tailscale.com/util/cibuild" "tailscale.com/util/lineread" @@ -505,7 +506,7 @@ func TestSSH(t *testing.T) { if err != nil { t.Fatal(err) } - lb, err := ipnlocal.NewLocalBackend(logf, "", + lb, err := ipnlocal.NewLocalBackend(logf, logid.PublicID{}, new(mem.Store), new(tsdial.Dialer), eng, 0) diff --git a/tsnet/tsnet.go b/tsnet/tsnet.go index dbde63aa7..ee4e4db0b 100644 --- a/tsnet/tsnet.go +++ b/tsnet/tsnet.go @@ -48,6 +48,7 @@ "tailscale.com/net/tsdial" "tailscale.com/smallzstd" "tailscale.com/types/logger" + "tailscale.com/types/logid" "tailscale.com/types/nettype" "tailscale.com/util/mak" "tailscale.com/wgengine" @@ -118,7 +119,7 @@ type Server struct { localClient *tailscale.LocalClient // in-memory logbuffer *filch.Filch logtail *logtail.Logger - logid string + logid logid.PublicID mu sync.Mutex listeners map[listenKey]*listener @@ -573,7 +574,6 @@ func (s *Server) start() (reterr error) { func (s *Server) startLogger(closePool *closeOnErrorPool) error { if inTest() { - s.logid = "test" return nil } cfgPath := filepath.Join(s.rootPath, "tailscaled.log.conf") @@ -590,7 +590,7 @@ func (s *Server) startLogger(closePool *closeOnErrorPool) error { if err := lpc.Validate(logtail.CollectionNode); err != nil { return fmt.Errorf("logpolicy.Config.Validate for %v: %w", cfgPath, err) } - s.logid = lpc.PublicID.String() + s.logid = lpc.PublicID s.logbuffer, err = filch.New(filepath.Join(s.rootPath, "tailscaled"), filch.Options{ReplaceStderr: false}) if err != nil { diff --git a/tstest/integration/tailscaled_deps_test_darwin.go b/tstest/integration/tailscaled_deps_test_darwin.go index 4f193e69f..c8d6fc23a 100644 --- a/tstest/integration/tailscaled_deps_test_darwin.go +++ b/tstest/integration/tailscaled_deps_test_darwin.go @@ -40,6 +40,7 @@ _ "tailscale.com/types/flagtype" _ "tailscale.com/types/key" _ "tailscale.com/types/logger" + _ "tailscale.com/types/logid" _ "tailscale.com/util/clientmetric" _ "tailscale.com/util/multierr" _ "tailscale.com/util/osshare" diff --git a/tstest/integration/tailscaled_deps_test_freebsd.go b/tstest/integration/tailscaled_deps_test_freebsd.go index 4f193e69f..c8d6fc23a 100644 --- a/tstest/integration/tailscaled_deps_test_freebsd.go +++ b/tstest/integration/tailscaled_deps_test_freebsd.go @@ -40,6 +40,7 @@ _ "tailscale.com/types/flagtype" _ "tailscale.com/types/key" _ "tailscale.com/types/logger" + _ "tailscale.com/types/logid" _ "tailscale.com/util/clientmetric" _ "tailscale.com/util/multierr" _ "tailscale.com/util/osshare" diff --git a/tstest/integration/tailscaled_deps_test_linux.go b/tstest/integration/tailscaled_deps_test_linux.go index 4f193e69f..c8d6fc23a 100644 --- a/tstest/integration/tailscaled_deps_test_linux.go +++ b/tstest/integration/tailscaled_deps_test_linux.go @@ -40,6 +40,7 @@ _ "tailscale.com/types/flagtype" _ "tailscale.com/types/key" _ "tailscale.com/types/logger" + _ "tailscale.com/types/logid" _ "tailscale.com/util/clientmetric" _ "tailscale.com/util/multierr" _ "tailscale.com/util/osshare" diff --git a/tstest/integration/tailscaled_deps_test_openbsd.go b/tstest/integration/tailscaled_deps_test_openbsd.go index 4f193e69f..c8d6fc23a 100644 --- a/tstest/integration/tailscaled_deps_test_openbsd.go +++ b/tstest/integration/tailscaled_deps_test_openbsd.go @@ -40,6 +40,7 @@ _ "tailscale.com/types/flagtype" _ "tailscale.com/types/key" _ "tailscale.com/types/logger" + _ "tailscale.com/types/logid" _ "tailscale.com/util/clientmetric" _ "tailscale.com/util/multierr" _ "tailscale.com/util/osshare" diff --git a/tstest/integration/tailscaled_deps_test_windows.go b/tstest/integration/tailscaled_deps_test_windows.go index 4ae702703..bb69d70ae 100644 --- a/tstest/integration/tailscaled_deps_test_windows.go +++ b/tstest/integration/tailscaled_deps_test_windows.go @@ -47,6 +47,7 @@ _ "tailscale.com/types/flagtype" _ "tailscale.com/types/key" _ "tailscale.com/types/logger" + _ "tailscale.com/types/logid" _ "tailscale.com/util/clientmetric" _ "tailscale.com/util/multierr" _ "tailscale.com/util/osshare" diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index b8f2eab0e..7a30fd84a 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -18,6 +18,7 @@ "tailscale.com/net/tstun" "tailscale.com/tstest" "tailscale.com/types/ipproto" + "tailscale.com/types/logid" "tailscale.com/wgengine" "tailscale.com/wgengine/filter" ) @@ -49,7 +50,7 @@ func TestInjectInboundLeak(t *testing.T) { t.Fatal("failed to get internals") } - lb, err := ipnlocal.NewLocalBackend(logf, "logid", new(mem.Store), dialer, eng, 0) + lb, err := ipnlocal.NewLocalBackend(logf, logid.PublicID{}, new(mem.Store), dialer, eng, 0) if err != nil { t.Fatal(err) } @@ -113,7 +114,7 @@ func makeNetstack(t *testing.T, config func(*Impl)) *Impl { } t.Cleanup(func() { ns.Close() }) - lb, err := ipnlocal.NewLocalBackend(logf, "logid", new(mem.Store), dialer, eng, 0) + lb, err := ipnlocal.NewLocalBackend(logf, logid.PublicID{}, new(mem.Store), dialer, eng, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) }