Use appendAction for translated HeaderValueOptions (#8678)

* update header value options to convert append to envoy v3's append action * update tests to expect appendAction * remove focused test * Update append description * add changelog * add extra informatnion on header append/removal docs * Apply suggestions from code review Co-authored-by: Art <artberger@users.noreply.github.com> * Update docs/content/guides/traffic_management/request_processing/append_remove_headers/_index.md Co-authored-by: Art <artberger@users.noreply.github.com> * Update docs/content/guides/traffic_management/request_processing/append_remove_headers/_index.md Co-authored-by: Art <artberger@users.noreply.github.com> --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Art <artberger@users.noreply.github.com>
solo-io · Sep 20, 2023 · 3fef8d7 · 3fef8d7
1 parent 5b288e7
commit 3fef8d7
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 22 deletions.
diff --git a/changelog/v1.15.6/translate-to-envoy-append-action.yaml b/changelog/v1.15.6/translate-to-envoy-append-action.yaml
@@ -0,0 +1,7 @@
+changelog:
+  - type: NON_USER_FACING
+    issueLink: https://github.com/solo-io/gloo/issues/8525
+    resolvesIssue: false
+    description: >-
+      Translate header manipulation's `append` to Envoy's `append_action`, the newer non-deprecated field.
+      skipCI-kube-tests:true
diff --git a/...nt/guides/traffic_management/request_processing/append_remove_headers/_index.md b/...nt/guides/traffic_management/request_processing/append_remove_headers/_index.md
@@ -251,6 +251,7 @@ Headers can be inherited by children options, such as shown in the following exa
            - header:
                key: x-route-table
                value: alphabet
+             append: false # overwrite the value of `x-route-table` if it already exists
    ```
 3. In the RouteTable child object, define other headers. In the following example, the `x-route-table` header is added to requests, and the `x-route-table: a` header is added to responses.
    ```yaml
@@ -282,4 +283,13 @@ Headers can be inherited by children options, such as shown in the following exa
 4. Now, requests that match the route `/a/1` get the following headers:
    * The `x-gateway-start-time` request header is inherited from the parent VirtualHost option.
    * The `x-route-table` request header is set in the child Route option.
-   * The `x-route-table` response header in the child overwrites the parent object's value of `alphabet` instead to `a`, because child objects take precedence in case of conflict.
+   * The `x-route-table` response header in the parent overwrites the child object's value of `a` instead to `alphabet`.
+
+Due to how header manipulations are processed, less specific headers overwrite more specific headers.
+
+From the [Envoy docs](https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers#custom-request-response-headers):
+> Headers are appended to requests/responses in the following order: weighted cluster level headers, route level headers, virtual host level headers and finally global level headers.
+
+In the previous example of the `x-route-table` response header, the virtual host level header overwrites the route level header because the virtual host level header is evaluated after the route level header.
+If you set `append: true` or omit this field on the virtual host, then the route level response header (`a`) would get appended to the virtual host level header (`alphabet`).
+If you set `append: false` on the route, the route does not affect the virtual host because the route is evaluated before the virtual host. In the example, the response header would stay `x-route-table: alphabet`.
diff --git a/...ithub.com/solo-io/gloo/projects/gloo/api/v1/options/headers/headers.proto.sk.md b/...ithub.com/solo-io/gloo/projects/gloo/api/v1/options/headers/headers.proto.sk.md
diff --git a/pkg/utils/api_conversion/type.go b/pkg/utils/api_conversion/type.go
@@ -81,6 +81,13 @@ func CheckForbiddenCustomHeaders(header envoycore_sk.HeaderValue) error {
 }
 
 func ToEnvoyHeaderValueOptions(option *envoycore_sk.HeaderValueOption, secrets *v1.SecretList, secretOptions HeaderSecretOptions) ([]*envoy_config_core_v3.HeaderValueOption, error) {
+	appendAction := envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD
+	if appendOption := option.GetAppend(); appendOption != nil {
+		if appendOption.GetValue() == false {
+			appendAction = envoy_config_core_v3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD
+		}
+	}
+
 	switch typedOption := option.GetHeaderOption().(type) {
 	case *envoycore_sk.HeaderValueOption_Header:
 		if err := CheckForbiddenCustomHeaders(*typedOption.Header); err != nil {
@@ -92,7 +99,7 @@ func ToEnvoyHeaderValueOptions(option *envoycore_sk.HeaderValueOption, secrets *
 					Key:   typedOption.Header.GetKey(),
 					Value: typedOption.Header.GetValue(),
 				},
-				Append: option.GetAppend(),
+				AppendAction: appendAction,
 			},
 		}, nil
 	case *envoycore_sk.HeaderValueOption_HeaderSecretRef:
@@ -117,7 +124,7 @@ func ToEnvoyHeaderValueOptions(option *envoycore_sk.HeaderValueOption, secrets *
 					Key:   key,
 					Value: value,
 				},
-				Append: option.GetAppend(),
+				AppendAction: appendAction,
 			})
 		}
 		return result, nil

