From 9f3150756d2e1780c60919eb43b615b132f8cd0e Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Wed, 23 Nov 2022 14:57:23 +0000 Subject: [PATCH] build: Improve fuzz tests' reliability Establish conventions which aligns with what is supported upstream today, whilst expanding on documentation to ensure folks have pointers on how to debug/check for issues going forwards. Signed-off-by: Paulo Gomes --- .../helmrelease_controller_fuzz_test.go | 273 ++++++++++++++++++ controllers/helmrelease_controller_test.go | 224 -------------- tests/fuzz/README.md | 45 ++- tests/fuzz/oss_fuzz_build.sh | 90 +++--- 4 files changed, 360 insertions(+), 272 deletions(-) create mode 100644 controllers/helmrelease_controller_fuzz_test.go diff --git a/controllers/helmrelease_controller_fuzz_test.go b/controllers/helmrelease_controller_fuzz_test.go new file mode 100644 index 0000000..96215a2 --- /dev/null +++ b/controllers/helmrelease_controller_fuzz_test.go @@ -0,0 +1,273 @@ +//go:build gofuzz_libfuzzer +// +build gofuzz_libfuzzer + +/* +Copyright 2020 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 controllers + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/yaml" + + v2 "github.com/fluxcd/helm-controller/api/v2beta1" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" +) + +func FuzzHelmReleaseReconciler_composeValues(f *testing.F) { + scheme := testScheme() + + tests := []struct { + targetPath string + valuesKey string + hrValues string + createObject bool + secretData []byte + configData string + }{ + { + targetPath: "flat", + valuesKey: "custom-values.yaml", + secretData: []byte(`flat: + nested: value +nested: value +`), + configData: `flat: value +nested: + configuration: value +`, + hrValues: ` +other: values +`, + createObject: true, + }, + { + targetPath: "'flat'", + valuesKey: "custom-values.yaml", + secretData: []byte(`flat: + nested: value +nested: value +`), + configData: `flat: value +nested: + configuration: value +`, + hrValues: ` +other: values +`, + createObject: true, + }, + { + targetPath: "flat[0]", + secretData: []byte(``), + configData: `flat: value`, + hrValues: ` +other: values +`, + createObject: true, + }, + { + secretData: []byte(`flat: + nested: value +nested: value +`), + configData: `flat: value +nested: + configuration: value +`, + hrValues: ` +other: values +`, + createObject: true, + }, + { + targetPath: "some-value", + hrValues: ` +other: values +`, + createObject: false, + }, + } + + for _, tt := range tests { + f.Add(tt.targetPath, tt.valuesKey, tt.hrValues, tt.createObject, tt.secretData, tt.configData) + } + + f.Fuzz(func(t *testing.T, + targetPath, valuesKey, hrValues string, createObject bool, secretData []byte, configData string) { + + // objectName represents a core Kubernetes name (Secret/ConfigMap) which is validated + // upstream, and also validated by us in the OpenAPI-based validation set in + // v2.ValuesReference. Therefore a static value here suffices, and instead we just + // play with the objects presence/absence. + objectName := "values" + resources := []runtime.Object{} + + if createObject { + resources = append(resources, + valuesConfigMap(objectName, map[string]string{valuesKey: configData}), + valuesSecret(objectName, map[string][]byte{valuesKey: secretData}), + ) + } + + references := []v2.ValuesReference{ + { + Kind: "ConfigMap", + Name: objectName, + ValuesKey: valuesKey, + TargetPath: targetPath, + }, + { + Kind: "Secret", + Name: objectName, + ValuesKey: valuesKey, + TargetPath: targetPath, + }, + } + + c := fake.NewFakeClientWithScheme(scheme, resources...) + r := &HelmReleaseReconciler{Client: c} + var values *apiextensionsv1.JSON + if hrValues != "" { + v, _ := yaml.YAMLToJSON([]byte(hrValues)) + values = &apiextensionsv1.JSON{Raw: v} + } + + hr := v2.HelmRelease{ + Spec: v2.HelmReleaseSpec{ + ValuesFrom: references, + Values: values, + }, + } + + // OpenAPI-based validation on schema is not verified here. + // Therefore some false positives may be arise, as the apiserver + // would not allow such values to make their way into the control plane. + // + // Testenv could be used so the fuzzing covers the entire E2E. + // The downsize being the resource and time cost per test would be a lot higher. + // + // Another approach could be to add validation to reject invalid inputs before + // the r.composeValues call. + _, _ = r.composeValues(logr.NewContext(context.TODO(), logr.Discard()), hr) + }) +} + +func FuzzHelmReleaseReconciler_reconcile(f *testing.F) { + scheme := testScheme() + tests := []struct { + valuesKey string + hrValues string + secretData []byte + configData string + }{ + { + valuesKey: "custom-values.yaml", + secretData: []byte(`flat: + nested: value +nested: value +`), + configData: `flat: value +nested: + configuration: value +`, + hrValues: ` +other: values +`, + }, + } + + for _, tt := range tests { + f.Add(tt.valuesKey, tt.hrValues, tt.secretData, tt.configData) + } + + f.Fuzz(func(t *testing.T, + valuesKey, hrValues string, secretData []byte, configData string) { + + var values *apiextensionsv1.JSON + if hrValues != "" { + v, _ := yaml.YAMLToJSON([]byte(hrValues)) + values = &apiextensionsv1.JSON{Raw: v} + } + + hr := v2.HelmRelease{ + Spec: v2.HelmReleaseSpec{ + Values: values, + }, + } + + hc := sourcev1.HelmChart{} + hc.ObjectMeta.Name = hr.GetHelmChartName() + hc.ObjectMeta.Namespace = hr.Spec.Chart.GetNamespace(hr.Namespace) + + resources := []runtime.Object{ + valuesConfigMap("values", map[string]string{valuesKey: configData}), + valuesSecret("values", map[string][]byte{valuesKey: secretData}), + &hc, + } + + c := fake.NewFakeClientWithScheme(scheme, resources...) + r := &HelmReleaseReconciler{ + Client: c, + EventRecorder: &DummyRecorder{}, + } + + _, _, _ = r.reconcile(logr.NewContext(context.TODO(), logr.Discard()), hr) + }) +} + +func valuesSecret(name string, data map[string][]byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Data: data, + } +} + +func valuesConfigMap(name string, data map[string]string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Data: data, + } +} + +func testScheme() *runtime.Scheme { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = v2.AddToScheme(scheme) + _ = sourcev1.AddToScheme(scheme) + return scheme +} + +// DummyRecorder serves as a dummy for kuberecorder.EventRecorder. +type DummyRecorder struct{} + +func (r *DummyRecorder) Event(object runtime.Object, eventtype, reason, message string) { +} + +func (r *DummyRecorder) Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { +} + +func (r *DummyRecorder) AnnotatedEventf(object runtime.Object, annotations map[string]string, + eventtype, reason string, messageFmt string, args ...interface{}) { +} diff --git a/controllers/helmrelease_controller_test.go b/controllers/helmrelease_controller_test.go index b54599e..a514c63 100644 --- a/controllers/helmrelease_controller_test.go +++ b/controllers/helmrelease_controller_test.go @@ -34,7 +34,6 @@ import ( "sigs.k8s.io/yaml" v2 "github.com/fluxcd/helm-controller/api/v2beta1" - sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) func TestHelmReleaseReconciler_composeValues(t *testing.T) { @@ -447,208 +446,6 @@ func TestValuesReferenceValidation(t *testing.T) { } } -func FuzzHelmReleaseReconciler_composeValues(f *testing.F) { - scheme := testScheme() - - tests := []struct { - targetPath string - valuesKey string - hrValues string - createObject bool - secretData []byte - configData string - }{ - { - targetPath: "flat", - valuesKey: "custom-values.yaml", - secretData: []byte(`flat: - nested: value -nested: value -`), - configData: `flat: value -nested: - configuration: value -`, - hrValues: ` -other: values -`, - createObject: true, - }, - { - targetPath: "'flat'", - valuesKey: "custom-values.yaml", - secretData: []byte(`flat: - nested: value -nested: value -`), - configData: `flat: value -nested: - configuration: value -`, - hrValues: ` -other: values -`, - createObject: true, - }, - { - targetPath: "flat[0]", - secretData: []byte(``), - configData: `flat: value`, - hrValues: ` -other: values -`, - createObject: true, - }, - { - secretData: []byte(`flat: - nested: value -nested: value -`), - configData: `flat: value -nested: - configuration: value -`, - hrValues: ` -other: values -`, - createObject: true, - }, - { - targetPath: "some-value", - hrValues: ` -other: values -`, - createObject: false, - }, - } - - for _, tt := range tests { - f.Add(tt.targetPath, tt.valuesKey, tt.hrValues, tt.createObject, tt.secretData, tt.configData) - } - - f.Fuzz(func(t *testing.T, - targetPath, valuesKey, hrValues string, createObject bool, secretData []byte, configData string) { - - // objectName represents a core Kubernetes name (Secret/ConfigMap) which is validated - // upstream, and also validated by us in the OpenAPI-based validation set in - // v2.ValuesReference. Therefore a static value here suffices, and instead we just - // play with the objects presence/absence. - objectName := "values" - resources := []runtime.Object{} - - if createObject { - resources = append(resources, - valuesConfigMap(objectName, map[string]string{valuesKey: configData}), - valuesSecret(objectName, map[string][]byte{valuesKey: secretData}), - ) - } - - references := []v2.ValuesReference{ - { - Kind: "ConfigMap", - Name: objectName, - ValuesKey: valuesKey, - TargetPath: targetPath, - }, - { - Kind: "Secret", - Name: objectName, - ValuesKey: valuesKey, - TargetPath: targetPath, - }, - } - - c := fake.NewFakeClientWithScheme(scheme, resources...) - r := &HelmReleaseReconciler{Client: c} - var values *apiextensionsv1.JSON - if hrValues != "" { - v, _ := yaml.YAMLToJSON([]byte(hrValues)) - values = &apiextensionsv1.JSON{Raw: v} - } - - hr := v2.HelmRelease{ - Spec: v2.HelmReleaseSpec{ - ValuesFrom: references, - Values: values, - }, - } - - // OpenAPI-based validation on schema is not verified here. - // Therefore some false positives may be arise, as the apiserver - // would not allow such values to make their way into the control plane. - // - // Testenv could be used so the fuzzing covers the entire E2E. - // The downsize being the resource and time cost per test would be a lot higher. - // - // Another approach could be to add validation to reject invalid inputs before - // the r.composeValues call. - _, _ = r.composeValues(logr.NewContext(context.TODO(), logr.Discard()), hr) - }) -} - -func FuzzHelmReleaseReconciler_reconcile(f *testing.F) { - scheme := testScheme() - tests := []struct { - valuesKey string - hrValues string - secretData []byte - configData string - }{ - { - valuesKey: "custom-values.yaml", - secretData: []byte(`flat: - nested: value -nested: value -`), - configData: `flat: value -nested: - configuration: value -`, - hrValues: ` -other: values -`, - }, - } - - for _, tt := range tests { - f.Add(tt.valuesKey, tt.hrValues, tt.secretData, tt.configData) - } - - f.Fuzz(func(t *testing.T, - valuesKey, hrValues string, secretData []byte, configData string) { - - var values *apiextensionsv1.JSON - if hrValues != "" { - v, _ := yaml.YAMLToJSON([]byte(hrValues)) - values = &apiextensionsv1.JSON{Raw: v} - } - - hr := v2.HelmRelease{ - Spec: v2.HelmReleaseSpec{ - Values: values, - }, - } - - hc := sourcev1.HelmChart{} - hc.ObjectMeta.Name = hr.GetHelmChartName() - hc.ObjectMeta.Namespace = hr.Spec.Chart.GetNamespace(hr.Namespace) - - resources := []runtime.Object{ - valuesConfigMap("values", map[string]string{valuesKey: configData}), - valuesSecret("values", map[string][]byte{valuesKey: secretData}), - &hc, - } - - c := fake.NewFakeClientWithScheme(scheme, resources...) - r := &HelmReleaseReconciler{ - Client: c, - EventRecorder: &DummyRecorder{}, - } - - _, _, _ = r.reconcile(logr.NewContext(context.TODO(), logr.Discard()), hr) - }) -} - func valuesSecret(name string, data map[string][]byte) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: name}, @@ -662,24 +459,3 @@ func valuesConfigMap(name string, data map[string]string) *corev1.ConfigMap { Data: data, } } - -func testScheme() *runtime.Scheme { - scheme := runtime.NewScheme() - _ = corev1.AddToScheme(scheme) - _ = v2.AddToScheme(scheme) - _ = sourcev1.AddToScheme(scheme) - return scheme -} - -// DummyRecorder serves as a dummy for kuberecorder.EventRecorder. -type DummyRecorder struct{} - -func (r *DummyRecorder) Event(object runtime.Object, eventtype, reason, message string) { -} - -func (r *DummyRecorder) Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { -} - -func (r *DummyRecorder) AnnotatedEventf(object runtime.Object, annotations map[string]string, - eventtype, reason string, messageFmt string, args ...interface{}) { -} diff --git a/tests/fuzz/README.md b/tests/fuzz/README.md index f2d2339..98e1d26 100644 --- a/tests/fuzz/README.md +++ b/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 \ No newline at end of file diff --git a/tests/fuzz/oss_fuzz_build.sh b/tests/fuzz/oss_fuzz_build.sh index 1b83519..d0a645b 100755 --- a/tests/fuzz/oss_fuzz_build.sh +++ b/tests/fuzz/oss_fuzz_build.sh @@ -16,64 +16,66 @@ set -euxo pipefail +# This file aims for: +# - Dynamically discover and build all fuzz tests within the repository. +# - Work for both local make fuzz-smoketest and the upstream oss-fuzz. + GOPATH="${GOPATH:-/root/go}" GO_SRC="${GOPATH}/src" PROJECT_PATH="github.com/fluxcd/helm-controller" -TMP_DIR=$(mktemp -d /tmp/oss_fuzz-XXXXXX) - -cleanup(){ - rm -rf "${TMP_DIR}" -} -trap cleanup EXIT +# install_deps installs all dependencies needed for upstream oss-fuzz. +# Unfortunately we can't pin versions here, as we want to always +# have the latest, so that we can reproduce errors occuring upstream. install_deps(){ - if ! command -v go-118-fuzz-build &> /dev/null || ! command -v addimport &> /dev/null; then - mkdir -p "${TMP_DIR}/go-118-fuzz-build" - - git clone https://github.com/AdamKorcz/go-118-fuzz-build "${TMP_DIR}/go-118-fuzz-build" - cd "${TMP_DIR}/go-118-fuzz-build" - go build -o "${GOPATH}/bin/go-118-fuzz-build" - - cd addimport - go build -o "${GOPATH}/bin/addimport" + if ! command -v go-118-fuzz-build &> /dev/null; then + go install github.com/AdamKorcz/go-118-fuzz-build@latest fi - - if ! command -v goimports &> /dev/null; then - go install golang.org/x/tools/cmd/goimports@latest - fi -} - -# Removes the content of test funcs which could cause the Fuzz -# tests to break. -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}" - - # After removing the body of the go testing funcs, consolidate the imports. - goimports -w "${filename}" } install_deps cd "${GO_SRC}/${PROJECT_PATH}" -go get github.com/AdamKorcz/go-118-fuzz-build/utils +# Ensure any project-specific requirements are catered for ahead of +# the generic build process. +if [ -f "tests/fuzz/oss_fuzz_prebuild.sh" ]; then + tests/fuzz/oss_fuzz_prebuild.sh +fi -# Iterate through all Go Fuzz targets, compiling each into a fuzzer. -test_files=$(grep -r --include='**_test.go' --files-with-matches 'func Fuzz' .) -for file in ${test_files} -do - remove_test_funcs "${file}" +modules=$(find . -mindepth 1 -maxdepth 4 -type f -name 'go.mod' | cut -c 3- | sed 's|/[^/]*$$||' | sort -u | sed 's;/go.mod;;g' | sed 's;go.mod;.;g') - targets=$(grep -oP 'func \K(Fuzz\w*)' "${file}") - for target_name in ${targets} - do - fuzzer_name=$(echo "${target_name}" | tr '[:upper:]' '[:lower:]') - target_dir=$(dirname "${file}") +for module in ${modules}; do - echo "Building ${file}.${target_name} into ${fuzzer_name}" - compile_native_go_fuzzer "${target_dir}" "${target_name}" "${fuzzer_name}" + 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' . || echo "") + if [ -z "${test_files}" ]; then + continue + fi + + go get github.com/AdamKorcz/go-118-fuzz-build/testing + + # Iterate through all Go Fuzz targets, compiling each into a fuzzer. + for file in ${test_files}; do + # If the subdir is a module, skip this file, as it will be handled + # at the next iteration of the outer loop. + if [ -f "$(dirname "${file}")/go.mod" ]; then + continue + fi + + 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). + module_name=$(echo ${module} | tr / _) + # Compose fuzzer name based on the lowercase version of the func names. + # The module name is added after the fuzz prefix, for better discoverability. + fuzzer_name=$(echo "${target_name}" | tr '[:upper:]' '[:lower:]' | sed "s;fuzz_;fuzz_${module_name}_;g") + target_dir=$(dirname "${file}") + + echo "Building ${file}.${target_name} into ${fuzzer_name}" + compile_native_go_fuzzer "${target_dir}" "${target_name}" "${fuzzer_name}" + done done done