allow role resolution errors in RARs

[deads2k]

Bug https://bugzilla.redhat.com/show_bug.cgi?id=1192310.

This allows role and rolebinding resolution problems during an RAR without failures. This is ok because we never return too broad a list.

[smarterclayton]

You need to define partial. Is partial "if I called this and I expected to get the full list, but I didn't, I can safely continue?" Or is partial "here's as much of the results as we can show"? If it's the latter, this needs to be a struct or something that conveys more data about it being a partial response (for one thing, people shouldn't be doing if len(e.EvaluationError) == 0 { // happy path })

[deads2k]

You need to define partial. Is partial "if I called this and I expected to get the full list, but I didn't, I can safely continue?" Or is partial "here's as much of the results as we can show"?

The choice of what to do belongs to the caller. In some cases, the caller may decide that incomplete results are worthless and their work should fail. In other cases (project cache as a for instance), the caller may decide that partial results are better than no results and use them.

[smarterclayton]

What use case for partial can you describe that needs this? I don't like
adding partial results to the API unless the user requests it, and even
then I'm not sure what scenario you want to improve by doing this.

On Sep 26, 2015, at 8:16 AM, David Eads notifications@github.com wrote:

You need to define partial. Is partial "if I called this and I expected to
get the full list, but I didn't, I can safely continue?" Or is partial
"here's as much of the results as we can show"?

The choice of what to do belongs to the caller. In some cases, the caller
may decide that incomplete results are worthless and their work should
fail. In other cases (project cache as a for instance), the caller may
decide that partial results are better than no results and use them.

—
Reply to this email directly or view it on GitHub
#4809 (comment).

[liggitt]

I think when determining which subjects have access to a project, if there is a role binding to an invalid role, we currently return an error rather than the subjects we could resolve. That makes project acl vulnerable to getting overly restrictive if a single role binding is invalid

[smarterclayton]

Ok, that's an excellent reason. So the next question is - if multiple
errors are possible, would the person setting this up need a structured
list of errors for rectification in the future? A single error is only
going to be readable to humans, and it's extremely likely the client doing
so isn't a human agent (doing the SAR). Who is going to use this error?
Should it be machine readable? Should it look like the details from
status?

On Sat, Sep 26, 2015 at 11:31 AM, Jordan Liggitt notifications@github.com
wrote:

I think when determining which subjects have access to a project, if there
is a role binding to an invalid role, we currently return an error rather
than the subjects we could resolve. That makes project acl vulnerable to
getting overly restrictive if a single role binding is invalid

—
Reply to this email directly or view it on GitHub
#4809 (comment).

[deads2k]

Ok, that's an excellent reason. So the next question is - if multiple
errors are possible, would the person setting this up need a structured
list of errors for rectification in the future? A single error is only
going to be readable to humans, and it's extremely likely the client doing
so isn't a human agent (doing the SAR). Who is going to use this error?
Should it be machine readable? Should it look like the details from
status?

This is an RAR (SAR is a clearer answer: "you can or can't"). I don't think that the structure of a []api.Status is significantly more useful than "some error occurred". I think there is value in a machine knowing that the results are partial and here's a string explaining why, but I don't think its likely that different classes of evaluation errors will be handled differently in code. It's not something I'd plan to do.

[smarterclayton]

So you're building a UI that lets someone change roles. You make a rar as
part of a "who can do this". In the process, you discover there are bad
roles in the project. As someone building that UI, you don't want
structured errors you can describe to an end user (who may have permissions
to fix them). The message, by itself, is a jumbled glob of text, and
probably is not user friendly (esp. if it's an aggregate error). If
there's value just to a machine, why not just a Boolean? if there's value
to a human, why not a list of errors?

This is an unusual enough concept to be adding to the API (which we have no
precedent to model) that I don't want to just add it without a clear
understanding of what it should be. I generally prefer structured errors
to opaque string blobs. What's the minimal structured error that conveys
value to a naive machine client, a human, and someone building a roles UI?

On Sep 27, 2015, at 11:15 AM, David Eads notifications@github.com wrote:

Ok, that's an excellent reason. So the next question is - if multiple
errors are possible, would the person setting this up need a structured
list of errors for rectification in the future? A single error is only
going to be readable to humans, and it's extremely likely the client doing
so isn't a human agent (doing the SAR). Who is going to use this error?
Should it be machine readable? Should it look like the details from
status?

This is an RAR (SAR is a clearer answer: "you can or can't"). I don't think
that the structure of a []api.Status is significantly more useful than
"some error occurred". I think there is value in a machine knowing that the
results are partial and here's a string explaining why, but I don't think
its likely that different classes of evaluation errors will be handled
differently in code. It's not something I'd plan to do.

—
Reply to this email directly or view it on GitHub
#4809 (comment).

[deads2k]

At a certain point you're arguing for markers. I'd claim that markers more appropriately belong in some sort of status command. I could even envision an endpoint for "give me markers", but I don't see that as the job of this error. I'm ok with a boolean in this case, but I thought a potentially human readable was a little cleaner. I might even go as far as a slice of strings, but plumbing something like []api.StatusError through the authorizer (that's where these originate) is improper.

[liggitt]

I'm in favor of tolerating lookup errors and returning a (potentially) partial list… the trouble is, the lookup has no way of knowing if the list is actually partial or not. Not sure that a field on the RAR is the right place to surface that info

[smarterclayton]

Explain why it's improper to return structured errors? I'm not necessarily
sold on a very detailed response, but - as an example - returning a
optional partialResults field (that might be nil) that contains a list of
structs with a single "Message" (or whatever we call a human readable
string) would satisfy me. You could even return an aggregate through the
authorizer along with your results, as long as you document that the
authorizer can returns errors and valid responses.

On Sep 28, 2015, at 1:16 AM, David Eads notifications@github.com wrote:

At a certain point you're arguing for markers. I'd claim that markers more
appropriately belong in some sort of status command. I could even envision
an endpoint for "give me markers", but I don't see that as the job of this
error. I'm ok with a boolean in this case, but I thought a potentially
human readable was a little cleaner. I might even go as far as a slice of
strings, but plumbing something like []api.StatusError through the
authorizer (that's where these originate) is improper.

—
Reply to this email directly or view it on GitHub
#4809 (comment).

[deads2k]

I don't think that this code here should return back a []StatusError.

[deads2k]

I don't think that this code here should return back a []StatusError.

Would []error be enough structure for you?

[smarterclayton]

No, it should just return error. But it should return an error that either satisfies Aggregate interface (sufficient for now) or we create a specific type of error that callers can check against. Returning an aggregate and not changing the signature is fine.

[deads2k]

No, it should just return error. But it should return an error that either satisfies Aggregate interface (sufficient for now) or we create a specific type of error that callers can check against. Returning an aggregate and not changing the signature is fine.

Ok, I think that's what I did: https://github.com/openshift/origin/pull/4809/files#diff-5654e020d459e05c8fcca57d5675ece4R87 .

[liggitt]

I'm fine with everything except the evaluation error field on the response

[deads2k]

I'm fine with everything except the evaluation error field on the response

Ok. How would you indicate the partial failure to clients? I'd consider the string to be better than a bool.

[liggitt]

we don't know whether it's a partial failure... that's the point. the roles that couldn't be resolved may have nothing to do with the resource in question. flagging role binding reference errors doesn't feel like it should be part of this response

[liggitt]

if you're expecting a particular user/group to come back in that list, and they don't, I think there should be a more focused way to look at the rolebindings for that user/group, at which point any unresolved references would make more sense

[deads2k]

if you're expecting a particular user/group to come back in that list, and they don't, I think there should be a more focused way to look at the rolebindings for that user/group, at which point any unresolved references would make more sense

I don't disagree that should exist (probably oc status if you have view rights to the necessary resources), but given that these evaluation errors might have affected your results (we can't know for sure) and almost certainly indicate a failure of intent, it seems like a good idea to indicate that.

I'll split into a separate pull to at least make us more tolerant for now.

[deads2k]

Split #4847 to allow progress to be made while we decide what is and isn't user intent.

[liggitt]

LGTM

[deads2k]

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3430/) (Image: devenv-fedora_2427)

[openshift-bot]

[Test]ing while waiting on the merge queue

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5248/)

[openshift-bot]

Evaluated for origin test up to e5214ae

[openshift-bot]

Evaluated for origin merge up to e5214ae