Improve Cascading Cleanup (#78)

Servers need to be deleted before networks, networks before identities etc. Kubernetes provides this via cascading foregroud deletion and blocking owner deletion. This basically removes a bunch of code where we were doing this ourselves. While it works via the API, be damn sure you use `kubectl delete --cascade=foreground` via the CLI if ever needed. Implements #75
unikorn-cloud · Nov 27, 2024 · 7ccbe68 · 7ccbe68
1 parent b0a2bcf
commit 7ccbe68
Show file tree

Hide file tree

Showing 11 changed files with 123 additions and 248 deletions.
diff --git a/charts/region/templates/identity-controller/clusterrole.yaml b/charts/region/templates/identity-controller/clusterrole.yaml
@@ -31,18 +31,13 @@ rules:
   - create
   - update
   - delete
-# Cascading deletion.
 - apiGroups:
   - region.unikorn-cloud.org
   resources:
   - quotas
-  - networks
-  - securitygroups
-  - servers
   verbs:
   - list
   - watch
-  - delete
 - apiGroups:
   - ""
   resources:

diff --git a/charts/region/templates/security-group-controller/clusterrole.yaml b/charts/region/templates/security-group-controller/clusterrole.yaml
@@ -39,14 +39,6 @@ rules:
   - create
   - update
   - delete
-- apiGroups:
-  - region.unikorn-cloud.org
-  resources:
-  - securitygrouprules
-  verbs:
-  - list
-  - watch
-  - delete
 - apiGroups:
   - ""
   resources:

diff --git a/pkg/handler/handler.go b/pkg/handler/handler.go
@@ -46,13 +46,20 @@ import (
 	"github.com/unikorn-cloud/region/pkg/providers"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
+var (
+	foregroundDeleteOptions = &client.DeleteOptions{
+		PropagationPolicy: ptr.To(metav1.DeletePropagationForeground),
+	}
+)
+
 type Handler struct {
 	// client gives cached access to Kubernetes.
 	client client.Client
@@ -109,7 +116,7 @@ func (h *Handler) getNetwork(ctx context.Context, id string) (*unikornv1.Network
 			return nil, errors.HTTPNotFound().WithError(err)
 		}
 
-		return nil, errors.OAuth2ServerError("unable to physical network identity").WithError(err)
+		return nil, errors.OAuth2ServerError("unable to network identity").WithError(err)
 	}
 
 	return resource, nil
@@ -493,7 +500,7 @@ func (h *Handler) DeleteApiV1OrganizationsOrganizationIDProjectsProjectIDIdentit
 		return
 	}
 
-	if err := h.client.Delete(r.Context(), identity); err != nil {
+	if err := h.client.Delete(r.Context(), identity, foregroundDeleteOptions); err != nil {
 		if kerrors.IsNotFound(err) {
 			errors.HandleError(w, r, errors.HTTPNotFound().WithError(err))
 			return
@@ -579,7 +586,7 @@ func (h *Handler) GetApiV1OrganizationsOrganizationIDNetworks(w http.ResponseWri
 	}
 
 	if err := h.client.List(r.Context(), &result, options); err != nil {
-		errors.HandleError(w, r, errors.OAuth2ServerError("unable to list physical networks").WithError(err))
+		errors.HandleError(w, r, errors.OAuth2ServerError("unable to list networks").WithError(err))
 		return
 	}
 
@@ -650,8 +657,14 @@ func (h *Handler) PostApiV1OrganizationsOrganizationIDProjectsProjectIDIdentitie
 		network.Spec.Tags = generateTagList(request.Spec.Tags)
 	}
 
+	// The resource belongs to its identity, for cascading deletion.
+	if err := controllerutil.SetOwnerReference(identity, network, h.client.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil {
+		errors.HandleError(w, r, errors.OAuth2ServerError("unable to set resource owner").WithError(err))
+		return
+	}
+
 	if err := h.client.Create(r.Context(), network); err != nil {
-		errors.HandleError(w, r, errors.OAuth2ServerError("unable to create physical network").WithError(err))
+		errors.HandleError(w, r, errors.OAuth2ServerError("unable to create network").WithError(err))
 		return
 	}
 
@@ -685,13 +698,13 @@ func (h *Handler) DeleteApiV1OrganizationsOrganizationIDProjectsProjectIDIdentit
 		return
 	}
 
-	if err := h.client.Delete(r.Context(), resource); err != nil {
+	if err := h.client.Delete(r.Context(), resource, foregroundDeleteOptions); err != nil {
 		if kerrors.IsNotFound(err) {
 			errors.HandleError(w, r, errors.HTTPNotFound().WithError(err))
 			return
 		}
 
-		errors.HandleError(w, r, errors.OAuth2ServerError("unable to delete physical network").WithError(err))
+		errors.HandleError(w, r, errors.OAuth2ServerError("unable to delete network").WithError(err))
 		return
 	}
 
@@ -819,7 +832,7 @@ func (h *Handler) generateQuota(ctx context.Context, organizationID, projectID s
 
 	// Ensure the quota is owned by the identity so it is automatically cleaned
 	// up on identity deletion.
-	if err := controllerutil.SetOwnerReference(identity, resource, h.client.Scheme()); err != nil {
+	if err := controllerutil.SetOwnerReference(identity, resource, h.client.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil {
 		return nil, err
 	}
 
@@ -1006,7 +1019,7 @@ func (h *Handler) generateSecurityGroup(ctx context.Context, organizationID, pro
 
 	// Ensure the security is owned by the identity so it is automatically cleaned
 	// up on identity deletion.
-	if err := controllerutil.SetOwnerReference(identity, resource, h.client.Scheme()); err != nil {
+	if err := controllerutil.SetOwnerReference(identity, resource, h.client.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil {
 		return nil, err
 	}
 
@@ -1076,7 +1089,7 @@ func (h *Handler) DeleteApiV1OrganizationsOrganizationIDProjectsProjectIDIdentit
 		return
 	}
 
-	if err := h.client.Delete(r.Context(), resource); err != nil {
+	if err := h.client.Delete(r.Context(), resource, foregroundDeleteOptions); err != nil {
 		if kerrors.IsNotFound(err) {
 			errors.HandleError(w, r, errors.HTTPNotFound().WithError(err))
 			return
@@ -1313,7 +1326,7 @@ func (h *Handler) generateSecurityGroupRule(ctx context.Context, organizationID,
 
 	// Ensure the security is owned by the security group so it is automatically cleaned
 	// up on security group deletion.
-	if err := controllerutil.SetOwnerReference(securityGroup, resource, h.client.Scheme()); err != nil {
+	if err := controllerutil.SetOwnerReference(securityGroup, resource, h.client.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil {
 		return nil, err
 	}
 
@@ -1456,7 +1469,14 @@ func (h *Handler) PostApiV1OrganizationsOrganizationIDProjectsProjectIDIdentitie
 		return
 	}
 
-	result, err := server.NewClient(h.client, h.namespace).Create(r.Context(), organizationID, projectID, identity, request)
+	// NOTE: exactly 1 is enforced at the API schema level.
+	network, err := h.getNetwork(r.Context(), request.Spec.Networks[0].Id)
+	if err != nil {
+		errors.HandleError(w, r, err)
+		return
+	}
+
+	result, err := server.NewClient(h.client, h.namespace).Create(r.Context(), organizationID, projectID, identity, network, request)
 	if err != nil {
 		errors.HandleError(w, r, err)
 		return

diff --git a/pkg/handler/server/client.go b/pkg/handler/server/client.go
@@ -68,9 +68,9 @@ func (c *Client) List(ctx context.Context, organizationID string) (openapi.Serve
 	return convertList(result), nil
 }
 
-func (c *Client) Create(ctx context.Context, organizationID, projectID string, identity *unikornv1.Identity, request *openapi.ServerWrite) (*openapi.ServerRead, error) {
+func (c *Client) Create(ctx context.Context, organizationID, projectID string, identity *unikornv1.Identity, network *unikornv1.Network, request *openapi.ServerWrite) (*openapi.ServerRead, error) {
 
-	resource, err := newGenerator(c.client, c.namespace, organizationID, projectID, identity).generate(ctx, request)
+	resource, err := newGenerator(c.client, c.namespace, organizationID, projectID, identity, network).generate(ctx, request)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/handler/server/conversion.go b/pkg/handler/server/conversion.go
@@ -182,15 +182,18 @@ type generator struct {
 	projectID string
 	// identity is the identity the resource is provisioned for.
 	identity *unikornv1.Identity
+	// network is the network tha resource is attacked to.
+	network *unikornv1.Network
 }
 
-func newGenerator(client client.Client, namespace, organizationID, projectID string, identity *unikornv1.Identity) *generator {
+func newGenerator(client client.Client, namespace, organizationID, projectID string, identity *unikornv1.Identity, network *unikornv1.Network) *generator {
 	return &generator{
 		client:         client,
 		namespace:      namespace,
 		organizationID: organizationID,
 		projectID:      projectID,
 		identity:       identity,
+		network:        network,
 	}
 }
 
@@ -214,9 +217,9 @@ func (g *generator) generate(ctx context.Context, in *openapi.ServerWrite) (*uni
 		},
 	}
 
-	// Ensure the server is owned by the identity so it is automatically cleaned
-	// up on identity deletion.
-	if err := controllerutil.SetOwnerReference(g.identity, resource, g.client.Scheme()); err != nil {
+	// Ensure the server is owned by the network so it is automatically cleaned
+	// up on cascading deletion.
+	if err := controllerutil.SetOwnerReference(g.network, resource, g.client.Scheme(), controllerutil.WithBlockOwnerDeletion(true)); err != nil {
 		return nil, err
 	}
 

diff --git a/pkg/openapi/schema.go b/pkg/openapi/schema.go
diff --git a/pkg/openapi/server.spec.yaml b/pkg/openapi/server.spec.yaml
@@ -1217,6 +1217,8 @@ components:
       description: A list of networks.
       type: array
       minItems: 1
+      # NOTE: this is pinned due to cascading deletion semantics, for now.
+      maxItems: 1
       items:
         $ref: '#/components/schemas/serverNetwork'
     serverNetwork:

diff --git a/pkg/providers/openstack/provider.go b/pkg/providers/openstack/provider.go
@@ -799,6 +799,11 @@ func (p *Provider) DeleteIdentity(ctx context.Context, identity *unikornv1.Ident
 
 	defer record()
 
+	// User never even created, so nothing else will have been.
+	if openstackIdentity.Spec.UserID == nil {
+		return nil
+	}
+
 	// Rescope to the user/project...
 	providerClient := NewPasswordProvider(p.region.Spec.Openstack.Endpoint, *openstackIdentity.Spec.UserID, *openstackIdentity.Spec.Password, *openstackIdentity.Spec.ProjectID)