From a95df388a29117fca7fde0aaba88328d05d4f8a3 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Thu, 14 Jan 2021 10:56:15 -0800 Subject: [PATCH 1/6] [bootstrap_env_content] xds bootstrap: support config content in env variable --- xds/internal/client/bootstrap/bootstrap.go | 34 ++++++++++++++++------ xds/internal/env/env.go | 5 ++++ 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/xds/internal/client/bootstrap/bootstrap.go b/xds/internal/client/bootstrap/bootstrap.go index e833a4c91f4f..50624ef6fce0 100644 --- a/xds/internal/client/bootstrap/bootstrap.go +++ b/xds/internal/client/bootstrap/bootstrap.go @@ -96,6 +96,28 @@ type xdsServer struct { ServerFeatures []string `json:"server_features"` } +func bootstrapConfigFromEnvVariable() ([]byte, error) { + fName := env.BootstrapFileName + fContent := env.BootstrapFileContent + + // Bootstrap file name has higher priority than bootstrap content. + if fName != "" { + // If file name is set + // - If file not found (or other errors), fail + // - Otherwise, use the content. + // + // Note that even if the content is invalid, we don't failover to the + // file content env variable. + return bootstrapFileReadFunc(fName) + } + + if fContent != "" { + return []byte(fContent), nil + } + + return nil, fmt.Errorf("xds: bootstrap environment variable (%q, or %q) not defined", "GRPC_XDS_BOOTSTRAP", "GRPC_XDS_BOOTSTRAP_CONFIG") +} + // NewConfig returns a new instance of Config initialized by reading the // bootstrap file found at ${GRPC_XDS_BOOTSTRAP}. // @@ -136,21 +158,15 @@ type xdsServer struct { func NewConfig() (*Config, error) { config := &Config{} - fName := env.BootstrapFileName - if fName == "" { - return nil, fmt.Errorf("xds: Environment variable %q not defined", "GRPC_XDS_BOOTSTRAP") - } - logger.Infof("Got bootstrap file location %q", fName) - - data, err := bootstrapFileReadFunc(fName) + data, err := bootstrapConfigFromEnvVariable() if err != nil { - return nil, fmt.Errorf("xds: Failed to read bootstrap file %s with error %v", fName, err) + return nil, fmt.Errorf("xds: Failed to get bootstrap config, error %v", err) } logger.Debugf("Bootstrap content: %s", data) var jsonData map[string]json.RawMessage if err := json.Unmarshal(data, &jsonData); err != nil { - return nil, fmt.Errorf("xds: Failed to parse file %s (content %v) with error: %v", fName, string(data), err) + return nil, fmt.Errorf("xds: Failed to parse bootstrap config (content %v) with error: %v", string(data), err) } serverSupportsV3 := false diff --git a/xds/internal/env/env.go b/xds/internal/env/env.go index c4b46bae171b..10d5e7e9f2b2 100644 --- a/xds/internal/env/env.go +++ b/xds/internal/env/env.go @@ -27,6 +27,7 @@ import ( const ( bootstrapFileNameEnv = "GRPC_XDS_BOOTSTRAP" + bootstrapFileContentEnv = "GRPC_XDS_BOOTSTRAP_CONFIG" xdsV3SupportEnv = "GRPC_XDS_EXPERIMENTAL_V3_SUPPORT" circuitBreakingSupportEnv = "GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING" timeoutSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT" @@ -37,6 +38,10 @@ var ( // configuration. Users can specify the location of the bootstrap file by // setting the environment variable "GRPC_XDS_BOOSTRAP". BootstrapFileName = os.Getenv(bootstrapFileNameEnv) + // BootstrapFileContent holds the content of the xDS bootstrap + // configuration. Users can specify the bootstrap config by + // setting the environment variable "GRPC_XDS_BOOSTRAP_CONFIG". + BootstrapFileContent = os.Getenv(bootstrapFileContentEnv) // V3Support indicates whether xDS v3 API support is enabled, which can be // done by setting the environment variable // "GRPC_XDS_EXPERIMENTAL_V3_SUPPORT" to "true". From ef9bcc018d2f6cc56efa33e7e8faadeb41687576 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Thu, 14 Jan 2021 11:06:45 -0800 Subject: [PATCH 2/6] [bootstrap_env_content] test refactor 1 --- .../client/bootstrap/bootstrap_test.go | 110 ++++++------------ 1 file changed, 35 insertions(+), 75 deletions(-) diff --git a/xds/internal/client/bootstrap/bootstrap_test.go b/xds/internal/client/bootstrap/bootstrap_test.go index 9d5aec26df71..0a86d0a26443 100644 --- a/xds/internal/client/bootstrap/bootstrap_test.go +++ b/xds/internal/client/bootstrap/bootstrap_test.go @@ -264,13 +264,17 @@ func (c *Config) compare(want *Config) error { return nil } +func fileReadFromFileMap(bootstrapFileMap map[string]string, name string) ([]byte, error) { + if b, ok := bootstrapFileMap[name]; ok { + return []byte(b), nil + } + return nil, os.ErrNotExist +} + func setupBootstrapOverride(bootstrapFileMap map[string]string) func() { oldFileReadFunc := bootstrapFileReadFunc - bootstrapFileReadFunc = func(name string) ([]byte, error) { - if b, ok := bootstrapFileMap[name]; ok { - return []byte(b), nil - } - return nil, os.ErrNotExist + bootstrapFileReadFunc = func(filename string) ([]byte, error) { + return fileReadFromFileMap(bootstrapFileMap, filename) } return func() { bootstrapFileReadFunc = oldFileReadFunc } } @@ -278,6 +282,23 @@ func setupBootstrapOverride(bootstrapFileMap map[string]string) func() { // TODO: enable leak check for this package when // https://github.com/googleapis/google-cloud-go/issues/2417 is fixed. +func testNewConfigWithFileNameEnv(t *testing.T, fileName string, wantError bool, wantConfig *Config) { + origBootstrapFileName := env.BootstrapFileName + env.BootstrapFileName = fileName + defer func() { env.BootstrapFileName = origBootstrapFileName }() + + c, err := NewConfig() + if (err != nil) != wantError { + t.Fatalf("NewConfig() returned error %v, wantError: %v", err, wantError) + } + if wantError { + return + } + if err := c.compare(wantConfig); err != nil { + t.Fatal(err) + } +} + // TestNewConfigV2ProtoFailure exercises the functionality in NewConfig with // different bootstrap file contents which are expected to fail. func TestNewConfigV2ProtoFailure(t *testing.T) { @@ -338,13 +359,7 @@ func TestNewConfigV2ProtoFailure(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - origBootstrapFileName := env.BootstrapFileName - env.BootstrapFileName = test.name - defer func() { env.BootstrapFileName = origBootstrapFileName }() - - if _, err := NewConfig(); err == nil { - t.Fatalf("NewConfig() returned nil error, expected to fail") - } + testNewConfigWithFileNameEnv(t, test.name, true, nil) }) } } @@ -382,17 +397,7 @@ func TestNewConfigV2ProtoSuccess(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - origBootstrapFileName := env.BootstrapFileName - env.BootstrapFileName = test.name - defer func() { env.BootstrapFileName = origBootstrapFileName }() - - c, err := NewConfig() - if err != nil { - t.Fatalf("NewConfig() failed: %v", err) - } - if err := c.compare(test.wantConfig); err != nil { - t.Fatal(err) - } + testNewConfigWithFileNameEnv(t, test.name, false, test.wantConfig) }) } } @@ -419,17 +424,7 @@ func TestNewConfigV3SupportNotEnabledOnClient(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - origBootstrapFileName := env.BootstrapFileName - env.BootstrapFileName = test.name - defer func() { env.BootstrapFileName = origBootstrapFileName }() - - c, err := NewConfig() - if err != nil { - t.Fatalf("NewConfig() failed: %v", err) - } - if err := c.compare(test.wantConfig); err != nil { - t.Fatal(err) - } + testNewConfigWithFileNameEnv(t, test.name, false, test.wantConfig) }) } } @@ -456,24 +451,15 @@ func TestNewConfigV3SupportEnabledOnClient(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - origBootstrapFileName := env.BootstrapFileName - env.BootstrapFileName = test.name - defer func() { env.BootstrapFileName = origBootstrapFileName }() - - c, err := NewConfig() - if err != nil { - t.Fatalf("NewConfig() failed: %v", err) - } - if err := c.compare(test.wantConfig); err != nil { - t.Fatal(err) - } + testNewConfigWithFileNameEnv(t, test.name, false, test.wantConfig) }) } } -// TestNewConfigBootstrapFileEnvNotSet tests the case where the bootstrap file +// TestNewConfigBootstrapEnvNotSet tests the case where the bootstrap file // environment variable is not set. -func TestNewConfigBootstrapFileEnvNotSet(t *testing.T) { +func TestNewConfigBootstrapEnvNotSet(t *testing.T) { + // FIXME: also test the other env vairable, and priority origBootstrapFileName := env.BootstrapFileName env.BootstrapFileName = "" defer func() { env.BootstrapFileName = origBootstrapFileName }() @@ -686,20 +672,7 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - origBootstrapFileName := env.BootstrapFileName - env.BootstrapFileName = test.name - defer func() { env.BootstrapFileName = origBootstrapFileName }() - - c, err := NewConfig() - if (err != nil) != test.wantErr { - t.Fatalf("NewConfig() returned: %v, wantErr: %v", err, test.wantErr) - } - if test.wantErr { - return - } - if err := c.compare(test.wantConfig); err != nil { - t.Fatal(err) - } + testNewConfigWithFileNameEnv(t, test.name, test.wantErr, test.wantConfig) }) } } @@ -764,20 +737,7 @@ func TestNewConfigWithServerResourceNameID(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - origBootstrapFileName := env.BootstrapFileName - env.BootstrapFileName = test.name - defer func() { env.BootstrapFileName = origBootstrapFileName }() - - c, err := NewConfig() - if (err != nil) != test.wantErr { - t.Fatalf("NewConfig() returned (%+v, %v), wantErr: %v", c, err, test.wantErr) - } - if test.wantErr { - return - } - if err := c.compare(test.wantConfig); err != nil { - t.Fatal(err) - } + testNewConfigWithFileNameEnv(t, test.name, test.wantErr, test.wantConfig) }) } } From 6cba88621e13d7227df9672588e4bfa9b31b6fa3 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Thu, 14 Jan 2021 11:29:43 -0800 Subject: [PATCH 3/6] [bootstrap_env_content] add test for content env variable --- .../client/bootstrap/bootstrap_test.go | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/xds/internal/client/bootstrap/bootstrap_test.go b/xds/internal/client/bootstrap/bootstrap_test.go index 0a86d0a26443..fc646c64e636 100644 --- a/xds/internal/client/bootstrap/bootstrap_test.go +++ b/xds/internal/client/bootstrap/bootstrap_test.go @@ -282,6 +282,8 @@ func setupBootstrapOverride(bootstrapFileMap map[string]string) func() { // TODO: enable leak check for this package when // https://github.com/googleapis/google-cloud-go/issues/2417 is fixed. +// This function overrides the bootstrap file NAME env variable, to test the +// code that reads file with the given fileName. func testNewConfigWithFileNameEnv(t *testing.T, fileName string, wantError bool, wantConfig *Config) { origBootstrapFileName := env.BootstrapFileName env.BootstrapFileName = fileName @@ -299,6 +301,30 @@ func testNewConfigWithFileNameEnv(t *testing.T, fileName string, wantError bool, } } +// This function overrides the bootstrap file CONTENT env variable, to test the +// code that uses the content from env directly. +func testNewConfigWithFileContentEnv(t *testing.T, fileName string, wantError bool, wantConfig *Config) { + b, err := bootstrapFileReadFunc(fileName) + if err != nil { + // If file reading failed, skip this test. + return + } + origBootstrapContent := env.BootstrapFileContent + env.BootstrapFileContent = string(b) + defer func() { env.BootstrapFileContent = origBootstrapContent }() + + c, err := NewConfig() + if (err != nil) != wantError { + t.Fatalf("NewConfig() returned error %v, wantError: %v", err, wantError) + } + if wantError { + return + } + if err := c.compare(wantConfig); err != nil { + t.Fatal(err) + } +} + // TestNewConfigV2ProtoFailure exercises the functionality in NewConfig with // different bootstrap file contents which are expected to fail. func TestNewConfigV2ProtoFailure(t *testing.T) { @@ -360,6 +386,7 @@ func TestNewConfigV2ProtoFailure(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { testNewConfigWithFileNameEnv(t, test.name, true, nil) + testNewConfigWithFileContentEnv(t, test.name, true, nil) }) } } @@ -398,6 +425,7 @@ func TestNewConfigV2ProtoSuccess(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { testNewConfigWithFileNameEnv(t, test.name, false, test.wantConfig) + testNewConfigWithFileContentEnv(t, test.name, false, test.wantConfig) }) } } @@ -425,6 +453,7 @@ func TestNewConfigV3SupportNotEnabledOnClient(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { testNewConfigWithFileNameEnv(t, test.name, false, test.wantConfig) + testNewConfigWithFileContentEnv(t, test.name, false, test.wantConfig) }) } } @@ -452,6 +481,7 @@ func TestNewConfigV3SupportEnabledOnClient(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { testNewConfigWithFileNameEnv(t, test.name, false, test.wantConfig) + testNewConfigWithFileContentEnv(t, test.name, false, test.wantConfig) }) } } @@ -673,6 +703,7 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { testNewConfigWithFileNameEnv(t, test.name, test.wantErr, test.wantConfig) + testNewConfigWithFileContentEnv(t, test.name, test.wantErr, test.wantConfig) }) } } @@ -738,6 +769,7 @@ func TestNewConfigWithServerResourceNameID(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { testNewConfigWithFileNameEnv(t, test.name, test.wantErr, test.wantConfig) + testNewConfigWithFileContentEnv(t, test.name, test.wantErr, test.wantConfig) }) } } From b14d5fc81b0531b67d2f853e38696be2ad25bbd4 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Thu, 14 Jan 2021 11:54:43 -0800 Subject: [PATCH 4/6] [bootstrap_env_content] more tests --- .../client/bootstrap/bootstrap_test.go | 49 +++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/xds/internal/client/bootstrap/bootstrap_test.go b/xds/internal/client/bootstrap/bootstrap_test.go index fc646c64e636..3368af1f6a58 100644 --- a/xds/internal/client/bootstrap/bootstrap_test.go +++ b/xds/internal/client/bootstrap/bootstrap_test.go @@ -486,17 +486,60 @@ func TestNewConfigV3SupportEnabledOnClient(t *testing.T) { } } -// TestNewConfigBootstrapEnvNotSet tests the case where the bootstrap file +// TestNewConfigBootstrapEnvPriority tests that the two env variables are read in correct priority +// +// the case where the bootstrap file // environment variable is not set. -func TestNewConfigBootstrapEnvNotSet(t *testing.T) { - // FIXME: also test the other env vairable, and priority +func TestNewConfigBootstrapEnvPriority(t *testing.T) { + origV3Support := env.V3Support + env.V3Support = true + defer func() { env.V3Support = origV3Support }() + + oldFileReadFunc := bootstrapFileReadFunc + bootstrapFileReadFunc = func(filename string) ([]byte, error) { + return fileReadFromFileMap(v2BootstrapFileMap, filename) + } + defer func() { bootstrapFileReadFunc = oldFileReadFunc }() + + goodFileName1 := "goodBootstrap" + goodConfig1 := nonNilCredsConfigV2 + + goodFileName2 := "serverSupportsV3" + goodFileContent2 := v3BootstrapFileMap[goodFileName2] + goodConfig2 := nonNilCredsConfigV3 + origBootstrapFileName := env.BootstrapFileName env.BootstrapFileName = "" defer func() { env.BootstrapFileName = origBootstrapFileName }() + origBootstrapContent := env.BootstrapFileContent + env.BootstrapFileContent = "" + defer func() { env.BootstrapFileContent = origBootstrapContent }() + + // When both env variables are empty, NewConfig should fail. if _, err := NewConfig(); err == nil { t.Errorf("NewConfig() returned nil error, expected to fail") } + + // When one of them is set, it should be used. + env.BootstrapFileName = goodFileName1 + env.BootstrapFileContent = "" + if c, err := NewConfig(); err != nil || c.compare(goodConfig1) != nil { + t.Errorf("NewConfig() = %v, %v, want: %v, %v", c, err, goodConfig1, nil) + } + + env.BootstrapFileName = "" + env.BootstrapFileContent = goodFileContent2 + if c, err := NewConfig(); err != nil || c.compare(goodConfig2) != nil { + t.Errorf("NewConfig() = %v, %v, want: %v, %v", c, err, goodConfig1, nil) + } + + // Set both, file name should be read. + env.BootstrapFileName = goodFileName1 + env.BootstrapFileContent = goodFileContent2 + if c, err := NewConfig(); err != nil || c.compare(goodConfig1) != nil { + t.Errorf("NewConfig() = %v, %v, want: %v, %v", c, err, goodConfig1, nil) + } } func init() { From eea8343d9d673a7f13b1022fc1c70e735e59325b Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Thu, 14 Jan 2021 15:35:05 -0800 Subject: [PATCH 5/6] [bootstrap_env_content] c1 --- xds/internal/client/bootstrap/bootstrap.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/xds/internal/client/bootstrap/bootstrap.go b/xds/internal/client/bootstrap/bootstrap.go index 50624ef6fce0..2cf361f61f60 100644 --- a/xds/internal/client/bootstrap/bootstrap.go +++ b/xds/internal/client/bootstrap/bootstrap.go @@ -108,6 +108,7 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) { // // Note that even if the content is invalid, we don't failover to the // file content env variable. + logger.Debugf("xds: using bootstrap file with name %q", fName) return bootstrapFileReadFunc(fName) } @@ -115,7 +116,7 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) { return []byte(fContent), nil } - return nil, fmt.Errorf("xds: bootstrap environment variable (%q, or %q) not defined", "GRPC_XDS_BOOTSTRAP", "GRPC_XDS_BOOTSTRAP_CONFIG") + return nil, fmt.Errorf("none of the bootstrap environment variables (%q or %q) defined", "GRPC_XDS_BOOTSTRAP", "GRPC_XDS_BOOTSTRAP_CONFIG") } // NewConfig returns a new instance of Config initialized by reading the @@ -160,13 +161,13 @@ func NewConfig() (*Config, error) { data, err := bootstrapConfigFromEnvVariable() if err != nil { - return nil, fmt.Errorf("xds: Failed to get bootstrap config, error %v", err) + return nil, fmt.Errorf("xds: Failed to read bootstrap config: %v", err) } logger.Debugf("Bootstrap content: %s", data) var jsonData map[string]json.RawMessage if err := json.Unmarshal(data, &jsonData); err != nil { - return nil, fmt.Errorf("xds: Failed to parse bootstrap config (content %v) with error: %v", string(data), err) + return nil, fmt.Errorf("xds: Failed to parse bootstrap config: %v", err) } serverSupportsV3 := false From 7003d7e8c79d6712ab0e782b9e96449633edeb2f Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Fri, 15 Jan 2021 11:02:41 -0800 Subject: [PATCH 6/6] [bootstrap_env_content] consts --- xds/internal/client/bootstrap/bootstrap.go | 2 +- xds/internal/env/env.go | 22 ++++++++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/xds/internal/client/bootstrap/bootstrap.go b/xds/internal/client/bootstrap/bootstrap.go index 2cf361f61f60..385ba87cd7df 100644 --- a/xds/internal/client/bootstrap/bootstrap.go +++ b/xds/internal/client/bootstrap/bootstrap.go @@ -116,7 +116,7 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) { return []byte(fContent), nil } - return nil, fmt.Errorf("none of the bootstrap environment variables (%q or %q) defined", "GRPC_XDS_BOOTSTRAP", "GRPC_XDS_BOOTSTRAP_CONFIG") + return nil, fmt.Errorf("none of the bootstrap environment variables (%q or %q) defined", env.BootstrapFileNameEnv, env.BootstrapFileContentEnv) } // NewConfig returns a new instance of Config initialized by reading the diff --git a/xds/internal/env/env.go b/xds/internal/env/env.go index 10d5e7e9f2b2..38b62808fa39 100644 --- a/xds/internal/env/env.go +++ b/xds/internal/env/env.go @@ -26,8 +26,18 @@ import ( ) const ( - bootstrapFileNameEnv = "GRPC_XDS_BOOTSTRAP" - bootstrapFileContentEnv = "GRPC_XDS_BOOTSTRAP_CONFIG" + // BootstrapFileNameEnv is the env variable to set bootstrap file name. + // Do not use this and read from env directly. Its value is read and kept in + // variable BootstrapFileName. + // + // When both bootstrap FileName and FileContent are set, FileName is used. + BootstrapFileNameEnv = "GRPC_XDS_BOOTSTRAP" + // BootstrapFileContentEnv is the env variable to set bootstrapp file + // content. Do not use this and read from env directly. Its value is read + // and kept in variable BootstrapFileName. + // + // When both bootstrap FileName and FileContent are set, FileName is used. + BootstrapFileContentEnv = "GRPC_XDS_BOOTSTRAP_CONFIG" xdsV3SupportEnv = "GRPC_XDS_EXPERIMENTAL_V3_SUPPORT" circuitBreakingSupportEnv = "GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING" timeoutSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT" @@ -37,11 +47,15 @@ var ( // BootstrapFileName holds the name of the file which contains xDS bootstrap // configuration. Users can specify the location of the bootstrap file by // setting the environment variable "GRPC_XDS_BOOSTRAP". - BootstrapFileName = os.Getenv(bootstrapFileNameEnv) + // + // When both bootstrap FileName and FileContent are set, FileName is used. + BootstrapFileName = os.Getenv(BootstrapFileNameEnv) // BootstrapFileContent holds the content of the xDS bootstrap // configuration. Users can specify the bootstrap config by // setting the environment variable "GRPC_XDS_BOOSTRAP_CONFIG". - BootstrapFileContent = os.Getenv(bootstrapFileContentEnv) + // + // When both bootstrap FileName and FileContent are set, FileName is used. + BootstrapFileContent = os.Getenv(BootstrapFileContentEnv) // V3Support indicates whether xDS v3 API support is enabled, which can be // done by setting the environment variable // "GRPC_XDS_EXPERIMENTAL_V3_SUPPORT" to "true".