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
Change BulkDecrypt to not rely on type tests #9373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me sad that we need to do this, but it does seem like the right choice given the bug. LGTM.
func (c *mapDecrypter) BulkDecrypt(ciphertexts []string) (map[string]string, error) { | ||
// Loop and find the entries that are already cached, then BulkDecrypt the rest | ||
secretMap := map[string]string{} | ||
var toDecrypt []string | ||
if c.cache == nil { | ||
// Don't bother searching for the cached subset if the cache is nil | ||
toDecrypt = ciphertexts | ||
} else { | ||
toDecrypt = make([]string, 0) | ||
for _, ct := range ciphertexts { | ||
if plaintext, ok := c.cache[ct]; ok { | ||
secretMap[ct] = plaintext | ||
} else { | ||
toDecrypt = append(toDecrypt, ct) | ||
} | ||
} | ||
} | ||
|
||
// try and bulk decrypt the rest | ||
decrypted, err := c.decrypter.BulkDecrypt(toDecrypt) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// And add them to the cache | ||
if c.cache == nil { | ||
c.cache = make(map[string]string) | ||
} | ||
|
||
for ct, pt := range decrypted { | ||
secretMap[ct] = pt | ||
c.cache[ct] = pt | ||
} | ||
|
||
return secretMap, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think current data flow is such that no this will never be hit, but it was pretty easy to write and means we won't trip up on a bad method if we refactor things slightly in the future that cause it to start being hit.
Description
BulkDecrypt
only made use of the bulk decryption if theDecrypter
passed in was itself aBulkDecrypter
. Only theserviceCrypter
implemented this interface but most of the time it's wrapped with acachingCrypter
.Rather than just adding BulkDecrypt to
cachingCrypter
I've just moved it to theDecrypter
interface and ensured that every implementation either does something efficient if it can, or callsDefaultBulkDecrypt
to just loop over 1-by-1.Fixes #9350
Checklist