From f8e6c2c3a407543bf10a0be44bf4228a78634231 Mon Sep 17 00:00:00 2001 From: Eric Wollesen <169516+ewollesen@users.noreply.github.com> Date: Wed, 15 Nov 2023 10:01:55 -0700 Subject: [PATCH] [BACK-2737] adds an alerts client (#679) * remove Timeout from config.Config Removing the timeout simplifies the Config and related Clients which no longer need to be concerned with it. It's used by a single client request. That request now sets a timeout on the context, preventing a bunch of unnecessary complication everywhere else. * deprecated functions are deprecated * add ConfigLoader interface This interface provides a way to migrate away from the env.reporter. The intention is to use envconfig in future work. I tried implementing the existing config.Reporter interface with envconfig, but wasn't able to find a solution that felt good. I wrote a solution using reflection, but it felt overcomplicated (as anything using reflection does), and I tried a brute force approach, but it was heavily coupled to the config structs, and was brittle as a result. This solution uses an inversion of responsibility pattern. Unfortunately it gets a bit repetitive with all the config/client/platform terminology that already exists, but each individual piece at least is simple and straightforward. Hopefully in time, we can further simplify or even just better name some of the pieces to let the code read more clearly. BACK-2500 * adds an alerts client BACK-2500 * make client config's UserAgent property optional The value is useful for debugging, but it's not being set shouldn't stop a test or any other process from running. Also fixed a few calls to Go stdlib deprecated functions. * update mongo/travis config Stolen from Alex's good work in https://github.com/tidepool-org/platform/pull/681 * force snyk to use govendor as its package manager Its autodetection isn't working correctly. --- .github/workflows/main.yml | 2 +- .travis.yml | 12 ++- alerts/client.go | 136 ++++++++++++++++++++++++ alerts/client_test.go | 118 ++++++++++++++++++++ alerts/repo.go | 1 - auth/client/client.go | 39 +++++-- auth/client/external.go | 110 +++++++++++++++---- auth/service/service/service.go | 12 ++- client/client.go | 9 +- client/client_test.go | 78 +++++++------- client/config.go | 71 ++++++++++--- client/config_test.go | 17 +-- data/client/client.go | 20 ++-- data/client/client_test.go | 7 -- data/service/service/standard.go | 8 +- metric/client/client_test.go | 7 -- platform/client.go | 18 +--- platform/config.go | 59 +++++++++- platform/config_test.go | 19 ++-- prescription/application/application.go | 1 + service/service/DEPRECATED_service.go | 6 +- service/service/authenticated.go | 6 +- task/service/service/service.go | 12 ++- user/client/client.go | 4 +- 24 files changed, 601 insertions(+), 171 deletions(-) create mode 100644 alerts/client.go create mode 100644 alerts/client_test.go delete mode 100644 alerts/repo.go diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 72aa0d55e..61cbdd175 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -10,4 +10,4 @@ jobs: env: SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }} with: - args: snyk ignore --id=SNYK-GOLANG-GITHUBCOMDISINTEGRATIONIMAGING-5880692 --expiry=2024-03-12 --policy-path=.snyk + args: --package-manager=govendor snyk ignore --id=SNYK-GOLANG-GITHUBCOMDISINTEGRATIONIMAGING-5880692 --expiry=2024-03-12 --policy-path=.snyk diff --git a/.travis.yml b/.travis.yml index f0360aee8..e5587cf85 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,20 +12,22 @@ go_import_path: github.com/tidepool-org/platform env: global: - MONGODB=6.0.11 - - MONGOSH=1.10.6 + - MONGOSH=2.0.2 - DIST=jammy before_install: - - sudo apt-get remove -y mongodb-org mongodb-org-mongos mongodb-org-server mongodb-org-shell mongodb-org-tools - - wget -nv https://repo.mongodb.org/apt/ubuntu/dists/${DIST}/mongodb-org/6.0/multiverse/binary-amd64/mongodb-org-server_${MONGODB}_amd64.deb -O /tmp/mongodb.deb - - wget -nv https://downloads.mongodb.com/compass/mongodb-mongosh_${MONGOSH}_amd64.deb -O /tmp/mongosh.deb - - sudo apt install /tmp/mongodb.deb /tmp/mongosh.deb + - sudo apt update + - sudo apt install -y docker-buildx mongodb-org=${MONGODB} mongodb-org-database=${MONGODB} mongodb-org-server=${MONGODB} mongodb-mongosh=${MONGOSH} mongodb-org-mongos=${MONGODB} mongodb-org-tools=${MONGODB} - mkdir /tmp/data - /usr/bin/mongod --replSet rs0 --dbpath /tmp/data --bind_ip 127.0.0.1 --logpath ${PWD}/mongod.log &> /dev/null & - until nc -z localhost 27017; do echo Waiting for MongoDB; sleep 1; done - /usr/bin/mongosh --host 127.0.0.1 --port 27017 --eval 'rs.initiate(); while (rs.status().startupStatus || (rs.status().hasOwnProperty("myState") && rs.status().myState != 1)) { printjson( rs.status() ); sleep(1000); }; printjson( rs.status() );' addons: + apt: + sources: + - sourceline: 'deb https://repo.mongodb.org/apt/ubuntu jammy/mongodb-org/6.0 multiverse' + key_url: 'https://pgp.mongodb.com/server-6.0.asc' artifacts: s3_region: us-west-2 paths: diff --git a/alerts/client.go b/alerts/client.go new file mode 100644 index 000000000..c02d5049f --- /dev/null +++ b/alerts/client.go @@ -0,0 +1,136 @@ +package alerts + +import ( + "context" + "fmt" + "net/http" + + "github.com/kelseyhightower/envconfig" + + "github.com/tidepool-org/platform/auth" + "github.com/tidepool-org/platform/client" + platformlog "github.com/tidepool-org/platform/log" + "github.com/tidepool-org/platform/log/null" + "github.com/tidepool-org/platform/platform" + "github.com/tidepool-org/platform/request" +) + +// Client for managing alerts configs. +type Client struct { + client PlatformClient + logger platformlog.Logger + token TokenProvider +} + +// NewClient builds a client for interacting with alerts API endpoints. +// +// If no logger is provided, a null logger is used. +func NewClient(client PlatformClient, token TokenProvider, logger platformlog.Logger) *Client { + if logger == nil { + logger = null.NewLogger() + } + return &Client{ + client: client, + logger: logger, + token: token, + } +} + +// platform.Client is one implementation +type PlatformClient interface { + ConstructURL(paths ...string) string + RequestData(ctx context.Context, method string, url string, mutators []request.RequestMutator, + requestBody interface{}, responseBody interface{}, inspectors ...request.ResponseInspector) error +} + +// client.External is one implementation +type TokenProvider interface { + // ServerSessionToken provides a server-to-server API authentication token. + ServerSessionToken() (string, error) +} + +// request performs common operations before passing a request off to the +// underlying platform.Client. +func (c *Client) request(ctx context.Context, method, url string, body any) error { + // Platform's client.Client expects a logger to exist in the request's + // context. If it doesn't exist, request processing will panic. + loggingCtx := platformlog.NewContextWithLogger(ctx, c.logger) + // Make sure the auth token is injected into the request's headers. + return c.requestWithAuth(loggingCtx, method, url, body) +} + +// requestWithAuth injects an auth token before calling platform.Client.RequestData. +// +// At time of writing, this is the only way to inject credentials into +// platform.Client. It might be nice to be able to use a mutator, but the auth +// is specifically handled by the platform.Client via the context field, and +// if left blank, platform.Client errors. +func (c *Client) requestWithAuth(ctx context.Context, method, url string, body any) error { + authCtx, err := c.ctxWithAuth(ctx) + if err != nil { + return err + } + return c.client.RequestData(authCtx, method, url, nil, body, nil) +} + +// Upsert updates cfg if it exists or creates it if it doesn't. +func (c *Client) Upsert(ctx context.Context, cfg *Config) error { + url := c.client.ConstructURL("v1", "alerts", cfg.UserID, cfg.FollowedUserID) + return c.request(ctx, http.MethodPost, url, cfg) +} + +// Delete the alerts config. +func (c *Client) Delete(ctx context.Context, cfg *Config) error { + url := c.client.ConstructURL("v1", "alerts", cfg.UserID, cfg.FollowedUserID) + return c.request(ctx, http.MethodDelete, url, nil) +} + +// ctxWithAuth injects a server session token into the context. +func (c *Client) ctxWithAuth(ctx context.Context) (context.Context, error) { + token, err := c.token.ServerSessionToken() + if err != nil { + return nil, fmt.Errorf("retrieving token: %w", err) + } + return auth.NewContextWithServerSessionToken(ctx, token), nil +} + +// ConfigLoader abstracts the method by which config values are loaded. +type ConfigLoader interface { + Load(*ClientConfig) error +} + +// envconfigLoader adapts envconfig to implement ConfigLoader. +type envconfigLoader struct { + platform.ConfigLoader +} + +// NewEnvconfigLoader loads values via envconfig. +// +// If loader is nil, it defaults to envconfig for platform values. +func NewEnvconfigLoader(loader platform.ConfigLoader) *envconfigLoader { + if loader == nil { + loader = platform.NewEnvconfigLoader(nil) + } + return &envconfigLoader{ + ConfigLoader: loader, + } +} + +// Load implements ConfigLoader. +func (l *envconfigLoader) Load(cfg *ClientConfig) error { + if err := l.ConfigLoader.Load(cfg.Config); err != nil { + return err + } + if err := envconfig.Process(client.EnvconfigEmptyPrefix, cfg); err != nil { + return err + } + // Override client.Client.Address to point to the data service. + cfg.Address = cfg.DataServiceAddress + return nil +} + +type ClientConfig struct { + *platform.Config + // DataServiceAddress is used to override client.Client.Address. + DataServiceAddress string `envconfig:"TIDEPOOL_DATA_SERVICE_ADDRESS" required:"true"` +} diff --git a/alerts/client_test.go b/alerts/client_test.go new file mode 100644 index 000000000..c5a771256 --- /dev/null +++ b/alerts/client_test.go @@ -0,0 +1,118 @@ +package alerts + +import ( + "context" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/tidepool-org/platform/auth" + "github.com/tidepool-org/platform/client" + "github.com/tidepool-org/platform/log" + "github.com/tidepool-org/platform/log/null" + "github.com/tidepool-org/platform/platform" +) + +const testToken = "auth-me" + +var _ = Describe("Client", func() { + var test404Server, test200Server *httptest.Server + var testAuthServer func(*string) *httptest.Server + + BeforeEach(func() { + t := GinkgoT() + // There's no need to create these before each test, but I can't get + // Ginkgo to let me start these just once. + test404Server = testServer(t, func(w http.ResponseWriter, r *http.Request) { + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + }) + test200Server = testServer(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + testAuthServer = func(token *string) *httptest.Server { + return testServer(t, func(w http.ResponseWriter, r *http.Request) { + *token = r.Header.Get(auth.TidepoolSessionTokenHeaderKey) + w.WriteHeader(http.StatusOK) + }) + } + }) + + Context("Delete", func() { + It("returns an error on non-200 responses", func() { + client, ctx := newAlertsClientTest(test404Server) + err := client.Delete(ctx, &Config{}) + Expect(err).Should(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("resource not found"))) + }) + + It("returns nil on success", func() { + client, ctx := newAlertsClientTest(test200Server) + err := client.Delete(ctx, &Config{}) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("injects an auth token", func() { + token := "" + client, ctx := newAlertsClientTest(testAuthServer(&token)) + _ = client.Delete(ctx, &Config{}) + Expect(token).To(Equal(testToken)) + }) + }) + + Context("Upsert", func() { + It("returns an error on non-200 responses", func() { + client, ctx := newAlertsClientTest(test404Server) + err := client.Upsert(ctx, &Config{}) + Expect(err).Should(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("resource not found"))) + }) + + It("returns nil on success", func() { + client, ctx := newAlertsClientTest(test200Server) + err := client.Upsert(ctx, &Config{}) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("injects an auth token", func() { + token := "" + client, ctx := newAlertsClientTest(testAuthServer(&token)) + _ = client.Upsert(ctx, &Config{}) + Expect(token).To(Equal(testToken)) + }) + }) +}) + +func buildTestClient(s *httptest.Server) *Client { + pCfg := &platform.Config{ + Config: &client.Config{ + Address: s.URL, + }, + } + token := mockTokenProvider(testToken) + pc, err := platform.NewClient(pCfg, platform.AuthorizeAsService) + Expect(err).ToNot(HaveOccurred()) + client := NewClient(pc, token, null.NewLogger()) + return client +} + +func newAlertsClientTest(server *httptest.Server) (*Client, context.Context) { + return buildTestClient(server), contextWithNullLogger() +} + +func contextWithNullLogger() context.Context { + return log.NewContextWithLogger(context.Background(), null.NewLogger()) +} + +type mockTokenProvider string + +func (p mockTokenProvider) ServerSessionToken() (string, error) { + return string(p), nil +} + +func testServer(t GinkgoTInterface, handler http.HandlerFunc) *httptest.Server { + s := httptest.NewServer(http.HandlerFunc(handler)) + t.Cleanup(s.Close) + return s +} diff --git a/alerts/repo.go b/alerts/repo.go deleted file mode 100644 index 3dc80686f..000000000 --- a/alerts/repo.go +++ /dev/null @@ -1 +0,0 @@ -package alerts diff --git a/auth/client/client.go b/auth/client/client.go index bd8adde3d..fd92f6efb 100644 --- a/auth/client/client.go +++ b/auth/client/client.go @@ -5,7 +5,6 @@ import ( "net/http" "github.com/tidepool-org/platform/auth" - "github.com/tidepool-org/platform/config" "github.com/tidepool-org/platform/errors" "github.com/tidepool-org/platform/log" "github.com/tidepool-org/platform/page" @@ -26,11 +25,8 @@ func NewConfig() *Config { } } -func (c *Config) Load(configReporter config.Reporter) error { - if err := c.Config.Load(configReporter); err != nil { - return err - } - return c.ExternalConfig.Load(configReporter.WithScopes("external")) +func (c *Config) Load(loader ConfigLoader) error { + return loader.Load(c) } func (c *Config) Validate() error { @@ -311,3 +307,34 @@ func (c *Client) DeleteRestrictedToken(ctx context.Context, id string) error { url := c.client.ConstructURL("v1", "restricted_tokens", id) return c.client.RequestData(ctx, http.MethodDelete, url, nil, nil, nil) } + +type ConfigLoader interface { + Load(*Config) error +} + +// configHybridLoader combines an ExternalConfigLoader with a platform.ConfigLoader. +// +// Whereas we usually have different implementations, in this case, it's just +// two other loaders together, so no need for multiple other implementations +// here. +type configHybridLoader struct { + ExternalConfigLoader + platform.ConfigLoader +} + +func NewConfigLoader(ext ExternalConfigLoader, plt platform.ConfigLoader) *configHybridLoader { + return &configHybridLoader{ + ExternalConfigLoader: ext, + ConfigLoader: plt, + } +} + +func (l *configHybridLoader) Load(cfg *Config) error { + if err := l.ExternalConfigLoader.Load(cfg.ExternalConfig); err != nil { + return err + } + if err := l.ConfigLoader.Load(cfg.Config); err != nil { + return err + } + return nil +} diff --git a/auth/client/external.go b/auth/client/external.go index 035d53dca..8eb9f45ff 100644 --- a/auth/client/external.go +++ b/auth/client/external.go @@ -9,7 +9,10 @@ import ( "go.uber.org/fx" + "github.com/kelseyhightower/envconfig" + "github.com/tidepool-org/platform/auth" + "github.com/tidepool-org/platform/client" "github.com/tidepool-org/platform/config" "github.com/tidepool-org/platform/errors" "github.com/tidepool-org/platform/log" @@ -26,10 +29,10 @@ const ( ServerSessionTokenTimeoutOnFailureLast = 60 * time.Second ) -var ExternalClientModule = fx.Provide(func(name ServiceName, reporter config.Reporter, logger log.Logger, lifecycle fx.Lifecycle) (auth.ExternalAccessor, error) { +var ExternalClientModule = fx.Provide(func(name ServiceName, loader ExternalConfigLoader, logger log.Logger, lifecycle fx.Lifecycle) (auth.ExternalAccessor, error) { cfg := NewExternalConfig() cfg.Config.UserAgent = string(name) - if err := cfg.Load(reporter.WithScopes("auth", "client", "external")); err != nil { + if err := cfg.Load(loader); err != nil { return nil, err } external, err := NewExternal(cfg, platform.AuthorizeAsService, string(name), logger) @@ -54,37 +57,30 @@ func ProvideServiceName(name string) fx.Option { return fx.Supply(ServiceName(name)) } +func ProvideExternalLoader(reporter config.Reporter) ExternalConfigLoader { + scoped := reporter.WithScopes("auth", "client", "external") + return NewExternalConfigReporterLoader(scoped) +} + type ServiceName string type ExternalConfig struct { *platform.Config - ServerSessionTokenSecret string - ServerSessionTokenTimeout time.Duration + ServerSessionTokenSecret string `envconfig:"TIDEPOOL_AUTH_CLIENT_EXTERNAL_SERVER_SESSION_TOKEN_SECRET"` + ServerSessionTokenTimeout time.Duration `envconfig:"TIDEPOOL_AUTH_CLIENT_EXTERNAL_SERVER_SESSION_TOKEN_TIMEOUT" default:"1h"` } func NewExternalConfig() *ExternalConfig { return &ExternalConfig{ Config: platform.NewConfig(), - ServerSessionTokenTimeout: 3600 * time.Second, + ServerSessionTokenTimeout: ServerSessionTokenTimeout, } } -func (e *ExternalConfig) Load(configReporter config.Reporter) error { - if err := e.Config.Load(configReporter); err != nil { - return err - } - - e.ServerSessionTokenSecret = configReporter.GetWithDefault("server_session_token_secret", "") - if serverSessionTokenTimeoutString, err := configReporter.Get("server_session_token_timeout"); err == nil { - var serverSessionTokenTimeoutInteger int64 - serverSessionTokenTimeoutInteger, err = strconv.ParseInt(serverSessionTokenTimeoutString, 10, 0) - if err != nil { - return errors.New("server session token timeout is invalid") - } - e.ServerSessionTokenTimeout = time.Duration(serverSessionTokenTimeoutInteger) * time.Second - } +const ServerSessionTokenTimeout = time.Hour - return nil +func (e *ExternalConfig) Load(loader ExternalConfigLoader) error { + return loader.Load(e) } func (e *ExternalConfig) Validate() error { @@ -349,3 +345,77 @@ func (e *External) serverSessionToken() string { return e.serverSessionTokenSafe } + +// ExternalConfigLoader abstracts the method by which config values are loaded. +type ExternalConfigLoader interface { + // Load sets config values for the properties of ExternalConfig. + Load(*ExternalConfig) error +} + +// externalConfigReporterLoader adapts a config.Reporter to implement ConfigLoader. +type externalConfigReporterLoader struct { + Reporter config.Reporter + platform.ConfigLoader +} + +func NewExternalConfigReporterLoader(reporter config.Reporter) *externalConfigReporterLoader { + return &externalConfigReporterLoader{ + Reporter: reporter, + ConfigLoader: platform.NewConfigReporterLoader(reporter), + } +} + +// Load implements ConfigLoader. +func (l *externalConfigReporterLoader) Load(cfg *ExternalConfig) error { + if err := l.ConfigLoader.Load(cfg.Config); err != nil { + return err + } + cfg.ServerSessionTokenSecret = l.Reporter.GetWithDefault("server_session_token_secret", "") + if serverSessionTokenTimeoutString, err := l.Reporter.Get("server_session_token_timeout"); err == nil { + var serverSessionTokenTimeoutInteger int64 + serverSessionTokenTimeoutInteger, err = strconv.ParseInt(serverSessionTokenTimeoutString, 10, 0) + if err != nil { + return errors.New("server session token timeout is invalid") + } + cfg.ServerSessionTokenTimeout = time.Duration(serverSessionTokenTimeoutInteger) * time.Second + } + + return nil +} + +// externalEnvconfigLoader adapts envconfig to implement ConfigLoader. +type externalEnvconfigLoader struct { + platform.ConfigLoader +} + +// NewExternalEnvconfigLoader loads values via envconfig. +// +// If loader is nil, it defaults to envconfig for platform values. +func NewExternalEnvconfigLoader(loader platform.ConfigLoader) *externalEnvconfigLoader { + if loader == nil { + loader = platform.NewEnvconfigLoader(nil) + } + return &externalEnvconfigLoader{ + ConfigLoader: loader, + } +} + +// Load implements ConfigLoader. +func (l *externalEnvconfigLoader) Load(cfg *ExternalConfig) error { + eeCfg := &struct { + Address string `envconfig:"TIDEPOOL_AUTH_CLIENT_EXTERNAL_ADDRESS" required:"true"` + *ExternalConfig + }{ExternalConfig: cfg} + if err := envconfig.Process(client.EnvconfigEmptyPrefix, eeCfg); err != nil { + return err + } + // Override the client.Config.Address. It's not possible to change the + // envconfig tag on the config.Client at runtime. In addition, we don't + // want to use the envconfig.Prefix so that the code is more easily + // searched. The results is that we have to override this value. + cfg.Config.Config.Address = eeCfg.Address + if err := l.ConfigLoader.Load(cfg.Config); err != nil { + return err + } + return nil +} diff --git a/auth/service/service/service.go b/auth/service/service/service.go index a95afa393..abc501197 100644 --- a/auth/service/service/service.go +++ b/auth/service/service/service.go @@ -245,7 +245,9 @@ func (s *Service) initializeDataSourceClient() error { cfg := platform.NewConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("data_source", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("data_source", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load data source client config") } @@ -306,7 +308,9 @@ func (s *Service) initializeTaskClient() error { cfg := platform.NewConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("task", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("task", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load task client config") } @@ -359,7 +363,9 @@ func (s *Service) initializeAuthClient() error { cfg := client.NewExternalConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("auth", "client", "external")); err != nil { + reporter := s.ConfigReporter().WithScopes("auth", "client", "external") + loader := client.NewExternalConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load auth client config") } diff --git a/client/client.go b/client/client.go index eb4682250..7468948ba 100644 --- a/client/client.go +++ b/client/client.go @@ -6,7 +6,6 @@ import ( "encoding/json" "fmt" "io" - "io/ioutil" "net/http" "net/url" "reflect" @@ -118,7 +117,9 @@ func (c *Client) createRequest(ctx context.Context, method string, url string, m return nil, errors.New("url is missing") } - mutators = append(mutators, request.NewHeaderMutator("User-Agent", c.userAgent)) + if c.userAgent != "" { + mutators = append(mutators, request.NewHeaderMutator("User-Agent", c.userAgent)) + } var body io.Reader if requestBody != nil { @@ -173,7 +174,7 @@ func (c *Client) handleResponse(ctx context.Context, res *http.Response, req *ht serializable := &errors.Serializable{} - if bites, err := ioutil.ReadAll(io.LimitReader(res.Body, 1<<20)); err != nil { + if bites, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)); err != nil { return nil, errors.Wrap(err, "unable to read response body") } else if len(bites) == 0 { logger.Error("Response body is empty, using defacto error for status code") @@ -228,6 +229,6 @@ func responseBodyFromBytes(bites []byte) interface{} { } func drainAndClose(reader io.ReadCloser) { - io.Copy(ioutil.Discard, reader) + io.Copy(io.Discard, reader) reader.Close() } diff --git a/client/client_test.go b/client/client_test.go index 169f5f2d0..c7c778831 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -33,13 +33,10 @@ type ResponseBody struct { } var _ = Describe("Client", func() { - var userAgent string var config *client.Config BeforeEach(func() { - userAgent = testHttp.NewUserAgent() config = client.NewConfig() - config.UserAgent = userAgent }) Context("New", func() { @@ -225,6 +222,45 @@ var _ = Describe("Client", func() { } }) + Context("with a user agent", func() { + var userAgent = testHttp.NewUserAgent() + JustBeforeEach(func() { + config.UserAgent = userAgent + clnt, err = client.New(config) + Expect(err).ToNot(HaveOccurred()) + }) + + It("sets the User-Agent header in requests", func() { + server.AppendHandlers(CombineHandlers( + VerifyHeaderKV("User-Agent", userAgent), + RespondWith(http.StatusNoContent, nil))) + + _, err = clnt.RequestStreamWithHTTPClient(ctx, method, url, mutators, nil, inspectors, httpClient) + + Expect(err).ToNot(HaveOccurred()) + Expect(server.ReceivedRequests()).To(HaveLen(1)) + }) + }) + + Context("without a user agent", func() { + JustBeforeEach(func() { + config.UserAgent = "" + clnt, err = client.New(config) + Expect(err).ToNot(HaveOccurred()) + }) + + It("doesn't set one (and the Go default is used)", func() { + server.AppendHandlers(CombineHandlers( + VerifyHeaderKV("User-Agent", "Go-http-client/1.1"), + RespondWith(http.StatusNoContent, nil))) + + _, err = clnt.RequestStreamWithHTTPClient(ctx, method, url, mutators, nil, inspectors, httpClient) + + Expect(err).ToNot(HaveOccurred()) + Expect(server.ReceivedRequests()).To(HaveLen(1)) + }) + }) + It("returns error if http client is missing", func() { reader, err = clnt.RequestStreamWithHTTPClient(ctx, method, url, mutators, requestBody, inspectors, nil) Expect(err).To(MatchError("http client is missing")) @@ -286,7 +322,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(nil), RespondWith(http.StatusOK, []byte(responseString), responseHeaders), @@ -314,7 +349,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -339,7 +373,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -361,7 +394,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -383,7 +415,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -405,7 +436,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -430,7 +460,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -452,7 +481,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -474,7 +502,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -496,7 +523,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -521,7 +547,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -543,7 +568,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -565,7 +589,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -587,7 +610,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(nil), RespondWith(http.StatusOK, []byte(responseString)), @@ -609,7 +631,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody([]byte(requestString)), RespondWith(http.StatusOK, []byte(responseString)), @@ -631,7 +652,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -704,7 +724,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(nil), RespondWith(http.StatusOK, test.MarshalResponseBody(&ResponseBody{Response: responseString}), responseHeaders), @@ -732,7 +751,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(nil), RespondWith(http.StatusOK, test.MarshalResponseBody(&ResponseBody{Response: responseString}), responseHeaders), @@ -753,7 +771,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -777,7 +794,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -798,7 +814,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -819,7 +834,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -840,7 +854,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -864,7 +877,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -885,7 +897,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -906,7 +917,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -927,7 +937,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -945,13 +954,11 @@ var _ = Describe("Client", func() { Context("with an unexpected response 500 with deserializable error body", func() { var responseErr error - BeforeEach(func() { responseErr = errorsTest.RandomError() server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -972,7 +979,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -993,7 +999,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -1015,7 +1020,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -1037,7 +1041,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(nil), RespondWith(http.StatusOK, test.MarshalResponseBody(&ResponseBody{Response: responseString}), responseHeaders), @@ -1058,7 +1061,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody([]byte(requestString)), RespondWith(http.StatusOK, test.MarshalResponseBody(&ResponseBody{Response: responseString}), responseHeaders), @@ -1079,7 +1081,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), @@ -1101,7 +1102,6 @@ var _ = Describe("Client", func() { server.AppendHandlers( CombineHandlers( VerifyRequest(method, path, fmt.Sprintf("%s=%s", parameterMutator.Key, parameterMutator.Value)), - VerifyHeaderKV("User-Agent", userAgent), VerifyHeaderKV("Content-Type", "application/json; charset=utf-8"), VerifyHeaderKV(headerMutator.Key, headerMutator.Value), VerifyBody(test.MarshalRequestBody(requestBody)), diff --git a/client/config.go b/client/config.go index d2cb93143..b74720633 100644 --- a/client/config.go +++ b/client/config.go @@ -2,31 +2,33 @@ package client import ( "net/url" - "time" + + "github.com/kelseyhightower/envconfig" "github.com/tidepool-org/platform/config" "github.com/tidepool-org/platform/errors" ) type Config struct { - Address string - UserAgent string - Timeout *time.Duration + Address string // this should be overridden for loaders using envconfig + // UserAgent is an optional way for a client to identify itself. + // + // This is usually set to the name of the service that's using the + // client. If left empty, the default Go http.Client value should be used. + // + // This value can be helpful when debugging. But remember that these + // values can be spoofed, so when in doubt, verify the client's source IP. + // + // More info: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent + UserAgent string `envconfig:"TIDEPOOL_USER_AGENT"` } func NewConfig() *Config { return &Config{} } -func (c *Config) Load(configReporter config.Reporter) error { - if configReporter == nil { - return errors.New("config reporter is missing") - } - - c.Address = configReporter.GetWithDefault("address", c.Address) - c.UserAgent = configReporter.GetWithDefault("user_agent", c.UserAgent) - - return nil +func (c *Config) Load(loader ConfigLoader) error { + return loader.Load(c) } func (c *Config) Validate() error { @@ -35,9 +37,48 @@ func (c *Config) Validate() error { } else if _, err := url.Parse(c.Address); err != nil { return errors.New("address is invalid") } - if c.UserAgent == "" { - return errors.New("user agent is missing") + + return nil +} + +// ConfigLoader abstracts the method by which config values are loaded. +type ConfigLoader interface { + // Load sets config values for the properties of Config. + Load(*Config) error +} + +// configReporterLoader adapts a config.Reporter to implement ConfigLoader. +type configReporterLoader struct { + Reporter config.Reporter +} + +func NewConfigReporterLoader(reporter config.Reporter) *configReporterLoader { + return &configReporterLoader{ + Reporter: reporter, } +} +// Load implements ConfigLoader. +func (l *configReporterLoader) Load(cfg *Config) error { + cfg.Address = l.Reporter.GetWithDefault("address", cfg.Address) + cfg.UserAgent = l.Reporter.GetWithDefault("user_agent", cfg.UserAgent) return nil } + +// EnvconfigEmptyPrefix should be the empty string. +// +// By forcing the use of the environment variable name in each tag, we aim to +// make the code more easily searchable. +const EnvconfigEmptyPrefix = "" + +// envconfigLoader adapts envconfig to implement ConfigLoader. +type envconfigLoader struct{} + +func NewEnvconfigLoader() *envconfigLoader { + return &envconfigLoader{} +} + +// Load implements ConfigLoader. +func (l *envconfigLoader) Load(cfg *Config) error { + return envconfig.Process(EnvconfigEmptyPrefix, cfg) +} diff --git a/client/config_test.go b/client/config_test.go index a9245e472..4c22fbcf2 100644 --- a/client/config_test.go +++ b/client/config_test.go @@ -38,22 +38,20 @@ var _ = Describe("Config", func() { Context("Load", func() { var configReporter *configTest.Reporter + var loader client.ConfigLoader BeforeEach(func() { configReporter = configTest.NewReporter() configReporter.Config["address"] = address configReporter.Config["user_agent"] = userAgent - }) - - It("returns an error if config reporter is missing", func() { - Expect(cfg.Load(nil)).To(MatchError("config reporter is missing")) + loader = client.NewConfigReporterLoader(configReporter) }) It("uses existing address if not set", func() { existingAddress := testHttp.NewAddress() cfg.Address = existingAddress delete(configReporter.Config, "address") - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Address).To(Equal(existingAddress)) Expect(cfg.UserAgent).To(Equal(userAgent)) }) @@ -62,13 +60,13 @@ var _ = Describe("Config", func() { existingUserAgent := testHttp.NewUserAgent() cfg.UserAgent = existingUserAgent delete(configReporter.Config, "user_agent") - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Address).To(Equal(address)) Expect(cfg.UserAgent).To(Equal(existingUserAgent)) }) It("returns successfully and uses values from config reporter", func() { - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Address).To(Equal(address)) Expect(cfg.UserAgent).To(Equal(userAgent)) }) @@ -91,11 +89,6 @@ var _ = Describe("Config", func() { Expect(cfg.Validate()).To(MatchError("address is invalid")) }) - It("returns an error if the user agent is missing", func() { - cfg.UserAgent = "" - Expect(cfg.Validate()).To(MatchError("user agent is missing")) - }) - It("returns success", func() { Expect(cfg.Validate()).To(Succeed()) Expect(cfg.Address).To(Equal(address)) diff --git a/data/client/client.go b/data/client/client.go index 4449ecde9..b824202b8 100644 --- a/data/client/client.go +++ b/data/client/client.go @@ -11,14 +11,15 @@ import ( "github.com/tidepool-org/platform/errors" "github.com/tidepool-org/platform/page" "github.com/tidepool-org/platform/platform" - "github.com/tidepool-org/platform/pointer" "github.com/tidepool-org/platform/request" "github.com/tidepool-org/platform/service" structureValidator "github.com/tidepool-org/platform/structure/validator" ) const ( - ExtendedTimeout = 300 * time.Second + // ExtendedTimeout is used by requests that we expect to take longer than + // usual. + ExtendedTimeout = 5 * time.Minute ) // TODO: Move interface to data package once upload dependency broken @@ -51,15 +52,8 @@ func New(cfg *platform.Config, authorizeAs platform.AuthorizeAs) (*ClientImpl, e return nil, err } - cfg.Timeout = pointer.FromDuration(ExtendedTimeout) - extendedTimeoutClient, err := platform.NewClient(cfg, authorizeAs) - if err != nil { - return nil, err - } - return &ClientImpl{ - client: clnt, - extendedTimeoutClient: extendedTimeoutClient, + client: clnt, }, nil } @@ -217,9 +211,11 @@ func (c *ClientImpl) UpdateBGMSummary(ctx context.Context, userId string) (*type func (c *ClientImpl) BackfillSummaries(ctx context.Context, typ string) (int, error) { var count int - url := c.extendedTimeoutClient.ConstructURL("v1", "summaries", "backfill", typ) + url := c.client.ConstructURL("v1", "summaries", "backfill", typ) - if err := c.extendedTimeoutClient.RequestData(ctx, http.MethodPost, url, nil, nil, &count); err != nil { + ctxWithTimeout, cancel := context.WithTimeout(ctx, ExtendedTimeout) + defer cancel() + if err := c.client.RequestData(ctxWithTimeout, http.MethodPost, url, nil, nil, &count); err != nil { return count, errors.Wrap(err, "backfill request returned an error") } diff --git a/data/client/client_test.go b/data/client/client_test.go index 6a9e3ec48..24f09550e 100644 --- a/data/client/client_test.go +++ b/data/client/client_test.go @@ -43,13 +43,6 @@ var _ = Describe("Client", func() { Expect(clnt).To(BeNil()) }) - It("returns an error if config user agent is missing", func() { - config.UserAgent = "" - clnt, err := dataClient.New(config, platform.AuthorizeAsService) - Expect(err).To(MatchError("config is invalid; user agent is missing")) - Expect(clnt).To(BeNil()) - }) - It("returns success", func() { clnt, err := dataClient.New(config, platform.AuthorizeAsService) Expect(err).ToNot(HaveOccurred()) diff --git a/data/service/service/standard.go b/data/service/service/standard.go index 9b73be431..9111aaabc 100644 --- a/data/service/service/standard.go +++ b/data/service/service/standard.go @@ -154,7 +154,9 @@ func (s *Standard) initializeMetricClient() error { cfg := platform.NewConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("metric", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("metric", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load metric client config") } @@ -174,7 +176,9 @@ func (s *Standard) initializePermissionClient() error { cfg := platform.NewConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("permission", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("permission", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load permission client config") } diff --git a/metric/client/client_test.go b/metric/client/client_test.go index 1e49ac893..ccdcef0dd 100644 --- a/metric/client/client_test.go +++ b/metric/client/client_test.go @@ -66,13 +66,6 @@ var _ = Describe("Client", func() { Expect(clnt).To(BeNil()) }) - It("returns an error if config user agent is missing", func() { - config.UserAgent = "" - clnt, err := metricClient.New(config, platform.AuthorizeAsUser, name, versionReporter) - Expect(err).To(MatchError("config is invalid; user agent is missing")) - Expect(clnt).To(BeNil()) - }) - It("returns success", func() { clnt, err := metricClient.New(config, platform.AuthorizeAsUser, name, versionReporter) Expect(err).ToNot(HaveOccurred()) diff --git a/platform/client.go b/platform/client.go index c921da055..aadac65d4 100644 --- a/platform/client.go +++ b/platform/client.go @@ -4,7 +4,6 @@ import ( "context" "io" "net/http" - "time" "github.com/tidepool-org/platform/auth" "github.com/tidepool-org/platform/client" @@ -17,7 +16,6 @@ type AuthorizeAs int const ( AuthorizeAsService AuthorizeAs = iota AuthorizeAsUser - DefaultTimeout = 60 * time.Second ) type Client struct { @@ -48,22 +46,12 @@ func NewClient(cfg *Config, authorizeAs AuthorizeAs) (*Client, error) { // return errors.New("service secret is missing") // } // } - var timeout time.Duration - if cfg.Timeout != nil { - timeout = *cfg.Timeout - } else { - timeout = DefaultTimeout - } - - httpClient := &http.Client{ - Timeout: timeout, - } return &Client{ Client: clnt, authorizeAs: authorizeAs, serviceSecret: cfg.ServiceSecret, - httpClient: httpClient, + httpClient: &http.Client{}, }, nil } @@ -83,6 +71,10 @@ func (c *Client) Mutators(ctx context.Context) ([]request.RequestMutator, error) } else if serverSessionToken := auth.ServerSessionTokenFromContext(ctx); serverSessionToken != "" { authorizationMutator = NewSessionTokenHeaderMutator(serverSessionToken) } else { + // TODO: Should this really error? It might be nice to allow other + // clients the option of handling authentication on their own if + // they'd like, rather than enforcing that this method must be + // used. return nil, errors.New("service secret is missing") } } else { diff --git a/platform/config.go b/platform/config.go index f52347495..f9de53581 100644 --- a/platform/config.go +++ b/platform/config.go @@ -1,13 +1,16 @@ package platform import ( + "github.com/kelseyhightower/envconfig" + "github.com/tidepool-org/platform/client" "github.com/tidepool-org/platform/config" ) +// Config extends client.Config with additional properties. type Config struct { *client.Config - ServiceSecret string + ServiceSecret string `envconfig:"TIDEPOOL_SERVICE_SECRET"` // this should be overridden for loaders using envconfig } func NewConfig() *Config { @@ -16,12 +19,58 @@ func NewConfig() *Config { } } -func (c *Config) Load(configReporter config.Reporter) error { - if err := c.Config.Load(configReporter); err != nil { +func (c *Config) Load(loader ConfigLoader) error { + return loader.Load(c) +} + +// ConfigLoader abstracts the method by which config values are loaded. +type ConfigLoader interface { + Load(*Config) error +} + +// configReporterLoader adapts config.Reporter to implement ConfigLoader. +type configReporterLoader struct { + Reporter config.Reporter + client.ConfigLoader +} + +func NewConfigReporterLoader(reporter config.Reporter) *configReporterLoader { + return &configReporterLoader{ + ConfigLoader: client.NewConfigReporterLoader(reporter), + Reporter: reporter, + } +} + +// LoadPlatform implements ConfigLoader. +func (l *configReporterLoader) Load(cfg *Config) error { + if err := l.ConfigLoader.Load(cfg.Config); err != nil { return err } + cfg.ServiceSecret = l.Reporter.GetWithDefault("service_secret", cfg.ServiceSecret) + return nil +} - c.ServiceSecret = configReporter.GetWithDefault("service_secret", c.ServiceSecret) +// envconfigLoader adapts envconfig to implement ConfigLoader. +type envconfigLoader struct { + client.ConfigLoader +} - return nil +// NewEnvconfigLoader loads values via envconfig. +// +// If loader is nil, it defaults to envconfig for client values. +func NewEnvconfigLoader(loader client.ConfigLoader) *envconfigLoader { + if loader == nil { + loader = client.NewEnvconfigLoader() + } + return &envconfigLoader{ + ConfigLoader: loader, + } +} + +// Load implements ConfigLoader. +func (l *envconfigLoader) Load(cfg *Config) error { + if err := l.ConfigLoader.Load(cfg.Config); err != nil { + return err + } + return envconfig.Process(client.EnvconfigEmptyPrefix, cfg) } diff --git a/platform/config_test.go b/platform/config_test.go index 812692779..edba20a42 100644 --- a/platform/config_test.go +++ b/platform/config_test.go @@ -45,23 +45,21 @@ var _ = Describe("Config", func() { Context("Load", func() { var configReporter *configTest.Reporter + var loader platform.ConfigLoader BeforeEach(func() { configReporter = configTest.NewReporter() configReporter.Config["address"] = address configReporter.Config["user_agent"] = userAgent configReporter.Config["service_secret"] = serviceSecret - }) - - It("returns an error if config reporter is missing", func() { - Expect(cfg.Load(nil)).To(MatchError("config reporter is missing")) + loader = platform.NewConfigReporterLoader(configReporter) }) It("uses existing address if not set", func() { existingAddress := testHttp.NewAddress() cfg.Address = existingAddress delete(configReporter.Config, "address") - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Address).To(Equal(existingAddress)) Expect(cfg.UserAgent).To(Equal(userAgent)) Expect(cfg.ServiceSecret).To(Equal(serviceSecret)) @@ -71,7 +69,7 @@ var _ = Describe("Config", func() { existingUserAgent := testHttp.NewUserAgent() cfg.UserAgent = existingUserAgent delete(configReporter.Config, "user_agent") - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Config).ToNot(BeNil()) Expect(cfg.Address).To(Equal(address)) Expect(cfg.UserAgent).To(Equal(existingUserAgent)) @@ -82,14 +80,14 @@ var _ = Describe("Config", func() { existingServiceSecret := test.RandomStringFromRangeAndCharset(1, 64, test.CharsetText) cfg.ServiceSecret = existingServiceSecret delete(configReporter.Config, "service_secret") - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Address).To(Equal(address)) Expect(cfg.UserAgent).To(Equal(userAgent)) Expect(cfg.ServiceSecret).To(Equal(existingServiceSecret)) }) It("returns successfully and uses values from config reporter", func() { - Expect(cfg.Load(configReporter)).To(Succeed()) + Expect(cfg.Load(loader)).To(Succeed()) Expect(cfg.Config).ToNot(BeNil()) Expect(cfg.Address).To(Equal(address)) Expect(cfg.UserAgent).To(Equal(userAgent)) @@ -115,11 +113,6 @@ var _ = Describe("Config", func() { Expect(cfg.Validate()).To(MatchError("address is invalid")) }) - It("returns an error if the user agent is missing", func() { - cfg.UserAgent = "" - Expect(cfg.Validate()).To(MatchError("user agent is missing")) - }) - It("returns success", func() { Expect(cfg.Validate()).To(Succeed()) Expect(cfg.Config).ToNot(BeNil()) diff --git a/prescription/application/application.go b/prescription/application/application.go index 558f7fb0a..f7cfd7cbc 100644 --- a/prescription/application/application.go +++ b/prescription/application/application.go @@ -19,6 +19,7 @@ var Prescription = fx.Options( client.ProvideServiceName("prescription"), client.ExternalClientModule, fx.Provide( + client.ProvideExternalLoader, prescriptionMongo.NewStore, prescriptionMongo.NewStatusReporter, service.NewDeviceSettingsValidator, diff --git a/service/service/DEPRECATED_service.go b/service/service/DEPRECATED_service.go index c48a50f73..5240c8f40 100644 --- a/service/service/DEPRECATED_service.go +++ b/service/service/DEPRECATED_service.go @@ -67,7 +67,11 @@ func (d *DEPRECATEDService) initializeAuthClient() error { cfg := authClient.NewConfig() cfg.UserAgent = d.UserAgent() cfg.ExternalConfig.UserAgent = d.UserAgent() - if err := cfg.Load(d.ConfigReporter().WithScopes("auth", "client")); err != nil { + reporter := d.ConfigReporter().WithScopes("auth", "client") + ext := authClient.NewExternalConfigReporterLoader(reporter.WithScopes("external")) + plt := platform.NewConfigReporterLoader(reporter) + loader := authClient.NewConfigLoader(ext, plt) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load auth client config") } diff --git a/service/service/authenticated.go b/service/service/authenticated.go index fc003a89b..c5621d6df 100644 --- a/service/service/authenticated.go +++ b/service/service/authenticated.go @@ -37,7 +37,11 @@ func (a *Authenticated) initializeAuthClient() error { cfg := authClient.NewConfig() cfg.UserAgent = a.UserAgent() cfg.ExternalConfig.UserAgent = a.UserAgent() - if err := cfg.Load(a.ConfigReporter().WithScopes("auth", "client")); err != nil { + reporter := a.ConfigReporter().WithScopes("auth", "client") + ext := authClient.NewExternalConfigReporterLoader(reporter.WithScopes("external")) + plt := platform.NewConfigReporterLoader(reporter) + loader := authClient.NewConfigLoader(ext, plt) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load auth client config") } diff --git a/task/service/service/service.go b/task/service/service/service.go index abae9e3b7..874254535 100644 --- a/task/service/service/service.go +++ b/task/service/service/service.go @@ -168,7 +168,9 @@ func (s *Service) initializeDataClient() error { cfg := platform.NewConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("data", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("data", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load data client config") } @@ -195,7 +197,9 @@ func (s *Service) initializeDataSourceClient() error { cfg := platform.NewConfig() cfg.UserAgent = s.UserAgent() - if err := cfg.Load(s.ConfigReporter().WithScopes("data_source", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("data_source", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load data source client config") } @@ -228,7 +232,9 @@ func (s *Service) initializeDexcomClient() error { cfg := client.NewConfig() cfg.UserAgent = s.UserAgent() - if err = cfg.Load(s.ConfigReporter().WithScopes("dexcom", "client")); err != nil { + reporter := s.ConfigReporter().WithScopes("dexcom", "client") + loader := client.NewConfigReporterLoader(reporter) + if err = cfg.Load(loader); err != nil { return errors.Wrap(err, "unable to load dexcom client config") } diff --git a/user/client/client.go b/user/client/client.go index a516dc0ed..8fab73a28 100644 --- a/user/client/client.go +++ b/user/client/client.go @@ -34,7 +34,9 @@ func NewDefaultClient(p Params) (user.Client, error) { cfg := platform.NewConfig() cfg.UserAgent = p.UserAgent - if err := cfg.Load(p.ConfigReporter.WithScopes("user", "client")); err != nil { + reporter := p.ConfigReporter.WithScopes("user", "client") + loader := platform.NewConfigReporterLoader(reporter) + if err := cfg.Load(loader); err != nil { return nil, errors.Wrap(err, "unable to get user client config") }