coder · johnstcn · Sep 13, 2021 · Sep 13, 2021 · Sep 13, 2021 · Sep 13, 2021
diff --git a/Makefile b/Makefile
@@ -33,5 +33,5 @@ test: test/go
 
 test/go:
 	@echo "--- go test"
-	go test -parallel=$(shell nproc) ./...
+	scripts/test_go.sh
 .PHONY: test/go
diff --git a/internal/api/skip.go b/internal/api/skip.go
@@ -1,11 +1,15 @@
 package api
 
 // SkippedResult returns a CheckResult indicating the check was skipped.
-func SkippedResult(name string, summary string) *CheckResult {
+func SkippedResult(name string, summary string, err error) *CheckResult {
+	details := make(map[string]interface{})
+	if err != nil {
+		details["error"] = err
+	}
 	return &CheckResult{
 		Name:    name,
 		State:   StateSkipped,
 		Summary: summary,
-		Details: map[string]interface{}{},
+		Details: details,
 	}
 }
diff --git a/internal/api/skip_test.go b/internal/api/skip_test.go
@@ -3,6 +3,8 @@ package api_test
 import (
 	"testing"
 
+	"golang.org/x/xerrors"
+
 	"cdr.dev/slog/sloggers/slogtest/assert"
 	"github.com/cdr/coder-doctor/internal/api"
 )
@@ -12,8 +14,10 @@ func TestSkippedResult(t *testing.T) {
 
 	checkName := "don't wanna"
 	checkSummary := "just because"
-	res := api.SkippedResult(checkName, checkSummary)
+	checkErr := xerrors.New("whoops")
+	res := api.SkippedResult(checkName, checkSummary, checkErr)
 	assert.Equal(t, "name matches", checkName, res.Name)
 	assert.Equal(t, "state matches", api.StateSkipped, res.State)
 	assert.Equal(t, "summary matches", checkSummary, res.Summary)
+	assert.True(t, "details has err", res.Details["error"] != nil)
 }
diff --git a/internal/checks/kube/rbac.go b/internal/checks/kube/rbac.go
@@ -22,20 +22,29 @@ import (
 	"k8s.io/kubectl/pkg/util/slice"
 )
 
+var errSelfSubjectRulesReviewNotSupported = xerrors.New("cluster does not support SelfSubjectRulesReview")
+
 // CheckRBAC checks the cluster for the RBAC permissions required by Coder.
 // It will attempt to first use a SelfSubjectRulesReview to determine the capabilities
 // of the user. If this fails (notably on GKE), fall back to using SelfSubjectAccessRequests
 // which is slower but is more likely to work.
 func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
-	if ssrrResults := k.checkRBACDefault(ctx); ssrrResults != nil {
+	ssrrResults, err := k.checkRBACDefault(ctx)
+	if err == nil {
 		return ssrrResults
 	}
 
-	k.log.Warn(ctx, "SelfSubjectRulesReview response incomplete, falling back to SelfSubjectAccessRequests (slow)")
-	return k.checkRBACFallback(ctx)
+	if xerrors.Is(err, errSelfSubjectRulesReviewNotSupported) {
+		// In this case, we should fall back to using SelfSubjectAccessRequests.
+		k.log.Warn(ctx, "unable to check via SelfSubjectRulesReview, falling back to SelfSubjectAccessRequests (slow)")
+		return k.checkRBACFallback(ctx)
+	}
+
+	// something else went wrong
+	return []*api.CheckResult{api.ErrorResult("kubernetes-rbac", "unable to check rbac", err)}
 }
 
-func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) []*api.CheckResult {
+func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) ([]*api.CheckResult, error) {
 	const checkName = "kubernetes-rbac-ssrr"
 	authClient := k.client.AuthorizationV1()
 	results := make([]*api.CheckResult, 0)
@@ -48,11 +57,11 @@ func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) []*api.CheckRe
 
 	response, err := authClient.SelfSubjectRulesReviews().Create(ctx, sar, metav1.CreateOptions{})
 	if err != nil {
-		return []*api.CheckResult{api.SkippedResult(checkName, "unable to create SelfSubjectRulesReview request: "+err.Error())}
+		return nil, xerrors.Errorf("create SelfSubjectRulesReview: %w", err)
 	}
 
 	if response.Status.Incomplete {
-		return nil // In this case, we should fall back to using SelfSubjectAccessRequests.
+		return nil, errSelfSubjectRulesReviewNotSupported
 	}
 
 	// convert the list of rules from the server to PolicyRules and dedupe/compact
@@ -97,7 +106,7 @@ func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) []*api.CheckRe
 		results = append(results, api.PassResult(checkName, summary))
 	}
 
