Project deletion metadata audit log #4262

teodor-yanev · 2024-08-23T11:13:39Z

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
In order for us to be able to calculate customer churn, we need to keep track of the deleted projects/accounts on Minder and the relevant metadata from a business perspective.

Fixes https://app.zenhub.com/workspaces/minder-sprints-666857b34fcef725d2efba7c/issues/gh/stacklok/infra/1906

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-08-23T11:23:14Z

coverage: 53.943% (+0.2%) from 53.703%
when pulling 7324d0b on project-deletion-audit
into 0cbe5ab on main.

evankanderson

I think there are other paths which can delete projects, in particular at least https://github.com/stacklok/minder/blob/main/internal/projects/deleter.go#L70 and https://github.com/stacklok/minder/blob/main/internal/projects/deleter.go#L117.

There's a path via https://github.com/stacklok/minder/blob/main/internal/controlplane/identity_events.go#L81 where users who delete their Stacklok IDP accounts (e.g. in Keycloak) will have their resources automatically cleaned up. That seems to call the non-struct version of DeleteUser, which is also called by the DeleteUser grpc endpoint.

evankanderson · 2024-08-23T21:21:13Z

internal/logger/telemetry_store.go

+// ProjectMetadata can be used to store project metadata in the context.
+type ProjectMetadata struct {


Can we call this "ProjectTombstone" or "ProjectDeletion", so it's clear that this is specifically for the case when a project is deleted?

Thanks for catching that and my bad for the obvious miss!

I've added the log audit at the core function which seems to be called from all other paths.

evankanderson

(Realized the previous comment should have been as "request changes")

JAORMX · 2024-08-26T15:21:07Z

internal/controlplane/handlers_projects.go

-		var err error
-		entitlements, err = qtx.GetEntitlementsByProjectID(ctx, projectID)
-		return err
-	})


If these calls aren't used anymore, can you clean these up from the database queries?

They are, in internal/projects/deleter.go

evankanderson

If we care about this functionality, we should add a test for it.

evankanderson · 2024-08-26T19:58:50Z

internal/projects/deleter.go

