Persist two singleton entities in SQL tables (#860)

* Persist two singleton entities in SQL tables

A table might not be the best place to store singleton entities, but by
doing this we ensure we can easily inspect them later and use the same
sort of persistence logic for these that we do elsewhere.

ServerSecret is stored upon retrieval so that we make sure that the same
secret is used in both Datastore and SQL (we wouldn't want to change
it).

* Responses to CR

* Don't have a separate ID for the singleton entities

* Rename secret UUID

* Rename and regenerate

core/src/main/java/google/registry/model/common/CrossTldSingleton.java

@@ -20,15 +20,16 @@ import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Id;
 import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.ImmutableObject;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.Transient;
 
 /** A singleton entity in Datastore. */
+@MappedSuperclass
 public abstract class CrossTldSingleton extends ImmutableObject {
 
-  public static final long SINGLETON_ID = 1;  // There is always exactly one of these.
+  public static final long SINGLETON_ID = 1; // There is always exactly one of these.
 
-  @Id
-  long id = SINGLETON_ID;
+  @Id @Transient long id = SINGLETON_ID;
 
-  @Parent
-  Key<EntityGroupRoot> parent = getCrossTldKey();
+  @Transient @Parent Key<EntityGroupRoot> parent = getCrossTldKey();
 }

core/src/main/java/google/registry/model/server/ServerSecret.java

@@ -14,29 +14,43 @@
 
 package google.registry.model.server;
 
-import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableList;
 import com.google.common.primitives.Longs;
+import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
+import com.googlecode.objectify.annotation.Ignore;
+import com.googlecode.objectify.annotation.OnLoad;
 import com.googlecode.objectify.annotation.Unindex;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
 import google.registry.model.common.CrossTldSingleton;
+import google.registry.persistence.VKey;
+import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.SqlEntity;
 import java.nio.ByteBuffer;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
+import javax.persistence.Column;
+import javax.persistence.Id;
+import javax.persistence.PostLoad;
+import javax.persistence.Transient;
 
 /** A secret number used for generating tokens (such as XSRF tokens). */
 @Entity
+@javax.persistence.Entity
 @Unindex
 @NotBackedUp(reason = Reason.AUTO_GENERATED)
 // TODO(b/27427316): Replace this with an entry in KMSKeyring
-public class ServerSecret extends CrossTldSingleton {
+public class ServerSecret extends CrossTldSingleton implements DatastoreEntity, SqlEntity {
 
   /**
    * Cache of the singleton ServerSecret instance that creates it if not present.
@@ -45,28 +59,34 @@ public class ServerSecret extends CrossTldSingleton {
    * Supplier that can be reset for testing purposes.
    */
   private static final LoadingCache<Class<ServerSecret>, ServerSecret> CACHE =
-      CacheBuilder.newBuilder().build(
-          new CacheLoader<Class<ServerSecret>, ServerSecret>() {
-            @Override
-            public ServerSecret load(Class<ServerSecret> unused) {
-              // Fast path - non-transactional load to hit memcache.
-              ServerSecret secret = ofy().load().entity(new ServerSecret()).now();
-              if (secret != null) {
-                return secret;
-              }
-              // Slow path - transactionally create a new ServerSecret (once per app setup).
-              return tm().transact(() -> {
-                // Check again for an existing secret within the transaction to avoid races.
-                ServerSecret secret1 = ofy().load().entity(new ServerSecret()).now();
-                if (secret1 == null) {
-                  UUID uuid = UUID.randomUUID();
-                  secret1 = create(uuid.getMostSignificantBits(), uuid.getLeastSignificantBits());
-                  ofy().saveWithoutBackup().entity(secret1).now();
+      CacheBuilder.newBuilder()
+          .build(
+              new CacheLoader<Class<ServerSecret>, ServerSecret>() {
+                @Override
+                public ServerSecret load(Class<ServerSecret> unused) {
+                  return retrieveAndSaveSecret();
                 }
-                return secret1;
               });
-            }
-          });
+
+  private static ServerSecret retrieveAndSaveSecret() {
+    VKey<ServerSecret> key =
+        VKey.create(
+            ServerSecret.class,
+            SINGLETON_ID,
+            Key.create(getCrossTldKey(), ServerSecret.class, SINGLETON_ID));
+    return tm().transact(
+            () -> {
+              // transactionally create a new ServerSecret (once per app setup) if necessary.
+              // return the ofy() result during Datastore-primary phase
+              ServerSecret secret =
+                  ofyTm().maybeLoad(key).orElseGet(() -> create(UUID.randomUUID()));
+              // During a dual-write period, write it to both Datastore and SQL
+              // even if we didn't have to retrieve it from the DB
+              ofyTm().transact(() -> ofyTm().putWithoutBackup(secret));
+              jpaTm().transact(() -> jpaTm().putWithoutBackup(secret));
+              return secret;
+            });
+  }
 
   /** Returns the global ServerSecret instance, creating it if one isn't already in Datastore. */
   public static ServerSecret get() {
@@ -77,23 +97,38 @@ public class ServerSecret extends CrossTldSingleton {
     }
   }
 
-  /** Most significant 8 bytes of the UUID value. */
-  long mostSignificant;
+  /** Most significant 8 bytes of the UUID value (stored separately for legacy purposes). */
+  @Transient long mostSignificant;
 
-  /** Least significant 8 bytes of the UUID value. */
-  long leastSignificant;
+  /** Least significant 8 bytes of the UUID value (stored separately for legacy purposes). */
+  @Transient long leastSignificant;
 
-  @VisibleForTesting
-  static ServerSecret create(long mostSignificant, long leastSignificant) {
-    ServerSecret secret = new ServerSecret();
-    secret.mostSignificant = mostSignificant;
-    secret.leastSignificant = leastSignificant;
-    return secret;
+  /** The UUID value itself. */
+  @Id
+  @Column(columnDefinition = "uuid")
+  @Ignore
+  UUID secret;
+
+  /** Convert the Datastore representation to SQL. */
+  @OnLoad
+  void onLoad() {
+    secret = new UUID(mostSignificant, leastSignificant);
   }
 
-  /** Returns the value of this ServerSecret as a UUID. */
-  public UUID asUuid() {
-    return new UUID(mostSignificant, leastSignificant);
+  /** Convert the SQL representation to Datastore. */
+  @PostLoad
+  void postLoad() {
+    mostSignificant = secret.getMostSignificantBits();
+    leastSignificant = secret.getLeastSignificantBits();
+  }
+
+  @VisibleForTesting
+  static ServerSecret create(UUID uuid) {
+    ServerSecret secret = new ServerSecret();
+    secret.mostSignificant = uuid.getMostSignificantBits();
+    secret.leastSignificant = uuid.getLeastSignificantBits();
+    secret.secret = uuid;
+    return secret;
   }
 
   /** Returns the value of this ServerSecret as a byte array. */
@@ -104,6 +139,16 @@ public class ServerSecret extends CrossTldSingleton {
         .array();
   }
 
+  @Override
+  public ImmutableList<SqlEntity> toSqlEntities() {
+    return ImmutableList.of(); // dually-written
+  }
+
+  @Override
+  public ImmutableList<DatastoreEntity> toDatastoreEntities() {
+    return ImmutableList.of(); // dually-written
+  }
+
   @VisibleForTesting
   static void resetCache() {
     CACHE.invalidateAll();

core/src/main/java/google/registry/model/tmch/TmchCrl.java

@@ -15,31 +15,50 @@
 package google.registry.model.tmch;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 
+import com.google.common.collect.ImmutableList;
+import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
 import google.registry.model.common.CrossTldSingleton;
-import javax.annotation.Nullable;
+import google.registry.model.tmch.TmchCrl.TmchCrlId;
+import google.registry.persistence.VKey;
+import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.SqlEntity;
+import java.io.Serializable;
+import java.util.Optional;
 import javax.annotation.concurrent.Immutable;
+import javax.persistence.Column;
+import javax.persistence.Id;
+import javax.persistence.IdClass;
 import org.joda.time.DateTime;
 
 /** Datastore singleton for ICANN's TMCH CA certificate revocation list (CRL). */
 @Entity
+@javax.persistence.Entity
 @Immutable
 @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)
-public final class TmchCrl extends CrossTldSingleton {
+@IdClass(TmchCrlId.class)
+public final class TmchCrl extends CrossTldSingleton implements DatastoreEntity, SqlEntity {
 
-  String crl;
-  DateTime updated;
-  String url;
+  @Id String crl;
+
+  @Id DateTime updated;
+
+  @Id String url;
 
   /** Returns the singleton instance of this entity, without memoization. */
-  @Nullable
-  public static TmchCrl get() {
-    return ofy().load().entity(new TmchCrl()).now();
+  public static Optional<TmchCrl> get() {
+    VKey<TmchCrl> key =
+        VKey.create(
+            TmchCrl.class, SINGLETON_ID, Key.create(getCrossTldKey(), TmchCrl.class, SINGLETON_ID));
+    // return the ofy() result during Datastore-primary phase
+    return ofyTm().transact(() -> ofyTm().maybeLoad(key));
   }
 
   /**
@@ -47,16 +66,18 @@ public final class TmchCrl extends CrossTldSingleton {
    *
    * <p>Please do not call this function unless your CRL is properly formatted, signed by the root,
    * and actually newer than the one currently in Datastore.
+   *
+   * <p>During the dual-write period, we write to both Datastore and SQL
    */
   public static void set(final String crl, final String url) {
-    tm()
-        .transactNew(
+    tm().transact(
             () -> {
               TmchCrl tmchCrl = new TmchCrl();
               tmchCrl.updated = tm().getTransactionTime();
               tmchCrl.crl = checkNotNull(crl, "crl");
               tmchCrl.url = checkNotNull(url, "url");
-              ofy().saveWithoutBackup().entity(tmchCrl);
+              ofyTm().transactNew(() -> ofyTm().putWithoutBackup(tmchCrl));
+              jpaTm().transactNew(() -> jpaTm().putWithoutBackup(tmchCrl));
             });
   }
 
@@ -74,4 +95,36 @@ public final class TmchCrl extends CrossTldSingleton {
   public final DateTime getUpdated() {
     return updated;
   }
+
+  @Override
+  public ImmutableList<SqlEntity> toSqlEntities() {
+    return ImmutableList.of(); // dually-written
+  }
+
+  @Override
+  public ImmutableList<DatastoreEntity> toDatastoreEntities() {
+    return ImmutableList.of(); // dually-written
+  }
+
+  static class TmchCrlId implements Serializable {
+
+    @Column(name = "certificateRevocations")
+    String crl;
+
+    @Column(name = "updateTimestamp")
+    DateTime updated;
+
+    String url;
+
+    /** Hibernate requires this default constructor. */
+    private TmchCrlId() {}
+
+    static TmchCrlId create(String crl, DateTime updated, String url) {
+      TmchCrlId result = new TmchCrlId();
+      result.crl = crl;
+      result.updated = updated;
+      result.url = url;
+      return result;
+    }
+  }
 }

core/src/main/java/google/registry/tmch/TmchCertificateAuthority.java

@@ -32,6 +32,7 @@ import java.security.GeneralSecurityException;
 import java.security.cert.CertificateParsingException;
 import java.security.cert.X509CRL;
 import java.security.cert.X509Certificate;
+import java.util.Optional;
 import javax.annotation.concurrent.Immutable;
 import javax.annotation.concurrent.ThreadSafe;
 import javax.inject.Inject;
@@ -82,14 +83,15 @@ public final class TmchCertificateAuthority {
               new CacheLoader<TmchCaMode, X509CRL>() {
                 @Override
                 public X509CRL load(final TmchCaMode tmchCaMode) throws GeneralSecurityException {
-                  TmchCrl storedCrl = TmchCrl.get();
-                  String crlContents;
-                  if (storedCrl == null) {
-                    String file = (tmchCaMode == PILOT) ? CRL_PILOT_FILE : CRL_FILE;
-                    crlContents = readResourceUtf8(TmchCertificateAuthority.class, file);
-                  } else {
-                    crlContents = storedCrl.getCrl();
-                  }
+                  Optional<TmchCrl> storedCrl = TmchCrl.get();
+                  String crlContents =
+                      storedCrl
+                          .map(TmchCrl::getCrl)
+                          .orElseGet(
+                              () -> {
+                                String file = (tmchCaMode == PILOT) ? CRL_PILOT_FILE : CRL_FILE;
+                                return readResourceUtf8(TmchCertificateAuthority.class, file);
+                              });
                   X509CRL crl = X509Utils.loadCrl(crlContents);
                   crl.verify(ROOT_CERTS.get(tmchCaMode).getPublicKey());
                   return crl;

core/src/main/resources/META-INF/persistence.xml

@@ -63,8 +63,10 @@
     <class>google.registry.model.reporting.DomainTransactionRecord</class>
     <class>google.registry.model.reporting.Spec11ThreatMatch</class>
     <class>google.registry.model.server.KmsSecretRevision</class>
+    <class>google.registry.model.server.ServerSecret</class>
     <class>google.registry.model.smd.SignedMarkRevocationList</class>
     <class>google.registry.model.tmch.ClaimsListShard</class>
+    <class>google.registry.model.tmch.TmchCrl</class>
     <class>google.registry.persistence.transaction.TransactionEntity</class>
     <class>google.registry.schema.cursor.Cursor</class>
     <class>google.registry.schema.domain.RegistryLock</class>

core/src/test/java/google/registry/model/server/ServerSecretTest.java

@@ -15,21 +15,25 @@
 package google.registry.model.server;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.common.CrossTldSingleton.SINGLETON_ID;
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 
+import com.googlecode.objectify.Key;
+import google.registry.model.EntityTestCase;
 import google.registry.model.ofy.RequestCapturingAsyncDatastoreService;
-import google.registry.testing.AppEngineExtension;
+import google.registry.persistence.VKey;
 import java.util.UUID;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link ServerSecret}. */
-public class ServerSecretTest {
+public class ServerSecretTest extends EntityTestCase {
 
-  @RegisterExtension
-  public final AppEngineExtension appEngine =
-      AppEngineExtension.builder().withDatastoreAndCloudSql().build();
+  ServerSecretTest() {
+    super(JpaEntityCoverageCheck.ENABLED);
+  }
 
   @BeforeEach
   void beforeEach() {
@@ -41,18 +45,20 @@ public class ServerSecretTest {
     ServerSecret secret = ServerSecret.get();
     assertThat(secret).isNotNull();
     assertThat(ofy().load().entity(new ServerSecret()).now()).isEqualTo(secret);
+    assertThat(loadFromSql()).isEqualTo(secret);
   }
 
   @Test
   void testGet_existingSecret_returned() {
-    ServerSecret secret = ServerSecret.create(123, 456);
+    ServerSecret secret = ServerSecret.create(new UUID(123, 456));
     ofy().saveWithoutBackup().entity(secret).now();
     assertThat(ServerSecret.get()).isEqualTo(secret);
     assertThat(ofy().load().entity(new ServerSecret()).now()).isEqualTo(secret);
+    assertThat(loadFromSql()).isEqualTo(secret);
   }
 
   @Test
-  void testGet_cachedSecret_returnedWithoutDatastoreRead() {
+  void testGet_cachedSecret() {
     int numInitialReads = RequestCapturingAsyncDatastoreService.getReads().size();
     ServerSecret secret = ServerSecret.get();
     int numReads = RequestCapturingAsyncDatastoreService.getReads().size();
@@ -62,16 +68,28 @@ public class ServerSecretTest {
   }
 
   @Test
-  void testAsUuid() {
-    UUID uuid = ServerSecret.create(123, 456).asUuid();
-    assertThat(uuid.getMostSignificantBits()).isEqualTo(123);
-    assertThat(uuid.getLeastSignificantBits()).isEqualTo(456);
+  void testAsBytes() {
+    byte[] bytes = ServerSecret.create(new UUID(123, 0x456)).asBytes();
+    assertThat(bytes).isEqualTo(new byte[] {0, 0, 0, 0, 0, 0, 0, 123, 0, 0, 0, 0, 0, 0, 0x4, 0x56});
   }
 
-  @Test
-  void testAsBytes() {
-    byte[] bytes = ServerSecret.create(123, 0x456).asBytes();
-    assertThat(bytes)
-        .isEqualTo(new byte[] {0, 0, 0, 0, 0, 0, 0, 123, 0, 0, 0, 0, 0, 0, 0x4, 0x56});
+  private static ServerSecret loadFromSql() {
+    return jpaTm()
+        .transact(
+            () ->
+                jpaTm()
+                    .getEntityManager()
+                    .createQuery("FROM ServerSecret", ServerSecret.class)
+                    .setMaxResults(1)
+                    .getResultStream()
+                    .findFirst()
+                    .get());
+  }
+
+  private static VKey<ServerSecret> createKey() {
+    return VKey.create(
+        ServerSecret.class,
+        SINGLETON_ID,
+        Key.create(getCrossTldKey(), ServerSecret.class, SINGLETON_ID));
   }
 }

core/src/test/java/google/registry/model/tmch/TmchCrlTest.java

@@ -15,22 +15,48 @@
 package google.registry.model.tmch;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 
-import google.registry.testing.AppEngineExtension;
+import google.registry.model.EntityTestCase;
+import java.util.Optional;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
 
 /** Unit tests for {@link TmchCrl}. */
-public class TmchCrlTest {
+public class TmchCrlTest extends EntityTestCase {
 
-  @RegisterExtension
-  public final AppEngineExtension appEngine =
-      AppEngineExtension.builder().withDatastoreAndCloudSql().build();
+  TmchCrlTest() {
+    super(JpaEntityCoverageCheck.ENABLED);
+  }
 
   @Test
   void testSuccess() {
-    assertThat(TmchCrl.get()).isNull();
-    TmchCrl.set("lolcat", "http://lol.cat");
-    assertThat(TmchCrl.get().getCrl()).isEqualTo("lolcat");
+    assertThat(TmchCrl.get()).isEqualTo(Optional.empty());
+    TmchCrl.set("lolcat", "https://lol.cat");
+    assertThat(TmchCrl.get().get().getCrl()).isEqualTo("lolcat");
+  }
+
+  @Test
+  void testDualWrite() {
+    TmchCrl expected = new TmchCrl();
+    expected.crl = "lolcat";
+    expected.url = "https://lol.cat";
+    expected.updated = fakeClock.nowUtc();
+    TmchCrl.set("lolcat", "https://lol.cat");
+    assertThat(ofy().load().entity(new TmchCrl()).now()).isEqualTo(expected);
+    assertThat(loadFromSql()).isEqualTo(expected);
+  }
+
+  private static TmchCrl loadFromSql() {
+    return jpaTm()
+        .transact(
+            () ->
+                jpaTm()
+                    .getEntityManager()
+                    .createQuery("FROM TmchCrl", TmchCrl.class)
+                    .setMaxResults(1)
+                    .getResultStream()
+                    .findFirst()
+                    .get());
   }
 }

core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java

@@ -43,12 +43,13 @@ import org.junit.jupiter.api.extension.RegisterExtension;
 /** Unit tests for {@link RequestAuthenticator}. */
 class RequestAuthenticatorTest {
 
-  @RegisterExtension final AppEngineExtension appEngine = AppEngineExtension.builder().build();
+  @RegisterExtension
+  final AppEngineExtension appEngine =
+      AppEngineExtension.builder().withDatastoreAndCloudSql().build();
 
-  private static final AuthSettings AUTH_NONE = AuthSettings.create(
-      ImmutableList.of(AuthMethod.INTERNAL),
-      AuthLevel.NONE,
-      UserPolicy.IGNORED);
+  private static final AuthSettings AUTH_NONE =
+      AuthSettings.create(
+          ImmutableList.of(AuthMethod.INTERNAL), AuthLevel.NONE, UserPolicy.IGNORED);
 
   private static final AuthSettings AUTH_INTERNAL_OR_ADMIN = AuthSettings.create(
       ImmutableList.of(AuthMethod.INTERNAL),

core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java

@@ -30,8 +30,10 @@ import google.registry.model.registry.RegistryTest;
 import google.registry.model.registry.label.ReservedListSqlDaoTest;
 import google.registry.model.reporting.Spec11ThreatMatchTest;
 import google.registry.model.server.KmsSecretRevisionSqlDaoTest;
+import google.registry.model.server.ServerSecretTest;
 import google.registry.model.smd.SignedMarkRevocationListDaoTest;
 import google.registry.model.tmch.ClaimsListDaoTest;
+import google.registry.model.tmch.TmchCrlTest;
 import google.registry.persistence.transaction.JpaEntityCoverageExtension;
 import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
 import google.registry.schema.cursor.CursorDaoTest;
@@ -95,8 +97,10 @@ import org.junit.runner.RunWith;
   RegistryTest.class,
   ReservedListSqlDaoTest.class,
   RegistryLockDaoTest.class,
+  ServerSecretTest.class,
   SignedMarkRevocationListDaoTest.class,
   Spec11ThreatMatchTest.class,
+  TmchCrlTest.class,
   // AfterSuiteTest must be the last entry. See class javadoc for details.
   AfterSuiteTest.class
 })