-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Docs: Update s3 repository docs with client settings #26033
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
Conversation
This commit updates the s3 repository docs to clearly mark settings as part of the s3 client settings, as well as those that are secure and must be stored in the elasticsearch keystore. relates elastic#25619
dadoonet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rjernst. I left some first comments.
docs/plugins/repository-s3.asciidoc
Outdated
| } | ||
| ---- | ||
| // CONSOLE | ||
| // TEST[skip:we don't have gcs setup while testing this] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/gcs/s3
docs/plugins/repository-s3.asciidoc
Outdated
| via: `cloud.aws.ec2.protocol` or `cloud.aws.s3.protocol`. | ||
| The client used to connect to S3 has a number of settings available. Client settings names are of | ||
| the form `s3.client.CLIENT_NAME.SETTING_NAME` and specified inside `elasticsearch.yml`. The | ||
| default client name is `default`, but can be customized with the repository setting `client`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should say here that people are expected to provide settings for a client named default.
The way I'm reading what you wrote makes me think that elasticsearch will automatically name a client: default.
It will actually try to find a client named default.
(not sure if what I say makes sense)
docs/plugins/repository-s3.asciidoc
Outdated
| } | ||
| ---- | ||
| // CONSOLE | ||
| // TEST[skip:we don't have gcs setup while testing this] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/gcs/s3
docs/plugins/repository-s3.asciidoc
Outdated
| `secret_key`:: | ||
|
|
||
| ===== Read timeout | ||
| An s3 secret key. Must be specified with `access_key`. (Secure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/access_key/secret_key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is there is correct. When configuring a secret_key, an access_key must also be configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the wording to:
The `access_key` setting must also be specified.
| cloud.aws.read_timeout: 30s | ||
| ---- | ||
| The s3 service endpoint to connect to. This will be automatically | ||
| figured out by the s3 client based on the bucket location, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? Is that true?
I think we can mention that it defaults to us-east-1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was true at one point. Looking at the AmazonS3Client code now, I'm not longer sure. I will do some more investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked and it does indeed still work. I created a bucket in us-west-1, and the only repository setting I set was the bucket name. I was able to create the repository, and create a snapshot just fine. I think what I have here is correct (not mentioning us-east-1), because otherwise one might be confused and think if there bucket is not in us-east-1 they would need to specify the endpoint.
docs/plugins/repository-s3.asciidoc
Outdated
|
|
||
| `protocol`:: | ||
|
|
||
| The protocl to use to connect to s3. Valid values are either `http` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/protocl/protocol
docs/plugins/repository-s3.asciidoc
Outdated
|
|
||
| The protocol to use (`http` or `https`). Defaults to value of | ||
| `cloud.aws.protocol` or `cloud.aws.s3.protocol`. | ||
| The name of the s3 client to use to connect to S3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add: Defaults to default.
| Specifies the path within bucket to repository data. Defaults to | ||
| value of `repositories.s3.base_path` or to root directory if not set. | ||
| Previously, the base_path could take a leading `/` (forward slash). | ||
| However, this has been deprecated and setting the base_path now should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still have global settings like repositories.s3.base_path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we do not. They were removed in #24445.
| (note that protocol can be `http` or `https`): | ||
| The following settings name be specified for a client. Some settings are sensitive and must be | ||
| stored in the {ref}/secure-settings.html[elasticsearch keystore], and are marked as `Secure`. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make that obvious and because people will like to do some copy and paste from the doc, I'd add an example like:
bin/elasticsearch-keystore add s3.client.default.access_key
bin/elasticsearch-keystore add s3.client.default.secret_keyThose 2 ones are going to be used all the time.
|
@dadoonet I reworded some of the docs based on your feedback. |
dadoonet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! LGTM. Thanks!
This commit updates the s3 repository docs to clearly mark settings as part of the s3 client settings, as well as those that are secure and must be stored in the elasticsearch keystore. relates #25619
This commit updates the s3 repository docs to clearly mark settings as part of the s3 client settings, as well as those that are secure and must be stored in the elasticsearch keystore. relates #25619
This commit updates the s3 repository docs to clearly mark settings as
part of the s3 client settings, as well as those that are secure and
must be stored in the elasticsearch keystore.
relates #25619