[A2-802] CreateRule projects API #410

srenatus · 2019-05-23T17:05:00Z

🔩 Description

Adds the CreateRule GRPC method to authz-service, wrapping its storage's CreateRule.

Items on Note

This includes some property-based tests in rules_property_test.go, where we encode what we want the system to do, and what "random inputs" look like. Then, the gopter library takes care of generating 100 inputs, and asserting the property. If it fails to hold, the library will attempt to shrink the input into a manageable test case (as anything random can be unwieldy).

👍 Definition of Done

you can create a rule from chef-automate dev grpcurl authz-service
the authz-service method is exposed via automate-gateway
expose operator (it's currently only "member-of" but it should be in the API) -- ❓ need input there, I'm missing context on the decision

👟 Demo Script / Repro Steps

start_all_services
rebuild components/authz-service, rebuild components/automate-gateway
create a rule: (hab pkg install -b core/jq-static for jq)

[121][default:/src:130]# curl -kH "api-token: $TOK" https://localhost/apis/iam/v2beta/rules -d "$(jq -n '{ id: "foo-rule", name: "my foo rule", type: "NODE", project_id: "foo-project", conditions: [{ type: "CHEF_SERVERS", values: ["prod", "staging"]}]}')"
{"error":"error creating rule with ID \"foo-rule\": foreign key violation","message":"error creating rule with ID \"foo-rule\": foreign key violation","code":13,"details":[]}[122][default:/src:0]#

note that this naive attempt failed -- the project doesn't exist -- and the error messaging could be better ⚠️
try again, creating the project first:

[122][default:/src:0]# curl -kH "api-token: $TOK" https://localhost/apis/iam/v2beta/projects -d "$(jq -n '{ id: "foo-project", name: "my foo project" }')"
{"project":{"name":"my foo project","id":"foo-project","type":"CUSTOM","projects":["foo-project"]}}
[123][default:/src:0]# curl -kH "api-token: $TOK" https://localhost/apis/iam/v2beta/rules -d "$(jq -n '{ id: "foo-rule", name: "my foo rule", type: "NODE", project_id: "foo-project", conditions: [{ type: "CHEF_SERVERS", values: ["prod", "staging"]}]}')" | jq .
{
  "rule": {
    "id": "foo3-rule",
    "project_id": "foo-project",
    "name": "my foo rule",
    "type": "NODE",
    "conditions": [
      {
        "type": "CHEF_SERVERS",
        "values": [
          "prod",
          "staging"
        ]
      }
    ]
  }
}

check the database that it was properly created:

[129][default:/src:113]# chef-automate dev psql chef_authz_service
psql (9.6.11)
SSL connection (protocol: TLSv1.2, cipher: ECDHE-RSA-AES256-GCM-SHA384, bits: 256, compression: off)
Type "help" for help.

chef_authz_service=# select * from iam_project_rules where id='foo-rule';
 db_id |    id    | project_id  |    name     | type
-------+----------+-------------+-------------+------
     3 | foo-rule | foo-project | my foo rule | node
(1 row)
chef_authz_service=# select * from iam_rule_conditions where rule_db_id=3;
 db_id | rule_db_id |     value      |  attribute  | operator
-------+------------+----------------+-------------+-----------
     1 |          3 | {prod,staging} | chef-server | member-of
(1 row)

⛓️ Related Resources

A2-650
- A2-802 👈

✅ Checklist

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

bcmdarroch

Had a few questions, looks good!

bcmdarroch · 2019-05-28T18:26:52Z

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

+}
+
+var apiToStorageConditionAttributes map[api.ProjectRuleConditionTypes]storage.ConditionAttribute
+var onceReverseConditionAttributesMapping sync.Once


what does sync.Once do?

https://golang.org/pkg/sync/#Once It's being used to populate the map apiToStorageConditionAttributes into memory exactly one time. Once will ensure it's only populated once instead of on every call so it's just cached.

Exactly! It's the lazy way for not having to maintain both mappings manually. Only A -> B is hardcoded, B -> A is initialized once in the way @tylercloke described.

bcmdarroch · 2019-05-28T18:33:42Z

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

+		"Conditions": gen.SliceOf(conditionsGen),
+	})
+
+	params := gopter.DefaultTestParametersWithSeed(seed)


what does gopter do for us?

The readme isn't the best, but it looks like it can be used to generate a bunch of random structs for tested based on possible values for the structs. What it looks like we are using it for here is having it generate lots of different combinations of all the conditions possibilities (since we have a lot of different condition types) so we can test CreateRule against many possible different condition inputs without manually managing how they are generated.

bcmdarroch · 2019-05-28T18:37:55Z

components/automate-gateway/api/iam/v2beta/common/rules.proto

+package chef.automate.api.iam.v2beta;
+option go_package = "github.com/chef/automate/components/automate-gateway/api/iam/v2beta/common";
+
+enum RuleType {


thanks for splitting this out! we probably should do the same for roles one of these days

tylercloke

Use of gopter looks cool for cases where we have a large set of possible inputs to our APIs!

tylercloke · 2019-05-28T21:57:56Z

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

+}
+
+var apiToStorageConditionAttributes map[api.ProjectRuleConditionTypes]storage.ConditionAttribute
+var onceReverseConditionAttributesMapping sync.Once


https://golang.org/pkg/sync/#Once It's being used to populate the map apiToStorageConditionAttributes into memory exactly one time. Once will ensure it's only populated once instead of on every call so it's just cached.

tylercloke · 2019-05-28T22:04:11Z

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

+		"Conditions": gen.SliceOf(conditionsGen),
+	})
+
+	params := gopter.DefaultTestParametersWithSeed(seed)


The readme isn't the best, but it looks like it can be used to generate a bunch of random structs for tested based on possible values for the structs. What it looks like we are using it for here is having it generate lots of different combinations of all the conditions possibilities (since we have a lot of different condition types) so we can test CreateRule against many possible different condition inputs without manually managing how they are generated.

msorens

Nice work; some comments and questions below.

msorens · 2019-05-29T03:55:34Z

api/interservice/authz/v2/project.proto

@@ -125,3 +132,16 @@ message Condition {
    ProjectRuleConditionTypes type = 1;
    repeated string values = 2;
 }
+
+// CreateRuleReq subsumes ProjectRule, adding id/project/name


Maybe it did, but looks like it is now an out-of-date comment. Perhaps "CreateRuleReq mirrors ProjectRule, adding validation" ?

msorens · 2019-05-29T04:06:37Z

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

@@ -371,9 +371,14 @@ func addProjectToStore(t *testing.T, store *cache.Cache, id, name string, projTy
 }

 func setupProjects(t *testing.T) (api.ProjectsClient, *cache.Cache, *mockEventServiceClient) {
+	cl, ca, _, mc, _ := setupProjectsAndRules(t)
+	return cl, ca, mc
+}


Add blank line after this

msorens · 2019-05-29T04:08:13Z

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

+	cl, ca, _, mc, _ := setupProjectsAndRules(t)
+	return cl, ca, mc
+}
+func setupProjectsAndRules(t *testing.T) (api.ProjectsClient, *cache.Cache, *cache.Cache, *mockEventServiceClient,


Odd place to break the line; consider:

func setupProjectsAndRules(t *testing.T) ( api.ProjectsClient, *cache.Cache, *cache.Cache, *mockEventServiceClient, int64) {

msorens · 2019-05-29T04:08:56Z

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

 	t.Helper()
 	ctx := context.Background()
-	prng.Seed(t)
+	seed := prng.GenSeed(t)


Why the seeding change?

because it's controlling randomness in gopter

msorens · 2019-05-29T04:10:24Z

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

@@ -371,9 +371,14 @@ func addProjectToStore(t *testing.T, store *cache.Cache, id, name string, projTy
 }

 func setupProjects(t *testing.T) (api.ProjectsClient, *cache.Cache, *mockEventServiceClient) {
+	cl, ca, _, mc, _ := setupProjectsAndRules(t)


Why does setupProjectsAndRules return arguments that are never used?

Not used for setupProjects. They are used for setupRules. 🤷‍♂ We could clean this up, I suppose. But I don't find it that terrible.

Ah, still used to thinking about private to namespace means private to file, but not true in Go. 👍

msorens · 2019-05-29T04:29:25Z

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

+						},
+					},
+				},
+			}, resp) // FIXME


What needs fixing?

msorens · 2019-05-29T04:30:15Z

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

+		}},
+		// happy path
+		{"with valid rule data, returns no error and creates the rule in storage", func(t *testing.T) {
+			resp, err := cl.CreateRule(ctx, &api.CreateRuleReq{


Was curious whether this reduced form would work, but it fails. Any thoughts on why?

ruleReq := &api.CreateRuleReq{ Id: "any-name", Name: "any name", ProjectId: "foo", Type: api.ProjectRuleTypes_NODE, Conditions: []*api.Condition{ { Type: api.ProjectRuleConditionTypes_CHEF_ORGS, Values: []string{"chef"}, }, }, } resp, err := cl.CreateRule(ctx, ruleReq) assert.NoError(t, err) rule := api.ProjectRule(*ruleReq) assert.Equal(t, &api.CreateRuleResp{ Rule: &rule, }, resp)

Didn't dive in deeper, but with this:

diff --git a/components/authz-service/server/v2/rules_test.go b/components/authz-service/server/v2/rules_test.go index 08a82b6c..c1be18cf 100644 --- a/components/authz-service/server/v2/rules_test.go +++ b/components/authz-service/server/v2/rules_test.go @@ -5,6 +5,7 @@ import ( "math/rand" "testing" + "github.com/kylelemons/godebug/pretty" cache "github.com/patrickmn/go-cache" "github.com/stretchr/testify/assert" "google.golang.org/grpc/codes" @@ -103,7 +104,8 @@ func TestCreateRule(t *testing.T) { }}, // happy path {"with valid rule data, returns no error and creates the rule in storage", func(t *testing.T) { - resp, err := cl.CreateRule(ctx, &api.CreateRuleReq{ + + ruleReq := &api.CreateRuleReq{ Id: "any-name", Name: "any name", ProjectId: "foo", @@ -114,22 +116,16 @@ func TestCreateRule(t *testing.T) { Values: []string{"chef"}, }, }, - }) + } + actual, err := cl.CreateRule(ctx, ruleReq) assert.NoError(t, err) - assert.Equal(t, &api.CreateRuleResp{ - Rule: &api.ProjectRule{ - Id: "any-name", - Name: "any name", - ProjectId: "foo", - Type: api.ProjectRuleTypes_NODE, - Conditions: []*api.Condition{ - { - Type: api.ProjectRuleConditionTypes_CHEF_ORGS, - Values: []string{"chef"}, - }, - }, - }, - }, resp) + rule := api.ProjectRule(*ruleReq) + expected := &api.CreateRuleResp{ + Rule: &rule, + } + if !assert.Equal(t, expected, actual) { + t.Log(pretty.Compare(expected, actual)) + } }}, }

...we get:

--- FAIL: TestCreateRule/with_valid_rule_data,_returns_no_error_and_creates_the_rule_in_storage (0.00s) Error Trace: rules_test.go:126 Error: Not equal: expected: &v2.CreateRuleResp{Rule:(*v2.ProjectRule)(0xc000192690), XXX_NoUnkeyedLiteral:struct {}{}, XXX_unrecognized:[]uint8(nil), XXX_sizecache:0} actual : &v2.CreateRuleResp{Rule:(*v2.ProjectRule)(0xc000192230), XXX_NoUnkeyedLiteral:struct {}{}, XXX_unrecognized:[]uint8(nil), XXX_sizecache:0} Diff: Test: TestCreateRule/with_valid_rule_data,_returns_no_error_and_creates_the_rule_in_storage rules_test.go:127: { Rule: { Id: "any-name", ProjectId: "foo", Name: "any name", Type: 0, Conditions: [ { Type: 1, Values: [ "chef", ], XXX_NoUnkeyedLiteral: { }, XXX_unrecognized: [ ], - XXX_sizecache: 8, + XXX_sizecache: 0, }, ], XXX_NoUnkeyedLiteral: { }, XXX_unrecognized: [ ], - XXX_sizecache: 35, + XXX_sizecache: 0, }, XXX_NoUnkeyedLiteral: { }, XXX_unrecognized: [ ], XXX_sizecache: 0, }

So, the trivial answer is: the XXX_sizecache fields don't match. 😉

Hmm... I did not see the expanded diff when I ran it, just this. Any idea why my output is different?

expected: &v2.CreateRuleResp{Rule:(*v2.ProjectRule)(0xc0001930a0), XXX_NoUnkeyedLiteral:struct {}{}, XXX_unrecognized:[]uint8(nil), XXX_sizecache:0} actual : &v2.CreateRuleResp{Rule:(*v2.ProjectRule)(0xc000192bd0), XXX_NoUnkeyedLiteral:struct {}{}, XXX_unrecognized:[]uint8(nil), XXX_sizecache:0} Diff:

For more information please reread -- my comment includes the diff of what I had to do to see that, too ☝️

🤔 Except it does not include the actual command; "diff --git..." is output... Am I missing something...?

Sorry that was the output of git diff.

msorens · 2019-05-29T04:33:59Z

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

+	"reflect"
+	"testing"
+
+	"github.com/leanovate/gopter"


We have used one data generator in our code base already, https://github.com/jaswdr/faker. Is gopter more useful?

Is gopter more useful?

100 times yes -- since it's not just a data generator. Back when we've introduced faker, you might remember our conversations (also with @phiggins, https://github.com/chef/a2/pull/4294 and previous gopter usage introduced earlier: https://github.com/chef/a2/pull/4369): fake data is nice for tests, but the icing on the cake is property based tests. This is what gopter gives us: a way to state properties, and assert them using a large battery of random inputs.

I.e. instead of

generate one random policy, see that it can be created

we formulate a property

when CreateRule is called with some input, it will lead to a rule in storage

and have the framework try that a hundred times with different inputs.

Also, any decent PBT framework gives you shrinking: when it finds an input such that the property not holds, it attempts to find the smallest input that does the same thing. It's vital for dealing with random inputs.

Now, this doesn't mean what we don't need unit tests. And it also doesn't mean that the way we're using it here is the one and only correct way.

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

This isn't new, it just hadn't been mentioned before. While we're at it, I've selected the latest version, 0.2.4. Signed-off-by: Stephan Renatus <[email protected]>

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

It's also a property that doesn't hold -- the ProjectRule type limits the condition types. This needs to be encoded in the generators, too. Signed-off-by: Stephan Renatus <[email protected]>

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

Felt better not to pile this onto the existing Policies service. Signed-off-by: Stephan Renatus <[email protected]>

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

This is so tedious. It's almost, but not exactly, the same code as that written in authz-service, translating from its GRPC service to its internal storage interface (and back). One thing to note: we're using an extra ENUM value in our protobuf definitions in the gateway API. It's a way to ensure that the enum is explicitly provided: if it's left off, the request message with not have the type that happens to be the first enum name, but an "unset" enum that allows discrimination. (Wrapping the enum could help, too, but this is neat and tidy and straightforward.) Signed-off-by: Stephan Renatus <[email protected]>

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

srenatus added WIP automate-auth iamv2 This issue or pull request applies to iamv2 work for Automate labels May 23, 2019

srenatus self-assigned this May 23, 2019

srenatus force-pushed the sr/a2-802/create-project-rule-api branch 2 times, most recently from 221b135 to e8d8281 Compare May 24, 2019 13:16

srenatus added WIP and removed WIP labels May 24, 2019

srenatus force-pushed the sr/a2-802/create-project-rule-api branch 2 times, most recently from 3f06bf0 to dacc4e6 Compare May 27, 2019 14:42

srenatus added WIP and removed WIP labels May 27, 2019

srenatus force-pushed the sr/a2-802/create-project-rule-api branch from dacc4e6 to 659f032 Compare May 28, 2019 11:50

srenatus removed the WIP label May 28, 2019

bcmdarroch approved these changes May 28, 2019

View reviewed changes

tylercloke approved these changes May 28, 2019

View reviewed changes

bcmdarroch mentioned this pull request May 28, 2019

[A2-806] GetRule + ListRules projects API #430

Merged

6 tasks

msorens approved these changes May 29, 2019

View reviewed changes

srenatus force-pushed the sr/a2-802/create-project-rule-api branch 2 times, most recently from fe6a7d9 to 9b87569 Compare May 29, 2019 07:48

srenatus added 10 commits May 29, 2019 15:59

authz-service/api: add CreateRule

4179667

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

authz-service/api: lots of silly code for CreateRule

e458b76

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

authz-service/api: add CreateRule [protobuf]

da74711

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

authz-service/CreateRule: avoid strings, add enum mappings

8ef3142

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

grpctest/status: s/NotNil/Error/

d8edd4d

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

authz-service/v2/CreateRule: start tests

455b3c6

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

authz-service/api: add input validation to CreateRule

8d800b9

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

authz-service/api: add input validation to CreateRule [regenerate]

1ca0728

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

authz-service/CreateRule: the tests now test API validation properly

eceb023

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

Gopkg.toml: add gopter

8cc7783

This isn't new, it just hadn't been mentioned before. While we're at it, I've selected the latest version, 0.2.4. Signed-off-by: Stephan Renatus <[email protected]>

srenatus added 13 commits May 29, 2019 15:59

Gopkg.toml: add gopter [revendor]

753c066

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

rules_test: cleanup test helper

b2e3367

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

rules_property_test: start naiive property-based test for CreateRule

4bdb0bb

It's also a property that doesn't hold -- the ProjectRule type limits the condition types. This needs to be encoded in the generators, too. Signed-off-by: Stephan Renatus <[email protected]>

rules_property_test: make this work for events

e1ef8ef

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

grpctest/status: make it return true if the assertion was met

5993e48

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

rules_property_test: add second property

f149fae

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

automate-gateway/api: add CreateRule, Rules service

eb7a7fd

Felt better not to pile this onto the existing Policies service. Signed-off-by: Stephan Renatus <[email protected]>

automate-gateway/api: add CreateRule, Rules service [regenerate]

efccc68

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

automate-gateway: wire /apis/iam/v2beta/rules in properly

620c8d1

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

api/interservice/project: fix comment

33abf90

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

api/interservice/project: fix comment [regenerate]

fb09b4e

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

authz-service/project_test: fix newlines

429afec

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

srenatus force-pushed the sr/a2-802/create-project-rule-api branch from 9b87569 to 429afec Compare May 29, 2019 13:59

srenatus merged commit 717e5f1 into master May 29, 2019

chef-ci deleted the sr/a2-802/create-project-rule-api branch May 29, 2019 15:01

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

ekmixon mentioned this pull request Dec 1, 2023

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

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-802] CreateRule projects API #410

[A2-802] CreateRule projects API #410

srenatus commented May 23, 2019 •

edited

Loading

bcmdarroch left a comment

bcmdarroch May 28, 2019

tylercloke May 28, 2019

srenatus May 29, 2019

bcmdarroch May 28, 2019

tylercloke May 28, 2019 •

edited

Loading

bcmdarroch May 28, 2019

tylercloke left a comment

tylercloke May 28, 2019

tylercloke May 28, 2019 •

edited

Loading

msorens left a comment

msorens May 29, 2019

msorens May 29, 2019

msorens May 29, 2019

msorens May 29, 2019

srenatus May 29, 2019

msorens May 29, 2019

srenatus May 29, 2019

msorens May 29, 2019

msorens May 29, 2019

msorens May 29, 2019

srenatus May 29, 2019

msorens May 29, 2019

srenatus May 29, 2019

msorens May 29, 2019

srenatus May 30, 2019

msorens May 29, 2019

srenatus May 29, 2019

[A2-802] CreateRule projects API #410

[A2-802] CreateRule projects API #410

Conversation

srenatus commented May 23, 2019 • edited Loading

🔩 Description

Items on Note

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

bcmdarroch 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

tylercloke May 28, 2019 • edited Loading

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

tylercloke May 28, 2019 • edited Loading

Choose a reason for hiding this comment

msorens 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

srenatus commented May 23, 2019 •

edited

Loading

tylercloke May 28, 2019 •

edited

Loading

tylercloke May 28, 2019 •

edited

Loading