[config/configtls] Enable goleak check

[crobert-1]

Description: <Describe what has changed.>

This change enables goleak to check the config/configtls package for memory and goroutine leaks. There was originally a goroutine leak which is fixed by calling the newly added method TLSServerSetting.Shutdown.

The major change of this PR is to make the *clientCAsFileReloader object a member of the TLSServerSetting struct, as well as adding a public Shutdown method to TLSServerSetting. This was the cleanest way I could add a way to shut down the reloader, otherwise it will be a leaked goroutine.

Also, I had to change LoadTLSConfig() to be a pointer type receiver, so that it could modify the given object by reference instead of only having the value copy of the TLSServerSetting object it was called on.

Link to tracking Issue: <Issue number if applicable> #9165

Testing: <Describe what testing was performed and which tests were added.> Goleak is passing successfully, as well as all existing tests.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.69%. Comparing base (93cbae4) to head (28e1284).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9220      +/-   ##
==========================================
- Coverage   91.71%   91.69%   -0.03%     
==========================================
  Files         406      406              
  Lines       18947    18958      +11     
==========================================
+ Hits        17378    17383       +5     
- Misses       1209     1214       +5     
- Partials      360      361       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jpkrohling]

Sorry about the go.mod conflict. Once that is fixed, this can be merged. Thank you!

[bogdandrutu]

@jpkrohling removed the ready-to-use since I believe this does not work for multiple calls to get settings, because of that I think is better to design a different API.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[crobert-1]

@jpkrohling and @bogdandrutu: I believe I've addressed the concerns mentioned, another look would be appreciated when you're able!

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[yurishkuro]

Q: why do we implement reloading of client CA file with a background goroutine instead of using the same approach as certReloader which does it on-demand when GetCertificate is called?

https://github.com/open-telemetry/opentelemetry-collector/blob/ede9e304314dddbdeb10b5ee06b1a49ab88e4f69/config/configtls/configtls.go#L140

This would avoid a goroutine leak and the unpleasant side-effects of having to call Shutdown on a "config" object.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[mx-psi]

@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog)

[crobert-1]

@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog)

Just waiting for reviews 👍

[mx-psi]

Can you take a look at Yuri's question above?

[mx-psi]

@crobert-1 Mind taking a look at https://github.com/open-telemetry/opentelemetry-collector/pull/9220#issuecomment-2119380494 ?

[crobert-1]

@crobert-1 Mind taking a look at #9220 (comment) ?

My apologies for the delay, thanks for the ping! I'm taking a look at this. There's no context from the original issue or implementing PR why this design was chosen. However, the issue says it should be done like the certReloader, which, as @yurishkuro pointed out, is done in a lazy manner on Get.

I'm working on implementing this to simply read the file on Get rather than the proactive watching approach that's currently implemented. If it works properly, I'll open a new PR instead of this approach.

Marking as draft until I get a chance to properly prove the alternative approach will work.