feat(context): add response.grpc_status by douglas-reid · Pull Request #9693 · envoyproxy/envoy

douglas-reid · 2020-01-15T22:21:14Z

Description:

This PR allows access to the gRPC status codes via the extensions common filter context. This will make it possible to report gRPC status codes in metrics and logs. This is a part of the work from: https://tinyurl.com/yy2cwmv5. A companion PR that hopes to use this code is istio/proxy#2624.

Risk Level: low
Testing: unit
Docs Changes: Updated RBAC filter attributes doc
Release Notes: none

Signed-off-by: Douglas Reid douglas-reid@users.noreply.github.com

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

htuch · 2020-01-16T03:22:05Z

@kyessenov would you have some time to take a first pass at this? Thanks.

douglas-reid · 2020-01-16T18:08:34Z

@htuch I believe @kyessenov is on vacation, so it will be a little while until he can take a look.

mandarjog · 2020-01-16T18:19:04Z

@PiotrSikora can you take a look?

PiotrSikora

LGTM, thanks!

mattklein123

LGTM with a small test question, thanks.

/wait

mattklein123 · 2020-01-17T20:32:50Z

source/extensions/filters/common/expr/context.cc

+        return CelValue::CreateInt64(status);
+      }
+    }
+    return {};


Do you have a test case that covers no headers/trailers and/or bad header/trailer values? (Optimally you could use Common::getGrpcStatus but not sure if it's worth it or not.

Thanks for the pointer to the Common:getGrpcStatus bit. I did not know that existed. That led me to look at the access log code that uses it, which ultimately led me to expand the fallbacks.

Now, after looking in trailers and headers, this will attempt to look at http response code and finally return Unknown if not found.

Matching the logging behavior seemed important.

I added a bunch of test cases for this updated behavior to match. Please let me know what you think.

Thanks, is there is any reasonable way to unify this logic and the access log logic so they don't fall out of sync? I agree they should be the same.

/wait-any

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

PiotrSikora · 2020-01-18T03:29:33Z

source/extensions/filters/common/expr/context.cc

+    if (code.has_value()) {
+      return CelValue::CreateInt64(Grpc::Utility::httpToGrpcStatus(code.value()));
+    }
+    return CelValue::CreateInt64(Grpc::Status::WellKnownGrpcStatus::Unknown);


I don't think that returning grpc-status: 2 (unknown) when it wasn't present in the response is a correct thing to do.

@PiotrSikora I was trying to match:

envoy/source/common/access_log/access_log_impl.cc

Lines 231 to 261 in 5248a4f

bool GrpcStatusFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,

const Http::HeaderMap& response_headers,

const Http::HeaderMap& response_trailers) {

// The gRPC specification does not guarantee a gRPC status code will be returned from a gRPC

// request. When it is returned, it will be in the response trailers. With that said, Envoy will

// treat a trailers-only response as a headers-only response, so we have to check the following

// in order:

// 1. response_trailers gRPC status, if it exists.

// 2. response_headers gRPC status, if it exists.

// 3. Inferred from info HTTP status, if it exists.

//

// If none of those options exist, it will default to Grpc::Status::WellKnownGrpcStatus::Unknown.

const std::array<absl::optional<Grpc::Status::GrpcStatus>, 3> optional_statuses = {{

{Grpc::Common::getGrpcStatus(response_trailers)},

{Grpc::Common::getGrpcStatus(response_headers)},

{info.responseCode() ? absl::optional<Grpc::Status::GrpcStatus>(

Grpc::Utility::httpToGrpcStatus(info.responseCode().value()))

: absl::nullopt},

}};

Grpc::Status::GrpcStatus status = Grpc::Status::WellKnownGrpcStatus::Unknown;

for (const auto& optional_status : optional_statuses) {

if (optional_status.has_value()) {

status = optional_status.value();

break;

}

}

const bool found = statuses_.find(status) != statuses_.end();

return exclude_ ? !found : found;

}

.

If not UNKNOWN, what status would you expect as a return value?

@douglas-reid fair point wrt being consistent with the access loggers, but I don't think any status should be returned as a value if there was no status sent on the wire...

Can't the return value by {}, absl::nullopt, or something else in that style?

It can definitely return {}. That was the way I had it originally.

I guess we need to decide on exactly what to do.

BTW, it looks like choosing "Unknown" has some prior art:

https://github.com/grpc/grpc-go/blob/master/internal/transport/http_util.go#L281-L288

I think it should be {}. There is quite a difference between missing field and grpc-status: 2.

