Skip to content

Conversation

@astefan
Copy link
Contributor

@astefan astefan commented Oct 19, 2018

This fixes the bug reported in #33817.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@costin costin added the >bug label Oct 19, 2018
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a comment.

if (!isSSLPropertyPresent && !isSchemaPresent) {
enabled = StringUtils.parseBoolean(SSL_DEFAULT);
} else {
if (isSSLPropertyPresent && isHttpsScheme && !StringUtils.parseBoolean(settings.getProperty(SSL))) {
Copy link
Contributor

@matriv matriv Oct 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove isSSLPropertyPresent here, it's always true as it's checked above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessarily always true @matriv. The first condition could be false if isSSLPropertyPresent is also false (but isSchemaPresent true). This check is on purpose set like this because I want the exception to be thrown only in that specific case.

@astefan astefan merged commit 91434f7 into elastic:master Oct 23, 2018
astefan added a commit to astefan/elasticsearch that referenced this pull request Oct 23, 2018
…col if used (elastic#34635)

* The default SSL option shouldn't override the https protocol if specified. Fixes elastic#33817
kcm pushed a commit that referenced this pull request Oct 30, 2018
…col if used (#34635)

* The default SSL option shouldn't override the https protocol if specified. Fixes #33817
@astefan astefan deleted the 33817_fix branch November 28, 2018 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants