feat(secrets): Add new secrets management package #797
Conversation
hsmatulis
force-pushed
the
main
branch
3 times, most recently
from
cf118e6 to
24ec0f7
Compare
hsmatulis
force-pushed
the
main
branch
18 times, most recently
from
1882b4c to
694bf26
Compare
Signed-off-by: Henrique Spanoudis Matulis <[email protected]>
hsmatulis
changed the title
|
@pintohutch @bernot-dev @bwplotka PTAL, should be ready! |
There was a problem hiding this comment.
Thanks, amazing work!
Generally, it's great - maybe first big question is if we want SecretField to store so much state or do we want to move some of this state to manager (or and providers). This might be more composable and easier to reason about. Prometheus discovery is doing some of this. I mentioned one idea (A) 1 and 2 in comments.
However, it's not a blocker, even in the current state, I would say we could try this out on Prometheus (and someone might try on AM side!). What matters is that I see a clean YAML surface format, clean code, idiomatic struct reflection to find fields and healthy amount of test, so amazing! With this we can iterate.
No matter if you want to try (A) or skip it for now, I think I would try to check before merging:
- Limit the global variable spread (https://github.com/prometheus/common/pull/797/files#r2414525826)
- If we can skip validator and timeout based validator state per field, if it's YAGNI (see https://github.com/prometheus/common/pull/797/files#r2414536374)
- Use testable examples: https://github.com/prometheus/common/pull/797/files#r2414499436
Then I would like to review in more depth the manager code, but generally... we could start with this! 🎉 Thanks!
| ) | ||
|
|
||
| // SecretField is a field containing a secret. | ||
| type SecretField struct { |
There was a problem hiding this comment.
readability nit: We have opportunity to "eat" secret prefix, since it's a secrets package, so everyone will use it generally as secrets.SecretField. So we could do secrets.Field if we want. To consider, not a blocker.
|
|
||
| type SecretFieldSettings struct { | ||
| RefreshInterval time.Duration `yaml:"refreshInterval,omitempty"` | ||
| Default string `yaml:"default,omitempty"` |
There was a problem hiding this comment.
Is this used? I don't see any tests for it. What this default use case should look like?
| } | ||
|
|
||
| type SecretFieldSettings struct { | ||
| RefreshInterval time.Duration `yaml:"refreshInterval,omitempty"` |
There was a problem hiding this comment.
question: Hm, good question if refresh is really relevant to all providers... E.g it's not for inline and.. also not for file based? Refreshing in general is a key concept here so we at least needs a good commentary in the code for this field on how it suppose to be implemented and consumed?
| t.Errorf("Expected error containing '%s', but got: %v", tc.errContains, gotErr) | ||
| } | ||
| return | ||
| } else if gotErr != nil { |
There was a problem hiding this comment.
Generally no need for "else" if you return above.
|
|
||
| In your configuration struct, use the `secrets.SecretField` type for any fields that should contain secrets. | ||
|
|
||
| ```go |
There was a problem hiding this comment.
Sustainability suggestion (non-blocking):
This is well documented, thanks!
However, it might be even better if we would replace the "How to use" section with a single or set of buildable examples as per https://go.dev/blog/examples .. and commentary in the key public code structures.
| } | ||
| } | ||
|
|
||
| func (s *SecretField) Get() string { |
There was a problem hiding this comment.
Nice! This make caller unaware of errors which can be both great and bad, depending what we do. I think it would be great to test out in this state 👍🏽
One alternative to make things leaner on SecretField (decompose responsibilities and state), would be if SecretField would allow storing (atomically) and getting secret value string. This way we could have common package global free and SecretField manager free (manager could be responsible to set correct things in config).
It could be even provider agnostic/free combined with https://github.com/prometheus/common/pull/797/files#r2414589581 idea (and avoid globals in this package)
| val reflect.Value | ||
| } | ||
|
|
||
| func getSecretFields(v interface{}) (secretPaths, error) { |
There was a problem hiding this comment.
Love it, epic! A go idiomatic way to find secret fields automatically and do something about it 👍🏽
| return manager, nil | ||
| } | ||
|
|
||
| func (m *Manager) registerMetrics(r prometheus.Registerer) { |
There was a problem hiding this comment.
Thanks for metrics, super important!
| return secret.secret | ||
| } | ||
|
|
||
| func (m *Manager) triggerRefresh(s *SecretField) { |
There was a problem hiding this comment.
note to myself: Manager code needs deeper review, quite many locks 🙈 prone to errors, but perhaps needed.
| return nil | ||
| } | ||
|
|
||
| func (s *SecretField) UnmarshalYAML(unmarshal func(interface{}) error) error { |
There was a problem hiding this comment.
Have we considered doing plainSecret thingy, but then unmarshal to any or map[string]any and let manager.ApplyConfig do the final parsing? This would make SecretField leaner and manager/provider agnostic (together with setter/getter for secret values https://github.com/prometheus/common/pull/797/files#r2414554264)
This PR introduces a new package, secrets, to prometheus/common. This package provides a unified way to handle secrets within configuration files for Prometheus and its ecosystem components. It is designed to be extensible and observable. See the proposal here