From 710876b3b10bdbe57c885463a861ab00b8119cc8 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 2 Apr 2019 09:05:24 -0400 Subject: [PATCH 1/4] Be lenient when parsing build flavor and type on the wire 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 | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/Build.java b/server/src/main/java/org/elasticsearch/Build.java index be37c56837d70..809c922042c3d 100644 --- a/server/src/main/java/org/elasticsearch/Build.java +++ b/server/src/main/java/org/elasticsearch/Build.java @@ -58,6 +58,14 @@ public String displayName() { } public static Flavor fromDisplayName(final String displayName) { + try { + return strictFromDisplayName(displayName); + } catch (final IllegalStateException e) { + return Flavor.UNKNOWN; + } + } + + public static Flavor strictFromDisplayName(final String displayName) { switch (displayName) { case "default": return Flavor.DEFAULT; @@ -92,6 +100,14 @@ public String displayName() { } public static Type fromDisplayName(final String displayName) { + try { + return strictFromDisplayName(displayName); + } catch (final IllegalStateException e) { + return Type.UNKNOWN; + } + } + + public static Type strictFromDisplayName(final String displayName) { switch (displayName) { case "deb": return Type.DEB; @@ -109,6 +125,7 @@ public static Type fromDisplayName(final String displayName) { throw new IllegalStateException("unexpected distribution type [" + displayName + "]; your distribution is broken"); } } + } static { @@ -119,8 +136,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.strictFromDisplayName(System.getProperty("es.distribution.flavor", "unknown")); + type = Type.strictFromDisplayName(System.getProperty("es.distribution.type", "unknown")); final String esPrefix = "elasticsearch-" + Version.CURRENT; final URL url = getElasticsearchCodeSourceLocation(); From 766243f6846c2fcb65dbf38dc6f00b4df8bc331c Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 2 Apr 2019 09:13:00 -0400 Subject: [PATCH 2/4] Refactor --- .../main/java/org/elasticsearch/Build.java | 42 ++++++++----------- .../action/main/MainResponse.java | 8 +++- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/Build.java b/server/src/main/java/org/elasticsearch/Build.java index 809c922042c3d..512c4c647f401 100644 --- a/server/src/main/java/org/elasticsearch/Build.java +++ b/server/src/main/java/org/elasticsearch/Build.java @@ -57,15 +57,7 @@ public String displayName() { return displayName; } - public static Flavor fromDisplayName(final String displayName) { - try { - return strictFromDisplayName(displayName); - } catch (final IllegalStateException e) { - return Flavor.UNKNOWN; - } - } - - public static Flavor strictFromDisplayName(final String displayName) { + public static Flavor fromDisplayName(final String displayName, final boolean strict) { switch (displayName) { case "default": return Flavor.DEFAULT; @@ -74,7 +66,11 @@ public static Flavor strictFromDisplayName(final String displayName) { case "unknown": return Flavor.UNKNOWN; default: - throw new IllegalStateException("unexpected distribution flavor [" + displayName + "]; your distribution is broken"); + if (strict) { + throw new IllegalStateException("unexpected distribution flavor [" + displayName + "]; your distribution is broken"); + } else { + return Flavor.UNKNOWN; + } } } @@ -99,15 +95,7 @@ public String displayName() { this.displayName = displayName; } - public static Type fromDisplayName(final String displayName) { - try { - return strictFromDisplayName(displayName); - } catch (final IllegalStateException e) { - return Type.UNKNOWN; - } - } - - public static Type strictFromDisplayName(final String displayName) { + public static Type fromDisplayName(final String displayName, final boolean strict) { switch (displayName) { case "deb": return Type.DEB; @@ -122,7 +110,11 @@ public static Type strictFromDisplayName(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; + } } } @@ -137,8 +129,8 @@ public static Type strictFromDisplayName(final String displayName) { final String version; // these are parsed at startup, and we require that we are able to recognize the values passed in by the startup scripts - flavor = Flavor.strictFromDisplayName(System.getProperty("es.distribution.flavor", "unknown")); - type = Type.strictFromDisplayName(System.getProperty("es.distribution.type", "unknown")); + 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(); @@ -232,12 +224,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 dc54a112f6779..8485c47c8c79b 100644 --- a/server/src/main/java/org/elasticsearch/action/main/MainResponse.java +++ b/server/src/main/java/org/elasticsearch/action/main/MainResponse.java @@ -129,8 +129,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"), From 5ea8cfd8dd1017ff1850dafebbe0b0ed346cf0b6 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 2 Apr 2019 09:14:08 -0400 Subject: [PATCH 3/4] Checkstyle --- server/src/main/java/org/elasticsearch/Build.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/Build.java b/server/src/main/java/org/elasticsearch/Build.java index 512c4c647f401..1b1cd8d3e720a 100644 --- a/server/src/main/java/org/elasticsearch/Build.java +++ b/server/src/main/java/org/elasticsearch/Build.java @@ -67,7 +67,8 @@ public static Flavor fromDisplayName(final String displayName, final boolean str return Flavor.UNKNOWN; default: if (strict) { - throw new IllegalStateException("unexpected distribution flavor [" + displayName + "]; your distribution is broken"); + final String message = "unexpected distribution flavor [" + displayName + "]; your distribution is broken"; + throw new IllegalStateException(message); } else { return Flavor.UNKNOWN; } From ac79f171b1f34b15e94d83699ee7be90607baf2e Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 2 Apr 2019 10:37:42 -0400 Subject: [PATCH 4/4] Add tests --- .../java/org/elasticsearch/BuildTests.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/BuildTests.java b/server/src/test/java/org/elasticsearch/BuildTests.java index 12af1d31841cf..b4e859c3698f5 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 { @@ -222,4 +225,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"))); + } + }