Add missing permission for S3 repository

[tlrx]

Note: PR against 2.4 branch, master will follow.

S3 repository needs a special permission to work because when no region is explicitly set the AWS SDK will load a JSON file that contain all Amazon's endpoints and will map the content of this file to plain old Java objects. To do that, it uses Jackson's databinding and reflection that require the java.lang.reflect.ReflectPermission "suppressAccessChecks" permission.

This issue only occur if no region is set in the repository setting and in the elasticsearch.yml file.

closes #18539

[dadoonet]

It looks good to me.

[dadoonet]

That being said, I wonder if we should better ourself fall back setting to a default region than giving permission to the SM

[tlrx]

We could fall back to default US region, but I think that some users also use this plugin with their own custom endpoint and enforcing a AWS default region here might be problematic?

[dadoonet]

IIRC setting the endpoint has precedence.

[tlrx]

I think defaulting to a region, whatever it is, is too trappy. I think that the current way endpoint & region settings are managed in the plugin is not fully coherent with the AWS SDK.

For example, this does not work:

PUT /_snapshot/my_s3_repository '{
  "type": "s3",
  "settings": {
    "bucket": "cloud-aws-test", 
    "region": "us-east" 
  }
}'

because we set the default endpoint to s3.amazonaws.com for regions "us-east" and "us-east-1".

I think we must review the way region override endpoints but for now I'm just fixing things so that it works.

So I'm +1 on adding the special permission for now.

[rmuir]

Same comment as on #19128

My objection is with the explanation: it makes it seem as if this is "justified", it is not. It is simply shitty code AWS code: they need to fix their access modifiers. Its not necessary.

Sorry but, if we explain it like we currently do, it makes it sound like they are doing nothing wrong, and nobody will ever fix it. The truth is you can submit a PR to AWS adding a missing public and the bug goes away.

[s1monw]

Sorry but, if we explain it like we currently do, it makes it sound like they are doing nothing wrong, and nobody will ever fix it. The truth is you can submit a PR to AWS adding a missing public and the bug goes away.

@tlrx can you open an issue with them to fix this?

[rmuir]

I think its ok to give the permission for now, before pushing I just want the comment to be correct so we know its a fixable situation. They have fixed this problem before in another part of the code (their configuration uses the same serialization).

[tlrx]

My objection is with the explanation: it makes it seem as if this is "justified", it is not. It is simply shitty code AWS code: they need to fix their access modifiers. Its not necessary.

I agree, my explanation is misleading, sorry. This is all my brain was able to produce in English after having spent so much time debugging ec2/s3 stuff.

I pointed to Jackson's because the stacktrace in #18539 shows that ClassUtil.checkAndFixAccess() method throws the exception when it tries to call setAccessible(true) on a public constructor without even checking the modifiers first, which seemed strange to me. Since the AWS Partitions class looked good to me (I also suspected a missing public on ctor or setter... did I miss something?) I suspected that Jackson's should check the modifiers first like it does in more recent versions.

I stopped there, hoping that a more recent version of AWS SDK will use a more recent version of Jackon Databinding that has more checks and options to configure object bindings.

Sorry but, if we explain it like we currently do, it makes it sound like they are doing nothing wrong, and nobody will ever fix it.

I agree, I updated my comment. Thanks for your feedback, please let me know if that's better now.

The truth is you can submit a PR to AWS adding a missing public and the bug goes away.

That was my first guess too but I think now that the issue can only be fixed with an update of the version of Jackson used by AWS SDK + a better configuration of Jackson's object mapper used by AWS SDK (like disabling MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS / OVERRIDE_PUBLIC_ACCESS_MODIFIERS / ALLOW_FINAL_FIELDS_AS_MUTATORS). So many things just to load a JSON config file...

I'll create an issue in the aws sdk java GitHub repository to track this.

Edit: Finally found aws/aws-sdk-java#528 and created aws/aws-sdk-java#766 to track this

[rmuir]

+1

[tlrx]

Merged in ef1bbe4