Skip to content
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

Proxy & TLS settings in OpAMPConnectionSettings, TelemetryConnectionSettings, and OtherConnectionSettings #203

Open
michel-laterman opened this issue Oct 1, 2024 · 1 comment

Comments

@michel-laterman
Copy link
Contributor

Connection settings that an OpAMP server offers should have some way to pass additional information that includes proxy settings and additional TLS settings.

TLS settings should TLS/mTLS settings defined in otel config tls.

Proxy settings may include:

  • proxy url
  • proxy headers
  • proxy client certificate
  • proxy TLS settings

Additionally I think that we should include other_settings in the definition of OpAMPConnectionSettings and TelemetryConnectionSettings to allow implementation to define their own additional settings.

@tigrannajaryan
Copy link
Member

One thing I am a bit worried about is the complexity this imposes on the agents. I think we need to keep this to a minimum and only include settings that are required for successful connection and avoid adding desirable but necessarily required settings. For example looking at otel config tls, I see things like write_buffer_size. I don't think we need to add settings like this.

With that in mind I would like to see a draft of what the absolutely minimum of required settings would look like in proto form.

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.

According to the rules we are allowed the breaking change since we are in Beta, but since we are so late in Beta stage, the bar is pretty high for breaking changes (cc @andykellr).

@michel-laterman if you can make a draft PR to show what the change would look like I think it would help the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants