From b043e9568cf9672cea30db947dc2ee153b5064c4 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 19 Jul 2022 21:02:46 -0400 Subject: [PATCH 1/4] 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 { From 098cbd701df2440f0d9a7fe108aa1fc263bc6204 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 20 Jul 2022 15:53:22 -0400 Subject: [PATCH 2/4] Responded to Doug's comments --- gcp/observability/config.go | 13 ++-- gcp/observability/observability_test.go | 90 ++++++++++--------------- 2 files changed, 43 insertions(+), 60 deletions(-) diff --git a/gcp/observability/config.go b/gcp/observability/config.go index f11e62bae22..4b4bc7645d4 100644 --- a/gcp/observability/config.go +++ b/gcp/observability/config.go @@ -74,9 +74,10 @@ 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) { +// unmarshalAndVerifyConfig unmarshals a json string representing an +// observability config into it's protobuf format, and also verifies the +// configuration's fields for validity. +func unmarshalAndVerifyConfig(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) @@ -95,11 +96,11 @@ func parseObservabilityConfig() (*configpb.ObservabilityConfig, error) { 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) + return nil, fmt.Errorf("error reading observability configuration file %q: %v", fileSystemPath, err) } - return unmarshalConfig(content) + return unmarshalAndVerifyConfig(content) } else if content := os.Getenv(envObservabilityConfig); content != "" { - return unmarshalConfig([]byte(content)) + return unmarshalAndVerifyConfig([]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 820f7e07a3c..e590d2d202d 100644 --- a/gcp/observability/observability_test.go +++ b/gcp/observability/observability_test.go @@ -21,7 +21,9 @@ package observability import ( "bytes" "context" + "encoding/json" "fmt" + "io/ioutil" "net" "os" "sync" @@ -683,39 +685,39 @@ 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") +// createTmpConfigInFileSystem creates a random observability config at a random +// place in the temporary portion of the file system dependent on system. It +// also sets the environment variable GRPC_CONFIG_OBSERVABILITY_JSON to point to +// this created config. +func createTmpConfigInFileSystem(rawJSON json.RawMessage) (*os.File, error) { + configJSONFile, err := ioutil.TempFile(os.TempDir(), "configJSON-") if err != nil { - t.Fatalf("cannot create file /tmp/configJSON: %v", err) + return nil, fmt.Errorf("cannot create file %v: %v", configJSONFile.Name(), err) } defer configJSONFile.Close() - _, err = configJSONFile.Write(configJSON) + _, err = configJSONFile.Write(rawJSON) if err != nil { - t.Fatalf("cannot write marshalled JSON: %v", err) + return nil, fmt.Errorf("cannot write marshalled JSON: %v", err) } - os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON") + os.Setenv(envObservabilityConfigJSON, configJSONFile.Name()) + return configJSONFile, nil /// do you even need to return this file if you're already closing it here? +} - if err := Start(context.Background()); err != nil { +// 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) { + configJSON := json.RawMessage(`{ + "destinationProjectId": "fake", + "logFilters":[{"pattern":"*","headerBytes":1073741824,"messageBytes":1073741824}] + }`) + _, err := createTmpConfigInFileSystem(configJSON) + if err != nil { + t.Fatalf("failed to create config in file system: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err != nil { t.Fatalf("error starting observability with valid config through file system: %v", err) } defer End() @@ -723,38 +725,18 @@ func (s) TestJSONEnvVarSet(t *testing.T) { // 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 +// take precedence, and an 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") + configJSON := json.RawMessage(`{ + "destinationProjectId":"fake", + "logFilters":[{"pattern":":-)"}, {"pattern":"*"}] + }`) + _, err := createTmpConfigInFileSystem(configJSON) if err != nil { - t.Fatalf("cannot create file /tmp/InvalidConfigJSON: %v", err) + t.Fatalf("failed to create config in file system: %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, From b7e6f7c1e6e38b5d04a67eebf2c3f3995ff46222 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 20 Jul 2022 16:16:57 -0400 Subject: [PATCH 3/4] Switch os.ReadFile to ioutil.ReadFile --- gcp/observability/config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gcp/observability/config.go b/gcp/observability/config.go index 4b4bc7645d4..b4aedd5ef9a 100644 --- a/gcp/observability/config.go +++ b/gcp/observability/config.go @@ -22,6 +22,7 @@ import ( "context" "encoding/json" "fmt" + "io/ioutil" "os" "regexp" @@ -94,7 +95,7 @@ func unmarshalAndVerifyConfig(rawJSON json.RawMessage) (*configpb.ObservabilityC func parseObservabilityConfig() (*configpb.ObservabilityConfig, error) { if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { - content, err := os.ReadFile(fileSystemPath) + content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 if err != nil { return nil, fmt.Errorf("error reading observability configuration file %q: %v", fileSystemPath, err) } From 84c612ccf66920e46159121bf2976654713a4b36 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 20 Jul 2022 17:02:40 -0400 Subject: [PATCH 4/4] Responded to Doug's comments --- gcp/observability/config.go | 2 +- gcp/observability/observability_test.go | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/gcp/observability/config.go b/gcp/observability/config.go index b4aedd5ef9a..ecda5b23007 100644 --- a/gcp/observability/config.go +++ b/gcp/observability/config.go @@ -76,7 +76,7 @@ func validateFilters(config *configpb.ObservabilityConfig) error { } // unmarshalAndVerifyConfig unmarshals a json string representing an -// observability config into it's protobuf format, and also verifies the +// observability config into its protobuf format, and also verifies the // configuration's fields for validity. func unmarshalAndVerifyConfig(rawJSON json.RawMessage) (*configpb.ObservabilityConfig, error) { var config configpb.ObservabilityConfig diff --git a/gcp/observability/observability_test.go b/gcp/observability/observability_test.go index e590d2d202d..c5fa59c6648 100644 --- a/gcp/observability/observability_test.go +++ b/gcp/observability/observability_test.go @@ -689,29 +689,29 @@ func (s) TestRefuseStartWithInvalidPatterns(t *testing.T) { // place in the temporary portion of the file system dependent on system. It // also sets the environment variable GRPC_CONFIG_OBSERVABILITY_JSON to point to // this created config. -func createTmpConfigInFileSystem(rawJSON json.RawMessage) (*os.File, error) { +func createTmpConfigInFileSystem(rawJSON string) (*os.File, error) { configJSONFile, err := ioutil.TempFile(os.TempDir(), "configJSON-") if err != nil { return nil, fmt.Errorf("cannot create file %v: %v", configJSONFile.Name(), err) } - defer configJSONFile.Close() - _, err = configJSONFile.Write(rawJSON) + _, err = configJSONFile.Write(json.RawMessage(rawJSON)) if err != nil { return nil, fmt.Errorf("cannot write marshalled JSON: %v", err) } os.Setenv(envObservabilityConfigJSON, configJSONFile.Name()) - return configJSONFile, nil /// do you even need to return this file if you're already closing it here? + return configJSONFile, nil } // 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) { - configJSON := json.RawMessage(`{ + configJSON := `{ "destinationProjectId": "fake", "logFilters":[{"pattern":"*","headerBytes":1073741824,"messageBytes":1073741824}] - }`) - _, err := createTmpConfigInFileSystem(configJSON) + }` + configJSONFile, err := createTmpConfigInFileSystem(configJSON) + defer configJSONFile.Close() if err != nil { t.Fatalf("failed to create config in file system: %v", err) } @@ -729,11 +729,12 @@ func (s) TestJSONEnvVarSet(t *testing.T) { // configuration being invalid, even if the direct configuration environment // variable is set and valid. func (s) TestBothConfigEnvVarsSet(t *testing.T) { - configJSON := json.RawMessage(`{ + configJSON := `{ "destinationProjectId":"fake", "logFilters":[{"pattern":":-)"}, {"pattern":"*"}] - }`) - _, err := createTmpConfigInFileSystem(configJSON) + }` + configJSONFile, err := createTmpConfigInFileSystem(configJSON) + defer configJSONFile.Close() if err != nil { t.Fatalf("failed to create config in file system: %v", err) }