New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cleanup: Use projects instead of groups and orgs #1054

Merged

JAORMX merged 1 commit into main from no-more-groups

Oct 3, 2023

Contributor

JAORMX commented Sep 29, 2023 •

edited

Loading

This takes into use the concept of projects in Mediator, thus replacing our usage
of groups.

In the background, we also got rid of the organizations SQL table and use projects
underneath. Organizations are identifiable via a is_organization boolean in the table.

Projects and Organizations now use a UUID index.

The intent is to have this PR reimplement the existing functionality as to not break existing logic. This is just the start in an effort to support hierarchical structures and consolidate our authz strategy.

JAORMX changed the title ~~cleanup: Use projects instead of groups and orgs~~ WIP: cleanup: Use projects instead of groups and orgs

JAORMX requested review from evankanderson and jhrozek

September 29, 2023 11:52

JAORMX force-pushed the no-more-groups branch 2 times, most recently from e63aa9e to 3921687 Compare

September 29, 2023 13:11

JAORMX requested a review from eleftherias

September 29, 2023 13:12

JAORMX force-pushed the no-more-groups branch from 3921687 to e5fb5c4 Compare

September 29, 2023 13:28

Member

evankanderson commented Sep 29, 2023

I suspect we'll need an additional "OrgInfo" table for org-level metadata for things like billing, but I'm happy to use a star schema for that.

evankanderson reviewed

View reviewed changes

Member

evankanderson left a comment

I made it to internal/engine so far, most of my comments are small and/or can be deferred.

