General vaults review and fixes

[tegefaulkes]

Description

This PR handles a general review of the vaults domain. General optimisations if they can be applied. Minor fixes for vault related issues and some touch ups to the vaults tests.

NOTE: this branches off from the git refactoring PR and is targeting that for now. But once the git refactoring is merged then this needs to rebase and target staging.

Issues Fixed

• Ref ENG-316
• Fixes Minor review and refactor of vaults domain for 'polykey' #718
• Fixes A single digit modification is not detected by isomorphic-git's status call #260
• Fixes Inconsistent vault log when pulling secrets from a modified vault (received vault log is different) - should use fast forward merge #714

Tasks

• 1. Review vaults domain, clean up and refactor anything as needed
• 2. Address A single digit modification is not detected by isomorphic-git's status call #260
• 3. Address Inconsistent vault log when pulling secrets from a modified vault (received vault log is different) - should use fast forward merge #714
• 4. Have a closer look at the vaults committing secrets and git garbage collection code.
• 5. Determine and apply any optimisations that can be made.
• 6. Review vaults tests and clean them up if needed.
• 7. Re-enable any vaults tests that have been disabled.
• 8. Possibly split up some test files so the vaults domain runs quicker in CI. vault ops is currently very slow.

Final checklist

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

[linear]

ENG-316 Minor review and refactor of vaults domain for 'polykey'

[CMCDragonkai]

What's the order of merging in relation to #709?

[tegefaulkes]

#709 merges first and then this one.

[tegefaulkes]

Switched base over staging now that git refactoring has been merged.

[tegefaulkes]

In the pullVault function in VaultInternal. There is an let error variable that is never set or modified but it affects some logic. I need to dig into this since it can't be intended as it is.

[tegefaulkes]

One of the vaults test for pulling was failing randomly. With some testing I found that it's random network traffic with the mainnet.

I'll need to make sure any tests using a node connection don't actually try to join the mainnet when testing. In this case we have a NodeConnectionManager for testing but give it an empty manifest. On the same note, it's very odd that the test is failing for failing to handle incoming RPC requests. This shouldn't crash us so maybe I need to look deeper into that.

So yeah, there are two problems here.

1. I have a minimal NodeConnectionManager needed to start forward connections but it's receiving reverse connections from the mainnet. Likely from trying to join it itself. We shouldn't be starting or receiving external connections unless the test explicitly wants to do this.
2. There is an RPC error for a missing handler propagating outwards causing the test to fail. By design this should only bubble through the stream back through the RPC. This isn't a critical error that should ever leave the RPC. Something is wrong here.

[tegefaulkes]

I may need to add #461 to this to address the intermittent failure mentioned above. Though at the same time, I feel like this PR is getting too large in scope? I'll think about it.

[tegefaulkes]

One of the vaults test for pulling was failing randomly. With some testing I found that it's random network traffic with the mainnet.

I'll need to make sure any tests using a node connection don't actually try to join the mainnet when testing. In this case we have a NodeConnectionManager for testing but give it an empty manifest. On the same note, it's very odd that the test is failing for failing to handle incoming RPC requests. This shouldn't crash us so maybe I need to look deeper into that.

So yeah, there are two problems here.

1. I have a minimal NodeConnectionManager needed to start forward connections but it's receiving reverse connections from the mainnet. Likely from trying to join it itself. We shouldn't be starting or receiving external connections unless the test explicitly wants to do this.
2. There is an RPC error for a missing handler propagating outwards causing the test to fail. By design this should only bubble through the stream back through the RPC. This isn't a critical error that should ever leave the RPC. Something is wrong here.

I made a new issue #722 And it will be addressed later, outside this PR.

[tegefaulkes]

vaultOps.addSecretDirectory is a weird case. All other vaultOps operate purely within the EFS with an secret contents passed directly into it. but addSecretDirectory seems to access the fs directly reading from it to copy a directory into a vault.

I don't like this. It makes it the only secret command that functions against the node's filesystem and not the Polykey-CLI's file system. We need to review this and work out what we want to do with it.

[tegefaulkes]

So addSecretDirectory needs to be refactored in how it works. currently it accesses the fs to read the directory when copying it to the vault. However, as mentioned in all other vaultOps that write data, the data is passed into the function. From the CLI perspective, the files are sent over the RPC when adding to the vault.

The problem here is addSecretDirectory is the odd duck out. The data isn't sent over the RPC, it's read directly from the fs. So we need to refactor it. There are two options

1. We refactor it so the directory and all it's contents are sent over the RPC to the addSecretDirectory command. A bit of a hassle but do-able. We'd have to encode directory and file contents in a binary stream. I'd rather handle this in a separate PR.
2. We remove the addSecretDirectory and it's CLI command. Seems like a QOL function.

[tegefaulkes]

I'm working on #714 right now. It's a pretty simple fix. However I found that we don't test the case where you pull a vault that deviated from the original history.

I modified a test to check this and isomorphic-git doesn't actually support handling that.

Merges with conflicts are not supported yet.
ErrorVaultsMergeConflict: Merges with conflicts are not supported yet.
    at constructor_.pullVault (/home/brian/workspace/Polykey/src/vaults/VaultInternal.ts:651:15)
    at /home/brian/workspace/Polykey/src/vaults/VaultManager.ts:789:9
    at withF (/home/brian/workspace/Polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)
    at constructor_.pullVault (/home/brian/workspace/Polykey/src/vaults/VaultManager.ts:784:5)
    at withF (/home/brian/workspace/Polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)
    at Object.<anonymous> (/home/brian/workspace/Polykey/tests/vaults/VaultManager.test.ts:1138:9)

