Cannot add multiple probe extensions dynamically via gRPC service.

[evanSpendlove]

TL;DR

Due to proto extensions being registered per message, multiple probe extensions cannot be added to Cloudprober using the gRPC service.

Issue Description

Currently, I cannot add more than one probe extension to Cloudprober using the gRPC service. I have a gRPC client that allows me to successfully send the AddProbe(), RemoveProbe() and ListProbe() requests to Cloudprober. However, when I try to add a second probe extension, I get an error as there is more than one extension of the ProbeDef proto. This error is thrown on line 92 of the probes/probes.go file.

Adding Multiple Probe Extensions

There are two parts to adding a probe extension in Cloudprober:

1. Register the probe extension in the internal extensions map using probes.RegisterProbeType()
2. Register and Set the proto extension of the ProbeDef proto.

The proto extension is registered globally for a given message, meaning it is not set on a per-probe-config basis, but rather on a global basis for the ProbeDef message.

When you try and add an extension probe to Cloudprober, it will eventually call the getExtensionProbe() function in probes/probes.go, line 89. In this function, the number of registered proto extensions is checked and an error is thrown if it is greater than 1.

As proto extensions are registered globally for a message (ProbeDef here), if you have more than one extension, this will always throw an error. Proto extensions are not consumed by adding a probe to Cloudprober, and there isn't a way that I can find of de-registering a proto extension. Therefore, one is limited to only one probe extension.

Extension Probes

I have been using the RedisProbe extension and a version of the FancyProbe from probes/testdata package. I changed the extension number of FancyProbe and included the necessary methods for a probe object.

Both of these work individually with the AddProbe() RPC, but these cannot be added together.

Possible Solution: Add an optional extension number field to AddProbeRequest

Adding the extension number to the fields of the AddProbeRequest message and using it in getExtensionProbe with FindExtensionByNumber() function of the protoregistry library is a possible solution. This could be an optional field to make it backwards compatible.

Unfortunately, I can't think of a way to resolve this without passing the extension number as part of the AddProbe() RPC. The protoregistry library, which handles proto extension registrations, does not provide a way to identify the extension of a particular message instance (rather than the message as a global type). I have also looked into the generated go methods of the ProbeDef message (in config.pb.go), but the extension number cannot be accessed here (only the extension range, which isn't useful).

[manugarg]

Thanks a lot for the extremely detailed bug report, @evanSpendlove.

I think there is a problem here:

cloudprober/probes/probes.go

Line 90 in 4bdceab

extensions := proto.RegisteredExtensions(p)

proto.RegisteredExtensions(p) gets extensions from the type of the proto message, instead of the actual proto message. We should do something else here, for example:

proto.ExtensionDescs(p)

I'll test that.

[manugarg]

proto.ExtensionDescs(pb) works. I'll submit a fix soon.

[evanSpendlove]

No problem! Thank you for responding so promptly!

I'm glad to see that you found a fix so quickly!

[manugarg]

#452 should fix this bug.

@evanSpendlove can you give it a try, or you'd like to wait for the next release (v0.11.0).

[evanSpendlove]

@manugarg I've tested adding multiple probe types dynamically using the gRPC service with this change and it works perfectly.

This is a really useful feature for the project that I'm working on. It will make our development process much easier and our code much cleaner.

Thank you so much for responding and fixing this so quickly!

[manugarg]

@evanSpendlove Sure. Good to know that it's working for you now. I'll close this issue now. I've opened another issue to track the next release: #453. Targeting Sep 15 for that.