Allow operators to remove the replaceip by passing "null" (#1097)

Netflix · May 23, 2024 · 2bdd2d1 · 2bdd2d1
1 parent fee4ddf
commit 2bdd2d1
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 12 deletions.
diff --git a/priam/src/main/java/com/netflix/priam/identity/InstanceIdentity.java b/priam/src/main/java/com/netflix/priam/identity/InstanceIdentity.java
@@ -29,6 +29,8 @@
 import java.util.List;
 import javax.inject.Inject;
 import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.conn.util.InetAddressUtils;
 
 /**
  * This class provides the central place to create and consume the identity of the instance - token,
@@ -161,8 +163,15 @@ public String getReplacedIp() {
     }
 
     public void setReplacedIp(String replacedIp) {
-        this.replacedIp = replacedIp;
-        if (!replacedIp.isEmpty()) this.isReplace = true;
+        if (StringUtils.isEmpty(replacedIp)) {
+            this.replacedIp = null;
+        } else if (InetAddressUtils.isIPv4Address(replacedIp)) {
+            this.replacedIp = replacedIp;
+        } else {
+            throw new IllegalArgumentException(
+                    replacedIp + " is neither empty nor a valid IpV4 address");
+        }
+        if (!StringUtils.isEmpty(replacedIp)) this.isReplace = true;
     }
 
     private static boolean isInstanceDummy(PriamInstance instance) {

diff --git a/priam/src/main/java/com/netflix/priam/resources/CassandraConfig.java b/priam/src/main/java/com/netflix/priam/resources/CassandraConfig.java
@@ -118,14 +118,16 @@ public Response getReplacedIp() {
     @POST
     @Path("/set_replaced_ip")
     public Response setReplacedIp(@QueryParam("ip") String ip) {
-        if (StringUtils.isEmpty(ip)) return Response.status(Status.BAD_REQUEST).build();
         try {
             priamServer.getInstanceIdentity().setReplacedIp(ip);
-            return Response.ok().build();
+        } catch (IllegalArgumentException e) {
+            logger.error("A replace IP must be a valid IPv4 address. Pass nothing to unset it.", e);
+            return Response.status(Status.BAD_REQUEST).build();
         } catch (Exception e) {
             logger.error("Error while overriding replacement ip", e);
             return Response.serverError().build();
         }
+        return Response.ok().build();
     }
 
     @GET

diff --git a/priam/src/test/java/com/netflix/priam/backup/identity/InstanceIdentityTest.java b/priam/src/test/java/com/netflix/priam/backup/identity/InstanceIdentityTest.java
@@ -20,10 +20,12 @@
 import static org.junit.Assert.*;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.truth.Truth;
 import com.netflix.priam.identity.DoubleRing;
 import com.netflix.priam.identity.InstanceIdentity;
 import com.netflix.priam.identity.PriamInstance;
 import org.junit.Test;
+import org.junit.jupiter.api.Assertions;
 
 public class InstanceIdentityTest extends InstanceTestUtils {
 
@@ -116,4 +118,46 @@ public void printInstance(PriamInstance ins, int hash) {
         System.out.println("ID: " + (ins.getId() - hash));
         System.out.println("PayLoad: " + ins.getToken());
     }
+
+    @Test
+    public void testSetReplacedIp_null() throws Exception {
+        InstanceIdentity identity = createInstanceIdentity("az1", "fakeinstancex");
+        identity.setReplacedIp(null);
+        Truth.assertThat(identity.getReplacedIp()).isNull();
+    }
+
+    @Test
+    public void testSetReplacedIp_valid() throws Exception {
+        InstanceIdentity identity = createInstanceIdentity("az1", "fakeinstancex");
+        identity.setReplacedIp("1.2.3.4");
+        Truth.assertThat(identity.getReplacedIp()).isEqualTo("1.2.3.4");
+    }
+
+    @Test
+    public void testSetReplacedIp_invalid() throws Exception {
+        InstanceIdentity identity = createInstanceIdentity("az1", "fakeinstancex");
+        Assertions.assertThrows(
+                IllegalArgumentException.class, () -> identity.setReplacedIp("foo"));
+    }
+
+    @Test
+    public void testSetReplacedIp_empty() throws Exception {
+        InstanceIdentity identity = createInstanceIdentity("az1", "fakeinstancex");
+        identity.setReplacedIp("");
+        Truth.assertThat(identity.isReplace()).isFalse();
+    }
+
+    @Test
+    public void testIsReplace_nullReplaceIp() throws Exception {
+        InstanceIdentity identity = createInstanceIdentity("az1", "fakeinstancex");
+        identity.setReplacedIp(null);
+        Truth.assertThat(identity.isReplace()).isFalse();
+    }
+
+    @Test
+    public void testIsReplace_validReplaceIp() throws Exception {
+        InstanceIdentity identity = createInstanceIdentity("az1", "fakeinstancex");
+        identity.setReplacedIp("1.2.3.4");
+        Truth.assertThat(identity.isReplace()).isTrue();
+    }
 }
diff --git a/priam/src/test/java/com/netflix/priam/resources/CassandraConfigTest.java b/priam/src/test/java/com/netflix/priam/resources/CassandraConfigTest.java
@@ -18,10 +18,10 @@
 package com.netflix.priam.resources;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.truth.Truth;
 import com.google.inject.Guice;
 import com.netflix.priam.PriamServer;
 import com.netflix.priam.backup.BRTestModule;
@@ -218,21 +218,58 @@ public void getReplacedAddress(@Mocked final InstanceIdentity identity) {
     }
 
     @Test
-    public void setReplacedIp() {
+    public void setReplacedIp_valid() {
         new Expectations() {
             {
                 priamServer.getInstanceIdentity();
                 result = instanceIdentity;
             }
         };
+        Truth.assertThat(resource.setReplacedIp("127.0.0.1").getStatus()).isEqualTo(200);
+    }
 
-        Response response = resource.setReplacedIp("127.0.0.1");
-        assertEquals(200, response.getStatus());
-        assertEquals("127.0.0.1", instanceIdentity.getReplacedIp());
-        assertTrue(instanceIdentity.isReplace());
+    @Test
+    public void setReplacedIp_null() {
+        new Expectations() {
+            {
+                priamServer.getInstanceIdentity();
+                result = instanceIdentity;
+            }
+        };
+        Truth.assertThat(resource.setReplacedIp(null).getStatus()).isEqualTo(200);
+    }
+
+    @Test
+    public void setReplacedIp_empty() {
+        new Expectations() {
+            {
+                priamServer.getInstanceIdentity();
+                result = instanceIdentity;
+            }
+        };
+        Truth.assertThat(resource.setReplacedIp("").getStatus()).isEqualTo(200);
+    }
 
-        response = resource.setReplacedIp(null);
-        assertEquals(400, response.getStatus());
+    @Test
+    public void setReplacedIp_invalid() {
+        new Expectations() {
+            {
+                priamServer.getInstanceIdentity();
+                result = instanceIdentity;
+            }
+        };
+        Truth.assertThat(resource.setReplacedIp("foo").getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    public void setReplacedIp_exception() {
+        new Expectations() {
+            {
+                priamServer.getInstanceIdentity();
+                result = null;
+            }
+        };
+        Truth.assertThat(resource.setReplacedIp("1.2.3.4").getStatus()).isEqualTo(500);
     }
 
     @Test