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.

Sign up for GitHub

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 a flag to opt out reporting api key uid and report unknown in network error. #897

Merged
merged 6 commits into from
May 16, 2024
Merged

Add a flag to opt out reporting api key uid and report unknown in network error. #897

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
632987a
Add flag to report api key uid and report unknown when the check is
Elliot-xq May 2, 2024
e1e47f9
Fix the unknown logic
Elliot-xq May 2, 2024
c6c1434
change from report_api_key_uid to enable_api_key_uid_reporting
Elliot-xq May 2, 2024
f6dce14
fix the format for api key uid
Elliot-xq May 2, 2024
2b28e6c
Merge branch 'master' into kangda-workspace
Elliot-xq May 10, 2024
b6995b1
fix testing
Elliot-xq May 16, 2024
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
3 changes: 3 additions & 0 deletions api/envoy/v12/http/service_control/config.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ message FilterConfig {
// How the filter config will handle failures when fetching access tokens.
espv2.api.envoy.v12.http.common.DependencyErrorBehavior dep_error_behavior =
10;

// If true, reports api_key_uid instead of api_key in ServiceControl report.
bool report_api_key_uid = 11;
}

message PerRouteFilterConfig {
Expand Down
10 changes: 9 additions & 1 deletion src/api_proxy/service_control/request_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,16 @@ Status set_credential_id(const SupportedLabel& l, const ReportRequestInfo& info,
api_key::ApiKeyState::VERIFIED) {
ASSERT(!info.api_key.empty(),
"API Key must be set, otherwise consumer would not be verified.");
(*labels)[l.name] = absl::StrCat(kApiKeyPrefix, info.check_response_info.api_key_uid.empty() ? info.api_key
if (info.report_api_key_uid) {
if (!info.check_response_info.error.name.empty()) {
(*labels)[l.name] = absl::StrCat(kApiKeyPrefix, "UNKNOWN");
} else {
(*labels)[l.name] = absl::StrCat(kApiKeyPrefix, info.check_response_info.api_key_uid.empty() ? info.api_key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still report API key if the flag is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, according to Chemist team, for some old apis, they don't have api_key_uid, thus we still need to report api_key when api_key_uid is empty.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Thanks.

: info.check_response_info.api_key_uid);
Elliot-xq marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
(*labels)[l.name] = absl::StrCat(kApiKeyPrefix, info.api_key);
}
} else if (!info.auth_issuer.empty()) {
std::string base64_issuer = Envoy::Base64Url::encode(
info.auth_issuer.data(), info.auth_issuer.size());
Expand Down
56 changes: 56 additions & 0 deletions src/api_proxy/service_control/request_builder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@ TEST_F(RequestBuilderTest, ReportApiKeyVerifiedWithApiKeyUIDTest) {

info.check_response_info.api_key_state = api_key::ApiKeyState::VERIFIED;
info.check_response_info.api_key_uid = "fake_api_key_uid";
info.report_api_key_uid = true;

gasv1::ReportRequest request;
ASSERT_TRUE(scp_.FillReportRequest(info, &request).ok());
Expand All @@ -451,6 +452,61 @@ TEST_F(RequestBuilderTest, ReportApiKeyVerifiedWithApiKeyUIDTest) {
ASSERT_EQ(fields.at("api_key").string_value(), "api_key_x");
}

TEST_F(RequestBuilderTest, ReportApiKeyVerifiedWithApiKeyUIDUnknownTest) {
ReportRequestInfo info;
FillOperationInfo(&info);

info.check_response_info.api_key_state = api_key::ApiKeyState::VERIFIED;
info.check_response_info.api_key_uid = "fake_api_key_uid";
info.report_api_key_uid = true;
info.check_response_info.error = {"RESOURCE_EXHAUSTED", false,
ScResponseErrorType::CONSUMER_QUOTA};

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

// Credential id is filled.
ASSERT_TRUE(request.operations(0).labels().contains("/credential_id"));
ASSERT_EQ(request.operations(0).labels().at("/credential_id"),
"apikey:UNKNOWN");

// Consumer id is filled.
ASSERT_EQ(request.operations(0).consumer_id(), "api_key:api_key_x");

// Log entry is filled.
const gasv1::LogEntry log_entry = request.operations(0).log_entries(0);
const auto fields = log_entry.struct_payload().fields();
ASSERT_TRUE(fields.contains("api_key"));
ASSERT_EQ(fields.at("api_key").string_value(), "api_key_x");
}

TEST_F(RequestBuilderTest, ReportApiKeyVerifiedNotReportApiKeyUIDTest) {
ReportRequestInfo info;
FillOperationInfo(&info);

info.check_response_info.api_key_state = api_key::ApiKeyState::VERIFIED;
info.check_response_info.api_key_uid = "fake_api_key_uid";
info.report_api_key_uid = false;

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

// Credential id is filled.
ASSERT_TRUE(request.operations(0).labels().contains("/credential_id"));
ASSERT_EQ(request.operations(0).labels().at("/credential_id"),
"apikey:api_key_x");

// Consumer id is filled.
ASSERT_EQ(request.operations(0).consumer_id(), "api_key:api_key_x");

// Log entry is filled.
const gasv1::LogEntry log_entry = request.operations(0).log_entries(0);
const auto fields = log_entry.struct_payload().fields();
ASSERT_TRUE(fields.contains("api_key"));
ASSERT_EQ(fields.at("api_key").string_value(), "api_key_x");
}


TEST_F(RequestBuilderTest, ReportApiKeyNotVerifiedTest) {
ReportRequestInfo info;
FillOperationInfo(&info);
Expand Down
6 changes: 5 additions & 1 deletion src/api_proxy/service_control/request_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,13 +273,17 @@ struct ReportRequestInfo : public OperationInfo {
// Trace id (in hex) the request is tied to.
std::string trace_id;

// If true, reports api key uid instead of api key.
bool report_api_key_uid;

ReportRequestInfo()
: http_response_code(0),
request_size(-1),
response_size(-1),
frontend_protocol(protocol::UNKNOWN),
backend_protocol(protocol::UNKNOWN),
compute_platform("UNKNOWN(ESPv2)") {}
compute_platform("UNKNOWN(ESPv2)"),
report_api_key_uid(false) {}
};

} // namespace service_control
Expand Down
3 changes: 3 additions & 0 deletions src/envoy/http/service_control/handler_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ void ServiceControlHandlerImpl::prepareReportRequest(

info.tracing_project_id =
require_ctx_->service_ctx().config().tracing_project_id();

info.report_api_key_uid =
cfg_parser_.config().report_api_key_uid();
}

void ServiceControlHandlerImpl::callCheck(
Expand Down
4 changes: 4 additions & 0 deletions src/go/configgenerator/filtergen/service_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type ServiceControlGenerator struct {
MethodRequirements []*scpb.Requirement
CallingConfig *scpb.ServiceControlCallingConfig
GCPAttributes *scpb.GcpAttributes
ReportApiKeyUid bool

NoopFilterGenerator
}
Expand Down Expand Up @@ -140,6 +141,7 @@ func NewServiceControlFilterGensFromOPConfig(serviceConfig *confpb.Service, opts
MethodRequirements: requirements,
CallingConfig: MakeSCCallingConfigFromOPConfig(opts),
GCPAttributes: params.GCPAttributes,
ReportApiKeyUid: opts.ServiceControlReportApiKeyUid,
},
}, nil
}
Expand Down Expand Up @@ -208,6 +210,7 @@ func (g *ServiceControlGenerator) GenFilterConfig() (proto.Message, error) {
},
GeneratedHeaderPrefix: g.GeneratedHeaderPrefix,
Requirements: g.MethodRequirements,
ReportApiKeyUid: g.ReportApiKeyUid,
}

accessTokenConfig := g.AccessToken.MakeAccessTokenConfig()
Expand Down Expand Up @@ -248,6 +251,7 @@ func (g *ServiceControlGenerator) GenFilterConfig() (proto.Message, error) {
}

filterConfig.DepErrorBehavior = depErrorBehaviorEnum

return filterConfig, nil
}

Expand Down
1 change: 1 addition & 0 deletions src/go/configgenerator/filtergen/service_control_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ func TestNewServiceControlFilterGensFromOPConfig_GenConfig(t *testing.T) {
ScCheckTimeoutMs: 5020,
ScQuotaRetries: 8,
ServiceControlNetworkFailOpen: false,
ServiceControlReportApiKeyUid: false,
},
WantFilterConfigs: []string{`
{
Expand Down
2 changes: 2 additions & 0 deletions src/go/configmanager/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ var (

ServiceControlNetworkFailOpen = flag.Bool("service_control_network_fail_open", defaults.ServiceControlNetworkFailOpen, ` In case of network failures when connecting to Google service control,
the requests will be allowed if this flag is on. The default is on.`)
ServiceControlReportApiKeyUid = flag.Bool("service_control_report_api_key_uid", defaults.ServiceControlReportApiKeyUid, ` If true, reports api_key_uid instead of api_key in ServiceControl report.`)

EnableGrpcForHttp1 = flag.Bool("enable_grpc_for_http1", defaults.EnableGrpcForHttp1, `Enable gRPC when the downstream is HTTP/1.1. The default is on.`)

Expand Down Expand Up @@ -287,6 +288,7 @@ func EnvoyConfigOptionsFromFlags() options.ConfigGeneratorOptions {
MergeSlashesInPath: *MergeSlashesInPath,
DisallowEscapedSlashesInPath: *DisallowEscapedSlashesInPath,
ServiceControlNetworkFailOpen: *ServiceControlNetworkFailOpen,
ServiceControlReportApiKeyUid: *ServiceControlReportApiKeyUid,
EnableGrpcForHttp1: *EnableGrpcForHttp1,
ConnectionBufferLimitBytes: *ConnectionBufferLimitBytes,
DisableJwksAsyncFetch: *DisableJwksAsyncFetch,
Expand Down
2 changes: 2 additions & 0 deletions src/go/options/configgenerator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ type ConfigGeneratorOptions struct {
MergeSlashesInPath bool
DisallowEscapedSlashesInPath bool
ServiceControlNetworkFailOpen bool
ServiceControlReportApiKeyUid bool
EnableGrpcForHttp1 bool
ConnectionBufferLimitBytes int

Expand Down Expand Up @@ -194,6 +195,7 @@ func DefaultConfigGeneratorOptions() ConfigGeneratorOptions {
MergeSlashesInPath: true,
DisallowEscapedSlashesInPath: false,
ServiceControlNetworkFailOpen: true,
ServiceControlReportApiKeyUid: false,
EnableGrpcForHttp1: true,
ConnectionBufferLimitBytes: -1,
ServiceManagementURL: "https://servicemanagement.googleapis.com",
Expand Down
2 changes: 2 additions & 0 deletions tests/start_proxy/start_proxy_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ def test_gen_proxy_config(self):
'--disable_tracing'
]),
# service_control_network_fail_policy=close
# service_control_report_api_key_uid=false
(['-R=managed','--enable_strict_transport_security',
'--http_port=8079', '--service_control_quota_retries=3',
'--service_control_report_timeout_ms=300',
Expand All @@ -217,6 +218,7 @@ def test_gen_proxy_config(self):
'--service_control_quota_retries', '3',
'--service_control_report_timeout_ms', '300',
'--service_control_network_fail_open=false',
'--service_control_report_api_key_uid=false',
'--check_metadata', '--underscores_in_headers',
'--disable_tracing'
]),
Expand Down