feat: Define new grants tables #5486

elimt · 2025-01-28T15:21:03Z

Create new tables for grants:

iam_role_global:
Roles that are placed in the global scope will be persisted in the iam_role_global table. A global role has a grant_scope which must be one of:

descendants
children
individual

This enforces that a global role's grants either apply to:

All orgs and projects.
All orgs.
An individual set of orgs and/or projects.

When the grant_scope is set to individual, entries for the specific set of orgs and/or projects can be added to the
iam_role_global_individual_grant_scope table.

Separately, a global role can be set to also apply its grant to the global scope by setting grant_this_role_scope to true.

iam_role_org:
Roles that are placed in an org scope will be persisted in the iam_role_org table. An org role has a grant_scope which must be one of:

children
individual

This enforces that an org role's grants either apply to:

All projects in the org.
An individual set of projects in the org.

When the grant_scope is set to individual, entries for the specific set of projects can be added to the
iam_role_org_individual_grant_scope table.
NOTE: The projects must belong to the org's scope

Separately, an org role can be set to also apply its grant to the org by setting grant_this_role_scope to true.

resource_enm:
Contains all boundary resources. This is used by iam_grant to set the resource from a canonical_grant.
iam_grant
Stores the canonical grant string and the resource for filtering on specific grants.

bosorawis · 2025-01-28T17:45:14Z

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

+  create trigger immutable_columns before update on iam_role_global_individual_grant_scope
+  for each row execute procedure immutable_columns('scope_id', 'create_time');


I think every column in this table is immutable, not sure if we need constraints for all of them though 🤔

bosorawis · 2025-01-28T17:45:55Z

internal/db/schema/migrations/oss/postgres/100/02_iam_role_org.up.sql

+  for each row execute procedure default_create_time();
+
+  -- ensure the project's parent is the role's scope
+  create or replace function ensure_project_belongs_to_org() returns trigger


nitpick:

Suggested change

create or replace function ensure_project_belongs_to_org() returns trigger

create or replace function ensure_project_belongs_to_role_org() returns trigger

Co-authored-by: David Kanney <[email protected]> Co-authored-by: Sorawis Nilparuk <[email protected]>

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

mgaffney · 2025-01-28T22:37:38Z

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

+  as $$
+  begin
+    if new.grant_this_role_scope is distinct from old.grant_this_role_scope then
+      new.grant_scope_update_time = now();


Should this be grant_this_role_scope_update_time?

Yes it should. I'll fix that

mgaffney · 2025-01-28T22:41:08Z

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

+  -- This is only applicable to the name and description columns because we
+  -- do not want the update_time to be updated when the grant_scope or grant_this_role_scope
+  -- columns are updated.


Are we sure about this? I thought we wanted the update_time to be updated no matter what changed. If we do what it says here, then the update_time column would behave differently from all of our other update_time columns. @tmessi do you have any thoughts on this?

Yea, the update_time should be updated when anything in the role aggregate changes, including any changes to the grant scopes.

It would probably be sufficient to use the existing update_time_column trigger function.

I would also note that the way this works currently is that in the iam repository functions that modify the nested structure,
like AddRoleGrants, we explicitly increment the version column of the role in the same transaction that we make the changes to the grant scopes. This along with the database trigger ensures that the update_time changes.

Okay because we are tracking different update times, I thought we wanted to treat them differently. I can remove this new function for specially handling this.

So with every update to a role subtype, we will update the update_time and the version

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

mgaffney · 2025-01-28T22:46:08Z

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

+        references iam_role(public_id)
+        on delete cascade
+        on update cascade,
+    scope_id wt_scope_id


When can the scope_id be null?

it should never be null. I'll update the column to scope_id wt_scope_id not null

mgaffney · 2025-01-28T22:46:30Z

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

+    name text,
+    description text,
+    grant_this_role_scope boolean not null default false,
+    grant_scope text


When can the grant_scope be null?

grant_scope can be null when the grant scope uses the special keyword this. The grant_this_role_scope will be set to true and the grant_scope column will be null

mgaffney · 2025-01-28T22:51:42Z

internal/db/schema/migrations/oss/postgres/100/05_iam_grant.up.sql

+  create table iam_grant (
+    canonical_grant text not null primary key,
+    resource text not null
+      constraint resource_enm_fkey
+        references resource_enm(string)
+        on delete restrict
+        on update cascade
+  );


Shouldn't the iam_role_grant table have a foreign key to this table?

Actually there should be one on the iam_role_grant table to ensure canonical_grant can't get updated on only iam_role_grant table. I'll add that

mgaffney · 2025-01-28T22:52:53Z

internal/db/sqltest/Makefile

-		 tests/org/*.sql \
-		 tests/wh/*/*.sql \
-		 tests/sentinel/*.sql \
-		 tests/credential/*/*.sql \
-		 tests/session/*.sql \
 		 tests/account/*/*.sql \
