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.

Author: jasontedor

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;
         }

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"),

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")));
+    }
+
 }