Resolvable secrets #3831
Resolvable secrets #3831
Conversation
mnonnenmacher
force-pushed
the
resolvable-secrets
branch
4 times, most recently
from
a9ba072 to
b9f4056
Compare
mnonnenmacher
force-pushed
the
resolvable-secrets
branch
2 times, most recently
from
36e4741 to
381268c
Compare
Add a class that describes a secret and the source that it can be resolved from. It will taken into use for ORT plugin configurations in the following commits. Signed-off-by: Martin Nonnenmacher <[email protected]>
Add a variant of `PluginConfig` that uses `ResolvableSecret` to record the source from which a secret must be resolved. It will be taken into use for ORT plugin configurations in the following commits. Signed-off-by: Martin Nonnenmacher <[email protected]>
Use the new `ResolvablePluginConfig` class instead of `PluginConfig` in job configurations to prepare for supporting the resolution of plugin secrets also from user configured secrets. At this point, all plugin secrets are still resolved from admin secrets. Signed-off-by: Martin Nonnenmacher <[email protected]>
The service will be used to resolve secrets from the hierarchy. Signed-off-by: Martin Nonnenmacher <[email protected]>
The class mainly tests the worker context itself, not the factory. Signed-off-by: Martin Nonnenmacher <[email protected]>
Signed-off-by: Martin Nonnenmacher <[email protected]>
The function will be used to avoid the need for accessing the secret storage directly. Signed-off-by: Martin Nonnenmacher <[email protected]>
Create the `ContextFactoryTestHelper` in a central place instead of in every single test. Signed-off-by: Martin Nonnenmacher <[email protected]>
Make the constructor arguments of `ContextFactoryTesteHelper` class properties because they were always initialized with the default values anyway. Signed-off-by: Martin Nonnenmacher <[email protected]>
Use the `SecretService` to resolve secret values instead of creating a separate instance of `SecretStorage` in `WorkerContextImpl`. Signed-off-by: Martin Nonnenmacher <[email protected]>
This is a fixup for 19bf3c1. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
resolvable-secrets
branch
from
381268c to
66d8ccf
Compare
Resolve secrets in `PluginConfig`s depending on their source. Previously, all secrets were resolved from config secrets. Note that currently all secrets still use `SecretSource.ADMIN`, this will be changed in a later commit. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
resolvable-secrets
branch
from
66d8ccf to
44d365e
Compare
mnonnenmacher
requested review from
MarcelBochtler,
bs-ondem,
nnobelis,
oheger-bosch,
sschuberth and
wkl3nk
as code owners
| @@ -0,0 +1,79 @@ | |||
| /* | |||
There was a problem hiding this comment.
Commit message:
will taken
will be taken
| override val descriptor = SerialDescriptor( | ||
| serialName = "org.eclipse.apoapsis.ortserver.model.ResolvableSecret.Custom", | ||
| original = delegate.descriptor | ||
| ) |
There was a problem hiding this comment.
Shouldn't the descriptor say it's a contextual serializer, similar to here?
In general, you might be able to simplify code by doing like in that other class and using the generated serializer as a delegate.
| @@ -0,0 +1,36 @@ | |||
| /* | |||
There was a problem hiding this comment.
Can the commit message maybe also explain if the goal is to remove the non-ResolvableSecret PluginConfig going forward?
| return parallelTransform(secrets.toList(), secretsCache, this::resolveSecret, ::extractSecretKey) | ||
| } | ||
| override suspend fun resolveSecrets(vararg secrets: Secret): Map<Secret, String> = | ||
| parallelTransform(secrets.toList(), secretsCache, this::resolveSecret, ::extractSecretKey) |
There was a problem hiding this comment.
Unrelated to this change as it aligns with 133 in the following regard, but as a general those this qualifiers do not seem to be necessary.
| /** The [Organization] the current repository and product belong to. */ | ||
| val organization: Organization | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
from config secrets
Should this say "from admin secrets"?
| private val repositoryRepository: RepositoryRepository, | ||
|
|
||
| /** The service for accessing secrets. */ | ||
| private val secretService: SecretService |
There was a problem hiding this comment.
The purpose of this commit is unclear to me. Why adding the service reference when it is not yet used? Shouldn't this be squashed with the later commit that actually takes the service into use?
| } | ||
|
|
||
| private val hierarchySecrets by lazy { | ||
| runBlocking { secretService.listForHierarchy(hierarchy) }.associateBy { it.name } |
There was a problem hiding this comment.
Not sure about this usage of runBlocking here, as it blocks the calling thread. It would probably be more coroutine-friendly to manage the field manually using a Mutex instead of the lazy approach.
Or, if the field is only used to test whether a referenced secret exists, could this test be shifted to later when the secret value is actually resolved?
| config?.let { c -> | ||
| val secrets = c.values.flatMap { pluginConfig -> pluginConfig.secrets.values.map { it.name } } | ||
| val resolvedSecrets = parallelTransform(secrets, configSecretsCache, this::resolveConfigSecret) { it } | ||
| val configSecrets = c.values.flatMap { pluginConfig -> |
There was a problem hiding this comment.
Maybe use partition here to distinguish between config and user secrets (assuming that there are only two sources)?
Resolve secrets in plugin configurations from either the user secrets or the admin secrets, depending on who configured the plugin. See the commit messages for details.