Skip to content
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

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

Merged
merged 9 commits into from
Jun 24, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5b2a918
authz-service/storage/v2: don't call p.processError on errors unrelat…
srenatus Jun 19, 2019
89e1949
authz-service/storage/v2: remove transactions where they're not used
srenatus Jun 19, 2019
fc73212
authz-service/v2/rules: expose status="applied"|"staged"
srenatus Jun 19, 2019
a7f5cfa
authz-service/v2/rules: expose status="applied"|"staged" [protobuf]
srenatus Jun 19, 2019
c36e11d
authz-service: update migration README.md
srenatus Jun 21, 2019
20977cf
review: rename sql function
srenatus Jun 21, 2019
888f100
review: add constant "applied" in rules_test.go
srenatus Jun 21, 2019
56ea291
review: add constant "applied" in postgres_test.go
srenatus Jun 21, 2019
b97fd6e
Fix rebase misses
Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
293 changes: 160 additions & 133 deletions api/interservice/authz/v2/project.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions api/interservice/authz/v2/project.pb.validate.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion api/interservice/authz/v2/project.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ message ProjectRule {
string name = 3;
ProjectRuleTypes type = 4;
repeated Condition conditions = 5;
bool deleted = 6;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

string status = 7;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

enum ProjectRuleTypes {
Expand Down Expand Up @@ -173,7 +175,9 @@ message GetRuleResp {
ProjectRule rule = 1;
}

message ListRulesReq {}
message ListRulesReq {
bool include_staged = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

message ListRulesResp {
repeated ProjectRule rules = 1;
Expand Down
11 changes: 10 additions & 1 deletion components/authz-service/server/v2/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,14 @@ func (s *state) GetRule(ctx context.Context, req *api.GetRuleReq) (*api.GetRuleR
}

func (s *state) ListRules(ctx context.Context, req *api.ListRulesReq) (*api.ListRulesResp, error) {
resp, err := s.store.ListRules(ctx)
if req.IncludeStaged {
return s.listRulesWithFunction(ctx, req, s.store.ListStagedAndAppliedRules)
}
return s.listRulesWithFunction(ctx, req, s.store.ListRules)
}

func (s *state) listRulesWithFunction(ctx context.Context, req *api.ListRulesReq, list func(context.Context) ([]*storage.Rule, error)) (*api.ListRulesResp, error) {
resp, err := list(ctx)
if err != nil {
return nil, status.Errorf(codes.Internal, "error retrieving rules: %s", err.Error())
}
Expand Down Expand Up @@ -491,6 +498,8 @@ func fromStorageRule(r *storage.Rule) (*api.ProjectRule, error) {
Type: t,
ProjectId: r.ProjectID,
Conditions: cs,
Deleted: r.Deleted,
Status: r.Status,
}, nil
}

Expand Down
21 changes: 15 additions & 6 deletions components/authz-service/server/v2/rules_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"github.com/chef/automate/lib/grpc/grpctest"
)

const applied = "applied"

func TestCreateRule(t *testing.T) {
ctx := context.Background()
cl, store, _, _ := setupRules(t)
Expand Down Expand Up @@ -143,6 +145,7 @@ func TestCreateRule(t *testing.T) {
Operator: api.ProjectRuleConditionOperators_MEMBER_OF,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
},
Status: applied,
},
}, resp)
}},
Expand Down Expand Up @@ -470,6 +473,7 @@ func TestGetRule(t *testing.T) {
Type: api.ProjectRuleTypes_NODE,
ProjectId: projectID,
Conditions: apiConditions,
Status: applied,
}

resp, err := cl.GetRule(ctx, &api.GetRuleReq{Id: id})
Expand Down Expand Up @@ -544,13 +548,15 @@ func TestListRules(t *testing.T) {
Type: api.ProjectRuleTypes_NODE,
ProjectId: projectID,
Conditions: apiConditions1,
Status: applied,
}
expected2 := api.ProjectRule{
Id: id2,
Name: name,
Type: api.ProjectRuleTypes_EVENT,
ProjectId: projectID,
Conditions: apiConditions2,
Status: applied,
}
expected := []*api.ProjectRule{&expected1, &expected2}

