Add model config handlers for save and load for secret backends #17404

wallyworld · 2024-05-23T05:34:25Z

Model config handlers are used to extract model config values to populate the data model on save, and the reverse on load. Many model config values are scalars and do belong in the key value data bag. However, things like secret backend and default space form part of the relational data model; as such, we need to provide a mechanism to handle this scenario.

As part of the implementation, the previously added logic to update the model secret backend was moved from secretbackend package to model config.

A config handler is called as part of the "update" and "get" service calls. Handlers act on a defined subset of the config keys. They specify what keys they are interested in - the service will extra those keys from the databag being saved and pass their values to the handler. After the handler is done, the keys are removed from the databag so the final write to the db only persists key values not already processed by a handler. For gets, the data model is queried and values are inserted into the config. This allows the current UX and api semantics to be retained.

There were also some missing validations for secret backend, based on model type. The validation logic is updated accordingly. Handlers have a Validate() method - if a handler is used, it's responsible for all validation for the attributes it cares about.

Sadly, updates from txn boundaries (writes to different dbs) and so the updates are not atomic. Iff handlers are run but the final db updates result in an error, a PartialSaveError is returned containing (if known) the attributes that could not be saved and the cause (some attributes will have been saved). It is expected the caller might try again with the same values. This strategy was done instead of attempting to rollback on error.

Also, test helpers are updated to take a model type.

Note - future work will be needed to improve model defaults. These are still just treated as a data bag of values and there's no validation done at all, and there's no corresponding modelling of non scalar values.

This PR implements support for secret backend - the model default space will also need to be done separately.

QA steps

$ juju model-config secret-backend
internal
$ juju model-config secret-backend=kubernetes
ERROR config key ["secret-backend"] not valid: iaas secret back cannot be set to "kubernetes" (not valid)
$ juju add-secret-backend myvault vault endpoint=http://10.0.0.1:8200
$ juju model-config secret-backend=myvault
$ juju model-config secret-backend
myvault
$ juju model-config secret-backend --format yaml
secret-backend:
  value: myvault
  source: model
$ juju model-config --reset secret-backend
$ juju model-config secret-backend
internal

Links

Jira card: JUJU-6051

ycliuhw

Nice change. Just one question.

domain/modelconfig/handlers/secrethandler.go

domain/modelconfig/state/controllerstate.go

hpidcock

Not sure I like this pattern but I see the need.

domain/modelconfig/handlers/secrethandler.go

domain/modelconfig/service/service.go

domain/modelconfig/validators/validators.go

domain/modelconfig/service/service.go

wallyworld · 2024-05-25T00:52:51Z

Not sure I like this pattern but I see the need.

We discussed at the sprint and it was agreed it was what we needed to do.

manadart

juju add-secret-backend myvault vault endpoint=http://10.0.0.1:8200 --debug hangs for me. There is no corresponding debug-log output.

I'm concerned about the cross-DB transaction issues. Even though we would lose some RI, wouldn't it be better to record the model's secret back-end in the model database, allowing you to get transactional safety?

domain/modelconfig/state/state.go

domain/modelconfig/state/controllerstate.go

domain/modelconfig/service/service.go

domain/model/state/testing/model.go

domain/modelconfig/handlers/interface.go

SimonRichardson · 2024-05-28T14:12:23Z

domain/modelconfig/handlers/secrethandler.go

+	err := h.BackendState.SetModelSecretBackend(ctx, h.ModelUUID, backendName)
+	if err != nil && !errors.Is(err, backenderrors.NotFound) {
+		return fmt.Errorf("cannot set secret backend to %q: %w", backendName, err)
+	}


Are we always going to be able to set the value in model config before setting the model config, are we ever going to have the requirement to do it post setting the model config?

If I understand you correctly, I'd rather do things in this order - do the relational model update(s) and then save the model config.

tlm · 2024-05-28T22:17:47Z

Thank you for the work @wallyworld. My review will follow after this but just wanted to dump a thought here that will lead into the review.

I think we need to make the decision now on what model config actually is as we are distorting the concepts at every layer and we are ending up with an identity problem.

To that end my current thinking is we need to think about ModelConfig as a user interface component and nothing more. Hopefully this will make more sense in my review comments.

tlm

Thanks @wallyworld for playing around with this and providing the prototype. Please know my comments come from a good place.

I want to discuss this more as I think there is a lot of concerns being mixed here.

domain/modelconfig/handlers/secrethandler.go

domain/modelconfig/service/service.go

domain/model/state/testing/model.go

domain/modelconfig/handlers/interface.go

domain/modelconfig/handlers/secrethandler.go

domain/modelconfig/service/service.go

tlm · 2024-05-29T01:36:03Z

