Skip to content

policy: add direct response#765

Merged
istio-testing merged 6 commits intoistio:release-1.1from
kyessenov:direct_response
Jan 22, 2019
Merged

policy: add direct response#765
istio-testing merged 6 commits intoistio:release-1.1from
kyessenov:direct_response

Conversation

@kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Jan 18, 2019

Add an error detail for gRPC status message that provides direct HTTP response code and message.
This is meant to be used for:

  • external authorization customizing the response
  • Mixer-driven redirect

The error detail should be supplied as part of a regular adapter CheckResult gRPC status.
Mixer will recognize it and translate to a route directive accordingly.

Since there is a need for a direct response in case of check rejection, also relaxing the route directive condition to be applied in case of an unsuccessful check (requires a minor proxy change). That means in case of rejection, we can use direct response for HTTP instead of always translating gRPC code and sending gRPC message.

cc @linsun @geeknoid @douglas-reid @mandarjog

Signed-off-by: Kuat Yessenov kuat@google.com

Signed-off-by: Kuat Yessenov <kuat@google.com>
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 18, 2019
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov kyessenov requested review from geeknoid and removed request for costinm and hklai January 18, 2019 04:58
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
// Direct HTTP response for a client-facing error message which can be attached
// to an RPC error.
message DirectHttpResponse {
// Optional HTTP status code. If not set, RPC error code is used.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about HTTP headers? Many of the HTTP statuses require additional information to be returned in HTTP headers. e.g.

  • HTTP 30x redirects require Location header to be present in the response
  • HTTP 40x might require Www-Authenticate header to be present in the response

Above are just a couple of examples, for proper flexibility it should be possible to control response headers content similar to the status code and body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but there is already a way to control headers here:
https://github.com/istio/api/blob/master/policy/v1beta1/cfg.proto#L175

The plan is to apply response header additions from the rules to direct responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a bit more, and I think it's better to supply headers here directly instead of relying on another header-manipulating feature. I'll make a follow-up.

@geeknoid
Copy link
Contributor

So if I'm following this correctly, the new proto would be returned by an adapter's CheckXXX call, correct?

@geeknoid
Copy link
Contributor

I think this needs to be into master, it shouldn't be part of 1.1

@kyessenov
Copy link
Contributor Author

@geeknoid The new proto is going to be packaged as Any and supplied within the standard google.rpc.Status. This is a standard policy for client-facing errors. gRPC errors are not meant to escape to the end-users.

Re: versioning: This is a customer request for 1.1. I'm open to a simpler solution, but this is the least invasive solution I can come up with. cc @duderino

@geeknoid
Copy link
Contributor

We're in lock down mode at the moment. We don't want new features in 1.1.

This can catch the next train.

@mandarjog
Copy link
Contributor

/lgtm

@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyessenov, mandarjog

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [kyessenov,mandarjog]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. review/done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants