Add a flag to opt out reporting api key uid and report unknown in net…

…work error. (#897)

* Add flag to report api key uid and report unknown when the check is
failed.

* Fix the unknown logic

* change from report_api_key_uid to enable_api_key_uid_reporting

* fix the format for api key uid

* fix testing

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 {

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])
 

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());

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);

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

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(

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
 }
 

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{`
 {

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,

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",

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'
               ]),