From c8636f6c9039e05b3f031821fcb6553ee8d4b14a Mon Sep 17 00:00:00 2001 From: gbrodman Date: Thu, 20 Jan 2022 20:39:07 +0000 Subject: [PATCH] Use a read-only replica SQL instance in RdapDomainSearchAction (#1495) We can use it more places later but this can serve as a template. We should inject the connection to the read-only replica (only created once) to the constructor of the action, then use that instead of the regular transaction manager. We add a transaction manager that simulates the read-only-replica behavior for testing purposes as well. In addition, we set the transaction isolation level to READ COMMITTED for this transaction manager (this is fine since we're never writing to it). Postgres requires this for replica SQL access (it fails if we try to use SERIALIZABLE) transactions. We didn't see this with the pipelines before since those already had transaction isolation level overrides --- .../module/pubapi/PubApiComponent.java | 2 + .../persistence/PersistenceModule.java | 4 + .../registry/rdap/RdapDomainSearchAction.java | 225 +++++++------- ...eplicaSimulatingJpaTransactionManager.java | 292 ++++++++++++++++++ .../rdap/RdapDomainSearchActionTest.java | 34 +- 5 files changed, 432 insertions(+), 125 deletions(-) create mode 100644 core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java diff --git a/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java b/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java index 727d92b18..479fad057 100644 --- a/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java +++ b/core/src/main/java/google/registry/module/pubapi/PubApiComponent.java @@ -30,6 +30,7 @@ import google.registry.keyring.api.KeyModule; import google.registry.keyring.kms.KmsModule; import google.registry.module.pubapi.PubApiRequestComponent.PubApiRequestComponentModule; import google.registry.monitoring.whitebox.StackdriverModule; +import google.registry.persistence.PersistenceModule; import google.registry.privileges.secretmanager.SecretManagerModule; import google.registry.request.Modules.Jackson2Module; import google.registry.request.Modules.NetHttpTransportModule; @@ -56,6 +57,7 @@ import javax.inject.Singleton; KeyringModule.class, KmsModule.class, NetHttpTransportModule.class, + PersistenceModule.class, PubApiRequestComponentModule.class, SecretManagerModule.class, ServerTridProviderModule.class, diff --git a/core/src/main/java/google/registry/persistence/PersistenceModule.java b/core/src/main/java/google/registry/persistence/PersistenceModule.java index 9a78896e4..8c25dbd70 100644 --- a/core/src/main/java/google/registry/persistence/PersistenceModule.java +++ b/core/src/main/java/google/registry/persistence/PersistenceModule.java @@ -277,6 +277,8 @@ public abstract class PersistenceModule { setSqlCredential(credentialStore, new RobotUser(RobotId.NOMULUS), overrides); replicaInstanceConnectionName.ifPresent( name -> overrides.put(HIKARI_DS_CLOUD_SQL_INSTANCE, name)); + overrides.put( + Environment.ISOLATION, TransactionIsolationLevel.TRANSACTION_READ_COMMITTED.name()); return new JpaTransactionManagerImpl(create(overrides), clock); } @@ -291,6 +293,8 @@ public abstract class PersistenceModule { HashMap overrides = Maps.newHashMap(beamCloudSqlConfigs); replicaInstanceConnectionName.ifPresent( name -> overrides.put(HIKARI_DS_CLOUD_SQL_INSTANCE, name)); + overrides.put( + Environment.ISOLATION, TransactionIsolationLevel.TRANSACTION_READ_COMMITTED.name()); return new JpaTransactionManagerImpl(create(overrides), clock); } diff --git a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java index d8ae9e7bd..cc37d417e 100644 --- a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java @@ -18,7 +18,6 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static google.registry.model.EppResourceUtils.loadByForeignKey; import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey; import static google.registry.model.ofy.ObjectifyService.auditedOfy; -import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; @@ -38,8 +37,10 @@ import com.google.common.primitives.Booleans; import com.googlecode.objectify.cmd.Query; import google.registry.model.domain.DomainBase; import google.registry.model.host.HostResource; +import google.registry.persistence.PersistenceModule.ReadOnlyReplicaJpaTm; import google.registry.persistence.VKey; import google.registry.persistence.transaction.CriteriaQueryBuilder; +import google.registry.persistence.transaction.JpaTransactionManager; import google.registry.rdap.RdapJsonFormatter.OutputDataType; import google.registry.rdap.RdapMetrics.EndpointType; import google.registry.rdap.RdapMetrics.SearchType; @@ -91,7 +92,11 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { @Inject @Parameter("name") Optional nameParam; @Inject @Parameter("nsLdhName") Optional nsLdhNameParam; @Inject @Parameter("nsIp") Optional nsIpParam; - @Inject public RdapDomainSearchAction() { + + @Inject @ReadOnlyReplicaJpaTm JpaTransactionManager readOnlyJpaTm; + + @Inject + public RdapDomainSearchAction() { super("domain search", EndpointType.DOMAINS); } @@ -223,32 +228,31 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { resultSet = getMatchingResources(query, true, querySizeLimit); } else { resultSet = - jpaTm() - .transact( - () -> { - CriteriaBuilder criteriaBuilder = - jpaTm().getEntityManager().getCriteriaBuilder(); - CriteriaQueryBuilder queryBuilder = - CriteriaQueryBuilder.create(DomainBase.class) - .where( - "fullyQualifiedDomainName", - criteriaBuilder::like, - String.format("%s%%", partialStringQuery.getInitialString())) - .orderByAsc("fullyQualifiedDomainName"); - if (cursorString.isPresent()) { - queryBuilder = - queryBuilder.where( - "fullyQualifiedDomainName", - criteriaBuilder::greaterThan, - cursorString.get()); - } - if (partialStringQuery.getSuffix() != null) { - queryBuilder = - queryBuilder.where( - "tld", criteriaBuilder::equal, partialStringQuery.getSuffix()); - } - return getMatchingResourcesSql(queryBuilder, true, querySizeLimit); - }); + readOnlyJpaTm.transact( + () -> { + CriteriaBuilder criteriaBuilder = + readOnlyJpaTm.getEntityManager().getCriteriaBuilder(); + CriteriaQueryBuilder queryBuilder = + CriteriaQueryBuilder.create(DomainBase.class) + .where( + "fullyQualifiedDomainName", + criteriaBuilder::like, + String.format("%s%%", partialStringQuery.getInitialString())) + .orderByAsc("fullyQualifiedDomainName"); + if (cursorString.isPresent()) { + queryBuilder = + queryBuilder.where( + "fullyQualifiedDomainName", + criteriaBuilder::greaterThan, + cursorString.get()); + } + if (partialStringQuery.getSuffix() != null) { + queryBuilder = + queryBuilder.where( + "tld", criteriaBuilder::equal, partialStringQuery.getSuffix()); + } + return getMatchingResourcesSql(queryBuilder, true, querySizeLimit); + }); } return makeSearchResults(resultSet); } @@ -270,20 +274,19 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { resultSet = getMatchingResources(query, true, querySizeLimit); } else { resultSet = - jpaTm() - .transact( - () -> { - CriteriaQueryBuilder builder = - queryItemsSql( - DomainBase.class, - "tld", - tld, - Optional.of("fullyQualifiedDomainName"), - cursorString, - DeletedItemHandling.INCLUDE) - .orderByAsc("fullyQualifiedDomainName"); - return getMatchingResourcesSql(builder, true, querySizeLimit); - }); + readOnlyJpaTm.transact( + () -> { + CriteriaQueryBuilder builder = + queryItemsSql( + DomainBase.class, + "tld", + tld, + Optional.of("fullyQualifiedDomainName"), + cursorString, + DeletedItemHandling.INCLUDE) + .orderByAsc("fullyQualifiedDomainName"); + return getMatchingResourcesSql(builder, true, querySizeLimit); + }); } return makeSearchResults(resultSet); } @@ -354,28 +357,28 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { .map(VKey::from) .collect(toImmutableSet()); } else { - return jpaTm() - .transact( - () -> { - CriteriaQueryBuilder builder = - queryItemsSql( - HostResource.class, - "fullyQualifiedHostName", - partialStringQuery, - Optional.empty(), - DeletedItemHandling.EXCLUDE); - if (desiredRegistrar.isPresent()) { - builder = - builder.where( - "currentSponsorClientId", - jpaTm().getEntityManager().getCriteriaBuilder()::equal, - desiredRegistrar.get()); - } - return getMatchingResourcesSql(builder, true, maxNameserversInFirstStage) - .resources().stream() - .map(HostResource::createVKey) - .collect(toImmutableSet()); - }); + return readOnlyJpaTm.transact( + () -> { + CriteriaQueryBuilder builder = + queryItemsSql( + HostResource.class, + "fullyQualifiedHostName", + partialStringQuery, + Optional.empty(), + DeletedItemHandling.EXCLUDE); + if (desiredRegistrar.isPresent()) { + builder = + builder.where( + "currentSponsorClientId", + readOnlyJpaTm.getEntityManager().getCriteriaBuilder()::equal, + desiredRegistrar.get()); + } + return getMatchingResourcesSql(builder, true, maxNameserversInFirstStage) + .resources() + .stream() + .map(HostResource::createVKey) + .collect(toImmutableSet()); + }); } } @@ -509,21 +512,20 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { parameters.put("desiredRegistrar", desiredRegistrar.get()); } hostKeys = - jpaTm() - .transact( - () -> { - javax.persistence.Query query = - jpaTm() - .getEntityManager() - .createNativeQuery(queryBuilder.toString()) - .setMaxResults(maxNameserversInFirstStage); - parameters.build().forEach(query::setParameter); - @SuppressWarnings("unchecked") - Stream resultStream = query.getResultStream(); - return resultStream - .map(repoId -> VKey.create(HostResource.class, repoId)) - .collect(toImmutableSet()); - }); + readOnlyJpaTm.transact( + () -> { + javax.persistence.Query query = + readOnlyJpaTm + .getEntityManager() + .createNativeQuery(queryBuilder.toString()) + .setMaxResults(maxNameserversInFirstStage); + parameters.build().forEach(query::setParameter); + @SuppressWarnings("unchecked") + Stream resultStream = query.getResultStream(); + return resultStream + .map(repoId -> VKey.create(HostResource.class, repoId)) + .collect(toImmutableSet()); + }); } return searchByNameserverRefs(hostKeys); } @@ -568,39 +570,38 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { } stream.forEach(domainSetBuilder::add); } else { - jpaTm() - .transact( - () -> { - for (VKey hostKey : hostKeys) { - CriteriaQueryBuilder queryBuilder = - CriteriaQueryBuilder.create(DomainBase.class) - .whereFieldContains("nsHosts", hostKey) - .orderByAsc("fullyQualifiedDomainName"); - CriteriaBuilder criteriaBuilder = - jpaTm().getEntityManager().getCriteriaBuilder(); - if (!shouldIncludeDeleted()) { - queryBuilder = - queryBuilder.where( - "deletionTime", criteriaBuilder::greaterThan, getRequestTime()); - } - if (cursorString.isPresent()) { - queryBuilder = - queryBuilder.where( - "fullyQualifiedDomainName", - criteriaBuilder::greaterThan, - cursorString.get()); - } - jpaTm() - .criteriaQuery(queryBuilder.build()) - .getResultStream() - .filter(this::isAuthorized) - .forEach( - (domain) -> { - Hibernate.initialize(domain.getDsData()); - domainSetBuilder.add(domain); - }); - } - }); + readOnlyJpaTm.transact( + () -> { + for (VKey hostKey : hostKeys) { + CriteriaQueryBuilder queryBuilder = + CriteriaQueryBuilder.create(DomainBase.class) + .whereFieldContains("nsHosts", hostKey) + .orderByAsc("fullyQualifiedDomainName"); + CriteriaBuilder criteriaBuilder = + readOnlyJpaTm.getEntityManager().getCriteriaBuilder(); + if (!shouldIncludeDeleted()) { + queryBuilder = + queryBuilder.where( + "deletionTime", criteriaBuilder::greaterThan, getRequestTime()); + } + if (cursorString.isPresent()) { + queryBuilder = + queryBuilder.where( + "fullyQualifiedDomainName", + criteriaBuilder::greaterThan, + cursorString.get()); + } + readOnlyJpaTm + .criteriaQuery(queryBuilder.build()) + .getResultStream() + .filter(this::isAuthorized) + .forEach( + (domain) -> { + Hibernate.initialize(domain.getDsData()); + domainSetBuilder.add(domain); + }); + } + }); } } List domains = domainSetBuilder.build().asList(); diff --git a/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java new file mode 100644 index 000000000..82bc97eb8 --- /dev/null +++ b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java @@ -0,0 +1,292 @@ +// Copyright 2022 The Nomulus Authors. All Rights Reserved. +// +// Licensed 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 google.registry.persistence.transaction; + +import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import google.registry.model.ImmutableObject; +import google.registry.persistence.VKey; +import java.util.Optional; +import java.util.function.Supplier; +import java.util.stream.Stream; +import javax.persistence.EntityManager; +import javax.persistence.Query; +import javax.persistence.TypedQuery; +import javax.persistence.criteria.CriteriaQuery; +import org.joda.time.DateTime; + +/** + * A {@link JpaTransactionManager} that simulates a read-only replica SQL instance. + * + *

We accomplish this by delegating all calls to the standard transaction manager except for + * calls that start transactions. For these, we create a transaction like normal but set it to READ + * ONLY mode before doing any work. This is similar to how the read-only Postgres replica works; it + * treats all transactions as read-only transactions. + */ +public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionManager { + + private final JpaTransactionManager delegate; + + public ReplicaSimulatingJpaTransactionManager(JpaTransactionManager delegate) { + this.delegate = delegate; + } + + public void teardown() { + delegate.teardown(); + } + + public EntityManager getStandaloneEntityManager() { + return delegate.getStandaloneEntityManager(); + } + + public EntityManager getEntityManager() { + return delegate.getEntityManager(); + } + + public JpaTransactionManager setDatabaseSnapshot(String snapshotId) { + return delegate.setDatabaseSnapshot(snapshotId); + } + + public TypedQuery query(String sqlString, Class resultClass) { + return delegate.query(sqlString, resultClass); + } + + public TypedQuery criteriaQuery(CriteriaQuery criteriaQuery) { + return delegate.criteriaQuery(criteriaQuery); + } + + public Query query(String sqlString) { + return delegate.query(sqlString); + } + + public boolean inTransaction() { + return delegate.inTransaction(); + } + + public void assertInTransaction() { + delegate.assertInTransaction(); + } + + public T transact(Supplier work) { + return delegate.transact( + () -> { + delegate.getEntityManager().createQuery("SET TRANSACTION READ ONLY").executeUpdate(); + return work.get(); + }); + } + + public T transactWithoutBackup(Supplier work) { + return transact(work); + } + + public T transactNoRetry(Supplier work) { + return transact(work); + } + + public void transact(Runnable work) { + transact( + () -> { + work.run(); + return null; + }); + } + + public void transactNoRetry(Runnable work) { + transact(work); + } + + public T transactNew(Supplier work) { + return transact(work); + } + + public void transactNew(Runnable work) { + transact(work); + } + + public T transactNewReadOnly(Supplier work) { + return transact(work); + } + + public void transactNewReadOnly(Runnable work) { + transact(work); + } + + public T doTransactionless(Supplier work) { + return delegate.doTransactionless(work); + } + + public DateTime getTransactionTime() { + return delegate.getTransactionTime(); + } + + public void insert(Object entity) { + delegate.insert(entity); + } + + public void insertAll(ImmutableCollection entities) { + delegate.insertAll(entities); + } + + public void insertAll(ImmutableObject... entities) { + delegate.insertAll(entities); + } + + public void insertWithoutBackup(ImmutableObject entity) { + delegate.insertWithoutBackup(entity); + } + + public void insertAllWithoutBackup(ImmutableCollection entities) { + delegate.insertAllWithoutBackup(entities); + } + + public void put(Object entity) { + delegate.put(entity); + } + + public void putAll(ImmutableObject... entities) { + delegate.putAll(entities); + } + + public void putAll(ImmutableCollection entities) { + delegate.putAll(entities); + } + + public void putWithoutBackup(ImmutableObject entity) { + delegate.putWithoutBackup(entity); + } + + public void putAllWithoutBackup(ImmutableCollection entities) { + delegate.putAllWithoutBackup(entities); + } + + public void update(Object entity) { + delegate.update(entity); + } + + public void updateAll(ImmutableCollection entities) { + delegate.updateAll(entities); + } + + public void updateAll(ImmutableObject... entities) { + delegate.updateAll(entities); + } + + public void updateWithoutBackup(ImmutableObject entity) { + delegate.updateWithoutBackup(entity); + } + + public void updateAllWithoutBackup(ImmutableCollection entities) { + delegate.updateAllWithoutBackup(entities); + } + + public boolean exists(VKey key) { + return delegate.exists(key); + } + + public boolean exists(Object entity) { + return delegate.exists(entity); + } + + public Optional loadByKeyIfPresent(VKey key) { + return delegate.loadByKeyIfPresent(key); + } + + public ImmutableMap, T> loadByKeysIfPresent( + Iterable> vKeys) { + return delegate.loadByKeysIfPresent(vKeys); + } + + public ImmutableList loadByEntitiesIfPresent(Iterable entities) { + return delegate.loadByEntitiesIfPresent(entities); + } + + public T loadByKey(VKey key) { + return delegate.loadByKey(key); + } + + public ImmutableMap, T> loadByKeys( + Iterable> vKeys) { + return delegate.loadByKeys(vKeys); + } + + public T loadByEntity(T entity) { + return delegate.loadByEntity(entity); + } + + public ImmutableList loadByEntities(Iterable entities) { + return delegate.loadByEntities(entities); + } + + public ImmutableList loadAllOf(Class clazz) { + return delegate.loadAllOf(clazz); + } + + public Stream loadAllOfStream(Class clazz) { + return delegate.loadAllOfStream(clazz); + } + + public Optional loadSingleton(Class clazz) { + return delegate.loadSingleton(clazz); + } + + public void delete(VKey key) { + delegate.delete(key); + } + + public void delete(Iterable> vKeys) { + delegate.delete(vKeys); + } + + public T delete(T entity) { + return delegate.delete(entity); + } + + public void deleteWithoutBackup(VKey key) { + delegate.deleteWithoutBackup(key); + } + + public void deleteWithoutBackup(Iterable> keys) { + delegate.deleteWithoutBackup(keys); + } + + public void deleteWithoutBackup(Object entity) { + delegate.deleteWithoutBackup(entity); + } + + public QueryComposer createQueryComposer(Class entity) { + return delegate.createQueryComposer(entity); + } + + public void clearSessionCache() { + delegate.clearSessionCache(); + } + + public boolean isOfy() { + return delegate.isOfy(); + } + + public void putIgnoringReadOnlyWithoutBackup(Object entity) { + delegate.putIgnoringReadOnlyWithoutBackup(entity); + } + + public void deleteIgnoringReadOnlyWithoutBackup(VKey key) { + delegate.deleteIgnoringReadOnlyWithoutBackup(key); + } + + public void assertDelete(VKey key) { + delegate.assertDelete(key); + } +} diff --git a/core/src/test/java/google/registry/rdap/RdapDomainSearchActionTest.java b/core/src/test/java/google/registry/rdap/RdapDomainSearchActionTest.java index 25c4553ee..32fd4ac14 100644 --- a/core/src/test/java/google/registry/rdap/RdapDomainSearchActionTest.java +++ b/core/src/test/java/google/registry/rdap/RdapDomainSearchActionTest.java @@ -15,6 +15,7 @@ package google.registry.rdap; import static com.google.common.truth.Truth.assertThat; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.rdap.RdapTestHelper.assertThat; import static google.registry.rdap.RdapTestHelper.parseJsonObject; import static google.registry.request.Action.Method.POST; @@ -45,6 +46,7 @@ import google.registry.model.registrar.Registrar; import google.registry.model.reporting.HistoryEntry; import google.registry.model.tld.Registry; import google.registry.persistence.VKey; +import google.registry.persistence.transaction.ReplicaSimulatingJpaTransactionManager; import google.registry.rdap.RdapMetrics.EndpointType; import google.registry.rdap.RdapMetrics.SearchType; import google.registry.rdap.RdapMetrics.WildcardType; @@ -93,39 +95,27 @@ class RdapDomainSearchActionTest extends RdapSearchActionTestCase"); + assertThat(response.getStatus()).isEqualTo(200); + } + @TestOfyAndSql void testDomainMatch_foundWithUpperCase() { login("evilregistrar");