diff --git a/azurebs/client/storage_client.go b/azurebs/client/storage_client.go index 0590df00..c0d57b77 100644 --- a/azurebs/client/storage_client.go +++ b/azurebs/client/storage_client.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "log/slog" + "net/http" "os" "strconv" "strings" @@ -107,6 +108,7 @@ type DefaultStorageClient struct { credential *azblob.SharedKeyCredential serviceURL string storageConfig config.AZStorageConfig + clientOptions *azcore.ClientOptions } func NewStorageClient(storageConfig config.AZStorageConfig) (StorageClient, error) { @@ -115,9 +117,64 @@ func NewStorageClient(storageConfig config.AZStorageConfig) (StorageClient, erro return nil, err } + clientOptions, err := buildClientOptions(storageConfig) + if err != nil { + return nil, err + } + serviceURL := fmt.Sprintf("https://%s.%s/%s", storageConfig.AccountName, storageConfig.StorageEndpoint(), storageConfig.ContainerName) - return DefaultStorageClient{credential: credential, serviceURL: serviceURL, storageConfig: storageConfig}, nil + return DefaultStorageClient{credential: credential, serviceURL: serviceURL, storageConfig: storageConfig, clientOptions: clientOptions}, nil +} + +// buildClientOptions builds the shared azcore client options carrying a custom +// *http.Client whenever an HTTP request timeout and/or response header timeout is +// configured. It returns nil when neither is set, preserving the SDK defaults. +func buildClientOptions(storageConfig config.AZStorageConfig) (*azcore.ClientOptions, error) { + httpRequestTimeout, err := storageConfig.HTTPRequestTimeoutValue() + if err != nil { + return nil, err + } + + responseHeaderTimeout, err := storageConfig.HTTPResponseHeaderTimeoutValue() + if err != nil { + return nil, err + } + + if httpRequestTimeout == 0 && responseHeaderTimeout == 0 { + return nil, nil + } + + // preserve the default transport settings from the azure-sdk-for-go runtime package + transport := http.DefaultTransport.(*http.Transport).Clone() + if responseHeaderTimeout > 0 { + transport.ResponseHeaderTimeout = responseHeaderTimeout + } + + httpClient := &http.Client{Timeout: httpRequestTimeout, Transport: transport} + + return &azcore.ClientOptions{Transport: httpClient}, nil +} + +func (dsc DefaultStorageClient) blockblobOptions() *blockblob.ClientOptions { + if dsc.clientOptions == nil { + return nil + } + return &blockblob.ClientOptions{ClientOptions: *dsc.clientOptions} +} + +func (dsc DefaultStorageClient) blobOptions() *azBlob.ClientOptions { + if dsc.clientOptions == nil { + return nil + } + return &azBlob.ClientOptions{ClientOptions: *dsc.clientOptions} +} + +func (dsc DefaultStorageClient) containerOptions() *azContainer.ClientOptions { + if dsc.clientOptions == nil { + return nil + } + return &azContainer.ClientOptions{ClientOptions: *dsc.clientOptions} } func (dsc DefaultStorageClient) Upload( @@ -138,7 +195,7 @@ func (dsc DefaultStorageClient) Upload( } defer cancel() - client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return nil, err } @@ -173,7 +230,7 @@ func (dsc DefaultStorageClient) UploadStream( } defer cancel() - client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return err } @@ -196,7 +253,7 @@ func (dsc DefaultStorageClient) Download( ) error { blobURL := fmt.Sprintf("%s/%s", dsc.serviceURL, source) slog.Info("Downloading blob from container", "container", dsc.storageConfig.ContainerName, "blob", source, "local_file", dest.Name()) - client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return err } @@ -226,7 +283,7 @@ func (dsc DefaultStorageClient) Copy( srcURL := fmt.Sprintf("%s/%s", dsc.serviceURL, srcBlob) destURL := fmt.Sprintf("%s/%s", dsc.serviceURL, destBlob) - destClient, err := blockblob.NewClientWithSharedKeyCredential(destURL, dsc.credential, nil) + destClient, err := blockblob.NewClientWithSharedKeyCredential(destURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return fmt.Errorf("failed to create destination client: %w", err) } @@ -268,7 +325,7 @@ func (dsc DefaultStorageClient) Delete( blobURL := fmt.Sprintf("%s/%s", dsc.serviceURL, dest) slog.Info("Deleting blob from container", "container", dsc.storageConfig.ContainerName, "blob", dest, "url", blobURL) - client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return err } @@ -295,7 +352,7 @@ func (dsc DefaultStorageClient) DeleteRecursive( slog.Info("Deleting all blobs in container", "container", dsc.storageConfig.ContainerName) } - containerClient, err := azContainer.NewClientWithSharedKeyCredential(dsc.serviceURL, dsc.credential, nil) + containerClient, err := azContainer.NewClientWithSharedKeyCredential(dsc.serviceURL, dsc.credential, dsc.containerOptions()) if err != nil { return fmt.Errorf("failed to create container client: %w", err) } @@ -315,7 +372,7 @@ func (dsc DefaultStorageClient) DeleteRecursive( for _, blob := range resp.Segment.BlobItems { blobURL := fmt.Sprintf("%s/%s", dsc.serviceURL, *blob.Name) - blobClient, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + blobClient, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { slog.Error("Failed to create blob client", "blob", *blob.Name, "error", err) continue @@ -338,7 +395,7 @@ func (dsc DefaultStorageClient) Exists( blobURL := fmt.Sprintf("%s/%s", dsc.serviceURL, dest) slog.Info("Checking if blob exists", "container", dsc.storageConfig.ContainerName, "blob", dest, "url", blobURL) - client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return false, err } @@ -365,7 +422,7 @@ func (dsc DefaultStorageClient) SignedUrl( blobURL := fmt.Sprintf("%s/%s", dsc.serviceURL, dest) slog.Info("Generating SAS URL for blob", "container", dsc.storageConfig.ContainerName, "blob", dest, "request_type", requestType, "expiration", expiration) - client, err := azBlob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := azBlob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blobOptions()) if err != nil { return "", err } @@ -398,7 +455,7 @@ func (dsc DefaultStorageClient) List( slog.Info("Listing blobs in container", "container", dsc.storageConfig.ContainerName) } - client, err := azContainer.NewClientWithSharedKeyCredential(dsc.serviceURL, dsc.credential, nil) + client, err := azContainer.NewClientWithSharedKeyCredential(dsc.serviceURL, dsc.credential, dsc.containerOptions()) if err != nil { return nil, fmt.Errorf("failed to create container client: %w", err) } @@ -437,7 +494,7 @@ func (dsc DefaultStorageClient) Properties( blobURL := fmt.Sprintf("%s/%s", dsc.serviceURL, dest) slog.Info("Getting properties for blob", "container", dsc.storageConfig.ContainerName, "blob", dest, "url", blobURL) - client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, nil) + client, err := blockblob.NewClientWithSharedKeyCredential(blobURL, dsc.credential, dsc.blockblobOptions()) if err != nil { return err } @@ -469,7 +526,7 @@ func (dsc DefaultStorageClient) Properties( func (dsc DefaultStorageClient) EnsureContainerExists() error { slog.Info("Ensuring container exists", "container", dsc.storageConfig.ContainerName) - containerClient, err := azContainer.NewClientWithSharedKeyCredential(dsc.serviceURL, dsc.credential, nil) + containerClient, err := azContainer.NewClientWithSharedKeyCredential(dsc.serviceURL, dsc.credential, dsc.containerOptions()) if err != nil { return fmt.Errorf("failed to create container client: %w", err) } diff --git a/azurebs/client/storage_client_test.go b/azurebs/client/storage_client_test.go new file mode 100644 index 00000000..277012b7 --- /dev/null +++ b/azurebs/client/storage_client_test.go @@ -0,0 +1,51 @@ +package client_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/cloudfoundry/storage-cli/azurebs/client" + "github.com/cloudfoundry/storage-cli/azurebs/config" +) + +var _ = Describe("NewStorageClient", func() { + baseConfig := func() config.AZStorageConfig { + return config.AZStorageConfig{ + AccountName: "account", + AccountKey: "Zm9vYmFy", // base64("foobar") + ContainerName: "container", + } + } + + It("succeeds without any http timeout configured", func() { + c, err := client.NewStorageClient(baseConfig()) + Expect(err).ToNot(HaveOccurred()) + Expect(c).ToNot(BeNil()) + }) + + It("succeeds with http request and response header timeouts configured", func() { + cfg := baseConfig() + cfg.HTTPRequestTimeout = "30s" + cfg.HTTPResponseHeaderTimeout = "10s" + + c, err := client.NewStorageClient(cfg) + Expect(err).ToNot(HaveOccurred()) + Expect(c).ToNot(BeNil()) + }) + + It("returns an error for an invalid http_request_timeout", func() { + cfg := baseConfig() + cfg.HTTPRequestTimeout = "30" // missing unit + + _, err := client.NewStorageClient(cfg) + Expect(err).To(MatchError(ContainSubstring("missing duration unit"))) + }) + + It("returns an error for an invalid http_response_header_timeout", func() { + cfg := baseConfig() + cfg.HTTPResponseHeaderTimeout = "-5s" + + _, err := client.NewStorageClient(cfg) + Expect(err).To(MatchError(ContainSubstring("must be greater than 0"))) + }) +}) diff --git a/azurebs/config/config.go b/azurebs/config/config.go index 14070943..e7b8ed9b 100644 --- a/azurebs/config/config.go +++ b/azurebs/config/config.go @@ -3,11 +3,17 @@ package config import ( "encoding/json" "errors" + "fmt" "io" + "strconv" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" ) +var errorNonPositiveHTTPRequestTimeout = errors.New("http_request_timeout must be greater than 0") +var errorNonPositiveHTTPResponseHeaderTimeout = errors.New("http_response_header_timeout must be greater than 0") + const storage cloud.ServiceName = "storage" var cloudConfig cloud.Configuration @@ -27,11 +33,13 @@ func init() { } type AZStorageConfig struct { - AccountName string `json:"account_name"` - AccountKey string `json:"account_key"` - ContainerName string `json:"container_name"` - Environment string `json:"environment"` - Timeout string `json:"put_timeout_in_seconds"` + AccountName string `json:"account_name"` + AccountKey string `json:"account_key"` + ContainerName string `json:"container_name"` + Environment string `json:"environment"` + Timeout string `json:"put_timeout_in_seconds"` + HTTPRequestTimeout string `json:"http_request_timeout"` + HTTPResponseHeaderTimeout string `json:"http_response_header_timeout"` } // NewFromReader returns a new azure-storage-cli configuration struct from the contents of reader. @@ -53,6 +61,14 @@ func NewFromReader(reader io.Reader) (AZStorageConfig, error) { return AZStorageConfig{}, err } + if _, err := config.HTTPRequestTimeoutValue(); err != nil { + return AZStorageConfig{}, err + } + + if _, err := config.HTTPResponseHeaderTimeoutValue(); err != nil { + return AZStorageConfig{}, err + } + return config, nil } @@ -74,3 +90,35 @@ func (c *AZStorageConfig) configureCloud() error { } return nil } + +// parseOptionalPositiveDuration parses a Go duration string (e.g. "30s", "2m"). +// An empty value means "unset" and returns a zero duration with no error. +// A bare number without a unit is rejected, as is a non-positive duration. +func parseOptionalPositiveDuration(fieldName, value string, nonPositiveErr error) (time.Duration, error) { + if value == "" { + return 0, nil + } + + if _, err := strconv.ParseFloat(value, 64); err == nil { + return 0, fmt.Errorf("invalid %s: missing duration unit", fieldName) + } + + d, err := time.ParseDuration(value) + if err != nil { + return 0, fmt.Errorf("invalid %s: %w", fieldName, err) + } + + if d <= 0 { + return 0, nonPositiveErr + } + + return d, nil +} + +func (c AZStorageConfig) HTTPRequestTimeoutValue() (time.Duration, error) { + return parseOptionalPositiveDuration("http_request_timeout", c.HTTPRequestTimeout, errorNonPositiveHTTPRequestTimeout) +} + +func (c AZStorageConfig) HTTPResponseHeaderTimeoutValue() (time.Duration, error) { + return parseOptionalPositiveDuration("http_response_header_timeout", c.HTTPResponseHeaderTimeout, errorNonPositiveHTTPResponseHeaderTimeout) +} diff --git a/azurebs/config/config_test.go b/azurebs/config/config_test.go index 9f54c4f5..8944d87d 100644 --- a/azurebs/config/config_test.go +++ b/azurebs/config/config_test.go @@ -3,6 +3,7 @@ package config_test import ( "bytes" "errors" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -87,6 +88,69 @@ var _ = Describe("Config", func() { }) }) }) + + Context("http timeouts", func() { + DescribeTable("HTTPRequestTimeoutValue", + func(value string, expected time.Duration, errSubstring string) { + c := config.AZStorageConfig{HTTPRequestTimeout: value} + result, err := c.HTTPRequestTimeoutValue() + if errSubstring == "" { + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(Equal(expected)) + } else { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(errSubstring)) + } + }, + Entry("empty means unset", "", time.Duration(0), ""), + Entry("valid duration", "30s", 30*time.Second, ""), + Entry("valid minutes", "2m", 2*time.Minute, ""), + Entry("bare number is rejected", "30", time.Duration(0), "missing duration unit"), + Entry("garbage is rejected", "abc", time.Duration(0), "invalid http_request_timeout"), + Entry("zero is rejected", "0s", time.Duration(0), "must be greater than 0"), + Entry("negative is rejected", "-5s", time.Duration(0), "must be greater than 0"), + ) + + DescribeTable("HTTPResponseHeaderTimeoutValue", + func(value string, expected time.Duration, errSubstring string) { + c := config.AZStorageConfig{HTTPResponseHeaderTimeout: value} + result, err := c.HTTPResponseHeaderTimeoutValue() + if errSubstring == "" { + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(Equal(expected)) + } else { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(errSubstring)) + } + }, + Entry("empty means unset", "", time.Duration(0), ""), + Entry("valid duration", "10s", 10*time.Second, ""), + Entry("bare number is rejected", "10", time.Duration(0), "missing duration unit"), + Entry("garbage is rejected", "abc", time.Duration(0), "invalid http_response_header_timeout"), + Entry("zero is rejected", "0s", time.Duration(0), "must be greater than 0"), + Entry("negative is rejected", "-5s", time.Duration(0), "must be greater than 0"), + ) + + It("NewFromReader accepts valid timeout strings", func() { + configJson := []byte(`{"http_request_timeout": "30s", "http_response_header_timeout": "10s"}`) + c, err := config.NewFromReader(bytes.NewReader(configJson)) + Expect(err).ToNot(HaveOccurred()) + Expect(c.HTTPRequestTimeout).To(Equal("30s")) + Expect(c.HTTPResponseHeaderTimeout).To(Equal("10s")) + }) + + It("NewFromReader rejects an invalid http_request_timeout", func() { + configJson := []byte(`{"http_request_timeout": "30"}`) + _, err := config.NewFromReader(bytes.NewReader(configJson)) + Expect(err).To(MatchError(ContainSubstring("missing duration unit"))) + }) + + It("NewFromReader rejects an invalid http_response_header_timeout", func() { + configJson := []byte(`{"http_response_header_timeout": "-1s"}`) + _, err := config.NewFromReader(bytes.NewReader(configJson)) + Expect(err).To(MatchError(ContainSubstring("must be greater than 0"))) + }) + }) }) type explodingReader struct{}