close
The Wayback Machine - https://web.archive.org/web/20201114123409/https://github.com/letsencrypt/boulder/pull/3447
Skip to content
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

Log fewer spurious CT submission errors. #3447

Merged
merged 6 commits into from Feb 16, 2018
Merged

Log fewer spurious CT submission errors. #3447

merged 6 commits into from Feb 16, 2018

Conversation

@jsha
Copy link
Contributor

@jsha jsha commented Feb 14, 2018

With the CT policy changes, we cancel any outstanding requests in a
group once we've gotten a successful response from any log in that
group. That means various function calls will return early with an error
code indicating cancellation. We want to avoid logging such error codes,
because they were not really errors, they were intentional.

This change introduces a small utility package called "canceled", which
checks for both context.Canceled and the gRPC return code indicating
cancelled.

With the CT policy changes, we cancel any outstanding requests in a
group once we've gotten a successful response from any log in that
group. That means various function calls will return early with an error
code indicating cancellation. We want to avoid logging such error codes,
because they were not really errors, they were intentional.

This change introduces a small utility package called "canceled", which
checks for both context.Canceled and the gRPC return code indicating
cancelled.
@jsha jsha requested review from cpu and rolandshoemaker Feb 14, 2018
@jsha jsha requested a review from letsencrypt/boulder-developers as a code owner Feb 14, 2018
@jsha
Copy link
Contributor Author

@jsha jsha commented Feb 14, 2018

@rolandshoemaker
Copy link
Contributor

@rolandshoemaker rolandshoemaker commented Feb 15, 2018

ctpolicy tests are still failing.

results <- result{sct: sct, err: err}
}(g)
if err != nil {
results <- result{err: fmt.Errorf("CT log group %d: %s", i, err)}

This comment has been minimized.

@rolandshoemaker

rolandshoemaker Feb 16, 2018
Contributor

So this is slightly confusing, and will end up requiring a person to go back to the config to figure out which group is actually being talked about. Probably not required to be done in this PR but I think we should actually turn the group description into a struct so that it can have actual name etc.

Copy link
Contributor

@rolandshoemaker rolandshoemaker left a comment

Looks good, one comment but I think that should actually be done in a follow up with a few other things, so approved.

@cpu
cpu approved these changes Feb 16, 2018
Copy link
Contributor

@cpu cpu left a comment

Looks good to me, thanks @jsha 📻

@cpu cpu merged commit d2a2826 into master Feb 16, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cpu cpu deleted the quieter-ct branch Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.