[A2-815] list rules with staged and current rules #646

srenatus · 2019-06-20T12:13:59Z

🔩 Description

ListRules: Refactor or write separate APIs to return both a current rules list and staged rules list

This adds a ListStagedAndAppliedRules to the storage layer, and another grpc method to authz-service exposing it.

TODO:

expose in the gateway, using an ENUM for applied/staged

👍 Definition of Done

ListRules: Refactor or write separate APIs to return both a current rules list and staged rules list

😉

👟 Demo Script / Repro Steps

rebuild components/authz-service
create a project

TODO

⛓️ Related Resources

A2-815

✅ Checklist

Necessary tests added/updated?
Necessary docs added/updated?
Code actually executed?
Vetting performed (unit tests, lint, etc.)?

srenatus

Some reviewer notes inline

srenatus · 2019-06-20T14:08:16Z

api/interservice/authz/v2/project.proto

@@ -111,6 +111,8 @@ message ProjectRule {
    string name = 3;
    ProjectRuleTypes type = 4;
    repeated Condition conditions = 5;
+    bool deleted = 6;


ℹ️Surfacing the deleted and staged/applied status of a rule in the interservice API. Nothing done for automate-gateway yet.

srenatus · 2019-06-20T14:08:38Z

api/interservice/authz/v2/project.proto

@@ -173,7 +175,9 @@ message GetRuleResp {
    ProjectRule rule = 1;
 }

-message ListRulesReq {}
+message ListRulesReq {
+    bool include_staged = 1;


ℹ️ Pass ListRulesReq{IncludeStaged: true} to also see staged rules.

srenatus · 2019-06-20T14:46:03Z

api/interservice/authz/v2/project.proto

@@ -111,6 +111,8 @@ message ProjectRule {
    string name = 3;
    ProjectRuleTypes type = 4;
    repeated Condition conditions = 5;
+    bool deleted = 6;
+    string status = 7;


OMG why no enum?!!!

I hear you -- but it's just so annoying to convert three different kinds of enums (storage layer, authz-service GRPC, automate-gateway GRPC), and I'm not convinced it's worth it. Happy to listen to arguments, but maybe we can just make our lives a little brighter and our PRs a little smaller. 😉

Also, in the external API (automate-gateway), we can (and should) still use an enum.

I really like the compromise of having an enum at the gateway and not in the domain layer. The enum unwrapping code is super super annoying.

srenatus · 2019-06-20T14:47:27Z

components/authz-service/server/v2/rules_test.go

@@ -143,6 +143,7 @@ func TestCreateRule(t *testing.T) {
 							Operator:  api.ProjectRuleConditionOperators_MEMBER_OF,


ℹ️I haven't added any tests on that layer yet.

srenatus · 2019-06-20T14:47:52Z

components/authz-service/storage/postgres/migration/sql/49_add_status_field_to_rules.up.sql

@@ -0,0 +1,154 @@
+BEGIN;


ℹ️ Updated SQL functions to return the "status" = "applied"/"staged".

Converted to use json_build_object in the process.

srenatus · 2019-06-20T14:49:33Z

components/authz-service/storage/v2/postgres/postgres.go

@@ -137,7 +137,7 @@ SELECT array_agg(policy_id) FROM pol_ids`,
 func (p *pg) ListPolicies(ctx context.Context) ([]*v2.Policy, error) {
 	projectsFilter, err := projectsListFromContext(ctx)
 	if err != nil {
-		return nil, p.processError(err)
+		return nil, err


one of two drive-by cleanups, please see the commit

srenatus · 2019-06-20T14:49:42Z

components/authz-service/storage/v2/postgres/postgres.go

 	}

 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()

-	tx, err := p.db.BeginTx(ctx, nil)


one of two drive-by cleanups, please see the commit

srenatus · 2019-06-20T14:50:28Z

components/authz-service/storage/v2/postgres/postgres.go

+	return p.listRulesUsingFunction(ctx, "SELECT query_rules($1)")
+}
+
+func (p *pg) ListStagedAndAppliedRules(ctx context.Context) ([]*v2.Rule, error) {


ℹ️ new method for retrieving rules of each state -- as mentioned in A2-815.

srenatus · 2019-06-20T14:51:04Z

components/authz-service/storage/v2/postgres/postgres_test.go

+		db.Flush(t)
+	}
+}
+func TestListStagedAndAppliedRules(t *testing.T) {


The amount of copy-paste involved in creating these tests is deeply unsatisfying :grumpycat:

srenatus · 2019-06-20T14:52:23Z

components/authz-service/storage/v2/postgres/postgres_test.go

 }

-func insertStagedRule(t *testing.T, db *testhelpers.TestDB, rule storage.Rule) {
+func insertStagedRule(t *testing.T, db *testhelpers.TestDB, rule *storage.Rule) {


ℹ️ We have to change the function type to pass a pointer to mutate the rule argument, to add the status below

bcmdarroch · 2019-06-20T15:59:37Z

components/authz-service/storage/postgres/migration/sql/49_add_status_field_to_rules.up.sql

+BEGIN;
+
+CREATE OR REPLACE FUNCTION
+  query_staged_and_applied_rules(_project_filter TEXT[])


does this query concatenate the rows from the two tables?

for GetStagedOrAppliedRule(id) we return the staged rule if it exists, otherwise return the applied rule (due to an acceptance requirement that users should always see the latest staged change). we may want to make this query work the same way - returning only one version of each rule.

although since i don't know of a specific case where we're using this query yet, this is probably ok for now.

A couple points.

Looking at GetStagedOrAppliedRule I do not believe it does what you think, @bcmdarroch . Looks like the SQL underlying that call, query_staged_rule, works exactly the same as this new function, returning both staged and applied rules.

I do take issue with the name of this new function query_staged_and_applied_rules compared to the aforementioned function query_staged_rule. Please rename one or the other so they differ only in plurality. 😁

I think we've resolved that in standup:

returning both staged and applied rules.

but we use QueryRowContext, so we're basically returning the staged one first (if it exists), so we're OK here, even if by accident. There might be a cleaner solution, but this one will do. 😉

query_staged_rules checks both applied and staged but only returns the applied rule if no staged version exists.

bcmdarroch · 2019-06-20T22:00:20Z

components/authz-service/storage/v2/memstore/memstore.go

@@ -243,6 +243,7 @@ func (s *State) GetPolicyChangeNotifier(ctx context.Context) (v2.PolicyChangeNot
 }

 func (s *State) CreateRule(_ context.Context, rule *storage.Rule) (*storage.Rule, error) {
+	rule.Status = "applied"


if we want to match postgres behavior, the status here would be staged. only on ApplyRule does everything staged get set to applied.

Leaving this as-is to not break all the memstore-based tests. Let's nuke them. 😉

tylercloke

Generally looks good to me. I find it a bit strange that we don't just have separate APIs for some of the staged vs applied stuff but that's the general shape we've taken so far so might as well see it through and refactor as needed in the future.

tylercloke · 2019-06-20T23:30:15Z

api/interservice/authz/v2/project.proto

@@ -111,6 +111,8 @@ message ProjectRule {
    string name = 3;
    ProjectRuleTypes type = 4;
    repeated Condition conditions = 5;
+    bool deleted = 6;
+    string status = 7;


I really like the compromise of having an enum at the gateway and not in the domain layer. The enum unwrapping code is super super annoying.

tylercloke · 2019-06-20T23:31:07Z

components/authz-service/server/v2/rules_test.go

@@ -470,6 +471,7 @@ func TestGetRule(t *testing.T) {
 				Type:       api.ProjectRuleTypes_NODE,
 				ProjectId:  projectID,
 				Conditions: apiConditions,
+				Status:     "applied",


Maybe this could be a constant? We have some at the DB level after the DeleteRule refactor PR merged.

yeah a constant in a different package 😉 this is only tests I'll add another package-level one.

tylercloke · 2019-06-20T23:31:50Z

components/authz-service/storage/v2/memstore/memstore.go

@@ -243,6 +243,7 @@ func (s *State) GetPolicyChangeNotifier(ctx context.Context) (v2.PolicyChangeNot
 }


Heh probably not as much WIP as we should. Left a comment in Slack.

msorens · 2019-06-21T04:40:44Z

components/authz-service/storage/postgres/migration/sql/49_add_status_field_to_rules.up.sql

+BEGIN;
+
+CREATE OR REPLACE FUNCTION
+  query_staged_and_applied_rules(_project_filter TEXT[])


A couple points.

Looking at GetStagedOrAppliedRule I do not believe it does what you think, @bcmdarroch . Looks like the SQL underlying that call, query_staged_rule, works exactly the same as this new function, returning both staged and applied rules.

I do take issue with the name of this new function query_staged_and_applied_rules compared to the aforementioned function query_staged_rule. Please rename one or the other so they differ only in plurality. 😁

msorens · 2019-06-21T04:43:32Z

components/authz-service/storage/postgres/migration/sql/49_add_status_field_to_rules.up.sql

+    'name', r.name,
+    'type', r.type,
+    'deleted', r.deleted,
+    'status', 'staged',


Consider SQL constants for 'staged' and 'applied'

Really? As far as I understand it, constants in SQL are scoped per-function. So, this functions could have a constant, but there's not much to gain, really, since both strings only occurr once.... Also it doesn't look common in our code base to have SQL constants. 😃

srenatus · 2019-06-21T12:52:48Z

rebased. will adress review comments later.

…ed to PG Signed-off-by: Stephan Renatus <[email protected]>

If these methods _had_ multiple SQL calls, using a transaction, properly threaded in, would be reasonable. However right now, they both only execute one query, so I've cleaned these up. > Read Committed is the default isolation level in PostgreSQL. When a > transaction uses this isolation level, a SELECT query (without a FOR > UPDATE/SHARE clause) sees only data committed before the query began; it > never sees either uncommitted data or changes committed during query > execution by concurrent transactions. In effect, a SELECT query sees a > snapshot of the database as of the instant the query begins to run. See https://www.postgresql.org/docs/9.1/transaction-iso.html Signed-off-by: Stephan Renatus <[email protected]>

Signed-off-by: Stephan Renatus <[email protected]>

Signed-off-by: Blake Johnson <[email protected]>

srenatus added automate-auth iamv2 This issue or pull request applies to iamv2 work for Automate labels Jun 20, 2019

srenatus self-assigned this Jun 20, 2019

srenatus force-pushed the sr/a2-815/list-rules-with-staged-and-current-rules branch 3 times, most recently from eda06f7 to b617f66 Compare June 20, 2019 13:52

srenatus commented Jun 20, 2019

View reviewed changes

srenatus added the WIP label Jun 20, 2019

bcmdarroch reviewed Jun 20, 2019

View reviewed changes

blakestier approved these changes Jun 20, 2019

View reviewed changes

bcmdarroch reviewed Jun 20, 2019

View reviewed changes

tylercloke reviewed Jun 20, 2019

View reviewed changes

msorens reviewed Jun 21, 2019

View reviewed changes

srenatus force-pushed the sr/a2-815/list-rules-with-staged-and-current-rules branch from b617f66 to 500d2a3 Compare June 21, 2019 12:51

srenatus force-pushed the sr/a2-815/list-rules-with-staged-and-current-rules branch from 500d2a3 to 72969e2 Compare June 21, 2019 17:18

srenatus marked this pull request as ready for review June 21, 2019 17:34

srenatus added 2 commits June 21, 2019 14:50

authz-service/storage/v2: don't call p.processError on errors unrelat…

5b2a918

…ed to PG Signed-off-by: Stephan Renatus <[email protected]>

bcmdarroch approved these changes Jun 21, 2019

View reviewed changes

srenatus and others added 7 commits June 21, 2019 15:13

authz-service/v2/rules: expose status="applied"|"staged"

fc73212

Signed-off-by: Stephan Renatus <[email protected]>

authz-service/v2/rules: expose status="applied"|"staged" [protobuf]

a7f5cfa

Signed-off-by: Stephan Renatus <[email protected]>

authz-service: update migration README.md

c36e11d

Signed-off-by: Stephan Renatus <[email protected]>

review: rename sql function

20977cf

Signed-off-by: Stephan Renatus <[email protected]>

review: add constant "applied" in rules_test.go

888f100

Signed-off-by: Stephan Renatus <[email protected]>

review: add constant "applied" in postgres_test.go

56ea291

Signed-off-by: Stephan Renatus <[email protected]>

Fix rebase misses

b97fd6e

Signed-off-by: Blake Johnson <[email protected]>

blakestier force-pushed the sr/a2-815/list-rules-with-staged-and-current-rules branch from cfe939d to b97fd6e Compare June 21, 2019 22:27

blakestier removed the WIP label Jun 21, 2019

srenatus merged commit ec30f22 into master Jun 24, 2019

chef-ci deleted the sr/a2-815/list-rules-with-staged-and-current-rules branch June 24, 2019 06:28

susanev added the auth-team anything that needs to be on the auth team board label Jul 20, 2019

snyk-bot mentioned this pull request Sep 12, 2021

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#17

Open

ekmixon mentioned this pull request May 17, 2022

[Snyk] Security upgrade css-loader from 0.25.0 to 1.0.0 ekmixon/automate#56

Open

ekmixon mentioned this pull request Dec 20, 2023

[Snyk] Fix for 29 vulnerabilities ekmixon/automate#149

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[A2-815] list rules with staged and current rules #646

[A2-815] list rules with staged and current rules #646

srenatus commented Jun 20, 2019 •

edited by msorens

Loading

srenatus left a comment

srenatus Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

tylercloke Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

srenatus Jun 20, 2019

bcmdarroch Jun 20, 2019

msorens Jun 21, 2019

srenatus Jun 21, 2019

bcmdarroch Jun 21, 2019

bcmdarroch Jun 20, 2019

srenatus Jun 21, 2019

tylercloke left a comment

tylercloke Jun 20, 2019

tylercloke Jun 20, 2019

srenatus Jun 21, 2019

tylercloke Jun 20, 2019

msorens Jun 21, 2019

msorens Jun 21, 2019

srenatus Jun 21, 2019

srenatus commented Jun 21, 2019

		@@ -143,6 +143,7 @@ func TestCreateRule(t *testing.T) {
		Operator: api.ProjectRuleConditionOperators_MEMBER_OF,

		@@ -243,6 +243,7 @@ func (s *State) GetPolicyChangeNotifier(ctx context.Context) (v2.PolicyChangeNot
		}

[A2-815] list rules with staged and current rules #646

[A2-815] list rules with staged and current rules #646

Conversation

srenatus commented Jun 20, 2019 • edited by msorens Loading

🔩 Description

TODO:

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

srenatus 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

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

tylercloke 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

srenatus commented Jun 21, 2019

srenatus commented Jun 20, 2019 •

edited by msorens

Loading