replace old ocsp and crl apiTests with new integration tests#120
replace old ocsp and crl apiTests with new integration tests#120
Conversation
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
This is looking great. Love to see some extensive use of the new integration testing framework.
I'm most concerned with two things:
- configurable ports (can introduce major flakiness)
- Assertions seem to be okay with
200,400, and other status codes. Unless it really is random or undetermined, these scenarios feel like specific cases we should be asserting for. What would cause a request to sometimes 200 and other times 400 when it comes to certificate verification? Seems suspicious to me, but maybe there is something to all of that I'm not aware of or considering.
Finally, is there any additional documentation for the new utils (or tests) that needs to be added to the integration testing docs content?
| [ | ||
| 'ocsp', | ||
| '-port', | ||
| String(port), |
There was a problem hiding this comment.
I think the ability to specify a port should be optional. This introduces the potential for flakiness if the specified port is already in use.
If possible, can this command self-allocate an available port number and then this method resolves that port number in the response object? Something like Node's http.createServer(0) ?
If not, could we look into some sort of retry mechanism at least?
From my own experience, I know there is no mechanism to "reserve" a port number; at least not with some race condition. Packages like this: https://www.npmjs.com/package/get-port are susceptible to race-conditions since it just spawns a server, reads the port number, then returns the number and you hope you can use that value before something else does.
There was a problem hiding this comment.
Yeah, I don't want to get too complex in here. I've updated what I can in here and might see what I can do about the crl and ocsp endpoints and cert generation. Have some ideas but that might be fine for a follow up.
There was a problem hiding this comment.
Eh...you've spent so much time and effort on the initial integration test setup I don't want to leave something like this for later. I'll get something pushed up in a bit to take care of the remaining potential port conflicts.... brb
There was a problem hiding this comment.
Thank you! I think preventing flake in our integration tests is crucial.
There was a problem hiding this comment.
Yeah, no worries. See how you feel about this. Leaving port allocation to a lower level would probably be possible, but more complex - especially across major platforms. I'm still not sure how this will run as it is across platforms and environments with the openssl dependency. I'll do further testing on my win and linux machines and follow up with additional prs if needed.
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { execSync } from 'node:child_process'; |
There was a problem hiding this comment.
Nit: Mixing import patterns is funny to me. I'd expect either all method imports (import { writeFile } from 'node:fs';) or all module imports (import child_process from 'node:child_process';). We likely should get a lint rule going for this. No big deal if you don't change it here.
There was a problem hiding this comment.
Guess it depends on if you want to use the default export vs named. To me it just depends on what seems to look better in code and this felt like a fine balance. Don't think it's a big deal if tidy and 100% down for additional useful guardrails.
| * @param certsPath - Path to directory containing test.crl | ||
| * @returns Promise resolving to CrlServerContext | ||
| */ | ||
| export async function startCrlServer(port: number, certsPath: string): Promise<CrlServerContext> { |
There was a problem hiding this comment.
Similar as above, I don't like specifying ports as it can be racey or flakey. I'll include the appropriate code change below since this one is at least using Node.js' createServer API.
There was a problem hiding this comment.
I've changed to auto assigned ports where more easily possible but the ocsp and clr endpoints need a know port at cert creation time so there's a little more involved. Added some comments and hopefully made it less likely that there would be conflicts - and if so, we should get a clear error in the logs if an an endpoint has a port conflict.
| err.code === 'ECONNRESET' || | ||
| err.code === 'EPROTO' || | ||
| err.code === 'ECONNREFUSED' || | ||
| err.message?.includes('socket hang up') || | ||
| err.message?.includes('certificate') || | ||
| err.message?.includes('closed'), |
There was a problem hiding this comment.
These feel like 3 different scenarios we should be testing for. Is it random how this situation would error? Does it change based on the node version?
There was a problem hiding this comment.
Tidied up a bunch of this to have clear narrow assertions. Anything else should be treated as an unexpected failure somewhere (code or test). Thanks for calling that out. I would have wanted to do this later anyway, but why not get a clean start. 🫠
| }, | ||
| (res) => { | ||
| res.resume(); | ||
| res.on('end', () => resolve(res.statusCode!)); |
There was a problem hiding this comment.
I wasn't aware the statusCode could be undefined or null.
There was a problem hiding this comment.
It can be undefined apparently...
|
|
||
| // First request - populates cache | ||
| const status1 = await makeRequest(); | ||
| ok(status1 === 200 || status1 === 404, 'First request should succeed'); |
There was a problem hiding this comment.
why is both 200 or 404 okay here?
| if (ocspResponder) { | ||
| await stopOcspResponder(ocspResponder); | ||
| ocspResponder = null; | ||
| } |
There was a problem hiding this comment.
This test stopping the ocsp responder that the other tests in this suite rely on is not ideal. First of all, I'm not sure if the Node.js test runner absolutely executes all tests within a suite in order. Furthermore, this would be very confusing if anyone ever added another test to this suite. In order to reduce potential issues, I'd prefer you do the following:
- Explicitly disable concurrency for this suite, and leave a comment as for why.
- Switch the suite level test function to async and then await each
test()call so they absolutely execute in the specified order.
So it'd look something like this:
// Concurrency is disabled by default in the Node.js test runner, and we are explicitly disabling it here
// since the last test in this suite must always run last in order to not pre-emptively disable the OCSP server.
suite('...', { concurrency: false }, async () => {
await test('...', async () => {});
await test('...', async () => {});
await test('...', async () => {});
});There was a problem hiding this comment.
Well, there's a comment now clarifying what will happen so that should be totally okay and not get us into trouble later. ;)
Yeah... explicit and clear order might be nice but it feels like it could be fragile still in a similar way. I might just add a completely separate suite file to isolate the behavior for that scenario...
There was a problem hiding this comment.
Left the suite intact but made the setup and teardown more explicit and resilient (and even better commented). See how this feels.
| // but no OCSP check occurs. Could get 401 if cert doesn't have a user. | ||
| ok( | ||
| res.statusCode === 200 || res.statusCode === 404 || res.statusCode === 401, |
There was a problem hiding this comment.
Similar suggestion as above; why are we okay with all of these scenarios? Isn't there some specific steps in order to get a 200 vs a 404 or otherwise?
There was a problem hiding this comment.
Some residual ideas from the older tests probably but they should be explicit
|
@Ethan-Arrowood thanks for all the feedback! I'll get back to this soon - head's down on something else. RE response codes I've cared more about 'access denied' vs 'okay-ish'. I have no problem refining these tests though to catch any potential false passes. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
I haven't looked at the details and trust you are sorting that out, but I certainly approve at a high-level.
heskew
force-pushed
the
cert-verification-integration-tests
branch
3 times, most recently
from
c8a8dac to
c52b676
Compare
heskew
force-pushed
the
cert-verification-integration-tests
branch
3 times, most recently
from
8dd73a5 to
f23bf1a
Compare
heskew
force-pushed
the
cert-verification-integration-tests
branch
from
f23bf1a to
5f3dc41
Compare
|
Just took a read through your replies. Sounds like you're on a much better track now. I'll wait until you mark this as ready for review before going through the code again. |
heskew
force-pushed
the
cert-verification-integration-tests
branch
from
3efea32 to
9ce6ab2
Compare
|
@Ethan-Arrowood - please give this another look whenever you get a chance |
4 participants
Resolves #118 and #119