From bbb15dcb85ad0713722850676cde40355a5a2a20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 30 Jun 2020 17:18:54 +0200 Subject: [PATCH] Introduce Options for the httpclient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/.keep | 0 .../make-httpclient-configurable.md | 5 ++ cmd/reva/download.go | 9 ++- cmd/reva/upload.go | 9 ++- examples/meshdirectory/meshdirectory.toml | 2 + .../http/services/datagateway/datagateway.go | 27 ++++++- .../services/dataprovider/dataprovider.go | 2 + internal/http/services/dataprovider/put.go | 7 +- internal/http/services/owncloud/ocdav/copy.go | 23 ++++-- internal/http/services/owncloud/ocdav/get.go | 6 +- .../http/services/owncloud/ocdav/ocdav.go | 2 + internal/http/services/owncloud/ocdav/put.go | 6 +- internal/http/services/owncloud/ocdav/tus.go | 6 +- pkg/auth/manager/oidc/oidc.go | 20 ++--- pkg/meshdirectory/manager/mentix/mentix.go | 13 +++- pkg/rhttp/client.go | 11 +-- pkg/rhttp/option.go | 74 +++++++++++++++++++ pkg/user/manager/rest/rest.go | 4 +- 18 files changed, 186 insertions(+), 40 deletions(-) create mode 100644 changelog/unreleased/.keep create mode 100644 changelog/unreleased/make-httpclient-configurable.md create mode 100644 pkg/rhttp/option.go diff --git a/changelog/unreleased/.keep b/changelog/unreleased/.keep new file mode 100644 index 00000000000..e69de29bb2d diff --git a/changelog/unreleased/make-httpclient-configurable.md b/changelog/unreleased/make-httpclient-configurable.md new file mode 100644 index 00000000000..fd83b1c494e --- /dev/null +++ b/changelog/unreleased/make-httpclient-configurable.md @@ -0,0 +1,5 @@ +Enhancement: Make httpclient configurable + +- Introduce Options for the httpclient (#914) + +https://github.com/cs3org/reva/pull/914 diff --git a/cmd/reva/download.go b/cmd/reva/download.go index d4d4a05f6e0..54533ed9648 100644 --- a/cmd/reva/download.go +++ b/cmd/reva/download.go @@ -23,6 +23,7 @@ import ( "io" "net/http" "os" + "time" "github.com/cs3org/reva/internal/http/services/datagateway" @@ -92,7 +93,13 @@ func downloadCommand() *command { } httpReq.Header.Set(datagateway.TokenTransportHeader, res.Token) - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + // TODO make insecure configurable + rhttp.Insecure(true), + // TODO make timeout configurable + rhttp.Timeout(time.Duration(24*int64(time.Hour))), + ) httpRes, err := httpClient.Do(httpReq) if err != nil { diff --git a/cmd/reva/upload.go b/cmd/reva/upload.go index d5ca0eb17f7..145f4cf0118 100644 --- a/cmd/reva/upload.go +++ b/cmd/reva/upload.go @@ -25,6 +25,7 @@ import ( "os" "path/filepath" "strconv" + "time" "github.com/cs3org/reva/internal/http/services/datagateway" @@ -129,7 +130,13 @@ func uploadCommand() *command { // create the tus client. c := tus.DefaultConfig() c.Resume = true - c.HttpClient = rhttp.GetHTTPClient(ctx) + c.HttpClient = rhttp.GetHTTPClient( + rhttp.Context(ctx), + // TODO make insecure configurable + rhttp.Insecure(true), + // TODO make timeout configurable + rhttp.Timeout(time.Duration(24*int64(time.Hour))), + ) c.Store, err = memorystore.NewMemoryStore() if err != nil { return err diff --git a/examples/meshdirectory/meshdirectory.toml b/examples/meshdirectory/meshdirectory.toml index c14f9b7fe3f..a56222e568c 100644 --- a/examples/meshdirectory/meshdirectory.toml +++ b/examples/meshdirectory/meshdirectory.toml @@ -13,3 +13,5 @@ driver = "mentix" # #[http.services.meshdirectory.drivers.mentix] #url = "http://localhost:9600/" +#insecure = true +#timeout = 10 diff --git a/internal/http/services/datagateway/datagateway.go b/internal/http/services/datagateway/datagateway.go index d9d42e1abf4..6dea7a3f8e8 100644 --- a/internal/http/services/datagateway/datagateway.go +++ b/internal/http/services/datagateway/datagateway.go @@ -24,6 +24,7 @@ import ( "net/http" "net/url" "path" + "time" "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/errtypes" @@ -53,6 +54,8 @@ type transferClaims struct { type config struct { Prefix string `mapstructure:"prefix"` TransferSharedSecret string `mapstructure:"transfer_shared_secret"` + Timeout int64 `mapstructure:"timeout"` + Insecure bool `mapstructure:"insecure"` } func (c *config) init() { @@ -167,7 +170,11 @@ func (s *svc) doHead(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), + rhttp.Insecure(s.conf.Insecure), + ) httpReq, err := rhttp.NewRequest(ctx, "HEAD", claims.Target, nil) if err != nil { log.Err(err).Msg("wrong request") @@ -205,7 +212,11 @@ func (s *svc) doGet(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), + rhttp.Insecure(s.conf.Insecure), + ) httpReq, err := rhttp.NewRequest(ctx, "GET", claims.Target, nil) if err != nil { log.Err(err).Msg("wrong request") @@ -259,7 +270,11 @@ func (s *svc) doPut(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), + rhttp.Insecure(s.conf.Insecure), + ) httpReq, err := rhttp.NewRequest(ctx, "PUT", target, r.Body) if err != nil { log.Err(err).Msg("wrong request") @@ -314,7 +329,11 @@ func (s *svc) doPatch(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), + rhttp.Insecure(s.conf.Insecure), + ) httpReq, err := rhttp.NewRequest(ctx, "PATCH", target, r.Body) if err != nil { log.Err(err).Msg("wrong request") diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go index ec8b1723b51..8ba4bc7e28a 100644 --- a/internal/http/services/dataprovider/dataprovider.go +++ b/internal/http/services/dataprovider/dataprovider.go @@ -39,6 +39,8 @@ type config struct { Prefix string `mapstructure:"prefix" docs:"data;The prefix to be used for this HTTP service"` Driver string `mapstructure:"driver" docs:"localhome;The storage driver to be used."` Drivers map[string]map[string]interface{} `mapstructure:"drivers" docs:"url:docs/config/packages/storage/fs;The configuration for the storage driver"` + Timeout int64 `mapstructure:"timeout"` + Insecure bool `mapstructure:"insecure"` DisableTus bool `mapstructure:"disable_tus" docs:"false;Whether to disable TUS uploads."` } diff --git a/internal/http/services/dataprovider/put.go b/internal/http/services/dataprovider/put.go index 6471b475d58..5dc93e57c87 100644 --- a/internal/http/services/dataprovider/put.go +++ b/internal/http/services/dataprovider/put.go @@ -24,6 +24,7 @@ import ( "path" "strconv" "strings" + "time" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/pkg/appctx" @@ -73,7 +74,11 @@ func (s *svc) doTusPut(w http.ResponseWriter, r *http.Request) { // create the tus client. c := tus.DefaultConfig() c.Resume = true - c.HttpClient = rhttp.GetHTTPClient(ctx) + c.HttpClient = rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), + rhttp.Insecure(s.conf.Insecure), + ) c.Store, err = memorystore.NewMemoryStore() if err != nil { w.WriteHeader(http.StatusInternalServerError) diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index 82d51d93dd8..db12c31c513 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -26,6 +26,7 @@ import ( "net/url" "path" "strings" + "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" @@ -169,7 +170,7 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { // TODO what if intermediate is a file? } - err = descend(ctx, client, srcStatRes.Info, dst, depth == "infinity") + err = s.descend(ctx, client, srcStatRes.Info, dst, depth == "infinity") if err != nil { log.Error().Err(err).Msg("error descending directory") w.WriteHeader(http.StatusInternalServerError) @@ -178,7 +179,7 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { w.WriteHeader(successCode) } -func descend(ctx context.Context, client gateway.GatewayAPIClient, src *provider.ResourceInfo, dst string, recurse bool) error { +func (s *svc) descend(ctx context.Context, client gateway.GatewayAPIClient, src *provider.ResourceInfo, dst string, recurse bool) error { log := appctx.GetLogger(ctx) log.Debug().Str("src", src.Path).Str("dst", dst).Msg("descending") if src.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER { @@ -215,7 +216,7 @@ func descend(ctx context.Context, client gateway.GatewayAPIClient, src *provider for i := range res.Infos { childDst := path.Join(dst, path.Base(res.Infos[i].Path)) - err := descend(ctx, client, res.Infos[i], childDst, recurse) + err := s.descend(ctx, client, res.Infos[i], childDst, recurse) if err != nil { return err } @@ -274,7 +275,11 @@ func descend(ctx context.Context, client gateway.GatewayAPIClient, src *provider } httpDownloadReq.Header.Set(datagateway.TokenTransportHeader, dRes.Token) - httpDownloadClient := rhttp.GetHTTPClient(ctx) + httpDownloadClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), + rhttp.Insecure(s.c.Insecure), + ) httpDownloadRes, err := httpDownloadClient.Do(httpDownloadReq) if err != nil { @@ -287,7 +292,7 @@ func descend(ctx context.Context, client gateway.GatewayAPIClient, src *provider } // do upload - err = tusUpload(ctx, uRes.UploadEndpoint, uRes.Token, dst, httpDownloadRes.Body, src.GetSize()) + err = s.tusUpload(ctx, uRes.UploadEndpoint, uRes.Token, dst, httpDownloadRes.Body, src.GetSize()) if err != nil { return err } @@ -295,14 +300,18 @@ func descend(ctx context.Context, client gateway.GatewayAPIClient, src *provider return nil } -func tusUpload(ctx context.Context, dataServerURL string, transferToken string, fn string, body io.Reader, length uint64) error { +func (s *svc) tusUpload(ctx context.Context, dataServerURL string, transferToken string, fn string, body io.Reader, length uint64) error { var err error log := appctx.GetLogger(ctx) // create the tus client. c := tus.DefaultConfig() c.Resume = true - c.HttpClient = rhttp.GetHTTPClient(ctx) + c.HttpClient = rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), + rhttp.Insecure(s.c.Insecure), + ) c.Store, err = memorystore.NewMemoryStore() if err != nil { return err diff --git a/internal/http/services/owncloud/ocdav/get.go b/internal/http/services/owncloud/ocdav/get.go index 6afe39d5031..449c12ac7c0 100644 --- a/internal/http/services/owncloud/ocdav/get.go +++ b/internal/http/services/owncloud/ocdav/get.go @@ -106,7 +106,11 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) { return } httpReq.Header.Set(datagateway.TokenTransportHeader, dRes.Token) - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), + rhttp.Insecure(s.c.Insecure), + ) httpRes, err := httpClient.Do(httpReq) if err != nil { diff --git a/internal/http/services/owncloud/ocdav/ocdav.go b/internal/http/services/owncloud/ocdav/ocdav.go index 00f647c7457..caf97878303 100644 --- a/internal/http/services/owncloud/ocdav/ocdav.go +++ b/internal/http/services/owncloud/ocdav/ocdav.go @@ -65,6 +65,8 @@ type Config struct { WebdavNamespace string `mapstructure:"webdav_namespace"` ChunkFolder string `mapstructure:"chunk_folder"` GatewaySvc string `mapstructure:"gatewaysvc"` + Timeout int64 `mapstructure:"timeout"` + Insecure bool `mapstructure:"insecure"` DisableTus bool `mapstructure:"disable_tus"` } diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go index 979b19e81ee..24d8eb576e1 100644 --- a/internal/http/services/owncloud/ocdav/put.go +++ b/internal/http/services/owncloud/ocdav/put.go @@ -253,7 +253,11 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) { // create the tus client. c := tus.DefaultConfig() c.Resume = true - c.HttpClient = rhttp.GetHTTPClient(ctx) + c.HttpClient = rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), + rhttp.Insecure(s.c.Insecure), + ) c.Store, err = memorystore.NewMemoryStore() if err != nil { w.WriteHeader(http.StatusInternalServerError) diff --git a/internal/http/services/owncloud/ocdav/tus.go b/internal/http/services/owncloud/ocdav/tus.go index 2c322e0f290..b4d6d69cf8d 100644 --- a/internal/http/services/owncloud/ocdav/tus.go +++ b/internal/http/services/owncloud/ocdav/tus.go @@ -167,7 +167,11 @@ func (s *svc) handleTusPost(w http.ResponseWriter, r *http.Request, ns string) { // TODO check this really streams if r.Header.Get("Content-Type") == "application/offset+octet-stream" { - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), + rhttp.Insecure(s.c.Insecure), + ) httpReq, err := rhttp.NewRequest(ctx, "PATCH", uRes.UploadEndpoint, r.Body) if err != nil { log.Err(err).Msg("wrong request") diff --git a/pkg/auth/manager/oidc/oidc.go b/pkg/auth/manager/oidc/oidc.go index 19a7fef8a14..ab6941cc18a 100644 --- a/pkg/auth/manager/oidc/oidc.go +++ b/pkg/auth/manager/oidc/oidc.go @@ -22,15 +22,14 @@ package oidc import ( "context" - "crypto/tls" "fmt" - "net/http" "time" oidc "github.com/coreos/go-oidc" user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/cs3org/reva/pkg/auth" "github.com/cs3org/reva/pkg/auth/manager/registry" + "github.com/cs3org/reva/pkg/rhttp" "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "github.com/rs/zerolog/log" @@ -143,18 +142,13 @@ func (am *mgr) Authenticate(ctx context.Context, clientID, clientSecret string) func (am *mgr) getOAuthCtx(ctx context.Context) context.Context { // Sometimes for testing we need to skip the TLS check, that's why we need a // custom HTTP client. - tr := &http.Transport{ - TLSClientConfig: &tls.Config{ - InsecureSkipVerify: am.c.Insecure, - }, + customHTTPClient := rhttp.GetHTTPClient( + rhttp.Context(ctx), + rhttp.Timeout(time.Second*10), + rhttp.Insecure(am.c.Insecure), // Fixes connection fd leak which might be caused by provider-caching - DisableKeepAlives: true, - } - - customHTTPClient := &http.Client{ - Transport: tr, - Timeout: time.Second * 10, - } + rhttp.DisableKeepAlive(true), + ) ctx = context.WithValue(ctx, oauth2.HTTPClient, customHTTPClient) return ctx } diff --git a/pkg/meshdirectory/manager/mentix/mentix.go b/pkg/meshdirectory/manager/mentix/mentix.go index 8da3a5c4294..f1003f4770d 100644 --- a/pkg/meshdirectory/manager/mentix/mentix.go +++ b/pkg/meshdirectory/manager/mentix/mentix.go @@ -23,6 +23,7 @@ import ( "encoding/json" "fmt" "net/http" + "time" "github.com/cs3org/reva/pkg/meshdirectory" "github.com/cs3org/reva/pkg/meshdirectory/manager/registry" @@ -55,8 +56,12 @@ func New(m map[string]interface{}) (meshdirectory.Manager, error) { client := &Client{ BaseURL: c.URL, - // TODO: pass/create context once it is required by GetHTTPClient - HTTPClient: rhttp.GetHTTPClient(context.TODO()), + HTTPClient: rhttp.GetHTTPClient( + // TODO: pass/create context once it is required by GetHTTPClient + rhttp.Context(context.TODO()), + rhttp.Insecure(c.Insecure), + rhttp.Timeout(time.Duration(c.Timeout*int64(time.Second))), + ), } mgr := &mgr{ @@ -68,7 +73,9 @@ func New(m map[string]interface{}) (meshdirectory.Manager, error) { } type config struct { - URL string `mapstructure:"url"` + URL string `mapstructure:"url"` + Timeout int64 `mapstructure:"timeout"` + Insecure bool `mapstructure:"insecure"` } type mgr struct { diff --git a/pkg/rhttp/client.go b/pkg/rhttp/client.go index 4f41b0087b4..02ff2828ee0 100644 --- a/pkg/rhttp/client.go +++ b/pkg/rhttp/client.go @@ -23,7 +23,6 @@ import ( "crypto/tls" "io" "net/http" - "time" "github.com/cs3org/reva/pkg/token" "github.com/pkg/errors" @@ -33,15 +32,17 @@ import ( // GetHTTPClient returns an http client with open census tracing support. // TODO(labkode): harden it. // https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779 -func GetHTTPClient(ctx context.Context) *http.Client { +func GetHTTPClient(opts ...Option) *http.Client { + options := newOptions(opts...) + httpClient := &http.Client{ - Timeout: time.Second * 10, + Timeout: options.Timeout, Transport: &ochttp.Transport{ Base: &http.Transport{ - // TODO: make TLS config configurable TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, + InsecureSkipVerify: options.Insecure, }, + DisableKeepAlives: options.DisableKeepAlive, }, }, } diff --git a/pkg/rhttp/option.go b/pkg/rhttp/option.go new file mode 100644 index 00000000000..b195741c1c5 --- /dev/null +++ b/pkg/rhttp/option.go @@ -0,0 +1,74 @@ +// Copyright 2018-2020 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package rhttp + +import ( + "context" + "time" +) + +// Option defines a single option function. +type Option func(o *Options) + +// Options defines the available options for this package. +type Options struct { + Context context.Context + Timeout time.Duration + Insecure bool + DisableKeepAlive bool +} + +// newOptions initializes the available default options. +func newOptions(opts ...Option) Options { + opt := Options{} + + for _, o := range opts { + o(&opt) + } + + return opt +} + +// Context provides a function to set the context option. +func Context(val context.Context) Option { + return func(o *Options) { + o.Context = val + } +} + +// Insecure provides a function to set the insecure option. +func Insecure(insecure bool) Option { + return func(o *Options) { + o.Insecure = insecure + } +} + +// Timeout provides a function to set the timeout option. +func Timeout(t time.Duration) Option { + return func(o *Options) { + o.Timeout = t + } +} + +// DisableKeepAlive provides a function to set the disablee keep alive option. +func DisableKeepAlive(disable bool) Option { + return func(o *Options) { + o.DisableKeepAlive = disable + } +} diff --git a/pkg/user/manager/rest/rest.go b/pkg/user/manager/rest/rest.go index 4a92fc9d2eb..f759e565f9d 100644 --- a/pkg/user/manager/rest/rest.go +++ b/pkg/user/manager/rest/rest.go @@ -151,7 +151,7 @@ func (m *manager) getAPIToken(ctx context.Context) (string, time.Time, error) { "audience": {m.conf.TargetAPI}, } - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient(rhttp.Context(ctx), rhttp.Timeout(10*time.Second), rhttp.Insecure(true)) httpReq, err := rhttp.NewRequest(ctx, "POST", m.conf.OIDCTokenEndpoint, strings.NewReader(params.Encode())) if err != nil { return "", time.Time{}, err @@ -186,7 +186,7 @@ func (m *manager) sendAPIRequest(ctx context.Context, url string) ([]interface{} return nil, err } - httpClient := rhttp.GetHTTPClient(ctx) + httpClient := rhttp.GetHTTPClient(rhttp.Context(ctx), rhttp.Timeout(10*time.Second), rhttp.Insecure(true)) httpReq, err := rhttp.NewRequest(ctx, "GET", url, nil) if err != nil { return nil, err