Add new http transport params to storage drivers #4160

tvdfly · 2023-11-18T08:04:28Z

This enables s3 storage driver users to disable keep alives "disablekeepalives" and disable idle connections "disableidleconnections".

Fixes #4159

milosgajdos · 2023-11-21T08:11:24Z

I'm wondering...if we're going to do this -- it seems reasonable -- we should figure out a way to do this across all storage drivers, now just the S3 driver 🤔

tvdfly · 2023-11-21T19:46:18Z

Thanks for the feedback @milosgajdos! I'm happy to try that out in this pr. I'll push an update in the next day or so. Thx

corhere

Thank you for working on adding OpenTelemetry instrumentation! That's been on our roadmap for a long time, and is currently being worked on in #3996. In the interests of not conflicting with that PR or duplicating efforts in adding OpenTelemetry instrumentation, it would be best to keep this PR focused on adding HTTP transport parameters. Could you please back out the OpenTelemetry changes? We would appreciate your help in getting #3996 ready to merge, too.

registry/storage/driver/azure/azure_auth.go

registry/storage/driver/azure/parser.go

registry/storage/driver/gcs/gcs.go

corhere · 2023-12-01T20:28:13Z

registry/storage/driver/gcs/gcs.go

+	disableKeepAlivesParam := false
+	disableKeepAlives := parameters["disablekeepalives"]
+	switch disableKeepAlives := disableKeepAlives.(type) {
+	case string:
+		b, err := strconv.ParseBool(disableKeepAlives)
+		if err != nil {
+			return nil, fmt.Errorf("the disablekeepalives parameter should be a boolean")
+		}
+		disableKeepAlivesParam = b
+	case bool:
+		disableKeepAlivesParam = disableKeepAlives
+	case nil:
+		// do nothing
+	default:
+		return nil, fmt.Errorf("the disablekeepalives parameter should be a boolean")
+	}


This is very repetitive. Given that the same validation rules apply to all the boolean parameters, how about extracting the common logic to a function or closure? E.g.:

boolParam := func(name string) (bool, error) { /* ... */ } disableKeepAlives, err := boolParam("disablekeepalives") if err != nil { return nil, err }

If you want to get really fancy, you could apply the panic-recover error handling pattern to elide having to write if err != nil when parsing each parameter, e.g.:

if boolParam("disablekeepalives") { /* ... */ }

Should that kind of refactoring be a separate pr? I was trying to maintain the pattern in nearby code. I'm happy to do that here for gcs.

The azure driver's config handling does seem easier to work with than the more manual extraction gcs & aws drivers do. Any reason we shouldn't move toward that for gcs, and aws?

docs/content/storage-drivers/azure.md

registry/storage/driver/azure/azure_auth.go

registry/storage/driver/gcs/gcs.go

registry/storage/driver/s3-aws/s3.go

milosgajdos · 2023-12-08T10:12:36Z

This now also needs a rebase.

tvdfly · 2023-12-14T23:41:03Z

Thank you for working on adding OpenTelemetry instrumentation! That's been on our roadmap for a long time, and is currently being worked on in #3996. In the interests of not conflicting with that PR or duplicating efforts in adding OpenTelemetry instrumentation, it would be best to keep this PR focused on adding HTTP transport parameters. Could you please back out the OpenTelemetry changes? We would appreciate your help in getting #3996 ready to merge, too.

Thanks for the review! I removed the otel work from this pr. I'll try #3996 and provide any feedback on otel there.

This enables the azure, gcs, and s3 storage driver users to disable keep alives `"disablekeepalives"`. Fixes distribution#4159 Signed-off-by: T Van Doren <108194999+tvdfly@users.noreply.github.com>

corhere

Why do you need to disable HTTP keep-alives in the storage driver clients? Nowhere in this PR's description nor in #4159 is that explained. Each feature and configuration knob we accept has ongoing maintenance costs. We need to make sure the benefits to you and to other users outweigh the cost before we can accept maintaining this option for the entire lifetime of distribution 3.x.

corhere · 2024-04-09T15:44:31Z

docs/content/storage-drivers/s3.md

@@ -32,6 +33,9 @@ Amazon S3 or S3 compatible services for object storage.
 | `usedualstack` | no | Use AWS dual-stack API endpoints. |
 | `accelerate` | no | Enable S3 Transfer Acceleration. |
 | `objectacl`  | no | The S3 Canned ACL for objects. The default value is "private". |
