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

Add feature flag to disable pending authz reuse #7836

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Nov 21, 2024

Pending authz reuse is a nice-to-have feature because it allows us to create fewer rows in the authz database table when creating new orders. However, stats show that less than 2% of authorizations that we attach to new orders are reused pending authzs. And as we move towards using a more streamlined database schema to store our orders, authorizations, and validation attempts, disabling pending authz reuse will greatly simplify our database schema and code.

CPS Compliance Review: our CPS does not speak to whether or not we reuse pending authorizations for new orders.
IN-10859 tracks enabling this flag in prod

Part of #7715

@aarongable aarongable requested a review from a team as a code owner November 21, 2024 01:19
Copy link
Contributor

@aarongable, this PR adds one or more new feature flags: NoPendingAuthzReuse. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document.

Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:".

Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I think we should consider some unit coverage for RegistrationAuthorityImpl.NewOrder with this flag enabled.

jprenken
jprenken previously approved these changes Nov 22, 2024
@jsha
Copy link
Contributor

jsha commented Nov 22, 2024

One good strategy for unittest coverage might be: locally, comment out the flag check such that the new code (GetValidAuthorizationsRequest) runs unconditionally, run the RA test and see what fails, then fix it / adjust it to run with and without the flag.

@aarongable
Copy link
Contributor Author

Yeah, I already did that, and the result is that precisely one test fails -- one that is expecting authz reuse and no longer gets it. We don't actually have any tests currently that cover the distinction between pending and valid authz reuse. So I'm attempting a larger rewrite and unification of the NewOrder tests to see if that can put us in a better state.

@aarongable
Copy link
Contributor Author

Okay, one of the unit test failures is a known flake:

--- FAIL: TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersRatelimit (0.29s)
    ra_test.go:1121: err was unexpectedly nil and should not have been
FAIL
FAIL	github.com/letsencrypt/boulder/ra	13.875s

The other unit test failure is in my new tests, but I am wholly unable to recreate it locally, even when running the exact same command on a clean docker setup:

--- FAIL: TestNewOrder_OrderReuse (0.11s)
    --- FAIL: TestNewOrder_OrderReuse/Duplicate_order,_different_regID (0.01s)
        ra_test.go:2292: 1 == 1
    --- FAIL: TestNewOrder_OrderReuse/Duplicate_order,_same_regID (0.01s)
        ra_test.go:2288: 4 != 1
FAIL
FAIL	github.com/letsencrypt/boulder/ra	8.998s

Copy link
Contributor

github-actions bot commented Dec 3, 2024

@aarongable, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

@aarongable aarongable merged commit 95e5f87 into main Dec 6, 2024
16 checks passed
@aarongable aarongable deleted the rm-pending-authz-reuse branch December 6, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants