-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add other_settings, TLS, and proxy settings to connection settings #205
base: main
Are you sure you want to change the base?
Add other_settings, TLS, and proxy settings to connection settings #205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark all new fields and messages with Status: [Development]
. New features start at this level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these headers in addition to headers in OpAMPConnectionSettings? Do we need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why TLSCertificate here? We already have one as a sibling of ProxyConnectionSettings.
e79ec75
to
b8770b8
Compare
b8770b8
to
7849fe0
Compare
Add
other_settings
, and a newTLSConnectionSettings
andProxyConnectionSettings
across all connection settings a server can offer.