Add option to have certificates specified inline

[mem]

Add fields to the TLSConfig struct so that it's possible to specify
either a path to the certificates (Certificate Authority, client cert,
client key) or specify them inline.

This is related to prometheus/prometheus#8551

Signed-off-by: Marcelo E. Magallon [email protected]

[mem]

All the changes like this are meant to reduce the noise around having to check whether the filename is set or the inline version is set.

[mem]

This is the change that I feel the most uneasy about because the existing mutex is protecting multiple things: access to the hash, the round tripper and the tlsConfig.

[mem]

I might be missing something obvious here: the hash is kept so that changes in the certificate can be detected, but in order to compute the hash, the file has to be loaded each time, so the potential savings in using a hash to avoid the longer byte-for-byte comparison are lost (?) by having to read the entire certificate from disk and then compute the hash. Is it that some changes in the file won't cause the hash to change (in other words, only a part of the file content's is used to compute the hash)?

[mem]

not sure where or if to document this bit: the !!binary tag causes the YAML library to treat the contents of the field as base64-encoded. The code itself will never see the base64-encoded version, only the decoded payload.

[roidelapluie]

+629 −97

To be honest, this feature simply does not seem to be worth it if this is the complexity we have to add in the code.

[mem]

+629 −97

To be honest, this feature simply does not seem to be worth it if this is the complexity we have to add in the code.

Just to be sure, are you talking about this bit here?

// If a CA cert is provided then let's read it in so we can validate the
// scrape target's certificate properly.
if ca, err := cfg.getCA(); err != nil {
	return nil, fmt.Errorf("unable to get specified CA %s: %w", cfg.getCAName(), err)
} else if ca != nil {
	if !updateRootCA(tlsConfig, ca) {
		return nil, fmt.Errorf("unable to use specified CA %s", cfg.getCAName())

Or is the comment about the CertGetter interface?

[roidelapluie]

Just the size of the diff and the time it will take to review seems huge. Additionally, this feature is very uncommon in software in general, and is not requested by users.

[mem]

Just the size of the diff and the time it will take to review seems huge. Additionally, this feature is very uncommon in software in general, and is not requested by users.

Oh, the size of the diff. I tried to understand the comment as line numbers... :-\

161     46      config/http_config.go
174     51      config/http_config_test.go
294     0       config/testdata/http.conf.tlsconfig.good.yml

the bulk of the diff is actually the test file. The second largest change is the test, which, yes, needs to be reviewed, but a good chunk of it is whitespace noise. There's only one completely new test, most of the other test changes are adding test cases to the already existing tests.

Excluding data and tests, the code diff is +161 -46, and most of that is about adding a few functions to keep things sane.

While I understand the comment about this not being requested, the original issue referenced in the commit's description is about making this consistent: some places require a file, some places require an inline secret. In my mind we cannot make things consistent unless we make them consistent in both directions: add the inline option where there's only a file, and add the file option where it's only inline. There are already several places where both options are offered.

[SuperQ]

I think this would be nice to have. The actual code change seems perfectly reasonable in size.

@mem can you rebase this to fix the conflicts?