feat: batch check relations #1521

patrickduffy95 · 2024-04-15T23:13:09Z

Related issue(s)

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

This change adds REST and gRPC endpoints for batch checking relations. The endpoint accepts a list of relation tuples to check, iterates through them (with concurrency), and returns a list of allowed responses.

REST API
New endpoint:
POST /relation-tuples/batch/check?max-depth=<depth>&parallelization-factor=<max-concurrent-requests>
Request body:

{
    "tuples": [
        {
            "namespace": <namespace>,
            "object": <object>,
            "relation": <relation>,
            "subject_id": <subject_id>,
            "subject_set": <subject_set>
        }
    ]
}

Response:

{
    "results": [
        {
            "allowed": true,
            "error": "an optional error message if the individual check fails"
        }
    ]
}

gRPC
New RPC:
CheckService/BatchCheck
Request:

// The request for a CheckService.BatchCheck RPC.
// Checks a batch of relations.
message BatchCheckRequest {
  repeated RelationTuple tuples = 1;

  // This field is not implemented yet and has no effect.
  bool latest = 2;
  // This field is not implemented yet and has no effect.
  string snaptoken = 3;
  // The maximum depth to search for a relation.
  //
  // If the value is less than 1 or greater than the global
  // max-depth then the global max-depth will be used instead.
  int32 max_depth = 4;
  // The number of check requests to perform in parallel.
  //
  // Will default to 5 if not provided. If provided, it must be
  // a positive integer
  optional int32 parallelization_factor = 5;
}

Response

// The response for a CheckService.BatchCheck rpc.
message BatchCheckResponse {
  // The results of the batch check. The order of these
  // results will match the order of the input.
  repeated CheckResponseWithError results = 1;
}

// The response for an individual check in the CheckService.BatchCheck rpc.
message CheckResponseWithError {
  // Whether the specified subject (id)
  // is related to the requested object.
  //
  // It is false by default if no ACL matches.
  bool allowed = 1;
  // If there was an error checking the tuple,
  // this will contain the error message.
  //
  // If the check was performed successfully, this will be empty.
  string error = 2;
  // This field is not implemented yet and has no effect.
  string snaptoken = 3;
}

Notes:

The maximum batch size is configurable based on a new limit.max_batch_check_size size with a default value of 10
The checks are executed individually. If any single check errors, the API will continue with the other checks. For the failed check, it returns not allowed along with an error message
The checks are executed in parallel. The max number of checks that can be executed in parallel is controlled by a "parallelization factor" that can optionally be included with each request. It defaults to 5.
I did not implement this for the CLI

CLAassistant · 2024-04-15T23:13:15Z

All committers have signed the CLA.

patrickduffy95 · 2024-04-25T18:27:17Z

@alnr would you be able to let me know if this PR is on the right track?

aeneasr · 2024-05-13T09:41:07Z

Hello, sorry for kot responding here. I think this feature is grand! @zepatrik and @hperl are finishing up some work on our end and can probably check out this PR in a week or two.

PrimeDominus · 2024-05-21T10:03:06Z

Thanks for implementing this. We've hit a roadblock and without this feature we have to write a bunch of custom code in our server which is getting cumbersome to maintain. Really looking forward to trying this.

patrickduffy95 · 2024-06-03T16:17:22Z

Hello, sorry for kot responding here. I think this feature is grand! @zepatrik and @hperl are finishing up some work on our end and can probably check out this PR in a week or two.

Thanks @aeneasr! Do you have a better idea now of when they might be able to take a look at it?

hperl

Thank you very much for your contribution! This looks very good already.

Besides the comments below, the main thing that is missing are some tests. I'd like to see tests for the happy path, going over the batch limit (bad request), graceful handling of on errornous tuple in the batch (should not fail the whole batch, but just that check), empty batch, and whatever edge case you can think of.

I left some comments below. As for your questions, here's my take on it:

Should we limit the number of tuples this batch API accepts?

Yes, please make it configurable with a conservative default (10?)

Should we parallelize the check requests?

Yes, I'd make the number of concurrent checks another parameter.

Is implementing only the POST okay for the REST API?

Yes, that will suffice.

hperl · 2024-06-04T08:41:45Z

internal/check/handler.go

@@ -49,13 +49,15 @@ func NewHandler(d handlerDependencies) *Handler {
 const (
 	RouteBase        = "/relation-tuples/check"
 	OpenAPIRouteBase = RouteBase + "/openapi"
+	BatchRoute       = "/relation-tuples/batchCheck"


How about /relation-tuples/batch/check? Then we can have other batch ops in the future.

hperl · 2024-06-04T08:53:18Z

internal/check/handler.go

+		}
+		allowed, err := h.d.PermissionEngine().CheckIsMember(ctx, t[0], maxDepth)
+		if err != nil {
+			return nil, err


Should we annotate here which tuple caused the error? It might be frustrating to submit a batch of checks and get an error and then having to binary-search which tuple caused the error.

Alternatively we could just add the error to the results list and then commence with the next tuple. WDYT?

Adding more context to the error is a good idea. The question then is whether to return errors at the tuple level vs for the API as a whole. If there are situations where clients might expect an error and would want to treat an error the same as receiving a false allowed value, I could see a benefit in continuing on error and returning for each tuple if an error was generated. As far as I know though, that isn't a normal case, so I will just add tuple details to the error message.

Changed my mind and went for proceeding with checking the whole batch and adding the error to the response for each tuple

hperl · 2024-06-04T09:10:44Z

internal/check/handler.go

+
+func (h *Handler) BatchCheck(ctx context.Context, req *rts.BatchCheckRequest) (*rts.BatchCheckResponse, error) {
+	responses := make([]*rts.CheckResponse, len(req.Tuples))
+	for i, reqTuple := range req.Tuples {


The logic is duplicated from the REST handler here. Ideally we'd implement this once and have gRPC and REST call that function.

patrickduffy95 · 2024-06-05T05:44:33Z

Thanks a lot for the review @hperl. I will let you know once this PR is updated.

…able

cmd/status/root_test.go

patrickduffy95 · 2024-06-13T21:50:51Z

@hperl I have update the PR based on your feedback and it is now ready for review.

PrimeDominus · 2024-07-08T06:37:18Z

I'm really looking forward to this functionality becoming available on Ory Network. @hperl is there anything else that needs to be done before this can be merged?

hperl

Thank you very much for your changes! This looks very good already. I just added some minor comments to imrpove this further.

internal/check/handler.go

internal/check/engine.go

internal/check/handler.go

hperl

This looks perfect now! Thanks for all the good work! 🎉

rbnbr · 2024-11-03T22:06:35Z

Hey, thank you for the great feature!

Now with this being merged in the master branch, could we also extend the Rest HTTP API (documentation) accordingly (https://www.ory.sh/docs/keto/reference/rest-api) and also make it available to the keto-client(s), especially for go (https://github.com/ory/keto-client-go)?

Thank you!

ryukinix · 2024-11-19T03:01:05Z

Very interesting! We were waiting for this since 2022. Awesome work!

jcottongit · 2024-12-07T08:47:04Z

@hperl Cheers for getting this in, can we get a docker image released with this feature included?

patrickduffy95 changed the title ~~Batch check relations~~ feat: Batch check relations Apr 15, 2024

batch check relations

a3f2113

patrickduffy95 force-pushed the batchCheck branch from d81f574 to a3f2113 Compare April 15, 2024 23:15

patrickduffy95 changed the title ~~feat: Batch check relations~~ feat: batch check relations Apr 15, 2024

Merge branch 'master' into batchCheck

1d5b2b8

hperl requested changes Jun 4, 2024

View reviewed changes

patrickduffy95 and others added 9 commits June 10, 2024 16:41

Merge branch 'master' into batchCheck

488efa5

rename path

3766fd3

shared parallelized function. batch size and parallelization configur…

f3791df

…able

move check to engine

9680c6c

fail individual requests

e06bbde

move parallelization factor to be request parameter

982cac8

document and update configurable max batch size

5060f84

end to end tests

b05176b

unit tests

8cbffee

patrickduffy95 commented Jun 13, 2024

View reviewed changes

cmd/status/root_test.go Show resolved Hide resolved

patrickduffy95 added 3 commits June 13, 2024 11:26

cleanup

60eeb08

run make format

20247ae

fix pipeline failures

a16fef5

patrickduffy95 marked this pull request as ready for review June 13, 2024 18:48

patrickduffy95 requested review from zepatrik and alnr as code owners June 13, 2024 18:48

Merge branch 'master' into batchCheck

50efbca

hperl requested changes Jul 17, 2024

View reviewed changes

internal/check/handler.go Outdated Show resolved Hide resolved

internal/check/engine.go Outdated Show resolved Hide resolved

internal/check/handler.go Outdated Show resolved Hide resolved

PR Feedback: move parallelization factor to config. Use err group

42639c0

hperl approved these changes Jul 18, 2024

View reviewed changes

hperl added this pull request to the merge queue Jul 18, 2024

Merged via the queue into ory:master with commit d670d50 Jul 18, 2024
22 checks passed

alnr mentioned this pull request Nov 18, 2024

Batch check request #812

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: batch check relations #1521

feat: batch check relations #1521

patrickduffy95 commented Apr 15, 2024 •

edited

Loading

CLAassistant commented Apr 15, 2024 •

edited

Loading

patrickduffy95 commented Apr 25, 2024

aeneasr commented May 13, 2024

PrimeDominus commented May 21, 2024

patrickduffy95 commented Jun 3, 2024

hperl left a comment

hperl Jun 4, 2024

hperl Jun 4, 2024

patrickduffy95 Jun 11, 2024

patrickduffy95 Jun 13, 2024

hperl Jun 4, 2024

patrickduffy95 commented Jun 5, 2024

patrickduffy95 commented Jun 13, 2024

PrimeDominus commented Jul 8, 2024 •

edited

Loading

hperl left a comment

hperl left a comment

rbnbr commented Nov 3, 2024 •

edited

Loading

ryukinix commented Nov 19, 2024

jcottongit commented Dec 7, 2024

feat: batch check relations #1521

feat: batch check relations #1521

Conversation

patrickduffy95 commented Apr 15, 2024 • edited Loading

Related issue(s)

Checklist

Further Comments

CLAassistant commented Apr 15, 2024 • edited Loading

patrickduffy95 commented Apr 25, 2024

aeneasr commented May 13, 2024

PrimeDominus commented May 21, 2024

patrickduffy95 commented Jun 3, 2024

hperl left a comment

Choose a reason for hiding this comment

hperl Jun 4, 2024

Choose a reason for hiding this comment

hperl Jun 4, 2024

Choose a reason for hiding this comment

patrickduffy95 Jun 11, 2024

Choose a reason for hiding this comment

patrickduffy95 Jun 13, 2024

Choose a reason for hiding this comment

hperl Jun 4, 2024

Choose a reason for hiding this comment

patrickduffy95 commented Jun 5, 2024

patrickduffy95 commented Jun 13, 2024

PrimeDominus commented Jul 8, 2024 • edited Loading

hperl left a comment

Choose a reason for hiding this comment

hperl left a comment

Choose a reason for hiding this comment

rbnbr commented Nov 3, 2024 • edited Loading

ryukinix commented Nov 19, 2024

jcottongit commented Dec 7, 2024

patrickduffy95 commented Apr 15, 2024 •

edited

Loading

CLAassistant commented Apr 15, 2024 •

edited

Loading

PrimeDominus commented Jul 8, 2024 •

edited

Loading

rbnbr commented Nov 3, 2024 •

edited

Loading