From 8ed4fd1dbc7f8dfe2b9d6321d922e39e2a59ab5f Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 15 Mar 2023 09:24:24 -0400 Subject: [PATCH] envknob/logknob: add package for configurable logging A LogKnob allows enabling logs with an envknob, netmap capability, and manually, and calling a logging function when logs are enabled. Signed-off-by: Andrew Dunham Change-Id: Id66c608d4e488bfd4eaa5e867a8d9289686748be --- envknob/logknob/logknob.go | 84 ++++++++++++++++++++++++++ envknob/logknob/logknob_test.go | 102 ++++++++++++++++++++++++++++++++ types/netmap/netmap.go | 11 ++++ 3 files changed, 197 insertions(+) create mode 100644 envknob/logknob/logknob.go create mode 100644 envknob/logknob/logknob_test.go diff --git a/envknob/logknob/logknob.go b/envknob/logknob/logknob.go new file mode 100644 index 000000000..ed45c7a94 --- /dev/null +++ b/envknob/logknob/logknob.go @@ -0,0 +1,84 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +// Package logknob provides a helpful wrapper that allows enabling logging +// based on either an envknob or other methods of enablement. +package logknob + +import ( + "sync/atomic" + + "golang.org/x/exp/slices" + "tailscale.com/envknob" + "tailscale.com/types/logger" +) + +// TODO(andrew-d): should we have a package-global registry of logknobs? It +// would allow us to update from a netmap in a central location, which might be +// reason enough to do it... + +// LogKnob allows configuring verbose logging, with multiple ways to enable. It +// supports enabling logging via envknob, via atomic boolean (for use in e.g. +// c2n log level changes), and via capabilities from a NetMap (so users can +// enable logging via the ACL JSON). +type LogKnob struct { + capName string + cap atomic.Bool + env func() bool + manual atomic.Bool +} + +// NewLogKnob creates a new LogKnob, with the provided environment variable +// name and/or NetMap capability. +func NewLogKnob(env, cap string) *LogKnob { + if env == "" && cap == "" { + panic("must provide either an environment variable or capability") + } + + lk := &LogKnob{ + capName: cap, + } + if env != "" { + lk.env = envknob.RegisterBool(env) + } else { + lk.env = func() bool { return false } + } + return lk +} + +// Set will cause logs to be printed when called with Set(true). When called +// with Set(false), logs will not be printed due to an earlier call of +// Set(true), but may be printed due to either the envknob and/or capability of +// this LogKnob. +func (lk *LogKnob) Set(v bool) { + lk.manual.Store(v) +} + +// NetMap is an interface for the parts of netmap.NetworkMap that we care +// about; we use this rather than a concrete type to avoid a circular +// dependency. +type NetMap interface { + SelfCapabilities() []string +} + +// UpdateFromNetMap will enable logging if the SelfNode in the provided NetMap +// contains the capability provided for this LogKnob. +func (lk *LogKnob) UpdateFromNetMap(nm NetMap) { + if lk.capName == "" { + return + } + + lk.cap.Store(slices.Contains(nm.SelfCapabilities(), lk.capName)) +} + +// Do will call log with the provided format and arguments if any of the +// configured methods for enabling logging are true. +func (lk *LogKnob) Do(log logger.Logf, format string, args ...any) { + if lk.shouldLog() { + log(format, args...) + } +} + +func (lk *LogKnob) shouldLog() bool { + return lk.manual.Load() || lk.env() || lk.cap.Load() +} diff --git a/envknob/logknob/logknob_test.go b/envknob/logknob/logknob_test.go new file mode 100644 index 000000000..425fb7804 --- /dev/null +++ b/envknob/logknob/logknob_test.go @@ -0,0 +1,102 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package logknob + +import ( + "bytes" + "fmt" + "testing" + + "tailscale.com/envknob" + "tailscale.com/tailcfg" + "tailscale.com/types/netmap" +) + +var testKnob = NewLogKnob( + "TS_TEST_LOGKNOB", + "https://tailscale.com/cap/testing", +) + +// Static type assertion for our interface type. +var _ NetMap = &netmap.NetworkMap{} + +func TestLogKnob(t *testing.T) { + t.Run("Default", func(t *testing.T) { + if testKnob.shouldLog() { + t.Errorf("expected default shouldLog()=false") + } + assertNoLogs(t) + }) + t.Run("Manual", func(t *testing.T) { + t.Cleanup(func() { testKnob.Set(false) }) + + assertNoLogs(t) + testKnob.Set(true) + if !testKnob.shouldLog() { + t.Errorf("expected shouldLog()=true") + } + assertLogs(t) + }) + t.Run("Env", func(t *testing.T) { + t.Cleanup(func() { + envknob.Setenv("TS_TEST_LOGKNOB", "") + }) + + assertNoLogs(t) + if testKnob.shouldLog() { + t.Errorf("expected default shouldLog()=false") + } + + envknob.Setenv("TS_TEST_LOGKNOB", "true") + if !testKnob.shouldLog() { + t.Errorf("expected shouldLog()=true") + } + assertLogs(t) + }) + t.Run("NetMap", func(t *testing.T) { + t.Cleanup(func() { testKnob.cap.Store(false) }) + + assertNoLogs(t) + if testKnob.shouldLog() { + t.Errorf("expected default shouldLog()=false") + } + + testKnob.UpdateFromNetMap(&netmap.NetworkMap{ + SelfNode: &tailcfg.Node{ + Capabilities: []string{ + "https://tailscale.com/cap/testing", + }, + }, + }) + if !testKnob.shouldLog() { + t.Errorf("expected shouldLog()=true") + } + assertLogs(t) + }) +} + +func assertLogs(t *testing.T) { + var buf bytes.Buffer + logf := func(format string, args ...any) { + fmt.Fprintf(&buf, format, args...) + } + + testKnob.Do(logf, "hello %s", "world") + const want = "hello world" + if got := buf.String(); got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func assertNoLogs(t *testing.T) { + var buf bytes.Buffer + logf := func(format string, args ...any) { + fmt.Fprintf(&buf, format, args...) + } + + testKnob.Do(logf, "hello %s", "world") + if got := buf.String(); got != "" { + t.Errorf("expected no logs, but got: %q", got) + } +} diff --git a/types/netmap/netmap.go b/types/netmap/netmap.go index b1fdf94b3..b3a9df8ba 100644 --- a/types/netmap/netmap.go +++ b/types/netmap/netmap.go @@ -125,6 +125,17 @@ func (nm *NetworkMap) MagicDNSSuffix() string { return name } +// SelfCapabilities returns SelfNode.Capabilities if nm and nm.SelfNode are +// non-nil. This is a method so we can use it in envknob/logknob without a +// circular dependency. +func (nm *NetworkMap) SelfCapabilities() []string { + if nm == nil || nm.SelfNode == nil { + return nil + } + + return nm.SelfNode.Capabilities +} + func (nm *NetworkMap) String() string { return nm.Concise() }