ratify icon indicating copy to clipboard operation
ratify copied to clipboard

docs: Create proposal for annotation-based filtering

Open asafalgawi opened this issue 1 year ago • 3 comments

Description

What this PR does / why we need it:

This PR adds a proposal for filtering artifacts based on their annotations before forwarding them to verification by one of the configured verifiers.]

The PR refers to issue #1772

Type of change

Added document to proposal directory.

asafalgawi avatar Sep 12 '24 10:09 asafalgawi

Hi @asafalgawi , thank you for the proposal. We have discussed this during the community meeting. We agree with the "latest" vulnerability report user scenario, however we need to understand the feasibility and scope of change. @binbin-li brought up good point today, the individual verifier might not have filtering abilities. Major refactoring might be required in executor code path. We still require more time to understand the notary scenarios a bit more, thankyou for your patience.

susanshi avatar Sep 19 '24 05:09 susanshi

Hi @susanshi, thanks for the reviewing the proposal and for the update :)

asafalgawi avatar Sep 19 '24 07:09 asafalgawi

Hi @asafalgawi , thank you for the proposal. We have discussed this during the community meeting. We agree with the "latest" vulnerability report user scenario, however we need to understand the feasibility and scope of change. @binbin-li brought up good point today, the individual verifier might not have filtering abilities. Major refactoring might be required in executor code path. We still require more time to understand the notary scenarios a bit more, thankyou for your patience.

Hi @asafalgawi, reviewing our current executor path below, we want to explore a bit more on the proposed implementation. How will the executor be refactored for filtering? Note we will not be able to change the verifier interfaces to maintain backward compatibility. If the current PR review meeting and community meeting time doesn't work, would you be open to meet and discuss in the once off meeting?

for _, referrerStore := range executor.ReferrerStores {
	for _, reference := range referrersResult.Referrers {  
		for _, verifier := range executor.Verifiers {
                }
        }
}
```	

susanshi avatar Sep 23 '24 01:09 susanshi

@susanshi I've updated the proposal based on our conversation, also added two implementation proposal. a "simple" one which theoretically impacts Ratify's scalability, the second, a bit more complex, which affects Ratify executor design

asafalgawi avatar Oct 08 '24 16:10 asafalgawi

@susanshi I've updated the proposal based on our conversation, also added two implementation proposal. a "simple" one which theoretically impacts Ratify's scalability, the second, a bit more complex, which affects Ratify executor design

thanks for the updates and comparison between the two implementation methods. My understanding option1 is more straight forward, and given a 1k referrer list, there are no perf and memory concerns. Option 2 memory benefits is unclear. Major refactoring in the executor is risky, unless the refactoring improves code readability /allows for easier future maintaince/ aligns with the direction of ratify v2 executor change. @binbin-li , do you see for executor refactoring benefits for ratify v2?

susanshi avatar Oct 10 '24 19:10 susanshi

@asafalgawi , just a note, we are not a cncf sandbox project and there is a new requirement on DCO. You might need to squash the current commits and sign a new commit with the -s flag , git commit -s -S -m <commit_message>. see details at https://github.com/ratify-project/ratify/blob/dev/CONTRIBUTING.md#commit :)

susanshi avatar Oct 10 '24 19:10 susanshi

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

see 28 files with indirect coverage changes

codecov[bot] avatar Oct 14 '24 22:10 codecov[bot]

@susanshi I've updated the proposal based on our conversation, also added two implementation proposal. a "simple" one which theoretically impacts Ratify's scalability, the second, a bit more complex, which affects Ratify executor design

thanks for the updates and comparison between the two implementation methods. My understanding option1 is more straight forward, and given a 1k referrer list, there are no perf and memory concerns. Option 2 memory benefits is unclear. Major refactoring in the executor is risky, unless the refactoring improves code readability /allows for easier future maintaince/ aligns with the direction of ratify v2 executor change. @binbin-li , do you see for executor refactoring benefits for ratify v2?

For this feature, I'd like to keep executor untouched. For v2 design, I plan to make executor just handle the core coordination among different components(policy, verifier, store). The last-n feature and possibly more filtering options sound more like a verifier feature, therefore executor is not a good place to handle it.

binbin-li avatar Oct 16 '24 14:10 binbin-li

@asafalgawi I agree with Susan and I think Option 1 seems like a better approach for now. If larger than 1k referrers becomes a user ask, we can revisit this.

akashsinghal avatar Oct 16 '24 17:10 akashsinghal

Great I'll start with a POC for option 1

Get Outlook for Androidhttps://aka.ms/AAb9ysg


From: Akash Singhal @.> Sent: Wednesday, October 16, 2024 8:36:54 PM To: ratify-project/ratify @.> Cc: Asaf Algawi @.>; Mention @.> Subject: Re: [ratify-project/ratify] docs: Create proposal for verifying 'last-n' artifacts only. (PR #1797)

@asafalgawihttps://github.com/asafalgawi I agree with Susan and I think Option 1 seems like a better approach for now. If larger than 1k referrers becomes a user ask, we can revisit this.

— Reply to this email directly, view it on GitHubhttps://github.com/ratify-project/ratify/pull/1797#issuecomment-2417484847, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BCRNDOFRKWSDBXYYW4SNGNTZ32P3NAVCNFSM6AAAAABOC5QPAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGQ4DIOBUG4. You are receiving this because you were mentioned.Message ID: @.***>

asafalgawi avatar Oct 16 '24 17:10 asafalgawi