Allow setting s3 forcepathstyle without regionendpoint

[schanzel]

Currently, the forcepathstyle parameter for the s3 storage driver is
considered only if the regionendpoint parameter is set. Since setting
a region endpoint explicitly is discouraged with AWS s3, it is not clear
how to enforce path style URLs with AWS s3.
This also means, that the default value (true) only applies if a region
endpoint is configured.

This change makes sure we always forward the forcepathstyle parameter
to the aws-sdk if present in the config. This is a breaking change where
a regionendpoint is configured but no explicit forcepathstyle value
is set.

Signed-off-by: Benjamin Schanzel benjamin.schanzel@bmw.de

[schanzel]

To retain the current behaviour, we could keep a default of true for forcepathstyle if the regionendpoint parameter is set. That would make the change non-braking but would also alter config defaults based on other config values. I'm not sure if that would be encouraged.

[schanzel]

Any chance getting this in, maybe in v3.0.0? @milosgajdos @thaJeztah @Jamstah

[milosgajdos]

Since setting a region endpoint explicitly is discouraged with AWS s3

Can you point me somewhere where I can read about this? I understand the reasons, just never came across AWS discouraging users from doing this

[schanzel]

That's stated in the distribution s3 driver docs here https://github.com/distribution/distribution/blob/main/docs/content/storage-drivers/s3.md?plain=1#L44

regionendpoint: (optional) Endpoint URL for S3 compatible APIs. This should not be provided when using Amazon S3.

[schanzel]

Anyone?

[milosgajdos]

Is there a reason why we've changed the bool flag/option into a string? It's not entirely clear to me why we do that

[schanzel]

This was to allow the value to be "unset", so we fall back to the aws-sdk default behavior (like we do now if we don't set the regionendpoint).
I'm happy to change that if there's a better way to do it, or if we are okay with always setting the WithS3ForcePathStyle parameter (and default to false).

[milosgajdos]

I see no reason why forcepathstyle can't remain a boolean and has to be changed to a string 🤷‍♂️

We will also need to update the docs explaining that the value takes effect regardless of the region settings.

[schanzel]

Thanks for the feedback. I updated the change accordingly, always setting WithS3ForcePathStyle with a default value of false (and keeping it a boolean).

[milosgajdos]

LGTM, PTAL @Jamstah @thaJeztah

[squizzi]

That would make the change non-braking but would also alter config defaults based on other config values. I'm not sure if that would be encouraged.

What are the drawbacks of retaining the current forcePathStyleBool := true with this change? As in setting that to true irregardless of params.RegionEndpoint's value? Would that work?

[schanzel]

That would probably work but would also mean we'd be using pathstyle URLs for aws s3 per default, too. The awssdk uses virutal-hosted-style URLs per default and pathstyle urls are ought to be deprecated with aws s3 at some point (cf. https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html).
So I think we shouldn't set this to true per default.