Fix flaky CT submission bug #1085
Conversation
breadyzhang
force-pushed
the
ct
branch
2 times, most recently
from
4c8b781 to
37ba9ac
Compare
…ould be submitted when minInclusions > 0.
…ine how safeSubmissionState decides which SCTs to insert in the results
- move base into switch expression - change maxSubmissionsPerGroup to maxSubmissionsPerOperator
Fix building when the new `wasip1` port is being used. This is a new target that will be introduced by go 1.21. For more details golang/go#58141 Signed-off-by: Flavio Castelli <fcastelli@suse.com>
|
I accidentally closed the PR due to a merge conflict mishap I had. |
…string]int so I am reverting it back to the updated state
…to groupsSubmitted groupNeeds was used for the old chrome policy when we required SCTs from specific groups. It's not necessary anymore with the new policies so a single integer (minSubmissions) should be suffice. groups is changed to groupsSubmitted to make it easier to understand upon a glance.
There was a problem hiding this comment.
Can you link to #1084 from the PR description so that GitHub can cross-link these?
I'm not familiar at all with this code so will need to bounce this to someone else for review. One thing that seems missing from this PR is a test case that demonstrates the bug you are fixing. I see some changes in the tests but I can't see the relevance to my understanding of the issue.
I have updated the PR description to link #1084 and added a general comment on the main changes in the PR.
Currently in |
submission/races.go
Outdated
| // Set the result only if the base group still needs SCTs more than total counts | ||
| // of minimum inclusions for other groups. | ||
| if sub.groupNeeds[ctpolicy.BaseName] > minInclusionsForOtherGroup { | ||
| if sub.remainingSubmissions > extraSubmissions { |
There was a problem hiding this comment.
Shouldn't this just be: extraSubmissions > 0?
Why does it compare a number of SCTs, with a number of groups?
There was a problem hiding this comment.
extraSubmissions represents the number of SCTs that need to be submitted to other log operators. If we are in this part of the branch, it implies that an SCT has already been submitted under this log operator. We need to make sure that adding this SCT to sub.results will still allow us to satisfy the distinct log operator criterion. For example, a certificate that has a lifetime > 180 days requires 3 SCTs but only 2 distinct log operators. This means we can use 2 SCTs from 1 log operator. We need to compare extraSubmissions with sub.remainingSubmissions
extraSubmissions was a misleading name. I have renamed it to reservedSubmissions and added a comment to clear up the confusion.
| } | ||
| } | ||
| sub.remainingSubmissions-- | ||
| } else if sub.remainingSubmissions > 0 { |
There was a problem hiding this comment.
This branch is only executed if sub.results[logURL].sct == nil, i.e if the SCT was not matched to a groupName above. Is this even something that we want to allow?
There was a problem hiding this comment.
sub.results[logURL].sct == nil means a previous SCT has been set in sub.results from the same log operator (the sct gets set to nil in sub.request(...)). This branch is used for the situation where an SCT from an already submitted log operator can be used in the results.
|
/gcbrun |
This reverts commit 1bf39e3.
Fixes #1084
Use a single integer (
remainingSubmissions) instead of a map (groupNeeds) for checking how many submissions are needed. The map is no longer needed since the new CT policies don't require submissions from specific log operators. Using an integer simplifies the logic for determining how many more submissions are needed and whether or not thesafeSubmissionsStateshould store the returned SCT.The
chromelikeunit test undersubmission/races_test.go:TestGetSCTsnow properly tests for the flake.Checklist