Merge pull request #100 from josephschorr/small-improvements

Small improvements to the proxy

Author: josephschorr

README.md

@@ -1,7 +1,7 @@
 # spicedb-kubeapi-proxy
 
-`spicedb-kubeapi-proxy` is a proxy that runs in between clients and the kube 
-apiserver that can authorize requests and filter responses using an embedded or 
+`spicedb-kubeapi-proxy` is a proxy that runs in between clients and the kube
+apiserver that can authorize requests and filter responses using an embedded or
 remote SpiceDB.
 
 ## Status
@@ -30,13 +30,13 @@ the current state of the project, but the primary goals before 1.0 are:
 ![Arch Diagram Dark](./docs/proxy-arch-dark.png#gh-dark-mode-only)![Arch Diagram Light](./docs/proxy-arch-light.png#gh-light-mode-only)
 
 The proxy authenticates itself with the downstream kube-apiserver (client certs
-if running out-of-cluster, service account token if running in-cluster). 
+if running out-of-cluster, service account token if running in-cluster).
 The proxy is configured with a set of rules that define how to authorize requests
 and how to filter responses by communicating with SpiceDB.
 
 There are three basic types of rule:
 
-- **Check** rules: these are used to authorize whether a request is allowed to 
+- **Check** rules: these are used to authorize whether a request is allowed to
   proceed at all. For example, a rule might say that a user can only list pods
   in a namespace `foo` if they have a `namespace:foo#list@user:alice` permission
   in SpiceDB.
@@ -46,34 +46,36 @@ There are three basic types of rule:
   in SpiceDB that enumerate the allowed pods, like `pod:foo/a#view@user:alice`
   and `pod:foo/b#view@user:alice`. In this example, `alice` would see pods `a`
   and `b` in namespace `foo`, but no others.
-- **Write** rules: these are used to write data to SpiceDB based on the request
-  that the proxy is authorizing. For example, if `alice` creates a new pod `c` 
-  in namespace `foo`, a rule can determine that a relationship should be written 
-  to SpiceDB that grants ownership, i.e. `pod:foo/a#view@user:alice`.
+- **Update Relationship** rules: these are used to write/delete data in
+  SpiceDB based on the request that the proxy is authorizing. For example,
+  if `alice` creates a new pod `c` in namespace `foo`, a rule can determine
+  that a relationship should be written to SpiceDB that grants ownership,
+  i.e. `pod:foo/a#view@user:alice`.
 
 Rules often work in tendem; for example, a `Check` rule might authorize a request
 to list pods in a namespace, and a `Filter` rule might further restrict the
 response to only include certain pods.
 
 Note that the proxy does not assume anything about the structure of the data in
 SpiceDB. It is up to the user to define the data in SpiceDB and the rules that
-the proxy uses to authorize and filter requests. 
+the proxy uses to authorize and filter requests.
 
-The proxy rejects any request for which it doesn't find a matching rule. 
+The proxy rejects any request for which it doesn't find a matching rule.
 
 # Development
 
 This project uses `mage` to offer various development-related commands.
 
 ```bash
-# run to get all available commands 
+# run to get all available commands
 brew install mage
 mage
 ```
 
 ## Tests
 
 Runs both unit and e2e tests
+
 ```bash
 mage test:all
 ```

deploy/rules.yaml

@@ -7,7 +7,7 @@ match:
   verbs: ["create"]
 mustNot:
 - tpl: "namespace:{{name}}#cluster@cluster:cluster"
-write:
+update:
 - tpl: "namespace:{{name}}#creator@user:{{user.Name}}"
 - tpl: "namespace:{{name}}#cluster@cluster:cluster"
 ---
@@ -18,7 +18,7 @@ match:
 - apiVersion: v1
   resource: namespaces
   verbs: ["delete"]
-write:
+update:
 - tpl: "namespace:{{name}}#creator@user:{{user.Name}}"
 - tpl: "namespace:{{name}}#cluster@cluster:cluster"
 ---
@@ -40,7 +40,7 @@ match:
 prefilter:
 - name: "resourceId"
   byResource:
-    tpl: "namespace:*#view@user:{{user.Name}}"
+    tpl: "namespace:$resourceID#view@user:{{user.Name}}"
 ---
 apiVersion: authzed.com/v1alpha1
 kind: ProxyRule
@@ -51,7 +51,7 @@ match:
   verbs: ["create"]
 mustNot:
 - tpl: "pod:{{name}}#namespace@namespace:{{namespace}}"
