Support bulk checks #146

mikemrm · 2023-07-24T17:28:31Z

This adds support for requesting access to multiple resources and actions.

jnschaeffer · 2023-07-25T18:15:02Z

internal/api/permissions.go

+type checkStatus struct {
+	Resource types.Resource
+	Action   string
+	Error    error
+}


I think I'd break this up into two structs: A checkRequest containing the resource/action pair and a checkResult containing the checkRequest object plus an error.

jnschaeffer · 2023-07-25T18:23:10Z

internal/api/permissions.go

+		return echo.NewHTTPError(http.StatusBadRequest, "invalid check request").SetInternal(multierr.Combine(errs...))
+	}
+
+	checkCh := make(chan *checkStatus)


So looking at this code, a channel of pointers seems risky. IMO we have two options here:

Spin up goroutines that receive off a <-chan checkRequest (see above) and send to a chan<- checkResponse

Use sync.WaitGroup and coordinate writes to a slice

The first option is more idiomatic Go, and I think is generally more robust against introducing data races. I'd recommend we move towards that approach. With that in mind, I'd recommend we make this a buffered channel, like make(chan checkResponse, len(reqBody.Actions)) and pre-populate the channel and close it before doing any work. That way we have a queue already primed for workers to consume from and don't have to do further coordination of work.

makes sense, updated.

jnschaeffer · 2023-07-25T18:27:18Z

internal/api/permissions.go

+
+	checkCh := make(chan *checkStatus)
+
+	wg := new(sync.WaitGroup)


Rather than use a sync.WaitGroup here, we can instead provide a channel for workers to send results to and iterate over that until we get back as many results as we expect, then close the channel to free resources.

Additionally, we can set up a new context using context.WithCancelCause, then when receiving results cancel the entire operation if we get any errors. SpiceDB seems pretty good at handling context propagation and terminating operations early so we can have things fail faster that way.

Removed WaitGroup and handle waiting with channels.

jnschaeffer · 2023-07-25T18:29:11Z

internal/api/permissions.go

+	case <-time.After(maxCheckDuration):
+		return echo.NewHTTPError(http.StatusInternalServerError, "checks didn't complete in time")


Setting up a context using context.WithTimeout is more idiomatic here and lets us handle all errors in one place.

Good point, updated.

Signed-off-by: Mike Mason <[email protected]>

jnschaeffer

Looking good - more thoughts.

jnschaeffer · 2023-07-26T13:51:48Z

internal/api/permissions.go

+			for check := range requestsCh {
+				result := &checkResult{
+					Request: check,
+				}
+
+				// Check the permissions
+				err := r.engine.SubjectHasPermission(ctx, subjectResource, check.Action, check.Resource)
+				if err != nil {
+					result.Error = err
+				}
+
+				// Check if doneCh has been closed, if so, don't write to resultsCh.
+				select {
+				case <-doneCh:
+					return
+				default:
+				}
+
+				resultsCh <- *result
+			}


We can probably write this using the context's Done channel. Something like this will work:

for { select { case check := <-requestsCh: // do the thing case <-ctx.Done(): result.Error = ctx.Err() } }

This lets us collect all errors instead of terminating early, which we can use as shown below.

ah yeah, took me a minute but I see what you mean. Thanks!

jnschaeffer · 2023-07-26T13:54:07Z

internal/api/permissions.go

+				result := &checkResult{
+					Request: check,
+				}


Seems like there's not a compelling reason for us to do &checkResult here since we dereference the value at the end of the loop body.

ah good call, fixed!

jnschaeffer · 2023-07-26T14:00:20Z

internal/api/permissions.go

+	go func() {
+		var count int
+
+		for result := range resultsCh {
+			count++
+
+			if result.Error != nil {
+				if errors.Is(result.Error, query.ErrActionNotAssigned) {
+					err := fmt.Errorf("%w: subject '%s' does not have permission to perform action '%s' on resource '%s'",
+						ErrAccessDenied, subject, result.Request.Action, result.Request.Resource.ID.String())
+
+					unauthorizedErrors++
+
+					allErrors = append(allErrors, err)
+				} else {
+					err := fmt.Errorf("check %d: %w", result.Request.Index, result.Error)
+
+					internalErrors++
+
+					allErrors = append(allErrors, err)
+				}
+
+				close(doneCh)
+				close(resultsCh)
+
+				return
+			}
+
+			if count == len(reqBody.Actions) {
+				close(doneCh)
+				close(resultsCh)
+			}
+		}
+	}()


Rather than ranging over the channel, if we know how many results we expect back we should be able to just read those. Something like this:

for i := 0; i < numChecks; i++ { select { case result := <-resultsCh: // check for errors case <-ctx.Done(): // context error handling } }

Doing this would let us take out doneCh completely and remove the need to process results in a separate goroutine from the rest of the handler.

ah yes, i see what you mean. it will just populate the errors with the same context error for all the ones which were cancelled. makes sense!

- Splits up the Request and Results - Switch to using context.WithTimeout instead of time.After to ensure context is cancelled - Replaces WaitGroup with looping through the known count and logging all errors Signed-off-by: Mike Mason <[email protected]>

jnschaeffer

Looks good! Two small things - neither are blocking.

jnschaeffer · 2023-07-26T14:46:11Z

internal/api/permissions.go

+					// if channel is closed, quit the go routine.
+					if !ok {
+						return
+					}


This makes sense but I'm not quite sure if it's necessary since we call defer cancel() above. Could be wrong though.

It is required, as we close the requests channel, which results in selecting the case but with the ok set to false. which resulted in empty results (default checkRequest value) being checked.

jnschaeffer · 2023-07-26T14:51:24Z

internal/api/permissions.go

+	if internalErrors != 0 {
+		return echo.NewHTTPError(http.StatusInternalServerError, "an error occurred checking permissions").SetInternal(multierr.Combine(allErrors...))
+	}
+
+	if unauthorizedErrors != 0 {
+		msg := fmt.Sprintf("subject '%s' does not have permission to the requested resource actions", subject)
+
+		return echo.NewHTTPError(http.StatusForbidden, msg).SetInternal(multierr.Combine(allErrors...))
+	}


I'm not sure if we need/want to also call RecordError or SetStatus here: https://pkg.go.dev/go.opentelemetry.io/otel/trace#Span

You'd need to check what the spans currently look like for that. Not a blocker though.

will track it and dig into it.

mikemrm force-pushed the support-bulk-checks branch from e8a4aaf to 6c4ed18 Compare July 24, 2023 17:29

mikemrm marked this pull request as ready for review July 24, 2023 17:51

mikemrm requested review from a team as code owners July 24, 2023 17:51

jnschaeffer requested changes Jul 25, 2023

View reviewed changes

mikemrm added 4 commits July 25, 2023 18:40

add support for bulk permission check requests

400833b

Signed-off-by: Mike Mason <[email protected]>

split out checker code

ade6fc1

Signed-off-by: Mike Mason <[email protected]>

add bulk permission checks to client

030f958

Signed-off-by: Mike Mason <[email protected]>

correct lint issues

aefb5ec

Signed-off-by: Mike Mason <[email protected]>

mikemrm force-pushed the support-bulk-checks branch 2 times, most recently from 8ef3f26 to 511e91e Compare July 25, 2023 20:44

mikemrm requested a review from jnschaeffer July 25, 2023 20:50

jnschaeffer reviewed Jul 26, 2023

View reviewed changes

mikemrm force-pushed the support-bulk-checks branch from 511e91e to 55fef0c Compare July 26, 2023 14:37

implement review suggestions

db7b681

- Splits up the Request and Results - Switch to using context.WithTimeout instead of time.After to ensure context is cancelled - Replaces WaitGroup with looping through the known count and logging all errors Signed-off-by: Mike Mason <[email protected]>

mikemrm force-pushed the support-bulk-checks branch from 55fef0c to db7b681 Compare July 26, 2023 14:39

mikemrm requested a review from jnschaeffer July 26, 2023 14:44

jnschaeffer approved these changes Jul 26, 2023

View reviewed changes

mikemrm merged commit 53a5013 into infratographer:main Jul 26, 2023

mikemrm deleted the support-bulk-checks branch July 26, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bulk checks #146

Support bulk checks #146

mikemrm commented Jul 24, 2023

jnschaeffer Jul 25, 2023

mikemrm Jul 25, 2023

jnschaeffer Jul 25, 2023

mikemrm Jul 25, 2023

jnschaeffer Jul 25, 2023

mikemrm Jul 25, 2023

jnschaeffer Jul 25, 2023

mikemrm Jul 25, 2023

jnschaeffer left a comment

jnschaeffer Jul 26, 2023

mikemrm Jul 26, 2023

jnschaeffer Jul 26, 2023

mikemrm Jul 26, 2023

jnschaeffer Jul 26, 2023

mikemrm Jul 26, 2023

jnschaeffer left a comment

jnschaeffer Jul 26, 2023

mikemrm Jul 26, 2023

jnschaeffer Jul 26, 2023

mikemrm Jul 26, 2023

		case <-time.After(maxCheckDuration):
		return echo.NewHTTPError(http.StatusInternalServerError, "checks didn't complete in time")

Support bulk checks #146

Support bulk checks #146

Conversation

mikemrm commented Jul 24, 2023

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

jnschaeffer 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

jnschaeffer 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