gh, gar, cdn, google workspace: Consider 404 success when deleting items

Co-authored-by: Christer Edvartsen <[email protected]>

internal/reconcilers/github/team/reconciler.go

@@ -130,37 +130,35 @@ func (r *githubTeamReconciler) Reconcile(ctx context.Context, client *apiclient.
 func (r *githubTeamReconciler) Delete(ctx context.Context, client *apiclient.APIClient, naisTeam *protoapi.Team, log logrus.FieldLogger) error {
 	if naisTeam.GithubTeamSlug == nil {
 		log.Warnf("missing slug in reconciler state, unable to delete GitHub team")
-	} else {
-		resp, err := r.teamsService.DeleteTeamBySlug(ctx, r.org, *naisTeam.GithubTeamSlug)
-		if err != nil {
-			return fmt.Errorf("delete GitHub team %q for team %q: %w", *naisTeam.GithubTeamSlug, naisTeam.Slug, err)
-		}
+		return nil
+	}
+
+	resp, err := r.teamsService.DeleteTeamBySlug(ctx, r.org, *naisTeam.GithubTeamSlug)
+	if resp != nil {
 		defer resp.Body.Close()
 
-		if resp.StatusCode != http.StatusNoContent {
-			body, _ := io.ReadAll(resp.Body)
-			return fmt.Errorf("unexpected server response from GitHub: %q: %q", resp.Status, string(body))
+		if resp.StatusCode == http.StatusNotFound {
+			// If not found, we consider the team already deleted
+			return nil
 		}
 	}
-
-	_, err := client.Reconcilers().DeleteState(ctx, &protoapi.DeleteReconcilerStateRequest{
-		ReconcilerName: r.Name(),
-		TeamSlug:       naisTeam.Slug,
-	})
 	if err != nil {
-		return err
+		return fmt.Errorf("delete GitHub team %q for team %q: %w", *naisTeam.GithubTeamSlug, naisTeam.Slug, err)
 	}
 
-	if naisTeam.GithubTeamSlug != nil {
-		reconcilers.AuditLogForTeam(
-			ctx,
-			client,
-			r,
-			auditActionDeleteGitHubTeam,
-			naisTeam.Slug,
-			"Deleted GitHub team with slug %q", *naisTeam.GithubTeamSlug,
-		)
+	if resp.StatusCode != http.StatusNoContent {
+		body, _ := io.ReadAll(resp.Body)
+		return fmt.Errorf("unexpected server response from GitHub: %q: %q", resp.Status, string(body))
 	}
+
+	reconcilers.AuditLogForTeam(
+		ctx,
+		client,
+		r,
+		auditActionDeleteGitHubTeam,
+		naisTeam.Slug,
+		"Deleted GitHub team with slug %q", *naisTeam.GithubTeamSlug,
+	)
 	return nil
 }
 

internal/reconcilers/github/team/reconciler_test.go

@@ -616,11 +616,7 @@ func TestGitHubReconciler_Delete(t *testing.T) {
 
 		graphClient := github_team_reconciler.NewMockGraphClient(t)
 		teamsService := github_team_reconciler.NewMockTeamsService(t)
-		apiClient, mockServer := apiclient.NewMockClient(t)
-		mockServer.Reconcilers.EXPECT().
-			DeleteState(mock.Anything, &protoapi.DeleteReconcilerStateRequest{ReconcilerName: "github:team", TeamSlug: teamSlug}).
-			Return(&protoapi.DeleteReconcilerStateResponse{}, nil).
-			Once()
+		apiClient, _ := apiclient.NewMockClient(t)
 
 		reconciler, err := github_team_reconciler.New(ctx, org, authEndpoint, serviceAccountEmail, github_team_reconciler.WithTeamsService(teamsService), github_team_reconciler.WithGraphClient(graphClient))
 		if err != nil {
@@ -709,10 +705,6 @@ func TestGitHubReconciler_Delete(t *testing.T) {
 		}
 
 		apiClient, mockServer := apiclient.NewMockClient(t)
-		mockServer.Reconcilers.EXPECT().
-			DeleteState(mock.Anything, &protoapi.DeleteReconcilerStateRequest{ReconcilerName: "github:team", TeamSlug: teamSlug}).
-			Return(&protoapi.DeleteReconcilerStateResponse{}, nil).
-			Once()
 		mockServer.AuditLogs.EXPECT().
 			Create(mock.Anything, mock.MatchedBy(func(req *protoapi.CreateAuditLogsRequest) bool {
 				return req.Action == "github:team:delete"

internal/reconcilers/google/cdn/reconciler.go

@@ -227,53 +227,13 @@ func (r *cdnReconciler) Reconcile(ctx context.Context, client *apiclient.APIClie
 
 func (r *cdnReconciler) Delete(ctx context.Context, client *apiclient.APIClient, naisTeam *protoapi.Team, log logrus.FieldLogger) error {
 	// reverse order of creation
-	bucketName := r.bucketName(naisTeam)
-
-	// get backendbucket
-	backendBucket, err := r.services.BackendBuckets.Get(ctx, &computepb.GetBackendBucketRequest{
-		BackendBucket: bucketName,
-		Project:       r.googleManagementProjectID,
-	})
-	if err != nil {
-		return fmt.Errorf("get backend bucket, %w", err)
-	}
-
-	// remove entry from urlmap
-	urlMap, err := r.services.UrlMap.Get(r.googleManagementProjectID, urlMapName).Do()
-	if err != nil {
-		return fmt.Errorf("get urlmap: %w", err)
-	}
-
-	for _, pm := range urlMap.PathMatchers {
-		seen := make(map[string]bool)
-		for _, pr := range pm.PathRules {
-			if !seen[pr.Service] {
-				seen[pr.Service] = true
-			}
-		}
 
-		pm.PathRules = slices.DeleteFunc(pm.PathRules, func(rule *compute.PathRule) bool {
-			return rule.Service == *backendBucket.SelfLink
-		})
-	}
-
-	_, err = r.services.UrlMap.Update(r.googleManagementProjectID, urlMapName, urlMap).Do()
-	if err != nil {
-		return fmt.Errorf("update urlMap: %w", err)
-	}
-
-	log.Infof("removed path rule for %q from url map %q", naisTeam.Slug, urlMapName)
+	bucketName := r.bucketName(naisTeam)
 
 	// delete backendbucket
-	if backendBucket != nil {
-		_, err = r.services.BackendBuckets.Delete(ctx, &computepb.DeleteBackendBucketRequest{
-			BackendBucket: bucketName,
-			Project:       r.googleManagementProjectID,
-		})
-		if err != nil {
-			return fmt.Errorf("delete backend bucket: %w", err)
-		}
-		log.Infof("deleted backend bucket %q", *backendBucket.Name)
+	shouldAudit, err := r.deleteBackendBucket(ctx, bucketName, naisTeam, log)
+	if err != nil {
+		return err
 	}
 
 	// remove bucket
@@ -313,6 +273,7 @@ func (r *cdnReconciler) Delete(ctx context.Context, client *apiclient.APIClient,
 			binding.Members = slices.DeleteFunc(binding.Members, func(member string) bool {
 				return member == fmt.Sprintf("serviceAccount:%s", serviceAccount.Email)
 			})
+			shouldAudit = true
 		}
 	}
 
@@ -327,23 +288,83 @@ func (r *cdnReconciler) Delete(ctx context.Context, client *apiclient.APIClient,
 	// delete service account
 	_, err = r.services.Iam.Projects.ServiceAccounts.Delete(serviceAccountName).Context(ctx).Do()
 	if err != nil {
-		return fmt.Errorf("delete service account: %w", err)
+		googleError, ok := err.(*googleapi.Error)
+		if !ok || googleError.Code != http.StatusNotFound {
+			return fmt.Errorf("delete service account: %w", err)
+		}
+	} else {
+		shouldAudit = true
 	}
 	log.Infof("deleted service account %q", serviceAccount.Email)
 
 	log.Infof("deleted cdn resources for team %q", naisTeam.Slug)
 
-	reconcilers.AuditLogForTeam(
-		ctx,
-		client,
-		r,
-		auditActionDeletedCdn,
-		naisTeam.Slug,
-		"Deleted CDN resources for %s", naisTeam.Slug,
-	)
+	if shouldAudit {
+		reconcilers.AuditLogForTeam(
+			ctx,
+			client,
+			r,
+			auditActionDeletedCdn,
+			naisTeam.Slug,
+			"Deleted CDN resources for %s", naisTeam.Slug,
+		)
+	}
 	return nil
 }
 
+func (r *cdnReconciler) deleteBackendBucket(ctx context.Context, bucketName string, naisTeam *protoapi.Team, log logrus.FieldLogger) (shouldAudit bool, err error) {
+	// get backendbucket
+	backendBucket, err := r.services.BackendBuckets.Get(ctx, &computepb.GetBackendBucketRequest{
+		BackendBucket: bucketName,
+		Project:       r.googleManagementProjectID,
+	})
+	if err != nil {
+		googleError, ok := err.(*googleapi.Error)
+		if !ok || googleError.Code != http.StatusNotFound {
+			return false, fmt.Errorf("get backend bucket, %w", err)
+		}
+		return false, nil
+	}
+
+	// remove entry from urlmap
+	urlMap, err := r.services.UrlMap.Get(r.googleManagementProjectID, urlMapName).Do()
+	if err != nil {
+		return false, fmt.Errorf("get urlmap: %w", err)
+	}
+
+	for _, pm := range urlMap.PathMatchers {
+		seen := make(map[string]bool)
+		for _, pr := range pm.PathRules {
+			if !seen[pr.Service] {
+				seen[pr.Service] = true
+			}
+		}
+
+		pm.PathRules = slices.DeleteFunc(pm.PathRules, func(rule *compute.PathRule) bool {
+			return rule.Service == *backendBucket.SelfLink
+		})
+	}
+
+	_, err = r.services.UrlMap.Update(r.googleManagementProjectID, urlMapName, urlMap).Do()
+	if err != nil {
+		return false, fmt.Errorf("update urlMap: %w", err)
+	}
+
+	log.Infof("removed path rule for %q from url map %q", naisTeam.Slug, urlMapName)
+
+	// delete backendbucket
+	_, err = r.services.BackendBuckets.Delete(ctx, &computepb.DeleteBackendBucketRequest{
+		BackendBucket: bucketName,
+		Project:       r.googleManagementProjectID,
+	})
+	if err != nil {
+		return false, fmt.Errorf("delete backend bucket: %w", err)
+	}
+
+	log.Infof("deleted backend bucket %q", *backendBucket.Name)
+	return true, nil
+}
+
 // bucketName returns a globally unique bucket name for the given team
 func (r *cdnReconciler) bucketName(naisTeam *protoapi.Team) string {
 	tenantTeamName := fmt.Sprintf("%s-%s", strings.ReplaceAll(r.tenantName, ".", "-"), naisTeam.Slug)

internal/reconcilers/google/gar/reconciler.go

@@ -179,6 +179,10 @@ func (r *garReconciler) Delete(ctx context.Context, client *apiclient.APIClient,
 	}
 	operation, err := r.artifactRegistry.DeleteRepository(ctx, req)
 	if err != nil {
+		googleError, ok := err.(*googleapi.Error)
+		if ok && googleError.Code == http.StatusNotFound {
+			return nil
+		}
 		return fmt.Errorf("delete GAR repository for team %q: %w", naisTeam.Slug, err)
 	}
 

internal/reconcilers/google/workspace_admin/reconciler.go

@@ -114,28 +114,26 @@ func (r *googleWorkspaceAdminReconciler) Reconcile(ctx context.Context, client *
 func (r *googleWorkspaceAdminReconciler) Delete(ctx context.Context, client *apiclient.APIClient, naisTeam *protoapi.Team, log logrus.FieldLogger) error {
 	if naisTeam.GoogleGroupEmail == nil {
 		log.Warnf("missing group email in team, assume team has already been deleted")
-	} else if err := r.adminDirectoryService.Groups.Delete(*naisTeam.GoogleGroupEmail).Context(ctx).Do(); err != nil {
-		return fmt.Errorf("delete Google directory group with email %q for team %q: %w", *naisTeam.GoogleGroupEmail, naisTeam.Slug, err)
+		return nil
 	}
 
-	_, err := client.Reconcilers().DeleteState(ctx, &protoapi.DeleteReconcilerStateRequest{
-		ReconcilerName: r.Name(),
-		TeamSlug:       naisTeam.Slug,
-	})
-	if err != nil {
-		return err
+	if err := r.adminDirectoryService.Groups.Delete(*naisTeam.GoogleGroupEmail).Context(ctx).Do(); err != nil {
+		googleError, ok := err.(*googleapi.Error)
+		if ok && googleError.Code == http.StatusNotFound {
+			// Group does not exist, assume it has already been deleted
+			return nil
+		}
+		return fmt.Errorf("delete Google directory group with email %q for team %q: %w", *naisTeam.GoogleGroupEmail, naisTeam.Slug, err)
 	}
 
-	if naisTeam.GoogleGroupEmail != nil {
-		reconcilers.AuditLogForTeam(
-			ctx,
-			client,
-			r,
-			auditActionGoogleWorkspaceAdminDelete,
-			naisTeam.Slug,
-			"Delete Google directory group with email %q", *naisTeam.GoogleGroupEmail,
-		)
-	}
+	reconcilers.AuditLogForTeam(
+		ctx,
+		client,
+		r,
+		auditActionGoogleWorkspaceAdminDelete,
+		naisTeam.Slug,
+		"Delete Google directory group with email %q", *naisTeam.GoogleGroupEmail,
+	)
 
 	return nil
 }

internal/reconcilers/google/workspace_admin/reconciler_test.go

@@ -208,11 +208,7 @@ func Test_Delete(t *testing.T) {
 			Purpose: teamPurpose,
 		}
 
-		apiClient, mockServer := apiclient.NewMockClient(t)
-		mockServer.Reconcilers.EXPECT().
-			DeleteState(mock.Anything, &protoapi.DeleteReconcilerStateRequest{TeamSlug: teamSlug, ReconcilerName: "google:workspace-admin"}).
-			Return(&protoapi.DeleteReconcilerStateResponse{}, nil).
-			Once()
+		apiClient, _ := apiclient.NewMockClient(t)
 
 		service, closer := getAdminDirectoryServiceAndClient(t, ctx, nil)
 		defer closer()
@@ -292,10 +288,6 @@ func Test_Delete(t *testing.T) {
 		defer closer()
 
 		apiClient, mockServer := apiclient.NewMockClient(t)
-		mockServer.Reconcilers.EXPECT().
-			DeleteState(mock.Anything, &protoapi.DeleteReconcilerStateRequest{TeamSlug: teamSlug, ReconcilerName: "google:workspace-admin"}).
-			Return(&protoapi.DeleteReconcilerStateResponse{}, nil).
-			Once()
 		mockServer.AuditLogs.EXPECT().
 			Create(mock.Anything, mock.MatchedBy(func(req *protoapi.CreateAuditLogsRequest) bool {
 				return req.Action == "google:workspace-admin:delete" && req.ReconcilerName == "google:workspace-admin"