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

fuzzing: add fuzzers from cncf-fuzzing #6569

Closed
wants to merge 7 commits into from
Closed

Conversation

AdamKorcz
Copy link
Contributor

Description

Issue reference

Moves the fuzzers to Daprs repository from cncf-fuzzing.

cc @ItalyPaleAle

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@AdamKorcz AdamKorcz requested review from a team as code owners June 21, 2023 12:02
@AdamKorcz AdamKorcz force-pushed the fuzz1 branch 6 times, most recently from a5f46b9 to 7e6608e Compare June 21, 2023 15:23
@ItalyPaleAle
Copy link
Contributor

@AdamKorcz this is great, thanks :) How are these being used? How can we trigger fuzz tests ourselves?

@AdamKorcz AdamKorcz force-pushed the fuzz1 branch 4 times, most recently from ca76a5f to 5628c3c Compare June 21, 2023 16:52
@JoshVanL
Copy link
Contributor

Thanks from me as well @AdamKorcz 😄

I'm also been doing some fuzz testing with our integration tests here. I think it's definitely a good idea for us to have fuzz testing at both the unit and integration level.

As a project, I think we should be using the same underlying fuzz testing library, so we need to decide on either github.com/AdaLogics/go-fuzz-headers or github.com/google/gofuzz. The reason I picked gofuzz was that this is what Kubernetes is using upstream, which the project is already heavily dependent on anyway. Does go-fuzz-headers provide us with functionality that gofuzz cannot?

Also notice you are using a personal fork of the upstream go-fuzz-headers, is there a particular reason for that @AdamKorcz? As a project, I don't think we should be relying on personal projects- particularly forks unless there is good reason for it.

@AdamKorcz
Copy link
Contributor Author

I'm also been doing some fuzz testing with our integration tests

That is great!

Does go-fuzz-headers provide us with functionality that gofuzz cannot?

I have not used gofuzz extensively. When I tried it (a couple of years ago) I found it to be ineffective. The project is more or less unmaintained, and I am not able to improve it. All projects here use go-fuzz-headers (along with containerd, Istio and non-CNCF projects as well): https://github.com/cncf/cncf-fuzzing/tree/main/projects.

Also notice you are using a personal fork of the upstream go-fuzz-headers, is there a particular reason for that @AdamKorcz?

This is because my branch has breaking changes that I want to upstream. I should do that within 1-2 weeks.

@AdamKorcz
Copy link
Contributor Author

How are these being used?

They are running continuously on OSS-Fuzz

How can we trigger fuzz tests ourselves?

You can run them locally with go test -fuzz=

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.12% ⚠️

Comparison is base (26e8352) 64.99% compared to head (25e7db1) 64.87%.

❗ Current head 25e7db1 differs from pull request most recent head c5e0c15. Consider uploading reports for the commit c5e0c15 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6569      +/-   ##
==========================================
- Coverage   64.99%   64.87%   -0.12%     
==========================================
  Files         228      232       +4     
  Lines       20823    20646     -177     
==========================================
- Hits        13533    13395     -138     
+ Misses       6163     6135      -28     
+ Partials     1127     1116      -11     

see 77 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoshVanL
Copy link
Contributor

I have not used gofuzz extensively. When I tried it (a couple of years ago) I found it to be ineffective.

Thanks, I am very interested to know what was ineffective. This package is now heavily used in our integration tests and we should change that now to avoid a larger rewrite in future if it is not good enough.

This is because my branch has breaking changes that I want to upstream. I should do that within 1-2 weeks.

👍

@AdamKorcz AdamKorcz closed this Jul 5, 2023
@AdamKorcz AdamKorcz reopened this Jul 5, 2023
@@ -0,0 +1,75 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Because fuzz tests can be compute-intensive, should we add a build tag so they're only run when those tests are invoked?

for example, build tag fuzztesting and then we add a command to the makefile make test-fuzz?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you run the fuzzers without the -fuzz flag, then they will run as unit tests, and as such, I don't believe a dedicated make rule is necessary to run the fuzzers.

Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
@AdamKorcz
Copy link
Contributor Author

The failing CI tests look unrelated to me.

@dapr-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot added the stale Issues and PRs without response label Nov 17, 2023
@dapr-bot
Copy link
Collaborator

This pull request has been automatically closed because it has not had activity in the last 67 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues and PRs without response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants