Reintroduce the ability to configure S3 repository credentials in cluster state #88652
Conversation
…ster state Revert of elastic#46147, we want to keep this functionality around for a little longer.
original-brownbear
added
>non-issue
:Distributed Coordination/Snapshot/Restore
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| @@ -210,6 +210,13 @@ private void registerSetting(Setting<?> setting) { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: remove this hack once we remove the deprecated ability to use repository settings in the cluster state in the S3 snapshot | |||
| // module | |||
| private boolean isS3InsecureCredentials(Setting<?> setting) { | |||
There was a problem hiding this comment.
This is a weird hack admittedly, but I needed this to get the ability to register the settings for filtering again. We don't have the requirement about the name containing a dot in 7.x and since this is just temporary I figured it was good enough.
| static { | ||
| AccessController.doPrivileged((PrivilegedAction<Void>) () -> { | ||
| // required for client settings overwriting when running in IDE | ||
| System.setProperty("es.allow_insecure_settings", "true"); |
There was a problem hiding this comment.
why is this needed when it is set via gradle? This doesn't seem right since it is setting the property forever, leaking into any test suites that run after it. Setting via gradle is the way to go (along with the isolated task for this test configuration).
There was a problem hiding this comment.
Fair point, I just copied it from 7.x as it's done there. Maybe do a follow-up removing the hack from both and backport that follow-up to 7.x as well.
I guess this was from a time when you couldn't neatly run these tests via Gradle from Intellij.
| @@ -141,7 +141,9 @@ public List<Setting<?>> getSettings() { | |||
| S3ClientSettings.USE_THROTTLE_RETRIES_SETTING, | |||
| S3ClientSettings.USE_PATH_STYLE_ACCESS, | |||
| S3ClientSettings.SIGNER_OVERRIDE, | |||
| S3ClientSettings.REGION | |||
| S3ClientSettings.REGION, | |||
| S3Repository.ACCESS_KEY_SETTING, | |||
There was a problem hiding this comment.
Instead of adding this back both as a cluster setting (fallback) and a repository specific setting, could we only add back the latter? Then we wouldn't need to register the setting, nor have the hack to allow non-dotted names for this edge case.
There was a problem hiding this comment.
We don't really have repository settings registered anywhere do we? As with my other comment, I had to add this back for the setting filtering on the REST layer to work so we don't serialize this back to the user (maybe there's a better workaround for this?).
There was a problem hiding this comment.
"entity" settings as I have been calling them recently are not registered anywhere. The settings registered here in Plugin.getSettings are for node, cluster and index settings. So if the intention is to only have this settable directly in the repositories rest api, the there is no need to register it.
There was a problem hiding this comment.
I get that we're abusing setting registering here, we definitely don't need these as cluster/node or index setting but we need the setting filtering during x-content serialization for these, which seems is only available by registering the setting as a filtered setting as done here (and in 7.x for the same reason).
Can I get the filtering in any other way?
There was a problem hiding this comment.
Ahh, I understand now. I don’t know of another way at the moment.
|
Thanks Ryan! |
Revert of #46147, we want to keep this functionality around for a little longer.
This change is effectively just a copy of the 7.x code that we still have for this functionality with the exception that we now log this kind of thing as a
CRITICALdeprecation and the hack I had to add to the settings module.