Skip to content

Commit

Permalink
server: fix misreported CRDB environment variables during startup
Browse files Browse the repository at this point in the history
During server startup, reportConfiguration (see cli/start.go), logs the
values of CRDB and other, unredacted environment variables to the Ops channel.
These environment variables are read from envVarRegistry.cache (see
GetEnvVarsUsed) which is assumed to have been populated during the
initialization of the (Go) runtime. Specifically, EnvOrDefaultXXX must
be invoked, which has a side-effect of populating the cache.
When it's invoked outside of the initializer (e.g., inside a func
that's not invoked until after initilization), the corresponding
environment variable will _not_ be in envVarRegistry.cache. Hence, it
will not be reported.

Reporting values of modified environment variables is useful for quickly
assembling a configuration context during debugging. It's unfortunate
that the current design relies on indirectly populating
envVarRegistry.cache during the initialization _without_ any sort of
enforcement wrt where EnvOrDefaultXXX is invoked. The change in this PR
doesn't attempt to fix it. Instead, the following misreported environment
variables are now reported,

 - COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING
 - COCKROACH_UPGRADE_TO_DEV_VERSION

Release note: None

Epic: none
  • Loading branch information
srosenberg committed Dec 6, 2022
1 parent 06bb238 commit de64104
Show file tree
Hide file tree
Showing 7 changed files with 22 additions and 9 deletions.
2 changes: 1 addition & 1 deletion pkg/cli/demo.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ func runDemoInternal(

// Only print details about the telemetry configuration if the
// user has control over it.
if cluster.TelemetryOptOut() {
if cluster.TelemetryOptOut {
cliCtx.PrintlnUnlessEmbedded("#\n# Telemetry disabled by configuration.")
} else {
cliCtx.PrintlnUnlessEmbedded("#\n" +
Expand Down
4 changes: 3 additions & 1 deletion pkg/clusterversion/cockroach_versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,8 @@ const (
finalVersion = invalidVersionKey
)

var allowUpgradeToDev = envutil.EnvOrDefaultBool("COCKROACH_UPGRADE_TO_DEV_VERSION", false)

var versionsSingleton = func() keyedVersions {
if developmentBranch {
// If this is a dev branch, we offset every version +1M major versions into
Expand All @@ -639,7 +641,7 @@ var versionsSingleton = func() keyedVersions {
// renumber only 2-4 to be +1M. It would then step from 3 "up" to 1000002 --
// which conceptually is actually back down to 2 -- then back to to 1000003,
// then on to 1000004, etc.
skipFirst := envutil.EnvOrDefaultBool("COCKROACH_UPGRADE_TO_DEV_VERSION", false)
skipFirst := allowUpgradeToDev
const devOffset = 1000000
first := true
for i := range rawVersionsSingleton {
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1724,7 +1724,7 @@ func (s *Server) PreStart(ctx context.Context) error {
}

// If enabled, start reporting diagnostics.
if s.cfg.StartDiagnosticsReporting && !cluster.TelemetryOptOut() {
if s.cfg.StartDiagnosticsReporting && !cluster.TelemetryOptOut {
s.startDiagnostics(workersCtx)
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/server/tenant.go
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ func (s *SQLServerWrapper) PreStart(ctx context.Context) error {
}

// If enabled, start reporting diagnostics.
if s.sqlServer.cfg.StartDiagnosticsReporting && !cluster.TelemetryOptOut() {
if s.sqlServer.cfg.StartDiagnosticsReporting && !cluster.TelemetryOptOut {
s.startDiagnostics(workersCtx)
}

Expand Down
6 changes: 2 additions & 4 deletions pkg/settings/cluster/cluster_settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,8 @@ type OverridesInformer interface {
IsOverridden(settingName string) bool
}

// TelemetryOptOut is a place for controlling whether to opt out of telemetry or not.
func TelemetryOptOut() bool {
return envutil.EnvOrDefaultBool("COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING", false)
}
// TelemetryOptOut controls whether to opt out of telemetry (including Sentry) or not.
var TelemetryOptOut = envutil.EnvOrDefaultBool("COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING", false)

// NoSettings is used when a func requires a Settings but none is available
// (for example, a CLI subcommand that does not connect to a cluster).
Expand Down
2 changes: 1 addition & 1 deletion pkg/upgrade/upgrades/permanent_upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func optInToDiagnosticsStatReporting(
ctx context.Context, _ clusterversion.ClusterVersion, deps upgrade.TenantDeps,
) error {
// We're opting-out of the automatic opt-in. See discussion in updates.go.
if cluster.TelemetryOptOut() {
if cluster.TelemetryOptOut {
return nil
}
_, err := deps.InternalExecutor.Exec(
Expand Down
13 changes: 13 additions & 0 deletions pkg/util/envutil/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,19 @@ func EnvOrDefaultString(name string, value string) string {

// EnvOrDefaultBool returns the value set by the specified environment
// variable, if any, otherwise the specified default value.
//
// N.B. EnvOrDefaultBool has the desired side-effect of populating envVarRegistry.cache.
// It has to be invoked during (var) init; otherwise, cli/start.go:reportConfiguration will not report the
// value of this environment variable in the server log, upon startup.
//
// Correct Usage: var allowUpgradeToDev = envutil.EnvOrDefaultBool("COCKROACH_UPGRADE_TO_DEV_VERSION", false)
//
// Incorrect Usage: func() {
// ...
// var allowUpgradeToDev envutil.EnvOrDefaultBool("COCKROACH_UPGRADE_TO_DEV_VERSION", false)
// }
//
// N.B. The same rule applies to the remaining EnvOrDefaultXXX defined here.
func EnvOrDefaultBool(name string, value bool) bool {
if str, present := getEnv(name, 1); present {
v, err := strconv.ParseBool(str)
Expand Down

0 comments on commit de64104

Please sign in to comment.