diff --git a/api/envoy/v12/http/service_control/config.proto b/api/envoy/v12/http/service_control/config.proto index f57222775..4e788b356 100644 --- a/api/envoy/v12/http/service_control/config.proto +++ b/api/envoy/v12/http/service_control/config.proto @@ -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 enable_api_key_uid_reporting = 11; } message PerRouteFilterConfig { diff --git a/docker/generic/start_proxy.py b/docker/generic/start_proxy.py index 1ac92f846..80bc895bf 100644 --- a/docker/generic/start_proxy.py +++ b/docker/generic/start_proxy.py @@ -577,6 +577,11 @@ def make_argparser(): connecting to Google service control. If it is `open`, the request will be allowed, otherwise, it will be rejected. Default is `open`. ''') + parser.add_argument('--service_control_enable_api_key_uid_reporting', + default=False, + help=''' + Enable when need to report api_key_uid in the telemetry report.''' + ) parser.add_argument( '--disable_jwks_async_fetch', action='store_true', @@ -1382,6 +1387,12 @@ def gen_proxy_config(args): if args.service_control_network_fail_policy == "close": proxy_conf.extend(["--service_control_network_fail_open=false"]) + if args.service_control_enable_api_key_uid_reporting: + proxy_conf.extend([ + "--service_control_enable_api_key_uid_reporting", + args.service_control_enable_api_key_uid_reporting + ]) + if args.service_json_path: proxy_conf.extend(["--service_json_path", args.service_json_path]) diff --git a/src/api_proxy/service_control/request_builder.cc b/src/api_proxy/service_control/request_builder.cc index 708ce11d5..4ce5ecf5a 100644 --- a/src/api_proxy/service_control/request_builder.cc +++ b/src/api_proxy/service_control/request_builder.cc @@ -383,10 +383,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 - : info.check_response_info.api_key_uid); + if (info.enable_api_key_uid_reporting) { + if (info.check_response_info.error.is_network_error) { + (*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 { + (*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()); diff --git a/src/api_proxy/service_control/request_builder_test.cc b/src/api_proxy/service_control/request_builder_test.cc index f48311cd4..669af9841 100644 --- a/src/api_proxy/service_control/request_builder_test.cc +++ b/src/api_proxy/service_control/request_builder_test.cc @@ -431,7 +431,8 @@ TEST_F(RequestBuilderTest, ReportApiKeyVerifiedWithApiKeyUIDTest) { 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.check_response_info.api_key_uid = "apikey:fake_api_key_uid"; + info.enable_api_key_uid_reporting = true; gasv1::ReportRequest request; ASSERT_TRUE(scp_.FillReportRequest(info, &request).ok()); @@ -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.enable_api_key_uid_reporting = true; + info.check_response_info.error = {"UNREACHABLE", true, + 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.enable_api_key_uid_reporting = 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); diff --git a/src/api_proxy/service_control/request_info.h b/src/api_proxy/service_control/request_info.h index 2a3879afa..0b000c111 100644 --- a/src/api_proxy/service_control/request_info.h +++ b/src/api_proxy/service_control/request_info.h @@ -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 enable_api_key_uid_reporting; + 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)"), + enable_api_key_uid_reporting(false) {} }; } // namespace service_control diff --git a/src/envoy/http/service_control/handler_impl.cc b/src/envoy/http/service_control/handler_impl.cc index 550a4bd1b..864566faa 100644 --- a/src/envoy/http/service_control/handler_impl.cc +++ b/src/envoy/http/service_control/handler_impl.cc @@ -188,6 +188,9 @@ void ServiceControlHandlerImpl::prepareReportRequest( info.tracing_project_id = require_ctx_->service_ctx().config().tracing_project_id(); + + info.enable_api_key_uid_reporting = + cfg_parser_.config().enable_api_key_uid_reporting(); } void ServiceControlHandlerImpl::callCheck( diff --git a/src/go/configgenerator/filtergen/service_control.go b/src/go/configgenerator/filtergen/service_control.go index 9872905f1..d988e3d5d 100644 --- a/src/go/configgenerator/filtergen/service_control.go +++ b/src/go/configgenerator/filtergen/service_control.go @@ -74,9 +74,10 @@ type ServiceControlGenerator struct { ComputePlatformOverride string // Service control configs. - MethodRequirements []*scpb.Requirement - CallingConfig *scpb.ServiceControlCallingConfig - GCPAttributes *scpb.GcpAttributes + MethodRequirements []*scpb.Requirement + CallingConfig *scpb.ServiceControlCallingConfig + GCPAttributes *scpb.GcpAttributes + EnableApiKeyUidReporting bool NoopFilterGenerator } @@ -140,6 +141,7 @@ func NewServiceControlFilterGensFromOPConfig(serviceConfig *confpb.Service, opts MethodRequirements: requirements, CallingConfig: MakeSCCallingConfigFromOPConfig(opts), GCPAttributes: params.GCPAttributes, + EnableApiKeyUidReporting: opts.ServiceControlEnableApiKeyUidReporting, }, }, nil } @@ -206,8 +208,9 @@ func (g *ServiceControlGenerator) GenFilterConfig() (proto.Message, error) { Cluster: clustergen.ServiceControlClusterName, Timeout: durationpb.New(g.HttpRequestTimeout), }, - GeneratedHeaderPrefix: g.GeneratedHeaderPrefix, - Requirements: g.MethodRequirements, + GeneratedHeaderPrefix: g.GeneratedHeaderPrefix, + Requirements: g.MethodRequirements, + EnableApiKeyUidReporting: g.EnableApiKeyUidReporting, } accessTokenConfig := g.AccessToken.MakeAccessTokenConfig() @@ -248,6 +251,7 @@ func (g *ServiceControlGenerator) GenFilterConfig() (proto.Message, error) { } filterConfig.DepErrorBehavior = depErrorBehaviorEnum + return filterConfig, nil } diff --git a/src/go/configgenerator/filtergen/service_control_test.go b/src/go/configgenerator/filtergen/service_control_test.go index 14cc621c6..7b58798b1 100644 --- a/src/go/configgenerator/filtergen/service_control_test.go +++ b/src/go/configgenerator/filtergen/service_control_test.go @@ -230,16 +230,17 @@ func TestNewServiceControlFilterGensFromOPConfig_GenConfig(t *testing.T) { HttpRequestTimeout: 2 * time.Minute, GeneratedHeaderPrefix: "X-Test-Header-", }, - DependencyErrorBehavior: commonpb.DependencyErrorBehavior_ALWAYS_INIT.String(), - ClientIPFromForwardedHeader: true, - LogRequestHeaders: ":method", - LogResponseHeaders: ":status", - LogJwtPayloads: "my-payload", - MinStreamReportIntervalMs: 8000, - ComputePlatformOverride: "ESPv2(Cloud Run)", - ScCheckTimeoutMs: 5020, - ScQuotaRetries: 8, - ServiceControlNetworkFailOpen: false, + DependencyErrorBehavior: commonpb.DependencyErrorBehavior_ALWAYS_INIT.String(), + ClientIPFromForwardedHeader: true, + LogRequestHeaders: ":method", + LogResponseHeaders: ":status", + LogJwtPayloads: "my-payload", + MinStreamReportIntervalMs: 8000, + ComputePlatformOverride: "ESPv2(Cloud Run)", + ScCheckTimeoutMs: 5020, + ScQuotaRetries: 8, + ServiceControlNetworkFailOpen: false, + ServiceControlEnableApiKeyUidReporting: false, }, WantFilterConfigs: []string{` { diff --git a/src/go/configmanager/flags/flags.go b/src/go/configmanager/flags/flags.go index fdb7a7331..41f6f44ff 100644 --- a/src/go/configmanager/flags/flags.go +++ b/src/go/configmanager/flags/flags.go @@ -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.`) + ServiceControlEnableApiKeyUidReporting = flag.Bool("service_control_enable_api_key_uid_reporting", defaults.ServiceControlEnableApiKeyUidReporting, ` 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.`) @@ -287,6 +288,7 @@ func EnvoyConfigOptionsFromFlags() options.ConfigGeneratorOptions { MergeSlashesInPath: *MergeSlashesInPath, DisallowEscapedSlashesInPath: *DisallowEscapedSlashesInPath, ServiceControlNetworkFailOpen: *ServiceControlNetworkFailOpen, + ServiceControlEnableApiKeyUidReporting: *ServiceControlEnableApiKeyUidReporting, EnableGrpcForHttp1: *EnableGrpcForHttp1, ConnectionBufferLimitBytes: *ConnectionBufferLimitBytes, DisableJwksAsyncFetch: *DisableJwksAsyncFetch, diff --git a/src/go/options/configgenerator.go b/src/go/options/configgenerator.go index 53b4c6210..106365799 100644 --- a/src/go/options/configgenerator.go +++ b/src/go/options/configgenerator.go @@ -109,14 +109,15 @@ type ConfigGeneratorOptions struct { LogResponseHeaders string MinStreamReportIntervalMs uint64 - SuppressEnvoyHeaders bool - UnderscoresInHeaders bool - NormalizePath bool - MergeSlashesInPath bool - DisallowEscapedSlashesInPath bool - ServiceControlNetworkFailOpen bool - EnableGrpcForHttp1 bool - ConnectionBufferLimitBytes int + SuppressEnvoyHeaders bool + UnderscoresInHeaders bool + NormalizePath bool + MergeSlashesInPath bool + DisallowEscapedSlashesInPath bool + ServiceControlNetworkFailOpen bool + ServiceControlEnableApiKeyUidReporting bool + EnableGrpcForHttp1 bool + ConnectionBufferLimitBytes int // JwtAuthn related flags DisableJwksAsyncFetch bool @@ -194,6 +195,7 @@ func DefaultConfigGeneratorOptions() ConfigGeneratorOptions { MergeSlashesInPath: true, DisallowEscapedSlashesInPath: false, ServiceControlNetworkFailOpen: true, + ServiceControlEnableApiKeyUidReporting: false, EnableGrpcForHttp1: true, ConnectionBufferLimitBytes: -1, ServiceManagementURL: "https://servicemanagement.googleapis.com", diff --git a/tests/start_proxy/start_proxy_test.py b/tests/start_proxy/start_proxy_test.py index 6d9fdfa19..ae6df6a6e 100644 --- a/tests/start_proxy/start_proxy_test.py +++ b/tests/start_proxy/start_proxy_test.py @@ -192,16 +192,19 @@ def test_gen_proxy_config(self): '--disable_tracing' ]), # service_control_network_fail_policy=open + # service_control_enable_api_key_uid_reporting=true (['-R=managed','--enable_strict_transport_security', '--http_port=8079', '--service_control_quota_retries=3', '--service_control_report_timeout_ms=300', '--service_control_network_fail_policy=open', '--check_metadata', + '--service_control_enable_api_key_uid_reporting=true', '--disable_tracing', '--underscores_in_headers'], ['bin/configmanager', '--logtostderr', '--rollout_strategy', 'managed', '--backend_address', 'http://127.0.0.1:8082', '--v', '0', '--listener_port', '8079', '--enable_strict_transport_security', '--service_control_quota_retries', '3', '--service_control_report_timeout_ms', '300', + '--service_control_enable_api_key_uid_reporting', 'true', '--check_metadata', '--underscores_in_headers', '--disable_tracing' ]),