Skip to content

Commit eda17f9

Browse files
UgnineSirdisUgnineSirdis
authored
[Http] Reply with structured issues when client accepts json data (#7576)
1 parent a2e8760 commit eda17f9

File tree

9 files changed

+104
-74
lines changed

9 files changed

+104
-74
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);
@@ -248,10 +254,15 @@ class THttpMonLegacyActorRequest : public TActorBootstrapped<THttpMonLegacyActor
248254
PassAway();
249255
}
250256

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

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

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

316334
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
@@ -200,7 +200,7 @@ class TJsonLocalRpc : public TActorBootstrapped<TJsonLocalRpc<TProtoRequest, TPr
200200
if (!Result->Status->IsSuccess()) {
201201
NJson::TJsonValue json;
202202
TString message;
203-
MakeErrorReply(json, message, Result->Status.value());
203+
MakeJsonErrorReply(json, message, Result->Status.value());
204204
TStringStream stream;
205205
NJson::WriteJson(&stream, &json);
206206
if (Result->Status->GetStatus() == NYdb::EStatus::UNAUTHORIZED) {

‎ydb/core/viewer/viewer.cpp

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -733,44 +733,6 @@ TString TViewer::GetHTTPFORWARD(const TRequestState& request, const TString& loc
733733
return res;
734734
}
735735

736-
void MakeErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status) {
737-
google::protobuf::RepeatedPtrField<Ydb::Issue::IssueMessage> protoIssues;
738-
NYql::IssuesToMessage(status.GetIssues(), &protoIssues);
739-
740-
message.clear();
741-
742-
NJson::TJsonValue& jsonIssues = jsonResponse["issues"];
743-
for (const auto& queryIssue : protoIssues) {
744-
NJson::TJsonValue& issue = jsonIssues.AppendValue({});
745-
NProtobufJson::Proto2Json(queryIssue, issue);
746-
}
747-
748-
TString textStatus = TStringBuilder() << status.GetStatus();
749-
jsonResponse["status"] = textStatus;
750-
751-
// find first deepest error
752-
std::stable_sort(protoIssues.begin(), protoIssues.end(), [](const Ydb::Issue::IssueMessage& a, const Ydb::Issue::IssueMessage& b) -> bool {
753-
return a.severity() < b.severity();
754-
});
755-
756-
const google::protobuf::RepeatedPtrField<Ydb::Issue::IssueMessage>* protoIssuesPtr = &protoIssues;
757-
while (protoIssuesPtr->size() > 0 && protoIssuesPtr->at(0).issuesSize() > 0) {
758-
protoIssuesPtr = &protoIssuesPtr->at(0).issues();
759-
}
760-
761-
if (protoIssuesPtr->size() > 0) {
762-
const Ydb::Issue::IssueMessage& issue = protoIssuesPtr->at(0);
763-
NProtobufJson::Proto2Json(issue, jsonResponse["error"]);
764-
message = issue.message();
765-
} else {
766-
jsonResponse["error"]["message"] = textStatus;
767-
}
768-
769-
if (message.empty()) {
770-
message = textStatus;
771-
}
772-
}
773-
774736
NKikimrViewer::EFlag GetFlagFromTabletState(NKikimrWhiteboard::TTabletStateInfo::ETabletState state) {
775737
NKikimrViewer::EFlag flag = NKikimrViewer::EFlag::Grey;
776738
switch (state) {

‎ydb/core/viewer/viewer.h

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

233-
void MakeErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status);
234-
235233
TString GetHTTPOKJSON();
236234
TString GetHTTPGATEWAYTIMEOUT();
237235
NKikimrViewer::EFlag GetFlagFromTabletState(NKikimrWhiteboard::TTabletStateInfo::ETabletState state);

‎ydb/core/viewer/viewer_query.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,7 @@ class TJsonQuery : public TViewerPipeClient {
512512
void MakeErrorReply(NJson::TJsonValue& jsonResponse, const NYdb::TStatus& status) {
513513
TString message;
514514

515-
NViewer::MakeErrorReply(jsonResponse, message, status);
515+
MakeJsonErrorReply(jsonResponse, message, status);
516516

517517
if (Span) {
518518
Span.EndError(message);
@@ -774,4 +774,3 @@ class TJsonQuery : public TViewerPipeClient {
774774
};
775775

776776
}
777-

‎ydb/core/viewer/ya.make

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,7 @@ PEERDIR(
551551
ydb/core/grpc_services
552552
ydb/core/grpc_services/local_rpc
553553
ydb/core/health_check
554+
ydb/core/mon
554555
ydb/core/node_whiteboard
555556
ydb/core/protos
556557
ydb/core/scheme

0 commit comments

Comments
 (0)