Skip to content

ratelimits: Fix two transaction construction bugs#7200

Merged
beautifulentropy merged 11 commits into
mainfrom
ratelimits-bug-fixes
Jan 8, 2024
Merged

ratelimits: Fix two transaction construction bugs#7200
beautifulentropy merged 11 commits into
mainfrom
ratelimits-bug-fixes

Conversation

@beautifulentropy

@beautifulentropy beautifulentropy commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member
  • Update parsing of overrides with Ids formatted as 'fqdnSet' to produce a hexadecimal string.
  • Update validation for Ids formatted as 'fqdnSet' when constructing a bucketKey for a transaction to validate before identifier construction.
  • Skip CertificatesPerDomain transactions when the limit is disabled.

Blocks #7201
Part of #5545

@beautifulentropy beautifulentropy requested a review from a team as a code owner December 8, 2023 22:49
@beautifulentropy beautifulentropy mentioned this pull request Dec 8, 2023
Merged
aarongable
aarongable reviewed Dec 11, 2023
View reviewed changes
Comment thread ratelimits/bucket.go
Comment thread ratelimits/bucket.go Outdated
@beautifulentropy beautifulentropy requested review from a team and aarongable December 11, 2023 20:33
aarongable
aarongable previously approved these changes Dec 11, 2023
View reviewed changes
Comment thread ratelimits/limit.go Outdated
@aarongable aarongable requested a review from a team December 11, 2023 20:35
@beautifulentropy @aarongable
Update ratelimits/limit.go
bf75cca 
Co-authored-by: Aaron Gable <aaron@letsencrypt.org>
aarongable
aarongable reviewed Dec 12, 2023
View reviewed changes
Comment thread ratelimits/bucket.go Outdated
Comment thread ratelimits/bucket.go Outdated
Comment thread ratelimits/limit.go Outdated
aarongable
aarongable reviewed Dec 13, 2023
View reviewed changes
Comment thread ratelimits/bucket.go Outdated
Comment thread ratelimits/names.go Outdated
jsha
jsha requested changes Dec 15, 2023
View reviewed changes

@jsha jsha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From the PR description:

Update parsing of overrides with Ids formatted as 'fqdnSet' to produce a hexadecimal string.

AFAICT this PR doesn't change parsing of FQDNSet overrides. It changes generation of bucket keys so they are generated as hex strings. Is that right?

Comment thread ratelimits/bucket.go Outdated
@beautifulentropy

beautifulentropy commented Dec 18, 2023

Copy link
Copy Markdown
Member Author

From the PR description:

Update parsing of overrides with Ids formatted as 'fqdnSet' to produce a hexadecimal string.

AFAICT this PR doesn't change parsing of FQDNSet overrides. It changes generation of bucket keys so they are generated as hex strings. Is that right?

On lines 161 and 204 of ratelimits/limit.go: when we parse a CertificatesPerFQDNSet overrides and construct the key to lookup this override we are also generating hex strings. This is identical to the change made to the bucket keys but specific to the parsing of overrides.

@beautifulentropy beautifulentropy mentioned this pull request Dec 18, 2023
Merged
@jsha

jsha commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

On lines 161 and 204 of ratelimits/limit.go

Aha, thanks for pointing that out 👍🏻

jsha
jsha previously approved these changes Dec 18, 2023
View reviewed changes
aarongable
aarongable previously approved these changes Dec 18, 2023
View reviewed changes

@aarongable aarongable left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM as-is if you want to land it right now, or happy to re-review if you have changes you want to make based on #7218

@beautifulentropy beautifulentropy dismissed stale reviews from aarongable and jsha via 6c74095 December 19, 2023 19:28
This was referenced Dec 20, 2023
Closed
Open
@beautifulentropy beautifulentropy merged commit 4e5105d into main Jan 8, 2024
@beautifulentropy beautifulentropy deleted the ratelimits-bug-fixes branch January 8, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgporada pgporada pgporada approved these changes

@aarongable aarongable aarongable approved these changes

@jsha jsha Awaiting requested review from jsha

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@beautifulentropy @jsha @pgporada @aarongable