From f296b225af1f8f57c1032f144e26e1e175ce9582 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Mon, 24 Apr 2017 11:08:29 -0700 Subject: [PATCH] Make FlowReporter log tld and various other fields As part of b/36599833, this makes FlowReporter log the tld(s) of every domain flow it executes, so we can provide ICANN reporting totals on a per-TLD basis. It also adds several other fields that we're computing anyway and which seem useful, particularly for debugging any issues we see in production with the data that we're attempting to record for ICANN reporting. The full set of fields is: - commandType (e.g. "create", "info", "transfer") - resourceType* (e.g. "domain", "contact", "host") - flowClassName (e.g. "ContactCreateFlow", "DomainRestoreRequestFlow") - targetId* (e.g. "ns1.foo.com", "bar.org", "contact-1234") - targetIds* - plural of the above, for multi-resource checks - tld** (e.g. "com", "co.uk") - extracted from targetId, lowercased - tlds** - plural of the above, deduplicated, for multi-resource checks * = only non-empty for resource flows (not e.g. login, logout, poll) ** = only non-empty for domain flows Note that TLD extraction is deliberately very lenient to avoid the complexity overhead of double-validation of the domain names in the common case. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=154070794 --- java/google/registry/flows/FlowReporter.java | 59 ++++++++- .../registry/flows/FlowReporterTest.java | 125 ++++++++++++++++-- .../registry/flows/ResourceFlowTestCase.java | 18 ++- .../flows/domain/ClaimsCheckFlowTest.java | 1 + .../flows/domain/DomainAllocateFlowTest.java | 3 + .../DomainApplicationCreateFlowTest.java | 3 + .../DomainApplicationDeleteFlowTest.java | 1 + .../domain/DomainApplicationInfoFlowTest.java | 1 + .../DomainApplicationUpdateFlowTest.java | 1 + .../flows/domain/DomainCheckFlowTest.java | 5 + .../flows/domain/DomainCreateFlowTest.java | 3 + .../flows/domain/DomainDeleteFlowTest.java | 1 + .../flows/domain/DomainInfoFlowTest.java | 1 + .../flows/domain/DomainRenewFlowTest.java | 1 + .../domain/DomainRestoreRequestFlowTest.java | 3 +- .../domain/DomainTransferApproveFlowTest.java | 1 + .../domain/DomainTransferCancelFlowTest.java | 1 + .../domain/DomainTransferQueryFlowTest.java | 1 + .../domain/DomainTransferRejectFlowTest.java | 1 + .../domain/DomainTransferRequestFlowTest.java | 1 + .../flows/domain/DomainUpdateFlowTest.java | 1 + 21 files changed, 215 insertions(+), 17 deletions(-) diff --git a/java/google/registry/flows/FlowReporter.java b/java/google/registry/flows/FlowReporter.java index b978ec8f4..0f776cc94 100644 --- a/java/google/registry/flows/FlowReporter.java +++ b/java/google/registry/flows/FlowReporter.java @@ -16,12 +16,18 @@ package google.registry.flows; import static com.google.common.io.BaseEncoding.base64; import static google.registry.xml.XmlTransformer.prettyPrint; +import static java.util.Collections.EMPTY_LIST; +import com.google.common.base.Ascii; +import com.google.common.base.Optional; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import google.registry.flows.FlowModule.ClientId; import google.registry.flows.FlowModule.InputXml; import google.registry.flows.annotations.ReportingSpec; import google.registry.model.eppcommon.Trid; +import google.registry.model.eppinput.EppInput; import google.registry.util.FormattingLogger; import javax.inject.Inject; import org.json.simple.JSONValue; @@ -48,6 +54,7 @@ public class FlowReporter { @Inject Trid trid; @Inject @ClientId String clientId; @Inject @InputXml byte[] inputXmlBytes; + @Inject EppInput eppInput; @Inject Class flowClass; @Inject FlowReporter() {} @@ -64,13 +71,57 @@ public class FlowReporter { // Explicitly log flow metadata separately from the EPP XML itself so that it stays compact // enough to be sure to fit in a single log entry (the XML part in rare cases could be long // enough to overflow into multiple log entries, breaking routine parsing of the JSON format). + String resourceType = eppInput.getResourceType().or(""); + boolean isDomain = "domain".equals(resourceType); + String singleTargetId = eppInput.getSingleTargetId().or(""); + ImmutableList targetIds = eppInput.getTargetIds(); logger.infofmt( "%s: %s", METADATA_LOG_SIGNATURE, - JSONValue.toJSONString(ImmutableMap.of( - "trid", trid.getServerTransactionId(), - "clientId", clientId, - "icannActivityReportField", extractActivityReportField(flowClass)))); + JSONValue.toJSONString(new ImmutableMap.Builder() + .put("serverTrid", trid.getServerTransactionId()) + .put("clientId", clientId) + .put("commandType", eppInput.getCommandType()) + .put("resourceType", resourceType) + .put("flowClassName", flowClass.getSimpleName()) + .put("targetId", singleTargetId) + .put("targetIds", targetIds) + .put("tld", isDomain ? extractTld(singleTargetId).or("") : "") + .put("tlds", isDomain ? extractTlds(targetIds).asList() : EMPTY_LIST) + .put("icannActivityReportField", extractActivityReportField(flowClass)) + .build())); + } + + /** + * Returns the guessed TLD of the given domain name, assuming a second-level domain name, or + * absent if no TLD could be detected. + * + *

This method is quick and dirty and doesn't attempt to validate the domain name in any way; + * it just takes anything after the first period to be the TLD and converts ASCII to lowercase. + * We want quick and dirty here because this will be called on not-yet-validated EPP XML where + * just about anything could be supplied, and there's no reason to validate twice when this just + * needs to be roughly correct. + */ + private static final Optional extractTld(String domainName) { + int index = domainName.indexOf('.'); + return index == -1 + ? Optional.absent() + : Optional.of(Ascii.toLowerCase(domainName.substring(index + 1))); + } + + /** + * Returns the set of unique results of {@link #extractTld} applied to each given domain name, + * excluding any absent results (i.e. cases where no TLD was detected). + */ + private static final ImmutableSet extractTlds(Iterable domainNames) { + ImmutableSet.Builder set = new ImmutableSet.Builder<>(); + for (String domainName : domainNames) { + Optional extractedTld = extractTld(domainName); + if (extractedTld.isPresent()) { + set.add(extractedTld.get()); + } + } + return set.build(); } /** diff --git a/javatests/google/registry/flows/FlowReporterTest.java b/javatests/google/registry/flows/FlowReporterTest.java index 890945b19..6b1a56ff4 100644 --- a/javatests/google/registry/flows/FlowReporterTest.java +++ b/javatests/google/registry/flows/FlowReporterTest.java @@ -19,13 +19,17 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions; import static java.nio.charset.StandardCharsets.UTF_8; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import com.google.common.base.Optional; import com.google.common.base.Predicate; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.testing.TestLogHandler; import google.registry.flows.annotations.ReportingSpec; import google.registry.model.eppcommon.Trid; +import google.registry.model.eppinput.EppInput; import google.registry.model.eppoutput.EppOutput.ResponseOrGreeting; import google.registry.model.eppoutput.EppResponse; import google.registry.model.reporting.IcannReportingTypes.ActivityReportField; @@ -68,6 +72,11 @@ public class FlowReporterTest extends ShardableTestCase { flowReporter.clientId = "TheRegistrar"; flowReporter.inputXmlBytes = "".getBytes(UTF_8); flowReporter.flowClass = TestCommandFlow.class; + flowReporter.eppInput = mock(EppInput.class); + when(flowReporter.eppInput.getCommandType()).thenReturn("info"); + when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.of("domain")); + when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("target.foo")); + when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("target.foo")); } @Test @@ -96,8 +105,15 @@ public class FlowReporterTest extends ShardableTestCase { flowReporter.recordToLogs(); assertThat(parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: "))) .containsExactly( - "trid", "server-456", + "serverTrid", "server-456", "clientId", "TheRegistrar", + "commandType", "info", + "resourceType", "domain", + "flowClassName", "TestCommandFlow", + "targetId", "target.foo", + "targetIds", ImmutableList.of("target.foo"), + "tld", "foo", + "tlds", ImmutableList.of("foo"), "icannActivityReportField", ""); } @@ -105,22 +121,109 @@ public class FlowReporterTest extends ShardableTestCase { public void testRecordToLogs_metadata_withReportingSpec() throws Exception { flowReporter.flowClass = TestReportingSpecCommandFlow.class; flowReporter.recordToLogs(); - assertThat(parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: "))) - .containsExactly( - "trid", "server-456", - "clientId", "TheRegistrar", - "icannActivityReportField", "srs-cont-check"); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("flowClassName", "TestReportingSpecCommandFlow"); + assertThat(json).containsEntry("icannActivityReportField", "srs-cont-check"); } @Test public void testRecordToLogs_metadata_noClientId() throws Exception { flowReporter.clientId = ""; flowReporter.recordToLogs(); - assertThat(parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: "))) - .containsExactly( - "trid", "server-456", - "clientId", "", - "icannActivityReportField", ""); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("clientId", ""); + } + + @Test + public void testRecordToLogs_metadata_notResourceFlow_noResourceTypeOrTld() throws Exception { + when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.absent()); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("resourceType", ""); + assertThat(json).containsEntry("tld", ""); + assertThat(json).containsEntry("tlds", ImmutableList.of()); + } + + + @Test + public void testRecordToLogs_metadata_notDomainFlow_noTld() throws Exception { + when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.of("contact")); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("resourceType", "contact"); + assertThat(json).containsEntry("tld", ""); + assertThat(json).containsEntry("tlds", ImmutableList.of()); + } + + @Test + public void testRecordToLogs_metadata_multipartDomainName_multipartTld() throws Exception { + when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("target.co.uk")); + when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("target.co.uk")); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("targetId", "target.co.uk"); + assertThat(json).containsEntry("targetIds", ImmutableList.of("target.co.uk")); + assertThat(json).containsEntry("tld", "co.uk"); + assertThat(json).containsEntry("tlds", ImmutableList.of("co.uk")); + } + + @Test + public void testRecordToLogs_metadata_multipleTargetIds_uniqueTldSet() throws Exception { + when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.absent()); + when(flowReporter.eppInput.getTargetIds()) + .thenReturn(ImmutableList.of("target.co.uk", "foo.uk", "bar.uk", "baz.com")); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("targetId", ""); + assertThat(json).containsEntry( + "targetIds", ImmutableList.of("target.co.uk", "foo.uk", "bar.uk", "baz.com")); + assertThat(json).containsEntry("tld", ""); + assertThat(json).containsEntry("tlds", ImmutableList.of("co.uk", "uk", "com")); + } + + @Test + public void testRecordToLogs_metadata_uppercaseDomainName_lowercaseTld() throws Exception { + when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("TARGET.FOO")); + when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("TARGET.FOO")); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("targetId", "TARGET.FOO"); + assertThat(json).containsEntry("targetIds", ImmutableList.of("TARGET.FOO")); + assertThat(json).containsEntry("tld", "foo"); + assertThat(json).containsEntry("tlds", ImmutableList.of("foo")); + } + + @Test + public void testRecordToLogs_metadata_invalidDomainName_stillGuessesTld() throws Exception { + when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("")); + when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("")); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("targetId", ""); + assertThat(json).containsEntry("targetIds", ImmutableList.of("")); + assertThat(json).containsEntry("tld", "com>"); + assertThat(json).containsEntry("tlds", ImmutableList.of("com>")); + } + + @Test + public void testRecordToLogs_metadata_domainWithoutPeriod_noTld() throws Exception { + when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("target,foo")); + when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("target,foo")); + flowReporter.recordToLogs(); + Map json = + parseJsonMap(findLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: ")); + assertThat(json).containsEntry("targetId", "target,foo"); + assertThat(json).containsEntry("targetIds", ImmutableList.of("target,foo")); + assertThat(json).containsEntry("tld", ""); + assertThat(json).containsEntry("tlds", ImmutableList.of()); } @SuppressWarnings("unchecked") diff --git a/javatests/google/registry/flows/ResourceFlowTestCase.java b/javatests/google/registry/flows/ResourceFlowTestCase.java index 1559d1d1d..ca0790bca 100644 --- a/javatests/google/registry/flows/ResourceFlowTestCase.java +++ b/javatests/google/registry/flows/ResourceFlowTestCase.java @@ -45,6 +45,7 @@ import java.util.logging.Level; import java.util.logging.Logger; import org.joda.time.DateTime; import org.joda.time.Duration; +import org.json.simple.JSONValue; import org.junit.Before; import org.junit.Test; @@ -173,10 +174,25 @@ public abstract class ResourceFlowTestCase