feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

johnstcn · 2021-09-01T14:57:41Z

Modifies RBAC checking logic to first attempt a SSRR. If this returns incomplete data (as can be seen in GKE), fallback to the slower method of multiple SSARs.
Adds associated unit tests.
fix: sets current namespace to default if unspecified.
Adds a make test target because I'm lazy and don't like typing.

NOTES:
a) Yes, that is a dependency on kubectl. I'm happy to drop it if it means just copy-pasting a couple of utility functions.
b) The way we check our own requirements against the SSRR PolicyRules is probably not optimal with regards to speed, but I figure we can worry about speeding it up later.

shortcut-integration · 2021-09-01T14:57:48Z

This pull request has been linked to Clubhouse Story #15962: Checks for required cluster API resources.

jawnsy

Thanks for your awesome work with this. I left some minor comments but none of them are blocking, we can merge and iterate. 👍

jawnsy · 2021-09-01T15:16:22Z

go.mod

@@ -11,5 +11,5 @@ require (
 	k8s.io/apimachinery v0.19.14
 	k8s.io/client-go v0.19.14
 	k8s.io/klog/v2 v2.10.0 // indirect
-	k8s.io/utils v0.0.0-20210305010621-2afb4311ab10 // indirect
+	k8s.io/kubectl v0.19.14


comment: tough call, seems we're either:

doing something upstream tells us not to do (import kubectl); or

signing on to maintain a copy of some code

Maybe this one in particular is OK and it's just stuff in k/k that we shouldn't import:

To use Kubernetes code as a library in other applications, see the list of published components. Use of the k8s.io/kubernetes module or k8s.io/kubernetes/... packages as libraries is not supported.

I'm unsure and inclined to just merge what you have 🤷‍♂️

jawnsy · 2021-09-01T15:18:16Z

Makefile

+
+test/go:
+	@echo "--- go test"
+	go test -parallel=$(shell nproc) ./...


suggestion: use go test -parallel ./...

I believe go test -parallel (no options) should auto-detect available processors, per go help testflag:

-parallel n Allow parallel execution of test functions that call t.Parallel. The value of this flag is the maximum number of tests to run simultaneously; by default, it is set to the value of GOMAXPROCS. Note that -parallel only applies within a single test binary. The 'go test' command may run tests for different packages in parallel as well, according to the setting of the -p flag (see 'go help build')

in the future, we might want to consider using gotestsum as the output is nicer

jawnsy · 2021-09-01T15:21:22Z

internal/checks/kube/rbac.go

+
+	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())}


suggestion: consider adding the error to the Details field, since it's structured data.

the longer-term idea is that someone should be able to submit an output log (JSON dump or something) of their CheckResult logs, and we should be able to ingest those into BigQuery or something as a form of cluster telemetry, to help guide our product decisions -- parsing text out is possible but I think it's better to avoid that

jawnsy · 2021-09-01T15:22:42Z

internal/checks/kube/rbac.go

+		breakdownRules = append(breakdownRules, rbacutil.BreakdownRule(rule)...)
+	}
+
+	compactRules, _ := rbacutil.CompactRules(breakdownRules) //nolint:errcheck - err is always nil


question/suggestion: is err always guaranteed by the API contract to be nil, or is that just the current implementation? if the latter, then I think we should still check, otherwise we can have panics if they change things later

it would seem unusual to me for them to return an error that is always guaranteed to be nil, but I'm not too familiar with this API

Current implementation upstream always returns nil, weirdly enough 🤷
It's probably not guaranteed though.

if it's not guaranteed, then IMO we should check, otherwise it's just a ticking time bomb :)

jawnsy · 2021-09-01T15:23:24Z

internal/checks/kube/rbac.go

+
+	compactRules, _ := rbacutil.CompactRules(breakdownRules) //nolint:errcheck - err is always nil
+	sort.Stable(rbacutil.SortableRuleSlice(compactRules))
+	for _, r := range compactRules {


nitpick: I don't know if we can check if Debug logging is enabled; if not, we could avoid the for loop/method call overhead

jawnsy · 2021-09-01T15:31:21Z

internal/checks/kube/rbac_test.go

+	tests := []struct {
+		Name     string
+		Response *authorizationv1.SelfSubjectRulesReview
+		F        func(*testing.T, []*api.CheckResult)


nitpick: just a personal preference and maybe anathema to Go's philosophy, I think it's clearer to use full names for things instead of one-letter variables, e.g. CheckFunc or something

jawnsy · 2021-09-01T15:31:59Z

internal/checks/kube/rbac_test.go

+			F: func(t *testing.T, results []*api.CheckResult) {
+				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)


suggestion/nitpick: we might want to consider a helper func that does this, similar to what we do in slog

jawnsy · 2021-09-01T15:33:12Z

internal/checks/kube/rbac_test.go

+	},
+}
+
+func strptr(s string) *string {


kubernetes has a pointer.String() util for this

jawnsy · 2021-09-01T15:34:10Z

internal/cmd/check/kubernetes/kubernetes.go

@@ -116,6 +116,9 @@ func run(cmd *cobra.Command, _ []string) error {
 	}

 	currentContext := rawConfig.Contexts[rawConfig.CurrentContext]
+	if currentContext.Namespace == "" {
+		currentContext.Namespace = "default"


I guess Kubernetes doesn't do this automatically, for some reason?

I don't know if we have a default namespace, if we did I'd assume it to be coder rather than default, so if we need a namespace we might want to consider using coder here?

jawnsy · 2021-09-01T15:35:20Z

internal/checks/kube/rbac_test.go

+		// TODO(cian): add many, many, more.
+	}
+
+	for _, testCase := range testCases {


nitpick: personally I use tests for the array and test for the loop var, not a strong feeling, might be nice to be consistent

* 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 #11

johnstcn added 7 commits August 30, 2021 10:35

add api.WarnResult

56ce81b

feat: use SSRR for RBAC instead of SSAR

c49a547

chore: Makefile: add test target

dcb95d4

fix: RBAC: use SSRR by default, fallback to SSAR

a98c45b

fix: ssrr checks

7e8bfdb

rbac_test: add more tests

b0f4120

fixup! feat: use SSRR for RBAC instead of SSAR

65da237

johnstcn requested a review from jawnsy September 1, 2021 15:06

johnstcn self-assigned this Sep 1, 2021

jawnsy approved these changes Sep 1, 2021

View reviewed changes

johnstcn merged commit 18da19e into main Sep 1, 2021

johnstcn deleted the cianjohnston/ch15962/rbac_ssrr branch September 1, 2021 15:47

johnstcn added a commit that referenced this pull request Sep 13, 2021

chore: internal/rbac: address comment from #11

575b134

johnstcn mentioned this pull request Sep 13, 2021

address comments from #11 #15

Merged

johnstcn added a commit that referenced this pull request Sep 13, 2021

fixup! chore: internal/rbac: address comment from #11

c7dd425

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

johnstcn commented Sep 1, 2021

shortcut-integration bot commented Sep 1, 2021

jawnsy left a comment

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

johnstcn Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

jawnsy Sep 1, 2021

feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

Conversation

johnstcn commented Sep 1, 2021

shortcut-integration bot commented Sep 1, 2021

jawnsy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment