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

Consider dropping external fuzz testing from oss-fuzz #9548

Open
killianmuldoon opened this issue Oct 12, 2023 · 7 comments
Open

Consider dropping external fuzz testing from oss-fuzz #9548

killianmuldoon opened this issue Oct 12, 2023 · 7 comments
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@killianmuldoon
Copy link
Contributor

We added fuzz testing from the oss-fuzz project as part of #6059, with the implementation done here: https://github.com/cncf/cncf-fuzzing/tree/main/projects/cluster-api

The implementation is currently undermaintained as it's completely external to the project and has low visibility. Building fails after changes are made to method signatures and need to be fixed. It is the responsibility of CAPI maintainers to keep these tests healthy, but they've been failing for a long time without being fixed (I'm definitely part of the problem here 😅 )

These tests are also complicated to maintain, and maintenance hasn't been done on them. They currently provide no signal for CAPI as they're failing to build.

I think we should try to replace these fuzz tests with an in-repo version.

This would involve:

  • Reviewing the current tests in the oss-fuzz repo.
  • Assessing whether native go fuzz can replace most or all of them.
  • Replacing these tests with new fuzz tests
  • Removing CAPI tests from cncf-fuzzing
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 12, 2023
@sbueringer
Copy link
Member

sbueringer commented Oct 17, 2023

Absolutely agree with that what we currently have is not very useful in its current state. I think right now we simply don't have the bandwith to deal with this, also not sure if it's worth a significant investment.

@killianmuldoon
Copy link
Contributor Author

/assign

@killianmuldoon
Copy link
Contributor Author

/triage accepted

I opened PRs on the relevant repos to remove the CAPI project and fuzzers.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 7, 2023
@killianmuldoon
Copy link
Contributor Author

Update with some context: cncf/cncf-fuzzing#461 (comment)

One of the folks working on CNCF fuzzing has volunteered to bring the fuzzers upstream. Let's see what that looks like in implementation, but it could be a step forward.

@AdamKorcz it would be good if you could explain what bringing the fuzzers upstream would look like to make sure it's acceptable in upstream Cluster API.

@AdamKorcz
Copy link

AdamKorcz commented Nov 13, 2023

I was planning on rewriting the fuzzers to native Go fuzz tests that can be run from the command line with go test -fuzz= and place the fuzzers in the directories that they test which is the recommended way of structuring tests in go. I would also add the build.sh file to CAPI so that maintainers easily can modify it.

@sbueringer
Copy link
Member

sbueringer commented Nov 14, 2023

Sounds like it would be a huge improvement. In any case we'll need someone who has the bandwith to fix potential findings, but maybe that's easier to achieve if we have an easy way to reproduce the failures.

Would be probably also good if there's an easy way to ignore findings if we think a specific issue doesn't have to be fixed. But I assume if the test is part of the CAPI repo we can just modify the test code for that.

@fabriziopandini
Copy link
Member

/kind cleanup
/priority backlog

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants