I get a mismatch here, am I misunderstanding the role of cr.Spec.Duration? Why does the returned signed certificate have a 0 time on its NotAfter field?

% go test ./pkg/controller/certificaterequests/ca/ -run TestCA_Sign
I0223 20:32:06.072826   16843 ca.go:128] cert-manager "msg"="certificate issued"
I0223 20:32:06.075193   16843 ca.go:128] cert-manager "msg"="certificate issued"
I0223 20:32:06.077904   16843 ca.go:128] cert-manager "msg"="certificate issued"
I0223 20:32:06.080078   16843 ca.go:128] cert-manager "msg"="certificate issued"
--- FAIL: TestCA_Sign (0.38s)
    --- FAIL: TestCA_Sign/when_the_CertificateRequest_has_the_duration_field_set,_it_should_appear_as_notAfter_on_the_signed_ca (0.00s)
        ca_test.go:440:
            	Error Trace:	ca_test.go:440
            	            				ca_test.go:545
            	Error:      	Not equal:
            	            	expected: time.Time{wall:0x459e058, ext:63749707326, loc:(*time.Location)(nil)}
            	            	actual  : time.Time{wall:0x0, ext:63749707326, loc:(*time.Location)(nil)}

            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1,3 +1,3 @@
            	            	 (time.Time) {
            	            	- wall: (uint64) 72999000,
            	            	+ wall: (uint64) 0,
            	            	  ext: (int64) 63749707326,
            	Test:       	TestCA_Sign/when_the_CertificateRequest_has_the_duration_field_set,_it_should_appear_as_notAfter_on_the_signed_ca
FAIL
FAIL	github.com/jetstack/cert-manager/pkg/controller/certificaterequests/ca	0.411s
FAIL

I think this is happening because time.Now() gives you a value to a higher precision than what ends up on the cert.

If I modify time.Now().UTC().Add(30*time.Minute) -> time.Now().UTC().Add(30*time.Minute).Round(time.Second *2) this test passes.

Not sure why the assertion shows it as 0.

Oohh thank you! I added String() in order to see a human-readable time, we can see the difference:

expected: 2021-02-24 14:26:38.587263 +0000 UTC
actual:   2021-02-24 14:26:38 +0000 UTC

I'll use your tip to fix the test case!

I used Truncate instead of Round, here is how I went:

// The notAfter field uses a precision of 1 second; the current time is
// truncated before adding the certificate request's duration value.
assert.Equal(t, time.Now().UTC().Truncate(1*time.Second).Add(30*time.Minute).String(), got.NotAfter.String())

Ah, yes truncating is obviously a better approach than rounding 😄

String() makes it more readable, but maybe asserting the equality of the actual structs (rather than strings) is more important?

OK, I'll do that then, and will show the .String() values in the error message instead

That makes sense, I think

Suggestion: is there maybe a way how we can not return here and still run the other checks even when we expect an error (i.e by testing that gotCert is nil etc)?
I feel like it is a little confusing that we return in a test- but happy to be convinced otherwise.

I also realize that returning here potentially "skips" below assertions that might have to be checked even if an error is returned...

Would it make sense to do the following instead:

if test.wantErr != "" {
	assert.EqualError(t, gotErr, test.wantErr)
} else {
	require.NoError(t, gotErr)

	require.NotNil(t, gotIssueResp)
	gotCert, err := pki.DecodeX509CertificateBytes(gotIssueResp.Certificate)
	require.NoError(t, err)
	test.assertSignedCert(t, gotCert)                
}

I think this is clearer! I am always torn between whether I actually want to check error strings and whether I want to verify for example that gotIssuerResp is nil in case of an error.
I personally have a slight preference for not having else blocks and not comparing error strings only types if possible:

// given that `wantsErr` is boolean and  `gotErr` is error
if test.wantsErr != (test.gotErr != nil) {
	// unexpected presence error or the lack of it, fail
}
require.Equal(test.IssuerResp, gotIssuerResp) // check both that it's correct when there's no error and nil when there is error 
if gotIssuerResp != nil {
	gotCert, err := pki.DecodeX509CertificateBytes(gotIssueResp.Certificate)
	require.NoError(t, err)
	test.assertSignedCert(t, gotCert)
}          

But I don't think this ^ is better, just different, so your fix looks good to me.

Apparently this requires some additional tolerance as it was reported off by 1:
#3475 (comment)

Yes, I might want to check that got.NotAfter is within the range

[expectNotAfter - 2 time.Second, expectNotAfter + 2 * time.Second]

or something like that 😅

In the long term, if the API is malleable, maybe you change:
https://github.com/jetstack/cert-manager/blob/923eec2d1bce1e2bc865fb7c5557c38ad0bd045f/pkg/util/pki/csr.go#L320-L321 so that you can provide a time provider instead of being forced to live in real time.

n.b. even if you don't do that, someone should fix it so that time.Now() is only called once. It's introducing drift between the two function calls which is arguably wrong.