Skip to content

Commit 8b5f111

Browse files
beautifulentropybeautifulentropy
committed
Address comments
1 parent 5ada869 commit 8b5f111

5 files changed

Lines changed: 71 additions & 51 deletions

File tree

‎cmd/boulder-va/main.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ type Config struct {
5959
// Leaving this value zero means the VA won't early-cancel slow remotes.
6060
SlowRemoteTimeout config.Duration
6161

62-
// ExperimentalVA configures an optional parallel VA that shadows the
62+
// ExperimentalVA configures an optional parallel VA that repeats the
6363
// primary VA's DCV and CAA checks using an alternative DNS resolver,
6464
// emitting comparison metrics without affecting the real validation
6565
// decision.
@@ -70,9 +70,13 @@ type Config struct {
7070
// DNSTimeout is the timeout for DNS queries. Defaults to the
7171
// primary VA's DNSTimeout if unset.
7272
DNSTimeout config.Duration `validate:"omitempty"`
73-
// SampleRate controls the rate of validations that are shadowed
74-
// (0.0 to 1.0). A value of 0 disables shadowing.
73+
// SampleRate controls the rate of validations that are repeated
74+
// (0.0 to 1.0). A value of 0 disables it entirely, while 1 repeats
75+
// all validations.
7576
SampleRate float64 `validate:"min=0,max=1"`
77+
// Timeout is the timeout for experimental validation operations.
78+
// This should be configured to match the RA->VA timeout.
79+
Timeout config.Duration `validate:"required"`
7680
}
7781
Features features.Config
7882
}
@@ -149,6 +153,7 @@ func main() {
149153

150154
var experimentalVA *va.ValidationAuthorityImpl
151155
var experimentalVASampleRate float64
156+
var experimentalVATimeout time.Duration
152157
if c.VA.ExperimentalVA != nil {
153158
servers, err := bdns.StartDynamicProvider(c.VA.ExperimentalVA.DNSProvider, 60*time.Second, "tcp")
154159
cmd.FailOnError(err, "Couldn't start experimental dynamic DNS server resolver")
@@ -190,9 +195,11 @@ func main() {
190195
c.VA.DNSAllowLoopbackAddresses,
191196
nil,
192197
0,
198+
0,
193199
)
194200
cmd.FailOnError(err, "Unable to create experimental VA")
195201
experimentalVASampleRate = c.VA.ExperimentalVA.SampleRate
202+
experimentalVATimeout = c.VA.ExperimentalVA.Timeout.Duration
196203
}
197204

198205
vai, err := va.NewValidationAuthorityImpl(
@@ -211,6 +218,7 @@ func main() {
211218
c.VA.DNSAllowLoopbackAddresses,
212219
experimentalVA,
213220
experimentalVASampleRate,
221+
experimentalVATimeout,
214222
)
215223
cmd.FailOnError(err, "Unable to create VA server")
216224

‎cmd/remoteva/main.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ func main() {
132132
c.RVA.DNSAllowLoopbackAddresses,
133133
nil,
134134
0,
135+
0,
135136
)
136137
cmd.FailOnError(err, "Unable to create Remote-VA server")
137138

‎va/caa.go‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -105,23 +105,25 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal
105105
prob.Detail = fmt.Sprintf("While processing CAA for %s: %s", ident.Value, prob.Detail)
106106
}
107107

108-
var localResult remoteResult
109-
if va.shouldDispatchExperiment() {
110-
defer func() {
111-
va.dispatchExperiment(opCAA, localResult, func(ctx context.Context) (remoteResult, error) {
112-
return va.experimentalVA.DoCAA(ctx, req)
113-
})
114-
}()
115-
}
116-
117108
// Capture the local validation result for experimental resolver comparison
118109
// before MPIC can influence the outcome.
119-
localResult, err = bgrpc.CAAResultToPB(filterProblemDetails(prob), va.perspective, va.rir)
110+
localResult, err := bgrpc.CAAResultToPB(filterProblemDetails(prob), va.perspective, va.rir)
120111
if err != nil {
121112
return nil, err
122113
}
114+
115+
if va.shouldRunExperiment() {
116+
go va.runExperiment(
117+
ctx,
118+
opCAA,
119+
proto.Clone(localResult).(*vapb.IsCAAValidResponse),
120+
func(ctx context.Context) (remoteResult, error) {
121+
return va.experimentalVA.DoCAA(ctx, req)
122+
})
123+
}
124+
123125
if prob != nil {
124-
return localResult.(*vapb.IsCAAValidResponse), nil
126+
return localResult, nil
125127
}
126128

127129
if va.isPrimaryVA() {

‎va/va.go‎

Lines changed: 44 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ type ValidationAuthorityImpl struct {
217217
allowRestrictedAddrs bool
218218
experimentalVA *ValidationAuthorityImpl
219219
experimentalVASampleRate float64
220+
experimentalVATimeout time.Duration
220221

221222
metrics *vaMetrics
222223
}
@@ -241,6 +242,7 @@ func NewValidationAuthorityImpl(
241242
allowRestrictedAddrs bool,
242243
experimentalVA *ValidationAuthorityImpl,
243244
experimentalVASampleRate float64,
245+
experimentalVATimeout time.Duration,
244246
) (*ValidationAuthorityImpl, error) {
245247

246248
if len(accountURIPrefixes) == 0 {
@@ -290,43 +292,46 @@ func NewValidationAuthorityImpl(
290292
allowRestrictedAddrs: allowRestrictedAddrs,
291293
experimentalVA: experimentalVA,
292294
experimentalVASampleRate: experimentalVASampleRate,
295+
experimentalVATimeout: experimentalVATimeout,
293296
}
294297

295298
return va, nil
296299
}
297300

298-
func (va *ValidationAuthorityImpl) shouldDispatchExperiment() bool {
301+
func (va *ValidationAuthorityImpl) shouldRunExperiment() bool {
299302
return va.experimentalVA != nil && rand.Float64() < va.experimentalVASampleRate
300303
}
301304

302-
func (va *ValidationAuthorityImpl) dispatchExperiment(operation string, primary remoteResult, experimentFunc func(context.Context) (remoteResult, error)) {
303-
go func() {
304-
ctx, cancel := context.WithTimeout(context.Background(), va.slowRemoteTimeout)
305-
defer cancel()
305+
// runExperiment compares the primary VA's local result against the experimental
306+
// VA's result and records a concurrence metric. On disagreement, it logs a
307+
// structured event with both results. The primary argument must not be nil.
308+
// Callers should invoke this in a goroutine.
309+
func (va *ValidationAuthorityImpl) runExperiment(ctx context.Context, operation string, primary remoteResult, experimentFunc func(context.Context) (remoteResult, error)) {
310+
ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), va.experimentalVATimeout)
311+
defer cancel()
306312

307-
experimentResult, err := experimentFunc(ctx)
313+
experimentResult, err := experimentFunc(ctx)
308314

309-
primaryPassed := primary.GetProblem() == nil
310-
experimentPassed := (err == nil) && (experimentResult.GetProblem() == nil)
315+
primaryPassed := primary.GetProblem() == nil
316+
experimentPassed := (err == nil) && (experimentResult.GetProblem() == nil)
311317

312-
if primaryPassed == experimentPassed {
313-
va.metrics.experimentConcurrence.WithLabelValues(operation, "true").Inc()
314-
return
315-
}
316-
va.metrics.experimentConcurrence.WithLabelValues(operation, "false").Inc()
317-
318-
logArgs := map[string]any{
319-
"operation": operation,
320-
"primaryPassed": primaryPassed,
321-
"primaryResult": primary,
322-
"experimentPassed": experimentPassed,
323-
"experimentResult": experimentResult,
324-
}
325-
if err != nil {
326-
logArgs["experimentErr"] = err.Error()
327-
}
328-
va.log.AuditInfo("Primary VA disagreed with experimental VA", logArgs)
329-
}()
318+
if primaryPassed == experimentPassed {
319+
va.metrics.experimentConcurrence.WithLabelValues(operation, "true").Inc()
320+
return
321+
}
322+
va.metrics.experimentConcurrence.WithLabelValues(operation, "false").Inc()
323+
324+
logArgs := map[string]any{
325+
"operation": operation,
326+
"primaryPassed": primaryPassed,
327+
"primaryResult": primary,
328+
"experimentPassed": experimentPassed,
329+
"experimentResult": experimentResult,
330+
}
331+
if err != nil {
332+
logArgs["experimentErr"] = err.Error()
333+
}
334+
va.log.AuditInfo("Primary VA disagreed with experimental VA", logArgs)
330335
}
331336

332337
// maxAllowedFailures returns the maximum number of allowed failures
@@ -819,23 +824,25 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV
819824
prob = detailedError(err)
820825
}
821826

822-
var localResult remoteResult
823-
if va.shouldDispatchExperiment() {
824-
defer func() {
825-
va.dispatchExperiment(opDCV, localResult, func(ctx context.Context) (remoteResult, error) {
826-
return va.experimentalVA.DoDCV(ctx, req)
827-
})
828-
}()
829-
}
830-
831827
// Capture the local validation result for experimental resolver comparison
832828
// before MPIC can influence the outcome.
833-
localResult, err = bgrpc.ValidationResultToPB(records, filterProblemDetails(prob), va.perspective, va.rir)
829+
localResult, err := bgrpc.ValidationResultToPB(records, filterProblemDetails(prob), va.perspective, va.rir)
834830
if err != nil {
835831
return nil, err
836832
}
833+
834+
if va.shouldRunExperiment() {
835+
go va.runExperiment(
836+
ctx,
837+
opDCV,
838+
proto.Clone(localResult).(*vapb.ValidationResult),
839+
func(ctx context.Context) (remoteResult, error) {
840+
return va.experimentalVA.DoDCV(ctx, req)
841+
})
842+
}
843+
837844
if prob != nil {
838-
return localResult.(*vapb.ValidationResult), nil
845+
return localResult, nil
839846
}
840847

841848
if va.isPrimaryVA() {

‎va/va_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ func setup(srv *httptest.Server, userAgent string, remoteVAs []RemoteVA, fakeDNS
154154
true,
155155
nil,
156156
0,
157+
0,
157158
)
158159
if err != nil {
159160
panic(fmt.Sprintf("Failed to create validation authority: %v", err))
@@ -332,6 +333,7 @@ func TestNewValidationAuthorityImplWithDuplicateRemotes(t *testing.T) {
332333
true,
333334
nil,
334335
0,
336+
0,
335337
)
336338
test.AssertError(t, err, "NewValidationAuthorityImpl allowed duplicate remote perspectives")
337339
test.AssertContains(t, err.Error(), "duplicate remote VA perspective \"dadaist\"")

0 commit comments

Comments
 (0)