From 243f701cf221f05152e36c104d1563815c9c23db Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Mon, 13 Apr 2020 12:43:12 -0700 Subject: [PATCH] Convert 4xx ServiceControl failures to 500 --- src/api_manager/service_control/aggregated.cc | 27 ++++++++++++------- src/nginx/t/check_return_403.t | 2 +- src/nginx/t/failed_check.t | 4 +-- src/nginx/t/report_failed_request.t | 12 ++++----- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/api_manager/service_control/aggregated.cc b/src/api_manager/service_control/aggregated.cc index fb33ddf48..404585e9a 100644 --- a/src/api_manager/service_control/aggregated.cc +++ b/src/api_manager/service_control/aggregated.cc @@ -93,12 +93,10 @@ const char servicecontrol_service[] = const char quotacontrol_service[] = "/google.api.servicecontrol.v1.QuotaController"; -// Define network failure error codes: -// All 5xx Http status codes are marked as network failure. // Http status code is converted to Status::code as: // https://github.com/cloudendpoints/esp/blob/master/src/api_manager/utils/status.cc#L364 // which is called by Status.ToProto() at Aggregated::Call() on_done function. -bool IsErrorCodeNetworkFailure(int code) { +bool StatusCodeIs5xxHttpCode(int code) { return code == Code::UNAVAILABLE || code == Code::INTERNAL || code == Code::UNIMPLEMENTED || code == Code::DEADLINE_EXCEEDED; } @@ -381,18 +379,29 @@ void Aggregated::Check( *response, service_control_proto_.service_name(), &response_info); on_done(status, response_info); } else { - // If network_fail_open is true, it is OK to proceed - if (network_fail_open_ && - IsErrorCodeNetworkFailure(status.error_code())) { + // All 5xx Http status codes are treated as network failure. + // If network_fail_open is true, it is OK to proceed with these errors. + if (network_fail_open_ && StatusCodeIs5xxHttpCode(status.error_code())) { env_->LogError( std::string("With network fail open policy, the request is allowed " "even the service control check failed with: " + status.ToString())); on_done(Status::OK, response_info); } else { - on_done(Status(status.error_code(), status.error_message(), - Status::SERVICE_CONTROL), - response_info); + // Preserve ServiceControl 5xx Http response code, especially 500 and + // 503. Convert non-5xx Http resonse code to 500 since they are most + // likely from API producer config errors. For example, wrong service + // account permission, or some key services are not enabled. + int error_code; + if (StatusCodeIs5xxHttpCode(status.error_code())) { + error_code = status.error_code(); + } else { + error_code = Code::INTERNAL; + } + + on_done( + Status(error_code, status.error_message(), Status::SERVICE_CONTROL), + response_info); } } delete response; diff --git a/src/nginx/t/check_return_403.t b/src/nginx/t/check_return_403.t index a8daa197e..e536a5b4f 100644 --- a/src/nginx/t/check_return_403.t +++ b/src/nginx/t/check_return_403.t @@ -97,7 +97,7 @@ my $response = ApiManager::http_get($NginxPort,'/shelves?key=this-is-an-api-key' $t->stop_daemons(); my ($response_headers, $response_body) = split /\r\n\r\n/, $response, 2; -like($response, qr/HTTP\/1\.1 403 Forbidden/, 'Returned HTTP 403'); +like($response, qr/HTTP\/1\.1 500 Internal Server Error/, 'Returned HTTP 500'); like($response, qr/detail error message line1/, 'error body line 1 is included'); like($response, qr/detail error message line2/, 'error body line 2 is included'); diff --git a/src/nginx/t/failed_check.t b/src/nginx/t/failed_check.t index 373e1cb5b..e08a03b61 100644 --- a/src/nginx/t/failed_check.t +++ b/src/nginx/t/failed_check.t @@ -99,12 +99,12 @@ like($response1, qr/content-type: application\/json/i, like($response1, qr/API endpoints-test.cloudendpointsapis.com is not enabled for the consumer project/i, "Error body contains 'activation error'."); -like($response2, qr/HTTP\/1\.1 403 Forbidden/, 'Response2 returned HTTP 403.'); +like($response2, qr/HTTP\/1\.1 500 Internal Server Error/, 'Response2 returned HTTP 500.'); like($response2, qr/content-type: application\/json/i, 'Unauthorized returned application/json body.'); like($response2, qr/Service control request failed/i, "Error body contains 'service control failed'."); -like($response3, qr/HTTP\/1\.1 400 Bad Request/, 'Response3 returned HTTP 400.'); +like($response3, qr/HTTP\/1\.1 500 Internal Server Error/, 'Response3 returned HTTP 500.'); like($response3, qr/content-type: application\/json/i, 'Unauthorized returned application/json body.'); diff --git a/src/nginx/t/report_failed_request.t b/src/nginx/t/report_failed_request.t index 8b013d5b1..0a390b9c7 100644 --- a/src/nginx/t/report_failed_request.t +++ b/src/nginx/t/report_failed_request.t @@ -92,7 +92,7 @@ $t->stop_daemons(); my ($response_headers, $response_body) = split /\r\n\r\n/, $response, 2; -like($response_headers, qr/HTTP\/1\.1 403 Forbidden/, 'Returned HTTP 403.'); +like($response_headers, qr/HTTP\/1\.1 500 Internal Server Error/, 'Returned HTTP 500.'); my @servicecontrol_requests = ApiManager::read_http_stream($t, 'servicecontrol.log'); is(scalar @servicecontrol_requests, 2, 'Service control was called twice.'); @@ -113,14 +113,14 @@ my $expected_report_body = ServiceControl::gen_report_body({ 'api_key' => 'this-is-an-api-key', 'api_method' => 'ListShelves', 'http_method' => 'GET', - 'log_message' => 'Method: ListShelves failed: PERMISSION_DENIED: Service control request failed with HTTP response code 403', - 'response_code' => '403', + 'log_message' => 'Method: ListShelves failed: INTERNAL: Service control request failed with HTTP response code 403', + 'response_code' => '500', 'error_cause' => 'service_control', - 'error_type' => '4xx', + 'error_type' => '5xx', 'request_size' => 62, - 'response_size' => 380, + 'response_size' => 367, 'request_bytes' => 62, - 'response_bytes' => 380, + 'response_bytes' => 367, 'producer_project_id' => 'endpoints-test', });