From faa10a30f90ed15e5f0791e444936b265ead3efc Mon Sep 17 00:00:00 2001 From: Paul Logston Date: Tue, 31 May 2022 22:45:33 +0800 Subject: [PATCH] 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 | 32 +++++---- pkg/cli/cliflags/flags.go | 6 ++ pkg/cli/context.go | 4 ++ pkg/cli/flags.go | 1 + pkg/cli/log_flags.go | 62 ++++++++++++++++- pkg/cli/log_flags_test.go | 142 +++++++++++++++++++++++++++++++++++--- pkg/cli/testdata/logflags | 29 ++++++++ 7 files changed, 250 insertions(+), 26 deletions(-) diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index 3eba4a43415b..5ffe577ec5b7 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -1525,26 +1525,30 @@ Available Commands: help Help about any command Flags: - -h, --help help for cockroach - --log - 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 name to read the logging configuration from. This has the same effect as - passing the content of the file via the --log flag. - (default ) - --version version for cockroach + -h, --help help for cockroach + --log + 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 name to read the logging configuration from. This has the same effect as + passing the content of the file via the --log flag. + (default ) + --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 { diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go index 15a78fb49c79..c6681dabc89c 100644 --- a/pkg/cli/cliflags/flags.go +++ b/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.`, diff --git a/pkg/cli/context.go b/pkg/cli/context.go index 3eb73826d8d6..16a52aa71644 100644 --- a/pkg/cli/context.go +++ b/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. diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index 0a5227bf2ded..cc4cf6732055 100644 --- a/pkg/cli/flags.go +++ b/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: ""}, cliflags.LogConfigFile) + stringSliceFlag(pf, &cliCtx.logConfigVars, cliflags.LogConfigVars) // Pre-v21.1 overrides. Deprecated. // TODO(knz): Remove this. diff --git a/pkg/cli/log_flags.go b/pkg/cli/log_flags.go index daefbad548e3..cf2d68945952 100644 --- a/pkg/cli/log_flags.go +++ b/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 +} diff --git a/pkg/cli/log_flags_test.go b/pkg/cli/log_flags_test.go index f22e9594e1a6..0d5b4d6afb93 100644 --- a/pkg/cli/log_flags_test.go +++ b/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) + } } } diff --git a/pkg/cli/testdata/logflags b/pkg/cli/testdata/logflags index d84ee4b68025..34be9e8cb62d 100644 --- a/pkg/cli/testdata/logflags +++ b/pkg/cli/testdata/logflags @@ -229,6 +229,35 @@ sql-slow-internal-only: ,true,crdb-v }, )>} +# Ensure variable expansion works. +run +start +--log=sinks: {fluent-servers: {health: {channels: [DEV], address: ${HOST_IP}:5170}}} +--log-config-vars=HOST_IP +---- +config: {)>, +, +sinks: {file-groups: {default: ,true,crdb-v2)>, +pebble: ,true,crdb-v2)>, +sql-audit: ,false,crdb-v2)>, +sql-auth: ,false,crdb-v2)>, +sql-exec: ,true,crdb-v2)>, +sql-slow: ,true,crdb-v2)>, +sql-slow-internal-only: ,true,crdb-v2)>}, +fluent-servers: {health: {channels: [ DEV], +net: tcp, +address: '1.2.3.4:5170', +filter: INFO, +format: json-fluent-compact, +redactable: true, +exit-on-error: false}}, +}, +)>} # It's possible to disable the stderr capture. run