- Notifications
You must be signed in to change notification settings - Fork10k
feat: add secrets management#17648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
bwplotka left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is good! Super minimal change surface.
I'd actually iterate over this PR and make this ready to be merged once common change is merged. We could double check all tests etc. 💪🏽
There are definitely some tests to fix, but it might be as trivial as adding(f *Field) Equals(other Field) bool so comparisons work correctly (they don't depend on state):
| }, | ||
| func(error) { | ||
| logger.Info("Stopping scrape discovery manager...") | ||
| cancelScrape() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It probably needs its own context.
| var ( | ||
| secretsManager*secrets.Manager | ||
| ) | ||
| // needs logger: logger.With("component", "secrets manager") | ||
| secretsManager,err=secrets.NewManager(context.Background(),prometheus.DefaultRegisterer,secrets.Providers,cfgFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| var ( | |
| secretsManager*secrets.Manager | |
| ) | |
| // needs logger: logger.With("component", "secrets manager") | |
| secretsManager,err=secrets.NewManager(context.Background(),prometheus.DefaultRegisterer,secrets.Providers,cfgFile) | |
| // TODO: Pass logger.With("component", "secrets manager") | |
| secretsManager,err:=secrets.NewManager(context.Background(),prometheus.DefaultRegisterer,secrets.Providers,cfgFile) |
| select { | ||
| case<-hup: | ||
| iferr:=reloadConfig(cfg.configFile,cfg.tsdb.EnableExemplarStorage,logger,noStepSubqueryInterval,callback,reloaders...);err!=nil { | ||
| iferr:=reloadConfig(cfg.configFile,cfg.tsdb.EnableExemplarStorage,logger,noStepSubqueryInterval,secretsManager,callback,reloaders...);err!=nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could probably DRY this g.Add with something like
// before loopreload = func()error { return reloadConfig(cfg.configFile, cfg.tsdb.EnableExemplarStorage, logger, noStepSubqueryInterval, callback, reloaders...),}// ...if err := reload(); err != nil {| } | ||
| } | ||
| iferr:=secretsManager.HydrateConfig(nil,conf);err!=nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we could work on this name. Reminds me to grab a cup of water though! (:
Maybe:
| iferr:=secretsManager.HydrateConfig(nil,conf);err!=nil { | |
| iferr:=secretsManager.ReloadConfig(context.TODO(),conf);err!=nil { |
| NamespaceDiscoveryNamespaceDiscovery`yaml:"namespaces,omitempty"` | ||
| Selectors []SelectorConfig`yaml:"selectors,omitempty"` | ||
| AttachMetadataAttachMetadataConfig`yaml:"attach_metadata,omitempty"` | ||
| ExampleSecret secrets.Field`yaml:"example_secret"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice, easy use 👍🏽
Let's remove and iterate over this PR to be effectively testable and potentially mergable once common change is done, looks promising!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Although your other examples use*secrets.Field (pointer)?
bwplotka commentedDec 8, 2025
For release log, keep in mind this is for user facing changes (e.g. user of Prometheus YAML). I would mention what user can expect (generic field and removal of ref). I'd recommend: |
bwplotka commentedDec 11, 2025
Exciting feature! We even plan to mention it on KubeCon EU in March if that's ok (: cc@hsmatulis Hopefully it's done until then 💪🏽 |
secrets: Add remote secrets providers
See the proposal here. This PR is currently a work in progress, demonstrating how user's would interact with the secrets management API
Which issue(s) does the PR fix:
Does this PR introduce a user-facing change?