Add helper method to config.Secret to load a file

[mem]

LoadFromFile is a very small helper to avoid repeating this code over and over again.

This is related to prometheus/prometheus#8551

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

[roidelapluie]

Where would this be used? When we load from file, we should get a String, not a Secret. The code path should be different: if a Secret is used directly, we should not have extra roundtrippers asking secrets every time.

This helper would: generate conversion from/to Secret on every request and request that everywhere we use a secret from file, we use a getter roundtripper.

[mem]

Oops, sorry, I missed this reply among all the notifications, @roidelapluie

Where would this be used? When we load from file, we should get a String, not a Secret. The code path should be different: if a Secret is used directly, we should not have extra roundtrippers asking secrets every time.

This helper would: generate conversion from/to Secret on every request and request that everywhere we use a secret from file, we use a getter roundtripper.

I think I see what you mean...

In Prometheus there are two roundtrippers: one stores a static secret and the other keeps a filename and reloads the secret on every request (if the secret changes, a new roundtripper is created).

If I look at discovery/azure/azure.go I see that it has a ClientSecret field in its SDConfig struct, and there's no ClientSecretFile (which is the thing that 8551 is asking for). So in order to add the later, it should be consistent and implement the reloading mechanism is whatever way makes sense. In that particular case, there's an "Authorizer" that's part of the API which looks like could be wrapped in order to provide this functionality.

So in other words, it's not enough to load the secret when marshaling YAML, something has to be added so that the knowledge of the existence of the file is preserved.

But since a config.Secret has no interface (it's casted into a string everywhere where it's used) we cannot encapsulate this behavior inside it.

The thing I was trying to avoid was having code everywhere asking "does this come from a file? yes, do this; no, do this other thing".

I can imagine adding a prefix to the string to signal that it's a file, not a plain string. And then add a method to return the secret (either the string or the contents of the file) as well as an indication as to whether the secret changed or not, but I also imagine that's prone to problems.

What about adding an interface? SecretLoader? For static secrets it simply returns the stored secret and it never changes. For dynamic secrets, it load the secret from wherever and returns whether it changed or not.

The question obviously is how to migrate smoothly from the Secret fields to the SecretLoader fields...

[mem]

What about adding an interface? SecretLoader? For static secrets it simply returns the stored secret and it never changes. For dynamic secrets, it load the secret from wherever and returns whether it changed or not.

The question obviously is how to migrate smoothly from the Secret fields to the SecretLoader fields...

In order to have something concrete to talk about, I uploaded a change along those lines here: https://github.com/prometheus/common/tree/mem/secret_loader

It's not even passing all unit tests, but most. It's trying to preserve the current behavior, but it will break with existing programs because the type is no longer a string but a struct.

Please bear I'm mind that I have looked at this too much, so I might be missing obvious things...