WFE: Check NewOrder rate limits#7201
Conversation
|
@beautifulentropy, this PR appears to contain configuration changes. Please ensure that a corresponding deployment ticket has been filed with the new configuration values. |
70590e2 to
fcb227c
Compare
|
Added some TODOs and moved this back to draft until I get those done. |
8aa9c0f to
3609d28
Compare
54744d1 to
3c1f7a9
Compare
aarongable
left a comment
There was a problem hiding this comment.
Starting this review because jsha is out today.
9756c68 to
ffcdcb1
Compare
Update RA and CA configuration to be more consistent with the identical MaxNames field added to the WFE by #7201.
aarongable
left a comment
There was a problem hiding this comment.
LGTM with one optional improvement
| var newOrderSuccessful bool | ||
| var errIsRateLimit bool | ||
| defer func() { | ||
| if !newOrderSuccessful && !errIsRateLimit { |
There was a problem hiding this comment.
Same comment here as on the other PR
pgporada
left a comment
There was a problem hiding this comment.
Looks good other than the few nits which could be addressed later or never.
| }, | ||
| LookupDNSAuthority: "consul.service.consul", | ||
| } | ||
| rc.PasswordConfig = cmd.PasswordConfig{ |
There was a problem hiding this comment.
Nit: You could include the PasswordConfig in the bredis.Config above.
LookupDNSAuthority: "consul.service.consul",
PasswordConfig: cmd.PasswordConfig{
PasswordFile: "test/secrets/ratelimits_redis_password",
},
}
| // defaults to 100. These limits are per section 7.1 of our combined | ||
| // CP/CPS, under "DV-SSL Subscriber Certificate". The value must match | ||
| // the CA and RA configurations. | ||
| MaxNames int `validate:"min=0,max=100"` |
There was a problem hiding this comment.
Nit: I understand that the validation tag catches this, but MaxNames could be declared as a uint instead. A minor type changes would be needed in the NewWebFrontEndImpl constructor and WebFrontEndImpl struct.
Add non-blocking checks of New Order limits to the WFE using the new key-value based rate limits package.
Part of #5545