orca: fix the delimiter of the native http parser (#37724)

Commit Message: orca: fix the delimiter of the native http parser Additional Description: This should also be cherry-pick to 1.32 to fix this problem. The previous ORCA parser will use ``:`` as the delimiter of key/value pair in the native HTTP report. This is wrong based on the design document. The correct delimiter should be ``=``. This change fixes the delimiter to ``=``. See #6614 and related documents for more details. https://docs.google.com/document/d/1NSnK3346BkBo1JUU3I9I5NYYnaJZQPt8_Z_XCBCI3uA/edit?tab=t.0 ``` Native HTTP header encoding. Similar to the JSON dictionary format, we will define Endpoint-Load-Metrics as having key=value format and use standard HTTP header encoding to either send the metrics as a sequence of HTTP headers or as a single header with [comma separated](https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2) key-value pairs. E.g. endpoint-load-metrics: TEXT cpu=0.3,mem=0.8,foo_bytes=123. ``` Risk Level: n/a. Testing: n/a. Docs Changes: n/a. Release Notes: added. --------- Signed-off-by: wangbaiping(wbpcode) <[email protected]>
envoyproxy · Dec 19, 2024 · 70b7478 · 70b7478
1 parent 4fa73a8
commit 70b7478
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 10 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -163,6 +163,11 @@ bug_fixes:
 - area: csrf
   change: |
     Handle requests that have a "privacy sensitive" / opaque origin (``Origin: null``) as if the request had no origin information.
+- area: orca
+  change: |
+    The previous ORCA parser will use ``:`` as the delimiter of key/value pair in the native HTTP report. This is wrong
+    based on the design document. The correct delimiter should be ``=``. This change add the ``=`` delimiter support to
+    match the design document and keep the ``:`` delimiter for backward compatibility.
 - area: http/1
   change: |
     Fixes sending overload crashes when HTTP/1 request is reset.

diff --git a/source/common/orca/orca_parser.cc b/source/common/orca/orca_parser.cc
@@ -133,7 +133,7 @@ absl::Status tryParseNativeHttpEncoded(const absl::string_view header,
   absl::flat_hash_set<absl::string_view> metric_names;
   for (const auto value : values) {
     std::pair<absl::string_view, absl::string_view> entry =
-        absl::StrSplit(value, absl::MaxSplits(':', 1), absl::SkipWhitespace());
+        absl::StrSplit(value, absl::MaxSplits(absl::ByAnyChar("=:"), 1), absl::SkipWhitespace());
     if (metric_names.contains(entry.first)) {
       return absl::AlreadyExistsError(
           absl::StrCat(kEndpointLoadMetricsHeader, " contains duplicate metric: ", entry.first));

diff --git a/test/common/orca/orca_parser_test.cc b/test/common/orca/orca_parser_test.cc
@@ -70,6 +70,17 @@ TEST(OrcaParserUtilTest, EmptyOrcaHeader) {
 }
 
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeader) {
+  Http::TestRequestHeaderMapImpl headers{
+      {std::string(kEndpointLoadMetricsHeader),
+       absl::StrCat(kHeaderFormatPrefixText,
+                    "cpu_utilization=0.7,application_utilization=0.8,mem_utilization=0.9,"
+                    "rps_fractional=1000,eps=2,"
+                    "named_metrics.foo=123,named_metrics.bar=0.2,utilization.total=0.5")}};
+  EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
+              StatusHelpers::IsOkAndHolds(ProtoEq(exampleOrcaLoadReport())));
+}
+
+TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderWithColon) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
        absl::StrCat(kHeaderFormatPrefixText,
@@ -83,7 +94,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeader) {
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderIncorrectFieldType) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
-       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization:\"0.7\"")}};
+       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization=\"0.7\"")}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(
                   absl::InvalidArgumentError("unable to parse custom backend load metric "
@@ -92,7 +103,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderIncorrectFieldType) {
 
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderNanMetricValue) {
   Http::TestRequestHeaderMapImpl headers{{std::string(kEndpointLoadMetricsHeader),
-                                          absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization:",
+                                          absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization=",
                                                        std::numeric_limits<double>::quiet_NaN())}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(absl::InvalidArgumentError(
@@ -101,7 +112,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderNanMetricValue) {
 
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderInfinityMetricValue) {
   Http::TestRequestHeaderMapImpl headers{{std::string(kEndpointLoadMetricsHeader),
-                                          absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization:",
+                                          absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization=",
                                                        std::numeric_limits<double>::infinity())}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(absl::InvalidArgumentError(
@@ -112,7 +123,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderInfinityMetricValue) {
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderNegativeMetricValue) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
-       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization:-1")}};
+       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization=-1")}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(absl::InvalidArgumentError(
                   "custom backend load metric value(cpu_utilization) cannot be negative.")));
@@ -121,7 +132,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderNegativeMetricValue) {
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderContainsDuplicateMetric) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
-       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization:0.7,cpu_utilization:0.8")}};
+       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization=0.7,cpu_utilization=0.8")}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(absl::AlreadyExistsError(absl::StrCat(
                   kEndpointLoadMetricsHeader, " contains duplicate metric: cpu_utilization"))));
@@ -130,7 +141,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderContainsDuplicateMetric) {
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderUnsupportedMetric) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
-       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization:0.7,unsupported_metric:0.8")}};
+       absl::StrCat(kHeaderFormatPrefixText, "cpu_utilization=0.7,unsupported_metric=0.8")}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(
                   absl::InvalidArgumentError("unsupported metric name: unsupported_metric")));
@@ -139,7 +150,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderUnsupportedMetric) {
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderContainsEmptyUtilizationMetricKey) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
-       absl::StrCat(kHeaderFormatPrefixText, "utilization.:0.9")}};
+       absl::StrCat(kHeaderFormatPrefixText, "utilization.=0.9")}};
   EXPECT_THAT(
       parseOrcaLoadReportHeaders(headers),
       StatusHelpers::HasStatus(absl::InvalidArgumentError("utilization metric key is empty.")));
@@ -150,7 +161,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderContainsDuplicateNamedMetric) {
       {std::string(kEndpointLoadMetricsHeader),
        absl::StrCat(
            kHeaderFormatPrefixText,
-           "named_metrics.foo:123,named_metrics.duplicate:123,named_metrics.duplicate:0.2")}};
+           "named_metrics.foo=123,named_metrics.duplicate=123,named_metrics.duplicate=0.2")}};
   EXPECT_THAT(
       parseOrcaLoadReportHeaders(headers),
       StatusHelpers::HasStatus(absl::AlreadyExistsError(absl::StrCat(
@@ -160,7 +171,7 @@ TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderContainsDuplicateNamedMetric) {
 TEST(OrcaParserUtilTest, NativeHttpEncodedHeaderContainsEmptyNamedMetricKey) {
   Http::TestRequestHeaderMapImpl headers{
       {std::string(kEndpointLoadMetricsHeader),
-       absl::StrCat(kHeaderFormatPrefixText, "named_metrics.:123")}};
+       absl::StrCat(kHeaderFormatPrefixText, "named_metrics.=123")}};
   EXPECT_THAT(parseOrcaLoadReportHeaders(headers),
               StatusHelpers::HasStatus(absl::InvalidArgumentError("named metric key is empty.")));
 }