lxd: Add support for forced deletion of projects

[boltmark]

Currently in draft state as this PR relies on deleting resources via requests to the server's own API. This is not ideal, and I am investigating if it is possible to avoid this. Right now it looks like this is the simplest option, but we could likely add implementation for each resource to perform the deletes without invoking the LXD API. Perhaps we could factor out some of the logic in the delete handlers so that we can reference it locally. Still looking into this -- WIP.

---

This PR adds support for forced deletion of projects. If the --force flag is provided when attempting to delete a project, and if the user confirms that forced deletion is what they want, then all resources associated with the project will be deleted and the project will be removed.

This PR also simplifies the projectIsEmpty function to utilize projectUsedBy instead of checking each resource manually.

Includes cherry-picks from lxc/incus#900.

Closes: #14316.

[github-actions]

Heads up @mionaalex - the "Documentation" label was applied to this issue.

[tomponline]

@markylaing this is the PR I mentioned earlier wrt to doing loopback client requests, not @kadinsayani PR in the end. Apologies for confusion.

[markylaing]

Wow this is a headache to read but I think I understand the reasoning. We want to use existing code to perform the deletion and not to reimplement any logic in the deletion handlers. We still want the handlers to perform all of their internal logic (e.g. forwarding storage volume/bucket requests to the member containing the volume and so on).

This is an example of where it would have been useful to have a design pattern in out API endpoints (such as MVC) where request parsing is separated from business logic.

As it stands I think this can be improved in the following ways:

1. Get the project used by list.
2. Parse this list entries with entity.ParseURL and categorise them.
3. For each entity type, iterate over the entities, and create a *http.Request and call the appropriate handler with it.

We should also consider the features configuration on the project here. Force deleting a project should delete any networks in the default project.

[markylaing]

@boltmark also could you please add some tests for this. Thanks :)

[boltmark]

Hi @tomponline, looping you into this question I had --

I'm facing a bit of an issue invoking deletion endpoints locally with dummy requests. Mainly, we currently seem to rely on everything going through the HTTP request routing/handling since we need mux to inject variables into the request context. For example, we expect to be able to access these mux vars here: https://github.com/canonical/lxd/blob/main/lxd/network_acls.go#L311.

I'm able to inject mux vars into the dummy requests via mux.SetURLVars, but this function is pretty explicit about only being there for testing purposes. Am I missing something here? If not and this is in fact an issue, is some sort of refactoring of endpoints worth it for this?

CC @markylaing

[markylaing]

Hi @tomponline, looping you into this question I had --

I'm facing a bit of an issue invoking deletion endpoints locally with dummy requests. Mainly, we currently seem to rely on everything going through the HTTP request routing/handling since we need mux to inject variables into the request context. For example, we expect to be able to access these mux vars here: https://github.com/canonical/lxd/blob/main/lxd/network_acls.go#L311.

I'm able to inject mux vars into the dummy requests via mux.SetURLVars, but this function is pretty explicit about only being there for testing purposes. Am I missing something here? If not and this is in fact an issue, is some sort of refactoring of endpoints worth it for this?

CC @markylaing

Hi @boltmark, sorry it's taken a while to get back to you on this one.

I agree that we shouldn't be using mux.SetURLVars.

I think there are two options:

1. We have a pattern of setting request details in the request context to avoid parsing data or calling the database more than once. For example, when deleting a certificate, we need to expand the certificate fingerprint before we perform the access check. The access handler sets the database entry for the certificate in the request context so that it is available in the main handler without re-querying the DB. We could adopt this pattern in all DELETE handlers such that all required information is in the request context. The project force delete code path could then set this information before calling the handlers.
2. Refactor all required DELETE handlers such that they get the required mux vars and query parameters (such as target) and pass them in to another function that actually performs the deletion and returns an api.StatusError. Then in the force delete code path, call those functions rather than the handlers directly.

I think it's worth getting @tomponline thoughts on this before doing any refactoring work though.

[tomponline]

Hi @tomponline, looping you into this question I had --
I'm facing a bit of an issue invoking deletion endpoints locally with dummy requests. Mainly, we currently seem to rely on everything going through the HTTP request routing/handling since we need mux to inject variables into the request context. For example, we expect to be able to access these mux vars here: https://github.com/canonical/lxd/blob/main/lxd/network_acls.go#L311.
I'm able to inject mux vars into the dummy requests via mux.SetURLVars, but this function is pretty explicit about only being there for testing purposes. Am I missing something here? If not and this is in fact an issue, is some sort of refactoring of endpoints worth it for this?
CC @markylaing

Hi @boltmark, sorry it's taken a while to get back to you on this one.

I agree that we shouldn't be using mux.SetURLVars.

I think there are two options:

1. We have a pattern of setting request details in the request context to avoid parsing data or calling the database more than once. For example, when deleting a certificate, we need to expand the certificate fingerprint before we perform the access check. The access handler sets the database entry for the certificate in the request context so that it is available in the main handler without re-querying the DB. We could adopt this pattern in all DELETE handlers such that all required information is in the request context. The project force delete code path could then set this information before calling the handlers.

2. Refactor all required `DELETE` handlers such that they get the required mux vars and query parameters (such as `target`) and pass them in to another function that actually performs the deletion and returns an `api.StatusError`. Then in the force delete code path, call those functions rather than the handlers directly.

I think it's worth getting @tomponline thoughts on this before doing any refactoring work though.

I dont think we should be bundling everything into the request context, especially stuff thats not altered from the request itself. Is that what you were meaning for 1?

It sounds like a combination of 1 (to reduce DB queries if needed) and 2 is the way to go.
But we should check whether we should be spending more time on this now or whether to park this or land as-is.

@escabo what would you like us to do with this one?

[markylaing]

I dont think we should be bundling everything into the request context, especially stuff thats not altered from the request itself. Is that what you were meaning for 1?

Not exactly, I was thinking to only add required information per-handler call. So rather than setting in the context of the incoming request, passing request.WithContext(<deletion args>) into the call to the appropriate handler.

As you say though, a combination of 1 and 2 is likely needed.

But we should check whether we should be spending more time on this now or whether to park this or land as-is.

I don't think we should land as is. We should at least iterate over the project used-by list and check that the caller has permission to delete the resource before calling back out via unix socket (to ourselves) to perform the deletion. Also, we should have a discussion as to whether we should ignore the security.protection.delete configuration when force deleting. Alternatively, we can add a new entitlement for this: can_force_delete on project.

[tomponline]

@markylaing thanks.

@escabo this cherry-pick PR has escalated, is it OK to continue or shall we park for now?

[escabo]

Sorry I missed the first ping, l have added this topic to next week's work session.

…

On Tue, Dec 10, 2024 at 6:01 AM Tom Parrott ***@***.***> wrote: @markylaing <https://github.com/markylaing> thanks. @escabo <https://github.com/escabo> this cherry-pick PR has escalated, is it OK to continue or shall we park for now? — Reply to this email directly, view it on GitHub <#14343 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHYR6DWWJUQAA57DBLDF3K32E3CYLAVCNFSM6AAAAABQSKTUF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZRGI2DIMRRGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>