Add initial introduction of providers to the database #955

JAORMX · 2023-09-13T12:00:30Z

This simply replaces the usages of the github provider checks and instead
provides a simple implementation of providers that will be expanded upon.

The intent is to leave the same functionality as-is, but instead rely on a
provider that's installed on every organization by default.

I've left traces of how the implementation will evolve by adding a definition
and a providers_type column to the providers database. These will serve as the means
of instantiating providers dynamically, instead of always building the github one
as we do today. But, to keep things short, I've left that out of this PR.

Related-to: stacklok/epics#34

jhrozek

Two quick comments, I need to wrap my head around where are you going with this. I /really/ like that we would get rid of referencing GitHub everywhere with a constant though!

jhrozek · 2023-09-13T12:38:16Z

database/migrations/000001_init.up.sql

+
+-- Create default GitHub provider
+INSERT INTO providers (name, group_id, definition)
+VALUES ('github', 1, '{}');


Not strictly related to what this work is trying to accomplish, but shouldn't we create higher numbered migrations from this point on?

Because of patches like this (work that still needs to be done) is why I was not too keen on freezing the initial migration and starting to work on higher numbers.

The change from group IDs to projects will be equally disruptive.

database/migrations/000001_init.up.sql

evankanderson · 2023-09-14T20:30:33Z

database/migrations/000001_init.up.sql

+    provider_id UUID NOT NULL REFERENCES providers(id) ON DELETE CASCADE,
    group_id INTEGER NOT NULL REFERENCES groups(id) ON DELETE CASCADE,


If providers are nested under group_ids, do we need an explicit group_id on this table and the following ones?

I'm a bit confused by this comment, there is a group I'd in this table and most of the ones I've changed.

That was my point -- if provider_id is unique by group, then if you know the provider_id, you know the corresponding group. My suggestion was to remove the group_id field here, since it was redundant with the provider. (Alternatively, since providers are unique by group_id and name, you could have a provider_name here along with the group_id and use that to index into the Providers table -- possibly even as a primary index rather than the UUID.)

I think part of this is that my brain is used to thinking about partitioning SQL data stores across shards, and I'm trying to latch onto those sharding keys as an organizing mechanism even though we don't need it for scale at this point.

ah! I see the misunderstanding. No, there could be several providers in one group. The name is just unique.

There can be several providers in one group, but we want to maintain the constraint that <this row>.group_id == <this row>.provider_id.<providers row>.group_id, right? I'm not sure there's a good way to express that in SQL; I was suggesting that we could remove the <this row>.group_id, or use group_id and and provider_name to avoid possible future database inconsistencies, e.g.

CREATE TABLE provider_access_tokens ( id SERIAL PRIMARY KEY, group_id INTEGER NOT NULL REFERENCES groups(id) ON DELETE CASCADE, provider TEXT NOT NULL, owner_filter TEXT, encrypted_token TEXT NOT NULL, expiriation_time TIMESTAMP NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT NOW(), updated_at TIMESTAMP NOT NULL DEFAULT NOW() FOREIGN KEY (group_id, provider) REFERENCES providers(group_id, name) ON DELETE CASCADE );

evankanderson · 2023-09-14T20:31:12Z

database/migrations/000001_init.up.sql

