backport of commit 8c18f24b9da475c13f7908e609c5d4be24c773e6 (#21611) (#21615)

* combine into one checker

* combine and simplify ci checks

* add to test package list

* remove testing test

* only run deprecations check

* only run deprecations check

* remove unneeded repo check

* fix bash options

Co-authored-by: miagilepner <mia.epner@hashicorp.com>

.github/scripts/generate-test-package-lists.sh

@@ -45,7 +45,8 @@ fi
 test_packages[4]+=" $base/http"
 test_packages[4]+=" $base/sdk/helper/pluginutil"
 test_packages[4]+=" $base/serviceregistration/kubernetes"
-test_packages[4]+=" $base/tools/godoctests/pkg/analyzer"
+test_packages[4]+=" $base/tools/codechecker/pkg/godoctests"
+test_packages[4]+=" $base/tools/codechecker/pkg/gonilnilfunctions"
 if [ "${ENTERPRISE:+x}" == "x" ] ; then
     test_packages[4]+=" $base/vault/external_tests/apilock"
     test_packages[4]+=" $base/vault/external_tests/filteredpaths"

.github/workflows/ci.yml

@@ -48,18 +48,6 @@ jobs:
             echo 'enterprise=' >> "$GITHUB_OUTPUT"
             echo 'go-build-tags=' >> "$GITHUB_OUTPUT"
           fi
-  semgrep:
-    name: Semgrep
-    needs:
-      - setup
-    runs-on: ${{ fromJSON(needs.setup.outputs.compute-tiny) }}
-    container:
-      image: returntocorp/semgrep@sha256:ffc6f3567654f9431456d49fd059dfe548f007c494a7eb6cd5a1a3e50d813fb3
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-      - name: Run Semgrep Rules
-        id: semgrep
-        run: semgrep ci --include '*.go' --config 'tools/semgrep/ci'
   setup-go-cache:
     name: Go Caches
     needs:
@@ -68,25 +56,6 @@ jobs:
     with:
       runs-on: ${{ needs.setup.outputs.compute-standard }}
     secrets: inherit
-  fmt:
-    name: Check Format
-    needs:
-      - setup
-    runs-on: ${{ fromJSON(needs.setup.outputs.compute-tiny) }}
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          go-version-file: ./.go-version
-          cache: true
-      - id: format
-        run: |
-          echo "Using gofumpt version $(go run mvdan.cc/gofumpt -version)"
-          make fmt
-          if ! git diff --exit-code; then
-            echo "Code has formatting errors. Run 'make fmt' to fix"
-            exit 1
-          fi
   diff-oss-ci:
     name: Diff OSS
     needs:

.github/workflows/code-checker.yml

@@ -0,0 +1,73 @@
+name: Run linters
+
+on:
+  pull_request:
+    types: [opened, synchronize, reopened, ready_for_review]
+  push:
+    branches:
+      - main
+      - release/**
+concurrency:
+  group: ${{ github.head_ref || github.run_id }}-lint
+  cancel-in-progress: true
+
+jobs:
+  deprecations:
+    name: Deprecated functions
+    runs-on: ubuntu-latest
+    if: github.base_ref == 'main'
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+        with:
+          fetch-depth: 0
+      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
+        with:
+          go-version-file: ./.go-version
+          cache: true
+      - run: make ci-deprecations
+        name: Check deprecations
+  codechecker:
+    name: Code checks 
+    runs-on: ubuntu-latest
+    if: github.base_ref == 'main'
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+        with:
+          fetch-depth: 0
+      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
+        with:
+          go-version-file: ./.go-version
+          cache: true
+        # Note: if there is a function we want to ignore the nilnil check for,
+        # You can add 'ignore-nil-nil-function-check' somewhere in the
+        # godoc for the function.
+      - run: make ci-vet-codechecker
+        name: Check custom linters
+  format:
+    name: Format
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
+        with:
+          go-version-file: ./.go-version
+          cache: true
+      - name: Go format
+        run: |
+          make ci-bootstrap
+          echo "Using gofumpt version $(go run mvdan.cc/gofumpt -version)"
+          make fmt
+          if ! git diff --exit-code; then
+            echo "Code has formatting errors. Run 'make fmt' to fix"
+            exit 1
+          fi
+  semgrep:
+    name: Semgrep
+    runs-on: ubuntu-latest
+    container:
+      image: returntocorp/semgrep@sha256:ffc6f3567654f9431456d49fd059dfe548f007c494a7eb6cd5a1a3e50d813fb3
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+      - name: Run Semgrep Rules
+        id: semgrep
+        run: semgrep ci --include '*.go' --config 'tools/semgrep/ci'

.github/workflows/drepecated-functions-checker.yml

@@ -1,31 +0,0 @@
-name: "Check Deprecations"
-
-on:
-  pull_request:
-    # Runs on PRs to main
-    branches:
-      - main
-
-jobs:
-  deprecations-check:
-    runs-on: ubuntu-latest
-    timeout-minutes: 30
-    steps:
-      - name: Checkout code
-        uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-        with:
-          fetch-depth: 0 # by default the checkout action doesn't checkout all branches
-      - name: Setup Go
-        uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          go-version-file: ./.go-version
-          cache: true
-      - name: Install required tools
-        run: |
-          make bootstrap
-      - name: Check deprecations for files in diff
-        run: |
-          # Need to run this from repository root and not from scripts/ as staticcheck works 
-          # only on packages
-          ./scripts/deprecations-checker.sh ${{ github.event.pull_request.base.ref }} ${{ github.event.repository.name }}
-          

.github/workflows/godoc-test-checker.yml

@@ -1,23 +0,0 @@
-name: Check Go Docs for tests
-
-on:
-  pull_request:
-    types: [opened, synchronize]
-    # Runs on PRs to main
-    branches:
-      - main
-
-jobs:
-  godoc-test-check:
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-        with:
-          fetch-depth: 0
-      - name: Set Up Go
-        uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          cache: true
-          go-version-file: ./.go-version
-      - name: Verify new tests have go docs
-        run: make ci-vet-godoctests

.gitignore

@@ -128,3 +128,6 @@ website/components/node_modules
 *.log
 
 tools/godoctests/.bin
+tools/gonilnilfunctions/.bin
+tools/codechecker/.bin
+.ci-bootstrap

Makefile

@@ -9,7 +9,9 @@ INTEG_TEST_TIMEOUT=120m
 VETARGS?=-asmdecl -atomic -bool -buildtags -copylocks -methods -nilfunc -printf -rangeloops -shift -structtags -unsafeptr
 EXTERNAL_TOOLS_CI=\
 	golang.org/x/tools/cmd/goimports \
-	github.com/golangci/revgrep/cmd/revgrep
+	github.com/golangci/revgrep/cmd/revgrep \
+	mvdan.cc/gofumpt \
+	honnef.co/go/tools/cmd/staticcheck
 EXTERNAL_TOOLS=\
 	github.com/client9/misspell/cmd/misspell
 GOFMT_FILES?=$$(find . -name '*.go' | grep -v pb.go | grep -v vendor)
@@ -113,31 +115,28 @@ vet:
 
 # deprecations runs staticcheck tool to look for deprecations. Checks entire code to see if it 
 # has deprecated function, variable, constant or field
-deprecations:
-	make bootstrap
-	repositoryName=$(basename `git rev-parse --show-toplevel`)
-	./scripts/deprecations-checker.sh "" repositoryName
+deprecations: bootstrap
+	@BUILD_TAGS='$(BUILD_TAGS)' ./scripts/deprecations-checker.sh ""
 
 # ci-deprecations runs staticcheck tool to look for deprecations. All output gets piped to revgrep
 # which will only return an error if changes that is not on main has deprecated function, variable, constant or field
-ci-deprecations:
-	make bootstrap
-	repositoryName=$(basename `git rev-parse --show-toplevel`)
-	./scripts/deprecations-checker.sh main repositoryName
+ci-deprecations: ci-bootstrap
+	@BUILD_TAGS='$(BUILD_TAGS)' ./scripts/deprecations-checker.sh main
 
-# tools/godoctests/.bin/godoctests builds the custom analyzer to check for godocs for tests
-tools/godoctests/.bin/godoctests:
-	@cd tools/godoctests && $(GO_CMD) build -o .bin/godoctests .
+tools/codechecker/.bin/codechecker:
+	@cd tools/codechecker && $(GO_CMD) build -o .bin/codechecker .
 
-# vet-godoctests runs godoctests on the test functions. All output gets piped to revgrep
-# which will only return an error if a new function is missing a godoc
-vet-godoctests: bootstrap tools/godoctests/.bin/godoctests
-	@$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep
+# vet-codechecker runs our custom linters on the test functions. All output gets
+# piped to revgrep which will only return an error if new piece of code violates
+# the check 
+vet-codechecker: bootstrap tools/codechecker/.bin/codechecker
+	@$(GO_CMD) vet -vettool=./tools/codechecker/.bin/codechecker -tags=$(BUILD_TAGS) ./... 2>&1 | revgrep
 
-# ci-vet-godoctests runs godoctests on the test functions. All output gets piped to revgrep
-# which will only return an error if a new function that is not on main is missing a godoc
-ci-vet-godoctests: ci-bootstrap tools/godoctests/.bin/godoctests
-	@$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep origin/main
+# vet-codechecker runs our custom linters on the test functions. All output gets
+# piped to revgrep which will only return an error if new piece of code that is 
+# not on main violates the check 
+ci-vet-codechecker: ci-bootstrap tools/codechecker/.bin/codechecker
+	@$(GO_CMD) vet -vettool=./tools/codechecker/.bin/codechecker -tags=$(BUILD_TAGS) ./... 2>&1 | revgrep origin/main
 
 # lint runs vet plus a number of other checkers, it is more comprehensive, but louder
 lint:
@@ -159,11 +158,13 @@ prep: fmtcheck
 	@if [ -d .git/hooks ]; then cp .hooks/* .git/hooks/; fi
 
 # bootstrap the build by downloading additional tools needed to build
-ci-bootstrap:
+ci-bootstrap: .ci-bootstrap
+.ci-bootstrap:
 	@for tool in  $(EXTERNAL_TOOLS_CI) ; do \
 		echo "Installing/Updating $$tool" ; \
 		GO111MODULE=off $(GO_CMD) get -u $$tool; \
 	done
+	@touch .ci-bootstrap
 
 # bootstrap the build by downloading additional tools that may be used by devs
 bootstrap: ci-bootstrap
@@ -240,7 +241,7 @@ fmtcheck:
 	@true
 #@sh -c "'$(CURDIR)/scripts/gofmtcheck.sh'"
 
-fmt:
+fmt: ci-bootstrap
 	find . -name '*.go' | grep -v pb.go | grep -v vendor | xargs go run mvdan.cc/gofumpt -w
 
 semgrep:

scripts/deprecations-checker.sh

@@ -22,31 +22,17 @@
 # Here, it is used to check if a deprecated function, variable, constant or field is used.
 
 # Run staticcheck 
+set -e
 echo "Performing deprecations check: running staticcheck"
 
-# Identify repository name
-if [ -z $2 ]; then
-    # local repository name
-    repositoryName=$(basename `git rev-parse --show-toplevel`)
-else
-    # github repository name from deprecated-functions-checker.yml
-    repositoryName=$2
-fi
-
-# Modify the command with the correct build tag based on repository
-if [ $repositoryName == "vault-enterprise" ]; then
-    staticcheckCommand=$(echo "staticcheck ./... -tags=enterprise")
-else
-    staticcheckCommand=$(echo "staticcheck ./...")
-fi
 
 # If no compare branch name is specified, output all deprecations
 # Else only output the deprecations from the changes added
 if [ -z $1 ]
     then
-        $staticcheckCommand | grep deprecated
+        staticcheck -checks="SA1019" -tags="$BUILD_TAGS"
     else
         # GitHub Actions will use this to find only changes wrt PR's base ref branch
         # revgrep CLI tool will return an exit status of 1 if any issues match, else it will return 0
-        $staticcheckCommand | grep deprecated 2>&1 | revgrep "$(git merge-base HEAD "origin/$1")"
+        staticcheck  -checks="SA1019" -tags="$BUILD_TAGS" 2>&1 | revgrep "$(git merge-base HEAD "origin/$1")"
 fi

tools/codechecker/main.go

@@ -0,0 +1,14 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package main
+
+import (
+	"github.com/hashicorp/vault/tools/codechecker/pkg/godoctests"
+	"github.com/hashicorp/vault/tools/codechecker/pkg/gonilnilfunctions"
+	"golang.org/x/tools/go/analysis/multichecker"
+)
+
+func main() {
+	multichecker.Main(gonilnilfunctions.Analyzer, godoctests.Analyzer)
+}

tools/codechecker/pkg/godoctests/analyzer.go

@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0
 
-package analyzer
+package godoctests
 
 import (
 	"go/ast"

tools/codechecker/pkg/godoctests/analyzer_test.go

@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0
 
-package analyzer
+package godoctests
 
 import (
 	"os"

tools/codechecker/pkg/gonilnilfunctions/analyzer.go

@@ -0,0 +1,171 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package gonilnilfunctions
+
+import (
+	"go/ast"
+	"go/types"
+	"reflect"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name:       "gonilnilfunctions",
+	Doc:        "Verifies that every go function with error as one of its two return types cannot return nil, nil",
+	Run:        run,
+	ResultType: reflect.TypeOf((interface{})(nil)),
+	Requires:   []*analysis.Analyzer{inspect.Analyzer},
+}
+
+// getNestedReturnStatements searches the AST for return statements, and returns
+// them in a tail-call optimized list.
+func getNestedReturnStatements(s ast.Stmt, returns []*ast.ReturnStmt) []*ast.ReturnStmt {
+	switch s := s.(type) {
+	case *ast.BlockStmt:
+		statements := make([]*ast.ReturnStmt, 0)
+		for _, stmt := range s.List {
+			statements = append(statements, getNestedReturnStatements(stmt, make([]*ast.ReturnStmt, 0))...)
+		}
+
+		return append(returns, statements...)
+	case *ast.BranchStmt:
+		return returns
+	case *ast.ForStmt:
+		return getNestedReturnStatements(s.Body, returns)
+	case *ast.IfStmt:
+		return getNestedReturnStatements(s.Body, returns)
+	case *ast.LabeledStmt:
+		return getNestedReturnStatements(s.Stmt, returns)
+	case *ast.RangeStmt:
+		return getNestedReturnStatements(s.Body, returns)
+	case *ast.ReturnStmt:
+		return append(returns, s)
+	case *ast.SwitchStmt:
+		return getNestedReturnStatements(s.Body, returns)
+	case *ast.SelectStmt:
+		return getNestedReturnStatements(s.Body, returns)
+	case *ast.TypeSwitchStmt:
+		return getNestedReturnStatements(s.Body, returns)
+	case *ast.CommClause:
+		statements := make([]*ast.ReturnStmt, 0)
+		for _, stmt := range s.Body {
+			statements = append(statements, getNestedReturnStatements(stmt, make([]*ast.ReturnStmt, 0))...)
+		}
+
+		return append(returns, statements...)
+	case *ast.CaseClause:
+		statements := make([]*ast.ReturnStmt, 0)
+		for _, stmt := range s.Body {
+			statements = append(statements, getNestedReturnStatements(stmt, make([]*ast.ReturnStmt, 0))...)
+		}
+
+		return append(returns, statements...)
+	case *ast.ExprStmt:
+		return returns
+	}
+	return returns
+}
+
+// run runs the analysis, failing for functions whose signatures contain two results including one error
+// (e.g. (something, error)), that contain multiple nil returns
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	nodeFilter := []ast.Node{
+		(*ast.FuncDecl)(nil),
+	}
+
+	inspector.Preorder(nodeFilter, func(node ast.Node) {
+		funcDecl, ok := node.(*ast.FuncDecl)
+		if !ok {
+			return
+		}
+
+		// If the function has the "Ignore" godoc comment, skip it
+		if strings.Contains(funcDecl.Doc.Text(), "ignore-nil-nil-function-check") {
+			return
+		}
+
+		// The function returns something
+		if funcDecl == nil || funcDecl.Type == nil || funcDecl.Type.Results == nil {
+			return
+		}
+
+		// The function has more than 1 return value
+		results := funcDecl.Type.Results.List
+		if len(results) < 2 {
+			return
+		}
+
+		// isError is a helper function to check if a Field is of error type
+		isError := func(field *ast.Field) bool {
+			if named, ok := pass.TypesInfo.TypeOf(field.Type).(*types.Named); ok {
+				namedObject := named.Obj()
+				return namedObject != nil && namedObject.Pkg() == nil && namedObject.Name() == "error"
+			}
+			return false
+		}
+
+		// one of the return values is error
+		var errorFound bool
+		for _, result := range results {
+			if isError(result) {
+				errorFound = true
+				break
+			}
+		}
+
+		if !errorFound {
+			return
+		}
+
+		// Since these statements might be e.g. blocks with
+		// other statements inside, we need to get the return statements
+		// from inside them, first.
+		statements := funcDecl.Body.List
+
+		returnStatements := make([]*ast.ReturnStmt, 0)
+		for _, statement := range statements {
+			returnStatements = append(returnStatements, getNestedReturnStatements(statement, make([]*ast.ReturnStmt, 0))...)
+		}
+
+		for _, returnStatement := range returnStatements {
+			numResultsNil := 0
+			results := returnStatement.Results
+
+			// We only want two-arg functions (something, nil)
+			// We can remove this block in the future if we change our mind
+			if len(results) != 2 {
+				continue
+			}
+
+			for _, result := range results {
+				// nil is an ident
+				ident, isIdent := result.(*ast.Ident)
+				if isIdent {
+					if ident.Name == "nil" {
+						// We found one nil in the return list
+						numResultsNil++
+					}
+				}
+			}
+			// We found N nils, and our function returns N results, so this fails the check
+			if numResultsNil == len(results) {
+				// All the return values are nil, so we fail the report
+				pass.Reportf(node.Pos(), "Function %s can return an error, and has a statement that returns only nils",
+					funcDecl.Name.Name)
+
+				// We break out of the loop of checking return statements, so that we don't repeat ourselves
+				break
+			}
+		}
+	})
+
+	var success interface{}
+	return success, nil
+}

tools/codechecker/pkg/gonilnilfunctions/analyzer_test.go

@@ -0,0 +1,23 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package gonilnilfunctions
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+// TestAnalyzer runs the analyzer on the test functions in testdata/funcs.go. The report from the analyzer is compared against
+// the comments in funcs.go beginning with "want." If there is no comment beginning with "want", then the analyzer is expected
+// not to report anything.
+func TestAnalyzer(t *testing.T) {
+	f, err := os.Getwd()
+	if err != nil {
+		t.Fatal("failed to get working directory", err)
+	}
+	analysistest.Run(t, filepath.Join(f, "testdata"), Analyzer, ".")
+}

tools/codechecker/pkg/gonilnilfunctions/testdata/funcs.go

@@ -0,0 +1,73 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package testdata
+
+func ReturnReturnOkay() (any, error) {
+	var i interface{}
+	return i, nil
+}
+
+func OneGoodOneBad() (any, error) { // want "Function OneGoodOneBad can return an error, and has a statement that returns only nils"
+	var i interface{}
+	if true {
+		return i, nil
+	}
+	return nil, nil
+}
+
+func OneBadOneGood() (any, error) { // want "Function OneBadOneGood can return an error, and has a statement that returns only nils"
+	var i interface{}
+	if true {
+		return nil, nil
+	}
+	return i, nil
+}
+
+func EmptyFunc() {}
+
+func TwoNilNils() (any, error) { // want "Function TwoNilNils can return an error, and has a statement that returns only nils"
+	if true {
+		return nil, nil
+	}
+	return nil, nil
+}
+
+// ThreeResults should not fail, as while it returns nil, nil, nil, it has three results, not two.
+func ThreeResults() (any, any, error) {
+	return nil, nil, nil
+}
+
+func TwoArgsNoError() (any, any) {
+	return nil, nil
+}
+
+func NestedReturn() (any, error) { // want "Function NestedReturn can return an error, and has a statement that returns only nils"
+	{
+		{
+			{
+				return nil, nil
+			}
+		}
+	}
+}
+
+func NestedForReturn() (any, error) { // want "Function NestedForReturn can return an error, and has a statement that returns only nils"
+	for {
+		for i := 0; i < 100; i++ {
+			{
+				return nil, nil
+			}
+		}
+	}
+}
+
+func AnyErrorNilNil() (any, error) { // want "Function AnyErrorNilNil can return an error, and has a statement that returns only nils"
+	return nil, nil
+}
+
+// Skipped should be skipped because of the following line:
+// ignore-nil-nil-function-check
+func Skipped() (any, error) {
+	return nil, nil
+}

tools/godoctests/main.go

@@ -1,13 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0
-
-package main
-
-import (
-	"github.com/hashicorp/vault/tools/godoctests/pkg/analyzer"
-	"golang.org/x/tools/go/analysis/singlechecker"
-)
-
-func main() {
-	singlechecker.Main(analyzer.Analyzer)
-}