- Notifications
You must be signed in to change notification settings - Fork0
In progress proof of concept using secret library#1
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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Henrique Matulis <hmatulis@google.com>
65a024d tob07b605Compare
bwplotka left a comment
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! Good progress. Should we switch main PR discussion to this one?
Nevertheless, I see the point for "hydration", it's for some validation and _file compat. Why not adding another method to secret for setting raw config and if it's set already we fail?
Generally I like theprometheus#797, but the test and code wouldn't work on this common flow because we don't usePrepareSecrets etc. I think it's a good time to iterate to get to a green state ofprometheus#797!
| secretField*Field,filePathstring)error { | ||
| ifh.manager==nil||h.manager.hydrating==false { | ||
| panic("OrFromFile can only be called during PrepareSecrets;the manager has not yet started hydration") | ||
| panic("secret field is sealed bythe manager; call this from PrepareSecrets") |
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 already return error, so we could use it. For deeply imported libraries ideally we have panic-less flows.
| returnerr | ||
| } | ||
| returnc.Validate() | ||
| func (c*TLSConfig)PrepareSecrets(h secrets.Hydrator)error { |
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.
Where do we use this? I can't see any use of this 🤔
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.
Ah, so this is for compatibility with _file. Do you think we could bake this intoUnmarshalYAML? Maybesecret.Field would need to have extra method for saving this info instead? Likefield.SetRawConfig?
| returnnil | ||
| } | ||
| func (m*Manager)HydrateConfig(configinterface{})error { |
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.
Hydrate is odd, like it does not tell much. Maybe reload is clearer?
| func (m*Manager)HydrateConfig(configinterface{})error { | |
| func (m*Manager)ReloadConfig(configinterface{})error { |
| // | ||
| // This method should be called before accessing any secret values to ensure | ||
| // that they are available. | ||
| func (m*Manager)SecretsReady(configinterface{}) (bool,error) { |
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.
Let's remove indeed
| path:=h.fields.paths[*secretField] | ||
| ifcount(secretField!=nil,len(filePath)>0)==2 { | ||
| path:=h.fields[secretField] | ||
| returnfmt.Errorf("at most one of %s & %s_file must be configured",path,path) |
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.
Can we validate this on reload? OrOrFromFile and the whole Hydrator should be done on reload? Seems the whole thing is not used inprometheus/prometheus#17648? 🤔
| // | ||
| // Returning an error here is FATAL. It indicates the configuration | ||
| // is structurally unsound and the Manager should exit immediately. | ||
| typeSecretPreparerinterface { |
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.
Not a fan it seems we replaceSecretsReady with some new stuff doing the same.
Why not just do this onReloadConfig (you called itHydrateConfig)?
| // VerifySecrets() error | ||
| // } | ||
| typeSecretVerifierinterface { | ||
| VerifySecrets()error |
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.
What's this, I can't find use of this method. Should we remove?
| VerifySecrets()error | ||
| } | ||
| typeHydratorstruct { |
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.
Perhaps:
| typeHydratorstruct { | |
| typeReloaderstruct { |
| // file: | ||
| // path: /path/to/password.txt | ||
| typeFieldstruct { | ||
| rawConfigany |
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.
| rawConfigany | |
| visibilitybool |
No description provided.