Skip to content

[nexus] Safer project deletion #2024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Dec 19, 2022
Merged

[nexus] Safer project deletion #2024

merged 22 commits into from
Dec 19, 2022

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Dec 7, 2022

Previously

Project deletion was very problematic - even though projects contain many resources, they could be deleted while their children objects still exist. This would effectively "orphan" those resources, making them inaccessible.

For example: Create an instance in a project, delete the project. Now the instance exists, but cannot be deleted.

This PR

Adds rcgen to projects, and ensures that they can no longer be deleted while containing child resources.

  • Implements DatastoreCollectionConfig<ChildResource> for Project, for many child resources
  • Uses Project::insert_resource to bump rcgen while creating child resources
  • Checks for child resources existing during project deletion
  • Adds tests

Areas worth a particular close review:

  • Coping with the default VPC. When projects are created, they are initialized with a default VPC named default, as described in RFD 21. This default VPC may additionally contain subnets - by default, a subnet named default is also created, but more may exist. At the time that we delete a project, we need to either delete these resources ourselves, or verify that they have been deleted by the user.
    • This PR currently explicitly deletes the default VPC / Subnet from the client-side, but I'm open towards performing that deletion automatically within the "delete project" request. However, it would be difficult to avoid doing so in a transaction - presumably if we succeed in deleting the default VPC, but fail in deleting the project, we'd want to "undo" deletion of the VPC.
    • Reading Implement Project-level defaults #1015 , it seems like users will have explicit control over the creation of the default VPC -- this makes me lean slightly more towards "deletion of the default VPC must be done manually; it's just another VPC".
  • IP Pool Ranges and External IPs. These get special treatment:
    • IP pool ranges should only exist within IP pools, so they are not treated as "direct children of projects". VPC subnets are also like this, but IP pool ranges contain a direct reference to project_id, which makes this seem a little quirkier to me.
    • Floating external IPs are not implemented (see: Implement Floating IPs #1334), so we cannot fully implement them safely bumping rcgen on creation. Regardless, we do check for their absence when deleting projects.

Before taking out of draft:

  • Deal with the default VPC in projects never being deleted
  • Fix the end-to-end test, which seems broken by this: 6400f95
  • Add tests for projects still containing these resources
    • Add external IP test: bef880a
    • See if we can stop the instance during creation; that would simplify testing: 7605115
  • Implement (or check if it's necessary?) the rcgen bump on ExternalIp creation: bef880a

Fixes #1482

Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding me to the review! I spent quite a bit of time looking at this and it was really helpful to me. I've put in a few comments where I had further questions but overall this makes sense to me.

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@smklein
Copy link
Collaborator Author

smklein commented Dec 15, 2022

I think I've addressed all comments requesting changes - if y'all have a chance, I'd appreciate another look

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the authz issues!

Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though I'm more of a learner than anything else here!

@smklein smklein enabled auto-merge (squash) December 19, 2022 18:41
@smklein smklein merged commit 6cf311b into main Dec 19, 2022
@smklein smklein deleted the project-deletion-safety branch December 19, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not delete Projects with child resources
3 participants