Add Region and Signer Algorithm Overrides to S3 Repos

[original-brownbear]

Exposes S3 SDK signing region and algorithm override settings as requested in #51861.

Closes #51861

[elasticmachine]

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

[tlrx]

I've left two minor comments. I'm a bit more concerned about how we test these settings, can we have an integration test for this?

[tlrx]

Typo singing

[original-brownbear]

Thanks for taking a look Tanguy!

an we have an integration test for this?

Sure we can, it's just a matter of coding it up :) I wonder if it's worth the effort? It seems that this would be outright testing the SDK.
I realise that we do this for retrying, but there we are party to things also in providing a resettable input stream. Here, we'd really just be testing the SDK since we do verify that the client is build with the correct settings in the current tests IMO.

The amount of code it takes to add these tests makes me not want to do it. The fact that SDKs have bugs does makes me want to add the test :D
Tbh. I don't think it's worth it adding it now but maybe we could add a TODO/issue asking to enhance our tests to cover the authorisation side of things as well for S3 and deal with it at lower priority?

[DaveCTurner]

I dunno, I think there's value in an integration test too, just checking that we've got all the end-to-end plumbing right. Maybe against Minio with an explicit region, given that the point of this feature is to support non-S3 services with nonempty regions?

[original-brownbear]

Maybe against Minio with an explicit region, given that the point of this feature is to support non-S3 services with nonempty regions?

The annoying thing with that is that it forces us to add yet another Minio IT run and I'm not sure you can really force Minio to only accept v4 signatures. So we can only test signer override + region.

But I just realised we have no auth testing whatsoever on our S3 mock ... probably good to have it in some form, let's add it for this effort then :) On it.

[tlrx]

Sure we can, it's just a matter of coding it up :) I wonder if it's worth the effort? It seems that this would be outright testing the SDK.

Yes I know :( But we have to maintain such settings for a long time and we previously broke them multiple times without noticing, mostly because we were lacking integration tests. This would also be easier to migrate to AWS SDK V2 if we have more tests. I completely understand your point but I think it will still be valuable at the end.

[DaveCTurner]

But I just realised we have no auth testing whatsoever on our S3 mock

Good catch. We used to, but it looks like we lost them here in #49107.

[DaveCTurner]

Good catch. We used to, but it looks like we lost them here in #49107.

Wait, I found them again. This logic got split up across S3HttpFixture and its subclasses. Sorry for the noise. Maybe you mean some stronger auth testing?

[original-brownbear]

Wait, I found them again. This logic got split up across S3HttpFixture and its subclasses. Sorry for the noise. Maybe you mean some stronger auth testing?

No worries, I was also confused by this and was only referring to the integration tests.

For now I decided to keep it simple though and added the minimum testing required to verify that both setting enforce the right region and/or signer override in d54c45f => let me know what you think :)

[rpasche]

I can confirm that this patch is working. I was able to backport it to 7.4.1 and able to test this plugin on one of our testclusters against an openstack swift container. Was able to run backup successfully. Would really love to see this included in v7.7. Thanks.

[ywelsch]

LGTM

[original-brownbear]

Thanks Yannick + Tanguy!