Reflection Service Fixes and Updates

[kt-jplotzke]

This pull request provides updates to the reflection service which is used by the grpc-labview server.

Primarily, two main issues are addressed:

• Previously, only the first serialized proto descriptor string was used and published by the reflection service. (fixes Reflection information is only published for a single service #452)
• Descriptor strings were passed from LabVIEW to the DLL without identifying which server instance to apply the description to. As a result, if multiple servers were started, each with different available services, the server may publish incorrect services via reflection.

DLL Updates

gRPC Reflection Service

Previously, the DLL used a ProtoDescriptorString singleton class to store the (single) descriptor string. This class has been eliminated.

The reflection service previously was instantiated using a gRPC ServerBuilder plugin architecture. Using this architecture, a LVProtoServerReflectionPlugin object was created for each LabVIEWgRPCServer and then subsequently created a LVProtoServerReflectionService instance using the CreateLVProtoReflection factory method. Because of the factory-style construction, specific arguments are not able to be passed to the LVProtoServerReflectionService object to define which service(s) are to be published by the service (as each reflection service may publish different information). That's why the ProtoDescriptorString singleton was used as a "global"-type variable before.

grpc-labview/src/lv_proto_server_reflection_plugin.cc

Lines 51 to 64 in 60727eb

	std::unique_ptr< ::grpc::ServerBuilderPlugin> CreateLVProtoReflection() {
	LVProtoServerReflectionPlugin* reflectionPluginInstance = new LVProtoServerReflectionPlugin();
	reflectionPluginInstance->AddFileDescriptorProto(grpc_labview::ProtoDescriptorString::getInstance()->getDescriptor());
	return std::unique_ptr< ::grpc::ServerBuilderPlugin>(reflectionPluginInstance);
	}
	void InitLVProtoReflectionServerBuilderPlugin() {
	static struct Initialize {
	Initialize() {
	::grpc::ServerBuilder::InternalAddPluginFactory(&CreateLVProtoReflection);
	}
	} initializer;
	}

To avoid a messy transfer of the reflection information to the service (as the reflection strings should be stored separately for each individual server), I eliminated the plugin and instead instantiate a LVProtoServerReflectionService instance directly for each LabVIEWgRPCServer:

https://github.com/kt-jplotzke/grpc-labview/blob/d9d518d2374e762e4ee5a19cb1c0cc6658672a6a/src/grpc_server.cc#L25-L31

This service is now registered directly instead of using the ServerBuilder plugin architecture:

https://github.com/kt-jplotzke/grpc-labview/blob/d9d518d2374e762e4ee5a19cb1c0cc6658672a6a/src/grpc_server.cc#L208

Storing Descriptor Strings

I also incorporated the storing/registering of descriptor strings directly into the LVProtoServerReflectionService class. The AddFileDescriptorProtoString method takes in a serialized proto descriptor string and adds (not replaces) it to the pool of reflection data which is published.

This method is called directly from LabVIEWgRPCServer::RegisterReflectionProtoString

https://github.com/kt-jplotzke/grpc-labview/blob/d9d518d2374e762e4ee5a19cb1c0cc6658672a6a/src/grpc_server.cc#L107-L110

Exported Functions

Because the previous exported function to register reflection strings DeserializeReflectionInfo did not take a server id as an input, I created a new exported function to leave the signature of the previous function alone.

RegisterReflectionProtoString is now used to pass a serialized descriptor string to a specified server instance. As parsing of the serialized string is now done when this function is called, an error (-1004) is returned if the serialized string is malformed and/or not able to be parsed. Since grpc-labview controls both the serialization and de-serialization of these strings, I don't expect that this would ever be the case, but is good to catch.

https://github.com/kt-jplotzke/grpc-labview/blob/d9d518d2374e762e4ee5a19cb1c0cc6658672a6a/src/grpc_interop.cc#L225-L240

Backwards Compatibilty

The former DeserializeReflectionInfo exported function is left for backwards-compatibility because all servers generated with grpc-labview v1.5.1.1 and before used a DLL call directly in the server template to DeserializeReflectionInfo. As a result, removing or modifying this function would break all previously generated servers. (This was updated in #451 so that all DLL calls are now part of the gRPC library functions and not directly in the generated code)

As a "hack"/"workaround" to not passing in a server ID, I added tracking of server instances inside the DLL:
https://github.com/kt-jplotzke/grpc-labview/blob/d9d518d2374e762e4ee5a19cb1c0cc6658672a6a/src/grpc_interop.cc#L130-L134

When DeserializeReflectionInfo is called, this function now finds all server instances which have been created but not running and registers the reflection data to those server instances. It is anticipated that in the case of multiple server instances that each instance would be created and run sequentially. If the instances are created and run in parallel, the reflection data may not be correctly published for these servers, however previously we already had potentially incorrect data.

LV Updates

I updated Register Reflection Descriptors.vi to use the new RegisterReflectionProtoString function and pass in the gRPC id. I also added error checking after the DLL call.

Note that this would be a breaking change due to the connector pane changing (adding the gRPCid), but because this VI was just added with PR #451 and not released yet, this change would have no affect to existing code.

I subsequently updated Register Descriptors.vi in the server template to also pass in the gRPCid.

Automated Test Updates

I added a test case to the New_ATS architecture to test reflection. Specifically, a LV server is created using a given proto file and reflection information for specific services, methods, and messages are queried using reflection from a Python client. This new test case is added to the list of test cases run during New_ATS tests.

Note that we should test reflection for both multiple proto files being used to generate a single LV server and multiple server instances running in parallel (these tests would have failed previously). However, the New_ATS architecture does not allow for either of these cases. Therefore, I am only submitting the one test case for now, but will create an issue to expand in the future.

Documentation

I also added a page on gRPC reflection and tools that I use to test gRPC servers via reflection.

[jasonmreding]

@kt-jplotzke This week was crazy for me. I will try to find some time early next week to review this if nobody else beats me to it.

[kt-jplotzke]

@kt-jplotzke This week was crazy for me. I will try to find some time early next week to review this if nobody else beats me to it.

No worries! No rush - however, because of the one VI connector pane change, I'd recommend trying to merge this before a v1.6.0.1 release to avoid a breaking change.

[jasonmreding]

We need to add thread safety around this collection.

[jasonmreding]

nit: Make this an else if on a single line to avoid some nesting.

Also, the predominant styling used throughout the repo is for curly braces to be stacked on their own individual lines rather than staggering the opening curly brace at the end of the flow control statement and the closing curly brace on its own line. I know its not everybody's preferred style, but I would prefer to maintain consistency.

[jasonmreding]

I'm still trying to find time to finish looking at these changes. However, it occurred to me while reviewing this change that we always enable reflection (and I guess we always did)?

Enabling reflection is generally considered a security vulnerability so at a minimum it seems like it should be an opt in feature that is enabled by explicit calls by the user to an API (secure by default). I'm not sure the best place to expose that in the API. Maybe you can take a crack at what you think would be best. If you want to maintain compatibility with always enabling reflection for old servers, I can live with that. Since this is a potential security issue, I also think it is reasonable to break this default behavior when upgrading to newer versions of the library and requiring server authors to explicitly opt in.

[kt-jplotzke]

Curious, do you know if there was an original intent on why reflection was originally added into grpc-labview in the first place? Is there some other toolkit / NI tool that depends on reflection? (I'll admit that I was originally surprised when it was originally added in, which made me feel like there must have been a specific reason -- trying to figure out what existing code will break if we make reflection turned off by default)

My first thought is to add an input to Create Services.vi that could selectively enable reflection for the specific service(s).

Granted, this wouldn't turn off reflection completely (the server would still publish the reflection service itself). If we want to completely turn this off, I would also add a server-level Set Reflection Server Enabled.vi that would control if the reflection service is even turned on.

Any thoughts/preferences on either approach?

[jasonmreding]

Curious, do you know if there was an original intent on why reflection was originally added into grpc-labview in the first place? Is there some other toolkit / NI tool that depends on reflection?

I wasn't very involved when all of this happened in the past so I cannot say with certainty. I think there might have been some integration with MATLAB some customer/user was trying to achieve, and it would only work with grpc server reflection. I guess maybe it was these repos that motivated the change:

• https://github.com/ni/grpc-json-client-matlab
• https://github.com/ni/grpc-json-client

It is also useful as a general test/development feature. However, in most other languages I've seen, you would typically only enable reflection in a debug/test environment and not as part of a production environment unless you were comfortable exposing it from a security standpoint.

Granted, this wouldn't turn off reflection completely (the server would still publish the reflection service itself)...

Yeah, I'm not opposed to having a lever where each service can opt in/out of whether its service definition is published via reflection. However, I think there should still be a server wide setting so that the reflection service itself isn't always present and responding with no metadata from the server. If we can easily infer that from each services registration (or lack there of) from the API mocked up in your previous post, then I think that is fine.

Otherwise, my thought is maybe the Start Server.vi should have a recommended input added to it to enable reflection where the default value is false. That would also necessitate a corresponding change to LVStartServer and LabVIEWgRPCServer::Run. LVStartServer would need to maintain backward compatibility so would require a new LVStartServer2 entry point where LVStartServer would chain to LVStartServer2 with default reflection behavior up for discussion (either maintain prior for reflection or disable it out of an over abundance of caution). Either that or new fine grained entry point would need to be added and called prior to LVStartServer from the VI layer. I'm trying to decide if there is any clear upside/downside to bundling the setting as part of Start Server.vi vs. having a new public API independent of Start Server.vi that is only valid to call before Start Server.vi. I'm inclined to just bundle it with Start Server.vi, but I am open to counter arguments.

[jasonmreding]

Should this be continue instead?

[jasonmreding]

We probably should be providing thread safe access to this collection. If you are allowing registration for reflection on a per service basis, then it seems reasonable that could be done in parallel on the LV diagram.

[jasonmreding]

Same thing here in terms of thread safe access. I wasn't able to find clear documentation, but AI says:

The C++ google::protobuf::DescriptorPool is designed to be thread-safe for concurrent read operations. This means that multiple threads can safely call const methods on a DescriptorPool instance simultaneously, such as retrieving descriptors.

However, if a DescriptorPool instance is being modified (e.g., adding new descriptors), then non-const operations are not thread-safe and require external synchronization to ensure exclusive access. In essence, while concurrent reads are safe, concurrent writes or a mix of reads and writes without proper synchronization are not.

It didn't list any sources, but I'm inclined to believe it.

Also, the header file includes the following comments for BuildFile:

// Convert the FileDescriptorProto to real descriptors and place them in
// this DescriptorPool. All dependencies of the file must already be in
// the pool. Returns the resulting FileDescriptor, or nullptr if there were
// problems with the input (e.g. the message was invalid, or dependencies
// were missing). Details about the errors are written to ABSL_LOG(ERROR).

I've encountered this sort of thing in other languages like C# and it is a real pain to deal with. This makes me wonder if the current solution is sufficient to handle more complicated services where there are multiple proto files being imported in a dependency chain. Perhaps this was true of the prior solution as well? To work around, this usually requires collecting all of the file descriptor protos ahead of time and then doing your own dependency analysis on them to sort them according to their dependency order so you can successfully register them for reflection purposes. I'm definitely not on solid ground here without doing some experimentation myself so if your experience indicates something different, feel free to say so.

[jasonmreding]

@kt-jplotzke I think it is likely we will want to make a new release for the repo and publish a package to vipm in the not too distant future. Given that, perhaps it would be better to submit a separate PR that just includes the LV API breaking changes if you do not expect to make much progress on this in the short term.

[kt-jplotzke]

@kt-jplotzke I think it is likely we will want to make a new release for the repo and publish a package to vipm in the not too distant future. Given that, perhaps it would be better to submit a separate PR that just includes the LV API breaking changes if you do not expect to make much progress on this in the short term.

That's fair. You brought up some good points that I need to think through and admittedly won't have much time to get through this either this week or next. I'll make this a draft PR for now and submit a PR with passing the gRPCid into the LV functions but just don't do anything with that id so that the connector pane can be updated.