Merge pull request #85173 from logston/backport21.1-82147

release-21.1: logging: Expand env vars in logging config

pkg/cli/cli_test.go

@@ -1525,26 +1525,30 @@ Available Commands:
   help              Help about any command
 
 Flags:
-  -h, --help                     help for cockroach
-      --log <string>             
-                                  Logging configuration, expressed using YAML syntax. For example, you can
-                                  change the default logging directory with: --log='file-defaults: {dir: ...}'.
-                                  See the documentation for more options and details.  To preview how the log
-                                  configuration is applied, or preview the default configuration, you can use
-                                  the 'cockroach debug check-log-config' sub-command.
-                                 
-      --log-config-file <file>   
-                                  File name to read the logging configuration from. This has the same effect as
-                                  passing the content of the file via the --log flag.
-                                  (default <unset>)
-      --version                  version for cockroach
+  -h, --help                      help for cockroach
+      --log <string>              
+                                   Logging configuration, expressed using YAML syntax. For example, you can
+                                   change the default logging directory with: --log='file-defaults: {dir: ...}'.
+                                   See the documentation for more options and details.  To preview how the log
+                                   configuration is applied, or preview the default configuration, you can use
+                                   the 'cockroach debug check-log-config' sub-command.
+                                  
+      --log-config-file <file>    
+                                   File name to read the logging configuration from. This has the same effect as
+                                   passing the content of the file via the --log flag.
+                                   (default <unset>)
+      --log-config-vars strings   
+                                   Environment variables that will be expanded if present in the body of the
+                                   logging configuration.
+                                  
+      --version                   version for cockroach
 
 Use "cockroach [command] --help" for more information about a command.
 `
 	helpExpected := fmt.Sprintf("CockroachDB command-line interface and server.\n\n%s",
 		// Due to a bug in spf13/cobra, 'cockroach help' does not include the --version
 		// flag. Strangely, 'cockroach --help' does, as well as usage error messages.
-		strings.ReplaceAll(expUsage, "      --version                  version for cockroach\n", ""))
+		strings.ReplaceAll(expUsage, "      --version                   version for cockroach\n", ""))
 	badFlagExpected := fmt.Sprintf("%s\nError: unknown flag: --foo\n", expUsage)
 
 	testCases := []struct {

pkg/cli/cliflags/flags.go

@@ -1480,6 +1480,12 @@ This has the same effect as passing the content of the file via
 the --log flag.`,
 	}
 