Expand Down Expand Up @@ -584,9 +590,9 @@ func TestListRulesForProject(t *testing.T) {
}
apiConditions2 := []*api.Condition{
{
Attribute: api.ProjectRuleConditionAttributes_CHEF_ORGS,
Values: []string{"chef"},
Operator: api.ProjectRuleConditionOperators_EQUALS,
Attribute: api.ProjectRuleConditionAttributes_CHEF_ORGS,
Values: []string{"chef"},
Operator: api.ProjectRuleConditionOperators_EQUALS,
},
}
storageConditions2 := []storage.Condition{
Expand All @@ -599,9 +605,9 @@ func TestListRulesForProject(t *testing.T) {
}
apiConditions3 := []*api.Condition{
{
Attribute: api.ProjectRuleConditionAttributes_CHEF_ORGS,
Values: []string{"other", "org"},
Operator: api.ProjectRuleConditionOperators_MEMBER_OF,
Attribute: api.ProjectRuleConditionAttributes_CHEF_ORGS,
Values: []string{"other", "org"},
Operator: api.ProjectRuleConditionOperators_MEMBER_OF,
},
}
storageConditions3 := []storage.Condition{
Expand Down Expand Up @@ -643,13 +649,15 @@ func TestListRulesForProject(t *testing.T) {
Type: api.ProjectRuleTypes_EVENT,
ProjectId: projectID2,
Conditions: apiConditions2,
Status: applied,
}
expected3 := api.ProjectRule{
Id: id3,
Name: name,
Type: api.ProjectRuleTypes_EVENT,
ProjectId: projectID2,
Conditions: apiConditions3,
Status: applied,
}
expected := []*api.ProjectRule{&expected2, &expected3}

Expand Down Expand Up @@ -746,6 +754,7 @@ func addRuleToStore(t *testing.T, store *cache.Cache, id, name string, ruleType
Type: ruleType,
ProjectID: projectID,
Conditions: conditions,
Status: applied,
}
store.Add(id, rule, 0)
}
Expand Down
156 changes: 156 additions & 0 deletions components/authz-service/storage/postgres/migration/sql/50_add_status_field_to_rules.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
BEGIN;

CREATE OR REPLACE FUNCTION
query_staged_and_applied_rules(_project_filter TEXT[])
RETURNS setof json AS $$

-- fetch staged rules
SELECT json_build_object(
'id', r.id,
'project_id', r.project_id,
'name', r.name,
'type', r.type,
'deleted', r.deleted,
'status', 'staged',
'conditions', json_agg(
json_build_object(
'value', rc.value,
'operator', rc.operator,
'attribute', rc.attribute
)
)
) AS rules
FROM iam_staged_project_rules AS r
INNER JOIN iam_staged_rule_conditions AS rc ON rc.rule_db_id=r.db_id
WHERE projects_match_for_rule(project_id, _project_filter)
GROUP BY r.id, r.project_id, r.name, r.type, r.deleted

UNION ALL

-- fetch applied rules
SELECT json_build_object(
'id', r.id,
'project_id', r.project_id,
'name', r.name,
'type', r.type,
'deleted', false,
'status', 'applied',
'conditions', json_agg(
json_build_object(
'value', rc.value,
'operator', rc.operator,
'attribute', rc.attribute
)
)
) AS rules
FROM iam_project_rules AS r
INNER JOIN iam_rule_conditions AS rc ON rc.rule_db_id=r.db_id
WHERE projects_match_for_rule(project_id, _project_filter)
GROUP BY r.id, r.project_id, r.name, r.type;

$$ LANGUAGE sql;

DROP FUNCTION IF EXISTS query_staged_rule(_rule_id TEXT, _project_filter TEXT[]);

CREATE OR REPLACE FUNCTION
query_staged_or_applied_rule(_rule_id TEXT, _project_filter TEXT[])
RETURNS json AS $$

-- check for applied rule
SELECT json_build_object(
'id', r.id,
'project_id', r.project_id,
'name', r.name,
'type', r.type,
'deleted', r.deleted,
'status', 'staged',
'conditions', json_agg(
json_build_object(
'value', rc.value,
'operator', rc.operator,
'attribute', rc.attribute
)
)
) AS rule
FROM iam_staged_project_rules AS r
INNER JOIN iam_staged_rule_conditions AS rc ON rc.rule_db_id=r.db_id
WHERE id=_rule_id AND projects_match_for_rule(project_id, _project_filter)
GROUP BY r.id, r.project_id, r.name, r.type, r.deleted

UNION ALL

-- check for applied rule
SELECT json_build_object(
'id', r.id,
'project_id', r.project_id,
'name', r.name,
'type', r.type,
'deleted', false,
'status', 'applied',
'conditions', json_agg(
json_build_object(
'value', rc.value,
'operator', rc.operator,
'attribute', rc.attribute
)
)
) AS rule
FROM iam_project_rules AS r
INNER JOIN iam_rule_conditions AS rc ON rc.rule_db_id=r.db_id
WHERE id=_rule_id AND projects_match_for_rule(project_id, _project_filter)
GROUP BY r.id, r.project_id, r.name, r.type;

$$ LANGUAGE sql;

CREATE OR REPLACE FUNCTION
query_rules(_project_filter TEXT[])
RETURNS setof json AS $$

SELECT json_build_object(
'id', r.id,
'project_id', r.project_id,
'name', r.name,
'type', r.type,
'status', 'applied',
'conditions', json_agg(
json_build_object(
'value', rc.value,
'operator', rc.operator,
'attribute', rc.attribute
)
)
) AS rule
FROM iam_project_rules AS r
INNER JOIN iam_rule_conditions AS rc ON rc.rule_db_id=r.db_id
WHERE projects_match_for_rule(project_id, _project_filter)
GROUP BY r.id, r.project_id, r.name, r.type;

$$ LANGUAGE sql;

CREATE OR REPLACE FUNCTION
query_rules_for_project(_project_id TEXT, _project_filter TEXT[])
RETURNS setof json AS $$

SELECT json_build_object(
'id', r.id,
'project_id', r.project_id,
'name', r.name,
'type', r.type,
'status', 'applied',
'conditions', json_agg(
json_build_object(
'value', rc.value,
'operator', rc.operator,
'attribute', rc.attribute
)
)
) AS rule
FROM iam_project_rules AS r
INNER JOIN iam_rule_conditions AS rc
ON rc.rule_db_id=r.db_id
WHERE _project_id=project_id AND projects_match_for_rule(project_id, _project_filter)
GROUP BY r.id, r.project_id, r.name, r.type;

$$ LANGUAGE sql;

COMMIT;
1 change: 1 addition & 0 deletions components/authz-service/storage/postgres/migration/sql/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,4 @@
- [`47_staged_rules_tables.up.sql`](47_staged_rules_tables.up.sql)
- [`48_remove_deleted_staged_conditions_column.up.sql`](48_remove_deleted_staged_conditions_column.up.sql)
- [`49_update_rule_triggers.up.sql`](49_update_rule_triggers.up.sql)
- [`50_add_status_field_to_rules.up.sql`](50_add_status_field_to_rules.up.sql)
16 changes: 15 additions & 1 deletion components/authz-service/storage/v2/memstore/memstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ func (s *State) GetPolicyChangeNotifier(ctx context.Context) (v2.PolicyChangeNot
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Memstore adaptations. I anticipate that we all kind of have some of these in our current WIP PRs... :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanna match it as close to postgres as possible, I think we'd want a staged cache and an applied cache. Is that worthwhile? Probably not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err := s.rules.Add(rule.ID, rule, cache.NoExpiration); err != nil {
return nil, storage_errors.ErrConflict
}
Expand Down Expand Up @@ -285,7 +286,7 @@ func (s *State) GetStagedOrAppliedRule(_ context.Context, id string) (*storage.R
return rule, nil
}

func (s *State) ListRules(_ context.Context) ([]*storage.Rule, error) {
func (s *State) ListStagedAndAppliedRules(_ context.Context) ([]*storage.Rule, error) {
items := s.rules.Items()
rules := []*storage.Rule{}

Expand All @@ -298,6 +299,19 @@ func (s *State) ListRules(_ context.Context) ([]*storage.Rule, error) {
return rules, nil
}

func (s *State) ListRules(_ context.Context) ([]*storage.Rule, error) {
items := s.rules.Items()
rules := []*storage.Rule{}

for _, item := range items {
if rule, ok := item.Object.(*storage.Rule); ok && rule.Status == "applied" {
rules = append(rules, rule)
}
}

return rules, nil
}

func (s *State) ListRulesForProject(_ context.Context, projectID string) ([]*storage.Rule, error) {
items := s.rules.Items()
rules := []*storage.Rule{}
Expand Down
Loading