add natsOptions_SetUserCredentialsFromMemory #621
Conversation
There was a problem hiding this comment.
Since this would be a new API, I would like you to rebase your PR from the dev branch and re-post. When you submit a PR you can select the base branch to submit against.
But also, I would prefer that we extend the userCreds struct and add a new field that would be a copy of the memory passed to the new API. But otherwise it would be very similar to natsOptions_SetUserCredentialsFromFiles(). I would simply rename natsConn_userFromFile to natsConn_userCreds or something like that and check for the new field in _sign() and the renamed natsConn_userFromFile. In other words, I think we can get away with the single new option API. Could you try?
src/nats.h
Outdated
| /** \brief Sets JWT handler and handler to sign nonce that uses seed. | ||
| * | ||
| * This function acts similarly to natsOptions_SetUserCredentialsFromFiles but reads from memory instead | ||
| * from a file. Also it assumes that `memory` contains both the JWT and NKey seed. Note that the format of memory is: |
There was a problem hiding this comment.
Looks like the sentence is not finished.
Understood and why not! Will check! |
|
Okay, so this version also has some test. I just assumed it's fine to put it next to the tests for UserCredsFromFiles... Whenever I push here, some tests fail. Are some flaky or is this expected? Right now linters seem fine but one test fails (which seems unrelated) Also, would you like to see an addition to What could also be improved would be better error messages in case of failed connection or wrong parameter detection upfront. I'm not so sure if the client has this everywhere, but I could add a bit about it if you want? And glad to receive any other feedback clearly. |
There was a problem hiding this comment.
Thanks Michael. This is a very good PR. There is just a small bug checking for memory allocation failure, and you need to include the new test in the test suite list by following the instructions that I have directed you too (I did it locally and your new test passes and does not have memory leak, but as of now it is not run in the PR itself).
Also, I would like you to squash to a single commit when I will approve and before I merge. Thanks again!
src/opts.c
Outdated
| uc->userOrChainedFile = NATS_STRDUP(uocf); | ||
| if (uc->userOrChainedFile == NULL) | ||
| uc->jwtAndSeedContent = NATS_STRDUP(jwtAndSeedContent); | ||
| if (uc == NULL) |
There was a problem hiding this comment.
It should be if (uc->jwtAndSeedContent == NULL) here.
test/test.c
Outdated
| @@ -34137,6 +34260,7 @@ static testInfo allTests[] = | |||
| {"GetLocalIPAndPort", test_GetLocalIPAndPort}, | |||
| {"UserCredsCallbacks", test_UserCredsCallbacks}, | |||
| {"UserCredsFromFiles", test_UserCredsFromFiles}, | |||
| {"UserCredsFromMemory", test_UserCredsFromMemory}, | |||
There was a problem hiding this comment.
This test that you added was actually not included in the suite. You need to run test/testsuite which will produce an updated test list and move it to the test/ directory and commit this change. See instructions here
|
Forgot to push yesterday evening. Anyways, some things to mention with current behaviour:
The tests mirror the behaviour, so it can be observed. (tests still failing... one second) |
I would say, this is very useful (I need it myself) because I am actually getting the JWT as env var in some docker container. I could write it to a file and then read that but it feels like unnecessary file I/O so I would rather directly read from memory.
I tested it personally and it works.
I can add UTs if you generally would accept this change (otherwise I'd rather save the time :)).