Add support for secret refs via a secret manager #572
Conversation
TheSpiritXIII
changed the title
TheSpiritXIII
force-pushed
the
http-secret-ref
branch
from
106011c
to
88d7263
Compare
TheSpiritXIII
force-pushed
the
http-secret-ref
branch
from
88d7263
to
7fd0a65
Compare
TheSpiritXIII
force-pushed
the
http-secret-ref
branch
from
7fd0a65
to
f3006cd
Compare
Signed-off-by: Daniel Hrabovcak <thespiritxiii@gmail.com>
TheSpiritXIII
force-pushed
the
http-secret-ref
branch
from
f3006cd
to
5033109
Compare
There was a problem hiding this comment.
Amazing! Generally LGTM, but lots of minor suggestions.
Disclaimer: @TheSpiritXIII is in my Google team. I tried to get more hands on this review for weeks, but no response. The secret provider feature was generally accepted 1, 2, so allowing myself to move forward after the careful review
| } | ||
|
|
||
| // NewRoundTripperFromConfig returns a new HTTP RoundTripper configured for the | ||
| // given config.HTTPClientConfig and config.HTTPClientOption. |
There was a problem hiding this comment.
lmao, what happened that we have both *Config and *Option 🙈
| @@ -148,6 +152,7 @@ type Authorization struct { | |||
| Type string `yaml:"type,omitempty" json:"type,omitempty"` | |||
| Credentials Secret `yaml:"credentials,omitempty" json:"credentials,omitempty"` | |||
| CredentialsFile string `yaml:"credentials_file,omitempty" json:"credentials_file,omitempty"` | |||
| CredentialsRef string `yaml:"credentials_ref,omitempty" json:"credentials_ref,omitempty"` | |||
There was a problem hiding this comment.
Would it be useful to add quick commentary what the *Ref field allows (e.g generic secret provider)?
| @@ -148,6 +152,7 @@ type Authorization struct { | |||
| Type string `yaml:"type,omitempty" json:"type,omitempty"` | |||
| Credentials Secret `yaml:"credentials,omitempty" json:"credentials,omitempty"` | |||
There was a problem hiding this comment.
not(non blocking): Wild idea, but what stops us to use Credentials file as a ref. Or *File?
I guess potential confusion and lack of ability for users to use inline somewhere, file somewhere and X secret provider in another place on one deployment? 🤔
| // nonZeroCount returns the amount of values that are non-zero. | ||
| func nonZeroCount[T comparable](values ...T) int { | ||
| count := 0 | ||
| var zero T |
| return false | ||
| } | ||
|
|
||
| type refSecret struct { |
There was a problem hiding this comment.
Can we add some quick commentary - bit more complex than other two
| return false | ||
| } | ||
|
|
||
| func secretFrom(secretManager SecretManager, text Secret, file, ref string) (secret, error) { |
There was a problem hiding this comment.
Can we add commentary about some non trivial semantics e.g.
- what if both text and file are set, what's the behaviour? Does this assumes some pre-validation?
| return false | ||
| } | ||
|
|
||
| func secretFrom(secretManager SecretManager, text Secret, file, ref string) (secret, error) { |
There was a problem hiding this comment.
| func secretFrom(secretManager SecretManager, text Secret, file, ref string) (secret, error) { | |
| func toSecret(secretManager SecretManager, text Secret, file, ref string) (secret, error) { |
Perhaps clearer?
| authType string | ||
| authCredentialsFile string | ||
| rt http.RoundTripper | ||
| type authorizationCredentialsRoundTripper struct { |
config/http_config.go
Outdated
| } | ||
| rt.mtx.RLock() |
There was a problem hiding this comment.
should we leverage immutable method to avoid any lock, similar to prev part:
There was a problem hiding this comment.
Plus we can move it to if clientSecret != nil { body?
| var err error | ||
| secret, err = rt.clientSecret.fetch() | ||
| secret, err = clientSecret.fetch(req.Context()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to read oauth2 client secret: %w", err) | ||
| } |
There was a problem hiding this comment.
I guess I missed this in prev PR, but this comment is still relevant #572 (comment)
Part 2 for secret provider support within common. See: https://groups.google.com/g/prometheus-developers/c/WKOej_pnhXg
Part 1 is here: #538