-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Provide an Option to Use Path-Style-Access with S3 Repo #41966
Provide an Option to Use Path-Style-Access with S3 Repo #41966
Conversation
* As discussed, added the option to use path style access back again and deprecated it. * Defaulted to `false` * Added warning to docs * Closes elastic#41816
Pinging @elastic/es-distributed |
@@ -95,6 +95,10 @@ | |||
static final Setting.AffixSetting<Boolean> USE_THROTTLE_RETRIES_SETTING = Setting.affixKeySetting(PREFIX, "use_throttle_retries", | |||
key -> Setting.boolSetting(key, ClientConfiguration.DEFAULT_THROTTLE_RETRIES, Property.NodeScope)); | |||
|
|||
/** Whether the s3 client should use path style access. */ | |||
static final Setting.AffixSetting<Boolean> USE_PATH_STYLE_ACCESS = Setting.affixKeySetting(PREFIX, "path_style_access", | |||
key -> Setting.boolSetting(key, false, Property.NodeScope, Property.Deprecated)); |
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.
Now that AWS changed course on this one and has announced continued support for this access pattern for existing/older buckets (https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story), maybe there's no need to change the default here before 8.0
?
@@ -127,9 +131,13 @@ | |||
/** Whether the s3 client should use an exponential backoff retry policy. */ | |||
final boolean throttleRetries; | |||
|
|||
/** Whether the s3 client should use path style access. */ | |||
final boolean pathStyleAccess; |
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.
The AWS SDK uses this as a ternary setting (true, false, null), with null being the default, which enables the path-style setting based on the configured endpoint. I think we should do treat this setting exactly the same way, allowing path-style setting both being explicitly enabled and disabled.
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.
Sounds good, so we break behavior after all by changing the default to null
? See my comment above, this may not be necessary (at least in 7.x)?
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's unclear what the behaviour of setPathStyleAccessEnabled(Boolean.FALSE)
is from the SDK docs. I think we're doing the right thing here, either enabling it or leaving it unset.
@ywelsch Adjusted this to a ternary setting now and default to the SDK behavior as discussed. Take a look when you have a chance :) |
docs/plugins/repository-s3.asciidoc
Outdated
`path_style_access`:: | ||
|
||
Whether to use the path style access pattern. If `true`, the path style access pattern will be used if false, DNS style access will | ||
be used. Defaults to letting the AWS S3 SDK decide the right access pattern dynamically. |
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.
AWS Java SDK
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/right access pattern/path style access/
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.
Let's add a link to the relevant SDK docs
docs/plugins/repository-s3.asciidoc
Outdated
@@ -145,6 +145,18 @@ settings belong in the `elasticsearch.yml` file. | |||
Whether retries should be throttled (i.e. should back off). Must be `true` | |||
or `false`. Defaults to `true`. | |||
|
|||
`path_style_access`:: | |||
|
|||
Whether to use the path style access pattern. If `true`, the path style access pattern will be used if false, DNS style access will |
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.
this sentence reads a bit funny with the if false
not being delimited
docs/plugins/repository-s3.asciidoc
Outdated
AWS S3 will stop supporting the path style access pattern for new buckets from | ||
https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story[September 30th, 2020]. | ||
Any setups using the path style access pattern should be switched to the domain style access pattern as soon as possible to ensure | ||
continued compatibility with the S3 repository plugin. |
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.
Technically it depends on how long AWS is going to support path-style-access on their Java SDK. I couldn't find any information on that. I'm not sure whether we should word this here as strongly, but instead point out that we rely on the SDK and that if that configuration option is going away, we will not be able to support this anymore.
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.
Isn't toning it down a little inconsistent with us now making a breaking change in 7.x
?
My thinking was that we're doing this breaking change because we fear that future versions of the SDK won't support path style access. If we don't think this is an imminent enough issue to strongly push users towards adjusting their environment asap then why make a breaking change and not just wait for 8.0
?
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.
Isn't toning it down a little inconsistent with us now making a breaking change in 7.x?
It's just going back to the pre-7.0 behavior, which was the more sensible default, especially given the current strategy of Amazon not to support path-style access in the future anymore, and there being no way to explicitly force-disable it in the SDK (i.e. it would still use path-style access with IP-based endpoint). This here makes it sound like we would break Minio support with path-style access at any moment in time, which sounds perhaps a bit overdramatic.
==== The S3 repository plugin uses the DNS style access pattern by default | ||
|
||
Starting in version 7.3 using the path style access pattern with the S3 repository is deprecated. | ||
Previously the S3 repository plugin was exclusively using the path style access pattern. This is a breaking |
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.
Perhaps qualify this by "In 7.0, 7.1 and 7.2"
/** Whether the s3 client should use path style access. */ | ||
static final Setting.AffixSetting<S3ClientSettings.PathStyleAccess> USE_PATH_STYLE_ACCESS = Setting.affixKeySetting( | ||
PREFIX, "path_style_access", | ||
key -> new Setting<>(key, s -> "default", s -> Boolean.parseBoolean(s) ? PathStyleAccess.ENABLED : PathStyleAccess.DISABLED, |
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.
this PR is missing tests, and I doubt that the parsing here is correct given that it never yields PathStyleAccess.DEFAULT
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.
My idea was to simply only allow parsing true
and false
here and let the DEFAULT
represent the setting not being set (I didn't see much value in allowing to explicitly set "default" or "null"). Not a good idea? :)
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.
We can make this a boolean setting (see my comment above) and use S3ClientOptions.DEFAULT_PATH_STYLE_ACCESS
, which is false, as the default value. We should also add a test to make sure this default never changes.
[float] | ||
==== The S3 repository plugin uses the DNS style access pattern by default | ||
|
||
Starting in version 7.3 using the path style access pattern with the S3 repository is deprecated. |
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'm not sure we should deprecate this at this time. AWS's SDK is going to support path-style access for a very long time to come (even after Sept. 2020, given that the SDK will still be used to connect to older repos).
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.
Same point as in the above, why make a breaking change then (that we know for a fact will break some environments)?
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.
The focus of this section should be on the fact that the behavior changes between 7.2- and 7.3+, going back to the 6.x behavior, and that the 7.0-7.2 behavior can be restored by setting this new setting. The deprecation of that new setting is secondary. As said, given that we don't have a time frame for removing this setting, I would prefer to defer the deprecation to a later time.
static final Setting.AffixSetting<S3ClientSettings.PathStyleAccess> USE_PATH_STYLE_ACCESS = Setting.affixKeySetting( | ||
PREFIX, "path_style_access", | ||
key -> new Setting<>(key, s -> "default", s -> Boolean.parseBoolean(s) ? PathStyleAccess.ENABLED : PathStyleAccess.DISABLED, | ||
Property.NodeScope, Property.Deprecated)); |
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.
As said above, I don't think we should log deprecation warnings.
@ywelsch all points addressed :), left some questions though. |
Jenkins run elasticsearch-ci/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.
I have added a few more replies. Also, still missing tests.
docs/plugins/repository-s3.asciidoc
Outdated
`path_style_access`:: | ||
|
||
Whether to use the path style access pattern. If `true`, the path style access pattern will be used. If set to`false`, DNS style access will | ||
be used. Defaults to letting the AWS Java SDK decide whether to use the path style access pattern dynamically |
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.
AFAICS, the SDK is implementing something different. If set to false, it is equivalent to the SDK's default of determining the path style access pattern depending on the configured endpoint (an IP will result in path-style access). The SDK only allows forcing this to true
, which means that all requests are forced to use path-style access. Implementation and docs should reflect 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.
RIght ... I should've looked on step deeper S3ClientOptions
uses boolean
already ... will simplify this PR then :)
/** Whether the s3 client should use path style access. */ | ||
static final Setting.AffixSetting<S3ClientSettings.PathStyleAccess> USE_PATH_STYLE_ACCESS = Setting.affixKeySetting( | ||
PREFIX, "path_style_access", | ||
key -> new Setting<>(key, s -> "default", s -> Boolean.parseBoolean(s) ? PathStyleAccess.ENABLED : PathStyleAccess.DISABLED, |
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.
We can make this a boolean setting (see my comment above) and use S3ClientOptions.DEFAULT_PATH_STYLE_ACCESS
, which is false, as the default value. We should also add a test to make sure this default never changes.
docs/plugins/repository-s3.asciidoc
Outdated
AWS S3 will stop supporting the path style access pattern for new buckets from | ||
https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story[September 30th, 2020]. | ||
Any setups using the path style access pattern should be switched to the domain style access pattern as soon as possible to ensure | ||
continued compatibility with the S3 repository plugin. |
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.
Isn't toning it down a little inconsistent with us now making a breaking change in 7.x?
It's just going back to the pre-7.0 behavior, which was the more sensible default, especially given the current strategy of Amazon not to support path-style access in the future anymore, and there being no way to explicitly force-disable it in the SDK (i.e. it would still use path-style access with IP-based endpoint). This here makes it sound like we would break Minio support with path-style access at any moment in time, which sounds perhaps a bit overdramatic.
|
||
Starting in version 7.3 using the path style access pattern with the S3 repository is deprecated. | ||
In versions 7.0, 7.1, and 7.2 the S3 repository plugin was exclusively using the path style access pattern. This is a breaking | ||
change for deployments that do not also allow for the DNS style access pattern. For short-term compatibility with these deployments users |
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.
only those that are not on AWS and that do not use IP-address based endpoint.
[float] | ||
==== The S3 repository plugin uses the DNS style access pattern by default | ||
|
||
Starting in version 7.3 using the path style access pattern with the S3 repository is deprecated. |
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.
The focus of this section should be on the fact that the behavior changes between 7.2- and 7.3+, going back to the 6.x behavior, and that the 7.0-7.2 behavior can be restored by setting this new setting. The deprecation of that new setting is secondary. As said, given that we don't have a time frame for removing this setting, I would prefer to defer the deprecation to a later time.
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've left a few smaller doc changes, but looks good o.w.
@DaveCTurner can you have a look as well?
docs/plugins/repository-s3.asciidoc
Outdated
(See https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/AmazonS3Builder.html#setPathStyleAccessEnabled-java.lang.Boolean-[AWS documentation] for details). | ||
Defaults to `false`. | ||
|
||
NOTE: In versions `7.0` and `7.1`, all bucket operations were using the path style access pattern in every situation without an option to |
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.
7.0, 7.1 and 7.2
@DaveCTurner ping :) |
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.
Sorry, somehow I missed this past the 7.3 feature freeze. In repentance, I've updated the docs to refer back to 7.0-7.3 and cleaned up the wording and formatting a bit too, see f76a14e. This LGTM now.
@@ -127,9 +131,13 @@ | |||
/** Whether the s3 client should use an exponential backoff retry policy. */ | |||
final boolean throttleRetries; | |||
|
|||
/** Whether the s3 client should use path style access. */ | |||
final boolean pathStyleAccess; |
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's unclear what the behaviour of setPathStyleAccessEnabled(Boolean.FALSE)
is from the SDK docs. I think we're doing the right thing here, either enabling it or leaving it unset.
thanks @DaveCTurner ! |
* Provide an Option to Use Path-Style-Access with S3 Repo * As discussed, added the option to use path style access back again and deprecated it. * Defaulted to `false` * Added warning to docs * Closes elastic#41816
Add support for elastic/elasticsearch#41966
Add support for S3 repo settings: path style access: elastic/elasticsearch#41966
Add support for S3 repo settings: path style access: elastic/elasticsearch#41966 (cherry picked from commit 6b0afaa)
As discussed, added the option to use path style access back again and
deprecated it.
Defaulted to falling back to the SDK defaults
Added warning to docs + breaking changes entry
Closes Path-style S3 access is deprecated #41816