From bac09211ae48254ffed82100300cf56aefee43ac Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 27 Sep 2024 11:38:06 -0400 Subject: [PATCH 1/5] Use buffered channel and process all results --- va/va.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/va/va.go b/va/va.go index 45c7f754758..962a57969ce 100644 --- a/va/va.go +++ b/va/va.go @@ -467,7 +467,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( err error } - results := make(chan *rvaResult) + results := make(chan *rvaResult, len(va.remoteVAs)) for _, i := range rand.Perm(len(va.remoteVAs)) { remoteVA := va.remoteVAs[i] @@ -486,7 +486,9 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( bad := 0 var firstProb *probs.ProblemDetails - for res := range results { + for i := 0; i < len(va.remoteVAs); i++ { + res := <-results + var currProb *probs.ProblemDetails if res.err != nil { @@ -524,12 +526,6 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) return firstProb } - - // If we somehow haven't returned early, we need to break the loop once all - // of the VAs have returned a result. - if good+bad >= len(va.remoteVAs) { - break - } } // This condition should not occur - it indicates the good/bad counts neither From 12a168f913bbad457a7329a839fe0ed58bae52ef Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 27 Sep 2024 12:02:36 -0400 Subject: [PATCH 2/5] Use contexts to manage goroutines --- va/va.go | 101 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 39 deletions(-) diff --git a/va/va.go b/va/va.go index 962a57969ce..f8f9e9381a2 100644 --- a/va/va.go +++ b/va/va.go @@ -467,16 +467,23 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( err error } - results := make(chan *rvaResult, len(va.remoteVAs)) + ctx, cancel := context.WithCancel(ctx) + defer cancel() + results := make(chan *rvaResult) for _, i := range rand.Perm(len(va.remoteVAs)) { remoteVA := va.remoteVAs[i] go func(rva RemoteVA, out chan<- *rvaResult) { res, err := rva.PerformValidation(ctx, req) - out <- &rvaResult{ + select { + case out <- &rvaResult{ hostname: rva.Address, response: res, err: err, + }: + case <-ctx.Done(): + // Context canceled, exit the goroutine. + return } }(remoteVA, results) } @@ -484,53 +491,69 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( required := len(va.remoteVAs) - va.maxRemoteFailures good := 0 bad := 0 + total := 0 var firstProb *probs.ProblemDetails - for i := 0; i < len(va.remoteVAs); i++ { - res := <-results - - var currProb *probs.ProblemDetails + for { + select { + case res := <-results: + total++ + + var currProb *probs.ProblemDetails + + if res.err != nil { + bad++ + + if canceled.Is(res.err) { + currProb = probs.ServerInternal("Remote PerformValidation RPC canceled") + } else { + va.log.Errf("Remote VA %q.PerformValidation failed: %s", res.hostname, res.err) + currProb = probs.ServerInternal("Remote PerformValidation RPC failed") + } + } else if res.response.Problems != nil { + bad++ + + var err error + currProb, err = bgrpc.PBToProblemDetails(res.response.Problems) + if err != nil { + va.log.Errf("Remote VA %q.PerformValidation returned malformed problem: %s", res.hostname, err) + currProb = probs.ServerInternal("Remote PerformValidation RPC returned malformed result") + } + } else { + good++ + } - if res.err != nil { - bad++ + if firstProb == nil && currProb != nil { + firstProb = currProb + } - if canceled.Is(res.err) { - currProb = probs.ServerInternal("Remote PerformValidation RPC canceled") - } else { - va.log.Errf("Remote VA %q.PerformValidation failed: %s", res.hostname, res.err) - currProb = probs.ServerInternal("Remote PerformValidation RPC failed") + if good >= required { + // Enough validations have succeeded to consider the + // authorization valid. Stop any remaining goroutines. + cancel() + return nil } - } else if res.response.Problems != nil { - bad++ - - var err error - currProb, err = bgrpc.PBToProblemDetails(res.response.Problems) - if err != nil { - va.log.Errf("Remote VA %q.PerformValidation returned malformed problem: %s", res.hostname, err) - currProb = probs.ServerInternal("Remote PerformValidation RPC returned malformed result") + if bad > va.maxRemoteFailures { + // Too many validations have failed to consider the + // authorization valid. Stop any remaining goroutines. + va.metrics.remoteValidationFailures.Inc() + firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) + cancel() + return firstProb } - } else { - good++ - } - if firstProb == nil && currProb != nil { - firstProb = currProb - } + if total >= len(va.remoteVAs) { + // This condition should not occur - it indicates the good/bad + // counts neither met the required threshold nor the + // maxRemoteFailures threshold. + cancel() + return probs.ServerInternal("Too few remote PerformValidation RPC results") + } - // Return as soon as we have enough successes or failures for a definitive result. - if good >= required { - return nil - } - if bad > va.maxRemoteFailures { - va.metrics.remoteValidationFailures.Inc() - firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) - return firstProb + case <-ctx.Done(): + return probs.ServerInternal("Context canceled before sufficient results received") } } - - // This condition should not occur - it indicates the good/bad counts neither - // met the required threshold nor the maxRemoteFailures threshold. - return probs.ServerInternal("Too few remote PerformValidation RPC results") } // logRemoteResults is called by `processRemoteCAAResults` when the From 1c80e44083b3de6fb227d50911da3faa3cd960c8 Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 27 Sep 2024 15:13:08 -0400 Subject: [PATCH 3/5] Use a buffered channel. --- va/va.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/va/va.go b/va/va.go index f8f9e9381a2..8daaa68a5db 100644 --- a/va/va.go +++ b/va/va.go @@ -469,7 +469,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( ctx, cancel := context.WithCancel(ctx) defer cancel() - results := make(chan *rvaResult) + results := make(chan *rvaResult, len(va.remoteVAs)) for _, i := range rand.Perm(len(va.remoteVAs)) { remoteVA := va.remoteVAs[i] @@ -529,16 +529,14 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( if good >= required { // Enough validations have succeeded to consider the - // authorization valid. Stop any remaining goroutines. - cancel() + // authorization valid. return nil } if bad > va.maxRemoteFailures { // Too many validations have failed to consider the - // authorization valid. Stop any remaining goroutines. + // authorization valid. va.metrics.remoteValidationFailures.Inc() firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) - cancel() return firstProb } @@ -546,7 +544,6 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( // This condition should not occur - it indicates the good/bad // counts neither met the required threshold nor the // maxRemoteFailures threshold. - cancel() return probs.ServerInternal("Too few remote PerformValidation RPC results") } From 8439d02da81130bf124a786479df2b0f32229b14 Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 27 Sep 2024 15:32:26 -0400 Subject: [PATCH 4/5] Revert the changes to the loop --- va/va.go | 93 +++++++++++++++++++++++++------------------------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/va/va.go b/va/va.go index 8daaa68a5db..ff4fc9ceaba 100644 --- a/va/va.go +++ b/va/va.go @@ -466,10 +466,10 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( response *vapb.ValidationResult err error } + results := make(chan *rvaResult, len(va.remoteVAs)) ctx, cancel := context.WithCancel(ctx) defer cancel() - results := make(chan *rvaResult, len(va.remoteVAs)) for _, i := range rand.Perm(len(va.remoteVAs)) { remoteVA := va.remoteVAs[i] @@ -491,66 +491,57 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( required := len(va.remoteVAs) - va.maxRemoteFailures good := 0 bad := 0 - total := 0 var firstProb *probs.ProblemDetails - for { - select { - case res := <-results: - total++ - - var currProb *probs.ProblemDetails - - if res.err != nil { - bad++ - - if canceled.Is(res.err) { - currProb = probs.ServerInternal("Remote PerformValidation RPC canceled") - } else { - va.log.Errf("Remote VA %q.PerformValidation failed: %s", res.hostname, res.err) - currProb = probs.ServerInternal("Remote PerformValidation RPC failed") - } - } else if res.response.Problems != nil { - bad++ - - var err error - currProb, err = bgrpc.PBToProblemDetails(res.response.Problems) - if err != nil { - va.log.Errf("Remote VA %q.PerformValidation returned malformed problem: %s", res.hostname, err) - currProb = probs.ServerInternal("Remote PerformValidation RPC returned malformed result") - } - } else { - good++ - } + for res := range results { + var currProb *probs.ProblemDetails - if firstProb == nil && currProb != nil { - firstProb = currProb - } + if res.err != nil { + bad++ - if good >= required { - // Enough validations have succeeded to consider the - // authorization valid. - return nil + if canceled.Is(res.err) { + currProb = probs.ServerInternal("Remote PerformValidation RPC canceled") + } else { + va.log.Errf("Remote VA %q.PerformValidation failed: %s", res.hostname, res.err) + currProb = probs.ServerInternal("Remote PerformValidation RPC failed") } - if bad > va.maxRemoteFailures { - // Too many validations have failed to consider the - // authorization valid. - va.metrics.remoteValidationFailures.Inc() - firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) - return firstProb + } else if res.response.Problems != nil { + bad++ + + var err error + currProb, err = bgrpc.PBToProblemDetails(res.response.Problems) + if err != nil { + va.log.Errf("Remote VA %q.PerformValidation returned malformed problem: %s", res.hostname, err) + currProb = probs.ServerInternal("Remote PerformValidation RPC returned malformed result") } + } else { + good++ + } - if total >= len(va.remoteVAs) { - // This condition should not occur - it indicates the good/bad - // counts neither met the required threshold nor the - // maxRemoteFailures threshold. - return probs.ServerInternal("Too few remote PerformValidation RPC results") - } + if firstProb == nil && currProb != nil { + firstProb = currProb + } - case <-ctx.Done(): - return probs.ServerInternal("Context canceled before sufficient results received") + // Return as soon as we have enough successes or failures for a definitive result. + if good >= required { + return nil + } + if bad > va.maxRemoteFailures { + va.metrics.remoteValidationFailures.Inc() + firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) + return firstProb + } + + // If we somehow haven't returned early, we need to break the loop once all + // of the VAs have returned a result. + if good+bad >= len(va.remoteVAs) { + break } } + + // This condition should not occur - it indicates the good/bad counts neither + // met the required threshold nor the maxRemoteFailures threshold. + return probs.ServerInternal("Too few remote PerformValidation RPC results") } // logRemoteResults is called by `processRemoteCAAResults` when the From faf43241fe0a79d465759fee6ef1e7cadbca5bd3 Mon Sep 17 00:00:00 2001 From: Samantha Date: Mon, 30 Sep 2024 12:18:51 -0400 Subject: [PATCH 5/5] Address comment. --- va/va.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/va/va.go b/va/va.go index ff4fc9ceaba..10ecfb2e00c 100644 --- a/va/va.go +++ b/va/va.go @@ -466,24 +466,17 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( response *vapb.ValidationResult err error } - results := make(chan *rvaResult, len(va.remoteVAs)) - ctx, cancel := context.WithCancel(ctx) - defer cancel() + results := make(chan *rvaResult, len(va.remoteVAs)) for _, i := range rand.Perm(len(va.remoteVAs)) { remoteVA := va.remoteVAs[i] go func(rva RemoteVA, out chan<- *rvaResult) { res, err := rva.PerformValidation(ctx, req) - select { - case out <- &rvaResult{ + out <- &rvaResult{ hostname: rva.Address, response: res, err: err, - }: - case <-ctx.Done(): - // Context canceled, exit the goroutine. - return } }(remoteVA, results) }