server: Removing advanced TLS config parameters - BREAKING CHANGE

[aknuds1]

I would like to remove advanced TLS config parameters stemming from github.com/prometheus/exporter-toolkit/web, that were introduced in commit 953ac9fb41437fee0bffcb364333ba624a35e043:

• cipher_suites
• curve_preferences
• min_version
• max_version
• prefer_server_cipher_suites

The motivation for removing these advanced parameters is that users would most likely not want to change them, and they add corresponding configuration parameters to the Grafana Mimir project, that we don't want. We also think they're not interesting to the Grafana Tempo and Loki projects.

[pracucci]

Makes sense to me!

[bboreham]

This is a breaking change for someone who used this library between merge of #230 and now?

[bboreham]

Can you add something to the description what is meant by "hide"?

Here's what I think:
Prior to #230 the TLSStruct had just the 4 string settings added by this PR.
#230 changed to a TLSStruct with 9 members.
These members are visible if you dump out the YAML for the object, or walk the struct using reflection.
So this PR is to hide them from that.

[aknuds1]

This is a breaking change for someone who used this library between merge of #230 and now?

@bboreham afraid so, in case anyone is using those parameters. Does that imply I should start a changelog, documenting the breaking change? Or do you think this breaking change is simply a bad idea?

[aknuds1]

Can you add something to the description what is meant by "hide"?

Here's what I think: Prior to #230 the TLSStruct had just the 4 string settings added by this PR. #230 changed to a TLSStruct with 9 members. These members are visible if you dump out the YAML for the object, or walk the struct using reflection. So this PR is to hide them from that.

@bboreham certainly, I can improve the description of the PR.

[bboreham]

Is it possible to set those parameters at all after this change?

If not, I don't think that would be "hide", it would be "remove".
You could hide them from yaml by putting "-" as the name; that seems more generous to users.

[pracucci]

You could hide them from yaml by putting "-" as the name; that seems more generous to users.

How can you set these config options using YAML if you use yaml:"-"? Also, another discrepancy is that there were no CLI flags registered for the new TLS options, so they were only configurable via YAML. Even so, have you looked at the actual accepted values? There are "list of integers" which I think don't offer a very good UX: I doubt anyone ever used them.

[bboreham]

How can you set these config options using YAML if you use yaml:"-"

I meant someone using this library as code could then make their own arrangements to set the parameters.

[aknuds1]

@bboreham

Is it possible to set those parameters at all after this change?

No.

If not, I don't think that would be "hide", it would be "remove". You could hide them from yaml by putting "-" as the name; that seems more generous to users.

That's a good point, didn't think of it.

[aknuds1]

How can you set these config options using YAML if you use yaml:"-"

I meant someone using this library as code could then make their own arrangements to set the parameters.

I see @bboreham's point, I think it makes sense. I can make this change.

[bboreham]

Looking at the specifics again, maybe it is ok to remove them completely.
I think the list being removed is:

	CipherSuites             []cipher   `yaml:"cipher_suites"`
	CurvePreferences         []curve    `yaml:"curve_preferences"`
	MinVersion               tlsVersion `yaml:"min_version"`
	MaxVersion               tlsVersion `yaml:"max_version"`
	PreferServerCipherSuites bool       `yaml:"prefer_server_cipher_suites"`

If you update the description, title and git commit message to state what you want to do, and include your justification, I'll come back and look at the PR again.

[aknuds1]

@bboreham ok, I can do that.

[aknuds1]

Updated PR title and description, plus Git commit message, to clarify the intention and justification.

[bboreham]

the Grafana Mimir project, that we don't want. We also think they're not interesting to the Grafana Tempo and Loki projects.

I must comment that this project does not exist for the sole benefit of Grafana Labs, but we can take that as some input.
If future users do want the parameters we can discuss that, either expose them or let those users fork the library.

[bboreham]

LGTM; thanks for updating.

[bboreham]

There are "list of integers" which I think don't offer a very good UX

Turns out prometheus/exporter-toolkit implements UnmarshalYAML so you could say something like:

cipher_suites:
- TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
- TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256

[bboreham]

PR to re-introduce some TLS config: #256