Migrate external EPP proxy to integrated Nomulus EPP server#3126
Conversation
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
| Matcher matcher = CLID_PATTERN.matcher(xml); | ||
| if (matcher.find()) { | ||
| registrarId = matcher.group(1).trim(); | ||
| logger.atInfo().log("Identified registrar: %s", registrarId); |
| public void channelRead(ChannelHandlerContext ctx, Object msg) { | ||
| ByteBuf buf = (ByteBuf) msg; | ||
| if (buf.equals(checkRequest)) { | ||
| ChannelFuture unusedFuture = ctx.writeAndFlush(checkResponse); |
| Future<?> unusedFuture = channel.close(); | ||
| }); | ||
| Future<?> unusedFuture = eventGroup.shutdownGracefully(); | ||
| Future<?> unusedFutureBusiness = businessGroup.shutdownGracefully(); |
| channel -> { | ||
| Future<?> unusedFuture = channel.close(); | ||
| }); | ||
| Future<?> unusedFuture = eventGroup.shutdownGracefully(); |
| .values() | ||
| .forEach( | ||
| channel -> { | ||
| Future<?> unusedFuture = channel.close(); |
|
FWIW I think most of the new code is copied from the proxy code base. You should use |
ptkach
left a comment
There was a problem hiding this comment.
Unfortunately that wasn't possible as we want to keep old proxy running in parallel with the new one, while we're testing the new service and it might take a while, as such the old proxy deletion will be a separate change.
@ptkach made 1 comment.
Reviewable status: 0 of 42 files reviewed, 15 unresolved discussions.
|
That's too bad. But I think you can salvage the situation by making a copy of the current proxy code (therefore losing the history), updating your build process to build the proxy from the copy, then renaming/refactoring your existing proxy code into the epp server. There is more work, but you get a cleaner result in the end where the full history is preserved, and the refactor is easier to review as only the changes from proxy to epp server are highlighted. I think you can ask agent to do the copying part and it should do a decent job. But it's ultimately your decision how you want to handle it. |
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
| // Simulate FakeHttpServletResponse modifying headers and writing payload | ||
| doAnswer( | ||
| invocation -> { | ||
| FakeHttpServletRequest req = invocation.getArgument(0); |
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.close(); |
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.writeAndFlush(out); |
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.writeAndFlush(out).addListener(ChannelFutureListener.CLOSE); |
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.close(); |
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.close(); |
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.close(); |
| @SuppressWarnings("unused") | ||
| Future<?> unusedFuture = ctx.close(); |
b104b79 to
304cf08
Compare
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
| ChannelHandler handler = handlerProvider.get(); | ||
| String handlerName = handler.getClass().getSimpleName(); | ||
| if (handlerName.equals("EppServiceHandler")) { | ||
| channelPipeline.addLast(businessGroup, handlerName, handler); |
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
gbrodman
left a comment
There was a problem hiding this comment.
nit: should change the commit message for more details and to use an action verb
@gbrodman reviewed 44 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on ptkach).
core/src/main/java/google/registry/config/files/default-config.yaml line 437 at r6 (raw file):
eppServer: # The integrated EPP server listens on this port. port: 30002
should we default to 700? i haven't read through the rest of this PR yet so i don't know if that's a valid question
core/src/main/java/google/registry/eppserver/EppServer.java line 80 at r6 (raw file):
ChannelHandler handler = handlerProvider.get(); String handlerName = handler.getClass().getSimpleName(); if (handlerName.equals("EppServiceHandler")) {
i think we can just compare handler.getClass() to the class directly, rather than hardcoding the class simple name?
core/src/main/java/google/registry/eppserver/handler/EppServiceHandler.java line 146 at r6 (raw file):
// 2. Trigger initial EPP <greeting> handleEppFrame(ctx, Unpooled.wrappedBuffer(helloBytes));
i think this is happening on the main IO thread right? since this just happens after we resolve the cert. I am pretty sure we want to offload this to an executor so that the initial hello isn't blocking the IO thread
core/src/main/java/google/registry/eppserver/quota/LocalConnectionLimiter.java line 77 at r6 (raw file):
} private void release(String key, ConcurrentHashMap<String, AtomicInteger> map) {
i think we need to computeIfAbsent this too otherwise one thread could increment an AtomicInteger that has already been removed from the map
core/src/main/java/google/registry/eppserver/quota/QuotaManager.java line 40 at r6 (raw file):
/** Lua script to atomically decrement a token bucket with a TTL. */ private static final String DECR_LUA =
you could probably use triplequoted strings for these , your call if that's cleaner
core/src/main/java/google/registry/util/FakeHttpServletRequest.java line 1 at r6 (raw file):
// Copyright 2024 The Nomulus Authors. All Rights Reserved.
2026? worth checking copyright dates on all the files since there's a ton of variance. Though I'm not sure how to treat copyright dates on the moved proxy files
core/src/main/java/google/registry/util/FakeHttpServletRequest.java line 53 at r6 (raw file):
private final Map<String, String> headers = new HashMap<>(); private final Map<String, Object> attributes = new HashMap<>(); private String method = "POST";
i think many of these can be final?
jetty/deploy-nomulus-for-env.sh line 36 at r6 (raw file):
echo "Updating cluster ${parts[0]} in location ${parts[1]}..." gcloud container fleet memberships get-credentials "${parts[0]}" --project "${project}" for service in frontend backend pubapi console epp-server
are we just going to have this open for anyone to connect to if they want? just double checking
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on ptkach).
core/src/main/java/google/registry/util/FakeHttpServletResponse.java line 68 at r6 (raw file):
@Override public void addHeader(String name, String value) { headers.put(name, value);
not a huge deal, but i'm p sure this should append the name+value on to the existing header
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
ptkach
left a comment
There was a problem hiding this comment.
Updated PR message and description, update commit title
@ptkach made 7 comments.
Reviewable status: 38 of 44 files reviewed, 24 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/config/files/default-config.yaml line 437 at r6 (raw file):
Previously, gbrodman wrote…
should we default to 700? i haven't read through the rest of this PR yet so i don't know if that's a valid question
30002 is an internal port, external port is 700, same as the old proxy
core/src/main/java/google/registry/eppserver/EppServer.java line 80 at r6 (raw file):
Previously, gbrodman wrote…
i think we can just compare handler.getClass() to the class directly, rather than hardcoding the class simple name?
Done
core/src/main/java/google/registry/eppserver/handler/EppServiceHandler.java line 146 at r6 (raw file):
Previously, gbrodman wrote…
i think this is happening on the main IO thread right? since this just happens after we resolve the cert. I am pretty sure we want to offload this to an executor so that the initial hello isn't blocking the IO thread
Good catch, I've copied it from proxy as is, but it works differently there so now executor is required.
core/src/main/java/google/registry/eppserver/quota/LocalConnectionLimiter.java line 77 at r6 (raw file):
Previously, gbrodman wrote…
i think we need to computeIfAbsent this too otherwise one thread could increment an AtomicInteger that has already been removed from the map
Good point, updated
core/src/main/java/google/registry/util/FakeHttpServletRequest.java line 53 at r6 (raw file):
Previously, gbrodman wrote…
i think many of these can be final?
Updated servername and contentType, method, requestUri, and body can not be set as final they have setters
jetty/deploy-nomulus-for-env.sh line 36 at r6 (raw file):
Previously, gbrodman wrote…
are we just going to have this open for anyone to connect to if they want? just double checking
Discussed offline
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 6 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on ptkach).
core/src/main/java/google/registry/eppserver/quota/LocalConnectionLimiter.java line 77 at r6 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
Good point, updated
whoops yeah, I meant computeIfPresent as you noted
I don't think we need or want to remove the AtomicIntegers entirely, as that makes acquire more complicated. I think you could keep acquire as is, and just change this to
computeIfPresent(
key,
(k, v) -> {
if (v.decrementAndGet() <= 0) {
return null;
}
return v;
}
that way we can be sure that the AtomicIntegers returned by computeIfPresent and computeIfAbsent will always be the "same" object.
core/src/main/java/google/registry/eppserver/EppServer.java line 81 at r7 (raw file):
ChannelHandler handler = handlerProvider.get(); String handlerName = handler.getClass().getSimpleName(); if (handler.getClass() == EppServiceHandler.class) {
TIL you can use double-equals for class objects
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
ptkach
left a comment
There was a problem hiding this comment.
@ptkach made 4 comments.
Reviewable status: 41 of 44 files reviewed, 20 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/eppserver/quota/LocalConnectionLimiter.java line 77 at r6 (raw file):
Previously, gbrodman wrote…
whoops yeah, I meant computeIfPresent as you noted
I don't think we need or want to remove the AtomicIntegers entirely, as that makes
acquiremore complicated. I think you could keepacquireas is, and just change this tocomputeIfPresent( key, (k, v) -> { if (v.decrementAndGet() <= 0) { return null; } return v; }that way we can be sure that the AtomicIntegers returned by computeIfPresent and computeIfAbsent will always be the "same" object.
That's what I did the first time, but AI still validated this as a race condition. Integet approach makes it bullet-proof. The race condition with what you suggested is the following:
- Initial State: Map contains
{"IP_A": AtomicInteger(1)}(1 active connection). - Thread B (Acquire): Calls
map.computeIfAbsent("IP_A", ...)which returns the reference to the existingAtomicInteger(value 1). - Thread A (Release): Calls
map.computeIfPresent("IP_A", ...)which locks key"IP_A".- It decrements the
AtomicIntegervalue to0. - Since
0 <= 0, it returnsnullwhich removes"IP_A"from the map. - It releases the lock.
- It decrements the
- Thread B: Calls
count.incrementAndGet()on the reference it obtained in Step 2. The value becomes1. Thread B thinks it successfully acquired a connection. - Current State:
- Connection B is active (represented by the orphaned
AtomicIntegerof value1which is no longer in the map). - The map has no entry for
"IP_A".
- Connection B is active (represented by the orphaned
- Thread C (Acquire): Calls
map.computeIfAbsent("IP_A", ...)-> since the key is missing, it creates a newAtomicInteger(0)and puts it in the map. - Thread C: Calls
count.incrementAndGet()-> value becomes1. (Thread C successfully acquires). - State: Both Connection B and Connection C are active (2 connections). The map only contains
{"IP_A": AtomicInteger(1)}(representing C). - Thread B (Release): Calls
map.computeIfPresent("IP_A", ...)-> obtains reference to Thread C'sAtomicInteger(value 1). - Thread B: Decrements Thread C's
AtomicIntegerto0and returnsnull-> removes"IP_A"from the map.
core/src/main/java/google/registry/eppserver/quota/QuotaManager.java line 40 at r6 (raw file):
Previously, gbrodman wrote…
you could probably use triplequoted strings for these , your call if that's cleaner
Thats' fine for now
core/src/main/java/google/registry/util/FakeHttpServletRequest.java line 1 at r6 (raw file):
Previously, gbrodman wrote…
2026? worth checking copyright dates on all the files since there's a ton of variance. Though I'm not sure how to treat copyright dates on the moved proxy files
Repaced
core/src/main/java/google/registry/util/FakeHttpServletResponse.java line 68 at r6 (raw file):
Previously, gbrodman wrote…
not a huge deal, but i'm p sure this should append the name+value on to the existing header
Done.
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 3 files and all commit messages, made 2 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on ptkach).
core/src/main/java/google/registry/eppserver/quota/LocalConnectionLimiter.java line 77 at r6 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
That's what I did the first time, but AI still validated this as a race condition. Integet approach makes it bullet-proof. The race condition with what you suggested is the following:
- Initial State: Map contains
{"IP_A": AtomicInteger(1)}(1 active connection).- Thread B (Acquire): Calls
map.computeIfAbsent("IP_A", ...)which returns the reference to the existingAtomicInteger(value 1).- Thread A (Release): Calls
map.computeIfPresent("IP_A", ...)which locks key"IP_A".
- It decrements the
AtomicIntegervalue to0.- Since
0 <= 0, it returnsnullwhich removes"IP_A"from the map.- It releases the lock.
- Thread B: Calls
count.incrementAndGet()on the reference it obtained in Step 2. The value becomes1. Thread B thinks it successfully acquired a connection.- Current State:
- Connection B is active (represented by the orphaned
AtomicIntegerof value1which is no longer in the map).- The map has no entry for
"IP_A".- Thread C (Acquire): Calls
map.computeIfAbsent("IP_A", ...)-> since the key is missing, it creates a newAtomicInteger(0)and puts it in the map.- Thread C: Calls
count.incrementAndGet()-> value becomes1. (Thread C successfully acquires).- State: Both Connection B and Connection C are active (2 connections). The map only contains
{"IP_A": AtomicInteger(1)}(representing C).- Thread B (Release): Calls
map.computeIfPresent("IP_A", ...)-> obtains reference to Thread C'sAtomicInteger(value 1).- Thread B: Decrements Thread C's
AtomicIntegerto0and returnsnull-> removes"IP_A"from the map.
yeah i see what you mean, we def need to essentially have the whole thing in a lock (which is what this now does).
core/src/main/java/google/registry/eppserver/quota/LocalConnectionLimiter.java line 78 at r8 (raw file):
return v + 1; } acquired[0] = false;
this line isn't necessary, though idk if the compiler complains if you get rid of it
you could also replace the contents if (v == null) {...} block with just v = 0 if you want, then it'll go through the other normal v < limit path. Either way probably fine.
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman reviewed 7 files and all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on ptkach).
relevant issues have been resolved
There was a problem hiding this comment.
⚠️ Attention Required: Lockfile Detected
This pull request contains modifications to one or more *.lockfile files. Please confirm that you have run update_dependency.sh to push new dependencies to the private repo.
Someone with Admin role must manually dismiss this review before merging.
relevant issues have been resolved
Migrate EPP traffic handling from the external EPP proxy container to an integrated, in-process Netty-based EPP server running within the Nomulus frontend container.
proxycontainer with an in-process Netty TCP server (EppServer) listening for registrar EPP traffic.EppServiceHandler->FakeHttpServletRequest->RegistryServlet).businessGroup) to prevent blocking the Netty I/O loop.epp-serverGKE service using a Layer 4 regional Load Balancer withexternalTrafficPolicy: Localto preserve the original client IP.Nomulus-Client-AddressandX-SSL-Certificate) to EPP validation flows.QuotaManagerutilizing Valkey (Redis) Lua scripts.EppServerLifecycleListenerto manage EPP Netty server bootstrap and teardown.MetricReporterstartup and shutdown underMetricsLifecycleListenerto ensure JVM and EPP server metrics are correctly pushed to Stackdriver.This change is