ConfigService gRPC and JS API

[niloc132]

Adds a new gRPC service that provides access to a limited set of configuration properties to unauthenticated users, and a different set to authenticated users.

Includes two drafts of a mechanism to read versions of various jars on the classpath. The goal in either case is to read the runtime version, rather than picking up the version that dhc was compiled against, in case of some version management/conflicts in downstream projects.

There are three "kinds" of versions we collect at this time:

• java.version is special, and is set by the jvm itself
• barrage.version or other external dependencies, where we need to read a version value from the jar. Barring the jar containing a specific file to indicate version, there are roughly two standard ways to do this, both supported by barrage (and an example of each is included):
  • Maven publishes a file at /META-INF/maven/$groupId/$artifactId/pom.properties from which the version property can be read. This doesn't work for jars built outside of maven (such as gradle, or via mvn deploy:deploy-file). Upside is that you can attempt to read it by GAV coords across the classpath.
  • Java jars include a manifest where the Implementation-Version can optionally be added, allowing it to be read from ClassInThatJar.class.getPackage().getImplementationVersion(). This is also optional, and many of our jars don't include it (very quick non-scientific survey showed it was missing from flatbuffers, guava, gwt, deephaven-hash). I'm leaning towards this.
• deephaven.version is the version of the build itself, so could be baked in to a properties file instead of relying on the above.

In order to keep versions available as a deephaven configuration value, they must be set as system properties at runtime, either iterated and set before Configuration is first init'd, or explicitly set on the configuration instance. The commented out code in Main.java is an example of the former, while the ConfigServiceGrpcImpl.java demonstrates the latter. I'm leaning towards the latter, as it makes it possible to read from configuration... to know what versions should be published to configuration.

The current implementation of reading versions from configuration works by expecting a list property be populated with key=fully.qualified.ClassName pairs. That will result in $key.version being published, by finding the Class instance based on the name (without initializing the class), then getting the Implementation-Version key from there. (This could be adjusted to read the pom.properties instead, given a GAV instead of a fqcn).

Fixes #2668

[devinrsmith]

I'm wondering about the "Constants" part of it - at least based on the current impl, users could manually set and change these values at runtime...

[niloc132]

Yep, naming is here to be critiqued. I don't think we want the server to push changes to these configuration values, so from a given client's perspective, we should assume that config values received are functionally constants, but at the same time, there is nothing preventing the server from changing them at runtime without a restart.

[mofojed]

On DHE the config values are certainly treated as constants. I'm alright if they change at runtime as long as there's a notification there's been a change, such that UI can re-load appropriately. It's definitely less complex if we just treat them as constants.

[mofojed]

