logging: Expand env vars in logging config

  This commit adds a command line flag `--log-config-vars` that is used
  to specify those environment variables that will be expanded in the
  logging config file contents. This flag enables operators to
  indirectly define an allow list of sorts of trusted environment
  variables.

  A desired use case of this flag is one where `HOST_IP` is pulled from
  the running container environment and used to direct logs to a node
  local instance of FluentBit. In this snippet below, the log.yaml
  configuration file can be defined with a generic `fluent-server` sink
  that specifies `${HOST_IP}:5170` as its address. CRDB will then pull
  the IP from the environment and replace the variable with the IP in
  the logging configuration.

  ```
  containers:
    - name: cockroach
      image: cockroachdb/cockroach
      args:
      - --log-config-file log.yaml
      - --log-config-vars HOST_IP
      env:
        - name: HOST_IP
          valueFrom:
            fieldRef:
              fieldPath: status.hostIP
  ```

  Closes #81146.

Release note (cli change): The CLI now contains a flag
(--log-config-vars) that allows for environment variables to be
specified for expansion within the logging configuration file. This
change allows for a single logging configuration file to service an
array of sinks without further manipulation of the configuration file.

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)
+		}
 	}
 }