Secrets interface cleanup #3842
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3842 +/- ##
========================================
Coverage 15.53% 15.53%
========================================
Files 731 731
Lines 81497 81497
Branches 1087 1087
========================================
Hits 12664 12664
Misses 68833 68833
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| """The list of secrets known to the operator, or None.""" | ||
| return self._secrets | ||
|
|
||
| def register_secrets(self, secrets): |
There was a problem hiding this comment.
doesn't this allow registering secrets that are not in the plugin definition?
There was a problem hiding this comment.
Yes it does. Also true for the current implementation in develop. It reflects a need for further refactoring as it would be more appropriate for secret registration to be owned by the ExecutionContext, not the Operator.
There was a problem hiding this comment.
The current implementation doesn't facilitate users to register unlisted secrets
There was a problem hiding this comment.
All I did was rename the add_secrets() method to register_secrets()
| self.__secrets = {} | ||
| self._operator_uri = None | ||
| self._resolver = None | ||
| class SecretsDictionary(Mapping): |
There was a problem hiding this comment.
Why are you removing this?!?
There was a problem hiding this comment.
The secret resolution logic is now inside ExecutionContext, and we get the other read-only aspects for free by implementing Mapping
There was a problem hiding this comment.
I don't believe we should make it a simple dict. I think we should at least try to make secrets secure and iterate
brimoor
changed the title
brimoor
changed the title
Change log
operator._plugin_secretsoutside of theOperatorclassExecutionContext, as opposed to being partly inExecutionContextand partly inSecretsDictionaryNotes
ctx.resolve_secret_values()is called. Currently that is not the case, as it also appears to be necessary forctx.secretsto dynamically resolve secrets. That behavior is maintained in this PR (although all secrets are resolved in a batch the first timectx.secretsis accessed, rather than resolving one-by-one).