+	g, ctx := errgroup.WithContext(ctx)
+	g.Go(func() error {
+		var err error
+		profilesCount, err = qtx.CountProfilesByProjectID(ctx, projectID)
+		return err
+	})
+	g.Go(func() error {
+		var err error
+		reposCount, err = qtx.CountRepositoriesByProjectID(ctx, projectID)
+		return err
+	})
+	g.Go(func() error {
+		var err error
+		entitlements, err = qtx.GetEntitlementsByProjectID(ctx, projectID)
+		return err
+	})
+
+	if err := g.Wait(); err != nil {
+		return nil, fmt.Errorf("error getting project metadata: %w", err)


Is it worth the effort to parallelize these three queries?

evankanderson · 2024-08-26T19:59:39Z

internal/projects/deleter.go

+	)
+
+	g, ctx := errgroup.WithContext(ctx)
+	g.Go(func() error {


If you use a named argument, this would be much shorter:

g.Go(func() err error { profilesCount, err = qtx.CountProfilesByProjectID(ctx, projectID) }

evankanderson · 2024-08-26T20:00:22Z

internal/projects/deleter.go

+		return nil, fmt.Errorf("error getting project metadata: %w", err)
+	}
+
+	var entitlementsFeatures []string


Use make with a size argument to avoid needing extra resizes:

Suggested change

var entitlementsFeatures []string

entitlementsFeatures := make([]string, 0, len(entitlements))

evankanderson · 2024-08-26T20:00:56Z

internal/projects/deleter_test.go

+	mockStore.EXPECT().CountProfilesByProjectID(gomock.Any(), proj).
+		Return(int64(0), nil)
+	mockStore.EXPECT().CountRepositoriesByProjectID(gomock.Any(), proj).
+		Return(int64(0), nil)
+	mockStore.EXPECT().GetEntitlementsByProjectID(gomock.Any(), proj).
+		Return([]db.Entitlement{}, nil)


Can you add at least one test where you verify that these are getting added to the telemetry context?

evankanderson

A couple very minor simplifications, but I think this can merge as-is.

evankanderson · 2024-08-27T13:10:52Z

internal/logger/telemetry_store.go

+	return pt.Project == other.Project &&
+		pt.ProfileCount == other.ProfileCount &&
+		pt.RepositoriesCount == other.RepositoriesCount &&
+		len(pt.Entitlements) == len(other.Entitlements) &&
+		func() bool {
+			for i, v := range pt.Entitlements {
+				if v != other.Entitlements[i] {
+					return false
+				}
+			}
+			return true
+		}()


This is probably simpler as:

Suggested change

return pt.Project == other.Project &&

pt.ProfileCount == other.ProfileCount &&

pt.RepositoriesCount == other.RepositoriesCount &&

len(pt.Entitlements) == len(other.Entitlements) &&

func() bool {

for i, v := range pt.Entitlements {

if v != other.Entitlements[i] {

return false

}

}

return true

}()

for i, v := range pt.Entitlements {

if v != other.Entitlements[i] {

return false

}

}

return pt.Project == other.Project &&

pt.ProfileCount == other.ProfileCount &&

pt.RepositoriesCount == other.RepositoriesCount &&

len(pt.Entitlements) == len(other.Entitlements)

True -> updated

There is a potential out-of-bounds issue with the suggested code so I changed it a bit.

evankanderson · 2024-08-27T13:15:34Z

database/query/entitlements.sql

+FROM entitlements
+WHERE project_id = sqlc.arg(project_id)::UUID;


I'm guessing if this was SELECT feature FROM entitlements you still wouldn't get a []string as the return type...

It actually does return a []string. I did it this way to be more generic and reusable in the future but if you think it's better for this one to only return a slice of the feature names then I'm ok with it.

Done, thanks

evankanderson · 2024-08-28T13:07:53Z

internal/logger/telemetry_store.go

+	if len(pt.Entitlements) != len(other.Entitlements) {
+		return false
+	}
+
+	for i, v := range pt.Entitlements {
+		if v != other.Entitlements[i] {
+			return false
+		}
+	}


Ooops, I just realized that there is slices.Equal that should do all this.

evankanderson · 2024-08-28T13:20:11Z

Go ahead and merge without slices.Equal, or ping me when you push the change.

add: project deletion metadata audit log

5853a75

teodor-yanev self-assigned this Aug 23, 2024

teodor-yanev requested a review from evankanderson August 23, 2024 11:13

teodor-yanev added 2 commits August 23, 2024 14:16

update: post transaction log

dadbd66

fix: make lint

be8e223

evankanderson reviewed Aug 23, 2024

View reviewed changes

evankanderson requested changes Aug 23, 2024

View reviewed changes

teodor-yanev added 2 commits August 26, 2024 16:39

rename: metadata -> tombstone

29ec6f6

fix: export logs at the correct place

b0dd128

teodor-yanev requested a review from evankanderson August 26, 2024 15:19

JAORMX reviewed Aug 26, 2024

View reviewed changes

evankanderson reviewed Aug 26, 2024

View reviewed changes

teodor-yanev added 5 commits August 27, 2024 02:02

update: slight refactoring

a44057d

add: to record

9c64e81

add: unit test for ensuring the tombstone is set correctly

9de091d

add: comment exported method

887dcba

add: test for tombstone equals

f6d7feb

teodor-yanev requested a review from evankanderson August 27, 2024 11:56

evankanderson previously approved these changes Aug 27, 2024

View reviewed changes

update: refactoring

21bb516

teodor-yanev dismissed evankanderson’s stale review via 21bb516 August 28, 2024 12:21

teodor-yanev requested a review from evankanderson August 28, 2024 12:22

fix: out of bounds issue

1458acc

evankanderson previously approved these changes Aug 28, 2024

View reviewed changes

update: slices equal

2ec3756

teodor-yanev dismissed evankanderson’s stale review via 2ec3756 August 28, 2024 13:29

evankanderson previously approved these changes Aug 28, 2024

View reviewed changes

remove: concurrent calls to the db

7324d0b

teodor-yanev dismissed evankanderson’s stale review via 7324d0b August 28, 2024 14:16

evankanderson approved these changes Aug 28, 2024

View reviewed changes

teodor-yanev merged commit 9a439d5 into main Aug 28, 2024
21 checks passed

teodor-yanev deleted the project-deletion-audit branch August 28, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project deletion metadata audit log #4262

Project deletion metadata audit log #4262

teodor-yanev commented Aug 23, 2024

coveralls commented Aug 23, 2024 •

edited

Loading

evankanderson left a comment

evankanderson Aug 23, 2024

teodor-yanev Aug 26, 2024

evankanderson left a comment

JAORMX Aug 26, 2024

teodor-yanev Aug 26, 2024

evankanderson left a comment

evankanderson Aug 26, 2024

evankanderson Aug 26, 2024

evankanderson Aug 26, 2024

evankanderson Aug 26, 2024

evankanderson left a comment

evankanderson Aug 27, 2024

teodor-yanev Aug 28, 2024 •

edited

Loading

evankanderson Aug 27, 2024

teodor-yanev Aug 28, 2024 •

edited

Loading

teodor-yanev Aug 28, 2024

evankanderson Aug 28, 2024

evankanderson commented Aug 28, 2024

		// ProjectMetadata can be used to store project metadata in the context.
		type ProjectMetadata struct {

	var entitlementsFeatures []string
	entitlementsFeatures := make([]string, 0, len(entitlements))

		FROM entitlements
		WHERE project_id = sqlc.arg(project_id)::UUID;

Project deletion metadata audit log #4262

Project deletion metadata audit log #4262

Conversation

teodor-yanev commented Aug 23, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Aug 23, 2024 • edited Loading

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teodor-yanev Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teodor-yanev Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Aug 28, 2024

coveralls commented Aug 23, 2024 •

edited

Loading

teodor-yanev Aug 28, 2024 •

edited

Loading

teodor-yanev Aug 28, 2024 •

edited

Loading