Skip to content

Update s3-repository docs after upgrade #1356

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

Merged
merged 11 commits into from
May 22, 2025
Merged

Conversation

nicktindall
Copy link
Contributor

These changes are to bring the docs into alignment with the changes made as part of the S3 upgrade elastic/elasticsearch#126843

These changes are to bring the docs into alignment with the changes made as part of the S3 upgrade elastic/elasticsearch#126843
@@ -119,6 +121,8 @@ The following list contains the available client settings. Those that must be st
`use_throttle_retries`
: Whether retries should be throttled (i.e. should back off). Must be `true` or `false`. Defaults to `true`.

Deprecated: This setting is ignored since version 8.19, retries are always throttled.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be inclined to remove the mention of this setting entirely in these docs. At least we should just say this setting does nothing rather than documenting its behaviour in older versions. But really there's no need to mention it here even. Its eventual removal will be handled by the upgrade assistant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also totally ok with removing this one. Are these deprecated settings documented in the reference docs? If they are, and if we properly document them there I think we should be fine (we can just add a link to the reference doc for the entire list of available settings).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3481a19


`protocol`
: The protocol to use to connect to S3. Valid values are either `http` or `https`. Defaults to `https`. When using HTTPS, this repository type validates the repository’s certificate chain using the JVM-wide truststore. Ensure that the root certificate authority is in this truststore using the JVM’s `keytool` tool. If you have a custom certificate authority for your S3 repository and you use the {{es}} [bundled JDK](../../deploy/self-managed/installing-elasticsearch.md#jvm-version), then you will need to reinstall your CA certificate every time you upgrade {{es}}.

Deprecated: This setting is ignored since version 8.19, specify the protocol in the `endpoint` setting instead.
Copy link
Contributor

@DaveCTurner DaveCTurner May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be inclined to remove the mention of this setting entirely in these docs. At least we should just say this setting does nothing rather than documenting its behaviour in older versions. But really there's no need to mention it here even. Its eventual removal will be handled by the upgrade assistant.

That said, as mentioned in this other PR on reflection I'd rather we made it so that a bare endpoint host or host:port value (without a scheme) took its scheme from this setting rather than just always using https://. And then these docs do make a little more sense, but still we should encourage folks not to use it and move the stuff about the trust store into the endpoint docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaveCTurner , I'm totally ok with removing the protocol setting, but then we should accomodate the comment about the CA certificate in the endpoint setting. What do you think?

I mean the following text:

When using HTTPS, this repository type validates the repository’s certificate chain using the JVM-wide truststore. Ensure that the root certificate authority is in this truststore using the JVM’s keytool tool. If you have a custom certificate authority for your S3 repository and you use the {{es}} bundled JDK, then you will need to reinstall your CA certificate every time you upgrade {{es}}.

We can leave it as a note on the endpoint setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in e555217

@@ -133,7 +137,7 @@ In versions `7.0`, `7.1`, `7.2` and `7.3` all bucket operations used the [now-de
: Whether chunked encoding should be disabled or not. If `false`, chunked encoding is enabled and will be used where appropriate. If `true`, chunked encoding is disabled and will not be used, which may mean that snapshot operations consume more resources and take longer to complete. It should only be set to `true` if you are using a storage service that does not support chunked encoding. See the [AWS Java SDK documentation](https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/AmazonS3Builder.html#disableChunkedEncoding--) for details. Defaults to `false`.

`region`
: Allows specifying the signing region to use. Specificing this setting manually should not be necessary for most use cases. Generally, the SDK will correctly guess the signing region to use. It should be considered an expert level setting to support S3-compatible APIs that require [v4 signatures](https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html) and use a region other than the default `us-east-1`. Defaults to empty string which means that the SDK will try to automatically determine the correct signing region.
: Specifies the signing region to use. If not specified, the SDK will attempt to guess the signing region to use, but it is recommended to configure this explicitly. Defaults to empty string which means that the SDK will try to automatically determine the correct signing region.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting now specifies more than just the signing region, it's also supposed to determine the regional endpoint too unless the .endpoint setting is configured, which is much more aligned with how the SDK expects to be used. We should put this up near the top since it probably should be set in most configurations now. I'd also recommend we don't mention about guessing the region - we can say that we use the SDK to try and determine the region automatically if not specified.

Copy link
Contributor Author

@nicktindall nicktindall May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4a159fc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also changed the endpoint docs to indicate it'll default to the regional endpoint for the configured region in 21db89b (I hope that's right)

@@ -133,7 +137,7 @@ In versions `7.0`, `7.1`, `7.2` and `7.3` all bucket operations used the [now-de
: Whether chunked encoding should be disabled or not. If `false`, chunked encoding is enabled and will be used where appropriate. If `true`, chunked encoding is disabled and will not be used, which may mean that snapshot operations consume more resources and take longer to complete. It should only be set to `true` if you are using a storage service that does not support chunked encoding. See the [AWS Java SDK documentation](https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/AmazonS3Builder.html#disableChunkedEncoding--) for details. Defaults to `false`.

`region`
: Allows specifying the signing region to use. Specificing this setting manually should not be necessary for most use cases. Generally, the SDK will correctly guess the signing region to use. It should be considered an expert level setting to support S3-compatible APIs that require [v4 signatures](https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html) and use a region other than the default `us-east-1`. Defaults to empty string which means that the SDK will try to automatically determine the correct signing region.
: Specifies the signing region to use. If not specified, the SDK will attempt to guess the signing region to use, but it is recommended to configure this explicitly. Defaults to empty string which means that the SDK will try to automatically determine the correct signing region.

`signer_override`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's also a deprecated no-op and should probably just be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in e58411f

@bmorelli25 bmorelli25 closed this May 20, 2025
@bmorelli25 bmorelli25 reopened this May 20, 2025
@bmorelli25 bmorelli25 requested a review from a team as a code owner May 20, 2025 21:25
Remove deprecated protocol setting, move certificate validation note to endpoint setting
Remove deprecated use_throttle_retries setting
@nicktindall nicktindall requested a review from DaveCTurner May 22, 2025 06:32
@@ -87,10 +90,7 @@ The following list contains the available client settings. Those that must be st
: An S3 session token. If set, the `access_key` and `secret_key` settings must also be specified.

`endpoint`
: The S3 service endpoint to connect to. This defaults to `s3.amazonaws.com` but the [AWS documentation](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region) lists alternative S3 endpoints. If you are using an [S3-compatible service](#repository-s3-compatible-services) then you should set this to the service’s endpoint.

`protocol`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting is now in use again, see #127744. It's deprecated, users should be setting endpoint to a full URL including scheme, but if they omit the scheme then for now we get it from this setting.

I support moving the info about the trust store to the endpoint setting tho.

@nicktindall nicktindall requested a review from DaveCTurner May 22, 2025 09:35
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nicktindall nicktindall requested a review from eedugon May 22, 2025 10:35
@eedugon
Copy link
Contributor

eedugon commented May 22, 2025

Quick question @nicktindall and @DaveCTurner . Now that we have removed region from the list of configurable settings, the starting sentence of endpoint feels a bit weird:

The S3 service endpoint to connect to. This defaults to the regional endpoint corresponding to the configured region

If region is not to be configured by the user (and we won't be showing region in the list), we shouldn't say that the default value relies on it.

Another comment for the removed items: Consider if it makes sense to add a link to reference doc if there are more advanced settings documented there, whatever you feel.

Copy link
Contributor

@eedugon eedugon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just added a minor suggestion (divide the content in 2 paragraphs) for better readability of the endpoint setting, and a comment about the region setting still mentioned inside endpoint.

@DaveCTurner
Copy link
Contributor

Now that we have removed region from the list of configurable settings

region is still there, it just moved

Consider if it makes sense to add a link to reference doc

This is the reference doc. There are no other places we document these settings.

@eedugon
Copy link
Contributor

eedugon commented May 22, 2025

Thanks @DaveCTurner , all looks great then in my opinion. Maybe better to have the endpoint details in 2 different paragraphs and that's all then.

@nicktindall nicktindall enabled auto-merge (squash) May 22, 2025 11:12
@nicktindall nicktindall removed the request for review from DiannaHohensee May 22, 2025 11:12
@nicktindall nicktindall merged commit 0ba8992 into main May 22, 2025
5 of 6 checks passed
@nicktindall nicktindall deleted the nicktindall-s3-repo-settings branch May 22, 2025 11:15
nicktindall added a commit to nicktindall/elasticsearch that referenced this pull request May 26, 2025
nicktindall added a commit to elastic/elasticsearch that referenced this pull request May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants