-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Address and Services registration to connect to Services #43547
base: main
Are you sure you want to change the base?
Conversation
It was opened as a draft to discuss the direction. There are still many missing pieces, and the implementation is far from complete, so it may be difficult to review, but I think we need intermediary reviews before we get too invested in the implementation to avoid wasted work. |
See #43220 for additional context |
🎊 PR Preview f0762e4 has been successfully built and deployed to https://quarkus-pr-main-43547-preview.surge.sh/version/main/guides/
|
I need to think about it, but it's close to Stork. I think it can be confusing. |
Also, some data will not be available at startup time, but "later". |
Which data? |
@@ -21,7 +21,7 @@ class RestClientRandomPortTest { | |||
.addClasses(EchoResource.class, EchoClient.class)) | |||
.overrideRuntimeConfigKey("quarkus.http.port", "0") | |||
.overrideRuntimeConfigKey("quarkus.http.test-port", "0") | |||
.overrideRuntimeConfigKey("quarkus.rest-client.EchoClient.url", "http://localhost:${quarkus.http.port}"); | |||
.overrideRuntimeConfigKey("quarkus.rest-client.EchoClient.url", "quarkus://vertx-http"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use URLs in this way. The URL specifies the protocol to use, not the address. We could potentially have a special syntax for referencing a service address within a configuration key, alongside an "aware" parser/property type that knows how to parse it. We'd need one for URL configs, and a separate one (or maybe two) for socket address/port number configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it's bad, by the way, just in this case for example we don't know whether it's http
or https
. We could theoretically glean that information from the service, but that's not 100% foolproof either because there could conceivably be URL schemes which are layered combinations of protocols, where the server would (at best) only be aware of the outer protocol. But that's just one example; any time you have a mismatch like this, it will be accompanied by a variety of subtle and hard-to-fix problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case quarkus://vertx-http
is not an URL
at all. Is just the key to lookup the service. It could has been foo.bar
, as long as the service would register itselft as foo.bar
. The service lookup would then return the proper URL
.
I admit it is confusing because on how I choose the key name and the configuration name (which is current name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I got that. But the problem is that the service is identified on the client differently than on the server. We lose information that would normally be found only in the client URL scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a way to make a short alias and then for us to discuss. And doesnt' have to be this way.
I'm not expecting to ask users to go and change all their configuration for service names, so setting the standard url as http://localhost:${quarkus.http.port}
would still need to be valid and we would need to rewrite the port (in case of randoms to use the right port).
Or we can just drop the names / alias completely and do only the rewriting of the port when required. Or do you want to make a hard break on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little complex because services could be defined by host+port, by just port, by socket address (123.45.67.89:1234
), etc. And, clients might define their access to that service by URL, by socket address, or by host+port, or some other creative scheme.
I think the abstraction of a "service" or "network service" is a good start. We can strictly define them as a "named" socket address. This is unambiguous (and actually corresponds reasonably well with what WildFly does).
If a client connects by socket address, then we would need a way to say "socket address or service", with a corresponding class that works with Converter
that can give the correct SocketAddress
when given access to the registry of running services.
If a client connects by URL, then we would need a way represent a URL with either a "normal" host/port part or else some token which signifies a local service should be used. This would be an extension relative to valid URL syntax so we cannot use java.net.URL/URI
to represent these; rather we'd have a Converter
-friendly class which can hold either a plain URL or else something that can resolve to a URL when given access to the registry of running services.
We could use a uniform syntax for these cases e.g. @{serviceName}
or https://@{serviceName}/my/path
, however if we do so, it should be made very clear that this syntax is not resolved by the configuration layer like properties are, but rather by the property's Converter
itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except it should not be named service, I prefer David's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit puzzled.
It seems that the proposed solution is actually a bit far from the initial problem (random ports). It seems to be a mix between service discovery and address resolution.
We should not have something that will introduce confusion.
private final String host; | ||
private final int port; | ||
|
||
public Service(final String protocol, final String host, final int port) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I already said, this is very close to Stork, and I think, it can be confusing.
Stork also has services, with a very similar API (with secure
as additional field indicating if TLS needs to be used or not).
throw new UnsupportedOperationException(); | ||
} | ||
|
||
public static Optional<Service> get(String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The registration may happen at any time, and there is not way for a consumer to listen for the registration.
import java.util.Objects; | ||
import java.util.Optional; | ||
|
||
public final class Address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vert.x 5 introduced Address andAddressResolver - that might be confusing to have something so close in term of naming (and actually locgic too)
} | ||
|
||
public URI toUri() { | ||
return URI.create(protocol + "://" + host + ":" + port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's slightly more complicated in reality (TLS or not, path
(when using proxy...)
SmallRyeConfig config = ConfigProvider.getConfig().unwrap(SmallRyeConfig.class); | ||
return config.getOptionalValue(uriValue().getName(), String.class); | ||
} | ||
Optional<Address> uri(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it can be confusing when using stork, as stork will use an URL or baseURL starting with stork://<service-name>
.
It's unclear if you need to use Stork or an Address.
@@ -21,7 +21,7 @@ class RestClientRandomPortTest { | |||
.addClasses(EchoResource.class, EchoClient.class)) | |||
.overrideRuntimeConfigKey("quarkus.http.port", "0") | |||
.overrideRuntimeConfigKey("quarkus.http.test-port", "0") | |||
.overrideRuntimeConfigKey("quarkus.rest-client.EchoClient.url", "http://localhost:${quarkus.http.port}"); | |||
.overrideRuntimeConfigKey("quarkus.rest-client.EchoClient.url", "quarkus://vertx-http"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except it should not be named service, I prefer David's suggestion.
Provide a
Services
repository to register services like Vert.x in a known alias. Consumers can then use these aliases to obtain the requiredAddress
to connect to the service:my.service.url=quarkus://vertx-http
The
Address
configuration can either use the service aliasquarkus://vertx-http
or a valid urlhttp://localhost:8080
. TheAddress
will first query the registry and transform the service alias to the actual address where the service is running or just return the actual value if the service cannot be found.One of the main goals of this change is to stop relying on system properties and configuration mutation to carry port changes between the configuration and the runtime service. See #42986.
Ideally,
Address
could also handlehttp://localhost:0
and query the registry for the service to use, which would allow us to avoid introducing breaking changes in the configuration.