From 690039914412e8c04ea86d775554267635c15bb5 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sat, 6 Apr 2019 17:23:51 -0400 Subject: [PATCH] Be lenient when parsing build flavor and type on the wire (#40734) Today we are strict when parsing build flavor and types off the wire. This means that if a later version introduces a new build flavor or type, an older version would not be able to parse what that new version is sending. For a practical example of this, we recently added the build type "docker", and this means that in a rolling upgrade scenario older nodes would not be able to understand the build type that the newer node is sending. This breaks clusters and is bad. We do not normally think of adding a new enumeration value as being a serialization breaking change, it is just not a lesson that we have learned before. We should be lenient here though, so that we can add future changes without running the risk of breaking ourselves horribly. It is either that, or we have super-strict testing infrastructure here yet still I fear the possibility of mistakes. This commit changes the parsing of build flavor and build type so that we are still strict at startup, yet we are lenient with values coming across the wire. This will help avoid us breaking rolling upgrades, or clients that are on an older version. --- .../main/java/org/elasticsearch/Build.java | 29 +++++++++---- .../action/main/MainResponse.java | 8 +++- .../java/org/elasticsearch/BuildTests.java | 42 +++++++++++++++++++ 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/Build.java b/server/src/main/java/org/elasticsearch/Build.java index be37c56837d70..1b1cd8d3e720a 100644 --- a/server/src/main/java/org/elasticsearch/Build.java +++ b/server/src/main/java/org/elasticsearch/Build.java @@ -57,7 +57,7 @@ public String displayName() { return displayName; } - public static Flavor fromDisplayName(final String displayName) { + public static Flavor fromDisplayName(final String displayName, final boolean strict) { switch (displayName) { case "default": return Flavor.DEFAULT; @@ -66,7 +66,12 @@ public static Flavor fromDisplayName(final String displayName) { case "unknown": return Flavor.UNKNOWN; default: - throw new IllegalStateException("unexpected distribution flavor [" + displayName + "]; your distribution is broken"); + if (strict) { + final String message = "unexpected distribution flavor [" + displayName + "]; your distribution is broken"; + throw new IllegalStateException(message); + } else { + return Flavor.UNKNOWN; + } } } @@ -91,7 +96,7 @@ public String displayName() { this.displayName = displayName; } - public static Type fromDisplayName(final String displayName) { + public static Type fromDisplayName(final String displayName, final boolean strict) { switch (displayName) { case "deb": return Type.DEB; @@ -106,9 +111,14 @@ public static Type fromDisplayName(final String displayName) { case "unknown": return Type.UNKNOWN; default: - throw new IllegalStateException("unexpected distribution type [" + displayName + "]; your distribution is broken"); + if (strict) { + throw new IllegalStateException("unexpected distribution type [" + displayName + "]; your distribution is broken"); + } else { + return Type.UNKNOWN; + } } } + } static { @@ -119,8 +129,9 @@ public static Type fromDisplayName(final String displayName) { final boolean isSnapshot; final String version; - flavor = Flavor.fromDisplayName(System.getProperty("es.distribution.flavor", "unknown")); - type = Type.fromDisplayName(System.getProperty("es.distribution.type", "unknown")); + // these are parsed at startup, and we require that we are able to recognize the values passed in by the startup scripts + flavor = Flavor.fromDisplayName(System.getProperty("es.distribution.flavor", "unknown"), true); + type = Type.fromDisplayName(System.getProperty("es.distribution.type", "unknown"), true); final String esPrefix = "elasticsearch-" + Version.CURRENT; final URL url = getElasticsearchCodeSourceLocation(); @@ -214,12 +225,14 @@ public static Build readBuild(StreamInput in) throws IOException { final Flavor flavor; final Type type; if (in.getVersion().onOrAfter(Version.V_6_3_0)) { - flavor = Flavor.fromDisplayName(in.readString()); + // be lenient when reading on the wire, the enumeration values from other versions might be different than what we know + flavor = Flavor.fromDisplayName(in.readString(), false); } else { flavor = Flavor.OSS; } if (in.getVersion().onOrAfter(Version.V_6_3_0)) { - type = Type.fromDisplayName(in.readString()); + // be lenient when reading on the wire, the enumeration values from other versions might be different than what we know + type = Type.fromDisplayName(in.readString(), false); } else { type = Type.UNKNOWN; } diff --git a/server/src/main/java/org/elasticsearch/action/main/MainResponse.java b/server/src/main/java/org/elasticsearch/action/main/MainResponse.java index 38d78fdc0c1a1..8b0e5c744e569 100644 --- a/server/src/main/java/org/elasticsearch/action/main/MainResponse.java +++ b/server/src/main/java/org/elasticsearch/action/main/MainResponse.java @@ -135,8 +135,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws final String buildType = (String) value.get("build_type"); response.build = new Build( - buildFlavor == null ? Build.Flavor.UNKNOWN : Build.Flavor.fromDisplayName(buildFlavor), - buildType == null ? Build.Type.UNKNOWN : Build.Type.fromDisplayName(buildType), + /* + * Be lenient when reading on the wire, the enumeration values from other versions might be different than what + * we know. + */ + buildFlavor == null ? Build.Flavor.UNKNOWN : Build.Flavor.fromDisplayName(buildFlavor, false), + buildType == null ? Build.Type.UNKNOWN : Build.Type.fromDisplayName(buildType, false), (String) value.get("build_hash"), (String) value.get("build_date"), (boolean) value.get("build_snapshot"), diff --git a/server/src/test/java/org/elasticsearch/BuildTests.java b/server/src/test/java/org/elasticsearch/BuildTests.java index f1d48c08b3953..e0d8140c708d6 100644 --- a/server/src/test/java/org/elasticsearch/BuildTests.java +++ b/server/src/test/java/org/elasticsearch/BuildTests.java @@ -35,7 +35,10 @@ import java.util.Set; import java.util.stream.Collectors; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasToString; +import static org.hamcrest.Matchers.sameInstance; public class BuildTests extends ESTestCase { @@ -223,4 +226,43 @@ public void testSerializationBWC() throws IOException { assertThat(post67pre70.build.getQualifiedVersion(), equalTo(post67Pre70Version.toString())); assertThat(post70.build.getQualifiedVersion(), equalTo(dockerBuild.build.getQualifiedVersion())); } + + public void testFlavorParsing() { + for (final Build.Flavor flavor : Build.Flavor.values()) { + // strict or not should not impact parsing at all here + assertThat(Build.Flavor.fromDisplayName(flavor.displayName(), randomBoolean()), sameInstance(flavor)); + } + } + + public void testTypeParsing() { + for (final Build.Type type : Build.Type.values()) { + // strict or not should not impact parsing at all here + assertThat(Build.Type.fromDisplayName(type.displayName(), randomBoolean()), sameInstance(type)); + } + } + + public void testLenientFlavorParsing() { + final String displayName = randomAlphaOfLength(8); + assertThat(Build.Flavor.fromDisplayName(displayName, false), equalTo(Build.Flavor.UNKNOWN)); + } + + public void testStrictFlavorParsing() { + final String displayName = randomAlphaOfLength(8); + @SuppressWarnings("ResultOfMethodCallIgnored") final IllegalStateException e = + expectThrows(IllegalStateException.class, () -> Build.Flavor.fromDisplayName(displayName, true)); + assertThat(e, hasToString(containsString("unexpected distribution flavor [" + displayName + "]; your distribution is broken"))); + } + + public void testLenientTypeParsing() { + final String displayName = randomAlphaOfLength(8); + assertThat(Build.Type.fromDisplayName(displayName, false), equalTo(Build.Type.UNKNOWN)); + } + + public void testStrictTypeParsing() { + final String displayName = randomAlphaOfLength(8); + @SuppressWarnings("ResultOfMethodCallIgnored") final IllegalStateException e = + expectThrows(IllegalStateException.class, () -> Build.Type.fromDisplayName(displayName, true)); + assertThat(e, hasToString(containsString("unexpected distribution type [" + displayName + "]; your distribution is broken"))); + } + }