Add null and empty string validation to S3 bucket #107883
Conversation
mhl-b
added
>enhancement
:Distributed/Snapshot/Restore
|
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Thanks @mhl-b! I'm hesitant to do this with a setting validator however. In general strengthening the validation of a setting is troublesome because of backwards compatibility: when upgrading from an older cluster, it may have metadata that it considers to be valid but which becomes invalid after the upgrade, and that typically means it will reject the whole cluster state as invalid. It's also fairly troublesome to have a setting whose default value is invalid.
That said, I think it might work in this context because of some details of how we use this particular Settings object which is quite different from other usages. But still, I'd rather we avoided using settings validation here and just checked the value in the constructor.
|
Thank you David, good to know that Settings can have a wider impact. |
| @@ -238,6 +237,10 @@ public void sendResponse(RestResponse response) { | |||
| ); | |||
| } | |||
|
|
|||
| private Settings.Builder defaultRepoSettings() { | |||
There was a problem hiding this comment.
nit: I'd be inclined to inline this. It's only used in a small number of places, it's not really any shorter like this, and indeed it's a little bit of a distraction to the reader to wonder what exactly those default settings are. It'd be different if there were many required settings ofc.
There was a problem hiding this comment.
Thank you, I wasnt sure about it, it make sense to inline since it just a single setting. Will update code.
| ).put(S3Repository.CHUNK_SIZE_SETTING.getKey(), new ByteSizeValue(chunk, ByteSizeUnit.MB).getStringRep()).build(); | ||
| } | ||
|
|
||
| private Settings.Builder defaultRepoSettings() { |
There was a problem hiding this comment.
nit: likewise here. Indeed extracting this method kinda messes up the otherwise-quite-nicely-formatted chain of .put calls in bufferAndChunkSettings().
| reason: "contains is a newly added assertion" | ||
| features: contains | ||
| - do: | ||
| cluster.state: { } |
There was a problem hiding this comment.
nit: cleaning up stuff like whitespace in YAML tests is ok but I'd generally prefer them in a separate PR - it makes git bisect easier to use and avoids cluttering up git annotate output etc.
There was a problem hiding this comment.
I will keep it mind in next PR's. I applied project formatting to entire file, it's relatively small one.
mhl-b
added
the
auto-merge
Add basic validation to S3 bucket name - nullity and empty string.
It is aligned with public docs for "bucket" as required field.
We might want to add more validations based on S3 naming rules.
This PR should not be a breaking change because missing bucket
will eventually throw exception later in the code with obscure error.
I've added yaml test to modules repository_s3/10_basic.yml,
not sure if it's a right place.
Addresses #107840