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

Deprecate tls::*_file in favor of tls::*_pem #7691

Closed
mx-psi opened this issue May 17, 2023 · 15 comments
Closed

Deprecate tls::*_file in favor of tls::*_pem #7691

mx-psi opened this issue May 17, 2023 · 15 comments

Comments

@mx-psi
Copy link
Member

mx-psi commented May 17, 2023

After #7676 is merged, we will have duplicate functionality on the TLSConfig struct, where you can pass certificate and keys by providing either a file path (e.g. tls::ca_file) or in-memory (e.g. tls::ca_pem). Because of our support for ${file:/path/to/file} URIs, the former is redundant; one can put tls::ca_pem: ${file:/path/to/ca} to get the same functionality.

Once #7676 is released, we should deprecate the tls::*_file options in favor of tls::*_pem options.

@bogdandrutu
Copy link
Member

@erikbaranowski do you want to fix this?

@mx-psi mx-psi removed the on hold label Jun 9, 2023
@mx-psi
Copy link
Member Author

mx-psi commented Jun 9, 2023

No longer on hold

@mx-psi
Copy link
Member Author

mx-psi commented Jan 24, 2024

We spoke about this offline, we generally agree that we can keep both options for 1.0 and potentially deprecate (either now or in the future) the *_file options (depending on how many components suffer from this issue).

There are some things we want to check before we go ahead and close this though:

  • Is the behavior the same with *_file and *_pem?
    • Do relative paths work in the same way? (e.g. is the relativeness with respect to where the binary is or is it with respect to where the configuration file is)
    • Do we do any validation on file permissions?
  • Do we have this happen in other places (@jpkrohling mentioned some authentication extension)

@jpkrohling
Copy link
Member

Jaeger Remote Sampling as well

@mx-psi
Copy link
Member Author

mx-psi commented Jan 24, 2024

Thinking about this more calmly, I think we should remove the *_file options. We are already going to break most users with the change related to #8510, so we may as well do this and we can spend more time on helping people migrate (like with #7631)

@atoulme
Copy link
Contributor

atoulme commented Jan 24, 2024

I see that reload is implemented for some of the configtls settings but only for the client CA file under the TLS server settings. Is there a use case for live reload of certs and pem files that must be provided by the package, or is the file provider able to provide this somehow?

@jpkrohling
Copy link
Member

Reloading certs is a VERY common use case. If the file provider can't provide this, we shouldn't remove the _file options.

@evan-bradley
Copy link
Contributor

The file provider could be made to reload the certs, but the semantics would be very different from how configtls does this today with the *_file options. The *_file options allow a running server to reload a certificate by taking advantage of the GetCertificate field in tls.Config to re-read the file from disk. If the file provider reloaded the certificate, it would force a reload of the whole Collector config and trigger the Collector to restart the service.

I think we will need to keep the *_file options at least for the foreseeable future, if not indefinitely.

@mx-psi
Copy link
Member Author

mx-psi commented Jan 25, 2024

Reloading of a single field in the configuration definitely seems difficult to achieve without a significant refactor, I think we can close this as not planned (though I would be interested in hearing @bogdandrutu's thoughts on this).

We can file a separate issue for exploring supporting this reloading (out of scope for 1.0).

I found a few other components that ask for a filepath in the configuration:

I can file another issue on contrib to see if we should remove/replace the option on these. Does this make sense?

@yurishkuro
Copy link
Member

I am surprised the discussion is going in this direction. What percentage of users do we expect to be able to provide data via files vs. in-memory mechanism? By my scientific napkin math it's 1000:1, with 95% confidence.

@mx-psi
Copy link
Member Author

mx-psi commented Jan 25, 2024

I am surprised the discussion is going in this direction. What percentage of users do we expect to be able to provide data via files vs. in-memory mechanism? By my scientific napkin math it's 1000:1, with 95% confidence.

I don't understand your argument, why should this ratio make us not close this issue?

@evan-bradley
Copy link
Contributor

I can file another issue on contrib to see if we should remove/replace the option on these. Does this make sense?

This sounds good to me. I think as a rule of thumb we can probably replace filepaths in the config with recommendations to use the file provider if the component does not re-read the file at runtime.

@mx-psi
Copy link
Member Author

mx-psi commented Feb 1, 2024

@yurishkuro I am waiting on your reply to #7691 (comment) to close this issue. Could you have a look?

@yurishkuro
Copy link
Member

why should this ratio make us not close this issue?

Because, if you don't dispute the "math", it will break 1000 users per 1 who needs the new syntax?

@mx-psi
Copy link
Member Author

mx-psi commented Feb 2, 2024

Because, if you don't dispute the "math", it will break 1000 users per 1 who needs the new syntax?

To be clear, we are not going to do any changes on configtls, since I believe your comment is about the other components, I am going to close this one as wontfix.

We may change things in other components, following the rule that Evan proposed on #7691 (comment). This is the kind of change for which we can provide automated migrations on the Operator and using a tool like #7631 if needed, so I think the breakage would be minor. In any case, we can discuss this in the specific issues for the components

@mx-psi mx-psi closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants