Move service and secret cleanup to ownerReferences to simplify the

deletion of teams (and reduce permissions requried)

Author: J12934

balancer/routes/adminDeleteInstance.go

@@ -18,23 +18,14 @@ func handleAdminDeleteInstance(bundle *bundle.Bundle) http.Handler {
 				return
 			}
 
+			// Only the deployment needs to be deleted explicitly.
+			// The service and secret are owned by the deployment via OwnerReferences and will be garbage collected by Kubernetes.
 			err := bundle.ClientSet.AppsV1().Deployments(bundle.RuntimeEnvironment.Namespace).Delete(req.Context(), fmt.Sprintf("juiceshop-%s", teamToDelete), metav1.DeleteOptions{})
 			if err != nil && !errors.IsNotFound(err) {
 				bundle.Log.Printf("Failed to delete deployment for team '%s': %s", teamToDelete, err)
 				http.Error(responseWriter, "", http.StatusInternalServerError)
 				return
 			}
-			err = bundle.ClientSet.CoreV1().Services(bundle.RuntimeEnvironment.Namespace).Delete(req.Context(), fmt.Sprintf("juiceshop-%s", teamToDelete), metav1.DeleteOptions{})
-			if err != nil && !errors.IsNotFound(err) {
-				bundle.Log.Printf("Failed to delete service for team '%s': %s", teamToDelete, err)
-				http.Error(responseWriter, "", http.StatusInternalServerError)
-				return
-			}
-
-			err = bundle.ClientSet.CoreV1().Secrets(bundle.RuntimeEnvironment.Namespace).Delete(req.Context(), fmt.Sprintf("juiceshop-%s", teamToDelete), metav1.DeleteOptions{})
-			if err != nil && !errors.IsNotFound(err) {
-				bundle.Log.Printf("Failed to delete secret for team '%s': %s", teamToDelete, err)
-			}
 
 			responseWriter.WriteHeader(http.StatusOK)
 			responseWriter.Write([]byte{}) // nosemgrep: go.lang.security.audit.xss.no-direct-write-to-responsewriter.no-direct-write-to-responsewriter

balancer/routes/adminDeleteInstance_test.go

@@ -10,7 +10,6 @@ import (
 	"github.com/juice-shop/multi-juicer/balancer/pkg/testutil"
 	"github.com/stretchr/testify/assert"
 	appsv1 "k8s.io/api/apps/v1"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes/fake"
@@ -40,35 +39,14 @@ func TestAdminDeleteInstanceHandler(t *testing.T) {
 			},
 		}
 	}