+| `usedualstack` | no | Use AWS S3 dual-stack endpoints, which support both IPv6 and IPv4, when set to `true`. The default is `false`. |


usedualstack is already documented, three rows up.

corhere · 2024-04-09T15:46:37Z

docs/content/storage-drivers/s3.md

@@ -32,6 +33,9 @@ Amazon S3 or S3 compatible services for object storage.
 | `usedualstack` | no | Use AWS dual-stack API endpoints. |
 | `accelerate` | no | Enable S3 Transfer Acceleration. |
 | `objectacl`  | no | The S3 Canned ACL for objects. The default value is "private". |
+| `usedualstack` | no | Use AWS S3 dual-stack endpoints, which support both IPv6 and IPv4, when set to `true`. The default is `false`. |
+| `multipartcombinesmallpart` | no | Combines small pending uploads with the ready part when `true`. The default is `true`. |


Where is this option implemented? I don't see it anywhere else in the PR nor could I find any evidence in the code base that this was already implemented.

corhere · 2024-04-09T15:47:03Z

docs/content/storage-drivers/s3.md

@@ -32,6 +33,9 @@ Amazon S3 or S3 compatible services for object storage.
 | `usedualstack` | no | Use AWS dual-stack API endpoints. |
 | `accelerate` | no | Enable S3 Transfer Acceleration. |
 | `objectacl`  | no | The S3 Canned ACL for objects. The default value is "private". |
+| `usedualstack` | no | Use AWS S3 dual-stack endpoints, which support both IPv6 and IPv4, when set to `true`. The default is `false`. |
+| `multipartcombinesmallpart` | no | Combines small pending uploads with the ready part when `true`. The default is `true`. |
+| `accelerate` | no | Use AWS S3 Transfer Acceleration endpoints when set to `true`. The default is `false`. |


accelerate is already documented, three rows up.

tvdfly force-pushed the s3-http-client-config branch from 0a266fd to e5df937 Compare November 29, 2023 02:48

milosgajdos requested review from thaJeztah and corhere November 29, 2023 21:45

corhere requested changes Dec 1, 2023

View reviewed changes

tvdfly force-pushed the s3-http-client-config branch from e5df937 to b31c8d5 Compare December 14, 2023 23:34

milosgajdos changed the title ~~Add new http transport params to s3 storage driver~~ Add new http transport params to storage drivers Dec 18, 2023

milosgajdos added the area/storage label Dec 18, 2023

Add new http transport params to azure, gcs, s3 storage drivers

d58e3ff

This enables the azure, gcs, and s3 storage driver users to disable keep alives `"disablekeepalives"`. Fixes distribution#4159 Signed-off-by: T Van Doren <108194999+tvdfly@users.noreply.github.com>

tvdfly force-pushed the s3-http-client-config branch from b31c8d5 to d58e3ff Compare December 19, 2023 23:51

milosgajdos requested a review from corhere April 8, 2024 11:20

corhere requested changes Apr 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new http transport params to storage drivers #4160

Add new http transport params to storage drivers #4160

tvdfly commented Nov 18, 2023 •

edited

milosgajdos commented Nov 21, 2023

tvdfly commented Nov 21, 2023

corhere left a comment •

edited

corhere Dec 1, 2023

tvdfly Dec 14, 2023

milosgajdos commented Dec 8, 2023 •

edited

tvdfly commented Dec 14, 2023

corhere left a comment

corhere Apr 9, 2024

corhere Apr 9, 2024

corhere Apr 9, 2024

Add new http transport params to storage drivers #4160

Are you sure you want to change the base?

Add new http transport params to storage drivers #4160

Conversation

tvdfly commented Nov 18, 2023 • edited

milosgajdos commented Nov 21, 2023

tvdfly commented Nov 21, 2023

corhere left a comment • edited

Choose a reason for hiding this comment

corhere Dec 1, 2023

Choose a reason for hiding this comment

tvdfly Dec 14, 2023

Choose a reason for hiding this comment

milosgajdos commented Dec 8, 2023 • edited

tvdfly commented Dec 14, 2023

corhere left a comment

Choose a reason for hiding this comment

corhere Apr 9, 2024

Choose a reason for hiding this comment

corhere Apr 9, 2024

Choose a reason for hiding this comment

corhere Apr 9, 2024

Choose a reason for hiding this comment

tvdfly commented Nov 18, 2023 •

edited

corhere left a comment •

edited

milosgajdos commented Dec 8, 2023 •

edited