transport.publish_address should contain CNAME (#45626)

Andrey Ershov · Andrey Ershov · commit dbc90653dcba · 2019-08-16T17:42:00.000+02:00
This commit adds CNAME reporting for transport.publish_address same way it's done for http.publish_address. Relates #32806 Relates #39970 (cherry picked from commit e0a2558)
diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
@@ -894,6 +894,9 @@ class BuildPlugin implements Plugin<Project> {
                 // TODO: remove this once ctx isn't added to update script params in 7.0
                 test.systemProperty 'es.scripting.update.ctx_in_params', 'false'
 
+                // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0
+                test.systemProperty 'es.transport.cname_in_publish_address', 'true'
+
                 test.testLogging { TestLoggingContainer logging ->
                     logging.showExceptions = true
                     logging.showCauses = true
diff --git a/docs/build.gradle b/docs/build.gradle
@@ -59,6 +59,9 @@ testClusters.integTest {
   extraConfigFile 'hunspell/en_US/en_US.dic', project(":server").file('src/test/resources/indices/analyze/conf_dir/hunspell/en_US/en_US.dic')
   // Whitelist reindexing from the local node so we can test it.
   setting 'reindex.remote.whitelist', '127.0.0.1:*'
+
+  // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0
+  systemProperty 'es.transport.cname_in_publish_address', 'true'
 }
 
 // build the cluster with all plugins
diff --git a/modules/lang-painless/build.gradle b/modules/lang-painless/build.gradle
@@ -28,6 +28,8 @@ esplugin {
 testClusters.integTest {
   module file(project(':modules:mapper-extras').tasks.bundlePlugin.archiveFile)
   systemProperty 'es.scripting.update.ctx_in_params', 'false'
+  // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0
+  systemProperty 'es.transport.cname_in_publish_address', 'true'
 }
 
 dependencies {
diff --git a/server/src/main/java/org/elasticsearch/common/transport/BoundTransportAddress.java b/server/src/main/java/org/elasticsearch/common/transport/BoundTransportAddress.java
@@ -22,6 +22,7 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
+import org.elasticsearch.common.network.InetAddresses;
 
 import java.io.IOException;
 
@@ -75,7 +76,12 @@ public void writeTo(StreamOutput out) throws IOException {
     @Override
     public String toString() {
         StringBuilder builder = new StringBuilder("publish_address {");
-        builder.append(publishAddress);
+        String hostString = publishAddress.address().getHostString();
+        String publishAddressString = publishAddress.toString();
+        if (InetAddresses.isInetAddress(hostString) == false) {
+            publishAddressString = hostString + '/' + publishAddress.toString();
+        }
+        builder.append(publishAddressString);
         builder.append("}, bound_addresses ");
         boolean firstAdded = false;
         for (TransportAddress address : boundAddresses) {
diff --git a/server/src/main/java/org/elasticsearch/transport/TransportInfo.java b/server/src/main/java/org/elasticsearch/transport/TransportInfo.java
@@ -19,26 +19,45 @@
 
 package org.elasticsearch.transport;
 
+import org.apache.logging.log4j.LogManager;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.network.InetAddresses;
 import org.elasticsearch.common.transport.BoundTransportAddress;
+import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.elasticsearch.common.Booleans.parseBoolean;
+
 public class TransportInfo implements Writeable, ToXContentFragment {
 
+    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(TransportInfo.class));
+
+    /** Whether to add hostname to publish host field when serializing. */
+    private static final boolean CNAME_IN_PUBLISH_ADDRESS =
+            parseBoolean(System.getProperty("es.transport.cname_in_publish_address"), false);
+
     private BoundTransportAddress address;
     private Map<String, BoundTransportAddress> profileAddresses;
+    private final boolean cnameInPublishAddress;
 
     public TransportInfo(BoundTransportAddress address, @Nullable Map<String, BoundTransportAddress> profileAddresses) {
+        this(address, profileAddresses, CNAME_IN_PUBLISH_ADDRESS);
+    }
+
+    public TransportInfo(BoundTransportAddress address, @Nullable Map<String, BoundTransportAddress> profileAddresses,
+                         boolean cnameInPublishAddress) {
         this.address = address;
         this.profileAddresses = profileAddresses;
+        this.cnameInPublishAddress = cnameInPublishAddress;
     }
 
     public TransportInfo(StreamInput in) throws IOException {
@@ -52,6 +71,7 @@ public TransportInfo(StreamInput in) throws IOException {
                 profileAddresses.put(key, value);
             }
         }
+        this.cnameInPublishAddress = CNAME_IN_PUBLISH_ADDRESS;
     }
 
     @Override
@@ -77,17 +97,35 @@ static final class Fields {
         static final String PROFILES = "profiles";
     }
 
+    private String formatPublishAddressString(String propertyName, TransportAddress publishAddress){
+        String publishAddressString = publishAddress.toString();
+        String hostString = publishAddress.address().getHostString();
+        if (InetAddresses.isInetAddress(hostString) == false) {
+            if (cnameInPublishAddress) {
+                publishAddressString = hostString + '/' + publishAddress.toString();
+            } else {
+                deprecationLogger.deprecated(
+                        propertyName + " was printed as [ip:port] instead of [hostname/ip:port]. "
+                                + "This format is deprecated and will change to [hostname/ip:port] in a future version. "
+                                + "Use -Des.transport.cname_in_publish_address=true to enforce non-deprecated formatting."
+                );
+            }
+        }
+        return publishAddressString;
+    }
+
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject(Fields.TRANSPORT);
         builder.array(Fields.BOUND_ADDRESS, (Object[]) address.boundAddresses());
-        builder.field(Fields.PUBLISH_ADDRESS, address.publishAddress().toString());
+        builder.field(Fields.PUBLISH_ADDRESS, formatPublishAddressString("transport.publish_address", address.publishAddress()));
         builder.startObject(Fields.PROFILES);
         if (profileAddresses != null && profileAddresses.size() > 0) {
             for (Map.Entry<String, BoundTransportAddress> entry : profileAddresses.entrySet()) {
                 builder.startObject(entry.getKey());
                 builder.array(Fields.BOUND_ADDRESS, (Object[]) entry.getValue().boundAddresses());
-                builder.field(Fields.PUBLISH_ADDRESS, entry.getValue().publishAddress().toString());
+                String propertyName = "transport." + entry.getKey() + ".publish_address";
+                builder.field(Fields.PUBLISH_ADDRESS, formatPublishAddressString(propertyName, entry.getValue().publishAddress()));
                 builder.endObject();
             }
         }
diff --git a/server/src/test/java/org/elasticsearch/transport/TransportInfoTests.java b/server/src/test/java/org/elasticsearch/transport/TransportInfoTests.java
@@ -0,0 +1,107 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.transport;
+
+import org.elasticsearch.common.network.NetworkAddress;
+import org.elasticsearch.common.transport.BoundTransportAddress;
+import org.elasticsearch.common.transport.TransportAddress;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.test.ESTestCase;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.util.Collections;
+import java.util.Map;
+
+public class TransportInfoTests extends ESTestCase {
+
+    private TransportInfo createTransportInfo(InetAddress address, int port, boolean cnameInPublishAddress) {
+        BoundTransportAddress boundAddress = new BoundTransportAddress(
+                new TransportAddress[]{new TransportAddress(address, port)},
+                new TransportAddress(address, port)
+        );
+        Map<String, BoundTransportAddress> profiles = Collections.singletonMap("test_profile", boundAddress);
+        return new TransportInfo(boundAddress, profiles, cnameInPublishAddress);
+    }
+
+    public void testCorrectlyDisplayPublishedCname() throws Exception {
+        InetAddress address = InetAddress.getByName("localhost");
+        int port = 9200;
+        assertPublishAddress(
+            createTransportInfo(address, port,true),
+            "localhost/" + NetworkAddress.format(address) + ':' + port
+        );
+    }
+
+    public void testHideCnameIfDeprecatedFormat() throws Exception {
+        InetAddress address = InetAddress.getByName("localhost");
+        int port = 9200;
+        assertPublishAddress(
+                createTransportInfo(address, port,false),
+                NetworkAddress.format(address) + ':' + port
+        );
+        assertWarnings("transport.publish_address was printed as [ip:port] instead of [hostname/ip:port]. " +
+                "This format is deprecated and will change to [hostname/ip:port] in a future version. " +
+                "Use -Des.transport.cname_in_publish_address=true to enforce non-deprecated formatting.",
+
+                "transport.test_profile.publish_address was printed as [ip:port] instead of [hostname/ip:port]. " +
+                "This format is deprecated and will change to [hostname/ip:port] in a future version. " +
+                "Use -Des.transport.cname_in_publish_address=true to enforce non-deprecated formatting.");
+    }
+
+    public void testCorrectDisplayPublishedIp() throws Exception {
+        InetAddress address = InetAddress.getByName(NetworkAddress.format(InetAddress.getByName("localhost")));
+        int port = 9200;
+        assertPublishAddress(
+                createTransportInfo(address, port,true),
+                NetworkAddress.format(address) + ':' + port
+        );
+    }
+
+    public void testCorrectDisplayPublishedIpv6() throws Exception {
+        InetAddress address = InetAddress.getByName(NetworkAddress.format(InetAddress.getByName("0:0:0:0:0:0:0:1")));
+        int port = 9200;
+        assertPublishAddress(
+                createTransportInfo(address, port,true),
+                new TransportAddress(address, port).toString()
+        );
+    }
+
+    @SuppressWarnings("unchecked")
+    private void assertPublishAddress(TransportInfo httpInfo, String expected) throws IOException {
+        XContentBuilder builder = XContentFactory.jsonBuilder();
+        builder.startObject();
+        httpInfo.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        builder.endObject();
+
+        Map<String, Object> transportMap = (Map<String, Object>) createParser(builder).map().get(TransportInfo.Fields.TRANSPORT);
+        Map<String, Object> profilesMap = (Map<String, Object>) transportMap.get("profiles");
+        assertEquals(
+            expected,
+            transportMap.get(TransportInfo.Fields.PUBLISH_ADDRESS)
+        );
+        assertEquals(
+                expected,
+                ((Map<String, Object>)profilesMap.get("test_profile")).get(TransportInfo.Fields.PUBLISH_ADDRESS)
+        );
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,9 @@ testClusters.integTest {`
`59`	`59`	`extraConfigFile 'hunspell/en_US/en_US.dic', project(":server").file('src/test/resources/indices/analyze/conf_dir/hunspell/en_US/en_US.dic')`
`60`	`60`	`// Whitelist reindexing from the local node so we can test it.`
`61`	`61`	`setting 'reindex.remote.whitelist', '127.0.0.1:*'`
	`62`	`+`
	`63`	`+ // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0`
	`64`	`+ systemProperty 'es.transport.cname_in_publish_address', 'true'`
`62`	`65`	`}`
`63`	`66`
`64`	`67`	`// build the cluster with all plugins`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,8 @@ esplugin {`
`28`	`28`	`testClusters.integTest {`
`29`	`29`	`module file(project(':modules:mapper-extras').tasks.bundlePlugin.archiveFile)`
`30`	`30`	`systemProperty 'es.scripting.update.ctx_in_params', 'false'`
	`31`	`+ // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0`
	`32`	`+ systemProperty 'es.transport.cname_in_publish_address', 'true'`
`31`	`33`	`}`
`32`	`34`
`33`	`35`	`dependencies {`