Fix verify logs repetition #344
Conversation
…earch) Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
| @@ -103,7 +106,13 @@ func (s *MemorySource) LoadEnvelope(reference string, env dsse.Envelope) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *MemorySource) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) { | |||
| func (s *MemorySource) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) { | |||
There was a problem hiding this comment.
@mikhailswift I found the issue which was producing the false negative results and it was the limiting of the memory source searches. I have now passed in the search dept integer to the Sourcer interface, which feels slightly wrong as only the memory source search needs it. What do you think?
There was a problem hiding this comment.
I don't think the memory source needs the depth -- and I don't think we can actually skip over subsequent memory searches where depth > 0.
If we do one round of searches, and find some attestations that match, and others that don't, but then find some back ref subjects on one of the matched attestations, we may discover more that match on the next iteration of searches.
There was a problem hiding this comment.
The current tests are passing even with this change, but I suspect it is not correct. Let me try to write a test that can demonstrate.
|
If you cherry-pick the two commits from #345 onto your branch, this does indeed break policy evaluation when we need to search for back refs. |
|
@mikhailswift thanks for review here. I will set as a requirement for this PR that we have an improvement to the current tests. |
|
Update: I am converting this to draft as @mikhailswift has highlighted some still existing issues with the code change. On top of that, additional tests should be added before this is ready for final review and merge. |
What this PR does / why we need it
Reopening this PR after #317 needed reverted due to issues with the attempts at limiting memory search invocations.
This PR introduces the same changes, but directly uses the search depth to decide whether to perform a search or not.
I am unsure whether changing the interface is the right way to go here, but figured I would raise the pull request for review anyhow.