@@ -154,7 +168,7 @@ CREATE TABLE artifact_versions (

 CREATE TABLE session_store (
    id SERIAL PRIMARY KEY,
-    provider TEXT NOT NULL,
+    provider UUID NOT NULL REFERENCES providers(id) ON DELETE CASCADE,
    grp_id INTEGER,


Is this group_id as we'd generally conceive of it? (Maybe I shouldn't poke that sleeping bear...)

Yes, which in the near future will have to change to projects

Sorry, I specifically meant the spelling was different than elsewhere.

oh, yeah, I don´t know why that is. We should probably change this in a separate PR.

database/query/providers.sql

internal/controlplane/handlers_authz.go

internal/controlplane/handlers_githubwebhooks.go

evankanderson · 2023-09-14T21:16:52Z

internal/providers/providers.go

@@ -32,13 +34,13 @@ import (
 // instead of a concrete github client.
 func BuildClient(
 	ctx context.Context,
-	prov string,
+	prov uuid.UUID,
 	groupID int32,


I don't think we need groupID here anymore.

evankanderson · 2023-09-14T21:21:34Z

Do we have a guess as to how many more database drops we have coming up? Should we plan for weekly drops until the second week of October, or is this the last one we forsee?

JAORMX · 2023-09-14T23:23:28Z

Two drops based on providers, one drop based.on group to projects, then one on IAM. That's what I foresee.

evankanderson · 2023-09-15T04:29:52Z

Thanks for the forecast on database drops. A few clarifications, but I'm willing to drop my required change.

Agreed on number of times we drop the database.

JAORMX · 2023-09-15T09:54:44Z

I talked with @yrobla and ended up changing the logic for the RevokeOAuthTokens call to what it intended to be with this providers concept.

evankanderson · 2023-09-21T08:01:28Z

(Let me know when you want another review)

JAORMX · 2023-09-22T04:13:49Z

Now 😃

JAORMX · 2023-09-22T12:24:20Z

@jhrozek could you also take a look at this one when you get a chance? Would really use as many eyes as possible here.

jhrozek · 2023-09-22T15:36:32Z

@jhrozek could you also take a look at this one when you get a chance? Would really use as many eyes as possible here.

Sure, thanks for the reminder!

jhrozek

I didn't really find any issues during code review. I only left one comment.

jhrozek · 2023-09-24T19:45:51Z

database/migrations/000001_init.up.sql

+
+-- Create default GitHub provider
+INSERT INTO providers (name, group_id, implements, definition)
+VALUES ('github', 1, ARRAY ['github', 'git', 'rest']::provider_type[], '{}');


Just a question and something that can be fixed in a migration later on - why did you choose this particular set for GitHub? I understand github and git, but what would we use rest for here? Also, did you consider adding oci since ghcr.io is tied to GitHub and we already support it (although in kind of a tacky way) for artifacts?

Not saying this is wrong, I'm just trying to picture how would other providers look like.

I didn't really see full-straight forward support for an oci provider from github. I can add it once I work on separating implementations from interfaces.

evankanderson

A few minor comments, but LGTM

evankanderson · 2023-09-22T21:11:34Z

database/migrations/000001_init.up.sql

@@ -115,7 +132,7 @@ CREATE TABLE signing_keys (
 CREATE TABLE repositories (
    id SERIAL PRIMARY KEY,
    provider TEXT NOT NULL,
-    group_id INTEGER NOT NULL REFERENCES groups(id) ON DELETE CASCADE,


Do you want to keep this REFERENCES foreign key constraint? I suppose it's not strictly needed...

it's part of the group_id + provider foreign key. Are you suggesting we re-add it?

It's also a foreign key into the groups table, but we can rely on the transitive ON DELETE CASCADE from providers if we prefer.

database/migrations/000001_init.up.sql

internal/controlplane/common.go

evankanderson · 2023-09-22T22:26:59Z

internal/controlplane/handlers_githubwebhooks.go

@@ -316,20 +306,27 @@ func (s *Server) parseGithubEventForProcessing(
 		return nil
 	}

-	return s.parseRepoEvent(context.Background(), ghclient.Github, payload, msg)
+	return s.parseRepoEvent(context.Background(), payload, msg)


Hmm -- the way we've done this right now, we don't pass pull_request or package type events through to s.parseRepoEvent. Not to fix here, but it feels like we should be using watermill's built-in fanout, as tested here:

https://github.com/stacklok/mediator/blob/main/internal/events/eventer_test.go#L88

That's a good point, we probably need to revisit this and reverify the logic. Right now what we do in each function is build a message with appropriate metadata for the event handler. Each branch will have a little more metadata needed, but I guess most of this could be refactored into pieces to be more readable.

evankanderson · 2023-09-22T22:32:25Z

internal/controlplane/handlers_oauth.go

-
-	// need to read all tokens from the provider and revoke them
-	tokens, err := s.store.GetAccessTokenByProvider(ctx, ghclient.Github)
+// This is in case of a security breach, where we need to revoke all tokens


Would this make more sense to execute as raw SQL if that happens?

Now that you raise this, I think I would prefer this to be part of an administrator's playbook instead of a function an admin can do via the control plane. This merely documented what the feature was intended for and made it work with the newer logic. Let me raise a bug to remove this altogether in further iterations.

This does indeed remove the tokens from the DB, but it also calls the GH API to invalidate the token, so that part might need a nice utility. We could have it as a separate sub-command or a separate container an admin could leverage.

evankanderson · 2023-09-24T20:08:33Z

internal/controlplane/handlers_repositories.go

+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "cannot infer group id")
+		}
+		in.GroupId = group


nit: You could put in.GroupId up in the assignment on line 263 if you want, since changes to the request message will be ignored when you return an error.

internal/controlplane/handlers_repositories.go

evankanderson · 2023-09-24T20:20:14Z

internal/gh/queries/sync_repo_db.go

+			if existingRepo.GroupID != groupId {
+				fmt.Println("got request to sync repository of different group. Skipping")
+				continue
+			}


At some point, we should record a metric here, but feel free to hold off for a future PR.

internal/reconcilers/run_policy.go

proto/mediator/v1/mediator.proto

This simply replaces the usages of the `github` provider checks and instead provides a simple implementation of providers that will be expanded upon. The intent is to leave the same functionality as-is, but instead rely on a provider that's installed on every organization by default. I've left traces of how the implementation will evolve by adding a definition and a `providers_type` column to the providers database. These will serve as the means of instantiating providers dynamically, instead of always building the github one as we do today. But, to keep things short, I've left that out of this PR.

This removes the group information from entities in favor of relying on providers for data integrity.

This reverts commit 84ce748.

Co-Authored-By: Evan Anderson <evan@stacklok.com>

These prevent us from creating multiple objects which is not the desired outcome.

JAORMX · 2023-09-25T06:04:53Z

Rebased and addressed some of the comments.

rdimitrov

Small nit, but noticed that on some places you missed using the providerError (and used its code representation) when searching for a provider.

jhrozek

I think most if not all comments have been addressed and the PR is now large enough that it makes sense to just keep on going with incremental PRs.

evankanderson · 2023-09-25T15:29:15Z

database/migrations/000001_init.up.sql

@@ -115,7 +132,7 @@ CREATE TABLE signing_keys (
 CREATE TABLE repositories (
    id SERIAL PRIMARY KEY,
    provider TEXT NOT NULL,
-    group_id INTEGER NOT NULL REFERENCES groups(id) ON DELETE CASCADE,


It's also a foreign key into the groups table, but we can rely on the transitive ON DELETE CASCADE from providers if we prefer.

JAORMX changed the title ~~Add initial introduction of providers to the database~~ WIP: Add initial introduction of providers to the database Sep 13, 2023

JAORMX requested a review from jhrozek September 13, 2023 12:05

JAORMX force-pushed the providers-init branch 2 times, most recently from b61edbf to 86fad62 Compare September 13, 2023 12:11

jhrozek reviewed Sep 13, 2023

View reviewed changes

JAORMX force-pushed the providers-init branch 7 times, most recently from 18e5ea5 to 41e65b0 Compare September 14, 2023 08:03

JAORMX changed the title ~~WIP: Add initial introduction of providers to the database~~ Add initial introduction of providers to the database Sep 14, 2023

jhrozek previously approved these changes Sep 14, 2023

View reviewed changes

database/migrations/000001_init.up.sql Show resolved Hide resolved

evankanderson self-requested a review September 14, 2023 13:04

evankanderson previously requested changes Sep 14, 2023

View reviewed changes

database/migrations/000001_init.up.sql Show resolved Hide resolved

evankanderson reviewed Sep 14, 2023

View reviewed changes

JAORMX dismissed jhrozek’s stale review via a5c215e September 15, 2023 09:06

JAORMX force-pushed the providers-init branch 2 times, most recently from a5c215e to 53d5a0c Compare September 15, 2023 09:11

JAORMX requested review from evankanderson and jhrozek September 15, 2023 09:26

JAORMX force-pushed the providers-init branch from 53d5a0c to 2166962 Compare September 15, 2023 09:53

JAORMX force-pushed the providers-init branch 2 times, most recently from a1de46c to 7392ff8 Compare September 15, 2023 11:51

JAORMX force-pushed the providers-init branch from acf4172 to cb3e901 Compare September 18, 2023 15:32

jhrozek previously approved these changes Sep 24, 2023

View reviewed changes

evankanderson previously approved these changes Sep 24, 2023

View reviewed changes

JAORMX and others added 11 commits September 25, 2023 08:32

Change logic of revoke tokens call to revoke everything

a4d832a

Remove hardcoded check for github in policy helper

c6f8536

Decouple entities from groups

c2fe49f

This removes the group information from entities in favor of relying on providers for data integrity.

Revert "Decouple entities from groups"

8becaa2

This reverts commit 84ce748.

Use provider name + group ID as compount foreign key

d3f38d8

Co-Authored-By: Evan Anderson <evan@stacklok.com>

Remove UNIQUE constraints for providers and groups from entities

745d412

These prevent us from creating multiple objects which is not the desired outcome.

Fix unit tests

352cccf

Remove unnecessary comment.

c364e40

rename stuttering function name

1c37ff3

reconcilers: Return error on transcient sql error

43ced75

JAORMX dismissed stale reviews from evankanderson and jhrozek via 43ced75 September 25, 2023 05:59

JAORMX force-pushed the providers-init branch from cb3e901 to 43ced75 Compare September 25, 2023 05:59

don't pass group ID to get repositories call

a5a5476

JAORMX requested review from evankanderson and jhrozek September 25, 2023 06:04

rdimitrov reviewed Sep 25, 2023

View reviewed changes

jhrozek approved these changes Sep 25, 2023

View reviewed changes

evankanderson approved these changes Sep 25, 2023

View reviewed changes

JAORMX merged commit 625e1f8 into main Sep 25, 2023
13 checks passed

JAORMX deleted the providers-init branch September 25, 2023 15:55

		provider_id UUID NOT NULL REFERENCES providers(id) ON DELETE CASCADE,
		group_id INTEGER NOT NULL REFERENCES groups(id) ON DELETE CASCADE,

Add initial introduction of providers to the database #955

Add initial introduction of providers to the database #955

Conversation

JAORMX commented Sep 13, 2023 • edited Loading

jhrozek 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAORMX Sep 15, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Sep 14, 2023

JAORMX commented Sep 14, 2023

evankanderson commented Sep 15, 2023

JAORMX commented Sep 15, 2023

evankanderson commented Sep 21, 2023

JAORMX commented Sep 22, 2023

JAORMX commented Sep 22, 2023

jhrozek commented Sep 22, 2023

jhrozek 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

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

JAORMX commented Sep 25, 2023

rdimitrov left a comment

Choose a reason for hiding this comment

jhrozek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAORMX commented Sep 13, 2023 •

edited

Loading

JAORMX Sep 15, 2023 •

edited

Loading