diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml index 08e264c9..ab68d44f 100644 --- a/.github/workflows/ci-cd.yml +++ b/.github/workflows/ci-cd.yml @@ -44,7 +44,7 @@ jobs: go get github.com/wadey/gocovmerge - name: Run build and test - timeout-minutes: 30 + timeout-minutes: 60 run: | cd $GITHUB_WORKSPACE && make && make ARCH=arm64 binary-arch-minimal && make ARCH=arm64 binary-arch env: diff --git a/Makefile b/Makefile index 353a4ef5..329dd1e9 100644 --- a/Makefile +++ b/Makefile @@ -68,6 +68,9 @@ test: check-skopeo $(NOTATION) $(shell sudo chmod a=rwx /etc/containers/certs.d/127.0.0.1:8089/*.key) go test -tags extended,containers_image_openpgp -v -trimpath -race -timeout 15m -cover -coverpkg ./... -coverprofile=coverage-extended.txt -covermode=atomic ./... go test -tags minimal,containers_image_openpgp -v -trimpath -race -cover -coverpkg ./... -coverprofile=coverage-minimal.txt -covermode=atomic ./... + # development-mode unit tests possibly using failure injection + go test -tags dev,extended,containers_image_openpgp -v -trimpath -race -timeout 15m -cover -coverpkg ./... -coverprofile=coverage-dev-extended.txt -covermode=atomic ./pkg/api/... ./pkg/test/... + go test -tags dev,minimal,containers_image_openpgp -v -trimpath -race -cover -coverpkg ./... -coverprofile=coverage-dev-minimal.txt -covermode=atomic ./pkg/api/... ./pkg/test/... .PHONY: run-bench run-bench: binary bench @@ -92,8 +95,8 @@ $(NOTATION): .PHONY: covhtml covhtml: - tail -n +2 coverage-minimal.txt > tmp.txt && mv tmp.txt coverage-minimal.txt - cat coverage-extended.txt coverage-minimal.txt > coverage.txt + go install github.com/wadey/gocovmerge@latest + gocovmerge coverage-minimal.txt coverage-extended.txt coverage-dev-minimal.txt coverage-dev-extended.txt > coverage.txt go tool cover -html=coverage.txt -o coverage.html $(GOLINTER): @@ -105,6 +108,8 @@ $(GOLINTER): check: ./golangcilint.yaml $(GOLINTER) $(GOLINTER) --config ./golangcilint.yaml run --enable-all --out-format=colored-line-number --build-tags minimal,containers_image_openpgp ./... $(GOLINTER) --config ./golangcilint.yaml run --enable-all --out-format=colored-line-number --build-tags extended,containers_image_openpgp ./... + $(GOLINTER) --config ./golangcilint.yaml run --enable-all --out-format=colored-line-number --build-tags dev,minimal,containers_image_openpgp ./... + $(GOLINTER) --config ./golangcilint.yaml run --enable-all --out-format=colored-line-number --build-tags dev,extended,containers_image_openpgp ./... swagger/docs.go: swag -v || go install github.com/swaggo/swag/cmd/swag diff --git a/errors/errors.go b/errors/errors.go index d7e4b065..ce70f38a 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -47,4 +47,5 @@ var ( ErrSyncMissingCatalog = errors.New("sync: couldn't fetch upstream registry's catalog") ErrMethodNotSupported = errors.New("storage: method not supported") ErrInvalidMetric = errors.New("metrics: invalid metric func") + ErrInjected = errors.New("test: injected failure") ) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index b0e53fdf..4e5766fd 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -43,6 +43,7 @@ import ( "zotregistry.io/zot/pkg/api" "zotregistry.io/zot/pkg/api/config" "zotregistry.io/zot/pkg/storage" + "zotregistry.io/zot/pkg/test" . "zotregistry.io/zot/test" ) @@ -2083,6 +2084,29 @@ func TestAuthorizationWithBasicAuth(t *testing.T) { So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusOK) + resp, err = resty.R().SetBasicAuth(username, passphrase). + Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + Convey("Hard to reach cases", func() { + injected := test.InjectFailure(0) + + // get tags with read access should get 200 + conf.AccessControl.Repositories[AuthorizationNamespace].Policies[0].Actions = + append(conf.AccessControl.Repositories[AuthorizationNamespace].Policies[0].Actions, "read") + resp, err = resty.R().SetBasicAuth(username, passphrase). + Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + if injected { + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) + } else { + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + } + }) + // head blob should get 200 now resp, err = resty.R().SetBasicAuth(username, passphrase). Head(baseURL + "/v2/" + AuthorizationNamespace + "/blobs/" + digest) @@ -2838,6 +2862,24 @@ func TestParallelRequests(t *testing.T) { assert.Equal(t, err, nil, "Error should be nil") assert.Equal(t, headResponse.StatusCode(), http.StatusNotFound, "response status code should return 404") + Convey("Hard to reach cases", t, func() { + _ = test.InjectFailure(0) + + headResponse, err := client.R().SetBasicAuth(username, passphrase). + Head(baseURL + "/v2/" + testcase.destImageName + "/manifests/test:1.0") + assert.Equal(t, err, nil, "Error should be nil") + assert.Equal(t, headResponse.StatusCode(), http.StatusNotFound, "response status code should return 404") + }) + + Convey("Hard to reach cases", t, func() { + _ = test.InjectFailure(1) + + headResponse, err := client.R().SetBasicAuth(username, passphrase). + Head(baseURL + "/v2/" + testcase.destImageName + "/manifests/test:1.0") + assert.Equal(t, err, nil, "Error should be nil") + assert.Equal(t, headResponse.StatusCode(), http.StatusNotFound, "response status code should return 404") + }) + getResponse, err := client.R().SetBasicAuth(username, passphrase). Get(baseURL + "/v2/" + testcase.destImageName + "/manifests/" + manifest) assert.Equal(t, err, nil, "Error should be nil") diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 467b3a8a..85efee37 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -32,6 +32,7 @@ import ( ext "zotregistry.io/zot/pkg/extensions" "zotregistry.io/zot/pkg/log" "zotregistry.io/zot/pkg/storage" + "zotregistry.io/zot/pkg/test" // as required by swaggo. _ "zotregistry.io/zot/swagger" @@ -165,7 +166,7 @@ func (rh *RouteHandler) ListTags(response http.ResponseWriter, request *http.Req name, ok := vars["name"] - if !ok || name == "" { + if !test.Ok(ok) || name == "" { response.WriteHeader(http.StatusNotFound) return @@ -287,7 +288,7 @@ func (rh *RouteHandler) CheckManifest(response http.ResponseWriter, request *htt vars := mux.Vars(request) name, ok := vars["name"] - if !ok || name == "" { + if !test.Ok(ok) || name == "" { response.WriteHeader(http.StatusNotFound) return @@ -296,7 +297,7 @@ func (rh *RouteHandler) CheckManifest(response http.ResponseWriter, request *htt imgStore := rh.getImageStore(name) reference, ok := vars["reference"] - if !ok || reference == "" { + if !test.Ok(ok) || reference == "" { WriteJSON(response, http.StatusNotFound, NewErrorList(NewError(MANIFEST_INVALID, map[string]string{"reference": reference}))) diff --git a/pkg/compliance/v1_0_0/check.go b/pkg/compliance/v1_0_0/check.go index 59a81b0f..fd8209ec 100644 --- a/pkg/compliance/v1_0_0/check.go +++ b/pkg/compliance/v1_0_0/check.go @@ -670,10 +670,26 @@ func CheckWorkflows(t *testing.T, config *compliance.Config) { So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusOK) + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n= ") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest) + + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=a") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest) + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=0") So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusOK) + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=0&last=100") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) + + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=0&last=test:0.0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=3") So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusOK) diff --git a/pkg/test/dev.go b/pkg/test/dev.go new file mode 100644 index 00000000..13fa4445 --- /dev/null +++ b/pkg/test/dev.go @@ -0,0 +1,81 @@ +//go:build dev +// +build dev + +// This file should be linked only in **development** mode. + +package test + +import ( + "sync" + + zerr "zotregistry.io/zot/errors" +) + +func Ok(ok bool) bool { + if !ok { + return ok + } + + if injectedFailure() { + return false + } + + return true +} + +func Error(err error) error { + if err != nil { + return err + } + + if injectedFailure() { + return zerr.ErrInjected + } + + return nil +} + +/** + * + * Failure injection infrastructure to cover hard-to-reach code paths. + * + **/ + +type inject struct { + skip int + enabled bool +} + +//nolint:gochecknoglobals // only used by test code +var ( + injlock sync.Mutex + injst = inject{} +) + +func InjectFailure(skip int) bool { + injlock.Lock() + injst = inject{enabled: true, skip: skip} + injlock.Unlock() + + return true +} + +func injectedFailure() bool { + injlock.Lock() + defer injlock.Unlock() + + if !injst.enabled { + return false + } + + if injst.skip == 0 { + // disable the injection point + injst.enabled = false + + return true + } + + injst.skip-- + + return false +} diff --git a/pkg/test/inject_test.go b/pkg/test/inject_test.go new file mode 100644 index 00000000..00547aa9 --- /dev/null +++ b/pkg/test/inject_test.go @@ -0,0 +1,120 @@ +//go:build dev +// +build dev + +// This file should be linked only in **development** mode. + +package test_test + +import ( + "errors" + "testing" + + . "github.com/smartystreets/goconvey/convey" + "zotregistry.io/zot/pkg/test" +) + +var ( + errKey1 = errors.New("key1 not found") + errKey2 = errors.New("key2 not found") + errNotZero = errors.New("not zero") + errCall1 = errors.New("call1 error") + errCall2 = errors.New("call2 error") +) + +func foo() error { + fmap := map[string]string{"key1": "val1", "key2": "val2"} + + _, ok := fmap["key1"] // should never fail + if !test.Ok(ok) { + return errKey1 + } + + _, ok = fmap["key2"] // should never fail + if !test.Ok(ok) { + return errKey2 + } + + return nil +} + +func errgen(i int) error { + if i != 0 { + return errNotZero + } + + return nil +} + +func bar() error { + err := errgen(0) // should never fail + if test.Error(err) != nil { + return errCall1 + } + + err = errgen(0) // should never fail + if test.Error(err) != nil { + return errCall2 + } + + return nil +} + +func alwaysErr() error { + return errNotZero +} + +func alwaysNotOk() bool { + return false +} + +func TestInject(t *testing.T) { + Convey("Injected failure", t, func(c C) { + // should be success without injection + err := foo() + So(err, ShouldBeNil) + + Convey("Check Ok", func() { + Convey("Without skipping", func() { + test.InjectFailure(0) // inject a failure + err := foo() // should be a failure + So(err, ShouldNotBeNil) // should be a failure + So(errors.Is(err, errKey1), ShouldBeTrue) + }) + + Convey("With skipping", func() { + test.InjectFailure(1) // inject a failure but skip first one + err := foo() // should be a failure + So(errors.Is(err, errKey1), ShouldBeFalse) + So(errors.Is(err, errKey2), ShouldBeTrue) + }) + }) + + // should be success without injection + err = bar() + So(err, ShouldBeNil) + + Convey("Check Err", func() { + Convey("Without skipping", func() { + test.InjectFailure(0) // inject a failure + err := bar() // should be a failure + So(err, ShouldNotBeNil) // should be a failure + So(errors.Is(err, errCall1), ShouldBeTrue) + }) + + Convey("With skipping", func() { + test.InjectFailure(1) // inject a failure but skip first one + err := bar() // should be a failure + So(errors.Is(err, errCall1), ShouldBeFalse) + So(errors.Is(err, errCall2), ShouldBeTrue) + }) + }) + }) + + Convey("Without injected failure", t, func(c C) { + err := alwaysErr() + So(test.Error(err), ShouldNotBeNil) + + ok := alwaysNotOk() + So(test.Ok(ok), ShouldBeFalse) + }) +} diff --git a/pkg/test/prod.go b/pkg/test/prod.go new file mode 100644 index 00000000..f8c34425 --- /dev/null +++ b/pkg/test/prod.go @@ -0,0 +1,20 @@ +//go:build !dev +// +build !dev + +package test + +func Error(err error) error { + return err +} + +func Ok(ok bool) bool { + return ok +} + +/** + * + * Failure injection infrastructure to cover hard-to-reach code paths (nop in production). + * + **/ + +func InjectFailure(skip int) bool { return false }