database/migrations/000001_init.up.sql

               CREATE TABLE projects (
                   id UUID NOT NULL DEFAULT gen_random_uuid() PRIMARY KEY,
                   name TEXT NOT NULL,
-                  metadata JSONB NOT NULL,
+                  is_organization BOOLEAN NOT NULL DEFAULT FALSE,

Member

evankanderson Sep 29, 2023

Rather than adding an is_organization which should be equivalent to parent_id IS NULL, let's put the organization information a separate table?

Contributor Author

JAORMX Sep 30, 2023

What do you think about adding a well known schema for this in the metadata?

database/migrations/000001_init.up.sql

Comment on lines +26 to +27

		CREATE UNIQUE INDEX ON projects(name) WHERE parent_id IS NULL; -- if parent_id is null, then name must be unique
		CREATE UNIQUE INDEX ON projects(parent_id, name) WHERE parent_id IS NOT NULL; -- if parent_id is not null, then name must be unique for that parent

Member

evankanderson Sep 29, 2023

Put these in the table definition as:

CREATE TABLE PROJECTS (
    -- Row definitions
    CONSTRAINT "unique name in parent" UNIQUE NULLS NOT DISTINCT (parent_id, name)
);

database/migrations/000001_init.up.sql

               -- roles table
               CREATE TABLE roles (
                   id SERIAL PRIMARY KEY,
-                  organization_id INTEGER NOT NULL REFERENCES organizations(id) ON DELETE CASCADE,
-                  group_id INTEGER REFERENCES groups(id) ON DELETE CASCADE,
+                  organization_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE,

Member

evankanderson Sep 29, 2023

Do we need an organization ID for roles anymore?

Contributor Author

JAORMX Sep 30, 2023

I'll leave that here for now and we can move it off later. Trying to replicate what we already have.

database/migrations/000001_init.up.sql

    
            @@ -60,7 +41,7 @@ CREATE TABLE roles (
          
              -- users table

              CREATE TABLE users (

                  id SERIAL PRIMARY KEY,

                  organization_id INTEGER NOT NULL REFERENCES organizations(id) ON DELETE CASCADE,

                  organization_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE,

Member

evankanderson Sep 29, 2023

Maybe not for this PR, but I'm wondering if we want users to be associated with only one organization. I suspect not. But maybe we only shave one yak today.

Contributor Author

JAORMX Sep 30, 2023

Yeah, I'm trying to keep the exact same functionality we have today with this new constructs. But I'd saynin the near future we should remove this restriction and allow users to belong to more than one org.

database/migrations/000001_init.up.sql

Comment on lines +55 to 60

    
              -- user/projects

              CREATE TABLE user_projects (

                  id SERIAL PRIMARY KEY,

                  user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,

                  group_id INTEGER NOT NULL REFERENCES groups(id) ON DELETE CASCADE

                  project_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE

              );

Member

evankanderson Sep 29, 2023

What is this supposed to be for? Should it also be referencing a role?

Contributor Author

JAORMX Sep 30, 2023

Currently this tracks users belonging to a group. I'm keeping the same functionality for now. In the near future we should remove this and rely on role assignments instead.

internal/controlplane/handlers_projects.go Outdated Show resolved Hide resolved

internal/controlplane/handlers_repositories.go Outdated Show resolved Hide resolved

internal/controlplane/handlers_repositories.go Outdated Show resolved Hide resolved

internal/controlplane/handlers_repositories.go Outdated Show resolved Hide resolved

internal/controlplane/handlers_repositories.go Outdated Show resolved Hide resolved

Contributor Author

JAORMX commented Sep 30, 2023

I suspect we'll need an additional "OrgInfo" table for org-level metadata for things like billing, but I'm happy to use a star schema for that.

We could do that. I was thinking instead of using the JSONB column to add company and billing info. We anyway will use another system for the actual billing so we would just have pointers to that external system anyway. Then the idea is to expose as little of the metadata to the user as possible. We might build a proprietary microservice to handle the billing metadata.

JAORMX force-pushed the no-more-groups branch 17 times, most recently from 240fb57 to 8812386 Compare

October 2, 2023 12:35

eleftherias reviewed

View reviewed changes

Contributor

eleftherias left a comment

Nice! I left one small comment about file names, so they don't accidentally slip through.

cmd/cli/app/project/group.go Outdated Show resolved Hide resolved

JAORMX force-pushed the no-more-groups branch 2 times, most recently from 16063e5 to 837f766 Compare

October 2, 2023 14:33

JAORMX force-pushed the no-more-groups branch from a3a83b6 to 1f11981 Compare

October 2, 2023 15:32

rdimitrov reviewed

View reviewed changes

Member

rdimitrov left a comment

I know it's still WIP, but it's nice to see this consistent 🚀 Try running git grep group to see other things that might have been missed, i.e. the rules type definitions, log messages and random variables are left to be updated 👍

internal/controlplane/handlers_organization.go Outdated Show resolved Hide resolved

JAORMX force-pushed the no-more-groups branch from 1f11981 to cbf6c19 Compare

October 3, 2023 07:28

JAORMX requested review from rdimitrov, eleftherias and evankanderson

October 3, 2023 07:28

JAORMX force-pushed the no-more-groups branch 3 times, most recently from c375b19 to 941428f Compare

October 3, 2023 08:36

lukehinds reviewed

View reviewed changes

internal/controlplane/handlers_organization.go Outdated

		IsProtected: true,
		Description: fmt.Sprintf("Default admin group for %s", org.Name),

Contributor

lukehinds Oct 3, 2023

nit: would admin group even exist still with this PR?

Contributor Author

JAORMX Oct 3, 2023

it does, we're not changing the existing functionality yet. Just making the current logic work with he new structure. We'll start changing logic in subsequent PRs

JAORMX force-pushed the no-more-groups branch 2 times, most recently from dc6e952 to 279e470 Compare

October 3, 2023 09:00

JAORMX changed the title ~~WIP: cleanup: Use projects instead of groups and orgs~~ cleanup: Use projects instead of groups and orgs

eleftherias previously approved these changes

View reviewed changes

JAORMX dismissed eleftherias’s stale review via

d5f12ae

October 3, 2023 09:23

JAORMX force-pushed the no-more-groups branch 2 times, most recently from d5f12ae to e85c7c9 Compare

October 3, 2023 09:25

eleftherias reviewed

View reviewed changes

internal/controlplane/handlers_projects.go Outdated Show resolved Hide resolved

JAORMX force-pushed the no-more-groups branch from e85c7c9 to 5a65044 Compare

October 3, 2023 09:32

eleftherias previously approved these changes

View reviewed changes


          cleanup: Use projects instead of groups and orgs

9d6706b

This takes into use the concept of projects in Mediator, thus replacing our usage
of groups.

In the background, we also got rid of the organizations SQL table and use projects
underneath. Organizations are identifiable via a `is_organization` boolean in the table.

Projects and Organizations now use a UUID index.

JAORMX dismissed eleftherias’s stale review via

9d6706b

October 3, 2023 10:06

JAORMX force-pushed the no-more-groups branch from 5a65044 to 9d6706b Compare

October 3, 2023 10:06

JAORMX requested review from eleftherias and lukehinds

October 3, 2023 10:07

eleftherias approved these changes

View reviewed changes

rdimitrov approved these changes

View reviewed changes

Contributor

jhrozek commented Oct 3, 2023

I didn't fully review the code but I tried the PR as both admin and a non-admin user and all the tasks that I normally use in development worked fine.

JAORMX merged commit 545aea2 into main

13 checks passed

JAORMX deleted the no-more-groups branch

October 3, 2023 12:22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rdimitrov rdimitrov approved these changes

eleftherias eleftherias approved these changes

jhrozek Awaiting requested review from jhrozek

evankanderson Awaiting requested review from evankanderson

lukehinds Awaiting requested review from lukehinds

Labels

None yet