Add other_settings, TLS, and proxy settings to connection settings #205
Conversation
There was a problem hiding this comment.
Thanks for the draft @michel-laterman
I left some comments, and I would like @andykellr to also review.
| @@ -277,6 +277,16 @@ message OpAMPConnectionSettings { | |||
| // If this field has no value or is set to 0, the Agent should not send any heartbeats. | |||
| // Status: [Development] | |||
| uint64 heartbeat_interval_seconds = 4; | |||
|
|
|||
| // Additional connection settings. These are Agent-specific and are up to the Agent | |||
There was a problem hiding this comment.
Mark all new fields and messages with Status: [Development]. New features start at this level.
There was a problem hiding this comment.
I posted earlier:
One other design consideration is to make sure OpAMPConnectionSettings, TelemetryConnectionSettings and OtherConnectionSettings are defined uniformly, with the exception of settings which are only applicable to a particular connection type (e.g. heartbeat_interval_seconds is unique to OpAMPConnectionSettings). A possible option would be to move all common settings to one proto message and include that from OpAMPConnectionSettings, TelemetryConnectionSettings and OtherConnectionSettings. That would be a breaking change, so we will need to discuss if it is worth it.
@andykellr what do you think?
There was a problem hiding this comment.
I would definitely like to avoid breaking changes at this point. I think the approach of having additional messages for TLS and Proxy settings creates enough uniformity between the connection settings messages.
I likely won't be able to review this PR until early next week, but I will take a look. At first glance it looks reasonable but you raise good questions.
There was a problem hiding this comment.
OK, let's avoid the breaking changes.
| map<string, string> other_settings = 5; | ||
|
|
||
| // Connection specific TLS settings. | ||
| TLSConnectionSettings tls = 6; |
There was a problem hiding this comment.
Why TLSConnectionSettings here? We already have one as a sibling of ProxyConnectionSettings.
| // set these in the request headers. | ||
| // For example: | ||
| // key="Authorization", Value="Basic YWxhZGRpbjpvcGVuc2VzYW1l". | ||
| Headers headers = 3; |
There was a problem hiding this comment.
Are these headers in addition to headers in OpAMPConnectionSettings? Do we need both?
There was a problem hiding this comment.
I think it would help make it clear for cases where a proxy requires these settings but the actual endpoint being reached does not
| // for this connection. | ||
| // This field is optional: if omitted the client SHOULD NOT use a client-side certificate. | ||
| // This field can be used to perform a client certificate revocation/rotation. | ||
| TLSCertificate certificate = 4; |
There was a problem hiding this comment.
Why TLSCertificate here? We already have one as a sibling of ProxyConnectionSettings.
michel-laterman
force-pushed
the
feat/tls-proxy-connection-settings
branch
from
e79ec75
to
b8770b8
Compare
michel-laterman
force-pushed
the
feat/tls-proxy-connection-settings
branch
from
b8770b8
to
7849fe0
Compare
Add
other_settings, and a newTLSConnectionSettingsandProxyConnectionSettingsacross all connection settings a server can offer.