Skip to content

valid_csr test: move mock clock initialisation before any operations #5942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 18, 2025

Conversation

sorindumitru
Copy link
Collaborator

Otherwise by the time we run the test the seconds of the clock might have rolled and cause us to think that the CA is valid for 1 hour - 1 second instead of 1 hour.

Affected functionality
tests

Description of change
move mock clock initialisation before any operations, otherwise by the time we run the test the seconds of the clock might have rolled and cause us to think that the CA is valid for 1 hour - 1 second instead of 1 hour.

Which issue this PR fixes
fixes #5841

Otherwise by the time we run the test the seconds of the clock might have rolled and cause us to think that the CA is valid for 1 hour - 1 second instead of 1 hour.

Signed-off-by: Sorin Dumitru <[email protected]>
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sorindumitru for looking at this.
I don't think that clock.Mock advances the time unless that the Add method is called? In that case, I don't think that this change would have any effect.

@sorindumitru
Copy link
Collaborator Author

I don't think that clock.Mock advances the time unless that the Add method is called? In that case, I don't think that this change would have any effect.

The problem is that the CA is created at the start of the function, because it's needed for some test expectations. It may be valid from 13:25:51.000 to 14:25:51.000, for example. This will also cap (or at least that's what it seemed it did) the lifetime of the issued intermediate CAs. The mock clock gets initialized for each subtest, using the current time. If the test takes longer than a second, the mock clock will have the now() as 14:25:52.000 as opposed to (14:25:51).

During the test we verify that that the difference between the NotAfter of the certs, which is capped at 14:25:51.000, and now() matches the TTL. But if we are in the situation where the seconds of now() has changed, we will be one second short (because NotAfter is capped based on the start of the test).

I was able to consistently reproduce this issue using -count 128 and it seemed to fix the issue with this patch. Another alternative would be to record now() at the start of test and in each subtest we create the mock clock using NewMockAt().

Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed explanation, @sorindumitru. That makes sense!

@amartinezfayo amartinezfayo merged commit ada8855 into spiffe:main Mar 18, 2025
35 checks passed
@amartinezfayo amartinezfayo added this to the 1.12.1 milestone Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test - TestMintX509CA/valid_CSR
2 participants