If we do have changes, sending a disconnect also would force the client to reconnect/fetch the server config values again as well. I don't think we're expecting any use cases where config values change on the fly, I'd think it's easier to just do disconnect/reload everything if the user does want to change config values (which shouldn't happen often?)

[devinrsmith]

Very neat, I didn't know this existed.

[devinrsmith]

Does Barrage provide Implementation-Version? If not, should we add it there instead?

[devinrsmith]

Ah, looks like it does, and this way is not used right now? barrage=io.deephaven.barrage.flatbuf.BarrageMessageWrapper ? I strongly lean towards getImplementationVersion().

[niloc132]

Agreed, I just wanted more than one option to discuss, see description. I think this is the better option, it is slightly more generally adopted, but you still can't assume it is present.

[devinrsmith]

I wonder if there is a way we can have gradle output a "manifest" file of all the GAV coordinates it actually uses at runtime (in the context of jetty-server-app, etc).

[niloc132]

(also discussed in slack)
Certainly possible, not even difficult - but it does but a burden on downstream consumers of the io.deephaven:server-jetty artifact to produce their own such artifact if they happen to manage any dependency differently that DHC itself specifies it. For example, if a bugfix release of barrage were to emerge that io.dh:server-jetty is compatible with, a project could specify that newer one in maven/ivy/sbt/gradle, but the version file wouldn't update unless we had scripts for their given build system.

In contrast, while the Package.getImplementationVersion() is not uniformly applied, it is part of Java itself, so a reasonable deployment should include it. In the event that a dependency doesn't include it but a project needs it, a "dummy" jar could be added to the project that exposes the dependency version, or set a regular configuration property and expose it through this service.

[mofojed]

Just some unused code to remove, tested it out in console and looks good:

var coreClient = new dh.CoreClient('http://localhost:10000')
console.log('serverConfigValues:', await coreClient.getServerConfigValues())
console.log('authConfigValues:', await coreClient.getAuthConfigValues())

[mofojed]

Just making sure - these are general server information, and we don't expect the user to be authenticated before retrieving them.

[niloc132]

Ah good catch - the server will have to auth this and fail the user in this case. i'll double check what dhe does here, but I think this is the same?

[mofojed]

Yes, on DHE it checks if you're authenticated for server config values: https://gitlab.deephaven.io/illumon/iris/-/blob/rc/silverheels/web/server-frontend/src/main/java/com/illumon/iris/web/server/WebApiServerImpl.java#L844
You can get the authConfigValues before being authenticated though (as you need them to know what you can login with): https://gitlab.deephaven.io/illumon/iris/-/blob/rc/silverheels/web/server-frontend/src/main/java/com/illumon/iris/web/server/WebApiServerImpl.java#L384
Also looks like there's a minor bug in DHE - if you try client.getServerConfigValues() before logged in, you'll get a Not logged in error (which makes sense), however if you then login with that same client object and call client.getServerConfigValues() afterward, you still get a Not logged in error. This is because the serverConfigValues is just a lazy that initializes the first time the get is called, and does not reset even if there was an error (or you login): https://gitlab.deephaven.io/illumon/iris/-/blob/rc/silverheels/web/client-api/src/main/java/com/illumon/iris/web/client/api/IrisClient.java#L80
Such a minor issue.

[niloc132]

I'm leaving this as is, re-querying the server each time, to avoid the bug you describe. The tradeoff is that if the client asks for the config again, they will get an extra round trip, and in theory could get different values, going by the current implementation. That might be a feature too, letting the client poll for values.

[mofojed]

I can still get the serverConfigValues without logging in first, as long as I get the auth constants first. I don't think that behaviour is consistent.

If I run:

var coreClient = new dh.CoreClient('http://localhost:10000')

then:

console.log('serverConfigValues:', await coreClient.getServerConfigValues())

I get an error. However, if I do:

var coreClient = new dh.CoreClient('http://localhost:10000')
console.log('authConfigValues:', await coreClient.getAuthConfigValues())

then:

console.log('serverConfigValues:', await coreClient.getServerConfigValues())

Then it works

[devinrsmith]

So, I do want to talk about pros and cons of our choices here wrt https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto. Is there benefit to relying on Struct/Value instead? Or, we could potentially mirror it with our own type and leave out the parts we don't need right now:

message Value {
  // The kind of value.
  oneof kind {
    // Represents a string value.
    string string_value = 3;
  }
}

This might leave us room in the future if we did want to transition to upstream types.
Thoughts on repeated vs map<string, ? We get a bit stronger of typing w/ a map.

[niloc132]

Agreed, in theory, will make the change.

I'm not sure what you mean by stronger typing though, the key is still a string the value is still whatever fits in the one-of/etc, which is still checked by tag. Indeed, the map<k,v> syntax is just sugar over a message with a 1 and 2 tag of the specified type, so key collisions can happen (though presumably the receiving side's runtime will prune the duplicates, like it would for non-repeated fields). Note especially the warning in the docs at https://developers.google.com/protocol-buffers/docs/proto3#backwards_compatibility:

Any protocol buffers implementation that supports maps must both produce and accept data that can be accepted by the above definition.

Functionally, the only difference here is that the oneof is moved out to its own message type, so we just pay for an extra tag "go decode a value now" "here's the type of the data" instead of just "here's the type of the data".

[devinrsmith]

Ah, great link. I didn't realize the semantics of map were equivalent to repeated.

[niloc132]

Updated, PTAL.

[niloc132]

I can still get the serverConfigValues without logging in first, as long as I get the auth constants first. I don't think that behaviour is consistent.

If I run:

var coreClient = new dh.CoreClient('http://localhost:10000')

then:

console.log('serverConfigValues:', await coreClient.getServerConfigValues())

I get an error. However, if I do:

var coreClient = new dh.CoreClient('http://localhost:10000')
console.log('authConfigValues:', await coreClient.getAuthConfigValues())

then:

console.log('serverConfigValues:', await coreClient.getServerConfigValues())

Then it works

Good catch. This is a bug in the new authentication changes - apparently in the absence of an attempt to authenticate, the server assumes you meant to be anonymous and sends back the appropriate headers, and the client "always try to keep auth headers up to date" accepts those. Ill follow-up with that separately.

[niloc132]

Nope, I'm wrong - the client is incorrectly authenticating as anonymous automatically, nothing to do with the server. Once we make CoreClient the only way to get an IdeSession etc, this will go away.