So I'm not actually sure it's possible to handle this case. We'd have to clone the vault from scratch to handle it really. Do we just throw an error urging the user to clone the vault fresh due to the deviating history?

[CMCDragonkai]

When you a pull a vault into a deviated history. This is what supposed to happen:

1. You are not allowed to do it.

At a higher level, the user upon editing a vault that is "pulled", should be doing a COW operation, the vault must be duplicated and separated from the old vault. The old vault maintains its ability to be pulled.

This means you cannot have a deviated history at all. It's not allowed by the constraints of the system.

[tegefaulkes]

I'm aware that we restrict editing of a vault that has been cloned. But the remote vault we clone from can do a branching commit and completely change it's history. Do we apply the same restraint in this case? Where we disallow pulling a vault who's history no longer matches our cloned vault's history?

Different sides of the same coin really. Just want to be clear since the example you gave was the opposite side to the example I described.

[CMCDragonkai]

I'm aware that we restrict editing of a vault that has been cloned. But the remote vault we clone from can do a branching commit and completely change it's history. Do we apply the same restraint in this case? Where we disallow pulling a vault who's history no longer matches our cloned vault's history?

Different sides of the same coin really. Just want to be clear since the example you gave was the opposite side to the example I described.

It must be disallowed on both sides.

Linear history must be maintained. If the other side corrupted the vault, you just decline the ability to pull. You indicate why.

[tegefaulkes]

I still need to do one last once-over, but it's ready for review. Doesn't need a deep review. Just so you are aware of changes and the more eyes on it the better. @CMCDragonkai @amydevs

I'll run through the checklist after review. No point doing most of the checklist if there are changes that need to be made still.

[CMCDragonkai]

How was #260 and #714 fixed? Can you describe the solution in the spec?

[CMCDragonkai]

I'm aware that we restrict editing of a vault that has been cloned. But the remote vault we clone from can do a branching commit and completely change it's history. Do we apply the same restraint in this case? Where we disallow pulling a vault who's history no longer matches our cloned vault's history?
Different sides of the same coin really. Just want to be clear since the example you gave was the opposite side to the example I described.

It must be disallowed on both sides.

Linear history must be maintained. If the other side corrupted the vault, you just decline the ability to pull. You indicate why.

And do you have a corresponding exception for these cases now?

[tegefaulkes]

I'm aware that we restrict editing of a vault that has been cloned. But the remote vault we clone from can do a branching commit and completely change it's history. Do we apply the same restraint in this case? Where we disallow pulling a vault who's history no longer matches our cloned vault's history?
Different sides of the same coin really. Just want to be clear since the example you gave was the opposite side to the example I described.

It must be disallowed on both sides.
Linear history must be maintained. If the other side corrupted the vault, you just decline the ability to pull. You indicate why.

And do you have a corresponding exception for these cases now?

Yes, there was an existing error for this called ErrorVaultsMergeConflict

[CMCDragonkai]

Does ErrorVaultsMergeConflict work both ways? It generically means both conflict during a pull and any attempt to conflict it locally? Or are you just ensuring that one cannot corrupt one's own vault automatically?

Are you preventing the node from mutating its own (pulled/cloned) vault, and breaking history for a vault that has been pulled or cloned...? Or maybe the error only shows up on the pulling/cloning side?

[CMCDragonkai]

You should update the spec of this PR describing any protocol changes - and how was the solution implemented in a human readable manner. Including in reference to the above comments.

[tegefaulkes]

Does ErrorVaultsMergeConflict work both ways? It generically means both conflict during a pull and any attempt to conflict it locally? Or are you just ensuring that one cannot corrupt one's own vault automatically?

Are you preventing the node from mutating its own (pulled/cloned) vault, and breaking history for a vault that has been pulled or cloned...? Or maybe the error only shows up on the pulling/cloning side?

Yes, weather the local history diverged or the remove diverged, it looks like a disconnect in the history that requires a merge. So the same error in both cases.

We can't modify local history anyway since any attempt to do so will result in a ErrorVaultRemoteDefined: Vault is a clone of a remote vault and can not be mutated error. So cloned vaults are immutable.

[tegefaulkes]

Ready to merge. I'll write up the changes in a moment.

[CMCDragonkai]

Ok message should be updated. 20 May 2024 14:36:00 Brian Botha ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In src/git/http.ts[#720 (comment)]: > + } catch (e) { + // Return without sending any data + return; + } if (packFile.packfile == null) utils.never('failed to create packFile data'); let packFileBuffer = Buffer.from(packFile.packfile.buffer); I mis-remembered, the *never* here is just to enforce the fact that *packfile* is defined. By our usage it should always be defined. So the *never* is proper here, just the message is off point. /** * - The packfile contents. Not present if `write` parameter was true, in which case the packfile was written straight to disk. */ packfile?: Uint8Array; }; — Reply to this email directly, view it on GitHub[#720 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAE4OHNLXJNGWXC7FT5QB33ZDF4S3AVCNFSM6AAAAABHVTVYWGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGMZDMNJXGE]. You are receiving this because you were mentioned. [Tracking image][https://github.com/notifications/beacon/AAE4OHKJBG66OMQSOCEB5GLZDF4S3A5CNFSM6AAAAABHVTVYWGWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT3DJQ6W.gif]