common

common copied to clipboard

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]

+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.

+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?

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.

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.

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?