Connect to the correct endpoints based on runtime

[jianglai]

This PR makes the following to functionality changes:

1. For the nomulus tool, a --gke flag can be passed to make it connect to the GKE endpoints, instead of the default GAE ones.

2. For Cloud Tasks, the correct endpoints will be inferred based on the runtime of the caller, i. e. Nomulus running on GAE will schedule tasks to run against GAE endpoints, and vice versa.

CloudTasksUtils is refactored for simplicity. We no longer provide createGetTask* and createPostTask* variants, in order to avoid combinatorial explosions. The allowed methods are instead checked against the Method enum defined in Action.

TESTED=Deployed to alpha and ran the following test:

1. Ran nomulus -e alpha list_tlds and nomulus --gke -e alpha list_tlds and confirmed that the corresponding GAE/GKE endpoints are called.

2. Ran nomulus -e alpha curl --service TOOLS -u /_dr/admin/list/tlds and nomulus --gke -e alpha curl --service BACKEND -u /_dr/admin/list/tlds and confirmed that the corresponding GAE/GKE endpoints are called.

3. Ran nomulus -e alpha curl -u /_dr/cron/fanout?queue=retryable-cron-tasks&endpoint=/_dr/task/relockDomain&runInEmpty and nomulus --gke -e alpha curl -u /_dr/cron/fanout?queue=retryable-cron-tasks&endpoint=/_dr/task/relockDomain&runInEmpty and confirmed that the corresponding GAE/GKE endpoints are called and Cloud Tasks tasks are enqueued for the appropriate endpoints for the respective runtime, and that these endpoints are in turn called by tasks.

This change is 

[jianglai]

Reviewed 114 of 128 files at r1, 17 of 17 files at r2, 12 of 12 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jianglai)

[jianglai]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gbrodman)

[jianglai]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gbrodman)

[jianglai]

Reviewed 20 of 20 files at r7, 19 of 19 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gbrodman)

[jianglai]

Gentle ping.

[gbrodman]

Reviewed 98 of 128 files at r1, 13 of 17 files at r2, 12 of 12 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 3 of 3 files at r6, 1 of 20 files at r7, 19 of 19 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jianglai)

---

core/src/main/java/google/registry/config/ConfigUtils.java line 32 at r8 (raw file):

  public static URL makeUrl(String url) {
    try {
      return new URI(url).toURL();

Why are we doing the conversion to URI before returning a url?

---

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

  }

  public static String getBaseDomain() {

we could probably change this and provideBaseDomain so that one of them uses the other

---

core/src/main/java/google/registry/tools/ServiceConnection.java line 62 at r8 (raw file):

  protected static final Pattern HTML_TITLE_TAG_PATTERN = Pattern.compile("<title>(.*?)</title>");

  protected final Service service;

can't these all still be private? I don't see any direct access anywhere (or any subclasses)

---

core/src/main/java/google/registry/tools/ServiceConnection.java line 136 at r8 (raw file):

    URL url = service.getServiceUrl();
    // Currently only GAE supports connecting to canary.
    if (service instanceof GaeService && useCanary) {

thoughts on maybe throwing an exception if the ServiceConnection is told to use the canary instance in a GKE environment? I suppose we could probably do that in the constructor

---

core/src/test/java/google/registry/batch/CloudTasksUtilsTest.java line 163 at r8 (raw file):

  @Test
  void testSuccess_createPostTasks() {
    Task task = cloudTasksUtils.createTask(TheAction.class, POST, params);

It seems odd to me that we can attempt to enqueue tasks with a method that is invalid for the given class. Maybe in CloudTasksUtils we verify that the given class's Action annotation contains the provided method and throw an exception otherwise?

For testing, we could just add POST to TheAction and then use OtherAction+POST to test the failure case.

[jianglai]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/config/ConfigUtils.java line 32 at r8 (raw file):

Previously, gbrodman wrote…

Why are we doing the conversion to URI before returning a url?

Because new URL(url) is deprecated...

---

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

Previously, gbrodman wrote…

we could probably change this and provideBaseDomain so that one of them uses the other

Haha, you'd think that, but getBaseDomain() is not part of the dependency injection framework (so it can be used at places outside the dependency tree). In practice, CONFIG_SETTINGS is the static field providing the binding to RegistryConfigSettings anyway, so we could have provideBaseDomain just call getBaseDomain. But I felt that is better to follow the dependency injection idiom.

---

core/src/main/java/google/registry/tools/ServiceConnection.java line 62 at r8 (raw file):

Previously, gbrodman wrote…

can't these all still be private? I don't see any direct access anywhere (or any subclasses)

I think you're right. I changed them to protected in a failed attempt to make the connection field injectable, and forgot to change them back.

---

core/src/main/java/google/registry/tools/ServiceConnection.java line 136 at r8 (raw file):

Previously, gbrodman wrote…

thoughts on maybe throwing an exception if the ServiceConnection is told to use the canary instance in a GKE environment? I suppose we could probably do that in the constructor

Sure. I'm still debating the best way to do canary in GKE, that's why I left it unimplemented for now.

---

core/src/test/java/google/registry/batch/CloudTasksUtilsTest.java line 163 at r8 (raw file):

Previously, gbrodman wrote…

It seems odd to me that we can attempt to enqueue tasks with a method that is invalid for the given class. Maybe in CloudTasksUtils we verify that the given class's Action annotation contains the provided method and throw an exception otherwise?

For testing, we could just add POST to TheAction and then use OtherAction+POST to test the failure case.

Good point. Previously we don't have access to the action class itself. Now that we do, we might as well check the allowed methods.

[jianglai]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gbrodman)

---

core/src/test/java/google/registry/batch/CloudTasksUtilsTest.java line 163 at r8 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Good point. Previously we don't have access to the action class itself. Now that we do, we might as well check the allowed methods.

It actually caught a bug!

[gbrodman]

Reviewed 3 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jianglai)

---

core/src/main/java/google/registry/config/ConfigUtils.java line 32 at r8 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Because new URL(url) is deprecated...

Oh weird! Sounds good to me.

---

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

Previously, jianglai (Lai Jiang) wrote…

Haha, you'd think that, but getBaseDomain() is not part of the dependency injection framework (so it can be used at places outside the dependency tree). In practice, CONFIG_SETTINGS is the static field providing the binding to RegistryConfigSettings anyway, so we could have provideBaseDomain just call getBaseDomain. But I felt that is better to follow the dependency injection idiom.

Eh yeah that's fair. Silly injection.