Skip to content

Commit 12a1ece

Browse files
UgnineSirdisUgnineSirdis
authored
[Http] Reply with structured issues when client accepts json data (#7691)
1 parent ef7e2a9 commit 12a1ece

File tree

9 files changed

+104
-75
lines changed

9 files changed

+104
-75
lines changed

‎ydb/core/mon/async_http_mon.cpp

Lines changed: 49 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include <ydb/core/grpc_services/base/base.h>
66
#include <ydb/core/base/ticket_parser.h>
77

8+
#include <library/cpp/json/json_writer.h>
89
#include <library/cpp/lwtrace/all.h>
910
#include <library/cpp/lwtrace/mon/mon_lwtrace.h>
1011
#include <ydb/library/actors/core/probes.h>
@@ -145,6 +146,11 @@ class THttpMonRequest : public NMonitoring::IMonHttpRequest {
145146
return {};
146147
}
147148

149+
bool AcceptsJsonResponse() {
150+
TStringBuf acceptHeader = GetHeader("Accept");
151+
return acceptHeader.find(TStringBuf("application/json")) != TStringBuf::npos;
152+
}
153+
148154
virtual TStringBuf GetCookie(TStringBuf name) const override {
149155
NHttp::TCookies cookies(GetHeader("Cookie"));
150156
return cookies.Get(name);
@@ -247,10 +253,15 @@ class THttpMonLegacyActorRequest : public TActorBootstrapped<THttpMonLegacyActor
247253
PassAway();
248254
}
249255

256+
bool CredentialsProvided() {
257+
return Container.GetCookie("ydb_session_id") || Container.GetHeader("Authorization");
258+
}
259+
250260
TString YdbToHttpError(Ydb::StatusIds::StatusCode status) {
251261
switch (status) {
252262
case Ydb::StatusIds::UNAUTHORIZED:
253-
return "401 Unauthorized";
263+
// YDB status UNAUTHORIZED is used for both access denied case and if no credentials were provided.
264+
return CredentialsProvided() ? "403 Forbidden" : "401 Unauthorized";
254265
case Ydb::StatusIds::INTERNAL_ERROR:
255266
return "500 Internal Server Error";
256267
case Ydb::StatusIds::UNAVAILABLE:
@@ -267,26 +278,45 @@ class THttpMonLegacyActorRequest : public TActorBootstrapped<THttpMonLegacyActor
267278
}
268279

269280
void ReplyErrorAndPassAway(const NKikimr::NGRpcService::TEvRequestAuthAndCheckResult& result) {
281+
ReplyErrorAndPassAway(result.Status, result.Issues, true);
282+
}
283+
284+
void ReplyErrorAndPassAway(Ydb::StatusIds::StatusCode status, const NYql::TIssues& issues, bool addAccessControlHeaders) {
270285
NHttp::THttpIncomingRequestPtr request = Event->Get()->Request;
271-
NHttp::THeaders headers(request->Headers);
272286
TStringBuilder response;
273287
TStringBuilder body;
274-
const TString httpError = YdbToHttpError(result.Status);
275-
body << "<html><body><h1>" << httpError << "</h1>";
276-
if (result.Issues) {
277-
body << "<p>" << result.Issues.ToString() << "</p>";
278-
}
279-
body << "</body></html>";
280-
TString origin = TString(headers["Origin"]);
281-
if (origin.empty()) {
282-
origin = "*";
288+
TStringBuf contentType;
289+
const TString httpError = YdbToHttpError(status);
290+
291+
if (Container.AcceptsJsonResponse()) {
292+
contentType = "application/json";
293+
NJson::TJsonValue json;
294+
TString message;
295+
MakeJsonErrorReply(json, message, issues, NYdb::EStatus(status));
296+
NJson::WriteJson(&body.Out, &json);
297+
} else {
298+
contentType = "text/html";
299+
body << "<html><body><h1>" << httpError << "</h1>";
300+
if (issues) {
301+
body << "<p>" << issues.ToString() << "</p>";
302+
}
303+
body << "</body></html>";
283304
}
305+
284306
response << "HTTP/1.1 " << httpError << "\r\n";
285-
response << "Access-Control-Allow-Origin: " << origin << "\r\n";
286-
response << "Access-Control-Allow-Credentials: true\r\n";
287-
response << "Access-Control-Allow-Headers: Content-Type,Authorization,Origin,Accept\r\n";
288-
response << "Access-Control-Allow-Methods: OPTIONS, GET, POST, PUT, DELETE\r\n";
289-
response << "Content-Type: text/html\r\n";
307+
if (addAccessControlHeaders) {
308+
NHttp::THeaders headers(request->Headers);
309+
TString origin = TString(headers["Origin"]);
310+
if (origin.empty()) {
311+
origin = "*";
312+
}
313+
response << "Access-Control-Allow-Origin: " << origin << "\r\n";
314+
response << "Access-Control-Allow-Credentials: true\r\n";
315+
response << "Access-Control-Allow-Headers: Content-Type,Authorization,Origin,Accept\r\n";
316+
response << "Access-Control-Allow-Methods: OPTIONS, GET, POST, PUT, DELETE\r\n";
317+
}
318+
319+
response << "Content-Type: " << contentType << "\r\n";
290320
response << "Content-Length: " << body.Size() << "\r\n";
291321
response << "\r\n";
292322
response << body;
@@ -295,21 +325,9 @@ class THttpMonLegacyActorRequest : public TActorBootstrapped<THttpMonLegacyActor
295325
}
296326

297327
void ReplyForbiddenAndPassAway(const TString& error = {}) {
298-
NHttp::THttpIncomingRequestPtr request = Event->Get()->Request;
299-
TStringBuilder response;
300-
TStringBuilder body;
301-
body << "<html><body><h1>403 Forbidden</h1>";
302-
if (!error.empty()) {
303-
body << "<p>" << error << "</p>";
304-
}
305-
body << "</body></html>";
306-
response << "HTTP/1.1 403 Forbidden\r\n";
307-
response << "Content-Type: text/html\r\n";
308-
response << "Content-Length: " << body.Size() << "\r\n";
309-
response << "\r\n";
310-
response << body;
311-
ReplyWith(request->CreateResponseString(response));
312-
PassAway();
328+
NYql::TIssues issues;
329+
issues.AddIssue(error);
330+
ReplyErrorAndPassAway(Ydb::StatusIds::UNAUTHORIZED, issues, false);
313331
}
314332

315333
void SendRequest(const NKikimr::NGRpcService::TEvRequestAuthAndCheckResult* result = nullptr) {

‎ydb/core/mon/mon.cpp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include <library/cpp/json/json_value.h>
1010
#include <library/cpp/json/json_reader.h>
11+
#include <library/cpp/protobuf/json/proto2json.h>
1112

1213
#include <util/string/ascii.h>
1314

@@ -88,6 +89,48 @@ NActors::IEventHandle* GetAuthorizeTicketResult(const NActors::TActorId& owner)
8889
}
8990
}
9091

92+
void MakeJsonErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status) {
93+
MakeJsonErrorReply(jsonResponse, message, status.GetIssues(), status.GetStatus());
94+
}
95+
96+
void MakeJsonErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYql::TIssues& issues, NYdb::EStatus status) {
97+
google::protobuf::RepeatedPtrField<Ydb::Issue::IssueMessage> protoIssues;
98+
NYql::IssuesToMessage(issues, &protoIssues);
99+
100+
message.clear();
101+
102+
NJson::TJsonValue& jsonIssues = jsonResponse["issues"];
103+
for (const auto& queryIssue : protoIssues) {
104+
NJson::TJsonValue& issue = jsonIssues.AppendValue({});
105+
NProtobufJson::Proto2Json(queryIssue, issue);
106+
}
107+
108+
TString textStatus = TStringBuilder() << status;
109+
jsonResponse["status"] = textStatus;
110+
111+
// find first deepest error
112+
std::stable_sort(protoIssues.begin(), protoIssues.end(), [](const Ydb::Issue::IssueMessage& a, const Ydb::Issue::IssueMessage& b) -> bool {
113+
return a.severity() < b.severity();
114+
});
115+
116+
const google::protobuf::RepeatedPtrField<Ydb::Issue::IssueMessage>* protoIssuesPtr = &protoIssues;
117+
while (protoIssuesPtr->size() > 0 && protoIssuesPtr->at(0).issuesSize() > 0) {
118+
protoIssuesPtr = &protoIssuesPtr->at(0).issues();
119+
}
120+
121+
if (protoIssuesPtr->size() > 0) {
122+
const Ydb::Issue::IssueMessage& issue = protoIssuesPtr->at(0);
123+
NProtobufJson::Proto2Json(issue, jsonResponse["error"]);
124+
message = issue.message();
125+
} else {
126+
jsonResponse["error"]["message"] = textStatus;
127+
}
128+
129+
if (message.empty()) {
130+
message = textStatus;
131+
}
132+
}
133+
91134
IMonPage* TMon::RegisterActorPage(TIndexMonPage* index, const TString& relPath,
92135
const TString& title, bool preTag, TActorSystem* actorSystem, const TActorId& actorId, bool useAuth, bool sortPages) {
93136
return RegisterActorPage({

‎ydb/core/mon/mon.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#pragma once
22

3+
#include <library/cpp/json/writer/json_value.h>
34
#include <library/cpp/monlib/service/monservice.h>
45
#include <library/cpp/monlib/dynamic_counters/counters.h>
56
#include <library/cpp/monlib/service/pages/resources/css_mon_page.h>
@@ -10,12 +11,17 @@
1011

1112
#include <ydb/library/actors/core/actor.h>
1213
#include <ydb/library/actors/core/mon.h>
14+
#include <ydb/library/yql/public/issue/yql_issue.h>
15+
#include <ydb/public/sdk/cpp/client/ydb_types/status/status.h>
1316

1417
namespace NActors {
1518

1619
IEventHandle* SelectAuthorizationScheme(const NActors::TActorId& owner, NMonitoring::IMonHttpRequest& request);
1720
IEventHandle* GetAuthorizeTicketResult(const NActors::TActorId& owner);
1821

22+
void MakeJsonErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYql::TIssues& issues, NYdb::EStatus status);
23+
void MakeJsonErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status);
24+
1925
class TActorSystem;
2026
struct TActorId;
2127

‎ydb/core/mon/ya.make

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,16 @@ SRCS(
1414
PEERDIR(
1515
library/cpp/json
1616
library/cpp/lwtrace/mon
17+
library/cpp/protobuf/json
1718
library/cpp/string_utils/url
1819
ydb/core/base
1920
ydb/core/grpc_services/base
2021
ydb/core/protos
2122
ydb/library/aclib
2223
ydb/library/actors/core
2324
ydb/library/actors/http
25+
ydb/library/yql/public/issue
26+
ydb/public/sdk/cpp/client/ydb_types/status
2427
)
2528

2629
END()

‎ydb/core/viewer/json_local_rpc.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ class TJsonLocalRpc : public TActorBootstrapped<TJsonLocalRpc<TProtoRequest, TPr
209209
if (!Result->Status->IsSuccess()) {
210210
NJson::TJsonValue json;
211211
TString message;
212-
MakeErrorReply(json, message, Result->Status.value());
212+
MakeJsonErrorReply(json, message, Result->Status.value());
213213
TStringStream stream;
214214
NJson::WriteJson(&stream, &json);
215215
if (Result->Status->GetStatus() == NYdb::EStatus::UNAUTHORIZED) {

‎ydb/core/viewer/json_query.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -478,7 +478,7 @@ class TJsonQuery : public TViewerPipeClient<TJsonQuery> {
478478
void MakeErrorReply(NJson::TJsonValue& jsonResponse, const NYdb::TStatus& status) {
479479
TString message;
480480

481-
NViewer::MakeErrorReply(jsonResponse, message, status);
481+
MakeJsonErrorReply(jsonResponse, message, status);
482482

483483
if (Span) {
484484
Span.EndError(message);
@@ -738,7 +738,5 @@ YAML::Node TJsonRequestSwagger<TJsonQuery>::GetSwagger() {
738738
)___");
739739
return node;
740740
}
741-
742-
743741
}
744742
}

‎ydb/core/viewer/viewer.cpp

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -774,44 +774,6 @@ TString TViewer::GetHTTPFORWARD(const TRequestState& request, const TString& loc
774774
return res;
775775
}
776776

777-
void MakeErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status) {
778-
google::protobuf::RepeatedPtrField<Ydb::Issue::IssueMessage> protoIssues;
779-
NYql::IssuesToMessage(status.GetIssues(), &protoIssues);
780-
781-
message.clear();
782-
783-
NJson::TJsonValue& jsonIssues = jsonResponse["issues"];
784-
for (const auto& queryIssue : protoIssues) {
785-
NJson::TJsonValue& issue = jsonIssues.AppendValue({});
786-
NProtobufJson::Proto2Json(queryIssue, issue);
787-
}
788-
789-
TString textStatus = TStringBuilder() << status.GetStatus();
790-
jsonResponse["status"] = textStatus;
791-
792-
// find first deepest error
793-
std::stable_sort(protoIssues.begin(), protoIssues.end(), [](const Ydb::Issue::IssueMessage& a, const Ydb::Issue::IssueMessage& b) -> bool {
794-
return a.severity() < b.severity();
795-
});
796-
797-
const google::protobuf::RepeatedPtrField<Ydb::Issue::IssueMessage>* protoIssuesPtr = &protoIssues;
798-
while (protoIssuesPtr->size() > 0 && protoIssuesPtr->at(0).issuesSize() > 0) {
799-
protoIssuesPtr = &protoIssuesPtr->at(0).issues();
800-
}
801-
802-
if (protoIssuesPtr->size() > 0) {
803-
const Ydb::Issue::IssueMessage& issue = protoIssuesPtr->at(0);
804-
NProtobufJson::Proto2Json(issue, jsonResponse["error"]);
805-
message = issue.message();
806-
} else {
807-
jsonResponse["error"]["message"] = textStatus;
808-
}
809-
810-
if (message.empty()) {
811-
message = textStatus;
812-
}
813-
}
814-
815777
NKikimrViewer::EFlag GetFlagFromTabletState(NKikimrWhiteboard::TTabletStateInfo::ETabletState state) {
816778
NKikimrViewer::EFlag flag = NKikimrViewer::EFlag::Grey;
817779
switch (state) {

‎ydb/core/viewer/viewer.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,6 @@ void SplitIds(TStringBuf source, char delim, std::unordered_set<ValueType>& valu
261261
GenericSplitIds<ValueType>(source, delim, std::inserter(values, values.end()));
262262
}
263263

264-
void MakeErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status);
265-
266264
TString GetHTTPOKJSON();
267265
TString GetHTTPGATEWAYTIMEOUT();
268266
NKikimrViewer::EFlag GetFlagFromTabletState(NKikimrWhiteboard::TTabletStateInfo::ETabletState state);

‎ydb/core/viewer/ya.make

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,7 @@ PEERDIR(
530530
ydb/core/grpc_services
531531
ydb/core/grpc_services/local_rpc
532532
ydb/core/health_check
533+
ydb/core/mon
533534
ydb/core/node_whiteboard
534535
ydb/core/protos
535536
ydb/core/scheme

0 commit comments

Comments
 (0)