Skip to content

Commit fc1b142

Browse files
johnstcnjohnstcn
authored
address comments from coder#11 (coder#15)
* chore: Makefile: use scripts/test_go.sh * chore: internal/api: add err param to SkippedResult * chore: scripts/test_go.sh: skip coveralls step if not running in CI * chore: internal/rbac: address comment from coder#11
1 parent 7c7c496 commit fc1b142

File tree

8 files changed

+67
-32
lines changed

8 files changed

+67
-32
lines changed

‎Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,5 +33,5 @@ test: test/go
3333

3434
test/go:
3535
@echo "--- go test"
36-
go test -parallel=$(shell nproc) ./...
36+
scripts/test_go.sh
3737
.PHONY: test/go

‎internal/api/skip.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
package api
22

33
// SkippedResult returns a CheckResult indicating the check was skipped.
4-
func SkippedResult(name string, summary string) *CheckResult {
4+
func SkippedResult(name string, summary string, err error) *CheckResult {
5+
details := make(map[string]interface{})
6+
if err != nil {
7+
details["error"] = err
8+
}
59
return &CheckResult{
610
Name: name,
711
State: StateSkipped,
812
Summary: summary,
9-
Details: map[string]interface{}{},
13+
Details: details,
1014
}
1115
}

‎internal/api/skip_test.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package api_test
33
import (
44
"testing"
55

6+
"golang.org/x/xerrors"
7+
68
"cdr.dev/slog/sloggers/slogtest/assert"
79
"github.com/cdr/coder-doctor/internal/api"
810
)
@@ -12,8 +14,10 @@ func TestSkippedResult(t *testing.T) {
1214

1315
checkName := "don't wanna"
1416
checkSummary := "just because"
15-
res := api.SkippedResult(checkName, checkSummary)
17+
checkErr := xerrors.New("whoops")
18+
res := api.SkippedResult(checkName, checkSummary, checkErr)
1619
assert.Equal(t, "name matches", checkName, res.Name)
1720
assert.Equal(t, "state matches", api.StateSkipped, res.State)
1821
assert.Equal(t, "summary matches", checkSummary, res.Summary)
22+
assert.True(t, "details has err", res.Details["error"] != nil)
1923
}

‎internal/checks/kube/rbac.go

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,20 +22,29 @@ import (
2222
"k8s.io/kubectl/pkg/util/slice"
2323
)
2424

25+
var errSelfSubjectRulesReviewNotSupported = xerrors.New("cluster does not support SelfSubjectRulesReview")
26+
2527
// CheckRBAC checks the cluster for the RBAC permissions required by Coder.
2628
// It will attempt to first use a SelfSubjectRulesReview to determine the capabilities
2729
// of the user. If this fails (notably on GKE), fall back to using SelfSubjectAccessRequests
2830
// which is slower but is more likely to work.
2931
func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
30-
if ssrrResults := k.checkRBACDefault(ctx); ssrrResults != nil {
32+
ssrrResults, err := k.checkRBACDefault(ctx)
33+
if err == nil {
3134
return ssrrResults
3235
}
3336

34-
k.log.Warn(ctx, "SelfSubjectRulesReview response incomplete, falling back to SelfSubjectAccessRequests (slow)")
35-
return k.checkRBACFallback(ctx)
37+
if xerrors.Is(err, errSelfSubjectRulesReviewNotSupported) {
38+
// In this case, we should fall back to using SelfSubjectAccessRequests.
39+
k.log.Warn(ctx, "unable to check via SelfSubjectRulesReview, falling back to SelfSubjectAccessRequests (slow)")
40+
return k.checkRBACFallback(ctx)
41+
}
42+
43+
// something else went wrong
44+
return []*api.CheckResult{api.ErrorResult("kubernetes-rbac", "unable to check rbac", err)}
3645
}
3746

38-
func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) []*api.CheckResult {
47+
func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) ([]*api.CheckResult, error) {
3948
const checkName = "kubernetes-rbac-ssrr"
4049
authClient := k.client.AuthorizationV1()
4150
results := make([]*api.CheckResult, 0)
@@ -48,11 +57,11 @@ func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) []*api.CheckRe
4857

4958
response, err := authClient.SelfSubjectRulesReviews().Create(ctx, sar, metav1.CreateOptions{})
5059
if err != nil {
51-
return []*api.CheckResult{api.SkippedResult(checkName, "unable to create SelfSubjectRulesReview request: "+err.Error())}
60+
return nil, xerrors.Errorf("create SelfSubjectRulesReview: %w", err)
5261
}
5362

5463
if response.Status.Incomplete {
55-
return nil // In this case, we should fall back to using SelfSubjectAccessRequests.
64+
return nil, errSelfSubjectRulesReviewNotSupported
5665
}
5766

5867
// 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
97106
results = append(results, api.PassResult(checkName, summary))
98107
}
99108

100-
return results
109+
return results, nil
101110
}
102111

103112
func satisfies(req *ResourceRequirement, verbs ResourceVerbs, rules []rbacv1.PolicyRule) error {

‎internal/checks/kube/rbac_test.go

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,19 @@ import (
1515
"github.com/cdr/coder-doctor/internal/api"
1616
)
1717

18+
func Test_CheckRBAC_Error(t *testing.T) {
19+
t.Parallel()
20+
21+
srv := newTestHTTPServer(t, 500, nil)
22+
defer srv.Close()
23+
client, err := kubernetes.NewForConfig(&rest.Config{Host: srv.URL})
24+
assert.Success(t, "failed to create client", err)
25+
26+
checker := NewKubernetesChecker(client)
27+
results := checker.CheckRBAC(context.Background())
28+
assert.True(t, "should contain one result", len(results) == 1)
29+
assert.True(t, "result should be failed", results[0].State == api.StateFailed)
30+
}
1831
func Test_CheckRBACFallback(t *testing.T) {
1932
t.Parallel()
2033

@@ -99,15 +112,16 @@ func Test_CheckRBACDefault(t *testing.T) {
99112
tests := []struct {
100113
Name string
101114
Response *authorizationv1.SelfSubjectRulesReview
102-
F func(*testing.T, []*api.CheckResult)
115+
TestFunc func(*testing.T, []*api.CheckResult, error)
103116
}{
104117
{
105118
Name: "nothing allowed",
106119
Response: &selfSubjectRulesReviewEmpty,
107-
F: func(t *testing.T, results []*api.CheckResult) {
120+
TestFunc: func(t *testing.T, results []*api.CheckResult, err error) {
108121
assert.False(t, "results should not be empty", len(results) == 0)
109122
for _, result := range results {
110-
assert.True(t, result.Name+" should have an error", result.Details["error"] != nil)
123+
assert.True(t, result.Name+" should not return an error", err == nil)
124+
assert.True(t, result.Name+" should contain an error in details", result.Details["error"] != nil)
111125
assert.True(t, result.Name+" should fail", result.State == api.StateFailed)
112126
}
113127
},
@@ -126,16 +140,16 @@ func Test_CheckRBACDefault(t *testing.T) {
126140
assert.Success(t, "failed to create client", err)
127141

128142
checker := NewKubernetesChecker(client)
129-
results := checker.checkRBACDefault(context.Background())
130-
test.F(t, results)
143+
results, err := checker.checkRBACDefault(context.Background())
144+
test.TestFunc(t, results, err)
131145
})
132146
}
133147
}
134148

135149
func Test_Satisfies(t *testing.T) {
136150
t.Parallel()
137151

138-
testCases := []struct {
152+
tests := []struct {
139153
Name string
140154
Requirement *ResourceRequirement
141155
Verbs ResourceVerbs
@@ -209,14 +223,14 @@ func Test_Satisfies(t *testing.T) {
209223
// TODO(cian): add many, many, more.
210224
}
211225

212-
for _, testCase := range testCases {
213-
testCase := testCase
214-
t.Run(testCase.Name, func(t *testing.T) {
215-
actual := satisfies(testCase.Requirement, testCase.Verbs, testCase.Rules)
216-
if testCase.Expected == nil {
217-
assert.Success(t, testCase.Name+"- should not error", actual)
226+
for _, test := range tests {
227+
test := test
228+
t.Run(test.Name, func(t *testing.T) {
229+
actual := satisfies(test.Requirement, test.Verbs, test.Rules)
230+
if test.Expected == nil {
231+
assert.Success(t, test.Name+"- should not error", actual)
218232
} else {
219-
assert.ErrorContains(t, testCase.Name+" - expected error contains", actual, *testCase.Expected)
233+
assert.ErrorContains(t, test.Name+" - expected error contains", actual, *test.Expected)
220234
}
221235
})
222236
}

‎internal/checks/kube/resources.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ func (k *KubernetesChecker) CheckResources(_ context.Context) []*api.CheckResult
1616
dc := k.client.Discovery()
1717
lists, err := dc.ServerPreferredResources()
1818
if err != nil {
19-
results = append(results, api.SkippedResult(checkName, "unable to fetch api resources from server: "+err.Error()))
19+
results = append(results, api.SkippedResult(checkName, "unable to fetch api resources from server", err))
2020
return results
2121
}
2222

‎internal/checks/local/helm.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ var versionRequirements = []VersionRequirement{
3232

3333
func (l *Checker) CheckLocalHelmVersion(ctx context.Context) *api.CheckResult {
3434
if l.target != api.CheckTargetKubernetes {
35-
return api.SkippedResult(LocalHelmVersionCheck, "not applicable for target "+string(l.target))
35+
return api.SkippedResult(LocalHelmVersionCheck, "not applicable for target "+string(l.target), nil)
3636
}
3737

3838
helmBin, err := l.lookPathF("helm")

‎scripts/test_go.sh

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# Run unit and integration tests for Go code
44

55
set -euo pipefail
6+
CI=${CI:-""}
67
PROJECT_ROOT="$(git rev-parse --show-toplevel)"
78
cd "$PROJECT_ROOT"
89
source "./scripts/lib.sh"
@@ -49,11 +50,14 @@ run_trace false gotestsum tool slowest \
4950
--jsonfile="$TESTREPORT_JSON" \
5051
--threshold="$threshold"
5152

52-
# From time to time, Coveralls seems to have an issue on their end, so
53-
# make a best-effort attempt to upload coverage but ignore failures
54-
set +e
55-
echo "--- Uploading test coverage report to Coveralls..."
56-
run_trace false goveralls -service=github -coverprofile="$COVERAGE"
57-
set -e
53+
# Skip coveralls if not running in CI
54+
if [[ -n "${CI}" ]]; then
55+
# From time to time, Coveralls seems to have an issue on their end, so
56+
# make a best-effort attempt to upload coverage but ignore failures
57+
set +e
58+
echo "--- Uploading test coverage report to Coveralls..."
59+
run_trace false goveralls -service=github -coverprofile="$COVERAGE"
60+
set -e
61+
fi
5862

5963
exit $test_status

0 commit comments

Comments
 (0)