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

Automated fuzzing on Fuzzit #2835

Closed
wants to merge 3 commits into from

Conversation

bookmoons
Copy link

@bookmoons bookmoons commented Sep 8, 2019

Integrates with Fuzzit for automated fuzzing. Proposed in #2670.

Under this setup Travis runs fuzzing on every build. It runs a fuzz regression test locally of past crashes and submits to Fuzzit for a full fuzzing run. The build ends as soon as the fuzzing run is in the queue. Fuzzit emails when it finds crashes.

This tries to fuzz the suggested targets plus some other identified parsers. Targets are:

  • control-api-http - HTTP requests to the control API server.
  • control-api-tcp - Binary requests to the control API server.
  • parse-config - Fuzz the config parser.
  • parse-reference - Fuzz the Docker image reference parser.

There's a JDWP specification parser in pkg/skaffold/debug that seems suitable for fuzzing. It's tough to get at because it's a private function. If you think it's worth fuzzing exposing it would make it possible.

A skaffold org is preconfigured on Fuzzit. If whoever will be administering signs in to create an account, I can get you added to the org.

Thank you for considering.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@bookmoons
Copy link
Author

@googlebot signed it

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@codecov
Copy link

codecov bot commented Sep 8, 2019

Codecov Report

Merging #2835 into master will increase coverage by 1.78%.
The diff coverage is 55.23%.

Impacted Files Coverage Δ
cmd/skaffold/app/cmd/run.go 47.05% <ø> (-5.58%) ⬇️
cmd/skaffold/app/cmd/flags.go 100% <ø> (ø) ⬆️
cmd/skaffold/app/cmd/completion.go 14.28% <0%> (+1.78%) ⬆️
cmd/skaffold/app/cmd/credits/export.go 0% <0%> (ø)
cmd/skaffold/app/cmd/schema/list.go 100% <100%> (ø)
cmd/skaffold/app/cmd/build.go 82.5% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/debug.go 50% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/deploy.go 80.64% <100%> (+2.07%) ⬆️
cmd/skaffold/app/cmd/render.go 42.1% <100%> (ø) ⬆️
cmd/skaffold/app/flags/build_output.go 82.85% <100%> (+4.28%) ⬆️
... and 130 more

@bookmoons
Copy link
Author

@dominikh Hi, I built on your good work for this PR. The googlebot is asking for consent to have your commits merged. Would you mind looking at it?

@bookmoons
Copy link
Author

I see there's a strange error in the build. A type error even though the types are identical.

/home/travis/gopath/src/skaffold/pkg/skaffold/docker/auth.go:69:39: cannot use cf (variable of type *configfile.ConfigFile) as *configfile.ConfigFile value in argument to gcp.AutoConfigureGCRCredentialHelper

Not sure what that's about, it works locally. Will be looking into this.

@dominikh
Copy link

dominikh commented Sep 9, 2019

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 9, 2019
@bookmoons
Copy link
Author

Thanks @dominikh.

@bookmoons bookmoons force-pushed the master branch 3 times, most recently from 1ef5450 to 4205463 Compare September 12, 2019 05:32
@bookmoons
Copy link
Author

Applied the Go modules approach from dvyukov/go-fuzz#195 (comment) to get this working. Thanks @mvdan for the pattern.

This is ready for review.

@balopat
Copy link
Contributor

balopat commented Oct 17, 2019

This looks good but I'm hesitant to add 8 more minutes to our feedback loop. Can you please change it to a job instead of a stage so at least it can run in parallel with the other jobs?

@bookmoons
Copy link
Author

Will do, thanks for looking at it.

dominikh and others added 2 commits October 18, 2019 14:00
Enables automated fuzzing on Fuzzit. Fuzz regression tests run
every push and PR. Full fuzzing runs every push. Uses genetic
fuzzer go-fuzz.

Fuzz targets are:
* parse-config - Configuration file parsing.
* parse-reference - Docker image reference parsing.
* control-api-tcp - Binary requests to control API server.
* control-api-http - HTTP requests to control API server.
@bookmoons
Copy link
Author

Rebased this and converted the fuzz stages to jobs. I'll check back to see how the build goes.

If you'd like to sign in to Fuzzit to make an account I can add you to the org. Then final setup is like this:

  • In Fuzzit, switch to the skaffold org with the dropdown at the top.
  • In settings grab an API key.
  • In repo settings in Travis paste it to envvar FUZZIT_API_KEY.

@tejal29 tejal29 added kokoro:run runs the kokoro jobs on a PR and removed kokoro:run runs the kokoro jobs on a PR needs-rebase labels Oct 18, 2019
@dgageot
Copy link
Contributor

dgageot commented Jan 15, 2020

@balopat do you think we should merge this (when it's rebased)?

@dgageot dgageot assigned dgageot and balopat and unassigned dgageot Jan 15, 2020
@balopat
Copy link
Contributor

balopat commented Jan 15, 2020

I'm not convinced. One thing is the slowness of the test, the other is that most of the test code seems to be HTTP specific instead of Skaffold's test code. It feels like this kind of code should live in a fuzz-http library or similar...I'm not sure I want to maintain this.

@dgageot
Copy link
Contributor

dgageot commented Jan 15, 2020

@balopat I agree. I'm sorry @bookmoons but I think maintaining that would cost us more that it brings.

@balopat
Copy link
Contributor

balopat commented Jan 15, 2020

Sounds good. Apologies for dragging this out this long. Closing.

@balopat balopat closed this Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants