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.

Sign up for GitHub

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

Jump to bottom

Cleanup 'CheckRenewalExemptionAtWFE' feature flag #7680

Closed
wants to merge 2 commits into from
Closed

Cleanup 'CheckRenewalExemptionAtWFE' feature flag #7680

wants to merge 2 commits into from

Conversation

ldlb9527
Copy link

Cleanup CheckRenewalExemptionAtWFE feature flag,it now determines exemption based on the isRenewal parameter.

fixes: #7511

@ldlb9527 ldlb9527 requested a review from a team as a code owner August 27, 2024 04:27
aarongable
aarongable requested changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Before landing this change, we need to land a separate change which enables this flag in the //test/config/[ra|wfe2].json files.

@ldlb9527
Copy link
Author

ldlb9527 commented Sep 9, 2024

Before landing this change, we need to land a separate change which enables this flag in the //test/config/[ra|wfe2].json files.

For the separate change, what can I do to help move this forward?

@beautifulentropy
Copy link
Member

beautifulentropy commented Sep 11, 2024 •
edited
Loading

Before landing this change, we need to land a separate change which enables this flag in the //test/config/[ra|wfe2].json files.

For the separate change, what can I do to help move this forward?

I've added a PR to do this (#7706).

@ldlb9527
Copy link
Author

I've added a PR to do this (#7706).

Once this PR is merged and passes the tests, I need to rebase and remove all the CheckRenewalExemptionAtWFE flags, including the ones added by this PR, right?

@beautifulentropy
Copy link
Member

I've added a PR to do this (#7706).

Once this PR is merged and passes the tests, I need to rebase and remove all the CheckRenewalExemptionAtWFE flags, including the ones added by this PR, right?

Yes, exactly.

aarongable
aarongable requested changes Oct 3, 2024
View reviewed changes
features/features.go
// renewal.
//
// TODO(#7511): Remove this feature flag.
CheckRenewalExemptionAtWFE bool
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't immediately remove feature flags -- to ensure ease of deployability alongside existing configuration files, we first deprecate them. This should be moved up to line 31, next to the other deprecated-but-not-yet-deleted feature flags.

ra/ra.go
@@ -677,19 +677,6 @@ func (ra *RegistrationAuthorityImpl) checkInvalidAuthorizationLimit(ctx context.
// rate limit. This rate limit ensures a client can not create more than the
// specified threshold of new orders within the specified time window.
func (ra *RegistrationAuthorityImpl) checkNewOrdersPerAccountLimit(ctx context.Context, acctID int64, names []string, limit ratelimit.RateLimitPolicy) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests are failing because this function no longer uses the input parameter names. That parameter should be removed from this function definition and from all call-sites.

test/config/ra.json
@@ -106,9 +106,7 @@
}
}
},
"features": {
"CheckRenewalExemptionAtWFE": true
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be left in place for now, to reflect the fact that we haven't actually deleted the feature flag from our production configs yet -- that can't happen until after the rest of this change has been merged and deployed.

test/config/wfe2.json
@@ -105,8 +105,7 @@
"authorizationLifetimeDays": 30,
"pendingAuthorizationLifetimeDays": 7,
"features": {
"ServeRenewalInfo": true,
"CheckRenewalExemptionAtWFE": true
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@aarongable aarongable requested review from a team and beautifulentropy and removed request for a team October 3, 2024 23:22
@beautifulentropy
Copy link
Member

beautifulentropy commented Oct 4, 2024 •
edited
Loading

The remaining unit test failures are due to the removal of calls to ra.SA.FQDNSetExists() within RA.checkNewOrdersPerAccountLimit() and RA.checkCertificatesPerNameLimit(). This is expected since these tests were validating a property of these methods that no longer exists: the ability to identify renewals. As this check is now handled at the WFE and renewals no longer reach this code path, I removed the affected tests in PR #7745. Thanks again for your work on this issue; I'm closing this PR in favor of #7745.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongable aarongable aarongable requested changes

@beautifulentropy beautifulentropy Awaiting requested review from beautifulentropy beautifulentropy is a code owner automatically assigned from letsencrypt/boulder-developers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ratelimits: Cleanup CheckRenewalExemptionAtWFE feature flag
3 participants
@ldlb9527 @beautifulentropy @aarongable