+	LogConfigVars = FlagInfo{
+		Name: "log-config-vars",
+		Description: `Environment variables that will be expanded if
+present in the body of the logging configuration.`,
+	}
+
 	DeprecatedStderrThreshold = FlagInfo{
 		Name:        "logtostderr",
 		Description: `Write log messages beyond the specified severity to stderr.`,

pkg/cli/context.go

@@ -176,6 +176,9 @@ type cliContext struct {
 
 	// logConfigInput is the YAML input for the logging configuration.
 	logConfigInput settableString
+	// logConfigVars is an array of environment variables used in the logging
+	// configuration that will be expanded by CRDB.
+	logConfigVars []string
 	// logConfig is the resulting logging configuration after the input
 	// configuration has been parsed and validated.
 	logConfig logconfig.Config
@@ -226,6 +229,7 @@ func setCliContextDefaults() {
 	cliCtx.extraConnURLOptions = nil
 	cliCtx.allowUnencryptedClientPassword = false
 	cliCtx.logConfigInput = settableString{s: ""}
+	cliCtx.logConfigVars = nil
 	cliCtx.logConfig = logconfig.Config{}
 	cliCtx.ambiguousLogDir = false
 	// TODO(knz): Deprecated in v21.1. Remove this.

pkg/cli/flags.go

@@ -290,6 +290,7 @@ func init() {
 		// Logging configuration.
 		varFlag(pf, &stringValue{settableString: &cliCtx.logConfigInput}, cliflags.Log)
 		varFlag(pf, &fileContentsValue{settableString: &cliCtx.logConfigInput, fileName: "<unset>"}, cliflags.LogConfigFile)
+		stringSliceFlag(pf, &cliCtx.logConfigVars, cliflags.LogConfigVars)
 
 		// Pre-v21.1 overrides. Deprecated.
 		// TODO(knz): Remove this.

pkg/cli/log_flags.go

@@ -17,9 +17,11 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
+	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/channel"
@@ -46,6 +48,10 @@ func setupLogging(ctx context.Context, cmd *cobra.Command, isServerCmd, applyCon
 		return errors.Newf("--%s is incompatible with legacy discrete logging flags", cliflags.Log.Name)
 	}
 
+	if err := validateLogConfigVars(cliCtx.logConfigVars); err != nil {
+		return errors.Wrap(err, "invalid logging configuration")
+	}
+
 	// Sanity check to prevent misuse of API.
 	if active, firstUse := log.IsActive(); active {
 		panic(errors.Newf("logging already active; first used at:\n%s", firstUse))
@@ -119,7 +125,17 @@ func setupLogging(ctx context.Context, cmd *cobra.Command, isServerCmd, applyCon
 
 	// If a configuration was specified via --log, load it.
 	if cliCtx.logConfigInput.isSet {
-		if err := h.Set(cliCtx.logConfigInput.s); err != nil {
+		s := cliCtx.logConfigInput.s
+
+		if len(cliCtx.logConfigVars) > 0 {
+			var err error
+			s, err = expandEnvironmentVariables(s, cliCtx.logConfigVars)
+			if err != nil {
+				return errors.Wrap(err, "unable to expand environment variables")
+			}
+		}
+
+		if err := h.Set(s); err != nil {
 			return err
 		}
 		if h.Config.FileDefaults.Dir != nil {
@@ -442,8 +458,6 @@ func addPredefinedLogFiles(c *logconfig.Config) {
 	}
 }
 
-// predefinedLogFiles are the files defined when the --log flag
-// does not otherwise override the file sinks.
 var predefinedLogFiles = map[logpb.Channel]string{
 	channel.STORAGE:           "pebble",
 	channel.SESSIONS:          "sql-auth",
@@ -463,3 +477,45 @@ var predefinedAuditFiles = map[logpb.Channel]bool{
 	//  channel.PRIVILEGES:       true,
 	channel.SENSITIVE_ACCESS: true,
 }
+
+// validateLogConfigVars return an error if any of the passed logging
+// configuration variables are are not permissible. For security, variables
+// that start with COCKROACH_ are explicitly disallowed. See #81146 for more.
+func validateLogConfigVars(vars []string) error {
+	for _, v := range vars {
+		if strings.HasPrefix(strings.ToUpper(v), "COCKROACH_") {
+			return errors.Newf("use of %s is not allowed as a logging configuration variable", v)
+		}
+	}
+	return nil
+}
+
+// expandEnvironmentVariables replaces variables used in string with their
+// values pulled from the environment. If there are variables in the string
+// that are not contained in vars, they will be replaced with the empty string.
+func expandEnvironmentVariables(s string, vars []string) (string, error) {
+	var err error
+
+	m := map[string]string{}
+	// Only pull variable values from the environment if their key is present
+	// in vars to create an allow list.
+	for _, k := range vars {
+		v, ok := envutil.ExternalEnvString(k, 1)
+		if !ok {
+			err = errors.CombineErrors(err, errors.Newf("variable %q is not defined in environment", k))
+			continue
+		}
+		m[k] = v
+	}
+
+	s = os.Expand(s, func(k string) string {
+		v, ok := m[k]
+		if !ok {
+			err = errors.CombineErrors(err, errors.Newf("unknown variable %q used in configuration", k))
+			return ""
+		}
+		return v
+	})
+
+	return s, err
+}

pkg/cli/log_flags_test.go

@@ -15,6 +15,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"strings"
 	"testing"
@@ -23,7 +24,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logconfig"
 	"github.com/cockroachdb/datadriven"
+	"github.com/cockroachdb/errors"
 	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // TestSetupLogging checks the behavior of logging flags.
@@ -64,6 +68,11 @@ func TestSetupLogging(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	require.NoError(t, os.Setenv("HOST_IP", "1.2.3.4"))
+	defer func() {
+		require.NoError(t, os.Unsetenv("HOST_IP"))
+	}()
+
 	ctx := context.Background()
 
 	datadriven.RunTest(t, "testdata/logflags", func(t *testing.T, td *datadriven.TestData) string {
@@ -132,8 +141,89 @@ func isServerCmd(thisCmd *cobra.Command) bool {
 	return false
 }
 
+func TestValidateLogConfigVars(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	for i, tc := range []struct {
+		vars        []string
+		expectedErr error
+	}{
+		{
+			vars: []string{"HOST_IP"},
+		},
+		{
+			vars:        []string{"COCKROACH_TEST"},
+			expectedErr: errors.Newf(`use of COCKROACH_TEST is not allowed as a logging configuration variable`),
+		},
+	} {
+		err := validateLogConfigVars(tc.vars)
+
+		if !errors.Is(tc.expectedErr, err) {
+			t.Errorf("%d. validateLogConfigVars err expected '%s', but got '%s'.",
+				i, tc.expectedErr, err)
+		}
+	}
+}
+
+func TestExpandEnvironmentVariables(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	require.NoError(t, os.Setenv("HOST_IP1", "1.2.3.4"))
+	require.NoError(t, os.Setenv("HOST_IP2", "5.6.7.8"))
+	defer func() {
+		require.NoError(t, os.Unsetenv("HOST_IP1"))
+		require.NoError(t, os.Unsetenv("HOST_IP2"))
+	}()
+	require.NoError(t, os.Unsetenv("EXPAND_ABSENT_VAR1"))
+	require.NoError(t, os.Unsetenv("EXPAND_ABSENT_VAR2"))
+
+	for _, tc := range []struct {
+		in             string
+		vars           []string
+		expectedOut    string
+		expectedErrMsg string
+	}{
+		{
+			in:          "$HOST_IP1",
+			vars:        []string{"HOST_IP1"},
+			expectedOut: "1.2.3.4",
+		},
+		{
+			in:          "${HOST_IP2}",
+			vars:        []string{"HOST_IP2"},
+			expectedOut: "5.6.7.8",
+		},
+		{
+			in:             "$EXPAND_ABSENT_VAR1",
+			vars:           []string{"EXPAND_ABSENT_VAR1"},
+			expectedErrMsg: `variable "EXPAND_ABSENT_VAR1" is not defined in environment`,
+		},
+		{
+			in:             "${EXPAND_ABSENT_VAR2}",
+			vars:           []string{"EXPAND_ABSENT_VAR2"},
+			expectedErrMsg: `variable "EXPAND_ABSENT_VAR2" is not defined in environment`,
+		},
+		{
+			in:             "$HOST_IP3",
+			expectedErrMsg: `unknown variable "HOST_IP3" used in configuration`,
+		},
+		{
+			in:             "${HOST_IP4}",
+			expectedErrMsg: `unknown variable "HOST_IP4" used in configuration`,
+		},
+	} {
+		out, err := expandEnvironmentVariables(tc.in, tc.vars)
+
+		if tc.expectedErrMsg != "" {
+			assert.EqualError(t, err, tc.expectedErrMsg)
+		} else {
+			assert.Nil(t, err)
+		}
+		assert.Equal(t, tc.expectedOut, out)
+	}
+}
+
 // TestLogFlagCombinations checks that --log and --log-config-file properly
-// override each other.
+// override each other and that --log-config-vars stores the appropriate values
+// in the cliContext struct.
 func TestLogFlagCombinations(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -163,15 +253,44 @@ func TestLogFlagCombinations(t *testing.T) {
 
 	f := startCmd.Flags()
 	testData := []struct {
-		args           []string
-		expectedLogCfg string
+		args            []string
+		expectedLogCfg  string
+		expectedLogVars []string
 	}{
-		{[]string{"start"}, ""},
-		{[]string{"start", "--log=foo"}, "foo"},
-		{[]string{"start", "--log-config-file=" + tmpfile.Name()}, filecontents},
-		{[]string{"start", "--log=foo", "--log=bar"}, "bar"},
-		{[]string{"start", "--log=foo", "--log-config-file=" + tmpfile.Name()}, filecontents},
-		{[]string{"start", "--log-config-file=" + tmpfile.Name(), "--log=bar"}, "bar"},
+		{
+			args:           []string{"start"},
+			expectedLogCfg: "",
+		},
+		{
+			args:           []string{"start", "--log=foo"},
+			expectedLogCfg: "foo",
+		},
+		{
+			args:           []string{"start", "--log-config-file=" + tmpfile.Name()},
+			expectedLogCfg: filecontents,
+		},
+		{
+			args:           []string{"start", "--log=foo", "--log=bar"},
+			expectedLogCfg: "bar",
+		},
+		{
+			args:           []string{"start", "--log=foo", "--log-config-file=" + tmpfile.Name()},
+			expectedLogCfg: filecontents,
+		},
+		{
+			args:           []string{"start", "--log-config-file=" + tmpfile.Name(), "--log=bar"},
+			expectedLogCfg: "bar",
+		},
+		{
+			args:            []string{"start", "--log-config-file=" + tmpfile.Name(), "--log-config-vars=HOST_IP"},
+			expectedLogCfg:  filecontents,
+			expectedLogVars: []string{"HOST_IP"},
+		},
+		{
+			args:            []string{"start", "--log-config-file=" + tmpfile.Name(), "--log-config-vars=HOST_IP,POD_NAME"},
+			expectedLogCfg:  filecontents,
+			expectedLogVars: []string{"HOST_IP", "POD_NAME"},
+		},
 	}
 
 	for i, td := range testData {
@@ -184,5 +303,10 @@ func TestLogFlagCombinations(t *testing.T) {
 			t.Errorf("%d. cliCtx.logConfigInput.s expected '%s', but got '%s'. td.args was '%#v'.",
 				i, td.expectedLogCfg, cliCtx.logConfigInput.s, td.args)
 		}
+
+		if !reflect.DeepEqual(td.expectedLogVars, cliCtx.logConfigVars) {
+			t.Errorf("%d. cliCtx.logConfigVars expected '%s', but got '%s'. td.args was '%#v'.",
+				i, td.expectedLogCfg, cliCtx.logConfigVars, td.args)
+		}
 	}
 }