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.

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

Creating audit domain #178

Merged
merged 4 commits into from
Jun 19, 2024
Merged

Conversation

tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Apr 26, 2024

Description

This PR addresses adding the Audit domain to the CLI.

I have some work in progress done, but it needs to be made more generic. The specific audit discovery command needs to be generalised to a audit command that handles all audit events.

We may need to add formatters for specific event types. For example the discovery events have verticies as the GestaltIdEncoded. It may be fine to output that as is. But ideally we'd keep gestaltId formatting consistent, so a node would be NodeIdEncoded and identitiy as identitiyId:providerId.

Issues Fixed

Tasks

  • 1. Create a generic polykey audit command.
  • 2. Support providing multiple event paths in a dot path format a.b.c.
  • 3. Create tests checking functionality.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

Copy link

linear bot commented Apr 26, 2024

@tegefaulkes tegefaulkes force-pushed the feature-eng-303-create-audit-domain branch from ffb3314 to 857d979 Compare April 29, 2024 04:17
@tegefaulkes
Copy link
Contributor Author

I've done most of the work for this already with the audit discovery command. This needs to be generalised into a audit command. Some polish needs to be applied, mainly supporting different option.

One main feature will need to be implemented as well. We need to support selecting multiple event paths at once using a dot path format of a.b, a.b.c etc. This will be provided as a varadic option. Any paths that are duplicate or a subset of parent paths need to be removed from the list before iterating over them.

Simplifying the list is as simple as sorting the paths and removing and duplicate paths or paths that are strictly a sub path. When sorted only the subsequent paths need to be compared.

Once we have a simplified subset of the paths we can create an iterator for each path. Then combine the output of these iterators into a single iterator by comparing the head event of each path and reading them off in the order of the event ID.

@tegefaulkes
Copy link
Contributor Author

I could add the logic for handling multiple event paths to the audit command directly. But I wonder if it's better to apply it to the RPC handler instead. The only issue with that is making changes to the RPC command may break the dashboard. I'll have to co-ordinate that with @amydevs .

@tegefaulkes tegefaulkes self-assigned this May 1, 2024
@CMCDragonkai CMCDragonkai assigned amydevs and unassigned tegefaulkes May 5, 2024
@amydevs
Copy link
Contributor

amydevs commented May 6, 2024

hmm i think multiple paths might be something to consider to add to the actual Audit class rather than just the CLI

@tegefaulkes
Copy link
Contributor Author

I agree, there is too much surface area for problems if the logic was implemented this side of the RPC. The core of the logic can be implemented either side so I'm doing some basic prototyping of it here first before transplanting it to the audit domain.

@amydevs amydevs force-pushed the feature-eng-303-create-audit-domain branch 2 times, most recently from 3a116b1 to 25b525a Compare May 6, 2024 05:47
@CMCDragonkai
Copy link
Member

If this is going to take longer than this cycle, prefer addressing this #181. That directly affects the flow as shown in our demo video.

This happened literally on Friday on the latest staging branch. So it's definitely a problem.

@tegefaulkes tegefaulkes force-pushed the feature-eng-303-create-audit-domain branch from 25b525a to 4b59b09 Compare May 27, 2024 03:12
@tegefaulkes
Copy link
Contributor Author

As part of working on this I'm making changes to the auditEventsGet.ts handler in the Polykey lib. The change is to allow selecting multiple paths when getting events. The problem with this is it will very likely be a breaking change for the dashboard that uses it. It would also be a major version change for the lib.

It's a lot more hassle than it's worth TBH. So as an alternative I can make a separate RPC command for using multiple audit paths when getting audit events. This would make it a minor version change and avoid having to fix up the dashboard.

@tegefaulkes tegefaulkes force-pushed the feature-eng-303-create-audit-domain branch 2 times, most recently from 8856f88 to 0947cc6 Compare May 31, 2024 02:00
@tegefaulkes tegefaulkes marked this pull request as ready for review May 31, 2024 02:00
@tegefaulkes
Copy link
Contributor Author

This is ready for review now.

@tegefaulkes
Copy link
Contributor Author

For the output I have it formatted as the following.

    >	
      id  	06659319332f7000bda6ed4d0f737d12
      path	discovery.vertex.queued
      data	
        vertex	node-A
    >	
      id  	0665931933337000a7059459c3f6ebfd
      path	discovery.vertex.queued
      data	
        vertex	node-B
        parent	node-A
    >	
      id  	066593193337700094c8b217c9a324db
      path	discovery.vertex.queued
      data	
        vertex	node-C
        parent	node-A
    >	
      id  	06659319333b7001a29fbc5ff3008e4a
      path	discovery.vertex.queued
      data	
        vertex	node-D
        parent	node-B

I find having a semi-empty space between each entry is much easier to read. However I think the standard approach is to have it like this?

    id  	0665931f14c470008512455a5736c54a
    path	discovery.vertex.queued
    data	
      vertex	node-A
    id  	0665931f14d070008fdc80ad92a46dd0
    path	discovery.vertex.queued
    data	
      vertex	node-B
      parent	node-A
    id  	0665931f14d0700189600c8ed5c8a8b5
    path	discovery.vertex.queued
    data	
      vertex	node-C
      parent	node-A
    id  	0665931f14d47001ae9bead8f8915880
    path	discovery.vertex.queued
    data	
      vertex	node-D
      parent	node-B

Which I personally really dislike. But I'll change it if that's the intended format.

@tegefaulkes
Copy link
Contributor Author

I also need to note that this PR is blocked by infrastructure prep work that @brynblack should be working on. I'll need feedback from her on that progress.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented May 31, 2024 via email

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Jun 3, 2024

The Polykey PR has some details for what needs to be done. Here are some links for reference @amydevs @brynblack

MatrixAI/Polykey#730 - Generally relevant.
MatrixAI/Polykey#730 (comment) - Main details for what needs to be done.

@tegefaulkes tegefaulkes force-pushed the feature-eng-303-create-audit-domain branch from 4a13392 to 2ce77a5 Compare June 18, 2024 07:13
@tegefaulkes
Copy link
Contributor Author

This is fully prepped now.

@tegefaulkes tegefaulkes merged commit 0485b67 into staging Jun 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Audit CLI commands for inspecting audit event history
3 participants