domain/modelconfig/service/service.go

 // - There is no changes to authorized keys.
 func (s *Service) updateModelConfigValidator(
+	modelType model.ModelType,


modelType should be fetched within this func. It makes no sense to make the call site do the extra work when we can do it in here do what we say on the tin.

This avoids doing 2 queries when 1 will do the job. The caller makes a call to st to get the model uuid and the model type in one query on the model table. The model type is used here and the uuid used by the caller in the next step. I really didn't want to make 2 queries to the model table.

domain/modelconfig/service/service.go

wallyworld · 2024-05-29T06:06:40Z

juju add-secret-backend myvault vault endpoint=http://10.0.0.1:8200 --debug hangs for me. There is no corresponding debug-log output.

I'm concerned about the cross-DB transaction issues. Even though we would lose some RI, wouldn't it be better to record the model's secret back-end in the model database, allowing you to get transactional safety?

Do you have a vault running? Part of the validation is to Ping() the backend to ensure it is configured correctly. Perhaps the ping is taking a while to time out. This isn't new to this PR per se. Assuming this does need to be a synchronous check, we really need a better way to provide feedback to the caller. If we want to make it async, that would allow a bad backend config to get into the model and the workflow to deal with that I think would possibly be more inconvenient; we don't have a way currently to surface bad config and inform the user how to fix it.

wallyworld · 2024-05-29T06:12:57Z

juju add-secret-backend myvault vault endpoint=http://10.0.0.1:8200 --debug hangs for me. There is no corresponding debug-log output.
I'm concerned about the cross-DB transaction issues. Even though we would lose some RI, wouldn't it be better to record the model's secret back-end in the model database, allowing you to get transactional safety?

Do you have a vault running? Part of the validation is to Ping() the backend to ensure it is configured correctly. Perhaps the ping is taking a while to time out. This isn't new to this PR per se. Assuming this does need to be a synchronous check, we really need a better way to provide feedback to the caller. If we want to make it async, that would allow a bad backend config to get into the model and the workflow to deal with that I think would possibly be more inconvenient; we don't have a way currently to surface bad config and inform the user how to fix it.

RE: the cross txn issues. This was discussed and it was felt this approach was better. It's a case of pick your poison. Adding a best effort rollback - which should have been done originally - mitigates somewhat the transactional safety issue whilst maintaining RI. Happy to discuss f2f. The purpose of this PR was to give a concrete look at the abstract idea thrown around at the sprint so as to allow more informed feedback. So the discussion is good. We can talk next dqlite sync.

tlm

Thanks @wallyworld for riffing on this. Have left some more comments. Happy to discuss.

domain/model/state/testing/model.go

domain/modelconfig/handlers/rollback.go

tlm · 2024-06-04T03:58:51Z

domain/modelconfig/handlers/secrethandler.go

+	// if there's any error.
+	currentBackendName, err := h.BackendState.GetModelSecretBackendName(ctx, h.ModelUUID)
+	if err != nil {
+		return noopRollback, errors.Trace(err)


Can we add some context here. Like what we are trying to do and with information.

We need to be careful not to go overboard with annotating errors. The caller which calls the OnSave will print the name of the handler and the message from GetModelSecretBackendName when combined with that will tell you exactly what went wrong without needing to write an essay. It was a deliberate choice not to annotate here.

tlm · 2024-06-04T03:59:45Z

domain/modelconfig/handlers/secrethandler.go

+		return noopRollback, errors.Trace(err)
+	}
+	rollbackFunc := func(ctx context.Context) error {
+		return h.BackendState.SetModelSecretBackend(ctx, h.ModelUUID, currentBackendName)


It would be great if we can add some context to the error here, What we are trying to do and with what information.

domain/modelconfig/handlers/secrethandler.go

domain/modelconfig/service/service.go

tlm · 2024-06-04T04:34:05Z

domain/modelconfig/service/service.go

+	}, nil
+}
+
+func (s *Service) runOnLoadHandlers(ctx context.Context, modelUUID model.UUID, stConfig map[string]string) error {


I have some thoughts on this func.

modelUUID should not be passed into the func. It is a requirement internally to the func to do its job and construct the config Handlers. runOnLoadHandlers can do this work for it self as model config is scoped to single model.

stConfig should not be passed into the function as on load handlers is performing merging that should be left up to the caller. Simply this func should just return the result of calling on load for all the handlers as a merged collection and let the call site worry about the merge. Also the func doesn't modify it's input arguments then.

I don't want to assume the model config map has modelUUID in it as we are trying to get away from that ASAP. So we need to pass in modelUUID as a separate parameter.

OnLoad handler runner has been tweaked.

modelUUID can be fetched from the model info table. You don't need to read model config for the id. My comments above still hold.

Yes, it is read from the model info table. But only once. It is then passed to all the handlers. There's no need to read the same thing multiple times.

domain/modelconfig/handlers/interface.go

domain/modelconfig/validators/validators.go

Model config handlers are used to extract model config values to populate the data model on save, and the reverse on load

Refactor test model creation

Improve doc comments Fix come contexts in tests Onload handlers return merged config

Introduce the concept of partial saves: save what we can and return a PartialSaveError Handlers take a specific set of attributes to work on

tlm · 2024-06-11T08:34:46Z

Not required anymore.

wallyworld added the 4.0 label May 23, 2024

wallyworld force-pushed the secretbackend-config branch 2 times, most recently from dee1b3a to 240ec3d Compare May 23, 2024 06:19

ycliuhw approved these changes May 24, 2024

View reviewed changes

domain/modelconfig/handlers/secrethandler.go Outdated Show resolved Hide resolved

domain/modelconfig/state/controllerstate.go Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

hpidcock reviewed May 24, 2024

View reviewed changes

hpidcock requested a review from tlm May 24, 2024 22:39

manadart reviewed May 28, 2024

View reviewed changes

domain/modelconfig/state/state.go Outdated Show resolved Hide resolved

domain/modelconfig/state/controllerstate.go Outdated Show resolved Hide resolved

domain/modelconfig/service/service.go Outdated Show resolved Hide resolved

SimonRichardson reviewed May 28, 2024

View reviewed changes

SimonRichardson self-requested a review May 28, 2024 15:51

tlm requested changes May 29, 2024

View reviewed changes

hpidcock added the has merge conflicts label May 30, 2024

wallyworld force-pushed the secretbackend-config branch from fb47984 to 0643d64 Compare May 31, 2024 01:24

wallyworld removed the has merge conflicts label May 31, 2024

wallyworld force-pushed the secretbackend-config branch 4 times, most recently from 0e93196 to 6448e85 Compare May 31, 2024 13:41

tlm requested changes Jun 4, 2024

View reviewed changes

wallyworld force-pushed the secretbackend-config branch 7 times, most recently from 4611f4f to 347aa02 Compare June 4, 2024 08:13

wallyworld force-pushed the secretbackend-config branch 6 times, most recently from 1f5ac1a to aa7ca1c Compare June 5, 2024 04:30

wallyworld added 5 commits June 5, 2024 15:06

Add model config handlers for save and load for secret backends

6d88b66

Model config handlers are used to extract model config values to populate the data model on save, and the reverse on load

Implement rollback for failed config handler operations

9653590

Refactor test model creation

Fix some error handling

78d8103

Improve doc comments Fix come contexts in tests Onload handlers return merged config

Move auto backend handling up a layer

9204c45

Rebase on main and resolve conflicts

2e43f77

wallyworld force-pushed the secretbackend-config branch from aa7ca1c to 2f512c2 Compare June 5, 2024 05:17

Remove handler rollback

89f21f2

Introduce the concept of partial saves: save what we can and return a PartialSaveError Handlers take a specific set of attributes to work on

wallyworld force-pushed the secretbackend-config branch from 2f512c2 to 89f21f2 Compare June 5, 2024 05:45

tlm closed this Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add model config handlers for save and load for secret backends #17404

Add model config handlers for save and load for secret backends #17404

wallyworld commented May 23, 2024 •

edited

Loading

ycliuhw left a comment

This comment was marked as resolved.

hpidcock left a comment

wallyworld commented May 25, 2024

manadart left a comment

SimonRichardson May 28, 2024

wallyworld May 29, 2024

tlm commented May 28, 2024

tlm left a comment

tlm May 29, 2024

wallyworld May 29, 2024

wallyworld commented May 29, 2024

wallyworld commented May 29, 2024

tlm left a comment

tlm Jun 4, 2024

wallyworld Jun 4, 2024

tlm Jun 4, 2024

wallyworld Jun 4, 2024

tlm Jun 4, 2024

wallyworld Jun 4, 2024

wallyworld Jun 4, 2024

tlm Jun 4, 2024

wallyworld Jun 5, 2024

tlm commented Jun 11, 2024

Add model config handlers for save and load for secret backends #17404

Add model config handlers for save and load for secret backends #17404

Conversation

wallyworld commented May 23, 2024 • edited Loading

QA steps

Links

ycliuhw left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

hpidcock left a comment

Choose a reason for hiding this comment

wallyworld commented May 25, 2024

manadart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm commented May 28, 2024

tlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyworld commented May 29, 2024

wallyworld commented May 29, 2024

tlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm commented Jun 11, 2024

wallyworld commented May 23, 2024 •

edited

Loading