diff --git a/Makefile b/Makefile index d30e5d9..868dbc0 100644 --- 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 index 06553e6..7816d33 100644 --- 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 index b2a5bf1..5561643 100644 --- 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 index f7c09a7..7c66b61 100644 --- 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 index 5da2732..2fc6be1 100644 --- 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,8 +140,8 @@ 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) }) } } @@ -135,7 +149,7 @@ func Test_CheckRBACDefault(t *testing.T) { 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 index f69b52d..f7a3b45 100644 --- 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 index 0c4ac67..ee9e3b9 100644 --- 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 index 3ac7d43..e1bffb9 100755 --- 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