Merge pull request #416 from pjbgf/fix-broken-fuzz

build: Improve fuzz tests' reliability

runtime/conditions/patch_fuzz_test.go

@@ -0,0 +1,55 @@
+package conditions
+
+import (
+	"testing"
+
+	fuzz "github.com/AdaLogics/go-fuzz-headers"
+	"github.com/fluxcd/pkg/runtime/conditions/testdata"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func Fuzz_PatchApply(f *testing.F) {
+	f.Fuzz(func(t *testing.T,
+		beforeData, afterData, setterData []byte) {
+
+		before, err := newFake(fuzz.NewConsumer(beforeData))
+		if err != nil {
+			return
+		}
+
+		after, err := newFake(fuzz.NewConsumer(afterData))
+		if err != nil {
+			return
+		}
+
+		patch := NewPatch(before, after)
+		setter, err := newFake(fuzz.NewConsumer(setterData))
+		if err != nil {
+			return
+		}
+
+		_ = patch.Apply(setter)
+	})
+}
+
+func newFake(fc *fuzz.ConsumeFuzzer) (*testdata.Fake, error) {
+	obj := &testdata.Fake{}
+	noOfConditions, err := fc.GetInt()
+	if err != nil {
+		return obj, err
+	}
+
+	maxNoOfConditions := 30
+	conditions := make([]metav1.Condition, 0)
+	for i := 0; i < noOfConditions%maxNoOfConditions; i++ {
+		c := metav1.Condition{}
+		err = fc.GenerateStruct(&c)
+		if err != nil {
+			return obj, err
+		}
+
+		conditions = append(conditions, c)
+	}
+	obj.SetConditions(conditions)
+	return obj, nil
+}

runtime/conditions/patch_test.go

@@ -27,7 +27,6 @@ import (
 	"testing"
 	"time"
 
-	fuzz "github.com/AdaLogics/go-fuzz-headers"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -290,49 +289,3 @@ func TestApplyDoesNotAlterLastTransitionTime(t *testing.T) {
 	g.Expect(err).ToNot(HaveOccurred())
 	g.Expect(latest.GetConditions()).To(Equal(after.GetConditions()))
 }
-
-func Fuzz_PatchApply(f *testing.F) {
-	f.Fuzz(func(t *testing.T,
-		beforeData, afterData, setterData []byte) {
-
-		before, err := newFake(fuzz.NewConsumer(beforeData))
-		if err != nil {
-			return
-		}
-
-		after, err := newFake(fuzz.NewConsumer(afterData))
-		if err != nil {
-			return
-		}
-
-		patch := NewPatch(before, after)
-		setter, err := newFake(fuzz.NewConsumer(setterData))
-		if err != nil {
-			return
-		}
-
-		_ = patch.Apply(setter)
-	})
-}
-
-func newFake(fc *fuzz.ConsumeFuzzer) (*testdata.Fake, error) {
-	obj := &testdata.Fake{}
-	noOfConditions, err := fc.GetInt()
-	if err != nil {
-		return obj, err
-	}
-
-	maxNoOfConditions := 30
-	conditions := make([]metav1.Condition, 0)
-	for i := 0; i < noOfConditions%maxNoOfConditions; i++ {
-		c := metav1.Condition{}
-		err = fc.GenerateStruct(&c)
-		if err != nil {
-			return obj, err
-		}
-
-		conditions = append(conditions, c)
-	}
-	obj.SetConditions(conditions)
-	return obj, nil
-}

runtime/conditions/unstructured_fuzz_test.go

@@ -0,0 +1,49 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+Copyright 2021 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package conditions
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+func Fuzz_Unstructured(f *testing.F) {
+	f.Add("type", "reason true", "condition message")
+
+	f.Fuzz(func(t *testing.T,
+		ct, reason, message string) {
+
+		cs := []metav1.Condition{{
+			Type:    ct,
+			Status:  metav1.ConditionUnknown,
+			Reason:  reason,
+			Message: message,
+		}}
+
+		u := &unstructured.Unstructured{
+			Object: map[string]interface{}{},
+		}
+		s := UnstructuredSetter(u)
+		s.SetConditions(cs)
+
+		g := UnstructuredGetter(u)
+		_ = g.GetConditions()
+	})
+}

runtime/conditions/unstructured_test.go

@@ -102,27 +102,3 @@ func TestUnstructuredSetConditions(t *testing.T) {
 	s.SetConditions(conditions)
 	g.Expect(s.GetConditions()).To(Equal(conditions))
 }
-
-func Fuzz_Unstructured(f *testing.F) {
-	f.Add("type", "reason true", "condition message")
-
-	f.Fuzz(func(t *testing.T,
-		ct, reason, message string) {
-
-		cs := []metav1.Condition{{
-			Type:    ct,
-			Status:  metav1.ConditionUnknown,
-			Reason:  reason,
-			Message: message,
-		}}
-
-		u := &unstructured.Unstructured{
-			Object: map[string]interface{}{},
-		}
-		s := UnstructuredSetter(u)
-		s.SetConditions(cs)
-
-		g := UnstructuredGetter(u)
-		_ = g.GetConditions()
-	})
-}

runtime/events/recorder_fuzzer_test.go

@@ -1,5 +1,5 @@
-//go:build gofuzz
-// +build gofuzz
+//go:build gofuzz_libfuzzer
+// +build gofuzz_libfuzzer
 
 /*
 Copyright 2021 The Flux authors
@@ -54,6 +54,7 @@ const defaultBinVersion = "1.24"
 //
 // TODO: refactor and remove build tag.
 func Fuzz_Eventf(f *testing.F) {
+
 	f.Fuzz(func(t *testing.T, data []byte) {
 		doOnce.Do(func() {
 			if err := ensureDependencies(); err != nil {

tests/fuzz/README.md

@@ -6,8 +6,6 @@ open source projects.
 The long running fuzzing execution is configured in the [oss-fuzz repository].
 Shorter executions are done on a per-PR basis, configured as a [github workflow].
 
-For fuzzers to be called, they must be compiled within [oss_fuzz_build.sh](./oss_fuzz_build.sh).
-
 ### Testing locally
 
 Build fuzzers:
@@ -19,12 +17,12 @@ All fuzzers will be built into `./build/fuzz/out`.
 
 Smoke test fuzzers:
 
+All the fuzzers will be built and executed once, to ensure they are fully functional.
+
 ```bash
 make fuzz-smoketest
 ```
 
-The smoke test runs each fuzzer once to ensure they are fully functional.
-
 Run fuzzer locally:
 ```bash
 ./build/fuzz/out/fuzz_conditions_match
@@ -39,7 +37,46 @@ Run fuzzer inside a container:
 		/out/fuzz_conditions_match
 ```
 
+### Caveats of creating oss-fuzz compatible tests
+
+#### Segregate fuzz tests
+
+OSS-Fuzz does not properly support mixed `*_test.go` files, in which there is a combination
+of fuzz and non-fuzz tests. To mitigate this problem, ensure your fuzz tests are not in the
+same file as other Go tests. As a pattern, call your fuzz test files `*_fuzz_test.go`.
+
+#### Build tags to avoid conflicts when running Go tests
+
+Due to the issue above, code duplication will occur when creating fuzz tests that rely on
+helper functions that are shared with other tests. To avoid build issues, add a conditional
+build tag at the top of the `*_fuzz_test.go` file:
+```go
+//go:build gofuzz_libfuzzer
+// +build gofuzz_libfuzzer
+```
+
+The build tag above is set at [go-118-fuzz-build].
+At this point in time we can't pass on specific tags from [compile_native_go_fuzzer].
+
+### Running oss-fuzz locally
+
+The `make fuzz-smoketest` is meant to be an easy way to reproduce errors that may occur
+upstream. If our checks ever run out of sync with upstream, the upstream tests can be
+executed locally with:
+
+```
+git clone --depth 1 https://github.com/google/oss-fuzz
+cd oss-fuzz
+python infra/helper.py build_image fluxcd
+python infra/helper.py build_fuzzers --sanitizer address --architecture x86_64 fluxcd
+python infra/helper.py check_build --sanitizer address --architecture x86_64 fluxcd
+```
+
+For latest info on testing oss-fuzz locally, refer to the [upstream guide].
 
 [oss fuzz]: https://github.com/google/oss-fuzz
 [oss-fuzz repository]: https://github.com/google/oss-fuzz/tree/master/projects/fluxcd
 [github workflow]: .github/workflows/cifuzz.yaml
+[upstream guide]: https://google.github.io/oss-fuzz/getting-started/new-project-guide/#testing-locally
+[go-118-fuzz-build]: https://github.com/AdamKorcz/go-118-fuzz-build/blob/b2031950a318d4f2dcf3ec3e128f904d5cf84623/main.go#L40
+[compile_native_go_fuzzer]: https://github.com/google/oss-fuzz/blob/c2d827cb78529fdc757c9b0b4fea0f1238a54814/infra/base-images/base-builder/compile_native_go_fuzzer#L32

tests/fuzz/oss_fuzz_build.sh

@@ -44,20 +44,6 @@ install_deps(){
 	fi
 }
 
-# Removes the content of test funcs which could cause the Fuzz
-# tests to break. This is not supported natively upstream.
-remove_test_funcs(){
-	filename=$1
-
-	echo "removing co-located *testing.T"
-	sed -i -e '/func Test.*testing.T) {$/ {:r;/\n}/!{N;br}; s/\n.*\n/\n/}' "${filename}"
-	# Remove gomega reference as it is not used by Fuzz tests.
-	sed -i 's;. "github.com/onsi/gomega";;g' "${filename}"
-
-	# After removing the body of the go testing funcs, consolidate the imports.
-	goimports -w "${filename}"
-}
-
 install_deps
 
 cd "${GO_SRC}/${PROJECT_PATH}"
@@ -68,7 +54,7 @@ for module in ${modules}; do
 	cd "${GO_SRC}/${PROJECT_PATH}/${module}"
 
 	# TODO: stop ignoring recorder_fuzzer_test.go. Temporary fix for fuzzing building issues.
-	test_files=$(grep -r --include='**_test.go' --files-with-matches 'func Fuzz' . | grep -v recorder_fuzzer_test.go || echo "")
+	test_files=$(grep -r --include='**_test.go' --files-with-matches 'func Fuzz' . || echo "")
 	if [ -z "${test_files}" ]; then
 		continue
 	fi
@@ -83,8 +69,6 @@ for module in ${modules}; do
 			continue
 		fi
 
-		remove_test_funcs "${file}"
-
 		targets=$(grep -oP 'func \K(Fuzz\w*)' "${file}")
 		for target_name in ${targets}; do
 			# Transform module path into module name (e.g. git/libgit2 to git_libgit2).