Repository: hazelcast/hazelcast-kubernetes (java)

Issue:
NodePort Service Requires Access to /nodes Kubernetes Resource
I'm trying to configure Hazelcast clients outside the Kubernetes cluster where our service is running embedded Hazelcast nodes. We are running in a shared Kubernetes cluster, where we have access only to our own namespaces. We do not have access to LoadBalancer services, so I'm using NodePort services to expose Hazelcast to the clients.

The service account I'm using currently does not have access to the `/nodes` resource. This means that the Hazelcast client connection fails in `KubernetesClient.enrichWithPublicAddresses` when calling `extractNodePublicIp(callGet(nodeUrl))`; the `callGet` method fails with HTTP 403 Forbidden.

I guess someone will say that this is exactly according to design and documentation; the service account should have access to the `/nodes` resource. However, at lesat in our case there is a very simple workaround; we can simply use the node name as public name.

To implement this requires an additional try-catch in `KubernetesClient.enrichWithPublicAddresses`:

    String nodeUrl = String.format("%s/api/v1/nodes/%s", kubernetesMaster, node);
    nodePublicIp = extractNodePublicIp(callGet(nodeUrl));

becomes:

    try {
        String nodeUrl = String.format("%s/api/v1/nodes/%s", kubernetesMaster, node);
        nodePublicIp = extractNodePublicIp(callGet(nodeUrl));
    } catch (Exception e2) {
        LOGGER.warning("Failed to find public IP for node, using node name instead: " + node);
        nodePublicIp = node;
    }

Does anyone see any major problem with this change, or could it added in a future release? Should I in that case create a pull request?


Pull request description:
#146: Added boolean parameter use-node-name-as-external-address, defa…
Pull request for issue #146; this adds the flag use-node-name-as-external-address to skip looking up the external IP using the /nodes resource.

fix #146 

Patch:
diff --git a/.gitignore b/.gitignore
index 134be63..04e6eef 100644
--- a/.gitignore
+++ b/.gitignore
@@ -12,3 +12,5 @@ build/
 *.iws
 .DS_Store
 atlassian-ide-plugin.xml
+.checkstyle
+.fbExcludeFilterFile
diff --git a/README.md b/README.md
index 7cfc459..c7edc5d 100644
--- a/README.md
+++ b/README.md
@@ -119,11 +119,12 @@ config.getNetworkConfig().getJoin().getKubernetesConfig().setEnabled(true)
       .setProperty("service-name", "MY-SERVICE-NAME");
 ```
 
-Here are the properties to configure the plugin, all of them are optional.
+There are several properties to configure the plugin, all of them are optional.
  * `namespace`: Kubernetes Namespace where Hazelcast is running; if not specified, the value is taken from the environment variables `KUBERNETES_NAMESPACE` or `OPENSHIFT_BUILD_NAMESPACE`
  * `service-name`: service name used to scan only PODs connected to the given service; if not specified, then all PODs in the namespace are checked
  * `service-label-name`, `service-label-value`: service label and value used to tag services that should form the Hazelcast cluster together
  * `resolve-not-ready-addresses`: if set to `true`, it checks also the addresses of PODs which are not ready; `false` by default
+ * `use-node-name-as-external-address`: if set to `true`, uses the node name to connect to a `NodePort` service instead of looking up the external IP using the API; `false` by default
  * `kubernetes-api-retries`: number of retries in case of issues while connecting to Kubernetes API; defaults to `3` 
  * `kubernetes-master`: URL of Kubernetes Master; `https://kubernetes.default.svc` by default
  * `api-token`: API Token to Kubernetes API; if not specified, the value is taken from the file `/var/run/secrets/kubernetes.io/serviceaccount/token`
