Skip to content

Add Continuous Fuzzing via Fuzzit#5890

Merged
juliusv merged 2 commits intoprometheus:masterfrom
fuzzitdev:fuzzit-integration
Aug 21, 2019
Merged

Add Continuous Fuzzing via Fuzzit#5890
juliusv merged 2 commits intoprometheus:masterfrom
fuzzitdev:fuzzit-integration

Conversation

@yevgenypats
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats commented Aug 14, 2019

This PR adds a continuous fuzzing integration to prometheus's travis pipeline via Fuzzit service.

This means the following:

  • Every time new code is pushed to master new fuzzers are built and push to fuzzit where they run continuously generate new test-cases and alerts if new crashes are found
  • Every pull-request the fuzzers runs through a quick regression tests with the generated test-cases from the previous step. If something new/old bugs are introduced you will see this immediately in the Travis check.

To take ownership of the organisation, please login to https://app.fuzzit.dev and let me know your account.

Please review and feel free to comment/ask questions.

@yevgenypats yevgenypats changed the title Add Continuous Fuzzing via Fuzzit WIP: Add Continuous Fuzzing via Fuzzit Aug 14, 2019
@yevgenypats
Copy link
Copy Markdown
Contributor Author

yevgenypats commented Aug 14, 2019

@juliusv @kjk @msiebuhr CCing you here.

@yevgenypats yevgenypats force-pushed the fuzzit-integration branch 6 times, most recently from 1159d6e to 23ba2f9 Compare August 15, 2019 21:22
@yevgenypats yevgenypats changed the title WIP: Add Continuous Fuzzing via Fuzzit Add Continuous Fuzzing via Fuzzit Aug 15, 2019
@yevgenypats
Copy link
Copy Markdown
Contributor Author

@juliusv @kjk @msiebuhr This PR is ready for review.

@yevgenypats
Copy link
Copy Markdown
Contributor Author

@juliusv @mseinhur ping. Can you review this?

Copy link
Copy Markdown
Contributor

@msiebuhr msiebuhr left a comment

Choose a reason for hiding this comment

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

While I implemented the first round of fuzzing some time (years?) ago, I must admit not being up to speed with Prometheus nor familiar with fuzzit, so I've stuck to general observations.

Comment thread .travis.yml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The first comment in fuzzit.sh says it should be called with either "fuzzing" or "local-regression".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah. I see from the bottom of fuzzit.sh that it will auto-select "local-regression" for pull-requests and "fuzzing" for everything else. Perhaps this should be encoded in a special argument? ./fuzzit.sh auto-select-for-travis, so there's no doubt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed. This is old docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes probably we will add something like this in the near future but for now I'm trying to avoid CI specific logic inside the CLI.

Comment thread fuzzit.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make better sense to write it in a single loop?

TARGETS=("FuzzParseMetric" "...")
for TARGET in "${TARGETS[@]}"
do
    go-fuzz-build -libfuzzer -func "$TARGET" -o "$TARGET.a" ./promql
    clang -fsanitize=fuzzer "$TARGET".a -o "$TARGET"
    rm "$TARGET".a
done

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes this would be better but the problem is that the name of targets on fuzzit and the function names are not the same. i.e -func $TARGET won't work. maybe I can do another array with FUZZ_FUNCTIONS?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@msiebuhr I change it to one loop, let me know if you think this is better?

Comment thread fuzzit.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Many places don't install wget by default. curl -s -o fuzzit https://github.com/fuzzitdev/fuzzit/releases/download/v2.4.23/fuzzit_Linux_x86_64 usually brings better luck.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can change this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The curl command doesnt work:( so I changed it back to wget it's available in travis so I hope it's not a problem.

@yevgenypats yevgenypats force-pushed the fuzzit-integration branch 3 times, most recently from 9cd54c0 to 298ecda Compare August 20, 2019 07:26
Comment thread .travis.yml Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add a comment what this env var is for (since you can't read its contents in the encrypted form here)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Comment thread fuzzit.sh Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add more comments here, and also for the go-fuzz-build steps above, to explain what their purpose is (for someone who isn't familiar with all the fuzz tools and steps of fuzzing)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

