Skip to content
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

Add flag enable_api_key_uid_reporting and report unknown when the check response status is not OK. #865

Merged
merged 3 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ load("@io_bazel_rules_python//python:pip.bzl", "pip_repositories", "pip_import")

pip_import(
name = "grpc_python_dependencies",
requirements = "@com_github_grpc_grpc//:requirements.bazel.txt",
requirements = "//:requirements.bazel.txt",
)

load("@com_github_grpc_grpc//bazel:grpc_deps.bzl", "grpc_deps", "grpc_test_only_deps")
Expand Down
16 changes: 16 additions & 0 deletions requirements.bazel.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# GRPC Python setup requirements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we keep this up to date now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to date requirements might require Python later versions which is incompatible with ESP build rules.

Currently it's pinned to a specific version which could not update. How do you think we should address this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I think we have no choice to postpone the tech debts to later. Can you please log a TODO bug? File one in bugnizer so we can ask for resources based on these data points.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, created todo and internal bug b/341556079.

coverage>=4.0
cython==0.28.3
enum34>=1.0.4
protobuf==3.5.0.post1
six>=1.10
wheel>=0.29
futures>=2.2.0
google-auth>=1.0.0
oauth2client==4.1.0
requests>=2.14.2
urllib3>=1.23
chardet==3.0.4
certifi==2017.4.17
idna==2.7
googleapis-common-protos==1.5.5
4 changes: 4 additions & 0 deletions src/api_manager/proto/server_config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ message ServiceControlConfig {
// In case of failing to connect to service control service, the requests
// are allowed if this field is true. Default is false.
bool network_fail_open = 16;

// If set to true, reports api_key_uid instead of api_key in ServiceControl
// report.
bool enable_api_key_uid_reporting = 17;
}

// Check aggregator config
Expand Down
8 changes: 7 additions & 1 deletion src/api_manager/service_control/aggregated.cc
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,16 @@ void Aggregated::Check(
TRACE(trace_span) << "Check returned with status: " << status.ToString();
CheckResponseInfo response_info;
if (status.ok()) {
bool enableApiKeyUid = server_config_
&& server_config_->has_service_control_config()
&& server_config_->service_control_config()
.enable_api_key_uid_reporting();
Status status = Proto::ConvertCheckResponse(
*response, service_control_proto_.service_name(), &response_info);
*response, service_control_proto_.service_name(), &response_info,
enableApiKeyUid);
on_done(status, response_info);
} else {
response_info.is_network_failure = true;
// 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())) {
Expand Down
7 changes: 6 additions & 1 deletion src/api_manager/service_control/info.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,14 @@ struct CheckResponseInfo {
// The api key uid of the request.
std::string api_key_uid;

// The check has failed because of network failure.
bool is_network_failure;

// By default api_key is valid and service is activated.
// They only set to false by the check response from server.
CheckResponseInfo() : is_api_key_valid(true), service_is_activated(true) {}
CheckResponseInfo() : is_api_key_valid(true),
service_is_activated(true),
is_network_failure(false) {}
};

struct QuotaRequestInfo : public OperationInfo {
Expand Down
22 changes: 18 additions & 4 deletions src/api_manager/service_control/proto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
//
////////////////////////////////////////////////////////////////////////////////
//
#include "absl/strings/str_cat.h"

#include "src/api_manager/service_control/proto.h"

#include <functional>
Expand Down Expand Up @@ -544,11 +546,20 @@ const char kServiceAgentPrefix[] = "ESP/";
Status set_credential_id(const SupportedLabel& l, const ReportRequestInfo& info,
Map<std::string, std::string>* labels) {
// The rule to set /credential_id is:
// 1) If api_key is available, set it as apiKey:API-KEY
// 1) If api_key is available.
// 1] if CheckRequest has RPC error, set it as "apikey:UNKNOWN".
// 2] if CheckRequest status is OK(RPC successful):
// if api_key_uid present, set it as api_key_uid.
// else, set it as "apikey:<api_key>".
// 2) If auth issuer and audience both are available, set it as:
// jwtAuth:issuer=base64(issuer)&audience=base64(audience)
if (!info.api_key.empty()) {
(*labels)[l.name] = absl::StrCat(kApiKeyPrefix, info.check_response_info.api_key_uid.empty() ? info.api_key : info.check_response_info.api_key_uid);
if (info.check_response_info.is_network_failure) {
(*labels)[l.name] = absl::StrCat(kApiKeyPrefix, "UNKNOWN");
} else {
(*labels)[l.name] = info.check_response_info.api_key_uid.empty() ? absl::StrCat(kApiKeyPrefix, info.api_key)
: info.check_response_info.api_key_uid;
}
} else if (!info.auth_issuer.empty()) {
// If auth is used, auth_issuer should NOT be empty since it is required.
char* base64_issuer = auth::esp_base64_encode(
Expand Down Expand Up @@ -1386,14 +1397,17 @@ Status Proto::ConvertAllocateQuotaResponse(

Status Proto::ConvertCheckResponse(const CheckResponse& check_response,
const std::string& service_name,
CheckResponseInfo* check_response_info) {
CheckResponseInfo* check_response_info,
bool enable_api_key_uid) {
if (check_response.check_info().consumer_info().project_number() > 0) {
// Store project id to check_response_info
check_response_info->consumer_project_id = std::to_string(
check_response.check_info().consumer_info().project_number());
}

check_response_info->api_key_uid = check_response.check_info().api_key_uid();
if (enable_api_key_uid) {
check_response_info->api_key_uid = check_response.check_info().api_key_uid();
}

if (check_response.check_errors().size() == 0) {
return Status::OK;
Expand Down
3 changes: 2 additions & 1 deletion src/api_manager/service_control/proto.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ class Proto final {
// failures.
static utils::Status ConvertCheckResponse(
const ::google::api::servicecontrol::v1::CheckResponse& response,
const std::string& service_name, CheckResponseInfo* check_response_info);
const std::string& service_name, CheckResponseInfo* check_response_info,
bool enable_api_key_uid = false);

static utils::Status ConvertAllocateQuotaResponse(
const ::google::api::servicecontrol::v1::AllocateQuotaResponse& response,
Expand Down
25 changes: 25 additions & 0 deletions src/api_manager/service_control/proto_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,31 @@ TEST_F(ProtoTest, CredentailIdApiKeyTest) {
"apikey:api_key_x");
}

TEST_F(ProtoTest, CredentailIdApiKeyUidTest) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: typo "credential"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

ReportRequestInfo info;
FillOperationInfo(&info);
info.check_response_info.api_key_uid = "apikey:fake_uid";

gasv1::ReportRequest request;
ASSERT_TRUE(scp_.FillReportRequest(info, &request).ok());

ASSERT_EQ(request.operations(0).labels().at("/credential_id"),
"apikey:fake_uid");
}

TEST_F(ProtoTest, CredentailIdApiKeyUidUnknownTest) {
ReportRequestInfo info;
FillOperationInfo(&info);
info.check_response_info.is_network_failure = true;
info.check_response_info.api_key_uid = "apikey:fake_uid";

gasv1::ReportRequest request;
ASSERT_TRUE(scp_.FillReportRequest(info, &request).ok());

ASSERT_EQ(request.operations(0).labels().at("/credential_id"),
"apikey:UNKNOWN");
}

TEST_F(ProtoTest, CredentailIdIssuerOnlyTest) {
ReportRequestInfo info;
FillOperationInfo(&info);
Expand Down
3 changes: 3 additions & 0 deletions start_esp/server-auto.conf.template
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ service_control_config {
%if service_control_network_fail_open:
network_fail_open: true
% endif
% if enable_api_key_uid_reporting:
enable_api_key_uid_reporting: true
% endif
}
% endif
% if jwks_cache_duration_in_s or redirect_authorization_url:
Expand Down
7 changes: 6 additions & 1 deletion start_esp/start_esp.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,8 @@ def write_server_config_template(server_config_path, args):
grpc_backend_ssl_credentials=args.grpc_backend_ssl_credentials,
jwks_cache_duration_in_s=args.jwks_cache_duration_in_s,
redirect_authorization_url=args.enable_jwt_authorization_url_redirect,
rollout_fetch_throttle_window_in_s=args.rollout_fetch_throttle_window_in_s)
rollout_fetch_throttle_window_in_s=args.rollout_fetch_throttle_window_in_s,
enable_api_key_uid_reporting=args.enable_api_key_uid_reporting)

server_config_file = server_config_path
if server_config_file.endswith('/'):
Expand Down Expand Up @@ -951,6 +952,10 @@ def make_argparser():
to call at the same time to exceed the quota, the calling time is throttled within
a window. This flag specifies the throttle window in seconds. Default is 5 minutes.
If number of ESP instances for a service is big, please increase this number.''')

parser.add_argument('--enable_api_key_uid_reporting', action='store_true',
help='''If set to true, reports api_key_uid instead of api_key in
ServiceControl report.''')

return parser

Expand Down
5 changes: 5 additions & 0 deletions start_esp/test/start_esp_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -371,5 +371,10 @@ def test_enable_backend_routing_conflicts_with_single_dash_flag(self):
return_code = os.system(config_generator)
self.assertEqual(return_code >> 8, 3)

def test_enable_api_key_uid_reporting(self):
expected_config_file = "./start_esp/test/testdata/expected_enable_api_key_uid_reporting.json"
config_generator = self.basic_config_generator + " --enable_api_key_uid_reporting"
self.run_test_with_expectation(expected_config_file, self.generated_server_config_file, config_generator)

if __name__ == '__main__':
unittest.main()
42 changes: 42 additions & 0 deletions start_esp/test/testdata/expected_enable_api_key_uid_reporting.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Auto-generated by start_esp
# Copyright (C) Extensible Service Proxy Authors
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions
# are met:
# 1. Redistributions of source code must retain the above copyright
# notice, this list of conditions and the following disclaimer.
# 2. Redistributions in binary form must reproduce the above copyright
# notice, this list of conditions and the following disclaimer in the
# documentation and/or other materials provided with the distribution.
#
# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
# SUCH DAMAGE.
service_config_rollout {
traffic_percentages {
key: "./start_esp/test/testdata/test_service_config_1.json"
value: 100
}
}
service_management_config {
url: "https://servicemanagement.googleapis.com"
}
service_control_config {
network_fail_open: true
enable_api_key_uid_reporting: true
}
experimental {
disable_log_status: true
}
rollout_strategy: "None"

Loading