Do not swallow exceptions thrown while parsing settings #13039
Conversation
This commit fixes an issue that was causing Elasticsearch to silently ignore settings files that contain garbage. The underlying issue was swallowing an SettingsException under the assumption that the only reason that an exception could be throw was due to the settings file not existing (in this case the IOException would be the cause of the swallowed SettingsException). This assumption is mistaken as an IOException could also be thrown due to an access error or a read error. Additionally, a SettingsException could be thrown exactly because garbage was found in the settings file. We should instead explicitly check that the settings file exists, and bomb on an exception thrown for any reason. Closes #13028
| // ignore | ||
| Path path = environment.configFile().resolve("elasticsearch" + allowedSuffix); | ||
| if (Files.exists(path)) { | ||
| settingsBuilder.loadFromPath(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm wondering if we should fail if we find several times the config file with different extensions?
There was a problem hiding this comment.
I think you're right, but I don't know the history of allowing multiple settings formats and whether or not it is used in practice. If we are going to make a breaking change on this, now (pre 2.0) is most definitely the time.
There was a problem hiding this comment.
FWIW I've used this in the past for production ES clusters to have a set of common settings (elasticsearch.yml) and node-specific settings (elasticsearch.json) to merge two files with settings.
That said, I still think it's safer/better to remove this feature and fail if more than one config file is found. It reduces the complexity for reasoning where a setting came from.
There was a problem hiding this comment.
still think it's safer/better to remove this feature and fail if more than one config file is found. It reduces the complexity for reasoning where a setting came from.
+1
There was a problem hiding this comment.
I'll open a separate issue for it.
|
+1, I think this is a good step. |
Do not swallow exceptions thrown while parsing settings
This commit fixes an issue that was causing Elasticsearch to silently
ignore settings files that contain garbage. The underlying issue was
swallowing an
SettingsExceptionunder the assumption that the onlyreason that an exception could be throw was due to the settings file
not existing (in this case the
IOExceptionwould be the cause of theswallowed
SettingsException). This assumption is mistaken as anIOExceptioncould also be thrown due to an access error or a readerror. Additionally, a
SettingsExceptioncould be thrown exactlybecause garbage was found in the settings file. We should instead
explicitly check that the settings file exists, and bomb on an
exception thrown for any reason.
Closes #13028