Skip to content

Commit 2d5e195

Browse files
jujubotjujubot
authored
Merge pull request juju#15250 from wallyworld/cmr-secret-data
juju#15250 For a cross model secret, the offering model needs to manage secret consumer info from the consuming model so that it can fire the `secret-removed` hook and remove references to the consumer when the app/units are deleted. Also a driveby bug fix to record the correct consumed revision for peek/refresh. ## Checklist - [X] Code style: imports ordered, good names, simple structure, etc - [X] Comments saying why design decisions were made - [X] Go unit tests, with comments saying what you're testing - ~[ ] [Integration tests](https://github.com/juju/juju/tree/develop/tests), with comments saying what you're testing~ - ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~ ## QA steps bootstrap 2 controllers, deploy juju-qa-dummy-source and offer it; in the other controller, deploy juju-qa-dummy-sink and create a cross model relation. ``` $ juju switch c1 $ juju exec --unit dummy-source/0 "secret-add foo=bar" secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0 $ juju exec --unit dummy-source/0 "secret-grant -r 0 secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0" $ juju switch c2 c1:admin/o -> c2:admin/c $ juju exec --unit dummy-sink/0 "secret-get secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0" foo: bar $ juju switch c1 c2:admin/c -> c1:admin/o $ juju exec --unit dummy-source/0 "secret-set secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0 foo=bar2" $ juju switch c2 c1:admin/o -> c2:admin/c $ juju exec --unit dummy-sink/0 "secret-get secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0" foo: bar $ juju exec --unit dummy-sink/0 "secret-get secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0 --peek" foo: bar2 $ juju exec --unit dummy-sink/0 "secret-get secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0" foo: bar $ juju exec --unit dummy-sink/0 "secret-get secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0 --label baz" foo: bar $ juju exec --unit dummy-sink/0 "secret-get --label baz" foo: bar $ juju switch c1 c2:admin/c -> c1:admin/o $ juju show-status-log dummy-source/0 ... 02 Mar 2023 14:08:17+10:00 juju-unit executing running secret-remove hook for secret:cg020rhkf17hs5nd9us0/1 ... $ juju switch c2 c1:admin/o -> c2:admin/c $ juju exec --unit dummy-sink/0 "secret-get --label baz --refresh" foo: bar2 $ juju exec --unit dummy-sink/0 "secret-get --label baz" foo: bar2 $ juju add-unit dummy-sink $ juju exec --unit dummy-sink/1 "secret-get secret://bdc94272-1c0f-49e2-84f6-e3df50772020/cg020rhkf17hs5nd9us0" foo: bar2 $ juju remove-unit dummy-sink/0 WARNING This command will perform the following actions: will remove unit dummy-sink/0 Continue [y/N]? y ``` In a mongo shell, ensure the secret permission record for the remote proxy of dummy-source/0 is deleted.
2 parents 6e1cac3 + 2154282 commit 2d5e195

File tree

15 files changed

+263
-22
lines changed

15 files changed

+263
-22
lines changed

‎apiserver/common/crossmodel/crossmodel.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func PublishRelationChange(backend Backend, relationTag names.Tag, change params
104104
}
105105
}
106106

107-
if err := handleDepartedUnits(change, applicationTag, rel); err != nil {
107+
if err := handleDepartedUnits(backend, change, applicationTag, rel); err != nil {
108108
return errors.Trace(err)
109109
}
110110

@@ -143,7 +143,7 @@ func handleSuspendedRelation(change params.RemoteRelationChangeEvent, rel Relati
143143
return nil
144144
}
145145

146-
func handleDepartedUnits(change params.RemoteRelationChangeEvent, applicationTag names.Tag, rel Relation) error {
146+
func handleDepartedUnits(backend Backend, change params.RemoteRelationChangeEvent, applicationTag names.Tag, rel Relation) error {
147147
for _, id := range change.DepartedUnits {
148148
unitTag := names.NewUnitTag(fmt.Sprintf("%s/%v", applicationTag.Id(), id))
149149
logger.Debugf("unit %v has departed relation %v", unitTag.Id(), rel.Tag().Id())
@@ -155,6 +155,9 @@ func handleDepartedUnits(change params.RemoteRelationChangeEvent, applicationTag
155155
if err := ru.LeaveScope(); err != nil {
156156
return errors.Trace(err)
157157
}
158+
if err := backend.RemoveSecretConsumer(unitTag); err != nil {
159+
return errors.Trace(err)
160+
}
158161
}
159162
return nil
160163
}

‎apiserver/common/crossmodel/interface.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ type Backend interface {
9999

100100
// ApplyOperation applies a model operation to the state.
101101
ApplyOperation(op state.ModelOperation) error
102+
103+
// RemoveSecretConsumer removes secret references for the specified consumer.
104+
RemoveSecretConsumer(consumer names.Tag) error
102105
}
103106

104107
// Relation provides access a relation in global state.

‎apiserver/facades/agent/secretsmanager/secrets.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -481,9 +481,12 @@ func (s *SecretsManagerAPI) getRemoteSecretContent(uri *coresecrets.URI, refresh
481481
var wantRevision int
482482
if err == nil {
483483
wantRevision = consumerInfo.CurrentRevision
484+
} else {
485+
// Not found so need to create a new record and populate
486+
// with latest revision.
487+
refresh = true
488+
consumerInfo = &coresecrets.SecretConsumerMetadata{}
484489
}
485-
refresh = refresh ||
486-
err != nil // Not found, so need to create one.
487490

488491
scopeToken, err := extClient.GetSecretAccessScope(uri, token, unitId)
489492
if err != nil {
@@ -508,11 +511,10 @@ func (s *SecretsManagerAPI) getRemoteSecretContent(uri *coresecrets.URI, refresh
508511
return nil, nil, false, errors.Trace(err)
509512
}
510513
if refresh || updateLabel {
511-
if consumerInfo == nil {
512-
consumerInfo = &coresecrets.SecretConsumerMetadata{}
514+
if refresh {
515+
consumerInfo.LatestRevision = latestRevision
516+
consumerInfo.CurrentRevision = latestRevision
513517
}
514-
consumerInfo.LatestRevision = latestRevision
515-
consumerInfo.CurrentRevision = latestRevision
516518
if label != "" {
517519
consumerInfo.Label = label
518520
}

‎apiserver/facades/agent/secretsmanager/secrets_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,8 +1028,7 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelExistingConsumerNoRe
10281028

10291029
s.secretsConsumer.EXPECT().SaveSecretConsumer(uri, consumer, &coresecrets.SecretConsumerMetadata{
10301030
Label: "foo",
1031-
LatestRevision: 666,
1032-
CurrentRevision: 666,
1031+
CurrentRevision: 665,
10331032
})
10341033

10351034
results, err := s.facade.GetSecretContentInfo(params.GetSecretContentArgs{

‎apiserver/facades/controller/crossmodelrelations/mock_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ func (st *mockState) ApplyOperation(state.ModelOperation) error {
103103
return nil
104104
}
105105

106+
func (st *mockState) RemoveSecretConsumer(consumer names.Tag) error {
107+
return nil
108+
}
109+
106110
func (st *mockState) AddRelation(eps ...state.Endpoint) (commoncrossmodel.Relation, error) {
107111
rel := &mockRelation{
108112
id: len(st.relations),

‎apiserver/facades/controller/crossmodelsecrets/crossmodelsecrets.go

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -204,26 +204,52 @@ func (s *CrossModelSecretsAPI) getSecretContent(arg params.GetRemoteSecretConten
204204
return nil, nil, 0, apiservererrors.ErrPerm
205205
}
206206

207-
md, err := secretState.GetSecret(uri)
208-
if err != nil {
209-
return nil, nil, 0, errors.Trace(err)
210-
}
211-
212-
var wantRevision int
207+
var (
208+
wantRevision int
209+
latestRevision int
210+
)
213211
// Use the latest revision as the current one if --peek.
214212
if arg.Peek || arg.Refresh {
215-
wantRevision = md.LatestRevision
213+
var err error
214+
latestRevision, err = s.getConsumedRevision(secretState, secretsConsumer, consumer, uri, arg.Refresh)
215+
if err != nil {
216+
return nil, nil, 0, errors.Trace(err)
217+
}
218+
wantRevision = latestRevision
216219
} else {
217220
wantRevision = *arg.Revision
218221
}
219222

220223
val, valueRef, err := secretState.GetSecretValue(uri, wantRevision)
221224
content := &secrets.ContentParams{SecretValue: val, ValueRef: valueRef}
222225
if err != nil || content.ValueRef == nil {
223-
return content, nil, md.LatestRevision, errors.Trace(err)
226+
return content, nil, latestRevision, errors.Trace(err)
224227
}
225228
backend, err := s.getBackend(uri.SourceUUID, content.ValueRef.BackendID)
226-
return content, backend, md.LatestRevision, errors.Trace(err)
229+
return content, backend, latestRevision, errors.Trace(err)
230+
}
231+
232+
func (s *CrossModelSecretsAPI) getConsumedRevision(secretsState SecretsState, secretsConsumer SecretsConsumer, consumer names.Tag, uri *coresecrets.URI, refresh bool) (int, error) {
233+
consumerInfo, err := secretsConsumer.GetSecretConsumer(uri, consumer)
234+
if err != nil && !errors.Is(err, errors.NotFound) {
235+
return 0, errors.Trace(err)
236+
}
237+
if err != nil {
238+
consumerInfo = &coresecrets.SecretConsumerMetadata{}
239+
refresh = true
240+
}
241+
242+
md, err := secretsState.GetSecret(uri)
243+
if err != nil {
244+
return 0, errors.Trace(err)
245+
}
246+
consumerInfo.CurrentRevision = md.LatestRevision
247+
if refresh {
248+
if err := secretsConsumer.SaveSecretConsumer(uri, consumer, consumerInfo); err != nil {
249+
return 0, errors.Trace(err)
250+
}
251+
}
252+
return md.LatestRevision, nil
227253
}
228254

229255
func (s *CrossModelSecretsAPI) getBackend(modelUUID string, backendID string) (*params.SecretBackendConfigResult, error) {

‎apiserver/facades/controller/crossmodelsecrets/crossmodelsecrets_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,14 @@ func ptr[T any](v T) *T {
139139
}
140140

141141
func (s *CrossModelSecretsSuite) TestGetSecretContentInfo(c *gc.C) {
142+
s.assertGetSecretContentInfo(c, false)
143+
}
144+
145+
func (s *CrossModelSecretsSuite) TestGetSecretContentInfoNewConsumer(c *gc.C) {
146+
s.assertGetSecretContentInfo(c, true)
147+
}
148+
149+
func (s *CrossModelSecretsSuite) assertGetSecretContentInfo(c *gc.C, newConsumer bool) {
142150
defer s.setup(c).Finish()
143151

144152
uri := coresecrets.NewURI().WithSource(coretesting.ModelTag.Id())
@@ -159,9 +167,18 @@ func (s *CrossModelSecretsSuite) TestGetSecretContentInfo(c *gc.C) {
159167
s.crossModelState.EXPECT().GetConsumerInfo("token3").Return(app3, offerUUID, nil)
160168
s.stateBackend.EXPECT().HasEndpoint(relation.Id(), "remote-app3").Return(false, nil)
161169

170+
consumerTag := names.NewUnitTag("remote-app/666")
171+
if newConsumer {
172+
s.secretsConsumer.EXPECT().GetSecretConsumer(uri, consumerTag).Return(nil, errors.NotFoundf(""))
173+
} else {
174+
s.secretsConsumer.EXPECT().GetSecretConsumer(uri, consumerTag).Return(&coresecrets.SecretConsumerMetadata{CurrentRevision: 69}, nil)
175+
}
162176
s.secretsState.EXPECT().GetSecret(uri).Return(&coresecrets.SecretMetadata{
163177
LatestRevision: 667,
164178
}, nil)
179+
s.secretsConsumer.EXPECT().SaveSecretConsumer(uri, consumerTag, &coresecrets.SecretConsumerMetadata{
180+
CurrentRevision: 667,
181+
}).Return(nil)
165182
s.secretsConsumer.EXPECT().SecretAccess(uri, consumer).Return(coresecrets.RoleView, nil)
166183
s.secretsState.EXPECT().GetSecretValue(uri, 667).Return(
167184
nil,

‎apiserver/facades/controller/crossmodelsecrets/mocks/secretsconsumer.go

Lines changed: 29 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎apiserver/facades/controller/crossmodelsecrets/state.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ type SecretsState interface {
1919
}
2020

2121
type SecretsConsumer interface {
22+
GetSecretConsumer(*secrets.URI, names.Tag) (*secrets.SecretConsumerMetadata, error)
23+
SaveSecretConsumer(*secrets.URI, names.Tag, *secrets.SecretConsumerMetadata) error
2224
SecretAccess(uri *secrets.URI, subject names.Tag) (secrets.SecretRole, error)
2325
SecretAccessScope(uri *secrets.URI, subject names.Tag) (names.Tag, error)
2426
}

‎apiserver/facades/controller/remoterelations/mocks/remoterelations_mocks.go

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎state/application.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ func (op *DestroyApplicationOperation) deleteSecrets() error {
391391
return errors.Annotatef(err, "deleting owned secrets for %q", op.app.Name())
392392
}
393393
// TODO(juju4) - remove
394-
if err := op.app.st.removeSecretConsumer(op.app.Tag()); err != nil {
394+
if err := op.app.st.RemoveSecretConsumer(op.app.Tag()); err != nil {
395395
return errors.Annotatef(err, "deleting secret consumer records for %q", op.app.Name())
396396
}
397397
return nil

‎state/remoteapplication.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,9 @@ func (op *DestroyRemoteApplicationOperation) Done(err error) error {
331331
}
332332
op.AddError(errors.Errorf("force erase saas application %q history proceeded despite encountering ERROR %v", op.app, err))
333333
}
334+
if err := op.deleteSecretReferences(); err != nil {
335+
logger.Errorf("cannot delete secret references for saas application %q: %v", op.app, err)
336+
}
334337
return nil
335338
}
336339

@@ -345,6 +348,13 @@ func (op *DestroyRemoteApplicationOperation) eraseHistory() error {
345348
return nil
346349
}
347350

351+
func (op *DestroyRemoteApplicationOperation) deleteSecretReferences() error {
352+
if err := op.app.st.removeRemoteSecretConsumer(op.app.Name()); err != nil {
353+
return errors.Annotatef(err, "deleting secret consumer records for %q", op.app.Name())
354+
}
355+
return nil
356+
}
357+
348358
// DestroyWithForce in addition to doing what Destroy() does,
349359
// when force is passed in as 'true', forces th destruction of remote application,
350360
// ignoring errors.
@@ -500,6 +510,12 @@ func (s *RemoteApplication) removeOps(asserts bson.D) ([]txn.Op, error) {
500510
tokenOps := r.removeRemoteEntityOps(s.Tag())
501511
ops = append(ops, tokenOps...)
502512

513+
secretConsumerPermissionsOps, err := s.st.removeConsumerSecretPermissionOps(s.Tag())
514+
if err != nil {
515+
return nil, errors.Annotatef(err, "deleting secret consumer records for %q", s.Name())
516+
}
517+
ops = append(ops, secretConsumerPermissionsOps...)
518+
503519
// If this is the last consumed app off an external controller,
504520
// also remove the external controller record.
505521
if s.doc.SourceControllerUUID != "" {

0 commit comments

Comments
 (0)