Refactor CA unittests to focus on top-level IssueCertificate #8422
Conversation
bf14a88 to
31c238e
Compare
jsha
left a comment
There was a problem hiding this comment.
Generally looks good! A few small tweaks.
You say in the PR description the vast majority of tests now test IssueCertificate instead of the unexported functions. But I see some tests of the unexported functions do remain - is there a pattern to which ones had to stay?
None of the old tests had to stay. The pattern is that I left the ones which felt like they would provide a barrier against misuse of the helper functions in the brief period before the helpers are deleted. I'd be happy to delete those extra tests in this PR if you think it's appropriate to do so; it would make #8424 even cleaner. |
jsha
left a comment
There was a problem hiding this comment.
I'm okay with landing this as-is. That said:
The pattern is that I left the ones which felt like they would provide a barrier against misuse of the helper functions in the brief period before the helpers are deleted. I'd be happy to delete those extra tests in this PR if you think it's appropriate to do so;
I think deleting the extra tests in this PR makes sense. We've replaced low-level tests with tests of the publicly visible functionality, which I think is generally good. The exception of course is when an internal-only function is factored out because it makes a particularly good target for narrowly targeted unittesting. But that doesn't apply here; these internal helper functions are a matter of RPC structure, not optimizing for testability.
There's a wacky world out there somewhere in which between this change and the subsequent change landing, someone tries to add another call to issueCertificateForPrecertificate somewhere bad.
Hopefully in such a world we'd also be adding new tests of the top-level functionality that suffice to cover the new calls. :D
Fair enough, done! I do think it is good to keep the follow-up PR wholly focused on code changes, not test changes. It makes this PR's diff slightly harder to review, but going through it commit-by-commit should still be easy enough. |
The issuePrecertificate, issuePrecertificateInner, and issueCertificateForPrecertificate helpers were originally necessary because the RA made two separate gRPC calls to the CA, before and after acquiring SCTs. Now, the CA handles acquiring SCTs itself, and there's only one gRPC entrypoint: ca.IssueCertficate. This means that the layers of abstraction represented by those old helpers are no longer necessary, and we can significantly simplify the CA's core logic. Inline the contents of issuePrecertificate, issuePrecertificateInner, and issueCertificateForPrecertificate directly into IssueCertificate. Remove the sa.GetCertificate check, because it is now impossible for the final-certificate issuance code to have been reached except by progressing through the precert issuance code. Remove the need to derive an issuer and a profile from the precert, because we are still holding onto the exact same issuer and profile that were used to issue the precert. Delete the unit tests which were only testing the underlying issuePrecertificate and issueCertificateForPrecertificate helpers. As of #8422, those tests were redundant. Move some helpers which were shared by the old and new tests to new homes closer to their now-sole callers. Part of #8390
Rewrite the vast majority of the CA unittests so that they test the exported functions (NewCertificateAuthorityImpl and IssueCertificate) rather than the non-exported helper functions (issuePrecertificate and issueCertificateForPrecertificate). This makes the tests more robust to future refactoring of the CA code itself, and will increase confidence in the correctness of those refactors.
Part of #8390