Skip to content
This repository was archived by the owner on Nov 14, 2024. It is now read-only.

internal/checks/kube: add more API resource requirements #10

Merged
merged 2 commits into from
Aug 27, 2021
Merged

internal/checks/kube: add more API resource requirements #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9834667
internal/checks/kube/resources.go: skip instead of fail if api-resour…
johnstcn Aug 27, 2021
468ee3b
internal/checks/kube: update resource requirements from current helm …
johnstcn Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions internal/checks/kube/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ import (
var _ = api.Checker(&KubernetesChecker{})

type KubernetesChecker struct {
namespace string
client kubernetes.Interface
writer api.ResultWriter
coderVersion *semver.Version
log slog.Logger
rbacRequirements map[*ResourceRequirement]ResourceVerbs
namespace string
client kubernetes.Interface
writer api.ResultWriter
coderVersion *semver.Version
log slog.Logger
reqs *VersionedResourceRequirements
}

type Option func(k *KubernetesChecker)
Expand All @@ -41,7 +41,7 @@ func NewKubernetesChecker(client kubernetes.Interface, opts ...Option) *Kubernet
opt(checker)
}

checker.rbacRequirements = findClosestVersionRequirements(checker.coderVersion)
checker.reqs = findClosestVersionRequirements(checker.coderVersion)

if err := checker.Validate(); err != nil {
panic(xerrors.Errorf("error validating kube checker: %w", err))
Expand Down Expand Up @@ -75,7 +75,7 @@ func WithNamespace(ns string) Option {
}

func (k *KubernetesChecker) Validate() error {
if k.rbacRequirements == nil {
if k.reqs == nil {
return xerrors.Errorf("unhandled coder version: %s", k.coderVersion.String())
}
return nil
Expand Down
20 changes: 17 additions & 3 deletions internal/checks/kube/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,21 @@ func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
authClient := k.client.AuthorizationV1()
results := make([]*api.CheckResult, 0)

for req, reqVerbs := range k.rbacRequirements {
for req, reqVerbs := range k.reqs.ResourceRequirements {
resName := fmt.Sprintf("%s-%s", checkName, req.Resource)
if err := k.checkOneRBAC(ctx, authClient, req, reqVerbs); err != nil {
summary := fmt.Sprintf("missing permissions on resource %s: %s", req.Resource, err)
results = append(results, api.ErrorResult(resName, summary, err))
continue
}

summary := fmt.Sprintf("%s: can %s", req.Resource, strings.Join(reqVerbs, ", "))
results = append(results, api.PassResult(resName, summary))
}

// TODO: delete this when the enterprise-helm role no longer requests resources on things
// that don't exist.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 100% duplicated code, but it's easy to delete.

for req, reqVerbs := range k.reqs.RoleOnlyResourceRequirements {
resName := fmt.Sprintf("%s-%s", checkName, req.Resource)
if err := k.checkOneRBAC(ctx, authClient, req, reqVerbs); err != nil {
summary := fmt.Sprintf("missing permissions on resource %s: %s", req.Resource, err)
Expand Down Expand Up @@ -70,10 +84,10 @@ func (k *KubernetesChecker) checkOneRBAC(ctx context.Context, authClient authori
return nil
}

func findClosestVersionRequirements(v *semver.Version) map[*ResourceRequirement]ResourceVerbs {
func findClosestVersionRequirements(v *semver.Version) *VersionedResourceRequirements {
for _, vreqs := range allRequirements {
if vreqs.VersionConstraints.Check(v) {
return vreqs.ResourceRequirements
return &vreqs
}
}
return nil
Expand Down
5 changes: 2 additions & 3 deletions internal/checks/kube/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.ErrorResult(checkName, "unable to fetch api resources from server", err))
results = append(results, api.SkippedResult(checkName, "unable to fetch api resources from server: "+err.Error()))
return results
}

Expand Down Expand Up @@ -45,8 +45,7 @@ func (k *KubernetesChecker) CheckResources(_ context.Context) []*api.CheckResult
}
}

versionReqs := findClosestVersionRequirements(k.coderVersion)
for versionReq := range versionReqs {
for versionReq := range k.reqs.ResourceRequirements {
if !resourcesAvailable[*versionReq] {
msg := fmt.Sprintf("missing required resource:%q group:%q version:%q", versionReq.Resource, versionReq.Group, versionReq.Version)
errResult := api.ErrorResult(checkName, msg, xerrors.New(msg))
Expand Down
62 changes: 50 additions & 12 deletions internal/checks/kube/resources_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,53 @@ import (
"github.com/cdr/coder-doctor/internal/api"
)

// This is a list of all known Resource and/or RBAC requirements for each verison of Coder.
// Order by version DESCENDING.
var allRequirements = []VersionedResourceRequirements{
{
VersionConstraints: api.MustConstraint(">= 1.20"),
ResourceRequirements: map[*ResourceRequirement]ResourceVerbs{
NewResourceRequirement("", "v1", "pods"): verbsCreateDeleteList,
NewResourceRequirement("", "v1", "secrets"): verbsCreateDeleteList,
NewResourceRequirement("", "v1", "serviceaccounts"): verbsCreateDeleteList,
NewResourceRequirement("", "v1", "services"): verbsCreateDeleteList,
NewResourceRequirement("apps", "apps/v1", "deployments"): verbsCreateDeleteList,
NewResourceRequirement("apps", "apps/v1", "replicasets"): verbsCreateDeleteList,
NewResourceRequirement("apps", "apps/v1", "statefulsets"): verbsCreateDeleteList,
NewResourceRequirement("networking.k8s.io", "networking.k8s.io/v1", "ingresses"): verbsCreateDeleteList,
NewResourceRequirement("rbac.authorization.k8s.io", "rbac.authorization.k8s.io/v1", "roles"): verbsCreateDeleteList,
NewResourceRequirement("rbac.authorization.k8s.io", "rbac.authorization.k8s.io/v1", "rolebindings"): verbsCreateDeleteList,
NewResourceRequirement("", "v1", "events"): verbsAll,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this here so we can have a threaded discussion:

At this point, running the check is starting to take a while, so I'd say the next step is to look into parallelising the RBAC checks (or using the equivalent API call to kubectl auth can-i --list).

@johnstcn I think it's preferable to figure out if we can reduce our requests before we parallelize, because doing stuff in parallel increases load on the API server and it might have throttling in place anyways. Is it possible to check multiple RBAC settings in one API call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I believe doing the equivalent of kubectl auth can-i --list should work here.
Would you be happy with me pursuing this in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, working code is better than perfect code :)

NewResourceRequirement("", "v1", "persistentvolumeclaims"): verbsAll,
NewResourceRequirement("", "v1", "pods"): verbsAll,
NewResourceRequirement("", "v1", "secrets"): verbsAll,
NewResourceRequirement("", "v1", "serviceaccounts"): verbsAll,
NewResourceRequirement("", "v1", "services"): verbsAll,
NewResourceRequirement("apps", "apps/v1", "deployments"): verbsAll,
NewResourceRequirement("apps", "apps/v1", "replicasets"): verbsAll,
NewResourceRequirement("apps", "apps/v1", "statefulsets"): verbsAll,
NewResourceRequirement("metrics.k8s.io", "metrics.k8s.io/v1beta1", "pods"): verbsGetListWatch,
NewResourceRequirement("networking.k8s.io", "networking.k8s.io/v1", "ingresses"): verbsAll,
NewResourceRequirement("networking.k8s.io", "networking.k8s.io/v1", "networkpolicies"): verbsAll,
NewResourceRequirement("rbac.authorization.k8s.io", "rbac.authorization.k8s.io/v1", "roles"): verbsGetCreate,
NewResourceRequirement("rbac.authorization.k8s.io", "rbac.authorization.k8s.io/v1", "rolebindings"): verbsGetCreate,
NewResourceRequirement("storage.k8s.io", "storage.k8s.io/v1", "storageclasses"): verbsGetListWatch,
},
RoleOnlyResourceRequirements: map[*ResourceRequirement]ResourceVerbs{
// The below permissions are required by the default coder role created by the Helm chart.
// Installation will fail if these are not present in the role being used to install Coder.
NewResourceRequirement("", "v1", "deployments"): verbsAll,
NewResourceRequirement("", "v1", "networkpolicies"): verbsAll,
NewResourceRequirement("", "v1", "pods/exec"): verbsAll,
NewResourceRequirement("", "v1", "pods/log"): verbsAll,
NewResourceRequirement("apps", "v1", "events"): verbsAll,
NewResourceRequirement("apps", "v1", "networkpolicies"): verbsAll,
NewResourceRequirement("apps", "v1", "persistentvolumeclaims"): verbsAll,
NewResourceRequirement("apps", "v1", "pods"): verbsAll,
NewResourceRequirement("apps", "v1", "pods/exec"): verbsAll,
NewResourceRequirement("apps", "v1", "pods/log"): verbsAll,
NewResourceRequirement("apps", "v1", "secrets"): verbsAll,
NewResourceRequirement("apps", "v1", "services"): verbsAll,
NewResourceRequirement("metrics.k8s.io", "v1beta1", "storageclasses"): verbsGetListWatch,
NewResourceRequirement("networking.k8s.io", "v1", "deployments"): verbsAll,
NewResourceRequirement("networking.k8s.io", "v1", "events"): verbsAll,
NewResourceRequirement("networking.k8s.io", "v1", "persistentvolumeclaims"): verbsAll,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of these don't appear to be networking requirements, maybe a typo of the group name here?

Not sure if that's related to your comment:

I ended up needing to break out some of the ResourceRequirements into a separate RoleOnlyResourceRequirements, due to how the role is specified in enterprise-helm. A number of the resources required to create the role don't seem to be things that can exist in the real world (please do correct me if I'm mistaken!)

There might be an issue in our Helm chart... The RBAC permissions are done very generically at an API level, so do not account for whether or not the resources actually exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the enterprise helm chart specifies the following:

rules:
  - apiGroups: ["", "apps", "networking.k8s.io"] # "" indicates the core API group
    resources: ["persistentvolumeclaims", "pods", "deployments", "services", "secrets", "pods/exec","pods/log", "events", "networkpolicies"]
    verbs: ["create", "get", "list", "watch", "update", "patch", "delete", "deletecollection"]
  - apiGroups: ["metrics.k8s.io", "storage.k8s.io"]
    resources: ["pods", "storageclasses"]
    verbs: ["get", "list", "watch"]

That seems to be exploded by Kubernetes to the things we're seeing.
I agree that we should fix it. However, it's a pretty high-impact change that we'd want to approach carefully, and I don't want to make coder-doctor working properly dependant on that being fixed.

NewResourceRequirement("networking.k8s.io", "v1", "pods"): verbsAll,
NewResourceRequirement("networking.k8s.io", "v1", "pods/exec"): verbsAll,
NewResourceRequirement("networking.k8s.io", "v1", "pods/log"): verbsAll,
NewResourceRequirement("networking.k8s.io", "v1", "secrets"): verbsAll,
NewResourceRequirement("networking.k8s.io", "v1", "services"): verbsAll,
NewResourceRequirement("storage.k8s.io", "v1", "pods"): verbsGetListWatch,
},
},
}
Expand All @@ -39,11 +72,16 @@ type ResourceVerbs []string
type VersionedResourceRequirements struct {
VersionConstraints *semver.Constraints
ResourceRequirements map[*ResourceRequirement]ResourceVerbs
// These are only required because the role in the Helm chart specifies broad swathes of permissions that
// don't necessarily exist in the real world.
RoleOnlyResourceRequirements map[*ResourceRequirement]ResourceVerbs
}

var verbsCreateDeleteList ResourceVerbs = []string{"create", "delete", "list"}
var verbsAll ResourceVerbs = []string{"create", "delete", "deletecollection", "get", "list", "update", "patch", "watch"}
var verbsGetListWatch = []string{"get", "list", "watch"}
var verbsGetCreate = []string{"get", "create"}

// NewResourceRequirement is just a convenience function for creating ResourceRequirements.
// NewResourceRequirement is just a convenience function for creating ResourceRequirements for which the resource type must exist.
func NewResourceRequirement(apiGroup, version, resource string) *ResourceRequirement {
return &ResourceRequirement{
Group: apiGroup,
Expand Down
117 changes: 43 additions & 74 deletions internal/checks/kube/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ func Test_KubernetesChecker_CheckResources(t *testing.T) {
w.WriteHeader(http.StatusOK)
err := json.NewEncoder(w).Encode(appsV1ResourceList)
assert.Success(t, "failed to encode response", err)
case "/apis/metrics.k8s.io/v1":
w.WriteHeader(http.StatusOK)
err := json.NewEncoder(w).Encode(metricsV1ResourceLilst)
assert.Success(t, "failed to encode response", err)
case "/apis/networking.k8s.io/v1":
w.WriteHeader(http.StatusOK)
err := json.NewEncoder(w).Encode(networkingV1ResourceList)
Expand All @@ -77,6 +81,10 @@ func Test_KubernetesChecker_CheckResources(t *testing.T) {
w.WriteHeader(http.StatusOK)
err := json.NewEncoder(w).Encode(rbacV1ResourceList)
assert.Success(t, "failed to encode response", err)
case "/apis/storage.k8s.io/v1":
w.WriteHeader(http.StatusOK)
err := json.NewEncoder(w).Encode(storageV1ResourceList)
assert.Success(t, "failed to encode response", err)
default:
w.WriteHeader(http.StatusNotFound)
}
Expand Down Expand Up @@ -124,6 +132,15 @@ var fullAPIGroupList *metav1.APIGroupList = &metav1.APIGroupList{
},
},
},
{
Name: "metrics.k8s.io",
Versions: []metav1.GroupVersionForDiscovery{
{
GroupVersion: "metrics.k8s.io/v1beta1",
Version: "v1",
},
},
},
{
Name: "networking.k8s.io",
Versions: []metav1.GroupVersionForDiscovery{
Expand All @@ -142,84 +159,36 @@ var fullAPIGroupList *metav1.APIGroupList = &metav1.APIGroupList{
},
},
},
},
}

var v1ResourceList = &metav1.APIResourceList{
TypeMeta: metav1.TypeMeta{
Kind: "APIResourceList",
},
GroupVersion: "v1",
APIResources: []metav1.APIResource{
{
Name: "pods",
Verbs: []string{"get"},
},
{
Name: "secrets",
Verbs: []string{"get"},
},
{
Name: "serviceaccounts",
Verbs: []string{"get"},
},
{
Name: "services",
Verbs: []string{"get"},
},
},
}

var appsV1ResourceList = &metav1.APIResourceList{
TypeMeta: metav1.TypeMeta{
Kind: "APIResourceList",
APIVersion: "v1",
},
GroupVersion: "apps/v1",
APIResources: []metav1.APIResource{
{
Name: "deployments",
Verbs: []string{"get"},
},
{
Name: "replicasets",
Verbs: []string{"get"},
},
{
Name: "statefulsets",
Verbs: []string{"get"},
Name: "storage.k8s.io",
Versions: []metav1.GroupVersionForDiscovery{
{
GroupVersion: "storage.k8s.io/v1",
Version: "v1",
},
},
},
},
}

var networkingV1ResourceList = &metav1.APIResourceList{
TypeMeta: metav1.TypeMeta{
Kind: "APIResourceList",
APIVersion: "v1",
},
GroupVersion: "networking.k8s.io/v1",
APIResources: []metav1.APIResource{
{
Name: "ingresses",
Verbs: []string{"get"},
},
},
}
var v1ResourceList = makeResourceList("", "v1", "persistentvolumes", "persistentvolumeclaims", "events", "pods", "secrets", "serviceaccounts", "services")
var appsV1ResourceList = makeResourceList("v1", "apps/v1", "deployments", "replicasets", "statefulsets")
var metricsV1ResourceLilst = makeResourceList("v1", "metrics.k8s.io/v1beta1", "pods")
var networkingV1ResourceList = makeResourceList("v1", "networking.k8s.io/v1", "ingresses", "networkpolicies")
var rbacV1ResourceList = makeResourceList("v1", "rbac.authorization.k8s.io/v1", "roles", "rolebindings")
var storageV1ResourceList = makeResourceList("v1", "storage.k8s.io/v1", "roles", "storageclasses")

var rbacV1ResourceList = &metav1.APIResourceList{
TypeMeta: metav1.TypeMeta{
Kind: "APIResourceList",
APIVersion: "v1",
},
GroupVersion: "rbac.authorization.k8s.io/v1",
APIResources: []metav1.APIResource{
{
Name: "roles",
Verbs: []string{"get"},
},
{
Name: "rolebindings",
Verbs: []string{"get"},
},
},
func makeResourceList(version, groupVersion string, resources ...string) *metav1.APIResourceList {
rl := &metav1.APIResourceList{
TypeMeta: metav1.TypeMeta{
Kind: "APIResourceList",
APIVersion: version,
},
GroupVersion: groupVersion,
APIResources: []metav1.APIResource{},
}
for _, resource := range resources {
rl.APIResources = append(rl.APIResources, metav1.APIResource{Name: resource, Verbs: []string{"get"}})
}
return rl
}