-		 tests/target/*.sql \
-		 tests/controller/*.sql \
-		 tests/hcp/*/*.sql \
+		 tests/alias/*.sql \
+		 tests/auth/*/*.sql \
 		 tests/census/*.sql \
-		 tests/kms/*.sql \
-		 tests/storage/*.sql \
+		 tests/controller/*.sql \
+		 tests/credential/*/*.sql \
 		 tests/domain/*.sql \
+		 tests/hcp/*/*.sql \
 		 tests/history/*.sql \
-		 tests/recording/*.sql \
-		 tests/alias/*.sql \
-		 tests/auth/*/*.sql \
-		 tests/purge/*.sql \
+		 tests/host/*.sql \
+		 tests/iam/*.sql \
+		 tests/kms/*.sql \
+		 tests/org/*.sql \
 		 tests/pagination/*.sql \
 		 tests/policy/*.sql \
-		 tests/host/*.sql \
-		 tests/server/*.sql


Why change the order of the existing tests?

I changed it to match the alphabetical order - similar to how these directories show up in the IDEs to try to make it easier to glance if any directory was missed.

mgaffney · 2025-01-28T22:53:51Z

internal/db/sqltest/tests/iam/iam_role_org.sql

+-- Copyright (c) HashiCorp, Inc.
+-- SPDX-License-Identifier: BUSL-1.1


Can we remove this (almost) empty file?

mgaffney · 2025-01-28T23:03:50Z

internal/db/schema/migrations/oss/postgres/100/04_resource.up.sql

+begin;
+
+  create table resource_enm (
+    string text not null primary key


I think string is a poor column name since it is a primitive type in many languages and it doesn't convey what the values of the column represent.

Shouldn't there be a constraint on this column since this is an enum table?

I updated the column to name so anyone looking at the table knows it's the resource name

Yes there should be. I'll add a constraint for valid resources

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

tmessi · 2025-01-29T14:27:51Z

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql

+
+  create trigger update_iam_role_table_update_time before update on iam_role_global
+    for each row execute procedure update_iam_role_table_update_time();
+


You also need a trigger to update the version column:

create trigger update_version_column after update on iam_role_global for each row execute procedure update_version_column();

You will also need this for iam_role_org and iam_role_project.

tmessi · 2025-01-29T14:57:42Z

internal/db/schema/migrations/oss/postgres/100/02_iam_role_org.up.sql

+  select scope_id
+    into org_scope_id
+    from iam_role_org
+   where public_id = new.role_id;
+
+  if org_scope_id is null then
+    raise exception 'role % not found in iam_role_org', new.role_id;
+  end if;
+
+  -- Find the project parent for the inserted scope_id
+  select parent_id
+    into project_parent_id
+    from iam_scope_project
+   where scope_id = new.scope_id;
+
+  if project_parent_id is null then
+    raise exception 'project scope_id % not found in iam_scope_project', new.scope_id;
+  end if;
+
+  -- Compare parent_id with the org scope
+  if project_parent_id != org_scope_id then
+    raise exception 'project % belongs to a different org',
+      new.scope_id;
+  end if;


I believe we can do this with a single query. We wouldn't be able to have as many detailed error messages, but some of these cases should not be possible given other constraints on tables.
So you could do something like:

perform from iam_scope_project join iam_role_org on iam_role_org.scope_id = iam_scope_project.parent_id where iam_scope_project.scope_id = new.scope_id and iam_role_org.public_id = new.role_id; if not found then raise exception 'project scope_id % not found in org', new.scope_id; end if;

tmessi · 2025-01-29T15:23:16Z

internal/db/sqltest/tests/iam/iam_role_global.sql

+    -- Set up a scope  to test against
+  insert into iam_scope
+    (type,      public_id,      parent_id)
+  values
+    ('org',     'o_1111111111', 'global');


I would take a look at the files in various *_persona.sql files in initdb.d. For the sqltests we tend to use the these for test setup. Each persona represents a different org. They can be used in different ways, depending on what is needed:

Generally the widgets persona is preferred. The resources from this persona do no get loaded when the database is initialized, instead they can be created as part of an individual test setup by calling wtt_load with the necessary aggregates.

Each test takes place in a single transaction, so that it can be rolled back. This can make some test scenarios difficult when they require multiple transactions especially with things like changes to timestamps. In these cases you can use either the colors or food truck personas which have their resources created during the database initialization phase.

I think in this case we could just used the widgets persona by calling:

wtt_load('widgets', 'iam');

If we need to only create some resources form iam, I would suggest we split up the existing _wtt_load_widgets_iam function into multiple. We could add new functions like _wtt_load_widgets_iam_scope, _wtt_load_widgets_user, _wtt_load_widgets_group, _wtt_load_widgets_grant, etc. And have _wtt_load_widgets_iam call each of these functions.
Then for these tests we could call:

wtt_load('widgets', 'iam_scope');

I switched to using the personas by calling

wtt_load('widgets', 'iam');

I have not found a need to load only specific parts of iam widget yet. Once necessary, I can split it up

tmessi · 2025-01-29T15:24:19Z

internal/db/sqltest/tests/iam/iam_role_global.sql

+  -- 2a) clean up any previous data
+  delete from iam_role;


We don't usually do this within the sqltests. I see a few spots where it seems like we started doing this, but instead I would recommend either splitting these tests out to separate files so they can run in separate transactions, or if you want keep it all in this one test file, you can use a new public_id for the subsequent tests.

tmessi · 2025-01-29T15:26:40Z

internal/db/sqltest/tests/iam/iam_role_global.sql

+  -- remove any existing roles and their grant scopes, so we can test fresh
+  delete from iam_role;


I don't think this should be necessary, we should be able to make these assertions even with the roles from the other personas in place.

Took that out. I switched to using the personas

- alter iam_role_grant to add constraint on canonical_grant - add comments to all functions and tables - remove function for only setting `update_time` on iam_role subtypes when only the name and description have been updated - add new trigger for setting update_time when iam_role subtypes are updated - add new trigger for updating the version when iam_role subtypes are updated - update resource_enm table column name from `string` to `name` - add constraint to resource_enm table for `name` column

- update update_time column on iam_role_org - update syntax error on `ensure_project_belongs_to_role_org` function - adding missing subtype trigger on iam_role_project - fix column insert on `resource_enm`

bosorawis and others added 9 commits December 26, 2024 15:45

add migration scripts

e16eb8d

delete duplicate resources

211b1db

fix color persona test setup

3ef09da

fix sqltest setup with new iam_grant

e747fc7

refactor makefiles for sqltest

4b68c1c

feat(db): split up DB migration & add sql tests

7623167

add timestamp for grant_scope updates

a270b98

feat: update timestamp for iam_role_global & iam_role_org tables

0ad7b5e

feat: update iam role timestamps

b71e32e

elimt added the pr/no-milestone Ignores the Milestone Check label Jan 28, 2025

elimt requested review from mgaffney and tmessi January 28, 2025 15:21

github-actions bot added core/db core core/sql labels Jan 28, 2025

chore: remove unapplicable comment

774af50

bosorawis reviewed Jan 28, 2025

View reviewed changes

elimt and others added 2 commits January 28, 2025 16:21

add timestamps on create or update to fields

a8894eb

Co-authored-by: David Kanney <[email protected]> Co-authored-by: Sorawis Nilparuk <[email protected]>

revert db/makefile change

6f0ae35

elimt force-pushed the bosorawis-sql-new-grants-tables-and-objects branch from e3afe7b to 6f0ae35 Compare January 28, 2025 21:24

elimt commented Jan 28, 2025

View reviewed changes

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql Show resolved Hide resolved

elimt commented Jan 28, 2025

View reviewed changes

internal/db/schema/migrations/oss/postgres/100/01_iam_role_global.up.sql Show resolved Hide resolved

mgaffney requested changes Jan 28, 2025

View reviewed changes

mgaffney reviewed Jan 28, 2025

View reviewed changes

tmessi requested changes Jan 29, 2025

View reviewed changes

elimt added 2 commits January 31, 2025 21:37

- fix comments

818e87a

- update update_time column on iam_role_org - update syntax error on `ensure_project_belongs_to_role_org` function - adding missing subtype trigger on iam_role_project - fix column insert on `resource_enm`

add test coverage for new migration files

e707cff

elimt requested review from mgaffney and tmessi February 1, 2025 03:30

elimt changed the title ~~WIP: SQL Define new grants tables~~ feat: Define new grants tables Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Define new grants tables #5486

feat: Define new grants tables #5486

elimt commented Jan 28, 2025 •

edited

Loading

bosorawis Jan 28, 2025

bosorawis Jan 28, 2025

mgaffney Jan 28, 2025

elimt Jan 30, 2025

mgaffney Jan 28, 2025

tmessi Jan 29, 2025

elimt Jan 30, 2025

mgaffney Jan 28, 2025

elimt Jan 30, 2025

mgaffney Jan 28, 2025

elimt Jan 30, 2025

mgaffney Jan 28, 2025

elimt Jan 30, 2025

mgaffney Jan 28, 2025

bosorawis Jan 28, 2025

mgaffney Jan 28, 2025

mgaffney Jan 28, 2025

elimt Jan 30, 2025

tmessi Jan 29, 2025

tmessi Jan 29, 2025

tmessi Jan 29, 2025

elimt Feb 1, 2025

tmessi Jan 29, 2025

tmessi Jan 29, 2025

elimt Feb 1, 2025

		create trigger immutable_columns before update on iam_role_global_individual_grant_scope
		for each row execute procedure immutable_columns('scope_id', 'create_time');

	create or replace function ensure_project_belongs_to_org() returns trigger
	create or replace function ensure_project_belongs_to_role_org() returns trigger

		-- Copyright (c) HashiCorp, Inc.
		-- SPDX-License-Identifier: BUSL-1.1


		create trigger update_iam_role_table_update_time before update on iam_role_global
		for each row execute procedure update_iam_role_table_update_time();

		-- remove any existing roles and their grant scopes, so we can test fresh
		delete from iam_role;

feat: Define new grants tables #5486

Are you sure you want to change the base?

feat: Define new grants tables #5486

Conversation

elimt commented Jan 28, 2025 • 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

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

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

elimt commented Jan 28, 2025 •

edited

Loading