Skip to content
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

No longer add illegal content type option to stored search templates #24251

Merged
merged 1 commit into from Apr 22, 2017

Conversation

@colings86
Copy link
Member

commented Apr 21, 2017

When parsing StoredSearchScript we were adding a Content type option that was forbidden (by a check that threw an exception) by the parser thats used to parse the template when we read it from the cluster state. This was stopping Elastisearch from starting after stored search templates had been added.

This change no longer adds the content type option to the StoredScriptSource object when parsing from the put search template request. This is safe because the StoredScriptSource content is always JSON when its stored in the cluster state since we do a conversion to JSON before this point.

Also removes the check for the content type in the options when parsing StoredScriptSource so users who already have stored scripts can start Elasticsearch.

Closes #24227

When parsing StoredSearchScript we were adding a Content type option that was forbidden (by a check that threw an exception) by the parser thats used to parse the template when we read it from the cluster state. This was stopping Elastisearch from starting after stored search templates had been added.

This change no longer adds the content type option to the StoredScriptSource object when parsing from the put search template request.  This is safe because the StoredScriptSource content is always JSON when its stored in the cluster state since we do a conversion to JSON before this point.

Also removes the check for the content type in the options when parsing StoredScriptSource so users who already have stored scripts can start Elasticsearch.

Closes #24227
Copy link
Contributor

left a comment

LGTM! Thank you for looking into this issue and fixing it! Left one minor comment.

@@ -266,8 +262,7 @@ public static StoredScriptSource parse(String lang, BytesReference content, XCon
//this is really for search templates, that need to be converted to json format
try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
builder.copyCurrentStructure(parser);
return new StoredScriptSource(lang, builder.string(),
Collections.singletonMap(Script.CONTENT_TYPE_OPTION, XContentType.JSON.mediaType()));
return new StoredScriptSource(lang, builder.string(), Collections.emptyMap());

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 21, 2017

Contributor

Since the check is being removed, maybe it makes sense to continue to store this with the content-type set to JSON? This only works now because of the default in the mustache engine which is likely fine because I don't see the default changing.

This comment has been minimized.

Copy link
@colings86

colings86 Apr 21, 2017

Author Member

I think we should not store the content type so we can add the check back in a later version?

This comment has been minimized.

Copy link
@javanna

javanna Apr 21, 2017

Member

I think that we should have different code-paths for when we validate user inputs and when we parse stuff from cluster state. We should not fail if we set the content-type ourselves, but only if it was set by users, I guess. In that case we wouldn't have to remove the check in setOptions, if it was only performed for user input. Yet, I am not too familiar with this validation check and I cannot tell how important it is, nor when it was introduced.

Side note: the reason why removing the content-type from the options map doesn't affect the parsing side of things seems to be that json is the default mime type anyways, so it didn't need to be set to json explicitly, see https://github.com/elastic/elasticsearch/blob/master/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java#L79 .

This comment has been minimized.

Copy link
@jdconrad

jdconrad Apr 21, 2017

Contributor

I agree with @javanna that we should ultimately have different paths for user input and loading cluster state, but I think that can be done in a follow up PR.

I added the ability to add compiler options because it may be needed for Painless in the future and there was already a single option there for templates anyway so it made sense to encapsulate it. Content-type is reserved internally for templates only, so this check was preventing non-template scripts from specifying content-type. The search template API never allows options to be input anyway so it's not a problem there. Removal of this check should be safe for now since scripting languages at worst will ignore compiler options or in the case of Painless see an unrecognized option and fail at compile time.

Also my apologies for introducing the bug, I hadn't fully thought through the code path shared by both user-input and internal cluster state loading.

This comment has been minimized.

Copy link
@javanna

javanna Apr 21, 2017

Member

ok I am totally fine with removing the check for now and reintroducing it later once we have two different code-paths.

Copy link
Contributor

left a comment

I think it'd be nice to have an upgrade test that creates templates but I think this gets the job done and we can build that test later.

template.endObject();
Map<String, String> options = new HashMap<>();
if (randomBoolean()) {
options.put(Script.CONTENT_TYPE_OPTION, xContentType.mediaType());

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 21, 2017

Contributor

Maybe add a comment here about how we don't really like the content_type option but we have to support it for 5.3.0 and 5.3.1?

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 21, 2017
In elastic#24251 we fix an issue with stored search templates that
this test would have discovered: stored search templates cause
the node to refuse to start. Technically a "restart" test would
have caught this as well and would have caught it more quickly.
But we already *have* an upgrade test and we don't have restart tests.
And testing this on upgrade is a good thing too.
@colings86

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2017

I'm away from my laptop now so unless anyone else can/wants to merge and back port this PR I'll do it first thing Monday morning.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2017

I'm away from my laptop now so unless anyone else can/wants to merge and back port this PR I'll do it first thing Monday morning.

I've kicked off a test run of this and #24258 locally. If it passes I'll merge and start the backport dance. But I'm going to be in and out all day so it probably won't be until tomorrow when I finish it.

@nik9000 nik9000 merged commit d4a6ba8 into elastic:master Apr 22, 2017
2 checks passed
2 checks passed
CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details
nik9000 added a commit that referenced this pull request Apr 22, 2017
In #24251 we fix an issue with stored search templates that
this test would have discovered: stored search templates cause
the node to refuse to start. Technically a "restart" test would
have caught this as well and would have caught it more quickly.
But we already *have* an upgrade test and we don't have restart tests.
And testing this on upgrade is a good thing too.
nik9000 added a commit that referenced this pull request Apr 22, 2017
…24251)

When parsing StoredSearchScript we were adding a Content type option that was forbidden (by a check that threw an exception) by the parser thats used to parse the template when we read it from the cluster state. This was stopping Elastisearch from starting after stored search templates had been added.

This change no longer adds the content type option to the StoredScriptSource object when parsing from the put search template request.  This is safe because the StoredScriptSource content is always JSON when its stored in the cluster state since we do a conversion to JSON before this point.

Also removes the check for the content type in the options when parsing StoredScriptSource so users who already have stored scripts can start Elasticsearch.

Closes #24227
nik9000 added a commit that referenced this pull request Apr 22, 2017
In #24251 we fix an issue with stored search templates that
this test would have discovered: stored search templates cause
the node to refuse to start. Technically a "restart" test would
have caught this as well and would have caught it more quickly.
But we already *have* an upgrade test and we don't have restart tests.
And testing this on upgrade is a good thing too.
nik9000 added a commit that referenced this pull request Apr 22, 2017
In #24251 we fix an issue with stored search templates that
this test would have discovered: stored search templates cause
the node to refuse to start. Technically a "restart" test would
have caught this as well and would have caught it more quickly.
But we already *have* an upgrade test and we don't have restart tests.
And testing this on upgrade is a good thing too.
nik9000 added a commit that referenced this pull request Apr 22, 2017
…24251)

