From b043e9568cf9672cea30db947dc2ee153b5064c4 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 19 Jul 2022 21:02:46 -0400 Subject: [PATCH] Added support for Enviornment Variable path specifier --- gcp/observability/config.go | 45 ++++++---- gcp/observability/observability_test.go | 107 ++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 16 deletions(-) diff --git a/gcp/observability/config.go b/gcp/observability/config.go index 428d527a30c..f11e62bae22 100644 --- a/gcp/observability/config.go +++ b/gcp/observability/config.go @@ -20,6 +20,7 @@ package observability import ( "context" + "encoding/json" "fmt" "os" "regexp" @@ -31,9 +32,10 @@ import ( ) const ( - envObservabilityConfig = "GRPC_CONFIG_OBSERVABILITY" - envProjectID = "GOOGLE_CLOUD_PROJECT" - logFilterPatternRegexpStr = `^([\w./]+)/((?:\w+)|[*])$` + envObservabilityConfig = "GRPC_CONFIG_OBSERVABILITY" + envObservabilityConfigJSON = "GRPC_CONFIG_OBSERVABILITY_JSON" + envProjectID = "GOOGLE_CLOUD_PROJECT" + logFilterPatternRegexpStr = `^([\w./]+)/((?:\w+)|[*])$` ) var logFilterPatternRegexp = regexp.MustCompile(logFilterPatternRegexpStr) @@ -72,21 +74,32 @@ func validateFilters(config *configpb.ObservabilityConfig) error { return nil } +// unmarshalConfig unmarshals a json string representing an observability config +// into it's protobuf format. +func unmarshalConfig(rawJSON json.RawMessage) (*configpb.ObservabilityConfig, error) { + var config configpb.ObservabilityConfig + if err := protojson.Unmarshal(rawJSON, &config); err != nil { + return nil, fmt.Errorf("error parsing observability config: %v", err) + } + if err := validateFilters(&config); err != nil { + return nil, fmt.Errorf("error parsing observability config: %v", err) + } + if config.GlobalTraceSamplingRate > 1 || config.GlobalTraceSamplingRate < 0 { + return nil, fmt.Errorf("error parsing observability config: invalid global trace sampling rate %v", config.GlobalTraceSamplingRate) + } + logger.Infof("Parsed ObservabilityConfig: %+v", &config) + return &config, nil +} + func parseObservabilityConfig() (*configpb.ObservabilityConfig, error) { - // Parse the config from ENV var - if content := os.Getenv(envObservabilityConfig); content != "" { - var config configpb.ObservabilityConfig - if err := protojson.Unmarshal([]byte(content), &config); err != nil { - return nil, fmt.Errorf("error parsing observability config from env %v: %v", envObservabilityConfig, err) - } - if err := validateFilters(&config); err != nil { - return nil, fmt.Errorf("error parsing observability config: %v", err) - } - if config.GlobalTraceSamplingRate > 1 || config.GlobalTraceSamplingRate < 0 { - return nil, fmt.Errorf("error parsing observability config: invalid global trace sampling rate %v", config.GlobalTraceSamplingRate) + if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { + content, err := os.ReadFile(fileSystemPath) + if err != nil { + return nil, fmt.Errorf("error reading file from env var %v: %v", envObservabilityConfigJSON, err) } - logger.Infof("Parsed ObservabilityConfig: %+v", &config) - return &config, nil + return unmarshalConfig(content) + } else if content := os.Getenv(envObservabilityConfig); content != "" { + return unmarshalConfig([]byte(content)) } // If the ENV var doesn't exist, do nothing return nil, nil diff --git a/gcp/observability/observability_test.go b/gcp/observability/observability_test.go index 62936ccec10..820f7e07a3c 100644 --- a/gcp/observability/observability_test.go +++ b/gcp/observability/observability_test.go @@ -675,6 +675,7 @@ func (s) TestRefuseStartWithInvalidPatterns(t *testing.T) { if err != nil { t.Fatalf("failed to convert config to JSON: %v", err) } + os.Setenv(envObservabilityConfigJSON, "") os.Setenv(envObservabilityConfig, string(configJSON)) // If there is at least one invalid pattern, which should not be silently tolerated. if err := Start(context.Background()); err == nil { @@ -682,7 +683,113 @@ func (s) TestRefuseStartWithInvalidPatterns(t *testing.T) { } } +// TestJSONEnvVarSet tests a valid observability configuration specified by the +// GRPC_CONFIG_OBSERVABILITY_JSON environment variable, whose value represents a +// file path pointing to a JSON encoded config. +func (s) TestJSONEnvVarSet(t *testing.T) { + config := &configpb.ObservabilityConfig{ + EnableCloudLogging: true, + DestinationProjectId: "fake", + LogFilters: []*configpb.ObservabilityConfig_LogFilter{ + { + Pattern: "*", + HeaderBytes: infinitySizeBytes, + MessageBytes: infinitySizeBytes, + }, + }, + } + + configJSON, err := protojson.Marshal(config) + if err != nil { + t.Fatalf("failed to convert config to JSON: %v", err) + } + + configJSONFile, err := os.Create("/tmp/configJSON") + if err != nil { + t.Fatalf("cannot create file /tmp/configJSON: %v", err) + } + defer configJSONFile.Close() + _, err = configJSONFile.Write(configJSON) + if err != nil { + t.Fatalf("cannot write marshalled JSON: %v", err) + } + os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON") + + if err := Start(context.Background()); err != nil { + t.Fatalf("error starting observability with valid config through file system: %v", err) + } + defer End() +} + +// TestBothConfigEnvVarsSet tests the scenario where both configuration +// environment variables are set. The file system environment variable should +// take precedence, and and error should return in the case of the file system +// configuration being invalid, even if the direct configuration environment +// variable is set and valid. +func (s) TestBothConfigEnvVarsSet(t *testing.T) { + invalidConfig := &configpb.ObservabilityConfig{ + EnableCloudLogging: true, + DestinationProjectId: "fake", + LogFilters: []*configpb.ObservabilityConfig_LogFilter{ + { + Pattern: ":-)", + }, + { + Pattern: "*", + }, + }, + } + invalidConfigJSON, err := protojson.Marshal(invalidConfig) + if err != nil { + t.Fatalf("failed to convert config to JSON: %v", err) + } + + invalidConfigJSONFile, err := os.Create("/tmp/InvalidConfigJSON") + if err != nil { + t.Fatalf("cannot create file /tmp/InvalidConfigJSON: %v", err) + } + defer invalidConfigJSONFile.Close() + _, err = invalidConfigJSONFile.Write(invalidConfigJSON) + if err != nil { + t.Fatalf("cannot write marshalled JSON: %v", err) + } + os.Setenv(envObservabilityConfigJSON, "/tmp/InvalidConfigJSON") + + // This configuration should be ignored, as precedence 2. + validConfig := &configpb.ObservabilityConfig{ + EnableCloudLogging: true, + DestinationProjectId: "fake", + LogFilters: []*configpb.ObservabilityConfig_LogFilter{ + { + Pattern: "*", + HeaderBytes: infinitySizeBytes, + MessageBytes: infinitySizeBytes, + }, + }, + } + validConfigJSON, err := protojson.Marshal(validConfig) + if err != nil { + t.Fatalf("failed to convert config to JSON: %v", err) + } + os.Setenv(envObservabilityConfig, string(validConfigJSON)) + if err := Start(context.Background()); err == nil { + t.Fatalf("Invalid patterns not triggering error") + } +} + +// TestErrInFileSystemEnvVar tests the scenario where an observability +// configuration is specified with environment variable that specifies a +// location in the file system for configuration, and this location doesn't have +// a file (or valid configuration). +func (s) TestErrInFileSystemEnvVar(t *testing.T) { + os.Setenv(envObservabilityConfigJSON, "/this-file/does-not-exist") + if err := Start(context.Background()); err == nil { + t.Fatalf("Invalid file system path not triggering error") + } +} + func (s) TestNoEnvSet(t *testing.T) { + os.Setenv(envObservabilityConfigJSON, "") os.Setenv(envObservabilityConfig, "") // If there is no observability config set at all, the Start should return an error. if err := Start(context.Background()); err == nil {