From 40e5026b263e1fddd882ff2a6c0f707f6d6e7ea0 Mon Sep 17 00:00:00 2001 From: Joshua Hoblitt Date: Tue, 22 Oct 2024 15:09:04 -0700 Subject: [PATCH] object: add httpClient param to object.NewS3Agent() To allow the caller to pass in their own transport when testing. Signed-off-by: Joshua Hoblitt (cherry picked from commit 57b7eeec800bc008507b7e2ff5b47d50d4503ece) --- .../ceph/object/bucket/provisioner.go | 13 ++------ .../ceph/object/notification/provisioner.go | 5 +-- pkg/operator/ceph/object/s3-handlers.go | 25 +++++++-------- pkg/operator/ceph/object/s3-handlers_test.go | 32 ++++++++++++++++--- tests/framework/clients/bucket.go | 6 +--- .../ceph_bucket_notification_test.go | 14 +++----- tests/integration/ceph_object_test.go | 14 +++----- 7 files changed, 50 insertions(+), 59 deletions(-) diff --git a/pkg/operator/ceph/object/bucket/provisioner.go b/pkg/operator/ceph/object/bucket/provisioner.go index 4e3dec79f..eba551186 100644 --- a/pkg/operator/ceph/object/bucket/provisioner.go +++ b/pkg/operator/ceph/object/bucket/provisioner.go @@ -735,15 +735,6 @@ func (p *Provisioner) setAdminOpsAPIClient() error { func (p *Provisioner) setS3Agent() error { var err error - - if p.insecureTLS { - p.s3Agent, err = object.NewInsecureS3Agent(p.accessKeyID, p.secretAccessKey, p.getObjectStoreEndpoint(), logger.LevelAt(capnslog.DEBUG)) - } else { - p.s3Agent, err = object.NewS3Agent(p.accessKeyID, p.secretAccessKey, p.getObjectStoreEndpoint(), logger.LevelAt(capnslog.DEBUG), p.tlsCert) - } - if err != nil { - return err - } - - return nil + p.s3Agent, err = object.NewS3Agent(p.accessKeyID, p.secretAccessKey, p.getObjectStoreEndpoint(), logger.LevelAt(capnslog.DEBUG), p.tlsCert, p.insecureTLS, nil) + return err } diff --git a/pkg/operator/ceph/object/notification/provisioner.go b/pkg/operator/ceph/object/notification/provisioner.go index dc1579fa0..47d0d37e5 100644 --- a/pkg/operator/ceph/object/notification/provisioner.go +++ b/pkg/operator/ceph/object/notification/provisioner.go @@ -91,10 +91,7 @@ func newS3Agent(p provisioner) (*object.S3Agent, error) { } } - if insecureTLS { - return object.NewInsecureS3Agent(accessKey, secretKey, objContext.Endpoint, logger.LevelAt(capnslog.DEBUG)) - } - return object.NewS3Agent(accessKey, secretKey, objContext.Endpoint, logger.LevelAt(capnslog.DEBUG), tlsCert) + return object.NewS3Agent(accessKey, secretKey, objContext.Endpoint, logger.LevelAt(capnslog.DEBUG), tlsCert, insecureTLS, nil) } // TODO: convert all rules without restrictions once the AWS SDK supports that diff --git a/pkg/operator/ceph/object/s3-handlers.go b/pkg/operator/ceph/object/s3-handlers.go index 13eccb826..6bc510ef0 100644 --- a/pkg/operator/ceph/object/s3-handlers.go +++ b/pkg/operator/ceph/object/s3-handlers.go @@ -39,27 +39,24 @@ type S3Agent struct { Client *s3.S3 } -func NewS3Agent(accessKey, secretKey, endpoint string, debug bool, tlsCert []byte) (*S3Agent, error) { - return newS3Agent(accessKey, secretKey, endpoint, debug, tlsCert, false) -} - -func NewInsecureS3Agent(accessKey, secretKey, endpoint string, debug bool) (*S3Agent, error) { - return newS3Agent(accessKey, secretKey, endpoint, debug, nil, true) -} - -func newS3Agent(accessKey, secretKey, endpoint string, debug bool, tlsCert []byte, insecure bool) (*S3Agent, error) { +func NewS3Agent(accessKey, secretKey, endpoint string, debug bool, tlsCert []byte, insecure bool, httpClient *http.Client) (*S3Agent, error) { logLevel := aws.LogOff if debug { logLevel = aws.LogDebug } - client := http.Client{ - Timeout: HttpTimeOut, - } tlsEnabled := false if len(tlsCert) > 0 || insecure { tlsEnabled = true - client.Transport = BuildTransportTLS(tlsCert, insecure) } + if httpClient == nil { + httpClient = &http.Client{ + Timeout: HttpTimeOut, + } + if tlsEnabled { + httpClient.Transport = BuildTransportTLS(tlsCert, insecure) + } + } + session, err := awssession.NewSession( aws.NewConfig(). WithRegion(CephRegion). @@ -68,7 +65,7 @@ func newS3Agent(accessKey, secretKey, endpoint string, debug bool, tlsCert []byt WithS3ForcePathStyle(true). WithMaxRetries(5). WithDisableSSL(!tlsEnabled). - WithHTTPClient(&client). + WithHTTPClient(httpClient). WithLogLevel(logLevel), ) if err != nil { diff --git a/pkg/operator/ceph/object/s3-handlers_test.go b/pkg/operator/ceph/object/s3-handlers_test.go index f7f20fe0c..628dea1e5 100644 --- a/pkg/operator/ceph/object/s3-handlers_test.go +++ b/pkg/operator/ceph/object/s3-handlers_test.go @@ -32,7 +32,7 @@ func TestNewS3Agent(t *testing.T) { t.Run("test without tls/debug", func(t *testing.T) { debug := false insecure := false - s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure) + s3Agent, err := NewS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure, nil) assert.NoError(t, err) assert.NotEqual(t, aws.LogDebug, s3Agent.Client.Config.LogLevel) assert.Equal(t, nil, s3Agent.Client.Config.HTTPClient.Transport) @@ -42,7 +42,7 @@ func TestNewS3Agent(t *testing.T) { debug := true logLevel := aws.LogDebug insecure := false - s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure) + s3Agent, err := NewS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure, nil) assert.NoError(t, err) assert.Equal(t, &logLevel, s3Agent.Client.Config.LogLevel) assert.Nil(t, s3Agent.Client.Config.HTTPClient.Transport) @@ -51,7 +51,7 @@ func TestNewS3Agent(t *testing.T) { t.Run("test without tls client cert but insecure tls", func(t *testing.T) { debug := true insecure := true - s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure) + s3Agent, err := NewS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure, nil) assert.NoError(t, err) assert.NotNil(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.RootCAs) // still includes sys certs assert.True(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.InsecureSkipVerify) @@ -61,7 +61,7 @@ func TestNewS3Agent(t *testing.T) { debug := true insecure := false tlsCert := []byte("tlsCert") - s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, debug, tlsCert, insecure) + s3Agent, err := NewS3Agent(accessKey, secretKey, endpoint, debug, tlsCert, insecure, nil) assert.NoError(t, err) assert.NotNil(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.RootCAs) assert.False(t, *s3Agent.Client.Config.DisableSSL) @@ -70,10 +70,32 @@ func TestNewS3Agent(t *testing.T) { debug := true insecure := true tlsCert := []byte("tlsCert") - s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, debug, tlsCert, insecure) + s3Agent, err := NewS3Agent(accessKey, secretKey, endpoint, debug, tlsCert, insecure, nil) assert.NoError(t, err) assert.NotNil(t, s3Agent.Client.Config.HTTPClient.Transport) assert.True(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.InsecureSkipVerify) assert.False(t, *s3Agent.Client.Config.DisableSSL) }) + t.Run("test with custom http.Client", func(t *testing.T) { + debug := true + logLevel := aws.LogDebug + insecure := false + httpClient := &http.Client{ + Transport: &http.Transport{ + // set some values to check if they are passed through + MaxIdleConns: 7, + MaxIdleConnsPerHost: 13, + MaxConnsPerHost: 17, + }, + } + s3Agent, err := NewS3Agent(accessKey, secretKey, endpoint, debug, nil, insecure, httpClient) + assert.NoError(t, err) + assert.Equal(t, &logLevel, s3Agent.Client.Config.LogLevel) + assert.NotNil(t, s3Agent.Client.Config.HTTPClient.Transport) + assert.True(t, *s3Agent.Client.Config.DisableSSL) + transport := s3Agent.Client.Config.HTTPClient.Transport + assert.Equal(t, 7, transport.(*http.Transport).MaxIdleConns) + assert.Equal(t, 13, transport.(*http.Transport).MaxIdleConnsPerHost) + assert.Equal(t, 17, transport.(*http.Transport).MaxConnsPerHost) + }) } diff --git a/tests/framework/clients/bucket.go b/tests/framework/clients/bucket.go index 6b86aca37..fd0216d7e 100644 --- a/tests/framework/clients/bucket.go +++ b/tests/framework/clients/bucket.go @@ -157,11 +157,7 @@ func (b *BucketOperation) CheckBucketNotificationSetonRGW(namespace, storeName, s3endpoint, _ := helper.ObjectClient.GetEndPointUrl(namespace, storeName) s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName) s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName) - if tlsEnabled { - s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true) - } else { - s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil) - } + s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil, tlsEnabled, nil) if err != nil { logger.Infof("failed to s3client due to %v", err) return false diff --git a/tests/integration/ceph_bucket_notification_test.go b/tests/integration/ceph_bucket_notification_test.go index d8330ec99..8e0bdfdfa 100644 --- a/tests/integration/ceph_bucket_notification_test.go +++ b/tests/integration/ceph_bucket_notification_test.go @@ -123,11 +123,8 @@ func testBucketNotifications(s *suite.Suite, helper *clients.TestClient, k8sh *u s3endpoint, _ := helper.ObjectClient.GetEndPointUrl(namespace, storeName) s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName) s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName) - if objectStore.Spec.IsTLSEnabled() { - s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true) - } else { - s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil) - } + insecure := objectStore.Spec.IsTLSEnabled() + s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil, insecure, nil) assert.Nil(t, err) logger.Infof("endpoint (%s) Accesskey (%s) secret (%s)", s3endpoint, s3AccessKey, s3SecretKey) @@ -227,11 +224,8 @@ func testBucketNotifications(s *suite.Suite, helper *clients.TestClient, k8sh *u s3endpoint, _ := helper.ObjectClient.GetEndPointUrl(namespace, storeName) s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName) s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName) - if objectStore.Spec.IsTLSEnabled() { - s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true) - } else { - s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil) - } + insecure := objectStore.Spec.IsTLSEnabled() + s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil, insecure, nil) assert.Nil(t, err) logger.Infof("endpoint (%s) Accesskey (%s) secret (%s)", s3endpoint, s3AccessKey, s3SecretKey) diff --git a/tests/integration/ceph_object_test.go b/tests/integration/ceph_object_test.go index 3763a3c1e..8ef593ca4 100644 --- a/tests/integration/ceph_object_test.go +++ b/tests/integration/ceph_object_test.go @@ -218,11 +218,8 @@ func testObjectStoreOperations(s *suite.Suite, helper *clients.TestClient, k8sh s3endpoint, _ := helper.ObjectClient.GetEndPointUrl(namespace, storeName) s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName) s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName) - if objectStore.Spec.IsTLSEnabled() { - s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true) - } else { - s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil) - } + insecure := objectStore.Spec.IsTLSEnabled() + s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil, insecure, nil) assert.Nil(t, err) logger.Infof("endpoint (%s) Accesskey (%s) secret (%s)", s3endpoint, s3AccessKey, s3SecretKey) @@ -334,11 +331,8 @@ func testObjectStoreOperations(s *suite.Suite, helper *clients.TestClient, k8sh s3AccessKey := string(secret.Data["AWS_ACCESS_KEY_ID"]) s3SecretKey := string(secret.Data["AWS_SECRET_ACCESS_KEY"]) - if objectStore.Spec.IsTLSEnabled() { - s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true) - } else { - s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil) - } + insecure := objectStore.Spec.IsTLSEnabled() + s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, true, nil, insecure, nil) assert.Nil(t, err) logger.Infof("endpoint (%s) Accesskey (%s) secret (%s)", s3endpoint, s3AccessKey, s3SecretKey) })