Expose the function to generate tls.Config

[gouthamve]

We're looking to reuse this in Cortex to client side encrypt our
connections and not having this exposed is making us copy this entire
function.

Signed-off-by: Goutham Veeramachaneni gouthamve@gmail.com

[brian-brazil]

Use of this code is blocked by the security review, and we also said we'd want it released in the node exporter for a while before using it elsewhere.

[SuperQ]

The original idea was to move this to prometheus/common once we had it figured out.

There's nothing in our governance that says we need to block this on security review.

[SuperQ]

LGTM

[gouthamve]

Yes, but we would like to have TLS in Cortex sooner rather than later. From what I can see, this code has gone through robust code-review by prometheus-team already and I've reviewed the code myself and am fairly confident about it.

While I understand that this has not gone through a security-review, the alternate is implementing it ourselves (with likely no security reviews in the future). I see reusing this in Cortex as major positive, but also understand that this doesn't have any stability guarantees and am completely okay with it :) We'd be okay with changing things and even throwing away this implementation if it doesn't hold up to a security scrutiny.

We understand that we're reusing this at our own risk.

[brian-brazil]

Waiting on security review, releasing, and then moving is what was agreed at the last dev summit, and is the current standing consensus. If you think we should change that decision a new consensus must be arrived at - rather than ignoring the spirit of that concensus.

[gouthamve]

@brian-brazil From my understanding, that consensus was about releasing it in the official exporters and publicizing it for wider use.

And we'd like to see this tested too, and blocking opportunities for this being tested by advanced users is also against the spirit of the consensus imo.

[brian-brazil]

The consensus was also that we'd get a security review before releasing it, and that review blocks all other parts of the consensus.

[discordianfish]

Agree with @gouthamve and @SuperQ. This isn't "releasing" it as would be including in prometheus/common and encouraging it's use, so LGTM.

[brian-brazil]

That's a very odd definition of releasing, I'd consider any appearance in a binary or making it a public API as releasing independent of where the code happens to live. My making it public you're encouraging the use of code that we've previously said must get a security review before a release.

It's also not kosher to merge a change when there's active disagreement about it.

[discordianfish]

This was first of all a change to the node-exporter. Nothing enforces anyone to use the exposed struct. There is nothing that would encourage, let alone force other Prometheus components to use this. So far this functionality simply a experiment within the node-exporter, so I don't see how this change is any different from any other change to the node-exporter.

Unless I'm mistaken, there is nothing like a veto where any prometheus org member could block a change that both maintainers agree on.

[brian-brazil]

A change being in one repo doesn't mean it isn't relevant to the project as a whole, and this particular feature is a project-level one where the first step happens to be in the node exporter repo. To take a simpler example, switching to a completely different logging library would be project level even if it the PR itself only touched one repo.

By a strict reading of the governance everything works on consensus by default, with maintainers additionally having a presumption of lazy consensus for things that only impact on their repo. I would hope that we'd try to work out our differences without having to get all rules lawyery though.

[discordianfish]

But this doesn't replace a common used component like a logging library but is about exposing an experimental feature so an interested 3rd party can evaluate it as well.

Regarding "our differences", I've tried and I gave up. If you are going all lawyery by trying to veto further PRs where I an @SuperQ are in agreement, I'll step down from maintaining this project.

[juliusv]

• Regarding the consensus to explicitly block the release of TLS features on a review: neither @SuperQ nor @RichiH remember it that way, nor do the dev summit docs show that as a consensus (won't repeat links here, but chat convos and follow-ups were had about that).

• Regarding the meaning of "releasing": I think to most on the team this would mean releasing a Node Exporter or other official Prometheus component version with these features. I don't think exposing a Go-level function (without signalling that it's meant to be used as a library by the general public) is quite at that level.

• Regarding this being a project-wide decision: somewhat agreed, especially if it was about releasing a binary containing these features, less so on the Go-level change.

• Regarding conflict resolution & merging: https://prometheus.io/governance/#technical-decisions isn't 100% clear to me on repo-level things, but I also think that if someone on the overall team voices strong objections that one should normally try to find a resolution, or in the worst case, call for a vote. However, @brian-brazil, the reality is that people have simply grown too tired of endless lawyerly discussions to try and find a resolution every time (which often wouldn't happen anyway), nor is it feasible to call a team-wide vote on every such disagreement. So it drives people into a spot where they either give up on their point, don't do things quite by the book, or quit project engagement completely, because they just see things being blocked all the time. The fact is that our current governance makes it really easy to block things, but requires a lot of energy to unblock a single person's veto.

Anyway, I think it's fine to expose the Go-level function for now, but not give any guarantees around it or even advertise it.