I think it should be {}. Otherwise, we run the risk of confusing the consumer for a non-gRPC request.

kyessenov · 2020-01-21T23:55:41Z

source/extensions/filters/common/expr/context.cc

  } else if (value == Flags) {
    return CelValue::CreateInt64(info_.responseFlags());
+  } else if (value == GrpcStatus) {
+    for (const auto wrapper : ((HeadersWrapper[]){trailers_, headers_})) {


I'm not happy about the linear performance here. Maybe add a TODO to cache the computed value?

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid · 2020-01-22T18:15:29Z

@mattklein123 @PiotrSikora @kyessenov please take another look. I've updated the PR to extract the fallback logic to the Common lib. Additionally, this PR should preserve the access_logger Unknown behavior, while not adopting that in the RBAC expr context (returning {} instead). Tests have been updated to match.

mattklein123

Nice, thanks!

…#377) Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

feat(context): add response.grpc_status

9f94267

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid requested a review from lizan as a code owner January 15, 2020 22:21

douglas-reid requested a review from kyessenov January 15, 2020 22:21

douglas-reid mentioned this pull request Jan 15, 2020

feat(context): add suppport for getting the grpc_status code envoyproxy/envoy-wasm#372

Closed

small cleanup

54875b8

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

htuch assigned kyessenov Jan 16, 2020

PiotrSikora previously approved these changes Jan 16, 2020

View reviewed changes

mattklein123 requested changes Jan 17, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 17, 2020

mattklein123 self-assigned this Jan 17, 2020

use getGrpcStatus and test fallbacks

fece3de

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid dismissed PiotrSikora’s stale review via fece3de January 17, 2020 23:12

repokitteh-read-only bot removed the waiting label Jan 17, 2020

douglas-reid added 3 commits January 17, 2020 15:13

fix typo in comment

fb5bc3e

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

remove redundant test

e790373

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

fix format

ee42ce3

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

repokitteh-read-only bot added the waiting:any label Jan 17, 2020

PiotrSikora suggested changes Jan 18, 2020

View reviewed changes

repokitteh-read-only bot removed the waiting:any label Jan 18, 2020

mattklein123 added the waiting label Jan 19, 2020

kyessenov reviewed Jan 21, 2020

View reviewed changes

refactor status fallback logic into common library

cf0162d

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

repokitteh-read-only bot removed the waiting label Jan 22, 2020

mattklein123 approved these changes Jan 22, 2020

View reviewed changes

mattklein123 merged commit e9624ce into envoyproxy:master Jan 22, 2020

PiotrSikora pushed a commit to istio/envoy that referenced this pull request Jan 24, 2020

feat(context): add response.grpc_status (envoyproxy#9693) (envoyproxy…

f4f6fbd

…#377) Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid deleted the grpc-status-code branch April 19, 2021 22:55

	bool GrpcStatusFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap&,
	const Http::HeaderMap& response_headers,
	const Http::HeaderMap& response_trailers) {
	// The gRPC specification does not guarantee a gRPC status code will be returned from a gRPC
	// request. When it is returned, it will be in the response trailers. With that said, Envoy will
	// treat a trailers-only response as a headers-only response, so we have to check the following
	// in order:
	// 1. response_trailers gRPC status, if it exists.
	// 2. response_headers gRPC status, if it exists.
	// 3. Inferred from info HTTP status, if it exists.
	//
	// If none of those options exist, it will default to Grpc::Status::WellKnownGrpcStatus::Unknown.
	const std::array<absl::optional<Grpc::Status::GrpcStatus>, 3> optional_statuses = {{
	{Grpc::Common::getGrpcStatus(response_trailers)},
	{Grpc::Common::getGrpcStatus(response_headers)},
	{info.responseCode() ? absl::optional<Grpc::Status::GrpcStatus>(
	Grpc::Utility::httpToGrpcStatus(info.responseCode().value()))
	: absl::nullopt},
	}};

	Grpc::Status::GrpcStatus status = Grpc::Status::WellKnownGrpcStatus::Unknown;
	for (const auto& optional_status : optional_statuses) {
	if (optional_status.has_value()) {
	status = optional_status.value();
	break;
	}
	}

	const bool found = statuses_.find(status) != statuses_.end();
	return exclude_ ? !found : found;
	}

Conversation

douglas-reid commented Jan 15, 2020

Uh oh!

htuch commented Jan 16, 2020

Uh oh!

douglas-reid commented Jan 16, 2020

Uh oh!

mandarjog commented Jan 16, 2020

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglas-reid commented Jan 22, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants