I'd suggest the LogLevel enum be lifted from the Logger message in the types definition and reused here instead of duplicating this for each API. Maybe the LOG_LEVEL_UNSPECIFIED value could be interpreted as "set it to the default"?

The main problem with reusing the types.LOG_LEVEL it is that there's no good way (easy to understand, easy to implement, easy to maintain) to add the additional value to an existing enum. This is why I created a SetValue enum instead of reusing the LogLevel with an additional "reset to config value" field. (The combination of the two would then require use to ignore one or the other when they are both set).

I don't think we should use LOG_LEVEL_UNSPECIFIED. That's a default within logrus that carries the meaning of "the log level was not set yet". It is used to handle detection of un-configured loggers in scenarios where file handlers and other items might be configured prior to the logger having a configured log level. In addition, UNSPECIFIED has the effective value of INFO, which means we would have to be very careful not to forward it to logrus, or risk getting the wrong log level.

Also this is identical to the cli options, so it basically keeps the CLI small and accurate to the options, making the parity between the CLI and the API 100%. If we ever expand this to include REST calls, such parity will be appreciated in the need to keep the CLI and API documentation nearly identical.

IMO, the API definition is a high-level representation of a concept, in this case, facilities to observe and adjust the log level of SPIRE core. The API definition of a log level should represent the log levels we want to support at a high level, not necessarily the log levels supported by the underlying logger implementation. Trying to align the definitions of the two should be a non-goal. We currently use logrus but one day we may not (logrus is no longer maintained). I very much anticipate a function as part of the API implementation that converts between the API definition of a log level to that of the (current) logger implementation in SPIRE.

From that perspective, I wouldn't worry too much about having to combine two enums or try to add a value.

Using "LOG_LEVEL_UNSPECIFIED" on SetLogLevel as "go back to the default" has some elegance in my opinion, but there are other ways we can represent "go back to the default" that don't rely on duplicating the log levels for the sake of adding a "DEFAULT" value just for the sake of the SetLogLevel call.

For example, you could two fields on SetLogLevel:

LogLevel log_level = 1;
bool unset = 2;

If unset is true, log_level is ignored and the value is unset. You could fail the API call with InvalidArgument if log_level was specified and unset was true.

I'd like to follow (as much as it makes sense) a policy of "no disinformation" in the API.

• UNSPECIFIED means we don't detail what log level to set, and that's not what we are doing, we are detailing the log level to be set to the configured-at-launch level.

• UNSET means that we are clearing out the log level setting, and no logging system that I'm aware of permits a nil log level, again we are setting it to the configured-at-launch level setting.

• Changing the word DEFAULT to CONFIGURED_AT_LAUNCH, LAUNCH_VALUE, or creating a new GRPC message to handle the action of setting to the configured-at-launch value are all approaches that are far much more difficult to misinterpret, even if documentation isn't available.

Trying to align the definitions of the two should be a non-goal.

I fully agree, and it's a good justification to not use UNSPECIFIED. It's also a good justification to not use Logrus Log Levels, except that the overriding purpose of the API is to set Logrus Log Levels, so at least some parity between the two is likely to reduce confusion.

I could set two fields, but then everyone will have to maintain the documentation on how to interpret the two fields. There's basically three ways out of this:

1. Create two enums (one for setting, one for reply)
2. Create two messages (one for setting to a specified log level, one for setting to the configured-at-launch log level)
3. Create two fields in one shared message (documenting on how the two interact, programming the code to match the documentation, and creating load on the future clients to generate a message containing both settings with the matching logic)

As you can see, option 3 puts more long lasting load on the maintenance, because while it will be nigh impossible to mess up the logic for options 1 or 2, it will be easier to mess up the logic for option 3, and it requires matching logic in the cli and grpc service. In the event that we expose this API to JSON style REST requests, it will then also require the clients to have matching "set both according to these rules" logic.

And yes, I wrote the code using your approach first, and after a few days realized that it was more elegant (doing more with less) after I merged the two fields into one enum.

Please reconsider your focus on option 3. I would happily implement options 1 or 2 to address your concerns.

Per offline conversation with Andrew:

• Add an UNSPECIFIED enum value for the log level enum's zero index, so we can tell if the value wasn't set.

After some back-and-forth, we both decided than an additional ResetLogLevel message (for the server and the agent APIs) would be preferable to having a combination of fields in a single SetLogLevel message that then required additional logic / rules to resolve if the log level was both set to a value and reset simultaneously.

@azdagron I would like to close this conversation as resolved, unless there are action items within it you still need to see in the PR.

nit: this comment seems out of place

Changed to // Set Log Level Request message.

typo

Gotta love vim, changed to message

I'd rather keep it as "set_level" as "log_level" can confuse input / output orientation of the value.

If set_level seems wrong, anything indicating a request would be fine by me. I just don't want the stammering of saying it's a log level type and then having nothing else to say except it's a log level (which is captured by the type).

I'd rather keep it as "set_level" as "log_level" can confuse input / output orientation of the value.

If set_level seems wrong, anything indicating a request would be fine by me. I just don't want the stammering of saying it's a log level type and then having nothing else to say except it's a log level (which is captured by the type).

some options that are more expressive than log_level

• set_level
• new_level
• requested_level

I know "set" is often overused, but it might fit the bill here. Let me know what you think.

Resolved in an offline chat: new_level will the the name.

The agent doesn't have the same authorization mechanisms as the server. This API will only be available over UDS. Probably appropriate to just remove this sentence (for all 3 RPCs) since anybody with access to the private admin socket has permissions to execute any function.

I'll alter all of the agent commentary to say something about it being serviced on the admin socket.

typo

fixed. Thank you.