settings: improve API and documentation

This commit improves documentation around masked vs non-masked
settings and improves the API: instead of a single `Lookup` function
with a "purpose" argument, we have two separate variants. This is
better because the two variants want to return different interfaces
(`Setting` vs `NonMaskedSetting`); in the latter case
callers had to do a type assertion.

We also unexport `MaskedSetting` as it is an internal wrapper only.

Release note: None
Epic: none

pkg/cli/gen.go

@@ -225,7 +225,7 @@ Output the list of cluster settings known to this binary.
 
 		var rows [][]string
 		for _, name := range settings.Keys(settings.ForSystemTenant) {
-			setting, ok := settings.Lookup(name, settings.LookupForLocalAccess, settings.ForSystemTenant)
+			setting, ok := settings.LookupForLocalAccess(name, settings.ForSystemTenant)
 			if !ok {
 				panic(fmt.Sprintf("could not find setting %q", name))
 			}

pkg/server/admin.go

@@ -1954,18 +1954,16 @@ func (s *adminServer) Settings(
 		return nil, serverError(ctx, err)
 	}
 
-	var lookupPurpose settings.LookupPurpose
+	redactValues := true
 	if isAdmin {
 		// Root accesses can customize the purpose.
 		// This is used by the UI to see all values (local access)
 		// and `cockroach zip` to redact the values (telemetry).
-		lookupPurpose = settings.LookupForReporting
 		if req.UnredactedValues {
-			lookupPurpose = settings.LookupForLocalAccess
+			redactValues = false
 		}
 	} else {
 		// Non-root access cannot see the values in any case.
-		lookupPurpose = settings.LookupForReporting
 		if err := s.adminPrivilegeChecker.requireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil {
 			return nil, err
 		}
@@ -1998,7 +1996,13 @@ func (s *adminServer) Settings(
 
 	resp := serverpb.SettingsResponse{KeyValues: make(map[string]serverpb.SettingsResponse_Value)}
 	for _, k := range keys {
-		v, ok := settings.Lookup(k, lookupPurpose, settings.ForSystemTenant)
+		var v settings.Setting
+		var ok bool
+		if redactValues {
+			v, ok = settings.LookupForReporting(k, settings.ForSystemTenant)
+		} else {
+			v, ok = settings.LookupForLocalAccess(k, settings.ForSystemTenant)
+		}
 		if !ok {
 			continue
 		}

pkg/server/admin_test.go

@@ -1180,7 +1180,7 @@ func TestAdminAPISettings(t *testing.T) {
 	allKeys := settings.Keys(settings.ForSystemTenant)
 
 	checkSetting := func(t *testing.T, k string, v serverpb.SettingsResponse_Value) {
-		ref, ok := settings.Lookup(k, settings.LookupForReporting, settings.ForSystemTenant)
+		ref, ok := settings.LookupForReporting(k, settings.ForSystemTenant)
 		if !ok {
 			t.Fatalf("%s: not found after initial lookup", k)
 		}

pkg/server/settingswatcher/settings_watcher.go

@@ -247,7 +247,7 @@ func (s *SettingsWatcher) handleKV(
 	}
 
 	if !s.codec.ForSystemTenant() {
-		setting, ok := settings.Lookup(name, settings.LookupForLocalAccess, s.codec.ForSystemTenant())
+		setting, ok := settings.LookupForLocalAccess(name, s.codec.ForSystemTenant())
 		if !ok {
 			log.Warningf(ctx, "unknown setting %s, skipping update", redact.Safe(name))
 			return nil
@@ -337,18 +337,14 @@ func (s *SettingsWatcher) setLocked(ctx context.Context, key string, val setting
 
 // setDefaultLocked sets a setting to its default value.
 func (s *SettingsWatcher) setDefaultLocked(ctx context.Context, key string) {
-	setting, ok := settings.Lookup(key, settings.LookupForLocalAccess, s.codec.ForSystemTenant())
+	setting, ok := settings.LookupForLocalAccess(key, s.codec.ForSystemTenant())
 	if !ok {
 		log.Warningf(ctx, "failed to find setting %s, skipping update", redact.Safe(key))
 		return
 	}
-	ws, ok := setting.(settings.NonMaskedSetting)
-	if !ok {
-		log.Fatalf(ctx, "expected non-masked setting, got %T", s)
-	}
 	val := settings.EncodedValue{
-		Value: ws.EncodedDefault(),
-		Type:  ws.Typ(),
+		Value: setting.EncodedDefault(),
+		Type:  setting.Typ(),
 	}
 	s.setLocked(ctx, key, val)
 }

pkg/server/settingswatcher/settings_watcher_external_test.go

@@ -236,14 +236,14 @@ func TestSettingsWatcherWithOverrides(t *testing.T) {
 
 	expect := func(setting, value string) {
 		t.Helper()
-		s, ok := settings.Lookup(setting, settings.LookupForLocalAccess, settings.ForSystemTenant)
+		s, ok := settings.LookupForLocalAccess(setting, settings.ForSystemTenant)
 		require.True(t, ok)
 		require.Equal(t, value, s.String(&st.SV))
 	}
 
 	expectSoon := func(setting, value string) {
 		t.Helper()
-		s, ok := settings.Lookup(setting, settings.LookupForLocalAccess, settings.ForSystemTenant)
+		s, ok := settings.LookupForLocalAccess(setting, settings.ForSystemTenant)
 		require.True(t, ok)
 		testutils.SucceedsSoon(t, func() error {
 			if actual := s.String(&st.SV); actual != value {
@@ -292,7 +292,7 @@ func TestSettingsWatcherWithOverrides(t *testing.T) {
 	expectSoon("i1", "10")
 
 	// Verify that version cannot be overridden.
-	version, ok := settings.Lookup("version", settings.LookupForLocalAccess, settings.ForSystemTenant)
+	version, ok := settings.LookupForLocalAccess("version", settings.ForSystemTenant)
 	require.True(t, ok)
 	versionValue := version.String(&st.SV)
 
@@ -423,7 +423,7 @@ func TestOverflowRestart(t *testing.T) {
 // two settings do not match. It generally gets used with SucceeedsSoon.
 func CheckSettingsValuesMatch(t *testing.T, a, b *cluster.Settings) error {
 	for _, k := range settings.Keys(false /* forSystemTenant */) {
-		s, ok := settings.Lookup(k, settings.LookupForLocalAccess, false /* forSystemTenant */)
+		s, ok := settings.LookupForLocalAccess(k, false /* forSystemTenant */)
 		require.True(t, ok)
 		if s.Class() == settings.SystemOnly {
 			continue

pkg/settings/masked.go

@@ -10,57 +10,52 @@
 
 package settings
 
-// MaskedSetting is a pseudo-variable constructed on-the-fly by Lookup
-// when the actual setting is non-reportable.
-type MaskedSetting struct {
+// maskedSetting is a wrapper for non-reportable settings that were retrieved
+// for reporting (see SetReportable and LookupForReporting).
+type maskedSetting struct {
 	setting NonMaskedSetting
 }
 
-var _ Setting = &MaskedSetting{}
-
-// UnderlyingSetting retrieves the actual setting object.
-func (s *MaskedSetting) UnderlyingSetting() NonMaskedSetting {
-	return s.setting
-}
+var _ Setting = &maskedSetting{}
 
 // String hides the underlying value.
-func (s *MaskedSetting) String(sv *Values) string {
+func (s *maskedSetting) String(sv *Values) string {
 	// Special case for non-reportable strings: we still want
 	// to distinguish empty from non-empty (= customized).
-	if st, ok := s.UnderlyingSetting().(*StringSetting); ok && st.String(sv) == "" {
+	if st, ok := s.setting.(*StringSetting); ok && st.String(sv) == "" {
 		return ""
 	}
 	return "<redacted>"
 }
 
 // Visibility returns the visibility setting for the underlying setting.
-func (s *MaskedSetting) Visibility() Visibility {
+func (s *maskedSetting) Visibility() Visibility {
 	return s.setting.Visibility()
 }
 
 // Key returns the key string for the underlying setting.
-func (s *MaskedSetting) Key() string {
+func (s *maskedSetting) Key() string {
 	return s.setting.Key()
 }
 
 // Description returns the description string for the underlying setting.
-func (s *MaskedSetting) Description() string {
+func (s *maskedSetting) Description() string {
 	return s.setting.Description()
 }
 
 // Typ returns the short (1 char) string denoting the type of setting.
-func (s *MaskedSetting) Typ() string {
+func (s *maskedSetting) Typ() string {
 	return s.setting.Typ()
 }
 
 // Class returns the class for the underlying setting.
-func (s *MaskedSetting) Class() Class {
+func (s *maskedSetting) Class() Class {
 	return s.setting.Class()
 }
 
 // TestingIsReportable is used in testing for reportability.
 func TestingIsReportable(s Setting) bool {
-	if _, ok := s.(*MaskedSetting); ok {
+	if _, ok := s.(*maskedSetting); ok {
 		return false
 	}
 	if e, ok := s.(internalSetting); ok {

pkg/settings/registry.go

@@ -278,35 +278,38 @@ func Keys(forSystemTenant bool) (res []string) {
 	return res
 }
 
-// Lookup returns a Setting by name along with its description.
-// For non-reportable setting, it instantiates a MaskedSetting
-// to masquerade for the underlying setting.
-func Lookup(name string, purpose LookupPurpose, forSystemTenant bool) (Setting, bool) {
+// LookupForLocalAccess returns a NonMaskedSetting by name. Used when a setting
+// is being retrieved for local processing within the cluster and not for
+// reporting; sensitive values are accessible.
+func LookupForLocalAccess(name string, forSystemTenant bool) (NonMaskedSetting, bool) {
 	s, ok := registry[name]
 	if !ok {
 		return nil, false
 	}
 	if !forSystemTenant && s.Class() == SystemOnly {
 		return nil, false
 	}
-	if purpose == LookupForReporting && !s.isReportable() {
-		return &MaskedSetting{setting: s}, true
-	}
 	return s, true
 }
 
-// LookupPurpose indicates what is being done with the setting.
-type LookupPurpose int
-
-const (
-	// LookupForReporting indicates that a setting is being retrieved
-	// for reporting and sensitive values should be scrubbed.
-	LookupForReporting LookupPurpose = iota
-	// LookupForLocalAccess indicates that a setting is being
-	// retrieved for local processing within the cluster and
-	// all values should be accessible
-	LookupForLocalAccess
-)
+// LookupForReporting returns a Setting by name. Used when a setting is being
+// retrieved for reporting.
+//
+// For settings that are non-reportable, the returned Setting hides the current
+// value (see Setting.String).
+func LookupForReporting(name string, forSystemTenant bool) (Setting, bool) {
+	s, ok := registry[name]
+	if !ok {
+		return nil, false
+	}
+	if !forSystemTenant && s.Class() == SystemOnly {
+		return nil, false
+	}
+	if !s.isReportable() {
+		return &maskedSetting{setting: s}, true
+	}
+	return s, true
+}
 
 // ForSystemTenant can be passed to Lookup for code that runs only on the system
 // tenant.
@@ -325,11 +328,13 @@ var ReadableTypes = map[string]string{
 	"m": "version",
 }
 
-// RedactedValue returns a string representation of the value for settings
-// types the are not considered sensitive (numbers, bools, etc) or
-// <redacted> for those with values could store sensitive things (i.e. strings).
+// RedactedValue returns:
+//   - a string representation of the value, if the setting is reportable (or it
+//     is a string setting with an empty value);
+//   - "<redacted>" if the setting is not reportable;
+//   - "<unknown>" if there is no setting with this name.
 func RedactedValue(name string, values *Values, forSystemTenant bool) string {
-	if setting, ok := Lookup(name, LookupForReporting, forSystemTenant); ok {
+	if setting, ok := LookupForReporting(name, forSystemTenant); ok {
 		return setting.String(values)
 	}
 	return "<unknown>"

pkg/settings/setting.go

@@ -31,19 +31,27 @@ type Setting interface {
 	// String returns the string representation of the setting's current value.
 	// It's used when materializing results for `SHOW CLUSTER SETTINGS` or `SHOW
 	// CLUSTER SETTING <setting-name>`.
+	//
+	// If this object implements a non-reportable setting that was retrieved for
+	// reporting (see LookupForReporting), String hides the actual value.
 	String(sv *Values) string
 
 	// Description contains a helpful text explaining what the specific cluster
 	// setting is for.
 	Description() string
 
-	// Visibility returns whether or not the setting is made publicly visible.
-	// Reserved settings are still accessible to users, but they don't get listed
-	// out when retrieving all settings.
+	// Visibility returns whether the setting is made publicly visible. Reserved
+	// settings are still accessible to users, but they don't get listed out when
+	// retrieving all settings.
 	Visibility() Visibility
 }
 
-// NonMaskedSetting is the exported interface of non-masked settings.
+// NonMaskedSetting is the exported interface of non-masked settings. A
+// non-masked setting provides access to the current value (even if the setting
+// is not reportable).
+//
+// A non-masked setting must not be used in the context of reporting values (see
+// LookupForReporting).
 type NonMaskedSetting interface {
 	Setting
 

pkg/settings/settings_test.go

@@ -351,7 +351,7 @@ func TestCache(t *testing.T) {
 
 	t.Run("lookup-system", func(t *testing.T) {
 		for _, s := range []settings.Setting{i1A, iVal, fA, fVal, dA, dVal, eA, mA, duA} {
-			result, ok := settings.Lookup(s.Key(), settings.LookupForLocalAccess, settings.ForSystemTenant)
+			result, ok := settings.LookupForLocalAccess(s.Key(), settings.ForSystemTenant)
 			if !ok {
 				t.Fatalf("lookup(%s) failed", s.Key())
 			}
@@ -362,7 +362,7 @@ func TestCache(t *testing.T) {
 	})
 	t.Run("lookup-tenant", func(t *testing.T) {
 		for _, s := range []settings.Setting{i1A, fA, dA, duA} {
-			result, ok := settings.Lookup(s.Key(), settings.LookupForLocalAccess, false /* forSystemTenant */)
+			result, ok := settings.LookupForLocalAccess(s.Key(), false /* forSystemTenant */)
 			if !ok {
 				t.Fatalf("lookup(%s) failed", s.Key())
 			}
@@ -373,7 +373,7 @@ func TestCache(t *testing.T) {
 	})
 	t.Run("lookup-tenant-fail", func(t *testing.T) {
 		for _, s := range []settings.Setting{iVal, fVal, dVal, eA, mA} {
-			_, ok := settings.Lookup(s.Key(), settings.LookupForLocalAccess, false /* forSystemTenant */)
+			_, ok := settings.LookupForLocalAccess(s.Key(), false /* forSystemTenant */)
 			if ok {
 				t.Fatalf("lookup(%s) should have failed", s.Key())
 			}
@@ -688,13 +688,13 @@ func TestCache(t *testing.T) {
 }
 
 func TestIsReportable(t *testing.T) {
-	if v, ok := settings.Lookup(
-		"bool.t", settings.LookupForLocalAccess, settings.ForSystemTenant,
+	if v, ok := settings.LookupForLocalAccess(
+		"bool.t", settings.ForSystemTenant,
 	); !ok || !settings.TestingIsReportable(v) {
 		t.Errorf("expected 'bool.t' to be marked as isReportable() = true")
 	}
-	if v, ok := settings.Lookup(
-		"sekretz", settings.LookupForLocalAccess, settings.ForSystemTenant,
+	if v, ok := settings.LookupForLocalAccess(
+		"sekretz", settings.ForSystemTenant,
 	); !ok || settings.TestingIsReportable(v) {
 		t.Errorf("expected 'sekretz' to be marked as isReportable() = false")
 	}
@@ -712,7 +712,7 @@ func TestOnChangeWithMaxSettings(t *testing.T) {
 	sv := &settings.Values{}
 	sv.Init(ctx, settings.TestOpaque)
 	var changes int
-	s, ok := settings.Lookup(maxName, settings.LookupForLocalAccess, settings.ForSystemTenant)
+	s, ok := settings.LookupForLocalAccess(maxName, settings.ForSystemTenant)
 	if !ok {
 		t.Fatalf("expected lookup of %s to succeed", maxName)
 	}

pkg/sql/crdb_internal.go

@@ -2006,9 +2006,7 @@ CREATE TABLE crdb_internal.cluster_settings (
 					"crdb_internal.cluster_settings", privilege.MODIFYCLUSTERSETTING, privilege.VIEWCLUSTERSETTING)
 		}
 		for _, k := range settings.Keys(p.ExecCfg().Codec.ForSystemTenant()) {
-			setting, _ := settings.Lookup(
-				k, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant(),
-			)
+			setting, _ := settings.LookupForLocalAccess(k, p.ExecCfg().Codec.ForSystemTenant())
 			strVal := setting.String(&p.ExecCfg().Settings.SV)
 			isPublic := setting.Visibility() == settings.Public
 			desc := setting.Description()