Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Remove repositories.s3 settings and deprecated settings #23276

Closed

Conversation

dadoonet
Copy link
Member

@dadoonet dadoonet commented Feb 20, 2017

This PR is labeled as work in progress. Do not merge it as is.

Global repositories settings we were able to set in elasticsearch config file under repositories.s3
name space are removed (deprecate in 5.x). This includes repositories.s3.bucket, repositories.s3.server_side_encryption,
repositories.s3.buffer_size, repositories.s3.max_retries, repositories.s3.use_throttle_retries,
repositories.s3.chunk_size, repositories.s3.compress, repositories.s3.storage_class, repositories.s3.canned_acl,
repositories.s3.base_path and repositories.s3.path_style_access.

We must set those settings per repository instead. Respectively bucket, server_side_encryption, buffer_size,
max_retries, use_throttle_retries, chunk_size, compress, storage_class, canned_acl, base_path and
path_style_access.

Related to #22800.

S3 plugin is now using named configurations. We basically define all the S3 clients we want to use by naming
them under s3.client.xxx prefix where xxx is the named configuration (use default as the name if you want to have
one applied by default).

As a consequence, the following settings have been removed: cloud.aws.access_key, cloud.aws.secret_key,
cloud.aws.protocol, cloud.aws.proxy.host, cloud.aws.proxy.port, cloud.aws.proxy.username,
cloud.aws.proxy.password, cloud.aws.signer, cloud.aws.read_timeout.
Also their S3 specific equivalent have been removed: cloud.aws.s3.access_key, cloud.aws.s3.secret_key,
cloud.aws.s3.protocol, cloud.aws.s3.proxy.host, cloud.aws.s3.proxy.port, cloud.aws.s3.proxy.username,
cloud.aws.s3.proxy.password, cloud.aws.s3.signer, cloud.aws.s3.read_timeout.
Also repositories.s3.access_key, repositories.s3.secret_key, repositories.s3.endpoint and
repositories.s3.protocol.

This also removes Signer as discussed in #22599. It will be marked as deprecated in 5.x branch.

Still to do:

  • Update S3 Documentation as I proposed at S3 repository: Add named configurations #22762 (comment)
  • Reintroduce registration of a repository in our REST tests. It requires to be able to register s3.client.default.access_key and s3.client.default.secret_key in the vault or to simulate it.
  • More code cleanup
  • More tests (manual tests and if possible more unit tests)

We just skip the original test as it requires to have first registered a key and a secret in the vault.
@rjernst
Copy link
Member

rjernst commented Feb 20, 2017

I had planned to do this once a number of other things are fixed, including the refactoring necessary to load named configs eagerly at construction. Can we hold off on this?

@dadoonet
Copy link
Member Author

@rjernst Did you start the refactoring yet? If not I think that merging first that kind of PR can simplify your refactoring work.

@rjernst
Copy link
Member

rjernst commented Feb 21, 2017

Yes, I have restarted the refactoring. I also have this PR sitting in a local branch. The reason I held off on this before the refactoring is I need to make it work with 5.x, so waiting to remove this allows for easy backporting, since at the moment 5.x and master are essentially the same.

@dadoonet
Copy link
Member Author

Fair enough. I doubt then that my PR is providing any help.
Let's wait and see what you did and then decide wether to close or rebase it.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@dadoonet
Copy link
Member Author

@rjernst I'm closing this one as I doubt it will be picked up. In case it helps, you can pick that change https://github.com/elastic/elasticsearch/pull/23276/files#diff-c710699b84701013d83a5fe7ddc9a6d3 in your own branch so you won't have to rewrite the plugin changes documentation part.

@dadoonet dadoonet closed this Mar 16, 2017
@clintongormley clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository S3 labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants