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

fix: return the last verified version that fits the constraint #574

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Jan 16, 2025

@Skarlso Skarlso requested a review from a team as a code owner January 16, 2025 08:49
Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

@Skarlso Skarlso force-pushed the fix-verified-version-deployment branch from bed93ef to 39c3b7f Compare January 16, 2025 08:55
Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

@Skarlso Skarlso force-pushed the fix-verified-version-deployment branch from e2649b6 to b214c6b Compare January 16, 2025 09:55
Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

Signed-off-by: Gergely Brautigam <[email protected]>
Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

Copy link
Contributor

@frewilhelm frewilhelm left a comment

Choose a reason for hiding this comment

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

lgtm

e2e/main_test.go Show resolved Hide resolved
pkg/ocm/fakes/fakes.go Outdated Show resolved Hide resolved
pkg/ocm/ocm.go Outdated Show resolved Hide resolved
Co-authored-by: Frederic Wilhelm <[email protected]>
Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

frewilhelm
frewilhelm previously approved these changes Jan 16, 2025
@Skarlso Skarlso added the area/ipcei Important Project of Common European Interest label Jan 16, 2025
@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 16, 2025

I'm waiting for Dan or the other person to test this PR.

@dee0sap
Copy link
Contributor

dee0sap commented Jan 16, 2025

Hey @Skarlso

Someone here was just testing this and noticed that the verify is repeatedly happening for all versions in the repo.

Looking at your change I see this is because you put the verify in ListComponentVersions.

Given the expense of Verify probably the check should be in GetLatestValidComponentVersion in this loop

for _, v := range versions {
		if valid, _ := constraint.Validate(v.Semver); valid {
                       if /* make verify check here */ {
                           return v.Version, nil
                       }
		}
	}

This way the expensive verify check is happening as few times as possible.

Oh, and another possible optimization....

In VerifyComponent I think there could be a big performance boost if there was first just a check to see if the ComponentDescriptor has all the signatures required by the ComponentVersion. From what I see in the code what I believe is happening is that we are going through and computing digests first.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 16, 2025

The whole thing could be rewritten but I just don't have the capacity for that right now. I'll make the adjustment though and put it into the semver constraint check. :)

@Skarlso Skarlso dismissed stale reviews from jakobmoellerdev and frewilhelm via a65b7fe January 16, 2025 21:25
…ponent-model/ocm-controller into fix-verified-version-deployment
@Skarlso Skarlso force-pushed the fix-verified-version-deployment branch from a65b7fe to e654954 Compare January 16, 2025 21:26
Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

@dee0sap
Copy link
Contributor

dee0sap commented Jan 16, 2025

Sounds good @Skarlso

For VerifyComponent, should I add an issue? Perhaps a feature enhancement?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 17, 2025

Yes sure. Also the whole thing is doing a lookup twice instead of just getting a version.

Copy link

Mend Scan Summary: ❌

Repository: open-component-model/ocm-controller

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 1
LICENSE RISK HIGH 9
RESTRICTED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

@Skarlso Skarlso merged commit 287a715 into main Jan 17, 2025
8 checks passed
@Skarlso Skarlso deleted the fix-verified-version-deployment branch January 17, 2025 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ipcei Important Project of Common European Interest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OCM-Controller skips verified components if they aren’t the latest
4 participants