Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 6901: set invalid request status if request parsing fails #573

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Issue 6901: set invalid request status if request parsing fails #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32b691e
set invalid request status if request parsing fails
rudrakhp Sep 11, 2024
862ffe9
Merge branch 'main' into handle_bad_requests
rudrakhp Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,6 @@ func (e *Error) Unwrap() error {
return e.err
}

func internalError(code string, err error) Error {
return Error{Code: code, err: err}
func newInternalError(code string, err error) *Error {
return &Error{Code: code, err: err}
}
72 changes: 43 additions & 29 deletions internal/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,22 +370,22 @@ func (p *envoyExtAuthzGrpcServer) Check(ctx context.Context, req *ext_authz_v3.C
func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*ext_authz_v3.CheckResponse, func() *rpc_status.Status, *Error) {
var err error
var evalErr error
var internalErr Error
var internalErr *Error
start := time.Now()
logger := p.manager.Logger()

result, stopeval, err := envoyauth.NewEvalResult()
if err != nil {
logger.WithFields(map[string]interface{}{"err": err}).Error("Unable to start new evaluation.")
internalErr = internalError(StartCheckErr, err)
return nil, func() *rpc_status.Status { return nil }, &internalErr
internalErr = newInternalError(StartCheckErr, err)
return nil, func() *rpc_status.Status { return nil }, internalErr
}

txn, txnClose, err := result.GetTxn(ctx, p.Store())
if err != nil {
logger.WithFields(map[string]interface{}{"err": err}).Error("Unable to start new storage transaction.")
internalErr = internalError(StartTxnErr, err)
return nil, func() *rpc_status.Status { return nil }, &internalErr
internalErr = newInternalError(StartTxnErr, err)
return nil, func() *rpc_status.Status { return nil }, internalErr
}

result.Txn = txn
Expand All @@ -398,9 +398,9 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
stopeval()
if p.cfg.EnablePerformanceMetrics {
var topdownError *topdown.Error
if internalErr.Unwrap() != nil && errors.As(internalErr.Unwrap(), &topdownError) {
if internalErr != nil && errors.As(internalErr.Unwrap(), &topdownError) {
p.metricErrorCounter.With(prometheus.Labels{"reason": topdownError.Code}).Inc()
} else if internalErr.Code != "" {
} else if internalErr != nil && internalErr.Code != "" {
p.metricErrorCounter.With(prometheus.Labels{"reason": internalErr.Code}).Inc()
}
}
Expand All @@ -422,27 +422,41 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*

input, err = envoyauth.RequestToInput(req, logger, p.cfg.protoSet, p.cfg.SkipRequestBodyParse)
if err != nil {
internalErr = internalError(RequestParseErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(RequestParseErr, err)
return &ext_authz_v3.CheckResponse{
Status: &rpc_status.Status{
Code: int32(code.Code_PERMISSION_DENIED),
Message: internalErr.Error(),
},
HttpResponse: &ext_authz_v3.CheckResponse_DeniedResponse{
DeniedResponse: &ext_authz_v3.DeniedHttpResponse{
Status: &ext_type_v3.HttpStatus{
Code: ext_type_v3.StatusCode(ext_type_v3.StatusCode_BadRequest),
},
Body: internalErr.Error(),
},
},
DynamicMetadata: nil,
}, stop, nil
}

if ctx.Err() != nil {
err = errors.Wrap(ctx.Err(), "check request timed out before query execution")
internalErr = internalError(CheckRequestTimeoutErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(CheckRequestTimeoutErr, err)
return nil, stop, internalErr
}

var inputValue ast.Value
inputValue, err = ast.InterfaceToValue(input)
if err != nil {
internalErr = internalError(InputParseErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(InputParseErr, err)
return nil, stop, internalErr
}

if err = envoyauth.Eval(ctx, p, inputValue, result); err != nil {
evalErr = err
internalErr = internalError(EnvoyAuthEvalErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthEvalErr, err)
return nil, stop, internalErr
}

resp := &ext_authz_v3.CheckResponse{}
Expand All @@ -451,8 +465,8 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
allowed, err = result.IsAllowed()
if err != nil {
err = errors.Wrap(err, "failed to get response status")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

status := int32(code.Code_PERMISSION_DENIED)
Expand All @@ -467,16 +481,16 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
responseHeaders, err = result.GetResponseEnvoyHeaderValueOptions()
if err != nil {
err = errors.Wrap(err, "failed to get response headers")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

var dynamicMetadata *_structpb.Struct
dynamicMetadata, err = result.GetDynamicMetadata()
if err != nil {
err = errors.Wrap(err, "failed to get dynamic metadata")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}
resp.DynamicMetadata = dynamicMetadata

Expand All @@ -486,16 +500,16 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
headersToRemove, err = result.GetRequestHTTPHeadersToRemove()
if err != nil {
err = errors.Wrap(err, "failed to get request headers to remove")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

var responseHeadersToAdd []*ext_core_v3.HeaderValueOption
responseHeadersToAdd, err = result.GetResponseHTTPHeadersToAdd()
if err != nil {
err = errors.Wrap(err, "failed to get response headers to send to client")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

resp.HttpResponse = &ext_authz_v3.CheckResponse_OkResponse{
Expand All @@ -510,16 +524,16 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
body, err = result.GetResponseBody()
if err != nil {
err = errors.Wrap(err, "failed to get response body")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

var httpStatus *ext_type_v3.HttpStatus
httpStatus, err = result.GetResponseEnvoyHTTPStatus()
if err != nil {
err = errors.Wrap(err, "failed to get response http status")
internalErr = internalError(EnvoyAuthResultErr, err)
return nil, stop, &internalErr
internalErr = newInternalError(EnvoyAuthResultErr, err)
return nil, stop, internalErr
}

deniedResponse := &ext_authz_v3.DeniedHttpResponse{
Expand Down
34 changes: 26 additions & 8 deletions internal/internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"context"
"errors"
"fmt"
ext_type_v2 "github.com/envoyproxy/go-control-plane/envoy/type"
ext_type_v3 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
"net/http"
"net/http/httptest"
"reflect"
Expand Down Expand Up @@ -862,12 +864,20 @@ func TestCheckBadDecisionWithLogger(t *testing.T) {
ctx := context.Background()
output, err := server.Check(ctx, &req)

if err == nil {
t.Fatal("Expected error but got nil")
if err != nil {
t.Fatal("Expected nil err")
}

if output != nil {
t.Fatalf("Expected no output but got %v", output)
if output == nil {
t.Fatal("Expected output but got nil")
}
if output.Status.Code != int32(code.Code_PERMISSION_DENIED) {
t.Fatalf("Expected status %v status: %v", int32(code.Code_PERMISSION_DENIED), output.Status.Code)
}
if deniedResponse, ok := output.HttpResponse.(*ext_authz.CheckResponse_DeniedResponse); !ok {
t.Fatalf("Expected http response of type ext_authz.CheckResponse_DeniedResponse")
} else if deniedResponse.DeniedResponse.Status.Code != ext_type_v3.StatusCode_BadRequest {
t.Fatalf("Unexpected http status code: %v", deniedResponse.DeniedResponse.Status.Code)
}

if len(customLogger.events) != 1 {
Expand Down Expand Up @@ -1056,12 +1066,20 @@ func TestCheckBadDecisionWithLoggerV2(t *testing.T) {
ctx := context.Background()
output, err := server.Check(ctx, &req)

if err == nil {
t.Fatal("Expected error but got nil")
if err != nil {
t.Fatal("Expected nil err")
}

if output != nil {
t.Fatalf("Expected no output but got %v", output)
if output == nil {
t.Fatal("Expected output but got nil")
}
if output.Status.Code != int32(code.Code_PERMISSION_DENIED) {
t.Fatalf("Expected status %v status: %v", int32(code.Code_PERMISSION_DENIED), output.Status.Code)
}
if deniedResponse, ok := output.HttpResponse.(*ext_authz_v2.CheckResponse_DeniedResponse); !ok {
t.Fatalf("Expected http response of type ext_authz.CheckResponse_DeniedResponse")
} else if deniedResponse.DeniedResponse.Status.Code != ext_type_v2.StatusCode_BadRequest {
t.Fatalf("Unexpected http status code: %v", deniedResponse.DeniedResponse.Status.Code)
}

if len(customLogger.events) != 1 {
Expand Down