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

Jump to bottom

authz: End2End test for AuditLogger #6304

Merged
merged 23 commits into from Jun 1, 2023
Merged

authz: End2End test for AuditLogger #6304

Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5778913
Draft of e2e test
erm-g May 18, 2023
cebf932
No Audit, Audit on Allow and Deny
erm-g May 18, 2023
d405ab2
Audit on Allow, Audit on Deny
erm-g May 19, 2023
0baf0e6
fix typo
erm-g May 19, 2023
b569c67
SPIFFE related testing
erm-g May 22, 2023
135565a
SPIFFE Id validation and certs creation script
erm-g May 22, 2023
c19e304
Address PR comments
erm-g May 23, 2023
1e7fcc4
Wrap tests using grpctest.Tester
erm-g May 23, 2023
e5991f2
Address PR comments
erm-g May 23, 2023
620d990
Change package name to authz_test to fit other end2end tests
erm-g May 23, 2023
cba398c
Add licence header, remove SPIFFE slice
erm-g May 24, 2023
de59ce5
Licence year change
erm-g May 24, 2023
9a8ee49
Address PR comments part 1
erm-g May 26, 2023
36bc552
Address PR comments part 2
erm-g May 26, 2023
b255385
Address PR comments part 3
erm-g May 26, 2023
7777c5b
Address PR comments final part
erm-g May 29, 2023
191fdf6
Drop newline for a brace
erm-g May 29, 2023
80d38b4
Address PR comments, fix outdated function comment
erm-g May 30, 2023
46fda00
Address PR comments
erm-g May 31, 2023
a02960d
Fix typo
erm-g May 31, 2023
56eba6e
Remove unused var
erm-g May 31, 2023
cb01a30
Address PR comment, change most test error handling to Errorf
erm-g Jun 1, 2023
4f30f15
Address PR comments
erm-g Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions authz/audit/audit_logging_test.go
View file
Open in desktop
Expand Up @@ -289,12 +289,15 @@ func (s) TestAuditLogger(t *testing.T) {
defer cancel()

if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantUnaryCallCode {
t.Fatalf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode)
t.Errorf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode)
}
if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantUnaryCallCode {
t.Fatalf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode)
t.Errorf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode)
}
stream, err := client.StreamingInputCall(ctx)
if err != nil {
t.Errorf("StreamingInputCall failed:%v", err)
Copy link
Member

Choose a reason for hiding this comment

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

This one needs to be Fatal because using the stream is illegal.

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 need some help here - go/go-style/decisions.md?cl=head#keep-going suggests to prefer Error over Fatal

to print out all of the failed checks in a single run

So for this test even if stream is not usable I can still proceed with unary calls to get the test results for it.
However the same doc suggests

Calling t.Fatal is primarily useful for reporting an unexpected error condition

and unusable stream is definitely something we don't expect.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

True, Error is nicer, generally, but this is a precondition for (some of) the rest of the test. The "ideal" would be to skip just the streaming calls and the checks that depend on them happening, but that really clutters the code and isn't worth it at all. Just Fatal since a precondition failed and keep things simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sounds good - I changed Errorf to be used for

  • cmp.Diff cases
  • errors which status.Code is not expected

}
stream, _ := client.StreamingInputCall(ctx)
req := &testpb.StreamingInputCallRequest{
Payload: &testpb.Payload{
Body: []byte("hi"),
Expand All @@ -304,18 +307,18 @@ func (s) TestAuditLogger(t *testing.T) {
t.Errorf("stream.Send failed:%v", err)
}
if _, err := stream.CloseAndRecv(); status.Code(err) != test.wantStreamingCallCode {
t.Fatalf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantStreamingCallCode)
t.Errorf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantStreamingCallCode)
}

// Compare expected number of allows/denies with content of the internal
// map of statAuditLogger.
if diff := cmp.Diff(lb.authzDecisionStat, test.wantAuthzOutcomes); diff != "" {
t.Fatalf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff)
t.Errorf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff)
}
// Compare last event received by statAuditLogger with expected event.
if test.eventContent != nil {
if diff := cmp.Diff(lb.lastEvent, test.eventContent); diff != "" {
t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff)
t.Errorf("Unexpected message\ndiff (-got +want):\n%s", diff)
}
}
})
Expand Down