-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Validate proxy base path at parse time #47912
Conversation
Pinging @elastic/es-core-features (:Core/Features/Monitoring) |
@elasticmachine run elasticsearch-ci/bwc |
if (Strings.isNullOrEmpty(value) == false) { | ||
try { | ||
RestClientBuilder.cleanPathPrefix(value); | ||
} catch (IllegalArgumentException e) { |
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 would suggest to open this up to RuntimeException, else if someone adds something like illegal state exception or a NPE this validation won't kick in and the exception will happen while applying the cluster state.
(key) -> Setting.simpleString( | ||
key, | ||
value -> { | ||
if (Strings.isNullOrEmpty(value) == false) { |
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.
what happens if the value is null or empty ? I think the validation here should fail, else it may fail on cluster state application.
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.
Currently, the REST client builder does not set a proxy base path if the setting's value is null or empty. Do you think it should be changed so the setting itself refuses null or empty values?
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 just realized we actually need to support null
as the way to un-set. If the underlying code already protects against empty (e.g. doesn't throw an exception) it should be fine as-is.
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.
A couple items to be more defensive / aggressive in validation.
@elastic/es-core-infra, this work covers both monitoring and settings if you want to review it. |
@elasticmachine update branch |
@jakelandis, I think this is ready for another look. |
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.
LGTM
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.
LGTM
Provides parse-time validation for
PROXY_BASE_PATH_SETTING
as described in #47711.