-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Supports return json type of response #6261
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.
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
Changes from all commits
462b70c
7e258ac
f533efd
dd9636a
73b2997
6b19f4a
4a67ec8
e4d6585
80f4e09
162c95f
f305fa7
67b56c9
20542e1
395a0f8
c5c1598
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library") | ||
| load("@envoy_api//bazel:api_build_system.bzl", "api_go_proto_library", "api_proto_library") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
|
|
@@ -13,3 +13,16 @@ api_proto_library( | |
| "//envoy/api/v2/core:base", | ||
| ], | ||
| ) | ||
|
|
||
| api_proto_library( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I would consider this a core type. Is this a generic JSON description or very specific to the idea of returning a JSON reply from HCM? |
||
| name = "local_reply_configuration", | ||
| srcs = ["local_reply_configuration.proto"], | ||
| visibility = [ | ||
| "//visibility:public", | ||
| ], | ||
| ) | ||
|
|
||
| api_go_proto_library( | ||
| name = "local_reply_configuration", | ||
| proto = ":local_reply_configuration", | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.data.core.v2alpha; | ||
|
|
||
| option java_outer_classname = "LocalReplyConfigurationProto"; | ||
| option java_multiple_files = true; | ||
| option java_package = "io.envoyproxy.envoy.data.core.v2alpha"; | ||
|
|
||
| // [#protodoc-title: Local reply configuration] | ||
|
|
||
| // Configure the local reply of the body type | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the body type? |
||
| message LocalReplyConfiguration { | ||
| message JsonReply { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't look like this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue mentions the need to remove the dependency of RapidJSON. The json conversion uses the method of protobuf to convert, so I defined a protobuf for converting the local reply body. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, interesting, I didn't know about that. In that case, my recommendation would be to just instantiate a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope to have a clear description scheme
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, Struct would be the canonical representation of JSON in proto. |
||
| string body = 1; | ||
| } | ||
|
|
||
| enum MediaType { | ||
| TEXT_PLAIN = 0; | ||
| APPLICATION_JSON = 1; | ||
| NEGOTIATE_VIA_ACCEPT_HEADER = 2; | ||
| } | ||
|
|
||
| MediaType media_type = 1; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,3 +6,4 @@ Core data | |
| :maxdepth: 2 | ||
|
|
||
| v2alpha/health_check_event.proto | ||
| v2alpha/local_reply_configuration.proto | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,28 +16,15 @@ | |
| #include "common/common/macros.h" | ||
| #include "common/common/stack_array.h" | ||
| #include "common/common/utility.h" | ||
| #include "common/grpc/utility.h" | ||
| #include "common/http/headers.h" | ||
| #include "common/http/message_impl.h" | ||
| #include "common/http/utility.h" | ||
| #include "common/protobuf/protobuf.h" | ||
|
|
||
| #include "absl/strings/match.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Grpc { | ||
|
|
||
| bool Common::hasGrpcContentType(const Http::HeaderMap& headers) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering whether, instead of moving this, it might be more useful to build a simple classifier function. It'd have a signature something like: ContentType contentType(const Http::HeaderMap& headers);... where
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea |
||
| const Http::HeaderEntry* content_type = headers.ContentType(); | ||
| // Content type is gRPC if it is exactly "application/grpc" or starts with | ||
| // "application/grpc+". Specifically, something like application/grpc-web is not gRPC. | ||
| return content_type != nullptr && | ||
| absl::StartsWith(content_type->value().getStringView(), | ||
| Http::Headers::get().ContentTypeValues.Grpc) && | ||
| (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() || | ||
| content_type->value() | ||
| .getStringView()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+'); | ||
| } | ||
|
|
||
| bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream) { | ||
| if (end_stream) { | ||
| // Trailers-only response, only grpc-status is required. | ||
|
|
@@ -46,7 +33,7 @@ bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_strea | |
| if (Http::Utility::getResponseStatus(headers) != enumToInt(Http::Code::OK)) { | ||
| return false; | ||
| } | ||
| return hasGrpcContentType(headers); | ||
| return Utility::hasGrpcContentType(headers); | ||
| } | ||
|
|
||
| absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap& trailers) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not super clear. Can you write a more detailed description of what this field is doing?