From 0cab59b4f69671da2ca00e0ea699d865b246e410 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 24 Aug 2021 12:39:46 +0000 Subject: [PATCH 1/3] internal/check/kube: validate rbac requirements --- internal/checks/kube/kubernetes.go | 20 ++++++++++++------ internal/checks/kube/kubernetes_test.go | 4 ++++ internal/checks/kube/rbac.go | 17 +++------------ internal/checks/kube/rbac_test.go | 23 --------------------- internal/cmd/check/kubernetes/kubernetes.go | 8 +++++++ 5 files changed, 29 insertions(+), 43 deletions(-) diff --git a/internal/checks/kube/kubernetes.go b/internal/checks/kube/kubernetes.go index 32c3a73..c40bec4 100644 --- a/internal/checks/kube/kubernetes.go +++ b/internal/checks/kube/kubernetes.go @@ -17,11 +17,12 @@ import ( var _ = api.Checker(&KubernetesChecker{}) type KubernetesChecker struct { - namespace string - client kubernetes.Interface - writer api.ResultWriter - coderVersion *semver.Version - log slog.Logger + namespace string + client kubernetes.Interface + writer api.ResultWriter + coderVersion *semver.Version + log slog.Logger + rbacRequirements []*RBACRequirement } type Option func(k *KubernetesChecker) @@ -40,6 +41,10 @@ func NewKubernetesChecker(client kubernetes.Interface, opts ...Option) *Kubernet opt(checker) } + // This may be nil. It is the responsibility of the caller to run + // KubernetesChecker.Validate() before Run(). + checker.rbacRequirements = findClosestVersionRequirements(checker.coderVersion) + return checker } @@ -67,7 +72,10 @@ func WithNamespace(ns string) Option { } } -func (*KubernetesChecker) Validate() error { +func (k *KubernetesChecker) Validate() error { + if k.rbacRequirements == nil { + return xerrors.Errorf("unhandled coder version: %s", k.coderVersion.String()) + } return nil } diff --git a/internal/checks/kube/kubernetes_test.go b/internal/checks/kube/kubernetes_test.go index dd4ab22..2e2a78a 100644 --- a/internal/checks/kube/kubernetes_test.go +++ b/internal/checks/kube/kubernetes_test.go @@ -6,6 +6,8 @@ import ( "k8s.io/client-go/kubernetes/fake" "cdr.dev/slog/sloggers/slogtest/assert" + + "github.com/Masterminds/semver/v3" ) func TestKubernetesOptions(t *testing.T) { @@ -15,6 +17,8 @@ func TestKubernetesOptions(t *testing.T) { checker := NewKubernetesChecker(clientset) assert.Success(t, "validation successful", checker.Validate()) + checker = NewKubernetesChecker(clientset, WithCoderVersion(semver.MustParse("1.19.0"))) + assert.ErrorContains(t, "KubernetesChecker with unknown version should fail to validate", checker.Validate(), "unhandled coder version") // var buf bytes.Buffer // log := slog.Make(sloghuman.Sink(&buf)).Leveled(slog.LevelDebug) // checker = NewKubernetesChecker(clientset, diff --git a/internal/checks/kube/rbac.go b/internal/checks/kube/rbac.go index 96f8e8a..5084f17 100644 --- a/internal/checks/kube/rbac.go +++ b/internal/checks/kube/rbac.go @@ -58,20 +58,9 @@ var allVersionedRBACRequirements = []VersionedRBACRequirements{ func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult { const checkName = "kubernetes-rbac" authClient := k.client.AuthorizationV1() - rbacReqs := findClosestVersionRequirements(k.coderVersion) results := make([]*api.CheckResult, 0) - if rbacReqs == nil { - results = append(results, - api.ErrorResult( - checkName, - "unable to check RBAC requirements", - xerrors.Errorf("unhandled coder version: %s", k.coderVersion.String()), - ), - ) - return results - } - for _, req := range rbacReqs.RBACRequirements { + for _, req := range k.rbacRequirements { resName := fmt.Sprintf("%s-%s", checkName, req.Resource) if err := k.checkOneRBAC(ctx, authClient, req); err != nil { summary := fmt.Sprintf("missing permissions on resource %s: %s", req.Resource, err) @@ -120,10 +109,10 @@ func (k *KubernetesChecker) checkOneRBAC(ctx context.Context, authClient authori return nil } -func findClosestVersionRequirements(v *semver.Version) *VersionedRBACRequirements { +func findClosestVersionRequirements(v *semver.Version) []*RBACRequirement { for _, vreqs := range allVersionedRBACRequirements { if vreqs.VersionConstraints.Check(v) { - return &vreqs + return vreqs.RBACRequirements } } return nil diff --git a/internal/checks/kube/rbac_test.go b/internal/checks/kube/rbac_test.go index d28c778..31b9fbd 100644 --- a/internal/checks/kube/rbac_test.go +++ b/internal/checks/kube/rbac_test.go @@ -13,8 +13,6 @@ import ( "cdr.dev/slog/sloggers/slogtest/assert" - "github.com/Masterminds/semver/v3" - "github.com/cdr/coder-doctor/internal/api" ) @@ -91,27 +89,6 @@ func Test_CheckRBAC_ClientError(t *testing.T) { } } -func Test_CheckRBAC_UnknownCoderVerseion(t *testing.T) { - t.Parallel() - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusInternalServerError) - })) - defer server.Close() - - client, err := kubernetes.NewForConfig(&rest.Config{Host: server.URL}) - assert.Success(t, "failed to create client", err) - - checker := NewKubernetesChecker(client, WithCoderVersion(semver.MustParse("0.0.1"))) - - results := checker.CheckRBAC(context.Background()) - for _, result := range results { - assert.ErrorContains(t, result.Name+" should show correct error", result.Details["error"].(error), "unhandled coder version") - assert.True(t, result.Name+" should fail", result.State == api.StateFailed) - } -} - var selfSubjectAccessReviewAllowed authorizationv1.SelfSubjectAccessReview = authorizationv1.SelfSubjectAccessReview{ Status: authorizationv1.SubjectAccessReviewStatus{ Allowed: true, diff --git a/internal/cmd/check/kubernetes/kubernetes.go b/internal/cmd/check/kubernetes/kubernetes.go index 7f9323a..9aab14e 100644 --- a/internal/cmd/check/kubernetes/kubernetes.go +++ b/internal/cmd/check/kubernetes/kubernetes.go @@ -141,10 +141,18 @@ func run(cmd *cobra.Command, _ []string) error { kube.WithNamespace(currentContext.Namespace), ) + if err := localChecker.Validate(); err != nil { + return xerrors.Errorf("failed to validate local checks: %w", err) + } + if err := localChecker.Run(cmd.Context()); err != nil { return xerrors.Errorf("run local checker: %w", err) } + if err := kubeChecker.Validate(); err != nil { + return xerrors.Errorf("failed to validate kube checker: %w", err) + } + if err := kubeChecker.Run(cmd.Context()); err != nil { return xerrors.Errorf("run kube checker: %w", err) } From 82e76be838ede63a175c5de4a03086e93198f51f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 24 Aug 2021 15:41:16 +0000 Subject: [PATCH 2/3] fix: validate local helm requirements --- internal/checks/local/helm.go | 4 +++- internal/checks/local/helm_test.go | 17 ++++++++--------- internal/checks/local/local.go | 6 +++++- internal/checks/local/local_test.go | 13 +++++++++++++ 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/internal/checks/local/helm.go b/internal/checks/local/helm.go index 4b1343f..531fd9e 100644 --- a/internal/checks/local/helm.go +++ b/internal/checks/local/helm.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/Masterminds/semver/v3" + "golang.org/x/xerrors" "cdr.dev/slog" "github.com/cdr/coder-doctor/internal/api" @@ -52,7 +53,8 @@ func (l *Checker) CheckLocalHelmVersion(ctx context.Context) *api.CheckResult { selectedVersion := findNearestHelmVersion(l.coderVersion) if selectedVersion == nil { - return api.ErrorResult(LocalHelmVersionCheck, fmt.Sprintf("checking coder version %s not supported", l.coderVersion.String()), nil) + // If this happens, Validate() was not called properly. + panic(xerrors.Errorf("helm version was nil - did you forget to call validate?")) } l.log.Debug(ctx, "selected coder version", slog.F("requested", l.coderVersion), slog.F("selected", selectedVersion.Coder)) diff --git a/internal/checks/local/helm_test.go b/internal/checks/local/helm_test.go index 34575ff..5348b54 100644 --- a/internal/checks/local/helm_test.go +++ b/internal/checks/local/helm_test.go @@ -127,18 +127,17 @@ func Test_CheckLocalHelmVersion(t *testing.T) { } }) - run(t, "helm: coder version is unsupported", func(t *testing.T, p *params) { + run(t, "helm: someone did not call validate", func(t *testing.T, p *params) { + defer func() { + if r := recover(); r == nil { + t.Errorf("this code should have panicked") + t.FailNow() + } + }() p.Opts = append(p.Opts, WithCoderVersion(semver.MustParse("v1.19"))) p.LP.Handle("helm", "/usr/local/bin/helm", nil) p.EX.Handle("/usr/local/bin/helm version --short", []byte("v3.6.0+g7f2df64"), nil) lc := NewChecker(p.Opts...) - err := lc.Run(p.Ctx) - assert.Success(t, "run local checker", err) - assert.False(t, "results should not be empty", p.W.Empty()) - for _, res := range p.W.Get() { - if res.Name == LocalHelmVersionCheck { - assert.Equal(t, "should fail", api.StateFailed, res.State) - } - } + _ = lc.Run(p.Ctx) }) } diff --git a/internal/checks/local/local.go b/internal/checks/local/local.go index 1ca5fd4..86eb2f2 100644 --- a/internal/checks/local/local.go +++ b/internal/checks/local/local.go @@ -83,7 +83,11 @@ func WithLookPathF(f LookPathF) Option { } } -func (*Checker) Validate() error { +func (l *Checker) Validate() error { + // Ensure we know the Helm version requirement for our Coder version. + if findNearestHelmVersion(l.coderVersion) == nil { + return xerrors.Errorf("unsupported coder version %s: compatible helm version not specified", l.coderVersion.String()) + } return nil } diff --git a/internal/checks/local/local_test.go b/internal/checks/local/local_test.go index f3b5570..f586672 100644 --- a/internal/checks/local/local_test.go +++ b/internal/checks/local/local_test.go @@ -4,8 +4,21 @@ import ( "context" "strings" "testing" + + "cdr.dev/slog/sloggers/slogtest/assert" + + "github.com/Masterminds/semver/v3" ) +func Test_LocalChecker_Validate(t *testing.T) { + t.Parallel() + lc := NewChecker() + assert.Success(t, "local checker with defaults should be successful", lc.Validate()) + + lc = NewChecker(WithCoderVersion(semver.MustParse("0.0.1"))) + assert.ErrorContains(t, "local checker with defaults should be successful", lc.Validate(), "unsupported coder version") +} + type execResult struct { Output []byte Err error From 27bb68c61b5647f16c8d5f1cf6324625431954e4 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 25 Aug 2021 09:25:34 +0000 Subject: [PATCH 3/3] fixup! fix: validate local helm requirements --- internal/checks/kube/kubernetes.go | 6 ++++-- internal/checks/kube/kubernetes_test.go | 24 +++++++++++++++++++----- internal/checks/local/helm.go | 5 ----- internal/checks/local/local.go | 6 +++++- internal/checks/local/local_test.go | 20 ++++++++++++++++---- main.go | 7 +++++++ 6 files changed, 51 insertions(+), 17 deletions(-) diff --git a/internal/checks/kube/kubernetes.go b/internal/checks/kube/kubernetes.go index c40bec4..6c0bd0f 100644 --- a/internal/checks/kube/kubernetes.go +++ b/internal/checks/kube/kubernetes.go @@ -41,10 +41,12 @@ func NewKubernetesChecker(client kubernetes.Interface, opts ...Option) *Kubernet opt(checker) } - // This may be nil. It is the responsibility of the caller to run - // KubernetesChecker.Validate() before Run(). checker.rbacRequirements = findClosestVersionRequirements(checker.coderVersion) + if err := checker.Validate(); err != nil { + panic(xerrors.Errorf("error validating kube checker: %w", err)) + } + return checker } diff --git a/internal/checks/kube/kubernetes_test.go b/internal/checks/kube/kubernetes_test.go index 2e2a78a..145c4fb 100644 --- a/internal/checks/kube/kubernetes_test.go +++ b/internal/checks/kube/kubernetes_test.go @@ -13,12 +13,26 @@ import ( func TestKubernetesOptions(t *testing.T) { t.Parallel() - clientset := fake.NewSimpleClientset() - checker := NewKubernetesChecker(clientset) - assert.Success(t, "validation successful", checker.Validate()) + t.Run("successful validation", func(t *testing.T) { + clientset := fake.NewSimpleClientset() + checker := NewKubernetesChecker(clientset) + assert.Success(t, "validation successful", checker.Validate()) + }) + + t.Run("validation failed: unhandled coder version", func(t *testing.T) { + defer func() { + r := recover() + if r == nil { + t.Errorf("expected a panic") + t.FailNow() + } + assert.ErrorContains(t, "KubernetesChecker with unknown version should fail to validate", r.(error), "unhandled coder version") + }() + + // This should panic + _ = NewKubernetesChecker(fake.NewSimpleClientset(), WithCoderVersion(semver.MustParse("1.19.0"))) + }) - checker = NewKubernetesChecker(clientset, WithCoderVersion(semver.MustParse("1.19.0"))) - assert.ErrorContains(t, "KubernetesChecker with unknown version should fail to validate", checker.Validate(), "unhandled coder version") // var buf bytes.Buffer // log := slog.Make(sloghuman.Sink(&buf)).Leveled(slog.LevelDebug) // checker = NewKubernetesChecker(clientset, diff --git a/internal/checks/local/helm.go b/internal/checks/local/helm.go index 531fd9e..0c4ac67 100644 --- a/internal/checks/local/helm.go +++ b/internal/checks/local/helm.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/Masterminds/semver/v3" - "golang.org/x/xerrors" "cdr.dev/slog" "github.com/cdr/coder-doctor/internal/api" @@ -52,10 +51,6 @@ func (l *Checker) CheckLocalHelmVersion(ctx context.Context) *api.CheckResult { } selectedVersion := findNearestHelmVersion(l.coderVersion) - if selectedVersion == nil { - // If this happens, Validate() was not called properly. - panic(xerrors.Errorf("helm version was nil - did you forget to call validate?")) - } l.log.Debug(ctx, "selected coder version", slog.F("requested", l.coderVersion), slog.F("selected", selectedVersion.Coder)) result := &api.CheckResult{ diff --git a/internal/checks/local/local.go b/internal/checks/local/local.go index 86eb2f2..4d2bc0d 100644 --- a/internal/checks/local/local.go +++ b/internal/checks/local/local.go @@ -44,6 +44,10 @@ func NewChecker(opts ...Option) *Checker { opt(checker) } + if err := checker.Validate(); err != nil { + panic(xerrors.Errorf("error validating local checker: %w", err)) + } + return checker } @@ -86,7 +90,7 @@ func WithLookPathF(f LookPathF) Option { func (l *Checker) Validate() error { // Ensure we know the Helm version requirement for our Coder version. if findNearestHelmVersion(l.coderVersion) == nil { - return xerrors.Errorf("unsupported coder version %s: compatible helm version not specified", l.coderVersion.String()) + return xerrors.Errorf("unhandled coder version %s: compatible helm version not specified", l.coderVersion.String()) } return nil } diff --git a/internal/checks/local/local_test.go b/internal/checks/local/local_test.go index f586672..6535a40 100644 --- a/internal/checks/local/local_test.go +++ b/internal/checks/local/local_test.go @@ -12,11 +12,23 @@ import ( func Test_LocalChecker_Validate(t *testing.T) { t.Parallel() - lc := NewChecker() - assert.Success(t, "local checker with defaults should be successful", lc.Validate()) - lc = NewChecker(WithCoderVersion(semver.MustParse("0.0.1"))) - assert.ErrorContains(t, "local checker with defaults should be successful", lc.Validate(), "unsupported coder version") + t.Run("successful validation", func(t *testing.T) { + lc := NewChecker() + assert.Success(t, "local checker with defaults should be successful", lc.Validate()) + }) + + t.Run("validation failed: unsupported coder version", func(t *testing.T) { + defer func() { + r := recover() + if r == nil { + t.Errorf("expected a panic") + t.FailNow() + } + assert.ErrorContains(t, "KubernetesChecker with unknown version should fail to validate", r.(error), "unhandled coder version") + }() + _ = NewChecker(WithCoderVersion(semver.MustParse("0.0.1"))) + }) } type execResult struct { diff --git a/main.go b/main.go index ec0aa65..f6e01e2 100644 --- a/main.go +++ b/main.go @@ -2,12 +2,19 @@ package main import ( "context" + "fmt" "os" "github.com/cdr/coder-doctor/internal/cmd" ) func main() { + defer func() { + if r := recover(); r != nil { + _, _ = fmt.Fprintln(os.Stderr, "fatal:", r.(error)) + } + os.Exit(1) + }() command := cmd.NewDefaultDoctorCommand() err := command.ExecuteContext(context.Background()) if err != nil {