Add mosapi client to intract with ICANN's monitoring system

[njshah301]

This change introduces a comprehensive client to interact with ICANN's Monitoring System API (MoSAPI). This provides direct, automated access to critical registry health and compliance data, moving Nomulus towards a more proactive monitoring posture.

A core, stateless MosApiClient that manages communication and authentication with the MoSAPI service using TLS client certificates.

---

This change is 

[CydeWeys]

Reviewable status: 0 of 17 files reviewed, 6 unresolved discussions (waiting on @njshah301)

---

core/src/main/java/google/registry/config/files/default-config.yaml line 623 at r1 (raw file):

  # URL for the MosAPI
  mosapiUrl: https://mosapi.icann.org
  entityType: ry

Add comment here.

---

core/src/main/java/google/registry/config/files/default-config.yaml line 626 at r1 (raw file):

  # List of TLDs to be monitored.
  tlds:
  - "test"

I'm not sure this is suitable as a default config value -- would empty list be better? What happens if you pass MosAPI a TLD of "test" -- does it actually return test data, or does it error out?

---

core/src/main/java/google/registry/config/RegistryConfigSettings.java line 269 at r1 (raw file):

  /** Configuration for Mosapi. */
  public static class MosApi {
    public String mosapiUrl;

Maybe this should be serverUrl or serviceUrl? It's already in the mosapi section of the JSON so it doesn't need to repeat mosapi again (and the next three correctly don't).

---

core/src/main/java/google/registry/config/RegistryConfig.java line 1437 at r1 (raw file):

     */
    @Provides
    @Config("entityType")

In line with the rest of the @Config names that are namespaced with the prefix mosapi, so too should this one. entityType on its own is too generic to be used project-wide.

---

core/src/main/java/google/registry/config/RegistryConfig.java line 1444 at r1 (raw file):

    @Provides
    @Config("mosapiTlds")
    public static List<String> provideMosapiTlds(RegistryConfigSettings config) {

The return type here should be ImmutableList, same as the other provides in this file, and in-line with the ImmutableCollections style guide of using the types directly rather than a generic mutable interface.

Also should this be a List, or should it be a Set? Does order matter? Are duplicates allowed? I think a Set might make more sense, maybe an ImmutableSortedSet under the hood if it's important to maintain alphabetical order for testing purposes.

---

core/src/main/java/google/registry/config/RegistryConfig.java line 1450 at r1 (raw file):

    @Provides
    @Config("mosapiServices")
    public static List<String> provideMosapiServices(RegistryConfigSettings config) {

Same considerations on return type as previous comment.

[CydeWeys]

Reviewable status: 0 of 17 files reviewed, 18 unresolved discussions (waiting on @njshah301)

---

core/src/main/java/google/registry/mosapi/module/MosApiModule.java line 39 at r1 (raw file):

  public static String provideMosapiTlsCert(SecretManagerClient secretManagerClient) {
    return secretManagerClient.getSecretData(
        "nomulus-dot-foo_tls-client-dot-crt-dot-pem", Optional.of("latest"));

Magic strings like this should be well-named static final String fields, not just repeated as bare strings in multiple places as they are here.

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 42 at r1 (raw file):

  public static MosApiException create(MosApiErrorResponse errorResponse) {
    String message =
        switch (errorResponse.resultCode()) {

This feels like the wrong way to handle this. Each of these cases should be an exception class that inherits from the base exception class, analogous to how EppExceptions work.

Hard to say exactly what the ideal format of this should be given that it's not really used in this PR -- I guess it's used more in subsequent followups?

---

util/src/main/java/google/registry/util/HttpModule.java line 50 at r1 (raw file):

  @Singleton
  @Named("mosapiHttpClient")
  static HttpClient provideMosapiHttpClient(

This being specific to MoSAPI leads me to believe it would be better placed in the mosapi package rather than the util package. The util package is only for things that will be used by multiple other packages (it's at the bottom of the dependency hierarchy), which this won't be.

---

util/src/main/java/google/registry/util/HttpModule.java line 54 at r1 (raw file):

    try {
      // 1. Parse the Certificate first
      CertificateFactory cf = CertificateFactory.getInstance("X.509");

There's a lot of logic here here for a provide method, maybe too much.

Also, a lot of these things you're constructing in here could themselves come from dependency injection.

---

util/src/main/java/google/registry/util/HttpModule.java line 66 at r1 (raw file):

      // 2. Parse the Private Key
      // This regex cleans up PKCS#1, PKCS#8, and all newlines/spaces safely.
      String privateKeyPem =

I feel like there's got to be a better way to read pem files, that is already aware of (and can handle) the typical key formatting/comments.

---

core/src/main/java/google/registry/mosapi/dto/MosApiErrorResponse.java line 15 at r1 (raw file):

// limitations under the License.

package google.registry.mosapi.dto;

This dto package naming convention is not something we use elsewhere in Nomulus. Typically we use model. Is that even necessary here though? How many other records do you envision having in here?

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 15 at r1 (raw file):

// limitations under the License.

package google.registry.mosapi.client;

Does this package needt o be mosapi.client, or can it just be mosapi? What other subpackages of mosapi are you envisioning that necessitates having client be broken off separately like this? It's probably not necessary to have individual packages that each contain only a few things. (And that can also end up requiring wider sharing than necessary, e.g. the default package-private visibility level won't be so useful.)

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 33 at r1 (raw file):

@Singleton
public class MosApiClient {

MosApiClient is basically a facade around your HttpUtils class, which itself is a facade around HttpClient. It seems like all these layers may not be necessary, when all that's happening is just sending a few HTTP requests, which is conceptually pretty simple? There's virtue in simplicity.

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 140 at r1 (raw file):

   * Builds the full URL for a request, including the base URL, entityId, path, and query params.
   */
  private String buildUrl(String entityId, String path, Map<String, String> queryParams) {

There's way too much manual string parsing going on here. Use URLEncoder instead. Also, when you are dealing with URLs, prefer passing around URI objects rather than primitive Strings. At the end of the day, all the way at the bottom of the chain, HttpClient is taking a URI parameter to make its calls, so once you've constructed a URL, pass it around as URI from that point on (rather than as a String).

---

core/src/test/java/google/registry/mosapi/dto/MosApiErrorResponseTest.java line 23 at r1 (raw file):

/** Unit tests for {@link MosApiErrorResponse}. */
public class MosApiErrorResponseTest {
  @Test

Nit: Here and elsewhere, we tend to leave a blank line between the class declaration and the first line of content in it.

---

util/src/main/java/google/registry/util/HttpUtils.java line 22 at r1 (raw file):

import java.io.UncheckedIOException;
import java.net.URI;
import java.net.http.HttpClient;

We're already using okhttp3.OkHttpClient elsewhere in Nomulus; thoughts on that vs this?

---

util/src/main/java/google/registry/util/HttpUtils.java line 58 at r1 (raw file):

  public static HttpResponse<String> sendGetRequest(
      HttpClient httpClient, String url, Map<String, String> headers) {
    HttpRequest.Builder requestBuilder = HttpRequest.newBuilder().uri(URI.create(url)).GET();

The fact that you are calling a wrapping URI.create() around the String url parameter leads me to believe that URLs should be constructed and passed around as URI objects much further up the chain.

[CydeWeys]

Reviewable status: 0 of 17 files reviewed, 21 unresolved discussions (waiting on @njshah301)

---

util/src/main/java/google/registry/util/HttpUtils.java line 137 at r1 (raw file):

  }

  private static BodyHandler<String> createDecompressingBodyHandler() {

There seriously isn't an Http client that handles this transparently? Seems like a lot of boilerplate code to ungzip a byte stream that shouldn't be necessary.

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 43 at r1 (raw file):

    String message =
        switch (errorResponse.resultCode()) {
          case "2012" -> "Date order is invalid: " + errorResponse.message();

Conceptually it seems like we need a MosApiResponse enum, similar to the existing org.apache.http.HttpStatus that we have, except we can make ours as an enum where this one might look something like one fot hese two:

INVALID_DATE_ORDER(2012, "Date order is invalid");
INVALID_DATE_ORDER(2012, "Date order is invalid: %s");

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 82 at r1 (raw file):

   * @throws MosApiAuthorizationException if the server returns a 401 Unauthorized status.
   */
  public HttpResponse<String> sendGetRequestWithDecompression(

I'm not sure there needs to be a separate overload for handling ungzipping of a response? This should always just be handled transparently; if you make an HTTP request and the response comes back gzipped, just handle that automatically at a lower level. Don't make higher level code have to be concerned with that.

[gbrodman]

Reviewable status: 0 of 17 files reviewed, 21 unresolved discussions (waiting on @CydeWeys and @njshah301)

---

core/src/main/java/google/registry/mosapi/module/MosApiModule.java line 39 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Magic strings like this should be well-named static final String fields, not just repeated as bare strings in multiple places as they are here.

also this should be in the configuration, not hard-coded (other users of the open source code won't be called Nomulus)

[njshah301]

I have resolved all comments

Reviewable status: 0 of 20 files reviewed, 20 unresolved discussions (waiting on @CydeWeys and @gbrodman)

---

core/src/main/java/google/registry/config/RegistryConfig.java line 1437 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

In line with the rest of the @Config names that are namespaced with the prefix mosapi, so too should this one. entityType on its own is too generic to be used project-wide.

resolved with @config("mosapiEntityType")

---

core/src/main/java/google/registry/config/RegistryConfig.java line 1444 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The return type here should be ImmutableList, same as the other provides in this file, and in-line with the ImmutableCollections style guide of using the types directly rather than a generic mutable interface.

Also should this be a List, or should it be a Set? Does order matter? Are duplicates allowed? I think a Set might make more sense, maybe an ImmutableSortedSet under the hood if it's important to maintain alphabetical order for testing purposes.

Good suggestion. We can keep ImmutableSet for both provideMosapiTlds and provideMosapiServices as the list of services and list of TLD will be unique and also order doesn't matter.

---

core/src/main/java/google/registry/config/RegistryConfig.java line 1450 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Same considerations on return type as previous comment.

resolved same considering above comment of ImmutableSet

---

core/src/main/java/google/registry/config/RegistryConfigSettings.java line 269 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Maybe this should be serverUrl or serviceUrl? It's already in the mosapi section of the JSON so it doesn't need to repeat mosapi again (and the next three correctly don't).

-Done. I have renamed this to serviceUrl

---

core/src/main/java/google/registry/config/files/default-config.yaml line 623 at r1 (raw file):
added comment

The type of entity being monitored.

For registries, this is 'ry'

For registrars, this is 'rr'

---

core/src/main/java/google/registry/config/files/default-config.yaml line 626 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm not sure this is suitable as a default config value -- would empty list be better? What happens if you pass MosAPI a TLD of "test" -- does it actually return test data, or does it error out?

You're right. I kept placeholder like YOUR_TLD1, YOUR_TLD2, so that people knows that they need to put TLD that they operate on.

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 42 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This feels like the wrong way to handle this. Each of these cases should be an exception class that inherits from the base exception class, analogous to how EppExceptions work.

Hard to say exactly what the ideal format of this should be given that it's not really used in this PR -- I guess it's used more in subsequent followups?

It will be used in followups. I am following the EppException flow to corrrect the format of MosApiException and also added Enum for this flow same as mentioned in EppException

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 43 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Conceptually it seems like we need a MosApiResponse enum, similar to the existing org.apache.http.HttpStatus that we have, except we can make ours as an enum where this one might look something like one fot hese two:

INVALID_DATE_ORDER(2012, "Date order is invalid");
INVALID_DATE_ORDER(2012, "Date order is invalid: %s");

added Enum MosApiResponse to fulfil requirement and also the comments where we will be using this statuses

---

core/src/main/java/google/registry/mosapi/module/MosApiModule.java line 39 at r1 (raw file):

Previously, gbrodman wrote…

also this should be in the configuration, not hard-coded (other users of the open source code won't be called Nomulus)

done with change. Also the TLS related credential configuration is moved to configuration

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 15 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Does this package needt o be mosapi.client, or can it just be mosapi? What other subpackages of mosapi are you envisioning that necessitates having client be broken off separately like this? It's probably not necessary to have individual packages that each contain only a few things. (And that can also end up requiring wider sharing than necessary, e.g. the default package-private visibility level won't be so useful.)

So I thought of having client related code in client package, like MosApiClient, MosApiServiceMonitoringClient, MosApiServiceAlarmClient, MosApiServiceDowntimeClient but your idea also look good to me where we can group classes by feature not type. Will move it to mosapi rather than mosapi.client.

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 33 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

MosApiClient is basically a facade around your HttpUtils class, which itself is a facade around HttpClient. It seems like all these layers may not be necessary, when all that's happening is just sending a few HTTP requests, which is conceptually pretty simple? There's virtue in simplicity.

The MosApiClient is a valuable abstraction because it encapsulates logic and error handling specific to MoSAPI. We are discussing to use between HttpClient vs OkHttp3, so I have kept Okhttp3 for our choice so it has reduced some boilerplate code and now we won't be using HttpUtils and HttpModule.

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 82 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm not sure there needs to be a separate overload for handling ungzipping of a response? This should always just be handled transparently; if you make an HTTP request and the response comes back gzipped, just handle that automatically at a lower level. Don't make higher level code have to be concerned with that.

We are using OkHttp3 and now this logics are removed as OkHttp3 inherently does this process.

---

core/src/main/java/google/registry/mosapi/client/MosApiClient.java line 140 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

There's way too much manual string parsing going on here. Use URLEncoder instead. Also, when you are dealing with URLs, prefer passing around URI objects rather than primitive Strings. At the end of the day, all the way at the bottom of the chain, HttpClient is taking a URI parameter to make its calls, so once you've constructed a URL, pass it around as URI from that point on (rather than as a String).

Good suggestion. I've refactored the code to build and pass HttpUrl objects directly, this will remove lot of boilerplate code since we are utilising OkHttp3

---

util/src/main/java/google/registry/util/HttpModule.java line 50 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This being specific to MoSAPI leads me to believe it would be better placed in the mosapi package rather than the util package. The util package is only for things that will be used by multiple other packages (it's at the bottom of the dependency hierarchy), which this won't be.

make sense, adding it inside MosApiModule

---

util/src/main/java/google/registry/util/HttpModule.java line 54 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

There's a lot of logic here here for a provide method, maybe too much.

Also, a lot of these things you're constructing in here could themselves come from dependency injection.

The logics are divided to seperate dependency injection providers, so now its neat.

---

util/src/main/java/google/registry/util/HttpModule.java line 66 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I feel like there's got to be a better way to read pem files, that is already aware of (and can handle) the typical key formatting/comments.

there is one way which is bouncycastle, I have tested that and I am using it in this

---

util/src/main/java/google/registry/util/HttpUtils.java line 22 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

We're already using okhttp3.OkHttpClient elsewhere in Nomulus; thoughts on that vs this?

using okhttp3 instead of HttpClient as it has much more benefit as per research

---

util/src/main/java/google/registry/util/HttpUtils.java line 58 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The fact that you are calling a wrapping URI.create() around the String url parameter leads me to believe that URLs should be constructed and passed around as URI objects much further up the chain.

done

---

util/src/main/java/google/registry/util/HttpUtils.java line 137 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

There seriously isn't an Http client that handles this transparently? Seems like a lot of boilerplate code to ungzip a byte stream that shouldn't be necessary.

okhttp3 handles this gracefully and I tested end to end

---

core/src/main/java/google/registry/mosapi/dto/MosApiErrorResponse.java line 15 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This dto package naming convention is not something we use elsewhere in Nomulus. Typically we use model. Is that even necessary here though? How many other records do you envision having in here?

I will use model to maintain consistency. There will be more models (10-15) as part of all the API endpoints we discussed.

---

core/src/test/java/google/registry/mosapi/dto/MosApiErrorResponseTest.java line 23 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Nit: Here and elsewhere, we tend to leave a blank line between the class declaration and the first line of content in it.

done

[CydeWeys]

Reviewable status: 0 of 20 files reviewed, 17 unresolved discussions (waiting on @gbrodman and @njshah301)

---

core/src/main/java/google/registry/config/files/default-config.yaml line 626 at r1 (raw file):

Previously, njshah301 (Nilay Shah) wrote…

You're right. I kept placeholder like YOUR_TLD1, YOUR_TLD2, so that people knows that they need to put TLD that they operate on.

Maybe make them lowercase though? Generally TLDs are expressed all lowercase.

[CydeWeys]

Reviewable status: 0 of 20 files reviewed, 9 unresolved discussions (waiting on @gbrodman and @njshah301)

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 15 at r3 (raw file):

// limitations under the License.

package google.registry.mosapi.exception;

Is it really necessary to have a separate exception package namespace just for exception classes? Isn't this the only file that's even going to be in this package anyway?

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 73 at r3 (raw file):

        case DATE_ORDER_INVALID -> new DateOrderInvalidException(errorResponse);
        case START_DATE_SYNTAX_INVALID -> new StartDateSyntaxInvalidException(errorResponse);
        case END_DATE_SYNTAX_INVALID -> new EndDateSyntaxInvalidException(errorResponse);

There should be a default: case here to handle anything else unexpected. (E.g. if at some point someone adds a new value to the enum without also modifying this code.)

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 76 at r3 (raw file):

      return checkResponseForAuthError(response);
    } catch (MosApiAuthorizationException e) {
      throw e;

What's the point of catch e throw e? You can literally just leave this out? MosApiAuthorizationException already extends MosApiException, which is already in the throws clause for the method.

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 78 at r3 (raw file):

      throw e;
    } catch (RuntimeException e) {
      throw new MosApiException("Error during GET request to " + url, e);

I'm not sure what value wrapping these exceptions provides? It's adding a lot of boilerplate code. Also you can just catch both RuntimeException and IOException without having to handle them separately.

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 118 at r3 (raw file):

      throw e;
    } catch (RuntimeException e) {
      throw new MosApiException("Error during POST request to " + url, e);

Same comment on exceptions.

[njshah301]

resolved all commits

Reviewable status: 0 of 20 files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @gbrodman)

---

core/src/main/java/google/registry/config/files/default-config.yaml line 626 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Maybe make them lowercase though? Generally TLDs are expressed all lowercase.

done

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 76 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What's the point of catch e throw e? You can literally just leave this out? MosApiAuthorizationException already extends MosApiException, which is already in the throws clause for the method.

While it seems redundant, the primary purpose of that specific catch block was to ensure that the MosApiAuthorizationException is not accidentally wrapped by the subsequent, more general catch (RuntimeException | IOException e) block but I have updated to couple it in single catch block

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 78 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm not sure what value wrapping these exceptions provides? It's adding a lot of boilerplate code. Also you can just catch both RuntimeException and IOException without having to handle them separately.

done

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 118 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Same comment on exceptions.

done

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 15 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Is it really necessary to have a separate exception package namespace just for exception classes? Isn't this the only file that's even going to be in this package anyway?

yeah, previously planning to contain all exceptions related classes as seperate package, but now it won't be needed

---

core/src/main/java/google/registry/mosapi/exception/MosApiException.java line 73 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

There should be a default: case here to handle anything else unexpected. (E.g. if at some point someone adds a new value to the enum without also modifying this code.)

make sense

[CydeWeys]

Reviewable status: 0 of 20 files reviewed, 2 unresolved discussions (waiting on @gbrodman and @njshah301)

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 46 at r4 (raw file):

    // Pre-calculate base URL and validate it to fail fast on bad config
    String fullUrl = String.format("%s/%s", mosapiUrl, entityType);
    if (HttpUrl.parse(fullUrl) == null) {

These three lines can just be:

checkArgumentNotNull(HttpUrl.parse(fullUrl), ...);

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 76 at r4 (raw file):

    } catch (RuntimeException | IOException e) {
      // Check if it's the specific authorization exception (re-thrown or caught here)
      if (e instanceof MosApiAuthorizationException) {

Look into the methods in the Throwables class, particularly throwIfInstanceOf() (which you want here) and throwIfUnchecked() (which you will likely end up using in other situations).

[njshah301]

resolved comments

Reviewable status: 0 of 20 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman)

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 46 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

These three lines can just be:

checkArgumentNotNull(HttpUrl.parse(fullUrl), ...);

done

---

core/src/main/java/google/registry/mosapi/MosApiClient.java line 76 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Look into the methods in the Throwables class, particularly throwIfInstanceOf() (which you want here) and throwIfUnchecked() (which you will likely end up using in other situations).

done in both places