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

Align version of dependencies when Fuzzing #2552

Closed
7 tasks done
pjbgf opened this issue Mar 18, 2022 · 9 comments
Closed
7 tasks done

Align version of dependencies when Fuzzing #2552

pjbgf opened this issue Mar 18, 2022 · 9 comments
Labels
area/ci CI related issues and pull requests good first issue Good for newcomers

Comments

@pjbgf
Copy link
Member

pjbgf commented Mar 18, 2022

The fuzz testing should be running against the same dependency versions that are used during testing and release of controllers, for improved reliability of the results.
In projects that depend on libgit2 (e.g. source-controller and image-automation-controller) this is even more so, as both git2go and libgit2 must be within a specific version range for them to work together.

The current implementation manages the fuzzing test code into a secondary go module (i.e. ./tests/fuzz) to avoid contaminating the
main go.mod with numerous dependencies that are not required for the controllers to run. This resulted in a few adhoc solutions which are not currently fully aligned across the projects.

The main change required is for go mod tidy to be replaced with a go get -d github.com/AdaLogics/go-fuzz-headers in projects in which the fuzzing code is copied back into the main modules:

In some projects the fuzz test runs on its own module, meaning that it requires a copy of the current project go.mod on top of the removal of go mod tidy:

This should no longer be required once #2417 lands.

@hiddeco hiddeco added the area/ci CI related issues and pull requests label Mar 18, 2022
@pjbgf pjbgf added the good first issue Good for newcomers label Mar 24, 2022
@ilanpillemer
Copy link

Hi @pjbgf , why is this a good first issue? I am looking around for good first issue to look at.

@pjbgf
Copy link
Member Author

pjbgf commented Mar 25, 2022

Hey @ilanpillemer, this is a straightforward change that needs to be consistent across all repositories.

The first part is pretty much ensure that we remove go mod tidy on all the files linked above. Then ensure that all the dependencies are still in place - in some cases this will require a go get....

Feel free to propose a PR and tag me for review.

@ilanpillemer
Copy link

ilanpillemer commented Mar 26, 2022

@pjbgf I did a grep on the first file in the list (for pkg) and I got three lines with go mod tidy.
Would this change happen in all three places or just the first one (as you just mention the first line in the description above)?
Based on your explanation above, I believe all three should be replaced. Just verifying I am understanding.

tests/fuzz/oss_fuzz_build.sh:37:go mod tidy
tests/fuzz/oss_fuzz_build.sh:51:go mod tidy
tests/fuzz/oss_fuzz_build.sh:60:go mod tidy

@pjbgf
Copy link
Member Author

pjbgf commented Mar 28, 2022

@pjbgf I did a grep on the first file in the list (for pkg) and I got three lines with go mod tidy. Would this change happen in all three places or just the first one (as you just mention the first line in the description above)? Based on your explanation above, I believe all three should be replaced. Just verifying I am understanding.

@ilanpillemer that's correct, in the pkg those 3 instances relate to three separate go modules.

@ilanpillemer
Copy link

@pjbgf as far as I could understand when eyeballing the sh files for tasks 6 and 7, the go.mod file is already being copied, so no changes were needed in that respect for tasks 6 and 7 if that assumption is correct based on that quick glance.

@pjbgf
Copy link
Member Author

pjbgf commented Mar 28, 2022

@ilanpillemer I think the only additional thing was the go get -d for the current project on image-automation-controller, but you already added that. 👍

@ilanpillemer
Copy link

@pjbgf I didnt use the -d flag though when I added it.

@pjbgf
Copy link
Member Author

pjbgf commented Mar 28, 2022

@ilanpillemer that should not be a problem, as it is simply updating the reference against "itself" locally.
Besides, once #2417 lands most of this logic will become redundant.

@ilanpillemer
Copy link

@pjbgf I didn't think it was a problem, as I was assuming it was ephemeral either way. Just mentioned it, in case there was some edge case funny that was not intuitive to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci CI related issues and pull requests good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants