refactor: remove application service dependency on secret service #18225
Conversation
wallyworld
force-pushed
the
refactor-appservice-dependency
branch
from
2dc8982
to
6dbeec4
Compare
The secret service was being passed as a dependency to the application service, to allow the application service to delete app and unit owned secrets. Instead of construcitng the entire secret service, pass in just the components needed for the application service to construct a secret deleter to do the job.
wallyworld
force-pushed
the
refactor-appservice-dependency
branch
from
6dbeec4
to
5007ce9
Compare
| func (s NotImplementedSecretService) GetSecretsForOwners(ctx domain.AtomicContext, owners ...secretservice.CharmSecretOwner) ([]*coresecrets.URI, error) { | ||
| return nil, nil | ||
| } | ||
| // NotImplementedSecretDeleter defines a secret service which does nothing. |
| if params.SecretBackendReferenceDeleter == nil { | ||
| params.SecretBackendReferenceDeleter = NotImplementedSecretDeleter{} | ||
| } | ||
| secretDeleter := &secretservice.SecretDeleter{ |
There was a problem hiding this comment.
This doesn't seem right. Why are we reliant on a service here? If it truly is a component, then the service can define an interface and operate on that. Then you could offer an implementation from the secret domain that provided that or have an application only version. Either way, we're coupling services here.
There was a problem hiding this comment.
There's complex logic in deleting a secret that is rightly implemented solely in the secret service. Services should be able to collaborate as part of a workflow. Either we allow services to be wired together via the dependency engine, we allow composition to be used, or we accept we need to cut and paste non trivial logic in different domains.
| } | ||
|
|
||
| query := ` | ||
| SELECT sm.secret_id AS &secretID.id |
There was a problem hiding this comment.
I'd create a view for this and use it in both places.
There was a problem hiding this comment.
I was looking at this today and realised that this is a movement of the service method from secret to application. Can you propose a patch with just that part?
We weren't happy with the previous pattern either. We need a solution for this problem. Services need to be able to collaborate as part of a workflow. If we don't want to use the dependency engine to wire up dependent services, and we don't want to use composition, then we need something. |
The secret service was being passed as a dependency to the application service, to allow the application service to delete app and unit owned secrets. Instead of constructing the entire secret service, pass in just the components needed for the application service to construct a secret deleter to do the job.
Since the secret service is not used directly by the application service, we can move the state logic to get secret owners from secret to application domain. This change to state is literally cut and paste.
For secret deletion, a SecretDeleter is created by both the application and secret services - this contains the complicated logic needed to delete secrets and maintain backends etc. The application service is now constructed with just the bits needed for the secret deleter, not the entire secret service. The components needed are a backend reference deleter and a backend admin config getter for a model.
There's still work needed to move some existing params out of the service packages, but this is for another PR.
As a drive by, fix an issue deleting secrets - the ports domain work neglected to update unit deleting to account for deleting the unit endpoint rows. Add this and update the test.
Also a drive by fix to the shell tests because "microk8s" is a valid bootstrap provider.
QA steps
juju bootstrap microk8s
juju add-model test
juju deploy snappass-test
juju exec -u snappass-test/0 -- secret-add foo=bar
juju remove-application snappass-test
Links
Jira card: JUJU-6930