ServerToAgentCommand.CommadType zero value is "restart"

[domodwyer]

Currently the CommandType enum has no "unspecified" catch all field - the "zero value" of the enum is Restart:

opamp-spec/proto/opamp/v1/opamp.proto

Lines 681 to 685 in 42f0187

	enum CommandType {
	// The Agent should restart. This request will be ignored if the Agent does not
	// support restart.
	CommandType_Restart = 0;
	}

Because the protobuf wire format is designed to be backwards compatible, it is generally considered good practice to map the zero value to some sentinel "unknown" / "unspecified" indicator as it prevents this scenario:

1. A client is running happily.
2. The protobuf spec is updated, adding a new command to the protocol: CommandType:Bananas
3. The OpAMP server is updated to support this new command.
4. For whatever reason, the server sends CommandType::Bananas to the client running an old version of the spec.
5. The client restarts because it doesn't recognise the Bananas command, and so maps it to the "zero value" which is CommandType::Restart.

Changing the zero value would be a breaking change, but defaulting the zero value to "restart" is a bit of a dangerous default, so might be worth considering a remap before taking this enum out of beta.

Most enums have the "unspecified" zero mapping already, but a couple are missing them - it might also be worth considering a protobuf linter to help catch this in the future - I'd be happy to submit a PR to add one.

[tigrannajaryan]

I agree, it was not a good idea.

I don't want to make a breaking change, but formally we can since this is still in "Beta".

@andykellr what do you think? How disruptive a change like this would be in practice if we change to CommandType_Restart = 1? It means existing clients cannot be restarted until they are upgraded to the new version.

[RassK]

A bit offtopic. Should client report the spec version in non-identifying attributes (or in a fixed field)? Should it be marked as a MUST? This could add a better overview how disruptive such change can be (the level is anyway individual).

I support the change but administrator needs to have visibility when to include such changes. It's very possible that different agents implement different spec versions.

[tigrannajaryan]

Should client report the spec version in non-identifying attributes (or in a fixed field)?

No. I am explicitly against version number based logic. If conditional operation is needed it should be based on capability flags, which is the mechanism that we use for signalling what the client and server support. Adding another way to signal capabilities is a slippery slope and can result in a complicated mess of "if" checks to figure out what to do.