diff --git a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java
index 77e70b2..c5a7f14 100644
--- a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java
+++ b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java
@@ -56,7 +56,7 @@ final class HazelcastKubernetesDiscoveryStrategy
 
     private static KubernetesClient buildKubernetesClient(KubernetesConfig config) {
         return new KubernetesClient(config.getNamespace(), config.getKubernetesMasterUrl(), config.getKubernetesApiToken(),
-                config.getKubernetesCaCertificate(), config.getKubernetesApiRetries());
+                config.getKubernetesCaCertificate(), config.getKubernetesApiRetries(), config.isUseNodeNameAsExternalAddress());
     }
 
     public void start() {
diff --git a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java
index d8f9308..5f3a7d0 100644
--- a/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java
+++ b/src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategyFactory.java
@@ -44,6 +44,7 @@ public class HazelcastKubernetesDiscoveryStrategyFactory
                 KubernetesProperties.SERVICE_LABEL_VALUE,
                 KubernetesProperties.NAMESPACE,
                 KubernetesProperties.RESOLVE_NOT_READY_ADDRESSES,
+                KubernetesProperties.USE_NODE_NAME_AS_EXTERNAL_ADDRESS,
                 KubernetesProperties.KUBERNETES_API_RETIRES,
                 KubernetesProperties.KUBERNETES_MASTER_URL,
                 KubernetesProperties.KUBERNETES_API_TOKEN,
diff --git a/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java b/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java
index 6a9ba01..bb0af5a 100644
--- a/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java
+++ b/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java
@@ -40,6 +40,7 @@ import static java.util.Collections.emptyList;
  *
  * @see <a href="https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11/">Kubernetes API</a>
  */
+@SuppressWarnings("checkstyle:methodcount")
 class KubernetesClient {
     private static final ILogger LOGGER = Logger.getLogger(KubernetesClient.class);
 
@@ -53,15 +54,18 @@ class KubernetesClient {
     private final String apiToken;
     private final String caCertificate;
     private final int retries;
+    private boolean useNodeNameAsExternalAddress;
 
     private boolean isNoPublicIpAlreadyLogged;
 
-    KubernetesClient(String namespace, String kubernetesMaster, String apiToken, String caCertificate, int retries) {
+    KubernetesClient(String namespace, String kubernetesMaster, String apiToken, String caCertificate, int retries,
+            boolean useNodeNameAsExternalAddress) {
         this.namespace = namespace;
         this.kubernetesMaster = kubernetesMaster;
         this.apiToken = apiToken;
         this.caCertificate = caCertificate;
         this.retries = retries;
+        this.useNodeNameAsExternalAddress = useNodeNameAsExternalAddress;
     }
 
     /**
@@ -296,15 +300,14 @@ class KubernetesClient {
                     // Load Balancer public IP cannot be found, try using NodePort.
                     Integer nodePort = extractNodePort(serviceJson);
                     String node = nodes.get(privateAddress);
-                    String nodePublicIp;
+                    String nodePublicAddress;
                     if (cachedNodePublicIps.containsKey(node)) {
-                        nodePublicIp = cachedNodePublicIps.get(node);
+                        nodePublicAddress = cachedNodePublicIps.get(node);
                     } else {
-                        String nodeUrl = String.format("%s/api/v1/nodes/%s", kubernetesMaster, node);
-                        nodePublicIp = extractNodePublicIp(callGet(nodeUrl));
-                        cachedNodePublicIps.put(node, nodePublicIp);
+                        nodePublicAddress = externalAddressForNode(node);
+                        cachedNodePublicIps.put(node, nodePublicAddress);
                     }
-                    publicIps.put(privateAddress, nodePublicIp);
+                    publicIps.put(privateAddress, nodePublicAddress);
                     publicPorts.put(privateAddress, nodePort);
                 }
             }
@@ -422,6 +425,18 @@ class KubernetesClient {
         return ports.get(0).asObject().get("nodePort").asInt();
     }
 
+    private String externalAddressForNode(String node) {
+        String nodeExternalAddress;
+        if (useNodeNameAsExternalAddress) {
+            LOGGER.info("Using node name instead of public IP for node, must be available from client: " + node);
+            nodeExternalAddress = node;
+        } else {
+            String nodeUrl = String.format("%s/api/v1/nodes/%s", kubernetesMaster, node);
+            nodeExternalAddress = extractNodePublicIp(callGet(nodeUrl));
+        }
+        return nodeExternalAddress;
+    }
+
     private static String extractNodePublicIp(JsonObject nodeJson) {
         for (JsonValue address : toJsonArray(nodeJson.get("status").asObject().get("addresses"))) {
             if ("ExternalIP".equals(address.asObject().get("type").asString())) {
diff --git a/src/main/java/com/hazelcast/kubernetes/KubernetesConfig.java b/src/main/java/com/hazelcast/kubernetes/KubernetesConfig.java
index 874bfc3..313827e 100644
--- a/src/main/java/com/hazelcast/kubernetes/KubernetesConfig.java
+++ b/src/main/java/com/hazelcast/kubernetes/KubernetesConfig.java
@@ -41,6 +41,7 @@ import static com.hazelcast.kubernetes.KubernetesProperties.SERVICE_LABEL_NAME;
 import static com.hazelcast.kubernetes.KubernetesProperties.SERVICE_LABEL_VALUE;
 import static com.hazelcast.kubernetes.KubernetesProperties.SERVICE_NAME;
 import static com.hazelcast.kubernetes.KubernetesProperties.SERVICE_PORT;
+import static com.hazelcast.kubernetes.KubernetesProperties.USE_NODE_NAME_AS_EXTERNAL_ADDRESS;
 
 /**
  * Responsible for fetching, parsing, and validating Hazelcast Kubernetes Discovery Strategy input properties.
@@ -60,6 +61,7 @@ final class KubernetesConfig {
     private final String serviceLabelValue;
     private final String namespace;
     private final boolean resolveNotReadyAddresses;
+    private final boolean useNodeNameAsExternalAddress;
     private final int kubernetesApiRetries;
     private final String kubernetesMasterUrl;
     private final String kubernetesApiToken;
@@ -77,6 +79,8 @@ final class KubernetesConfig {
         this.serviceLabelValue = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, SERVICE_LABEL_VALUE, "true");
         this.namespace = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, NAMESPACE, getNamespaceOrDefault());
         this.resolveNotReadyAddresses = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, RESOLVE_NOT_READY_ADDRESSES, false);
+        this.useNodeNameAsExternalAddress
+                = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, USE_NODE_NAME_AS_EXTERNAL_ADDRESS, false);
         this.kubernetesApiRetries
                 = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_API_RETIRES, DEFAULT_KUBERNETES_API_RETRIES);
         this.kubernetesMasterUrl = getOrDefault(properties, KUBERNETES_SYSTEM_PREFIX, KUBERNETES_MASTER_URL, DEFAULT_MASTER_URL);
@@ -255,6 +259,10 @@ final class KubernetesConfig {
         return resolveNotReadyAddresses;
     }
 
+    boolean isUseNodeNameAsExternalAddress() {
+        return useNodeNameAsExternalAddress;
+    }
+
     int getKubernetesApiRetries() {
         return kubernetesApiRetries;
     }
@@ -286,6 +294,7 @@ final class KubernetesConfig {
                 + "service-label-value: " + serviceLabelValue + ", "
                 + "namespace: " + namespace + ", "
                 + "resolve-not-ready-addresses: " + resolveNotReadyAddresses + ", "
+                + "use-node-name-as-external-address: " + useNodeNameAsExternalAddress + ", "
                 + "kubernetes-api-retries: " + kubernetesApiRetries + ", "
                 + "kubernetes-master: " + kubernetesMasterUrl + "}";
     }
diff --git a/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java b/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java
index bad8ca7..1db1347 100644
--- a/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java
+++ b/src/main/java/com/hazelcast/kubernetes/KubernetesProperties.java
@@ -91,6 +91,14 @@ public final class KubernetesProperties {
      */
     public static final PropertyDefinition RESOLVE_NOT_READY_ADDRESSES = property("resolve-not-ready-addresses", BOOLEAN);
 
+    /**
+     * <p>Configuration key: <tt>use-node-name-as-external-address</tt></p>
+     * Defines if the node name should be used as external address, instead of looking up the external IP using
+     * the <code>/nodes</code> resource. Default is false.
+     */
+    public static final PropertyDefinition USE_NODE_NAME_AS_EXTERNAL_ADDRESS = property("use-node-name-as-external-address",
+            BOOLEAN);
+
     /**
      * <p>Configuration key: <tt>kubernetes-api-retries</tt></p>
      * Defines the number of retries to Kubernetes API. Defaults to: 3.


Test patch:
diff --git a/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java b/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java
index e715cf0..778b333 100644
--- a/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java
+++ b/src/test/java/com/hazelcast/kubernetes/KubernetesClientTest.java
@@ -56,8 +56,7 @@ public class KubernetesClientTest {
 
     @Before
     public void setUp() {
-        String kubernetesMasterUrl = String.format("http://%s:%d", KUBERNETES_MASTER_IP, wireMockRule.port());
-        kubernetesClient = new KubernetesClient(NAMESPACE, kubernetesMasterUrl, TOKEN, CA_CERTIFICATE, RETRIES);
+        kubernetesClient = newKubernetesClient(false);
         stubFor(get(urlMatching("/api/.*")).atPriority(5)
                                            .willReturn(aResponse().withStatus(401).withBody("\"reason\":\"Forbidden\"")));
     }
@@ -364,37 +363,11 @@ public class KubernetesClientTest {
         // given
         stub(String.format("/api/v1/namespaces/%s/pods", NAMESPACE), podsListResponse());
         stub(String.format("/api/v1/namespaces/%s/endpoints", NAMESPACE), endpointsListResponse());
-        //language=JSON
-        //language=JSON
-        String serviceResponse1 = "{\n"
-                + "  \"kind\": \"Service\",\n"
-                + "  \"spec\": {\n"
-                + "    \"ports\": [\n"
-                + "      {\n"
-                + "        \"port\": 32123,\n"
-                + "        \"targetPort\": 5701,\n"
-                + "        \"nodePort\": 31916\n"
-                + "      }\n"
-                + "    ]\n"
-                + "  }\n"
-                + "}\n";
-        stub(String.format("/api/v1/namespaces/%s/services/service-0", NAMESPACE), serviceResponse1);
 
-        //language=JSON
-        String serviceResponse2 = "{\n"
-                + "  \"kind\": \"Service\",\n"
-                + "  \"spec\": {\n"
-                + "    \"ports\": [\n"
-                + "      {\n"
-                + "        \"port\": 32124,\n"
-                + "        \"targetPort\": 5701,\n"
-                + "        \"nodePort\": 31917\n"
-                + "      }\n"
-                + "    ]\n"
-                + "  }\n"
-                + "}";
-        stub(String.format("/api/v1/namespaces/%s/services/service-1", NAMESPACE), serviceResponse2);
+        stub(String.format("/api/v1/namespaces/%s/services/service-0", NAMESPACE), nodePortService1Response());
+        stub(String.format("/api/v1/namespaces/%s/services/service-1", NAMESPACE), nodePortService2Response());
 
+        //language=JSON
         String nodeResponse1 = "{\n"
                 + "  \"kind\": \"Node\",\n"
                 + "  \"status\": {\n"
@@ -437,6 +410,30 @@ public class KubernetesClientTest {
         assertThat(formatPublic(result), containsInAnyOrder(ready("35.232.226.200", 31916), ready("35.232.226.201", 31917)));
     }
 
+    @Test
+    public void endpointsByNamespaceWithNodeName() {
+        // given
+        // create KubernetesClient with useNodeNameAsExternalAddress=true
+        kubernetesClient = newKubernetesClient(true);
+
+        stub(String.format("/api/v1/namespaces/%s/pods", NAMESPACE), podsListResponse());
+        stub(String.format("/api/v1/namespaces/%s/endpoints", NAMESPACE), endpointsListResponse());
+
+        stub(String.format("/api/v1/namespaces/%s/services/service-0", NAMESPACE), nodePortService1Response());
+        stub(String.format("/api/v1/namespaces/%s/services/service-1", NAMESPACE), nodePortService2Response());
+
+        String forbiddenBody = "\"reason\":\"Forbidden\"";
+        stub("/api/v1/nodes/node-name-1", 403, forbiddenBody);
+        stub("/api/v1/nodes/node-name-2", 403, forbiddenBody);
+
+        // when
+        List<Endpoint> result = kubernetesClient.endpoints();
+
+        // then
+        assertThat(format(result), containsInAnyOrder(ready("192.168.0.25", 5701), ready("172.17.0.5", 5702)));
+        assertThat(formatPublic(result), containsInAnyOrder(ready("node-name-1", 31916), ready("node-name-2", 31917)));
+    }
+
     private static String podsListResponse() {
         //language=JSON
         return "{\n"
@@ -561,13 +558,43 @@ public class KubernetesClientTest {
                 + "}";
     }
 
+    private static String nodePortService1Response() {
+        //language=JSON
+        return "{\n"
+        + "  \"kind\": \"Service\",\n"
+        + "  \"spec\": {\n"
+        + "    \"ports\": [\n"
+        + "      {\n"
+        + "        \"port\": 32123,\n"
+        + "        \"targetPort\": 5701,\n"
+        + "        \"nodePort\": 31916\n"
+        + "      }\n"
+        + "    ]\n"
+        + "  }\n"
+        + "}\n";
+    }
+
+    private static String nodePortService2Response() {
+        //language=JSON
+        return "{\n"
+        + "  \"kind\": \"Service\",\n"
+        + "  \"spec\": {\n"
+        + "    \"ports\": [\n"
+        + "      {\n"
+        + "        \"port\": 32124,\n"
+        + "        \"targetPort\": 5701,\n"
+        + "        \"nodePort\": 31917\n"
+        + "      }\n"
+        + "    ]\n"
+        + "  }\n"
+        + "}";
+    }
+
     @Test
     public void forbidden() {
         // given
         String forbiddenBody = "\"reason\":\"Forbidden\"";
-        stubFor(get(urlEqualTo(String.format("/api/v1/namespaces/%s/pods", NAMESPACE)))
-                .withHeader("Authorization", equalTo(String.format("Bearer %s", TOKEN)))
-                .willReturn(aResponse().withStatus(403).withBody(forbiddenBody)));
+        stub(String.format("/api/v1/namespaces/%s/pods", NAMESPACE), 403, forbiddenBody);
 
         // when
         List<Endpoint> result = kubernetesClient.endpoints();
@@ -580,9 +607,7 @@ public class KubernetesClientTest {
     public void wrongApiToken() {
         // given
         String unauthorizedBody = "\"reason\":\"Unauthorized\"";
-        stubFor(get(urlEqualTo(String.format("/api/v1/namespaces/%s/pods", NAMESPACE)))
-                .withHeader("Authorization", equalTo(String.format("Bearer %s", TOKEN)))
-                .willReturn(aResponse().withStatus(401).withBody(unauthorizedBody)));
+        stub(String.format("/api/v1/namespaces/%s/pods", NAMESPACE), 401, unauthorizedBody);
 
         // when
         List<Endpoint> result = kubernetesClient.endpoints();
@@ -595,14 +620,17 @@ public class KubernetesClientTest {
     public void unknownException() {
         // given
         String notRetriedErrorBody = "\"reason\":\"Forbidden\"";
-        stubFor(get(urlEqualTo(String.format("/api/v1/namespaces/%s/pods", NAMESPACE)))
-                .withHeader("Authorization", equalTo(String.format("Bearer %s", TOKEN)))
-                .willReturn(aResponse().withStatus(501).withBody(notRetriedErrorBody)));
+        stub(String.format("/api/v1/namespaces/%s/pods", NAMESPACE), 501, notRetriedErrorBody);
 
         // when
         kubernetesClient.endpoints();
     }
 
+    private KubernetesClient newKubernetesClient(boolean useNodeNameAsExternalAddress) {
+        String kubernetesMasterUrl = String.format("http://%s:%d", KUBERNETES_MASTER_IP, wireMockRule.port());
+        return new KubernetesClient(NAMESPACE, kubernetesMasterUrl, TOKEN, CA_CERTIFICATE, RETRIES, useNodeNameAsExternalAddress);
+    }
+
     private static List<String> format(List<Endpoint> addresses) {
         List<String> result = new ArrayList<String>();
         for (Endpoint address : addresses) {
@@ -626,9 +654,13 @@ public class KubernetesClientTest {
     }
 
     private static void stub(String url, String response) {
+        stub(url, 200, response);
+    }
+
+    private static void stub(String url, int status, String response) {
         stubFor(get(urlEqualTo(url))
                 .withHeader("Authorization", equalTo(String.format("Bearer %s", TOKEN)))
-                .willReturn(aResponse().withStatus(200).withBody(response)));
+                .willReturn(aResponse().withStatus(status).withBody(response)));
     }
 
     private static void stub(String url, Map<String, String> queryParams, String response) {
diff --git a/src/test/java/com/hazelcast/kubernetes/KubernetesConfigTest.java b/src/test/java/com/hazelcast/kubernetes/KubernetesConfigTest.java
index 2875aee..a7569fd 100644
--- a/src/test/java/com/hazelcast/kubernetes/KubernetesConfigTest.java
+++ b/src/test/java/com/hazelcast/kubernetes/KubernetesConfigTest.java
@@ -79,6 +79,7 @@ public class KubernetesConfigTest {
         assertEquals(DiscoveryMode.KUBERNETES_API, config.getMode());
         assertEquals("default", config.getNamespace());
         assertEquals(false, config.isResolveNotReadyAddresses());
+        assertEquals(false, config.isUseNodeNameAsExternalAddress());
         assertEquals(TEST_API_TOKEN, config.getKubernetesApiToken());
         assertEquals(TEST_CA_CERTIFICATE, config.getKubernetesCaCertificate());
     }
@@ -116,6 +117,19 @@ public class KubernetesConfigTest {
         assertEquals(serviceLabelValue, config.getServiceLabelValue());
     }
 
+    @Test
+    public void kubernetesApiNodeNameAsExternalAddress() {
+        // given
+        Map<String, Comparable> properties = createProperties();
+        properties.put(KubernetesProperties.USE_NODE_NAME_AS_EXTERNAL_ADDRESS.key(), true);
+
+        // when
+        KubernetesConfig config = new KubernetesConfig(properties);
+
+        // then
+        assertEquals(true, config.isUseNodeNameAsExternalAddress());
+    }
+
     @Test(expected = InvalidConfigurationException.class)
     public void invalidConfigurationBothModesConfigured() {
         // given