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

Adding multiple path support when requesting audit events #730

Merged
merged 1 commit into from
May 30, 2024

Conversation

tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented May 29, 2024

Description

This PR adds the auditEventMultiPathGet handler for getting audit events while specifying multiple event paths. The paths are specified in dot path format a.b.c. and filtered down to the minimum common paths in the list. Multiple iterators are created and combined when returning the stream.

Issues Fixed

Tasks

  1. Create auditEventMultiPathGet handler for efficiently getting audit events while combining multiple audit paths.
  2. Expand tests to check for properly sorted order in ascending and descending.

Final checklist

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

@tegefaulkes tegefaulkes force-pushed the feature-audit-multipath branch from 75655ee to 4705bc9 Compare May 29, 2024 04:05
src/audit/utils.ts Outdated Show resolved Hide resolved
src/audit/utils.ts Outdated Show resolved Hide resolved
src/audit/utils.ts Show resolved Hide resolved
src/client/handlers/AuditEventsMultiPathGet.ts Outdated Show resolved Hide resolved
@tegefaulkes
Copy link
Contributor Author

Change of plans, the functionality of auditEventMultiPathGet is getting rolled into auditEventGet. We're going with the hard breaking change.

@CMCDragonkai
Copy link
Member

Don't change your plans yet, investigate the total cost of the change - scope it out and then decide.

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented May 29, 2024

Yeah, it seems like a 1 line change https://github.com/MatrixAI/Polykey-Network-Status/blob/dea74578e15d4d3402f568204c65a03cb6670ce9/src/routes/api/nodes/index.ts#L38C1-L43C14 for Polykey-Network-Status. path: ['node', 'connection'], will need to be changed to paths: ['node.connection'],

I believe Polykey-Network-Dashboard will also need a dependency update for this change as well.

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented May 29, 2024

The audit domain does a lot of type magic. While it's impressive it's very hard to follow what's happening and why. It's really cool that the output of the getAuditEvents is narrowed by the TopicPath used. However if we wanted to tell the difference between two events then we'd have to do further narrowing anyway.

It would be a lot cleaner to have all the event types extend a generic base auditEvent type and preform further narrowing based on the event path or a type parameter.

I'm not sure this is a change I want to make in the scope of this PR however. That said, We're already making changes that have downstream affects, so it might be worth looking at.

@tegefaulkes
Copy link
Contributor Author

Ok, the changes made by auditEventsMultiPathGet has been merged into auditEventsGet. Note that the changes to auditEventsGet WILL break the dashboard until that is updated.

Also note that the auditEventsGet had some type magic applied to it to automatically set it's output type based on the topicPath provided to it. This is incompatible with the multi-path changes, so it had to be removed. auditEventsGet will return just a generic AuditEventSerialized. You'll have to narrow this down by verifying the specific audit event it is.

…t events while selecting multiple paths

[ci skip]
@tegefaulkes tegefaulkes force-pushed the feature-audit-multipath branch from 3342b63 to c1669b6 Compare May 29, 2024 07:00
@tegefaulkes
Copy link
Contributor Author

Ready to merge. Unless we want to look into simplifying how all the types work but that can be a new PR.

@CMCDragonkai
Copy link
Member

Yeah, it seems like a 1 line change https://github.com/MatrixAI/Polykey-Network-Status/blob/dea74578e15d4d3402f568204c65a03cb6670ce9/src/routes/api/nodes/index.ts#L38C1-L43C14 for Polykey-Network-Status. path: ['node', 'connection'], will need to be changed to paths: ['node.connection'],

I believe Polykey-Network-Dashboard will also need a dependency update for this change as well.

Make sure @brynblack is prepared for this changeover. Spec out the procedure and review with @amydevs to document this zero downtime update.

Copy link

linear bot commented May 30, 2024

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented May 30, 2024

Let's see, I'll need @amydevs input on the scope of what is affected in the dashboard. I think @brynblack will be coordinating the infrastructure changes needed for this as well.

At first blush it seems that PKNS needs to be updated to use the new API for the auditEventsGet RPC call. This is two parts

  1. The path parameter is now paths: Array<Array<string>>. That part is simple.
  2. The output from the RPC call was simplified to AuditEvent without inferring the sub type. So you'll need to narrow the event down the the expected connection event when processing the output. This should be doable via the path parameter of the event.

I think PKND depends on the PKNS So that will need to be updated as well. On top of that, the seed nodes will need to have the new version of the CLI with the changes to support it. Only the seed nodes need the update since only ask them for connection events.

So the minimal downtime procedure here is...

  1. Merge in the audit domain changes with the lib dependency update for them.
  2. Prepare PRs for PKNS and PKND that updates to handle the API changes.
  3. Do a new CLI release, this should automatically update the seed nodes with the new changes.
  4. Merge the PKNS and PKND changes, this should automatically update the dashboard with the new changes.

I think that covers everything? Any input @amydevs ?

We'll need a new issue for coordinating this, but I'm unsure where. Also, I can proceed with the audit domain in CLI and merge that. But a new CLI version is blocked by prep work for the dashboard supporting it.

@tegefaulkes
Copy link
Contributor Author

I'm merging this now, but MatrixAI/Polykey-CLI#178 is blocked by the above mentioned prep-work

@tegefaulkes tegefaulkes merged commit 527a75a into staging May 30, 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.

2 participants