opamp-spec icon indicating copy to clipboard operation
opamp-spec copied to clipboard

Add other_settings, TLS, and proxy settings to connection settings

Open michel-laterman opened this issue 1 year ago • 1 comments

Add other_settings, and a new TLSConnectionSettings and ProxyConnectionSettings across all connection settings a server can offer.

  • Closes #203

michel-laterman avatar Oct 15 '24 22:10 michel-laterman

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: michel-laterman / name: Michel Laterman (e8c6c769bec2549c3a7f006c4c5558ae7ad0b9a5, a174175346e52130b2b0a89849d5189be4c70ce7, 3946b01af98d2bfccfca7da7f9bc31b33e5847b2, 7849fe0b20b9354f80ac0a926d990c0ae0aa3d6c, 2b15b106e82987ecd6a6ec2f9030bea83e889aa2, 41ad8fb6738c901819da9a63fd2f8a1ebeab5d09, 1af8ba587cdbc2b4f5fd8ba741ed39e775d5aefc, 15fdbc082d2c419b0fa33259a52985c1257a960f, bc3845c508eca9dbd3ebfbef3c043c1c65bba19c, 05f2846e96133247da5293940b2993a0e954ff54, 3500ba391f56cb456626a15667765885edd86bac, 0f7ceb900e9187d76025ee2a717ed32d3b9668dc, a2918c7fc6479af4623ca0cb37f43ae3ce4d8256, 349fa55325f6da6e13d438a22360e68c6ca0b054, 4d860172acef8df90f518aa25452f6e13fe25d9c, ab67942f47c39e3623a84f16744b22c44a425ce5, 40329c170b7e699ee6aa18458b0048aa63712311, 09377dddbcbb8dd4a693263e92e57ba3c3809be7)
  • :white_check_mark: login: BinaryFissionGames / name: Brandon Johnson (df8b65c247f012284f419864a72fa25fa1687f06)
  • :white_check_mark: login: tigrannajaryan / name: Tigran Najaryan (1fd13b4f22d61f6ef750dd1d9ba00b2118f42a33, bc5eb2d328690a34f9831dc73f378fdd3bbd22f7)

I'll recreate the ProxySettings in another pr

michel-laterman avatar Oct 22 '24 23:10 michel-laterman

@michel-laterman we had a discussion with other OpAMP approvers/maintainers and decided to follow the Otel spec's requirements for making new proposals, namely to ask for prototypes that demonstrate the new capabilities. We will be formalizing the requirements in this PR: https://github.com/open-telemetry/opamp-spec/pull/207

I think it is important for this particular proposal to show how the TLS settings will be used. It is not a trivial change, so a working code would help understand it better.

tigrannajaryan avatar Nov 27 '24 17:11 tigrannajaryan

@tigrannajaryan, I have a WIP demo for offering the TLS settings: https://github.com/open-telemetry/opamp-go/pull/338

I have yet to add a demo for Agent-initiated CA trust Flow.

michel-laterman avatar Jan 09 '25 02:01 michel-laterman

To give a rough recap on my question about initial CA distribution during today's opamp sig meeting.

The current spec as written uses an implicit workflow where the client will connect with InsecureSkipVerify: true and the server will send offer connection settings to a new agent in response to the initial connection. My demo implementation: https://github.com/open-telemetry/opamp-go/pull/338 does this if both the client and server are started with special flags (-await-ca/-send-ca)

The idea I have for changing this is to make it explicit behaviour. We add a new attribute to OpAMPConnectionSettingsRequest (called something like ConnectionSettingsRequest) that a client can use to demand ConnectionSettingsOffers from the server. With this approach, in order to trigger the setting being sent, the client would be started with a new flag (i.e., -request-connection-settings), and would connect to the opamp server with InsecureSkipVerify: true initially, but include OpAMPConnectionSettingsRequest.ConnectionSettingsRequest in the first message sent to the server

michel-laterman avatar Mar 19 '25 19:03 michel-laterman

I went ahead and made the intial CA distribution require a explicit signal from a client. I have already added to my demo: https://github.com/open-telemetry/opamp-go/pull/338 I'm going to mark this as ready for review

michel-laterman avatar Apr 02 '25 21:04 michel-laterman

Can you describe the use of the hash? It isn't used in the example implementation. When would the client request settings again? Also is the hash stored somewhere on the client?

The example implementation in opamp-go uses a sync.Once called requestConnectionSettingsOnce to ensure that there is only one request for the connection settings. If we only expect this to happen once then it should probably be stated in the spec. Regarding that implementation, I think there could be a case where the connection is dropped or the server fails to respond (could send 503 Service Unavailable) and the client will not attempt to request the settings again because the Once will have completed. I think the spec should be more clear about the circumstances under which the client will/should send a SettingsRequest.

andykellr avatar Apr 29 '25 15:04 andykellr

Can you describe the use of the hash? It isn't used in the example implementation. When would the client request settings again? Also is the hash stored somewhere on the client?

@andykellr hash for this is meant to match ConnectionSettingsOffers.hash there's references to it in opamp-go, but I don't think it's used at the moment.

The example implementation in opamp-go uses a sync.Once called requestConnectionSettingsOnce to ensure that there is only one request for the connection settings. If we only expect this to happen once then it should probably be stated in the spec. Regarding that implementation, I think there could be a case where the connection is dropped or the server fails to respond (could send 503 Service Unavailable) and the client will not attempt to request the settings again because the Once will have completed. I think the spec should be more clear about the circumstances under which the client will/should send a SettingsRequest.

Good point, I'll remove the use of Once and leave it purely up to the client

michel-laterman avatar Apr 30 '25 14:04 michel-laterman

Notes from the SIG meeting, we're splitting this PR into:

  • add TLSSettings to offered connections (also add a description on what the settings are intended for)
  • add a report offered config message that mirrors remoteconfigsettings; the status enum used by remoteconfig status will be made more generic and reused between the both
  • add the CA initial trust workflow

michel-laterman avatar Apr 30 '25 19:04 michel-laterman

Thanks for you patience @michel-laterman

tigrannajaryan avatar May 06 '25 19:05 tigrannajaryan