Implementing secrets write command
#292
Conversation
tests/secrets/writeFile.test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| test.prop([stdinArb, contentArb], { numRuns: 5 })( |
There was a problem hiding this comment.
Using multiple runs on a test which uses RPC calls is very time-taking. Running five tests takes almost 10 seconds.
I'm not sure what to do with this. Is this a reasonable trade-off, or should I reduce the runs even more? Or this small number of runs cannot hope to catch any real bugs, so I need to run more tests?
There was a problem hiding this comment.
That shouldn't be the case. Can you flamegraph this?
There was a problem hiding this comment.
I'm not sure how to use flamegraph, so I have asked Brian's help with this.
In the meantime, I ran some more tests, and got an average of 400ms after trying to cat a secret. However, I got an average of 2789ms when I tried to run secrets write with the same string as what the file contained for the secrets cat tests (it being "testing").
secrets cat relies on a Duplex Handler to stream file paths to the handler and stream file contents from the handler. However, secrets write relies on a Unary Handler to do so, as we want to upgrade to a Raw Handler, but it needs too much work for it to be included in the first iteration.
As such, I believe the performance impact is due to the different handlers. This is also reinforced by the fact that running a similar test on secrets list gets a similar average time of 519ms while it is using a Server Handler.
There was a problem hiding this comment.
I doubt it's the kind of handler that's the problem. Keep in mind that the main difference between list, cat and write is the fact that you're writing to a file this time. You wan't to sanity check that the writing itself isn't just this slow normally. Fortunately we can compare to the vaults test in Polykey to see how long a write takes there. Take that as the base line.
There was a problem hiding this comment.
We did some digging and narrowed it down to the middlewear. Possibly the password hashing step. Needs some more digging so we create a new issue to track it. #294
|
Looks OK to merge. |
|
All tasks have been done and approval has been granted. Merging. |
chore: added short/long names for commands chore: deleted CommandUpdate fix: renamed test file chore: updated class names for commands fix: removed old tests
aryanjassal
force-pushed
the
feature-unix-write
branch
from
ca225b8 to
ba1345c
Compare
|
I got Terminalizer working on NixOS. It was a little involved. First, it is a npm package which doesn't exist under the nix repository. So, I had to install it globally. As I cannot use the regular This allowed me to record terminal, but rendering was failing as it needed to import Electron, and it was a dynamically linked library. Nix was complaining about it, so I launched a [aryanj@matrix-34xx:~]$ NIXPKGS_ALLOW_UNFREE=1 nix-shell -p steam-runThis launched me into an environment which simulates regular linux libraries. Then, running Do note that rendering Terminalizer recordings is much slower than asciinema recordings. AGG (asciinema gif generator) can generate a gif within a second, while for this recording, Terminalizer rendering took a bit more than 4 minutes. On the other hand, editing recorded files is very easy, as each input/output is stored in a YAML file as a pair of delay and content fields. In asciinema, if I wanted to remove a section from the recording, I had to modify the time stamps across the entire file, otherwise there would have been an awkward delay present in the recording. Terminalizer removes that completely, as it uses delay, which calculates the delay from previous content chunk. This makes it trivial to edit recordings. The output file being YAML also means that full syntax highlighting/LSP support also exists. Overall, except the workarounds needed for Nix and the long rendering times, I would prefer using Terminalizer more than asciinema. Here is the rendered gif. The recording file will also be attached for reference and archival purposes. We can edit the background color and padding. I have added ample padding, so we can crop it to our needs later than need to re-export with a larger padding.
|
Description
The secrets write command will take data written to
stdinand write that into a secret file of our choosing.To simplify this, work has been put into Polykey#814 to automatically write to a new file if it exists, and create the file if it doesn't.
As the PR in Polykey modifies the behaviour of
VaultsSecretsEdit(removes it, actually, and replaces it withVaultsSecretsWriteFile), thesecrets editcommand also needs to be updated with this new behaviour.Issues Fixed
Tasks
secrets writecommandsecrets editwith new RPC handlersecrets updateFinal checklist