-write:
+update:
 - tpl: "pod:{{namespacedName}}#creator@user:{{user.Name}}"
 - tpl: "pod:{{name}}#namespace@namespace:{{namespace}}"
 ---
@@ -62,7 +62,7 @@ match:
 - apiVersion: v1
   resource: pods
   verbs: ["delete"]
-write:
+update:
 - tpl: "pod:{{namespacedName}}#creator@user:{{user.Name}}"
 - tpl: "pod:{{name}}#namespace@namespace:{{namespace}}"
 ---
@@ -85,4 +85,4 @@ prefilter:
 - namespace: "splitNamespace(resourceId)"
   name: "splitName(resourceId)"
   byResource:
-    tpl: "pod:*#view@user:{{user.Name}}"
+    tpl: "pod:$resourceID#view@user:{{user.Name}}"

e2e/proxy_test.go

@@ -623,7 +623,7 @@ var (
 				Resource:     "namespaces",
 				Verbs:        []string{"create"},
 			}},
-			Writes: []proxyrule.StringOrTemplate{{
+			Updates: []proxyrule.StringOrTemplate{{
 				Template: "namespace:{{name}}#creator@user:{{user.Name}}",
 			}, {
 				Template: "namespace:{{name}}#cluster@cluster:cluster",
@@ -639,7 +639,7 @@ var (
 				Resource:     "namespaces",
 				Verbs:        []string{"delete"},
 			}},
-			Writes: []proxyrule.StringOrTemplate{{
+			Updates: []proxyrule.StringOrTemplate{{
 				Template: "namespace:{{name}}#creator@user:{{user.Name}}",
 			}, {
 				Template: "namespace:{{name}}#cluster@cluster:cluster",
@@ -669,7 +669,7 @@ var (
 			}},
 			PreFilters: []proxyrule.PreFilter{{
 				Name:       "resourceId",
-				ByResource: &proxyrule.StringOrTemplate{Template: "namespace:*#view@user:{{user.Name}}"},
+				ByResource: &proxyrule.StringOrTemplate{Template: "namespace:$resourceID#view@user:{{user.Name}}"},
 			}},
 		},
 	}
@@ -682,7 +682,7 @@ var (
 				Resource:     "pods",
 				Verbs:        []string{"create"},
 			}},
-			Writes: []proxyrule.StringOrTemplate{{
+			Updates: []proxyrule.StringOrTemplate{{
 				Template: "pod:{{namespacedName}}#creator@user:{{user.Name}}",
 			}, {
 				Template: "pod:{{name}}#namespace@namespace:{{namespace}}",
@@ -701,7 +701,7 @@ var (
 			Checks: []proxyrule.StringOrTemplate{{
 				Template: "pod:{{namespacedName}}#edit@user:{{user.Name}}",
 			}},
-			Writes: []proxyrule.StringOrTemplate{{
+			Updates: []proxyrule.StringOrTemplate{{
 				Template: "pod:{{namespacedName}}#creator@user:{{user.Name}}",
 			}, {
 				Template: "pod:{{name}}#namespace@namespace:{{namespace}}",
@@ -745,7 +745,7 @@ var (
 			PreFilters: []proxyrule.PreFilter{{
 				Namespace:  "splitNamespace(resourceId)",
 				Name:       "splitName(resourceId)",
-				ByResource: &proxyrule.StringOrTemplate{Template: "pod:*#view@user:{{user.Name}}"},
+				ByResource: &proxyrule.StringOrTemplate{Template: "pod:$resourceID#view@user:{{user.Name}}"},
 			}},
 		},
 	}

pkg/authz/distributedtx/workflow.go

@@ -2,10 +2,11 @@ package distributedtx
 
 import (
 	"fmt"
-	"k8s.io/klog/v2"
 	"net/http"
 	"time"
 
+	"k8s.io/klog/v2"
+
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -43,7 +44,7 @@ type WriteObjInput struct {
 	Header        http.Header
 	UserInfo      *user.DefaultInfo
 	ObjectMeta    *metav1.ObjectMeta
-	Rels          []*v1.Relationship
+	UpdateRels    []*v1.Relationship
 	Preconditions []*v1.Precondition
 	Body          []byte
 }
@@ -142,13 +143,14 @@ func PessimisticWriteToSpiceDBAndKube(ctx workflow.Context, input *WriteObjInput
 	}
 	preconditions = append(preconditions, input.Preconditions...)
 
+	rels := input.UpdateRels
 	operation := v1.RelationshipUpdate_OPERATION_TOUCH
 	if input.RequestInfo.Verb == "delete" {
 		operation = v1.RelationshipUpdate_OPERATION_DELETE
 	}
 
-	updates := make([]*v1.RelationshipUpdate, 0, len(input.Rels))
-	for _, r := range input.Rels {
+	updates := make([]*v1.RelationshipUpdate, 0, len(rels))
+	for _, r := range rels {
 		updates = append(updates, &v1.RelationshipUpdate{
 			Operation:    operation,
 			Relationship: r,
@@ -239,13 +241,14 @@ func OptimisticWriteToSpiceDBAndKube(ctx workflow.Context, input *WriteObjInput)
 	}
 
 	// TODO: this could optionally use dry-run to preflight the kube request
+	rels := input.UpdateRels
 	operation := v1.RelationshipUpdate_OPERATION_CREATE
 	if input.RequestInfo.Verb == "delete" {
 		operation = v1.RelationshipUpdate_OPERATION_DELETE
 	}
 
-	updates := make([]*v1.RelationshipUpdate, 0, len(input.Rels))
-	for _, r := range input.Rels {
+	updates := make([]*v1.RelationshipUpdate, 0, len(rels))
+	for _, r := range rels {
 		updates = append(updates, &v1.RelationshipUpdate{
 			Operation:    operation,
 			Relationship: r,

pkg/authz/distributedtx/workflow_test.go

@@ -70,7 +70,7 @@ func TestWorkflow(t *testing.T) {
 				RequestInfo: &request.RequestInfo{Verb: "create"},
 				UserInfo:    &user.DefaultInfo{Name: "janedoe"},
 				ObjectMeta:  &metav1.ObjectMeta{Name: "my_object_meta"},
-				Rels: []*v1.Relationship{{
+				UpdateRels: []*v1.Relationship{{
 					Resource: &v1.ObjectReference{
 						ObjectType: "namespace",
 						ObjectId:   "my_object_meta",

pkg/authz/filter.go

@@ -177,17 +177,18 @@ func filterWatch(ctx context.Context, client v1.PermissionsServiceClient, watchC
 			OptionalObjectTypes: []string{filter.Rel.ResourceType},
 		})
 		if err != nil {
-			fmt.Println(err)
+			klog.V(3).ErrorS(err, "error on filterWatch")
 			return
 		}
+
 		for {
 			resp, err := watchResource.Recv()
 			if errors.Is(err, io.EOF) {
 				break
 			}
 
 			if err != nil {
-				fmt.Println(err)
+				klog.V(3).ErrorS(err, "error on watchResource.Recv")
 				return
 			}
 
@@ -211,7 +212,7 @@ func filterWatch(ctx context.Context, client v1.PermissionsServiceClient, watchC
 					},
 				})
 				if err != nil {
-					fmt.Println(err)
+					klog.V(3).ErrorS(err, "error on CheckPermission")
 					return
 				}
 
@@ -225,32 +226,30 @@ func filterWatch(ctx context.Context, client v1.PermissionsServiceClient, watchC
 					fmt.Println(err)
 					return
 				}
-				fmt.Println(data)
-				fmt.Println("RESPONSE", string(byteIn))
 
 				name, err := filter.Name.Search(data)
 				if err != nil {
-					fmt.Println(err)
+					klog.V(3).ErrorS(err, "error on filter.Name.Search")
 					return
 				}
-				fmt.Println("GOT NAME", name)
+
 				if name == nil || len(name.(string)) == 0 {
 					return
 				}
+
 				namespace, err := filter.Namespace.Search(data)
 				if err != nil {
-					fmt.Println(err)
+					klog.V(3).ErrorS(err, "error on filter.Namespace.Search")
 					return
 				}
-				fmt.Println("GOT NAMESPACE", namespace)
 				if namespace == nil {
 					namespace = ""
 				}
+
 				nn := types.NamespacedName{Name: name.(string), Namespace: namespace.(string)}
 
 				// TODO: this should really be over a single channel to prevent
 				//  races on add/remove
-				fmt.Println(u.Relationship.Resource.ObjectId, cr.Permissionship)
 				if cr.Permissionship == v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION {
 					authzData.allowedNNC <- nn
 				} else {

pkg/authz/write.go

@@ -16,14 +16,14 @@ import (
 
 // write performs a dual write according to the passed rule
 func write(ctx context.Context, w http.ResponseWriter, r *rules.RunnableRule, input *rules.ResolveInput, workflowClient *client.Client) error {
-	writeRels := make([]*v1.Relationship, 0, len(r.Writes))
-	for _, write := range r.Writes {
+	updateRels := make([]*v1.Relationship, 0, len(r.Updates))
+	for _, write := range r.Updates {
 		write := write
 		rel, err := rules.ResolveRel(write, input)
 		if err != nil {
 			return fmt.Errorf("unable to resolve write rule (%v): %w", rel, err)
 		}
-		writeRels = append(writeRels, &v1.Relationship{
+		updateRels = append(updateRels, &v1.Relationship{
 			Resource: &v1.ObjectReference{
 				ObjectType: rel.ResourceType,
 				ObjectId:   rel.ResourceID,
@@ -59,7 +59,7 @@ func write(ctx context.Context, w http.ResponseWriter, r *rules.RunnableRule, in
 		preconditions = append(preconditions, p)
 	}
 
-	resp, err := dualWrite(ctx, workflowClient, input, writeRels, preconditions, r.LockMode)
+	resp, err := dualWrite(ctx, workflowClient, input, updateRels, preconditions, r.LockMode)
 	if err != nil {
 		return fmt.Errorf("dual write failed: %w", err)
 	}
@@ -82,7 +82,7 @@ func write(ctx context.Context, w http.ResponseWriter, r *rules.RunnableRule, in
 // getWriteRule returns the first matching rule with `write` defined
 func getWriteRule(matchingRules []*rules.RunnableRule) *rules.RunnableRule {
 	for _, r := range matchingRules {
-		if len(r.Writes) > 0 {
+		if len(r.Updates) > 0 {
 			// we can only do one dual-write per request without some way of
 			// marking some writes async.
 			return r
@@ -93,13 +93,13 @@ func getWriteRule(matchingRules []*rules.RunnableRule) *rules.RunnableRule {
 
 // dualWrite configures the dtx for writing to kube and spicedb and waits for
 // the response
-func dualWrite(ctx context.Context, workflowClient *client.Client, input *rules.ResolveInput, rels []*v1.Relationship, preconditions []*v1.Precondition, lockMode proxyrule.LockMode) (*distributedtx.KubeResp, error) {
+func dualWrite(ctx context.Context, workflowClient *client.Client, input *rules.ResolveInput, updateRels []*v1.Relationship, preconditions []*v1.Precondition, lockMode proxyrule.LockMode) (*distributedtx.KubeResp, error) {
 	writeInput := &distributedtx.WriteObjInput{
 		RequestInfo:   input.Request,
 		UserInfo:      input.User,
 		Body:          input.Body,
 		Header:        input.Headers,
-		Rels:          rels,
+		UpdateRels:    updateRels,
 		Preconditions: preconditions,
 	}
 	if input.Object != nil {
@@ -129,22 +129,22 @@ func preconditionFromRel(rel *rules.ResolvedRel) *v1.Precondition {
 			ResourceType: rel.ResourceType,
 		},
 	}
-	if rel.ResourceID != "*" {
+	if rel.ResourceID != "$resourceID" {
 		p.Filter.OptionalResourceId = rel.ResourceID
 	}
-	if rel.ResourceRelation != "*" {
+	if rel.ResourceRelation != "$resourceRelation" {
 		p.Filter.OptionalRelation = rel.ResourceRelation
 	}
-	if rel.SubjectType != "*" || rel.SubjectID != "*" || rel.SubjectRelation != "*" {
+	if rel.SubjectType != "$subjectType" || rel.SubjectID != "$subjectID" || rel.SubjectRelation != "$subjectRelation" {
 		p.Filter.OptionalSubjectFilter = &v1.SubjectFilter{}
 	}
-	if rel.SubjectType != "*" {
+	if rel.SubjectType != "$subjectType" {
 		p.Filter.OptionalSubjectFilter.SubjectType = rel.SubjectType
 	}
-	if rel.SubjectID != "*" {
+	if rel.SubjectID != "$subjectID" {
 		p.Filter.OptionalSubjectFilter.OptionalSubjectId = rel.SubjectID
 	}
-	if rel.SubjectRelation != "*" && rel.SubjectRelation != "" {
+	if rel.SubjectRelation != "$subjectRelation" && rel.SubjectRelation != "" {
 		p.Filter.OptionalSubjectFilter.OptionalRelation = &v1.SubjectFilter_RelationFilter{
 			Relation: rel.SubjectRelation,
 		}