xds: E2E Test for Audit Logging #6377
Conversation
| @@ -605,6 +630,12 @@ func (s) TestRBACHTTPFilter(t *testing.T) { | |||
| for _, test := range tests { | |||
| t.Run(test.name, func(t *testing.T) { | |||
| func() { | |||
There was a problem hiding this comment.
This is not new but do you know why such a anonymous func is needed here?
There was a problem hiding this comment.
No clue, I'm just working into the existing test - at a glance it does seem unnecessary
| @@ -660,6 +693,17 @@ func (s) TestRBACHTTPFilter(t *testing.T) { | |||
| } | |||
| // Toggle RBAC back on for next iterations. | |||
| envconfig.XDSRBAC = true | |||
There was a problem hiding this comment.
I think it's worth considering adding some check to verify audit logging is not active till RBAC is toggled back.
There was a problem hiding this comment.
I think it ends up being somewhat implicit in the test - it makes 2 calls with RBAC on, then 2 calls with RBAC off, and the expected outcome for logging is allowed: 2, denied: 0
There was a problem hiding this comment.
Agree that this isn't the best as it requires holding some test state in your head as you look at it, but I hesitate to make large changes to this existing test
This PR adds testing for audit logging logic to the existing RBAC tests. It very closely matches the tests for the authz path - #6304
I've currently only added a very basic test of the audit logger - the purpose here is to make sure that we can construct an audit logger through the xds path and it works.
I'm open to adding test cases for the various combinations of audit logging and authorization policies, but I think that's not really what we are testing here, and it's a pretty explosive combination and pollutes the test (if it's not necessary). All these cases should be covered in other tests like the unit tests and the authz path tests. @dfawley @easwars what do you think? I see compelling reasons for either choice.
RELEASE NOTES: N/A