@juliusv
Copy link
Copy Markdown
Member

juliusv commented Aug 20, 2019

Thanks! General Prometheus style comment: aim to make comments sentence-style (start capitalized, end with period).

Otherwise looks good, though I'm neither a fuzzing nor CI expert.

@simonpasquier you are the one with the most touched lines in .travis.yml so if you could just take a last look before merging if I missed anything serious, that'd be cool :)

@yevgenypats yevgenypats force-pushed the fuzzit-integration branch 2 times, most recently from 1197af4 to 6eb8b7d Compare August 20, 2019 07:50
@yevgenypats
Copy link
Copy Markdown
Contributor Author

@juliusv and @msiebuhr Thanks for the quick review! Done regarding the comments style as well.

Signed-off-by: Krzysztof Kowalczyk <kkowalczyk@gmail.com>
@yevgenypats
Copy link
Copy Markdown
Contributor Author

All checks passes! Let's merge!:)

Copy link
Copy Markdown
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the PR in details but in general I'm in favor of consolidating on Circle CI (nothing against Travis but Circle handles the build/release workflow in addition to test while Travis only runs tests). Would it be possible to apply this to .circleci/config.yml instead?

@yevgenypats
Copy link
Copy Markdown
Contributor Author

Hi @simonpasquier I guess it would be possible. Though It was harder to do that in circle for some reason I don't recall now. Would it be possible to do that in another PR?

@simonpasquier
Copy link
Copy Markdown
Member

I'd rather see it in Circle as eventually we don't want to maintain duplicate CI systems.

@yevgenypats
Copy link
Copy Markdown
Contributor Author

Ok, got it. will try to migrate this to Circle now.

@yevgenypats yevgenypats force-pushed the fuzzit-integration branch 10 times, most recently from 345cbbb to 3a37309 Compare August 20, 2019 20:21
@yevgenypats
Copy link
Copy Markdown
Contributor Author

@simonpasquier done. The fuzzing regression passes. This is ready for review. Also some of you need to sign up to https://app.fuzzit.dev and let me know what's your username so I can put as an admin of prometheus. once that done you will need to copy the api key from fuzzit to CircleCI secret as FUZZIT_API_KEY

@yevgenypats
Copy link
Copy Markdown
Contributor Author

All checks passes!

@juliusv
Copy link
Copy Markdown
Member

juliusv commented Aug 21, 2019

@yevgenypats Thanks for all the adjustments, especially the CircleCI port! Looks great to my naive eyes now - in principle ready to merge from my perspective, could you just add the DCO Signed-Off-By line to your last commit so that our DCO check passes?

I created a Fuzzit account for my GitHub user (juliusv - julius.volz@gmail.com).

Signed-off-by: Yevgeny Pats <yp@fuzzit.dev>
@yevgenypats
Copy link
Copy Markdown
Contributor Author

Done. Added you to Prometheus org. You need to copy the api key from here https://app.fuzzit.dev/orgs/prometheus/settings to CircleCi Environment as FUZZIT_API_KEY

@juliusv
Copy link
Copy Markdown
Member

juliusv commented Aug 21, 2019

@yevgenypats Thanks! I added the environment variable on CircleCI. Gonna merge once the last check turns green :)

@juliusv juliusv merged commit 0e1767b into prometheus:master Aug 21, 2019
@yevgenypats
Copy link
Copy Markdown
Contributor Author

yevgenypats commented Aug 21, 2019 via email

@simonpasquier
Copy link
Copy Markdown
Member

Thanks! Could you add me too (simonpasquier - pasquier.simon_at_gmail.com) please?

@yevgenypats
Copy link
Copy Markdown
Contributor Author

@simonpasquier done. you should have access now to https://app.fuzzit.dev/orgs/prometheus/dashboard

@yevgenypats
Copy link
Copy Markdown
Contributor Author

@juliusv
Copy link
Copy Markdown
Member

juliusv commented Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants