Greatly increase the upper limit of proxy instances in production (#2259)

From our investigation, the Monday night WHOIS storm does not cause any
strain to the backend system. The backend latency metrics are all well within
the limits. The latency measured from the proxy matches observed latency
by the prober, and we see that the "used" CPU is 1.5x of "requested" CPU
during the time when the latency is above the threshold.

Making this change hopefully removes the proxy as the bottleneck and
ameliorate the pages.

core/src/main/java/google/registry/model/registrar/Registrar.java

@@ -22,6 +22,7 @@ import static com.google.common.base.Strings.nullToEmpty;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
 import static com.google.common.collect.Sets.immutableEnumSet;
+import static com.google.common.collect.Streams.stream;
 import static com.google.common.io.BaseEncoding.base64;
 import static google.registry.config.RegistryConfig.getDefaultRegistrarWhoisServer;
 import static google.registry.model.CacheUtils.memoizeWithShortExpiration;
@@ -794,6 +795,24 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J
       }
     }
 
+    // Making sure there's no registrar with the same ianaId already in the system
+    private static boolean isNotADuplicateIanaId(
+        Iterable<Registrar> registrars, Registrar newInstance) {
+      // Return early if newly build registrar is not type REAL or ianaId is
+      // reserved by ICANN - https://www.iana.org/assignments/registrar-ids/registrar-ids.xhtml
+      if (!Type.REAL.equals(newInstance.type)
+          || ImmutableSet.of(1L, 8L).contains(newInstance.ianaIdentifier)) {
+        return true;
+      }
+
+      return stream(registrars)
+          .filter(registrar -> Type.REAL.equals(registrar.getType()))
+          .filter(registrar -> !Objects.equals(newInstance.registrarId, registrar.getRegistrarId()))
+          .noneMatch(
+              registrar ->
+                  Objects.equals(newInstance.ianaIdentifier, registrar.getIanaIdentifier()));
+    }
+
     public Builder setContactsRequireSyncing(boolean contactsRequireSyncing) {
       getInstance().contactsRequireSyncing = contactsRequireSyncing;
       return this;
@@ -912,6 +931,15 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J
               "Supplied IANA ID is not valid for %s registrar type: %s",
               getInstance().type, getInstance().ianaIdentifier));
 
+      // We do not allow creating Real registrars with IANA ID that's already in the system
+      // b/315007360 - for more details
+      checkArgument(
+          isNotADuplicateIanaId(loadAllCached(), getInstance()),
+          String.format(
+              "Rejected attempt to create a registrar with ianaId that's already in the system -"
+                  + " %s",
+              getInstance().ianaIdentifier));
+
       // In order to grant access to real TLDs, the registrar must have a corresponding billing
       // account ID for that TLD's billing currency.
       ImmutableSet<String> nonBillableTlds =

core/src/main/java/google/registry/reporting/icann/IcannHttpReporter.java

@@ -14,7 +14,6 @@
 
 package google.registry.reporting.icann;
 
-import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_BAD_REQUEST;
 import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_OK;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.net.MediaType.CSV_UTF_8;
@@ -38,6 +37,7 @@ import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.nio.charset.StandardCharsets;
 import java.security.GeneralSecurityException;
 import java.util.List;
 import javax.inject.Inject;
@@ -90,30 +90,31 @@ public class IcannHttpReporter {
     UrlConnectionUtils.setPayload(connection, reportBytes, CSV_UTF_8.toString());
     connection.setInstanceFollowRedirects(false);
 
-    int responseCode;
-    byte[] content;
+    int responseCode = 0;
+    byte[] content = null;
     try {
       responseCode = connection.getResponseCode();
-      // Only responses with a 200 or 400 status have a body. For everything else, we can return
-      // false early.
-      if (responseCode != STATUS_CODE_OK && responseCode != STATUS_CODE_BAD_REQUEST) {
-        logger.atWarning().log("Connection to ICANN server failed", connection);
+      content = UrlConnectionUtils.getResponseBytes(connection);
+      if (responseCode != STATUS_CODE_OK) {
+        XjcIirdeaResult result = parseResult(content);
+        logger.atWarning().log(
+            "PUT rejected, status code %s:\n%s\n%s",
+            result.getCode().getValue(), result.getMsg(), result.getDescription());
         return false;
       }
-      content = UrlConnectionUtils.getResponseBytes(connection);
+    } catch (IOException e) {
+      logger.atWarning().withCause(e).log(
+          "Connection to ICANN server failed with responseCode %s and connection %s",
+          responseCode == 0 ? "not available" : responseCode, connection);
+      return false;
+    } catch (XmlException e) {
+      logger.atWarning().withCause(e).log(
+          "Failed to parse ICANN response with responseCode %s and content %s",
+          responseCode, new String(content, StandardCharsets.UTF_8));
+      return false;
     } finally {
       connection.disconnect();
     }
-    // We know that an HTTP 200 response can only contain a result code of
-    // 1000 (i. e. success), there is no need to parse it.
-    // See: https://tools.ietf.org/html/draft-lozano-icann-registry-interfaces-13#page-16
-    if (responseCode != STATUS_CODE_OK) {
-      XjcIirdeaResult result = parseResult(content);
-      logger.atWarning().log(
-          "PUT rejected, status code %s:\n%s\n%s",
-          result.getCode().getValue(), result.getMsg(), result.getDescription());
-      return false;
-    }
     return true;
   }
 
@@ -164,4 +165,5 @@ public class IcannHttpReporter {
                 reportType));
     }
   }
+
 }

core/src/test/java/google/registry/model/registrar/RegistrarTest.java

@@ -201,6 +201,23 @@ class RegistrarTest extends EntityTestCase {
         () -> new Registrar.Builder().setRegistrarId("abcdefghijklmnopq"));
   }
 
+  @Test
+  void testFailure_duplicateIanaId() {
+    persistResource(
+        registrar.asBuilder().setRegistrarId("registrar1").setIanaIdentifier(10L).build());
+
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                registrar.asBuilder().setRegistrarId("registrar2").setIanaIdentifier(10L).build());
+
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "Rejected attempt to create a registrar with ianaId that's already in the system");
+  }
+
   @Test
   void testSetCertificateHash_alsoSetsHash() {
     registrar = registrar.asBuilder().setClientCertificate(null, fakeClock.nowUtc()).build();

core/src/test/java/google/registry/testing/DatabaseHelper.java

@@ -778,7 +778,7 @@ public final class DatabaseHelper {
 
   /** Persists and returns a {@link Registrar} with the specified registrarId. */
   public static Registrar persistNewRegistrar(String registrarId) {
-    return persistNewRegistrar(registrarId, registrarId + " name", Registrar.Type.REAL, 100L);
+    return persistNewRegistrar(registrarId, registrarId + " name", Registrar.Type.REAL, 8L);
   }
 
   /** Persists and returns a list of {@link Registrar}s with the specified registrarIds. */

proxy/kubernetes/proxy-service.yaml

@@ -46,5 +46,5 @@ spec:
     apiVersion: apps/v1
     kind: Deployment
     name: proxy-deployment
-  maxReplicas: 10
-  minReplicas: 1
+  maxReplicas: 50
+  minReplicas: 10