When parsing StoredSearchScript we were adding a Content type option that was forbidden (by a check that threw an exception) by the parser thats used to parse the template when we read it from the cluster state. This was stopping Elastisearch from starting after stored search templates had been added.

This change no longer adds the content type option to the StoredScriptSource object when parsing from the put search template request.  This is safe because the StoredScriptSource content is always JSON when its stored in the cluster state since we do a conversion to JSON before this point.

Also removes the check for the content type in the options when parsing StoredScriptSource so users who already have stored scripts can start Elasticsearch.

Closes #24227
nik9000 added a commit that referenced this pull request Apr 22, 2017
…24251)

When parsing StoredSearchScript we were adding a Content type option that was forbidden (by a check that threw an exception) by the parser thats used to parse the template when we read it from the cluster state. This was stopping Elastisearch from starting after stored search templates had been added.

This change no longer adds the content type option to the StoredScriptSource object when parsing from the put search template request.  This is safe because the StoredScriptSource content is always JSON when its stored in the cluster state since we do a conversion to JSON before this point.

Also removes the check for the content type in the options when parsing StoredScriptSource so users who already have stored scripts can start Elasticsearch.

Closes #24227
nik9000 added a commit that referenced this pull request Apr 22, 2017
In #24251 we fix an issue with stored search templates that
this test would have discovered: stored search templates cause
the node to refuse to start. Technically a "restart" test would
have caught this as well and would have caught it more quickly.
But we already *have* an upgrade test and we don't have restart tests.
And testing this on upgrade is a good thing too.
@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2017

All backported.

asettouf added a commit to asettouf/elasticsearch that referenced this pull request Apr 23, 2017
…lastic#24251)

When parsing StoredSearchScript we were adding a Content type option that was forbidden (by a check that threw an exception) by the parser thats used to parse the template when we read it from the cluster state. This was stopping Elastisearch from starting after stored search templates had been added.

This change no longer adds the content type option to the StoredScriptSource object when parsing from the put search template request.  This is safe because the StoredScriptSource content is always JSON when its stored in the cluster state since we do a conversion to JSON before this point.

Also removes the check for the content type in the options when parsing StoredScriptSource so users who already have stored scripts can start Elasticsearch.

Closes elastic#24227
asettouf added a commit to asettouf/elasticsearch that referenced this pull request Apr 23, 2017
In elastic#24251 we fix an issue with stored search templates that
this test would have discovered: stored search templates cause
the node to refuse to start. Technically a "restart" test would
have caught this as well and would have caught it more quickly.
But we already *have* an upgrade test and we don't have restart tests.
And testing this on upgrade is a good thing too.
@colings86

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

@nik9000 thanks very much for doing this

@colings86 colings86 deleted the colings86:fix/24227 branch Apr 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.