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

pgcryptocipherccl: convert invariant errors into assertions #110774

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

andyyang890
Copy link
Collaborator

This patch changes the checks for programmer error in the pgcryptocipherccl package to use errors.AssertionFailedf so that they are surfaced with greater visibility.

Epic: None

Release note: None

@andyyang890 andyyang890 requested a review from a team as a code owner September 16, 2023 20:03
@blathers-crl
Copy link

blathers-crl bot commented Sep 16, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andyyang890 andyyang890 requested a review from rafiss September 16, 2023 20:03
@andyyang890 andyyang890 force-pushed the pgcrypto_cipher_assertions branch 2 times, most recently from 76c292c to a46ce56 Compare September 16, 2023 21:38
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm, but just one nit! just to check - none of these errors can occur from strange user input, right?

pkg/ccl/pgcryptoccl/pgcryptocipherccl/cipher.go Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@andyyang890 andyyang890 left a comment

Choose a reason for hiding this comment

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

just to check - none of these errors can occur from strange user input, right?

Yup! The assertions in cipher.go should only be hit if we forgot a enum case in the switch statement and the assertion in pkcsPad would only happen if we passed in an invalid block size (not directly configurable by user). Neither of these is currently happening.

pkg/ccl/pgcryptoccl/pgcryptocipherccl/cipher.go Outdated Show resolved Hide resolved
This patch changes the checks for programmer error in the `pgcryptocipherccl`
package to use `errors.AssertionFailedf` so that they are surfaced with
greater visibility.

Release note: None
@andyyang890 andyyang890 force-pushed the pgcrypto_cipher_assertions branch from a46ce56 to 0e38f38 Compare September 18, 2023 14:26
@andyyang890
Copy link
Collaborator Author

tftr!

bors r=rafiss

@craig craig bot merged commit 0b2ced7 into cockroachdb:master Sep 18, 2023
@craig
Copy link
Contributor

craig bot commented Sep 18, 2023

Build succeeded:

@andyyang890 andyyang890 deleted the pgcrypto_cipher_assertions branch September 18, 2023 23:52
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.

3 participants