Fixes the error handling in kms/hashivault #1497
Conversation
Previously with this patch sigstore#1414, the error was handled but the order of the error was not correct, because while using Tekton Chains with hashivault, chains controller was getting paniced because the the order of error handled was not correct Hence, with this patch, error is handled correctly and also a condition is added to check if the item retrieved is nil or not to avoid Tekton Chains controller pod crashing Signed-off-by: PuneetPunamiya <ppunamiy@redhat.com>
PuneetPunamiya
force-pushed
the
fix-crashing
branch
from
b41131b
to
19443d7
Compare
| @@ -246,12 +246,17 @@ func (h *hashivaultClient) public() (crypto.PublicKey, error) { | |||
| return nil | |||
| }, | |||
| ) | |||
|
|
|||
| item := h.keyCache.Get(cacheKey, ttlcache.WithLoader[string, crypto.PublicKey](loader)) | |||
| if lerr != nil { | |||
There was a problem hiding this comment.
Yeah before we added this up, but the chains controller pod was failing
Also as far as I understood, loader was called when we are assigning the value to item and hence if we add this error check up, at that time error is nil
Adding it here, I think handles the error properly
There was a problem hiding this comment.
Good catch - If I'm understanding this correctly, lerr will always be nil in the previous code because it only gets set once the loader is invoked, which is in the Get call.
There was a problem hiding this comment.
yes exactly, and hence even if there was an error, it was not returned earlier and the item would be nil which will indeed panic the code
Hence with the current patch, the error would be handled correctly
| @@ -246,12 +246,17 @@ func (h *hashivaultClient) public() (crypto.PublicKey, error) { | |||
| return nil | |||
| }, | |||
| ) | |||
|
|
|||
| item := h.keyCache.Get(cacheKey, ttlcache.WithLoader[string, crypto.PublicKey](loader)) | |||
| if lerr != nil { | |||
There was a problem hiding this comment.
Good catch - If I'm understanding this correctly, lerr will always be nil in the previous code because it only gets set once the loader is invoked, which is in the Get call.
| if lerr != nil { | ||
| return nil, lerr | ||
| } | ||
|
|
||
| item := h.keyCache.Get(cacheKey, ttlcache.WithLoader[string, crypto.PublicKey](loader)) | ||
| return item.Value(), lerr | ||
| if item == nil { |
There was a problem hiding this comment.
Is this just to be defensive, or is there a case where an item won't be found in the cache and lerr will be nil?
There was a problem hiding this comment.
I would say, just to be more defensive,
Because if we look at this function I see close to zero chances an item will be nil, if lerr is nil
|
@cpanato Can you take a look too? |
|
thanks! |
Previously with this patch Handles the error in kms/hashivault #1414,
the error was handled but the order of the error was not correct, because
while using Tekton Chains with hashivault, chains controller was getting
paniced because the order of error handled was not correct
Hence, with this patch, error is handled correctly and also a condition is
added to check if the item retrieved is nil or not to avoid Tekton Chains
controller pod crashing