-	return results
+	return results, nil
 }
 
 func satisfies(req *ResourceRequirement, verbs ResourceVerbs, rules []rbacv1.PolicyRule) error {

diff --git a/internal/checks/kube/rbac_test.go b/internal/checks/kube/rbac_test.go
@@ -15,6 +15,19 @@ import (
 	"github.com/cdr/coder-doctor/internal/api"
 )
 
+func Test_CheckRBAC_Error(t *testing.T) {
+	t.Parallel()
+
+	srv := newTestHTTPServer(t, 500, nil)
+	defer srv.Close()
+	client, err := kubernetes.NewForConfig(&rest.Config{Host: srv.URL})
+	assert.Success(t, "failed to create client", err)
+
+	checker := NewKubernetesChecker(client)
+	results := checker.CheckRBAC(context.Background())
+	assert.True(t, "should contain one result", len(results) == 1)
+	assert.True(t, "result should be failed", results[0].State == api.StateFailed)
+}
 func Test_CheckRBACFallback(t *testing.T) {
 	t.Parallel()
 
@@ -99,15 +112,16 @@ func Test_CheckRBACDefault(t *testing.T) {
 	tests := []struct {
 		Name     string
 		Response *authorizationv1.SelfSubjectRulesReview
-		F        func(*testing.T, []*api.CheckResult)
+		TestFunc func(*testing.T, []*api.CheckResult, error)
 	}{
 		{
 			Name:     "nothing allowed",
 			Response: &selfSubjectRulesReviewEmpty,
-			F: func(t *testing.T, results []*api.CheckResult) {
+			TestFunc: func(t *testing.T, results []*api.CheckResult, err error) {
 				assert.False(t, "results should not be empty", len(results) == 0)
 				for _, result := range results {
-					assert.True(t, result.Name+" should have an error", result.Details["error"] != nil)
+					assert.True(t, result.Name+" should not return an error", err == nil)
+					assert.True(t, result.Name+" should contain an error in details", result.Details["error"] != nil)
 					assert.True(t, result.Name+" should fail", result.State == api.StateFailed)
 				}
 			},
@@ -126,16 +140,16 @@ func Test_CheckRBACDefault(t *testing.T) {
 			assert.Success(t, "failed to create client", err)
 
 			checker := NewKubernetesChecker(client)
-			results := checker.checkRBACDefault(context.Background())
-			test.F(t, results)
+			results, err := checker.checkRBACDefault(context.Background())
+			test.TestFunc(t, results, err)
 		})
 	}
 }
 
 func Test_Satisfies(t *testing.T) {
 	t.Parallel()
 
-	testCases := []struct {
+	tests := []struct {
 		Name        string
 		Requirement *ResourceRequirement
 		Verbs       ResourceVerbs
@@ -209,14 +223,14 @@ func Test_Satisfies(t *testing.T) {
 		// TODO(cian): add many, many, more.
 	}
 
-	for _, testCase := range testCases {
-		testCase := testCase
-		t.Run(testCase.Name, func(t *testing.T) {
-			actual := satisfies(testCase.Requirement, testCase.Verbs, testCase.Rules)
-			if testCase.Expected == nil {
-				assert.Success(t, testCase.Name+"- should not error", actual)
+	for _, test := range tests {
+		test := test
+		t.Run(test.Name, func(t *testing.T) {
+			actual := satisfies(test.Requirement, test.Verbs, test.Rules)
+			if test.Expected == nil {
+				assert.Success(t, test.Name+"- should not error", actual)
 			} else {
-				assert.ErrorContains(t, testCase.Name+" - expected error contains", actual, *testCase.Expected)
+				assert.ErrorContains(t, test.Name+" - expected error contains", actual, *test.Expected)
 			}
 		})
 	}

diff --git a/internal/checks/kube/resources.go b/internal/checks/kube/resources.go
@@ -16,7 +16,7 @@ func (k *KubernetesChecker) CheckResources(_ context.Context) []*api.CheckResult
 	dc := k.client.Discovery()
 	lists, err := dc.ServerPreferredResources()
 	if err != nil {
-		results = append(results, api.SkippedResult(checkName, "unable to fetch api resources from server: "+err.Error()))
+		results = append(results, api.SkippedResult(checkName, "unable to fetch api resources from server", err))
 		return results
 	}
 

diff --git a/internal/checks/local/helm.go b/internal/checks/local/helm.go
@@ -32,7 +32,7 @@ var versionRequirements = []VersionRequirement{
 
 func (l *Checker) CheckLocalHelmVersion(ctx context.Context) *api.CheckResult {
 	if l.target != api.CheckTargetKubernetes {
-		return api.SkippedResult(LocalHelmVersionCheck, "not applicable for target "+string(l.target))
+		return api.SkippedResult(LocalHelmVersionCheck, "not applicable for target "+string(l.target), nil)
 	}
 
 	helmBin, err := l.lookPathF("helm")

diff --git a/scripts/test_go.sh b/scripts/test_go.sh
@@ -3,6 +3,7 @@
 # Run unit and integration tests for Go code
 
 set -euo pipefail
+CI=${CI:-""}
 PROJECT_ROOT="$(git rev-parse --show-toplevel)"
 cd "$PROJECT_ROOT"
 source "./scripts/lib.sh"
@@ -49,11 +50,14 @@ run_trace false gotestsum tool slowest \
   --jsonfile="$TESTREPORT_JSON" \
   --threshold="$threshold"
 
-# From time to time, Coveralls seems to have an issue on their end, so
-# make a best-effort attempt to upload coverage but ignore failures
-set +e
-echo "--- Uploading test coverage report to Coveralls..."
-run_trace false goveralls -service=github -coverprofile="$COVERAGE"
-set -e
+# Skip coveralls if not running in CI
+if [[ -n "${CI}" ]]; then
+  # From time to time, Coveralls seems to have an issue on their end, so
+  # make a best-effort attempt to upload coverage but ignore failures
+  set +e
+  echo "--- Uploading test coverage report to Coveralls..."
+  run_trace false goveralls -service=github -coverprofile="$COVERAGE"
+  set -e
+fi
 
 exit $test_status