chore: internal/rbac: address comment from #11

johnstcn · johnstcn · commit 575b13434b14 · 2021-09-13T10:26:47.000Z
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)}
+		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)
 			}
 		})
 	}
