Skip to content
Snippets Groups Projects

Revert short acme order expiration time

Merged Vladimir Shushlin requested to merge revert-85a7bf7a into master
1 unresolved thread

What does this MR do?

Reverts !22878 (merged)

Shortening of an expiration time for acme orders made us hit Let's Encrypt rate limit for new orders: #197978 (closed)

We try to renew all these 917 domains every 2 hours, hit rate limit and retry every 15 minutes 🙈

[ gprd ] production> PagesDomain.need_auto_ssl_renewal.count
=> 917
[ gprd ] production> PagesDomain.need_auto_ssl_renewal.where.not(certificate:nil).count
=> 358
[ gprd ] production> PagesDomainAcmeOrder.expired.count
=> 154
[ gprd ] production> PagesDomainAcmeOrder.count
=> 770

The rate limit is:

For users of the ACME v2 API you can create a maximum of 300 New Orders per account per 3 hours. https://letsencrypt.org/docs/rate-limits/

Currently, we can make order expiration time about 9 hours (917 / 300 * 3), but I'd vote for just rolling back the change since the release date is very close.

And we'll need to expedite #30146 (closed)

Self-managed customers are unlikely to face the issue, but I think it's still better to revert this change in the %12.7, so I'm adding Pick into auto-deploy. Is this enough to push this MR to the release?

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Vladimir Shushlin

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Sean Carroll
  • Sean Carroll approved this merge request

    approved this merge request

  • Nick Thomas resolved all threads

    resolved all threads

  • LGTM, thanks @vshushlin

    I think Pick into auto-deploy is sufficient.

  • Nick Thomas approved this merge request

    approved this merge request

  • merged

  • Nick Thomas mentioned in commit fdd4a846

    mentioned in commit fdd4a846

  • Automatically picked into 12-8-auto-deploy-20200119, will merge into 12-8-auto-deploy-20200119 ready for 12.8.0-ee.

  • Nick Thomas mentioned in commit 5e37c15f

    mentioned in commit 5e37c15f

  • Stan Hu added 1 deleted label

    added 1 deleted label

  • Automatically picked into !23551 (merged), will merge into 12-7-stable-ee ready for 12.7.1-ee.

  • GitLab Release Tools Bot removed 1 deleted label

    removed 1 deleted label

  • Nick Thomas mentioned in commit cf30a9da

    mentioned in commit cf30a9da

  • mentioned in merge request !23551 (merged)

  • Please register or sign in to reply
    Loading