Skip to content

Refactor streaming audit headers

Michael Beckerrequested to merge
feat/366350 into master

What does this MR do and why?

This change is prep work for issues 366350

A draft of how these services will be used can be seen here

We want to emit audit events whenever the custom streaming event headers are creating/updated/destroyed

We have decided to refactor the existing logic into service classes, and then add then add the actual audit events using this method.

I put the services under ee/app/services because the custom http header feature is only available in EE gitlab

This first commit is only a refactor, so the existing specs have not been touched (*sort of) and the actual audit event work is not included in this commit

How to set up and validate locally

  1. ci should pass
  2. the steps from !88063 (merged) should still work (copied below)
  • Enable the feature flag (streaming_audit_event_headers)
  • Ensure you have a valid ultimate licence.
  • Inside a group, go to Security & Compliance -> Audit Events -> Streams
  • Create a new streaming destination. (Requestbin is always good for this)
  • Use the new mutation to create a header:
mutation {
  auditEventsStreamingHeadersCreate(input: { destinationId: "gid://gitlab/AuditEvents::ExternalAuditEventDestination/DESTINATION_ID_HERE", key: "X-Custom-Header-Tanuki", value: "EveryoneCanContribute" }) {
    errors
    clientMutationId
  }
}
  • Perform an auditable task. (Add a user to a project, create a project, etc.)
  • In the streamed event payload, ensure that the list of headers contains the newly created header.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

a note on the spec change

Ideally, in a refactor one should not touch existing specs at all.

However, this spec file started failing due to the initial key being incorrect in spec expectations:

Failures:

  1) Update an external audit event destination header when feature is licensed when current user is a group owner updates the header with the correct attributes
     Failure/Error:
       expect { subject }.to change { header.reload.key }.from('key-1').to('new-key')
                                                         .and change { header.reload.value }.from('bar')
                                                                                            .to('new-value')

       expected `header.reload.key` to have initially been "key-1", but was "key-3"
     # ./ee/spec/requests/api/graphql/audit_events/streaming/headers/update_spec.rb:56:in `block (4 levels) in <main>'
     ...

I think it is because I added additional usages of the header factory in some new specs, and therefore these fail because the factory uses an incrementing sequence

some supporting evidence for this is that if you run a specific spec, the test will pass. It only fails if you run the whole directory (example below)

\textcolor{red}{\text{Though, this explanation isn't entirely satisfying due to the fact that the spec fails **without me running my newly added specs**}}:

bundle exec rspec ./ee/spec/requests/api/graphql/audit_events/streaming/headers/*

so if any reviewers see something I am missing it would be much appreciated 😊

Edited by Michael Becker

Merge request reports