Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(xp-treatment,plugin): Fix treatment service and plugin's method of initiating management service client #89

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions plugins/turing/manager/experiment_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"net/http"
"strconv"
"time"

Expand Down Expand Up @@ -167,15 +168,19 @@ func NewExperimentManager(configData json.RawMessage) (manager.CustomExperimentM
}

// Create Google Client
httpClient := http.DefaultClient
googleClient, err := auth.InitGoogleClient(context.Background())
if err != nil {
return nil, err
if err == nil {
googleClient.Timeout = defaultRequestTimeout
httpClient = googleClient
} else {
log.Infof("Google default credential not found. Fallback to HTTP default client")
}
googleClient.Timeout = defaultRequestTimeout

// Create XP client
client, err := xpclient.NewClientWithResponses(
config.BaseURL,
xpclient.WithHTTPClient(googleClient),
xpclient.WithHTTPClient(httpClient),
)
if err != nil {
return nil, fmt.Errorf("Unable to create XP management client: %s", err.Error())
Expand Down
1 change: 0 additions & 1 deletion treatment-service/appcontext/appcontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ func NewAppContext(cfg *config.Config) (*AppContext, error) {
localStorage, err := models.NewLocalStorage(
cfg.GetProjectIds(),
cfg.ManagementService.URL,
cfg.ManagementService.AuthorizationEnabled,
cfg.DeploymentConfig.GoogleApplicationCredentialsEnvVar,
)
if err != nil {
Expand Down
1 change: 0 additions & 1 deletion treatment-service/appcontext/appcontext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ func TestContext(t *testing.T) {
localStorage, err := models.NewLocalStorage(
testConfig.GetProjectIds(),
testConfig.ManagementService.URL,
testConfig.ManagementService.AuthorizationEnabled,
"",
)
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ func (suite *TreatmentServiceTestSuite) TestAllFiltersSwitchback() {
}

func (suite *TreatmentServiceTestSuite) TestLocalStorage() {
storage, err := models.NewLocalStorage([]models.ProjectId{1}, suite.managementServiceServer.URL, false, "")
storage, err := models.NewLocalStorage([]models.ProjectId{1}, suite.managementServiceServer.URL, "")
suite.Require().NoError(err)
suite.Require().NotEmpty(storage)

Expand Down
40 changes: 21 additions & 19 deletions treatment-service/models/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,31 +561,33 @@ func (s *LocalStorage) getAllProjects() ([]*pubsub.ProjectSettings, error) {
func NewLocalStorage(
projectIds []ProjectId,
xpServer string,
authzEnabled bool,
googleApplicationCredentialsEnvVar string,
) (*LocalStorage, error) {
// Set up Request Modifiers
clientOptions := []managementClient.ClientOption{}
if authzEnabled {
var googleClient *http.Client
var err error
// Init Google client for Authz. When using a non-empty googleApplicationCredentialsEnvVar that contains a file
// path to a credentials file, the credentials file MUST contain a Google SERVICE ACCOUNT for authentication to
// work correctly
if filepath := os.Getenv(googleApplicationCredentialsEnvVar); filepath != "" {
googleClient, err = auth.InitGoogleClientFromCredentialsFile(context.Background(), filepath)
} else {
googleClient, err = auth.InitGoogleClient(context.Background())
}
if err != nil {
return nil, err
}

clientOptions = append(
clientOptions,
managementClient.WithHTTPClient(googleClient),
)
httpClient := http.DefaultClient
var googleClient *http.Client
var err error
// Init Google client for Authz. When using a non-empty googleApplicationCredentialsEnvVar that contains a file
// path to a credentials file, the credentials file MUST contain a Google SERVICE ACCOUNT for authentication to
// work correctly
if filepath := os.Getenv(googleApplicationCredentialsEnvVar); filepath != "" {
googleClient, err = auth.InitGoogleClientFromCredentialsFile(context.Background(), filepath)
} else {
googleClient, err = auth.InitGoogleClient(context.Background())
}

if err == nil {
httpClient = googleClient
} else {
log.Println("Google default credential not found. Fallback to HTTP default client")
}

clientOptions = append(
clientOptions,
managementClient.WithHTTPClient(httpClient),
)
xpClient, err := managementClient.NewClientWithResponses(xpServer, clientOptions...)
if err != nil {
return nil, err
Expand Down
Loading