Validating monitoring hosts setting while parsing #47246
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit lifts the validation of the monitoring hosts setting into the setting itself, rather than when the setting is used. This prevents a scenario where an invalid value for the setting is accepted, but then later fails while applying a cluster state with the invalid setting.
|
Pinging @elastic/es-core-features |
|
@jasontedor - I missed that you were working on this too. I put up a quicker fix PR that prevents the major issue from occurring. #47249 I think a proper fix is more inline with what you have here (updates to the setting infrastructure). However it should be noted that this issue is not limited to just the host field. I put some notes in the PR. |
|
@jakelandis I don't see the notes, did you click submit review? If you're referring to the other settings, for example |
|
@jasontedor the notes are just the commit message on #47249 (which i will close in favor of this).
yup. thanks. |
|
Oh, I see, you were referring to other PR. Thanks for clarifying! |
Today when settings validate, they can only validate against settings that are of the same type. While this strong-type is convenient from a development perspective, it is too limiting in that some settings need to validate against settings of a different type. For example, the list setting xpack.monitoring.exporters.<namespace>.host wants to validate that it is non-empty if and only if the string setting xpack.monitoring.exporters.<namespace>.type is "http". Today this is impossible since the settings validation framework only allows that setting to validate against other list settings. This commit increases the flexibility here to validate against settings of arbitrary type, at the expense of losing strong-typing during development.
This reverts commit 606d374.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 4, 2019
This commit lifts the validation of the monitoring hosts setting into the setting itself, rather than when the setting is used. This prevents a scenario where an invalid value for the setting is accepted, but then later fails while applying a cluster state with the invalid setting.
This was referenced Feb 3, 2020
albertzaharovits
added a commit
that referenced
this pull request
Feb 24, 2020
Add validation for the following logfile audit settings:
xpack.security.audit.logfile.events.include
xpack.security.audit.logfile.events.exclude
xpack.security.audit.logfile.events.ignore_filters.*.users
xpack.security.audit.logfile.events.ignore_filters.*.realms
xpack.security.audit.logfile.events.ignore_filters.*.roles
xpack.security.audit.logfile.events.ignore_filters.*.indices
Closes #52357
Relates #47711 #47038
Follows the example from #47246
albertzaharovits
added a commit
to albertzaharovits/elasticsearch
that referenced
this pull request
Feb 24, 2020
Add validation for the following logfile audit settings:
xpack.security.audit.logfile.events.include
xpack.security.audit.logfile.events.exclude
xpack.security.audit.logfile.events.ignore_filters.*.users
xpack.security.audit.logfile.events.ignore_filters.*.realms
xpack.security.audit.logfile.events.ignore_filters.*.roles
xpack.security.audit.logfile.events.ignore_filters.*.indices
Closes elastic#52357
Relates elastic#47711 elastic#47038
Follows the example from elastic#47246
albertzaharovits
added a commit
to albertzaharovits/elasticsearch
that referenced
this pull request
Feb 24, 2020
Add validation for the following logfile audit settings:
xpack.security.audit.logfile.events.include
xpack.security.audit.logfile.events.exclude
xpack.security.audit.logfile.events.ignore_filters.*.users
xpack.security.audit.logfile.events.ignore_filters.*.realms
xpack.security.audit.logfile.events.ignore_filters.*.roles
xpack.security.audit.logfile.events.ignore_filters.*.indices
Closes elastic#52357
Relates elastic#47711 elastic#47038
Follows the example from elastic#47246
albertzaharovits
added a commit
that referenced
this pull request
Feb 24, 2020
Add validation for the following logfile audit settings:
xpack.security.audit.logfile.events.include
xpack.security.audit.logfile.events.exclude
xpack.security.audit.logfile.events.ignore_filters.*.users
xpack.security.audit.logfile.events.ignore_filters.*.realms
xpack.security.audit.logfile.events.ignore_filters.*.roles
xpack.security.audit.logfile.events.ignore_filters.*.indices
Closes #52357
Relates #47711 #47038
Follows the example from #47246
albertzaharovits
added a commit
that referenced
this pull request
Feb 24, 2020
Add validation for the following logfile audit settings:
xpack.security.audit.logfile.events.include
xpack.security.audit.logfile.events.exclude
xpack.security.audit.logfile.events.ignore_filters.*.users
xpack.security.audit.logfile.events.ignore_filters.*.realms
xpack.security.audit.logfile.events.ignore_filters.*.roles
xpack.security.audit.logfile.events.ignore_filters.*.indices
Closes #52357
Relates #47711 #47038
Follows the example from #47246
jakelandis
added a commit
that referenced
this pull request
Jun 4, 2020
#47711 and #47246 helped to validate that monitoring settings are rejected at time of setting the monitoring settings. Else an invalid monitoring setting can find it's way into the cluster state and result in an exception thrown [1] on the cluster state application (there by causing significant issues). Some additional monitoring settings have been identified that can result in invalid cluster state that also result in exceptions thrown on cluster state application. All settings require a type of either http or local to be applicable. When a setting is changed, the exporters are automatically updated with the new settings. However, if the old or new settings lack of a type setting an exception will be thrown (since exporters are always of type 'http' or 'local'). Arguably we shouldn't blindly create and destroy new exporters on each monitoring setting update, but the lifecycle of the exporters is abit out the scope this PR is trying to address. This commit introduces a similar methodology to check for validity as #47711 and #47246 but this time for ALL (including non-http) settings. Monitoring settings are not useful unless there an exporter with a type defined. The type is used as dependent setting, such that it must exist to set the value. This ensures that when any monitoring settings changes that they can only get added to cluster state if the type exists. If the type exists (and the other validations pass) then the exporters will get re-built and the cluster state remains valid. Tests have been included to ensure that all dynamic monitoring settings have the type as dependent settings. [1] org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this pull request
Jun 4, 2020
elastic#47711 and elastic#47246 helped to validate that monitoring settings are rejected at time of setting the monitoring settings. Else an invalid monitoring setting can find it's way into the cluster state and result in an exception thrown [1] on the cluster state application (there by causing significant issues). Some additional monitoring settings have been identified that can result in invalid cluster state that also result in exceptions thrown on cluster state application. All settings require a type of either http or local to be applicable. When a setting is changed, the exporters are automatically updated with the new settings. However, if the old or new settings lack of a type setting an exception will be thrown (since exporters are always of type 'http' or 'local'). Arguably we shouldn't blindly create and destroy new exporters on each monitoring setting update, but the lifecycle of the exporters is abit out the scope this PR is trying to address. This commit introduces a similar methodology to check for validity as elastic#47711 and elastic#47246 but this time for ALL (including non-http) settings. Monitoring settings are not useful unless there an exporter with a type defined. The type is used as dependent setting, such that it must exist to set the value. This ensures that when any monitoring settings changes that they can only get added to cluster state if the type exists. If the type exists (and the other validations pass) then the exporters will get re-built and the cluster state remains valid. Tests have been included to ensure that all dynamic monitoring settings have the type as dependent settings. [1] org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this pull request
Jun 4, 2020
elastic#47711 and elastic#47246 helped to validate that monitoring settings are rejected at time of setting the monitoring settings. Else an invalid monitoring setting can find it's way into the cluster state and result in an exception thrown [1] on the cluster state application (there by causing significant issues). Some additional monitoring settings have been identified that can result in invalid cluster state that also result in exceptions thrown on cluster state application. All settings require a type of either http or local to be applicable. When a setting is changed, the exporters are automatically updated with the new settings. However, if the old or new settings lack of a type setting an exception will be thrown (since exporters are always of type 'http' or 'local'). Arguably we shouldn't blindly create and destroy new exporters on each monitoring setting update, but the lifecycle of the exporters is abit out the scope this PR is trying to address. This commit introduces a similar methodology to check for validity as elastic#47711 and elastic#47246 but this time for ALL (including non-http) settings. Monitoring settings are not useful unless there an exporter with a type defined. The type is used as dependent setting, such that it must exist to set the value. This ensures that when any monitoring settings changes that they can only get added to cluster state if the type exists. If the type exists (and the other validations pass) then the exporters will get re-built and the cluster state remains valid. Tests have been included to ensure that all dynamic monitoring settings have the type as dependent settings. [1] org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis
added a commit
that referenced
this pull request
Jun 5, 2020
…57704) #47711 and #47246 helped to validate that monitoring settings are rejected at time of setting the monitoring settings. Else an invalid monitoring setting can find it's way into the cluster state and result in an exception thrown [1] on the cluster state application (there by causing significant issues). Some additional monitoring settings have been identified that can result in invalid cluster state that also result in exceptions thrown on cluster state application. All settings require a type of either http or local to be applicable. When a setting is changed, the exporters are automatically updated with the new settings. However, if the old or new settings lack of a type setting an exception will be thrown (since exporters are always of type 'http' or 'local'). Arguably we shouldn't blindly create and destroy new exporters on each monitoring setting update, but the lifecycle of the exporters is abit out the scope this PR is trying to address. This commit introduces a similar methodology to check for validity as #47711 and #47246 but this time for ALL (including non-http) settings. Monitoring settings are not useful unless there an exporter with a type defined. The type is used as dependent setting, such that it must exist to set the value. This ensures that when any monitoring settings changes that they can only get added to cluster state if the type exists. If the type exists (and the other validations pass) then the exporters will get re-built and the cluster state remains valid. Tests have been included to ensure that all dynamic monitoring settings have the type as dependent settings. [1] org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis
added a commit
that referenced
this pull request
Jun 5, 2020
…57703) #47711 and #47246 helped to validate that monitoring settings are rejected at time of setting the monitoring settings. Else an invalid monitoring setting can find it's way into the cluster state and result in an exception thrown [1] on the cluster state application (there by causing significant issues). Some additional monitoring settings have been identified that can result in invalid cluster state that also result in exceptions thrown on cluster state application. All settings require a type of either http or local to be applicable. When a setting is changed, the exporters are automatically updated with the new settings. However, if the old or new settings lack of a type setting an exception will be thrown (since exporters are always of type 'http' or 'local'). Arguably we shouldn't blindly create and destroy new exporters on each monitoring setting update, but the lifecycle of the exporters is abit out the scope this PR is trying to address. This commit introduces a similar methodology to check for validity as #47711 and #47246 but this time for ALL (including non-http) settings. Monitoring settings are not useful unless there an exporter with a type defined. The type is used as dependent setting, such that it must exist to set the value. This ensures that when any monitoring settings changes that they can only get added to cluster state if the type exists. If the type exists (and the other validations pass) then the exporters will get re-built and the cluster state remains valid. Tests have been included to ensure that all dynamic monitoring settings have the type as dependent settings. [1] org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit lifts the validation of the monitoring hosts setting into the setting itself, rather than when the setting is used. This prevents a scenario where an invalid value for the setting is accepted, but then later fails while applying a cluster state with the invalid setting.
Closes #47125