-	createServiceForTeam := func(team string) *corev1.Service {
-		return &corev1.Service{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      fmt.Sprintf("juiceshop-%s", team),
-				Namespace: "test-namespace",
-				Labels: map[string]string{
-					"app.kubernetes.io/name":    "juice-shop",
-					"app.kubernetes.io/part-of": "multi-juicer",
-					"team":                      team,
-				},
-			},
-			Spec: corev1.ServiceSpec{
-				Ports: []corev1.ServicePort{
-					{
-						Port: 3000,
-					},
-				},
-			},
-		}
-	}
-
 	t.Run("deleting instances requires admin login", func(t *testing.T) {
 		req, _ := http.NewRequest("DELETE", "/balancer/api/admin/teams/foobar/delete", nil)
 		req.Header.Set("Cookie", fmt.Sprintf("team=%s", testutil.SignTestTeamname("some team")))
 		rr := httptest.NewRecorder()
 
 		server := http.NewServeMux()
 
-		clientset := fake.NewClientset(createDeploymentForTeam("foobar"), createServiceForTeam("foobar"))
+		clientset := fake.NewClientset(createDeploymentForTeam("foobar"))
 		bundle := testutil.NewTestBundleWithCustomFakeClient(clientset)
 		AddRoutes(server, bundle)
 
@@ -85,7 +63,7 @@ func TestAdminDeleteInstanceHandler(t *testing.T) {
 
 		server := http.NewServeMux()
 
-		clientset := fake.NewClientset(createDeploymentForTeam("foobar"), createServiceForTeam("foobar"))
+		clientset := fake.NewClientset(createDeploymentForTeam("foobar"))
 		bundle := testutil.NewTestBundleWithCustomFakeClient(clientset)
 		AddRoutes(server, bundle)
 
@@ -94,7 +72,7 @@ func TestAdminDeleteInstanceHandler(t *testing.T) {
 		assert.Equal(t, http.StatusBadRequest, rr.Code)
 	})
 
-	t.Run("deletes both deployments and services of teams", func(t *testing.T) {
+	t.Run("deletes deployment of team", func(t *testing.T) {
 		req, _ := http.NewRequest("DELETE", "/balancer/api/admin/teams/foobar/delete", nil)
 		req.Header.Set("Cookie", fmt.Sprintf("team=%s", testutil.SignTestTeamname("admin")))
 		rr := httptest.NewRecorder()
@@ -103,9 +81,7 @@ func TestAdminDeleteInstanceHandler(t *testing.T) {
 
 		clientset := fake.NewClientset(
 			createDeploymentForTeam("foobar"),
-			createServiceForTeam("foobar"),
 			createDeploymentForTeam("other-team"),
-			createServiceForTeam("other-team"),
 		)
 		bundle := testutil.NewTestBundleWithCustomFakeClient(clientset)
 		AddRoutes(server, bundle)
@@ -119,16 +95,10 @@ func TestAdminDeleteInstanceHandler(t *testing.T) {
 
 		assert.Equal(t, "delete", actions[0].GetVerb())
 		assert.Equal(t, schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, actions[0].GetResource())
-		assert.Equal(t, "delete", actions[1].GetVerb())
-		assert.Equal(t, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "services"}, actions[1].GetResource())
 
 		deployments, err := clientset.AppsV1().Deployments("test-namespace").List(context.Background(), metav1.ListOptions{})
 		assert.Nil(t, err)
 		assert.Len(t, deployments.Items, 1)
-
-		services, err := clientset.CoreV1().Services("test-namespace").List(context.Background(), metav1.ListOptions{})
-		assert.Nil(t, err)
-		assert.Len(t, services.Items, 1)
 	})
 
 }

balancer/routes/join.go

@@ -145,23 +145,23 @@ func createANewTeam(context context.Context, bundle *bundle.Bundle, team string,
 		return
 	}
 
+	deployment, err := createDeploymentForTeam(context, bundle, team, passcodeHash)
+	if err != nil {
+		bundle.Log.Printf("Failed to create deployment: %s", err)
+		http.Error(w, "failed to create deployment", http.StatusInternalServerError)
+		return
+	}
+
 	if bundle.Config.JuiceShopConfig.LLM.Enabled {
-		err = createLLMTokenSecretForTeam(context, bundle, team)
+		err = createLLMTokenSecretForTeam(context, bundle, team, deployment)
 		if err != nil {
 			bundle.Log.Printf("Failed to create LLM token secret: %s", err)
 			http.Error(w, "failed to create LLM token secret", http.StatusInternalServerError)
 			return
 		}
 	}
 
-	err = createDeploymentForTeam(context, bundle, team, passcodeHash)
-	if err != nil {
-		bundle.Log.Printf("Failed to create deployment: %s", err)
-		http.Error(w, "failed to create deployment", http.StatusInternalServerError)
-		return
-	}
-
-	err = createServiceForTeam(context, bundle, team)
+	err = createServiceForTeam(context, bundle, team, deployment)
 	if err != nil {
 		bundle.Log.Printf("Failed to create service: %s", err)
 		http.Error(w, "failed to create service", http.StatusInternalServerError)
@@ -272,7 +272,21 @@ func writeUnauthorizedResponse(responseWriter http.ResponseWriter) {
 	responseWriter.Write(errorResponseBody) // nosemgrep: go.lang.security.audit.xss.no-direct-write-to-responsewriter.no-direct-write-to-responsewriter
 }
 
-// uid of the balancer kubernetes deployment resource. used to "attach" created juice shop deployments and services to the balancer deployment so that they get deleted when the balancer gets deleted
+func getDeploymentOwnerReferences(deployment *appsv1.Deployment) []metav1.OwnerReference {
+	truePointer := true
+	return []metav1.OwnerReference{
+		{
+			APIVersion:         "apps/v1",
+			Kind:               "Deployment",
+			Name:               deployment.Name,
+			UID:                deployment.UID,
+			Controller:         &truePointer,
+			BlockOwnerDeletion: &truePointer,
+		},
+	}
+}
+
+// uid of the balancer kubernetes deployment resource. used to "attach" created juice shop deployments to the balancer deployment so that they get deleted when the balancer gets deleted
 var deploymentUid types.UID
 
 func getOwnerReferences(context context.Context, bundle *bundle.Bundle) ([]metav1.OwnerReference, error) {
@@ -302,10 +316,10 @@ func getOwnerReferences(context context.Context, bundle *bundle.Bundle) ([]metav
 	return ownerReferences, nil
 }
 
-func createDeploymentForTeam(context context.Context, bundle *bundle.Bundle, team string, passcodeHash string) error {
+func createDeploymentForTeam(context context.Context, bundle *bundle.Bundle, team string, passcodeHash string) (*appsv1.Deployment, error) {
 	ownerReferences, err := getOwnerReferences(context, bundle)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	podLabels := map[string]string{}
@@ -433,16 +447,11 @@ func createDeploymentForTeam(context context.Context, bundle *bundle.Bundle, tea
 		},
 	}
 
-	_, err = bundle.ClientSet.AppsV1().Deployments(bundle.RuntimeEnvironment.Namespace).Create(context, deployment, metav1.CreateOptions{})
-	return err
+	created, err := bundle.ClientSet.AppsV1().Deployments(bundle.RuntimeEnvironment.Namespace).Create(context, deployment, metav1.CreateOptions{})
+	return created, err
 }
 
-func createServiceForTeam(context context.Context, bundle *bundle.Bundle, team string) error {
-	ownerReferences, err := getOwnerReferences(context, bundle)
-	if err != nil {
-		return err
-	}
-
+func createServiceForTeam(context context.Context, bundle *bundle.Bundle, team string, ownerDeployment *appsv1.Deployment) error {
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: fmt.Sprintf("juiceshop-%s", team),
@@ -454,7 +463,7 @@ func createServiceForTeam(context context.Context, bundle *bundle.Bundle, team s
 				"app.kubernetes.io/instance":  fmt.Sprintf("juice-shop-%s", team),
 				"app.kubernetes.io/part-of":   "multi-juicer",
 			},
-			OwnerReferences: ownerReferences,
+			OwnerReferences: getDeploymentOwnerReferences(ownerDeployment),
 		},
 		Spec: corev1.ServiceSpec{
 			Selector: map[string]string{
@@ -469,7 +478,7 @@ func createServiceForTeam(context context.Context, bundle *bundle.Bundle, team s
 		},
 	}
 
-	_, err = bundle.ClientSet.CoreV1().Services(bundle.RuntimeEnvironment.Namespace).Create(context, service, metav1.CreateOptions{})
+	_, err := bundle.ClientSet.CoreV1().Services(bundle.RuntimeEnvironment.Namespace).Create(context, service, metav1.CreateOptions{})
 	return err
 }
 
@@ -507,17 +516,12 @@ func buildJuiceShopEnv(bundle *bundle.Bundle, team string) []corev1.EnvVar {
 	return envVars
 }
 
-func createLLMTokenSecretForTeam(ctx context.Context, bundle *bundle.Bundle, team string) error {
+func createLLMTokenSecretForTeam(ctx context.Context, bundle *bundle.Bundle, team string, ownerDeployment *appsv1.Deployment) error {
 	token, err := signutil.Sign(team, bundle.Config.CookieConfig.SigningKey)
 	if err != nil {
 		return fmt.Errorf("failed to sign LLM token: %w", err)
 	}
 
-	ownerReferences, err := getOwnerReferences(ctx, bundle)
-	if err != nil {
-		return err
-	}
-
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: fmt.Sprintf("juiceshop-%s", team),
@@ -526,7 +530,7 @@ func createLLMTokenSecretForTeam(ctx context.Context, bundle *bundle.Bundle, tea
 				"app.kubernetes.io/component": "llm-token",
 				"app.kubernetes.io/part-of":   "multi-juicer",
 			},
-			OwnerReferences: ownerReferences,
+			OwnerReferences: getDeploymentOwnerReferences(ownerDeployment),
 		},
 		Data: map[string][]byte{
 			"token": []byte(token),

balancer/routes/join_test.go

@@ -119,8 +119,8 @@ func TestJoinHandler(t *testing.T) {
 			{
 				APIVersion:         "apps/v1",
 				Kind:               "Deployment",
-				Name:               "balancer",
-				UID:                "34c0bb8a-240b-4f2a-84ae-2eb2258298f9",
+				Name:               deployment.Name,
+				UID:                deployment.UID,
 				Controller:         &truePointer,
 				BlockOwnerDeletion: &truePointer,
 			},

cleaner/main.go

@@ -40,17 +40,15 @@ func main() {
 	cleanupSummary := runCleanup(clientset, currentTime, maxInactiveTime)
 
 	logger.Println("Finished cleaning up JuiceShop deployments.")
-	logger.Printf("Deleted %d deployment(s) and %d service(s) successfully", cleanupSummary.SuccessfulDeploymentDeletions, cleanupSummary.SuccessfulServiceDeletions)
-	if (cleanupSummary.FailedDeploymentDeletions + cleanupSummary.FailedServiceDeletions) > 0 {
-		logger.Printf("Failed to delete %d deployment(s) and %d service(s)", cleanupSummary.FailedDeploymentDeletions, cleanupSummary.FailedServiceDeletions)
+	logger.Printf("Deleted %d deployment(s) successfully", cleanupSummary.SuccessfulDeletions)
+	if cleanupSummary.FailedDeletions > 0 {
+		logger.Printf("Failed to delete %d deployment(s)", cleanupSummary.FailedDeletions)
 	}
 }
 
 type CleanupSummary struct {
-	SuccessfulDeploymentDeletions int
-	SuccessfulServiceDeletions    int
-	FailedDeploymentDeletions     int
-	FailedServiceDeletions        int
+	SuccessfulDeletions int
+	FailedDeletions     int
 }
 
 func runCleanup(clientset kubernetes.Interface, currentTime time.Time, maxInactive time.Duration) CleanupSummary {
@@ -66,12 +64,7 @@ func runCleanup(clientset kubernetes.Interface, currentTime time.Time, maxInacti
 		logger.Println("No JuiceShop deployments found. Nothing to do.")
 	}
 
-	summary := CleanupSummary{
-		SuccessfulDeploymentDeletions: 0,
-		SuccessfulServiceDeletions:    0,
-		FailedDeploymentDeletions:     0,
-		FailedServiceDeletions:        0,
-	}
+	summary := CleanupSummary{}
 
 	for _, deployment := range deployments.Items {
 		lastConnectedTimestampString, hasAnnotation := deployment.Annotations["multi-juicer.owasp-juice.shop/lastRequest"]
@@ -88,26 +81,15 @@ func runCleanup(clientset kubernetes.Interface, currentTime time.Time, maxInacti
 		name := deployment.Name
 		if currentTime.Sub(time.UnixMilli(lastConnectedTimestamp)) > maxInactive {
 			logger.Printf("Deleting instance '%s' as it has been inactive for longer than %s", name, maxInactive.String())
+			// Only the deployment needs to be deleted explicitly.
+			// The service and secret are owned by the deployment via OwnerReferences and will be garbage collected by Kubernetes.
 			err = clientset.AppsV1().Deployments(namespace).Delete(context.Background(), name, metav1.DeleteOptions{})
 			if err != nil && !errors.IsNotFound(err) {
 				logger.Printf("Failed to delete deployment %s: %v", name, err)
-				summary.FailedDeploymentDeletions++
-				continue
-			}
-			summary.SuccessfulDeploymentDeletions++
-			err = clientset.CoreV1().Services(namespace).Delete(context.Background(), name, metav1.DeleteOptions{})
-			if err != nil && !errors.IsNotFound(err) {
-				logger.Printf("Failed to delete service %s: %v", name, err)
-				summary.FailedServiceDeletions++
+				summary.FailedDeletions++
 				continue
 			}
-			summary.SuccessfulServiceDeletions++
-
-			err = clientset.CoreV1().Secrets(namespace).Delete(context.Background(), name, metav1.DeleteOptions{})
-			if err != nil && !errors.IsNotFound(err) {
-				logger.Printf("Failed to delete kubernetes secret for %s: %v", name, err)
-			}
-
+			summary.SuccessfulDeletions++
 			logger.Printf("Successfully deleted instance %s", name)
 		} else {
 			logger.Printf("Skipping deployment %s as it has been active recently", name)