diff --git a/pkg/utils/api_conversion/type_test.go b/pkg/utils/api_conversion/type_test.go
@@ -33,9 +33,7 @@ var _ = Describe("Type conversion", func() {
 						Key:   "allowed",
 						Value: "header",
 					},
-					Append: &wrappers.BoolValue{
-						Value: true,
-					},
+					AppendAction: envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD,
 				},
 			}
 			headers, err := ToEnvoyHeaderValueOptionList(allowedHeaders, nil, HeaderSecretOptions{})

diff --git a/projects/gloo/api/v1/options/headers/headers.proto b/projects/gloo/api/v1/options/headers/headers.proto
@@ -46,8 +46,9 @@ message HeaderValueOption {
     // Header name/value pair that this option applies to.
     HeaderValue header = 1;
 
-    // Should the value be appended? If true (default), the value is appended to
-    // existing values.
+    // Specifies if the value should be appended or overwrite an existing header. Defaults to true.
+    // If set to true, this maps to Envoy's `append_value: APPEND_IF_EXISTS_OR_ADD`, where the value gets be appended the header's value(s) if exists, or created if it does not.
+    // If set to false, this maps to Envoy's `append_value: OVERWRITE_IF_EXISTS_OR_ADD`, where the header's value will be overwritten if it exists, or created if it does not.
     google.protobuf.BoolValue append = 2;
 }
 

diff --git a/projects/gloo/pkg/api/v1/options/headers/headers.pb.go b/projects/gloo/pkg/api/v1/options/headers/headers.pb.go
diff --git a/projects/gloo/pkg/plugins/headers/plugin.go b/projects/gloo/pkg/plugins/headers/plugin.go
@@ -247,12 +247,19 @@ func convertResponseHeaderValueOption(
 			return nil, CantSetHostHeaderError
 		}
 
+		appendAction := envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD
+		if appendOption := h.GetAppend(); appendOption != nil {
+			if appendOption.GetValue() == false {
+				appendAction = envoy_config_core_v3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD
+			}
+		}
+
 		out = append(out, &envoy_config_core_v3.HeaderValueOption{
 			Header: &envoy_config_core_v3.HeaderValue{
 				Key:   header.GetKey(),
 				Value: header.GetValue(),
 			},
-			Append: h.GetAppend(),
+			AppendAction: appendAction,
 		})
 	}
 	return out, nil

diff --git a/projects/gloo/pkg/plugins/headers/plugin_test.go b/projects/gloo/pkg/plugins/headers/plugin_test.go
@@ -299,9 +299,9 @@ var testHeaderManip = &headers.HeaderManipulation{
 }
 
 var expectedHeaders = envoyHeaderManipulation{
-	RequestHeadersToAdd:     []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "foo", Value: "bar"}, Append: &wrappers.BoolValue{Value: true}}},
+	RequestHeadersToAdd:     []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "foo", Value: "bar"}, AppendAction: envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD}},
 	RequestHeadersToRemove:  []string{"a"},
-	ResponseHeadersToAdd:    []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "foo", Value: "bar"}, Append: &wrappers.BoolValue{Value: true}}},
+	ResponseHeadersToAdd:    []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "foo", Value: "bar"}, AppendAction: envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD}},
 	ResponseHeadersToRemove: []string{"b"},
 }
 
@@ -314,8 +314,8 @@ var testHeaderManipWithSecrets = &headers.HeaderManipulation{
 }
 
 var expectedHeadersWithSecrets = envoyHeaderManipulation{
-	RequestHeadersToAdd:     []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "Authorization", Value: "basic dXNlcjpwYXNzd29yZA=="}, Append: &wrappers.BoolValue{Value: true}}},
+	RequestHeadersToAdd:     []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "Authorization", Value: "basic dXNlcjpwYXNzd29yZA=="}, AppendAction: envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD}},
 	RequestHeadersToRemove:  []string{"a"},
-	ResponseHeadersToAdd:    []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "foo", Value: "bar"}, Append: &wrappers.BoolValue{Value: true}}},
+	ResponseHeadersToAdd:    []*envoy_config_core_v3.HeaderValueOption{{Header: &envoy_config_core_v3.HeaderValue{Key: "foo", Value: "bar"}, AppendAction: envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD}},
 	ResponseHeadersToRemove: []string{"b"},
 }
diff --git a/projects/gloo/pkg/translator/translator_test.go b/projects/gloo/pkg/translator/translator_test.go
@@ -1277,9 +1277,7 @@ var _ = Describe("Translator", func() {
 							Key:   "Authorization",
 							Value: "basic dXNlcjpwYXNzd29yZA==",
 						},
-						Append: &wrappers.BoolValue{
-							Value: true,
-						},
+						AppendAction: envoy_config_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD,
 					},
 				}
 
@@ -2754,9 +2752,7 @@ var _ = Describe("Translator", func() {
 						Key:   "client-id",
 						Value: "%REQ(client-id)%",
 					},
-					Append: &wrappers.BoolValue{
-						Value: false,
-					},
+					AppendAction: envoy_config_core_v3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD,
 				},
 			))
 		})