VAULT-15835 Add GHA that checks for nil, nil returns on functions that return an error (#21099)

* VAULT-15385 Add GHA that checks for nil, nil returns on functions that return an error

* VAULT-15385 add failing function, for sanity

* VAULT-15385 fix makefile

* VAULT-15385 remove test dir

* VAULT-15385 Fix typo

* VAULT-15385 fix job name

* VAULT-15385 Add test to packages

* VAULT-15835 add opt-out

* VAULT-15835 Wrong file for comment

* VAULT-15835 remove failing function

* VAULT-15835 return not nil-nil :)

* VAULT-15835 Restrict to two-result functions

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

@@ -46,6 +46,7 @@ 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/gonilnilfunctions/pkg/analyzer"
 if [ "${ENTERPRISE:+x}" == "x" ] ; then
     test_packages[4]+=" $base/vault/external_tests/apilock"
     test_packages[4]+=" $base/vault/external_tests/filteredpaths"

.github/workflows/nil-nil-function-checker.yml

@@ -0,0 +1,26 @@
+name: Check Functions For nil, nil returns
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+    # Runs on PRs to main
+    branches:
+      - main
+
+jobs:
+  # Note: if there is a function we want to ignore this check for,
+  # You can add 'ignore-nil-nil-function-check' somewhere in the
+  # godoc for the function.
+  nil-nil-function-check:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2
+        with:
+          fetch-depth: 0
+      - name: Set Up Go
+        uses: actions/setup-go@4d34df0c2316fe8122ab82dc22947d607c0c91f9 # v4.0.0
+        with:
+          cache: true
+          go-version-file: ./.go-version
+      - name: Verify functions don't return nil, nil
+        run: make ci-vet-gonilnilfunctions

.gitignore

@@ -133,3 +133,5 @@ website/components/node_modules
 *.log
 
 tools/godoctests/.bin
+tools/gonilnilfunctions/.bin
+

Makefile

@@ -139,6 +139,21 @@ vet-godoctests: bootstrap tools/godoctests/.bin/godoctests
 ci-vet-godoctests: ci-bootstrap tools/godoctests/.bin/godoctests
 	@$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep origin/main
 
+# tools/gonilnilfunctions/.bin/gonilnilfunctions builds the custom analyzer to check for nil, nil function returns
+tools/gonilnilfunctions/.bin/gonilnilfunctions:
+	@cd tools/gonilnilfunctions && $(GO_CMD) build -o .bin/gonilnilfunctions .
+
+# vet-gonilnilfunctions runs gonilnilfunctions on functions. All output gets piped to revgrep
+# which will only return an error if a new function returns nil, nil (where one of the nils could be an error)
+vet-gonilnilfunctions: bootstrap tools/gonilnilfunctions/.bin/gonilnilfunctions
+	@$(GO_CMD) vet -vettool=./tools/gonilnilfunctions/.bin/gonilnilfunctions ./... 2>&1 | revgrep
+
+# ci-vet-gonilnilfunctions runs gonilnilfunctions on functions. All output gets piped to revgrep
+# which will only return an error if a new function that is not on main has an issue
+ci-vet-gonilnilfunctions: ci-bootstrap tools/gonilnilfunctions/.bin/gonilnilfunctions
+	@$(GO_CMD) vet -vettool=./tools/gonilnilfunctions/.bin/gonilnilfunctions ./... 2>&1 | revgrep origin/main
+
+
 # lint runs vet plus a number of other checkers, it is more comprehensive, but louder
 lint:
 	@$(GO_CMD) list -f '{{.Dir}}' ./... | grep -v /vendor/ \
@@ -281,7 +296,7 @@ hana-database-plugin:
 mongodb-database-plugin:
 	@CGO_ENABLED=0 $(GO_CMD) build -o bin/mongodb-database-plugin ./plugins/database/mongodb/mongodb-database-plugin
 
-.PHONY: bin default prep test vet bootstrap ci-bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin influxdb-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin ember-dist ember-dist-dev static-dist static-dist-dev assetcheck check-vault-in-path packages build build-ci semgrep semgrep-ci vet-godoctests ci-vet-godoctests
+.PHONY: bin default prep test vet bootstrap ci-bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin influxdb-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin ember-dist ember-dist-dev static-dist static-dist-dev assetcheck check-vault-in-path packages build build-ci semgrep semgrep-ci vet-godoctests ci-vet-godoctests vet-gonilnilfunctions ci-vet-gonilnilfunctions
 
 .NOTPARALLEL: ember-dist ember-dist-dev
 

tools/gonilnilfunctions/main.go

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

tools/gonilnilfunctions/pkg/analyzer/analyzer.go

@@ -0,0 +1,171 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package analyzer
+
+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/gonilnilfunctions/pkg/analyzer/analyzer_test.go

@@ -0,0 +1,23 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package analyzer
+
+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/gonilnilfunctions/pkg/analyzer/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
+}