Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494

kevin85421 · 2024-01-21T23:42:52Z

Proposed changes

Problem: Users want to add, set, and remove response headers.

Solution: Use add_header NGINX directive to support ResponseHeaderModifier.

If the action is set, we simply configure the add_header directive with the given value in the HTTPRoute spec.
If the action is remove, we configure the add_header directive with the value set to an empty string.
If the action is add, we create a mapping for the $http_header_name variable appending a comma at the end of any client provided headers (if present) and prepend this to the given value in the HTTPRoute spec.

Testing:

Unit tests: I will add unit tests after I receive some early feedback.

Manual test

Add the following ResponseHeaderModifier to echo-route.yaml.

      - type: ResponseHeaderModifier
        responseHeaderModifier:
          set:
          - name: My-Overwrite-Header
            value: response-this-is-the-only-value
          add:
          - name: Accept-Encoding
            value: response-accept-encoding
          - name: My-cool-header
            value: response-appended-value
          remove:
          - User-Agent

Follow this example to install the backend server, HTTPRoute, and Gateway.

Port forwarding

kubectl -n nginx-gateway port-forward $NGF_POD 8080:80 8443:443

Send a request

curl -v --resolve echo.example.com:$GW_PORT:$GW_IP http://echo.example.com:$GW_PORT/headers -H "My-Cool-Header:my-client-value" -H "My-Overwrite-Header:dont-see-this"

Check the output of the curl command:

* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Server: nginx/1.25.3
< Date: Mon, 22 Jan 2024 07:28:07 GMT
< Content-Type: text/plain
< Content-Length: 327
< Connection: keep-alive
< Accept-Encoding: response-accept-encoding
< My-cool-header: my-client-value,response-appended-value
< My-Overwrite-Header: response-this-is-the-only-value
< Host: echo.example.com:8080
< X-Forwarded-For: 127.0.0.1
< Connection: close
<
Headers:
 header 'Accept-Encoding' is 'compress'
 header 'My-cool-header' is 'my-client-value,this-is-an-appended-value'
 header 'My-Overwrite-Header' is 'this-is-the-only-value'
 header 'Host' is 'echo.example.com:8080'
 header 'X-Forwarded-For' is '127.0.0.1'
 header 'Connection' is 'close'
 header 'Accept' is '*/*'

Closes #1397

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

internal/mode/static/nginx/config/http/config.go

internal/mode/static/nginx/config/servers.go

Problem: Users want to add, set, and remove response headers. Solution: Use `add_header` NGINX directive to support `ResponseHeaderModifier`. * If the action is `set`, we simply configure the `add_header` directive with the given value in the HTTPRoute spec. * If the action is `remove`, we configure the `add_header` directive with the value set to an empty string. * If the action is `add`, we create a mapping for the `$http_header_name` variable appending a comma at the end of any client provided headers (if present) and prepend this to the given value in the HTTPRoute spec.

kate-osborn · 2024-01-24T17:44:07Z

@kevin85421 thanks for working on this!

We have three actions for ResponseHeaderModifier: add, set, and remove. We also need to consider the cases where the header we are performing an action on already exists in the response headers. This gives us the following table:

Case	Action	Header exists?	Expected behavior
1	add	no	set header to value
2	add	yes	append to existing header value
3	set	no	set header to value
4	set	yes	overwrite existing header value
5	remove	no	unset header (no-op)
6	remove	yes	unset header

With your current implementation, cases 1, 2, and 3 work as expected, but the rest do not.

There are a couple of reasons for that.

(1) In the manual test you described, you check the request headers instead of the response headers. More on testing later.

(2) The add_header NGINX directive works differently from the proxy_set_header directive.

It does not overwrite a header's value.

Instead, it adds another header:value pair. For example, say the upstream server responds with a response header X-My-Header: existing-header. The directive add_header X-My-Header "new-header"; will add X-My-Header: new-header to the response headers, but it will not remove the X-My-Header: existing-header pair.
Similarly, when the value is "", it does not remove the header. It just doesn't add a new header:value pair to the response headers.
It only adds a header when it receives a successful (2xx/3xx) response from the upstream server. This is easily fixed by adding the always parameter to the directive. always will ensure that NGINX always adds the header, regardless of the response code.

To get all the cases passing, we can add the following nginx directives:

(1) add:

add: 
- name: X-Header-Add
  value: header-add

==>

 add_header_name X-Header-Add "header-add" always;

(2) set: we need two directives for this action:

set:
- name: X-Header-Set
  value: header-set

==>

proxy_hide_header X-Header-Set;
add_header X-Header-Set "header-set" always;

(3) remove:

remove:
- name: X-Header-Remove

==>

proxy_hide_header X-Header-Remove;

There's no need for map in this solution because add_header does not overwrite the existing headers.

To verify the functionality, run the conformance tests locally. You will need to add HTTPRouteResponseHeaderModifier to the list of supported features and follow the instructions to run the tests.

To iterate more quickly and run only the HTTPRouteResponseHeaderModifier test, you can add the go test flag --run TestConformance/HTTPRouteResponseHeaderModifier to go test command in the run-conformance-tests Makefile target.

If you would like to test manually, ping me on Slack, and I can send you the Yaml files I used for testing.

Thanks again for working on this!

kevin85421 · 2024-01-24T19:48:36Z

@kate-osborn Thank you for your detailed reply!

In the manual test you described, you check the request headers instead of the response headers. More on testing later.

Do you mean that the following output from the curl command is still showing request headers instead of response headers?

< HTTP/1.1 200 OK
< Server: nginx/1.25.3
< Date: Mon, 22 Jan 2024 07:28:07 GMT
< Content-Type: text/plain
< Content-Length: 327
< Connection: keep-alive
< Accept-Encoding: response-accept-encoding
< My-cool-header: my-client-value,response-appended-value
< My-Overwrite-Header: response-this-is-the-only-value
< Host: echo.example.com:8080
< X-Forwarded-For: 127.0.0.1
< Connection: close

kate-osborn · 2024-01-24T23:11:29Z

@kate-osborn Thank you for your detailed reply!

In the manual test you described, you check the request headers instead of the response headers. More on testing later.

Do you mean that the following output from the curl command is still showing request headers instead of response headers?
< HTTP/1.1 200 OK
< Server: nginx/1.25.3
< Date: Mon, 22 Jan 2024 07:28:07 GMT
< Content-Type: text/plain
< Content-Length: 327
< Connection: keep-alive
< Accept-Encoding: response-accept-encoding
< My-cool-header: my-client-value,response-appended-value
< My-Overwrite-Header: response-this-is-the-only-value
< Host: echo.example.com:8080
< X-Forwarded-For: 127.0.0.1
< Connection: close

@kevin85421 sorry, I meant the output from the echo app:

Headers:
 header 'Accept-Encoding' is 'compress'
 header 'My-cool-header' is 'my-client-value,this-is-an-appended-value'
 header 'My-Overwrite-Header' is 'this-is-the-only-value'
 header 'Host' is 'echo.example.com:8080'
 header 'X-Forwarded-For' is '127.0.0.1'
 header 'Connection' is 'close'
 header 'Accept' is '*/*'

That app writes the request headers.

The curl command you used does show the response headers.

kevin85421 · 2024-01-27T23:49:45Z

I tried to test this PR manually. The curl command gets the following output which seems to fulfill the expected behaviors for all six cases mentioned in this comment. I will try to run the conformance test to figure out whether I have any misunderstanding about the expected behavior.

Use this new echo-route.yaml.

filters:
    - type: ResponseHeaderModifier
      responseHeaderModifier:
        set:
        - name: To-Set-Exist-Header
          value: overwrite-value
        - name: Set-New-Header
          value: set-new-header
        add:
        - name: To-Add-Exist-Header
          value: response-appended-value
        - name: Add-New-Header
          value: add-new-header
        remove:
        - To-Remove-Exist-Header
        - Remove-Non-Exist-Header

Submit a request with three headers, To-Add-Exist-Header, To-Set-Exist-Header, and To-Remove-Exist-Header.

curl -v --resolve echo.example.com:$GW_PORT:$GW_IP http://echo.example.com:$GW_PORT/headers -H "To-Add-Exist-Header:original-add-value" -H "To-Set-Exist-Header:original-set-value" -H "To-Remove-Exist-Header:original-remove-value"

The curl command gets the following output which seems to fulfill the expected behaviors for all six cases mentioned in this comment.

kevin85421 · 2024-01-28T07:28:07Z

I can't easily run conformance tests locally based on this doc.

Based on HTTPRouteResponseHeaderModification, we need to use HTTPRouteResponseHeaderModification instead of HTTPRouteResponseHeaderModifier for SUPPORTED_FEATURES, but I am not 100% sure.
Use --run-test TestConformance/HTTPRouteResponseHeaderModifier instead of --run. I am also not 100% sure.
Eventually, I chose not to use --run-test. Instead, I removed all features in SUPPORTED_FEATURES except for HTTPRouteResponseHeaderModification, i.e.
```
SUPPORTED_FEATURES = HTTPRouteResponseHeaderModification
```
- For tests in httproute-response-header-modifier.go, 3 tests pass and 5 tests fail.
- I can't understand what the conformance tests are doing.
  - The doc installs NGF in provisioning mode instead of static mode. I am not sure what's the difference between these two modes.
  - After the conformance tests were complete (fail), I tried to submit curl requests to reproduce the 5 failed tests, but the requests couldn't be sent to the backend servers correctly. The Gateway same-namespace is in the GatewayConflict status, and I am not sure whether it is expected or not.

I finally decided to modify YAMLs to run the conformance manually.

# Step 1: Install NGF
# Step 2: Gateway / Service / Backend server ...
# https://gist.github.com/kevin85421/1b0d21ff5c2735950da4c73bfd0bb1ee
kubectl apply -f manifests.yaml

# Step 3: HTTPRoute
# https://gist.github.com/kevin85421/41c72b2194a887edd6cf01c5bdbccda4
kubectl apply -f httproute-response-header-modifier.yaml

# Step 4: Send a request to simulate test 1 (2nd test) in httproute-response-header-modifier.go.
curl -v http://127.0.0.1:8080/set -H "X-Echo-Set-Header:Some-Other-Header:val,X-Header-Set:some-other-value"

test 1 in httproute-response-header-modifier.go

The curl command gets the following output, and it has two X-Header-Set as mentioned in this comment.

 < HTTP/1.1 200 OK
 < Server: nginx/1.25.3
 < Date: Sun, 28 Jan 2024 06:35:14 GMT
 < Content-Type: application/json
 < Content-Length: 455
 < Connection: keep-alive
 < Some-Other-Header: val
 < X-Content-Type-Options: nosniff
 < X-Header-Set: some-other-value
 < X-Header-Set: set-overwrites-values
 < Host: 127.0.0.1:8080
 < X-Forwarded-For: 127.0.0.1
 < Connection: close

I am not sure why there are no same headers in this comment.

kate-osborn · 2024-01-29T22:33:29Z

@kevin85421

Based on HTTPRouteResponseHeaderModification, we need to use HTTPRouteResponseHeaderModification instead of HTTPRouteResponseHeaderModifier for SUPPORTED_FEATURES, but I am not 100% sure.

Yes, you're right. I mistakenly gave you the test's ShortName instead of the SupportedFeature name.

Use --run-test TestConformance/HTTPRouteResponseHeaderModifier instead of --run. I am also not 100% sure.

I was able to get the HTTPRouteResponseHeaderModifier test running by adding --run TestConformance/HTTPRouteResponseHeaderModifier to the go test command in the conformance Makefile:

go test -v . -run TestConformance/HTTPRouteResponseHeaderModifier -tags conformance,experimental...

I haven't tried using the -run-test argument, but seems like that works as well.

The doc installs NGF in provisioning mode instead of static mode. I am not sure what's the difference between these two modes.

We use this mode only for conformance tests because the tests create multiple Gateway resources, which the static mode does not support. Theprovisioner mode runs this code which implements data plane provisioning for NGINX Gateway Fabric (NGF): it creates an NGF static mode Deployment for each Gateway that belongs to the provisioner GatewayClass.

After the conformance tests were complete (fail), I tried to submit curl requests to reproduce the 5 failed tests, but the requests couldn't be sent to the backend servers correctly. The Gateway same-namespace is in the GatewayConflict status, and I am not sure whether it is expected or not.

The conformance tests clean up after themselves, so my guess is that all the resources were deleted after the tests finished, which is why you weren't able to reach any of the backends.

The Gateway same-namespace is in the GatewayConflict status, and I am not sure whether it is expected or not.

That's not expected. Were you running NGF in provisioner mode? When I ran the conformance tests on your branch using the make install-ngf-local-build and then make run-conformance-tests, 3/8 of the test cases for the HTTPRouteResponseHeaderModification passed, and all the Gateways were Programmed.

I am not sure why there are no same headers in #1494 (comment).

The HTTPRouteResponseHeaderModifier test uses this echo-server as the Backend, which allows you to set the response headers for a request by specifying them in the X-Echo-Set-Header request header. The echo-server reads in the header key-value pairs from the X-Echo-Set-Header request header and writes each key-value pair as response headers (see this function).

When you sent the curl request curl -v http://127.0.0.1:8080/set -H "X-Echo-Set-Header:Some-Other-Header:val,X-Header-Set:some-other-value" you essentially told the echo-server to set the following response headers: Some-Other-Header: val and X-Header-Set: some-other-value.

Before, when you were testing with our header example backend, you were not setting the response headers in your curl request. Instead, you were setting the request headers. Our header example backend is not as smart as the echo-server, and there is no way to set the response headers with it.

The curl command gets the following output which seems to fulfill the expected behaviors for all six cases mentioned in #1494 (comment).

When verifying response headers, add the -i flag to your curl commands. From the curl man page:

 -i, --include
              Include the HTTP response headers in the output. The HTTP response headers can include things like server name, cookies, date of the document, HTTP version and more...

              To view the request headers, consider the -v, --verbose option.

I thought -v included both request and response headers, but the man page tells a different story.

I hope this helps, thanks again for your effort on this! Feel free to ping me on slack if you run into any more issues.

internal/mode/static/nginx/config/servers.go

kevin85421 · 2024-02-04T07:25:29Z

internal/mode/static/nginx/config/http/config.go

+	ProxyPass             string
+	HTTPMatchVar          string
+	ProxySetHeaders       []Header
+	AddResponseHeaders    []Header


We might consider reusing dataplane.HTTPHeaderFilter instead of using AddResponseHeaders, SetResponseHeaders, and RemoveResponseHeaders. Why do we need both Header and dataplane.HTTPHeader?

We are trying to keep the dataplane configuration as dataplane-agnostic as possible. We translate Kubernetes APIs -> dataplane configuration -> nginx configuration. Each layer is separate.

kevin85421 · 2024-02-04T07:34:14Z

Use --run-test TestConformance/HTTPRouteResponseHeaderModifier instead of --run. I am also not 100% sure.

I will get the following error when I use --run.

kubectl run -i conformance \
      --image=conformance-test-runner:latest --image-pull-policy=Never \
        --overrides='{ "spec": { "serviceAccountName": "conformance" }  }' \
        --restart=Never -- sh -c "go test -v . -tags conformance,experimental -args --gateway-class=nginx \
                                                        --run TestConformance/HTTPRouteResponseHeaderModifier \
                                                --supported-features=HTTPRouteResponseHeaderModification --version= \
                                                        --report-output=output.txt; cat output.txt" | tee output.txt
If you don't see a command prompt, try pressing enter.
flag provided but not defined: -run

Were you running NGF in provisioner mode?

I installed NGF in static mode instead of running make install-ngf-local-build.

kevin85421 · 2024-02-04T09:01:52Z

Manual test

make install-ngf-local-build
make run-conformance-tests
# Test passes. See https://gist.github.com/kevin85421/f9aa82a311a52e1240c70ae3f088f722 for more details.

kevin85421 · 2024-02-04T09:14:37Z

Instead, it adds another header:value pair. For example, say the upstream server responds with a response header X-My-Header: existing-header. The directive add_header X-My-Header "new-header"; will add X-My-Header: new-header to the response headers, but it will not remove the X-My-Header: existing-header pair.

I followed your suggestions and removed map from the add headers, and the conformance tests pass. However, I don't really understand the expected behavior of add. Take this test as an example, the backend will set Some-Other-Header:val and X-Header-Add:some-other-value, and the ResponseHeaderModifier for the path /add is shown as follow.

    filters:
    - type: ResponseHeaderModifier
      responseHeaderModifier:
        add:
        - name: X-Header-Add
          value: add-appends-values

In my expectation, the response headers should be Some-Other-Header:val and X-Header-Add:some-other-value,add-appends-values. However, when I tried to reproduce the test manually by running the following curl command. The response headers have Some-Other-Header:val, X-Header-Add:some-other-value, and X-Header-Add:add-appends-values. I am not sure why conformance tests pass when there are two X-Header-Add headers.

curl -i http://127.0.0.1:8080/add -H "X-Echo-Set-Header:Some-Other-Header:val,X-Header-Add:some-other-value"

kate-osborn · 2024-02-05T22:25:49Z

Use --run-test TestConformance/HTTPRouteResponseHeaderModifier instead of --run. I am also not 100% sure.

I will get the following error when I use --run.

kubectl run -i conformance \
      --image=conformance-test-runner:latest --image-pull-policy=Never \
        --overrides='{ "spec": { "serviceAccountName": "conformance" }  }' \
        --restart=Never -- sh -c "go test -v . -tags conformance,experimental -args --gateway-class=nginx \
                                                        --run TestConformance/HTTPRouteResponseHeaderModifier \
                                                --supported-features=HTTPRouteResponseHeaderModification --version= \
                                                        --report-output=output.txt; cat output.txt" | tee output.txt
If you don't see a command prompt, try pressing enter.
flag provided but not defined: -run

--run is an argument for go test, so it has to be placed before -args. The arguments after -args are passed to the test binary.

kevin85421 · 2024-02-05T22:39:52Z

--run is an argument for go test, so it has to be placed before -args. The arguments after -args are passed to the test binary.

I feel so embarrassed about my dumb question lol.

kate-osborn · 2024-02-07T01:28:21Z

Instead, it adds another header:value pair. For example, say the upstream server responds with a response header X-My-Header: existing-header. The directive add_header X-My-Header "new-header"; will add X-My-Header: new-header to the response headers, but it will not remove the X-My-Header: existing-header pair.

I followed your suggestions and removed map from the add headers, and the conformance tests pass. However, I don't really understand the expected behavior of add. Take this test as an example, the backend will set Some-Other-Header:val and X-Header-Add:some-other-value, and the ResponseHeaderModifier for the path /add is shown as follow.
    filters:
    - type: ResponseHeaderModifier
      responseHeaderModifier:
        add:
        - name: X-Header-Add
          value: add-appends-values
In my expectation, the response headers should be Some-Other-Header:val and X-Header-Add:some-other-value,add-appends-values. However, when I tried to reproduce the test manually by running the following curl command. The response headers have Some-Other-Header:val, X-Header-Add:some-other-value, and X-Header-Add:add-appends-values. I am not sure why conformance tests pass when there are two X-Header-Add headers.
curl -i http://127.0.0.1:8080/add -H "X-Echo-Set-Header:Some-Other-Header:val,X-Header-Add:some-other-value"

@kevin85421 I believe that multiple entries of the same header name are equivalent to one header name with a comma-separated list of values, so long as the order of the headers is correct. To be sure, I asked this question in the Gateway API slack channel: https://kubernetes.slack.com/archives/CR0H13KGA/p1707260286602689.

kate-osborn · 2024-02-07T20:56:56Z

@kevin85421 I believe that multiple entries of the same header name are equivalent to one header name with a comma-separated list of values, so long as the order of the headers is correct. To be sure, I asked this question in the Gateway API slack channel: https://kubernetes.slack.com/archives/CR0H13KGA/p1707260286602689.

@kevin85421 confirmed that they are equivalent. I hope that answers your question. I will take a look at the rest of your PR this week 🙂

kevin85421 · 2024-02-12T17:18:04Z

@kate-osborn Thank you!

Problem: Users want to add, set, and remove response headers. Solution: Use add_header and proxy_hide_header NGINX directives to support ResponseHeaderModifier. If the action is set, we configure the proxy_hide_header directive to remove the header if exists and add_header directive with the given value in the HTTPRoute spec. If the action is remove, we configure the proxy_hide_header directive to remove the header. If the action is add, we simply configure the add_header directive with the given value in the HTTPRoute spec.

ADubhlaoich

LGTM for the docs changes: I am not approving because most of the PR is code-based.

Problem: Users want to add, set, and remove response headers. Solution: Use add_header and proxy_hide_header NGINX directives to support ResponseHeaderModifier. If the action is set, we configure the proxy_hide_header directive to remove the header if exists and add_header directive with the given value in the HTTPRoute spec. If the action is remove, we configure the proxy_hide_header directive to remove the header. If the action is add, we simply configure the add_header directive with the given value in the HTTPRoute spec.

kate-osborn · 2024-02-22T01:43:50Z

Btw, gitleaks complains about this fake private key for testing, so I need to run SKIP=gitleaks git commit to commit.

🤔 this key is in the .gitleaksignore file: https://github.com/nginxinc/nginx-gateway-fabric/blob/a66255b593c4cdb7aaa1b340ab425cc27c90aa37/.gitleaksignore#L6.

Perhaps rebasing will solve the issue?

@kevin85421

kate-osborn

Following up on extra validation:

Let's disallow the following special header fields in the ResponseHeaderModifier:

"Server"
"Date"
"X-Pad"
Headers starting with "X-Accel"
"Content-Type"
"Content-Length"
"Connection"

After some testing, trying to modify these headers in the same way we modify other headers doesn't work. I think it's better to disallow them through validation and document in the compatibility doc that they aren't supported header values.

internal/mode/static/nginx/config/servers.go

kate-osborn · 2024-02-20T21:33:39Z

internal/mode/static/nginx/config/servers_test.go

+						Add:    []http.Header{},
+						Set:    []http.Header{},
+						Remove: []string{},
+					},
 				},
 			},
 		},


Can you add some unit tests for the generateResponseHeaders function?

Let's also add to the createServers test to cover the case where multiple compatible filters are defined. All filters should be compatible except URLRewrite and RequestRedirect.

site/content/overview/gateway-api-compatibility.md

kate-osborn · 2024-02-20T21:50:59Z

internal/mode/static/state/graph/httproute.go

@@ -806,16 +809,16 @@ func validateFilterRewrite(
 }

 func validateFilterHeaderModifier(
+	filterType v1.HTTPRouteFilterType,


This line of code changed in a recent commit: https://github.com/nginxinc/nginx-gateway-fabric/blob/8d5f023eb4b7bd167a98adcdfae82ad41bb20249/internal/mode/static/state/graph/httproute.go#L825

Since we now provide the path, I don't think we need to specify the child name in the error message. We can just say:

if headerModifier == nil { return field.ErrorList{field.Required(filterPath, "cannot be nil")} }

Problem: Users want to add, set, and remove response headers. Solution: Use add_header and proxy_hide_header NGINX directives to support ResponseHeaderModifier. If the action is set, we configure the proxy_hide_header directive to remove the header if exists and add_header directive with the given value in the HTTPRoute spec. If the action is remove, we configure the proxy_hide_header directive to remove the header. If the action is add, we simply configure the add_header directive with the given value in the HTTPRoute spec.

kevin85421 · 2024-02-25T23:08:05Z

Thank @ADubhlaoich and @kate-osborn for the review! I have already addressed some comments. I have been very busy recently. I will do my best to address #1494 (review) and #1494 (comment) as soon as possible, possibly by next weekend.

github-actions · 2024-03-11T02:34:13Z

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

kevin85421 · 2024-03-11T02:37:38Z

I will update this PR today.

kevin85421 · 2024-03-11T13:24:57Z

internal/mode/static/state/graph/httproute.go

+	responseHeaderModifier *v1.HTTPHeaderFilter,
+	filterPath *field.Path,
+) field.ErrorList {
+	if errList := validateFilterHeaderModifier(validator, responseHeaderModifier, filterPath); errList != nil {


If responseHeaderModifier is invalid for both validateFilterHeaderModifier and disallowedResponseHeaderSet, should we include both in allErrs, or do we only need to return validateFilterHeaderModifier?

I think we only need to return the errors from validateFilterHeaderModifier.

@pleshakov, does that seem right to you?

I think we only need to return the errors from validateFilterHeaderModifier.

Sounds reasonable.

In the check, I would check the length of errList, not that it doesn't equal to nil - just so that we don't dependent on zero-length vs nil slice differences.

kevin85421 · 2024-03-11T13:31:36Z

Add a validation function based on Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494 (review): 12123fb
- TODO: Update docs
- TODO: Try testing the validation function manually. Will NGF log error messages or file Kubernetes events if the header modifier is invalid?

I will update the docs after #1494 (comment) got answered.

TODO: Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494 (comment)

kate-osborn · 2024-03-14T17:55:12Z

internal/mode/static/state/graph/httproute.go

+		}
+	}
+	for _, h := range responseHeaderModifier.Set {
+		valErr := field.Invalid(filterPath.Child("set"), h, "header name is not allowed")


I recommend extracting a function that accepts []v1.HTTPHeader and field.Path and checks if the header names are allowed.

Similar to how it's done in the validateFilterHeaderModifierFields function,

kate-osborn · 2024-03-14T17:59:35Z

internal/mode/static/state/graph/httproute_test.go

+				},
+			},
+			expectErrCount: 3,
+			name:           "invalid response header modifier filter",


nit:

Suggested change

name: "invalid response header modifier filter",

name: "response header modifier filter with disallowed header name",

kate-osborn · 2024-03-14T17:59:55Z

internal/mode/static/state/graph/httproute_test.go

+				},
+			},
+			expectErrCount: 3,
+			name:           "invalid response header modifier filter",


nit:

Suggested change

name: "invalid response header modifier filter",

name: "response header modifier filter with disallowed header name prefix",

github-actions · 2024-04-04T02:35:04Z

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

kevin85421 · 2024-04-05T09:22:59Z

I have been very busy recently and may not be able to work on this PR for the next 2 weeks. Hence, I am closing this PR to avoid blocking the community's roadmap. Feel free to take over this issue. I may reopen it in the future if I find the time to work on it and if the issue remains unresolved. Sorry for wasting your time.

kevin85421 commented Jan 22, 2024

View reviewed changes

internal/mode/static/nginx/config/http/config.go Outdated Show resolved Hide resolved

kevin85421 commented Jan 22, 2024

View reviewed changes

internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved

kevin85421 added 5 commits January 22, 2024 07:20

refactor validateFilterHeaderModifier

5cf3099

Rename ValidateRequestHeader to ValidateFilterHeader

9d8188d

Rename AddHeaders to AddHeaderDirectives

0241709

Update servers_template.go

e159b03

kevin85421 force-pushed the ResponseHeaderModifier branch from ca02a6d to e159b03 Compare January 22, 2024 07:22

mpstefan assigned kate-osborn Jan 22, 2024

kevin85421 added 4 commits February 4, 2024 06:53

Pass conformance tests

3d7e6bf

Remove Map

649d90e

Remove fmt.Printf

4274622

Change TODO to FIXME

4bbc00f

kevin85421 commented Feb 4, 2024

View reviewed changes

internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved

kevin85421 commented Feb 4, 2024

View reviewed changes

internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved

kevin85421 commented Feb 4, 2024

View reviewed changes

kevin85421 force-pushed the ResponseHeaderModifier branch from e91f5e2 to c0f98ab Compare February 20, 2024 01:38

kevin85421 changed the title ~~WIP: Add support for ResponseHeaderModifier for HTTPRouteRule objects~~ Add support for ResponseHeaderModifier for HTTPRouteRule objects Feb 20, 2024

kevin85421 marked this pull request as ready for review February 20, 2024 01:39

kevin85421 requested review from a team as code owners February 20, 2024 01:39

ADubhlaoich reviewed Feb 20, 2024

View reviewed changes

kate-osborn suggested changes Feb 22, 2024

View reviewed changes

lucacome added the community label Feb 23, 2024

kevin85421 added 4 commits February 25, 2024 17:28

github-actions bot added the stale Pull requests/issues with no activity label Mar 11, 2024

update

12123fb

kevin85421 commented Mar 11, 2024

View reviewed changes

mpstefan removed the stale Pull requests/issues with no activity label Mar 11, 2024

kate-osborn suggested changes Mar 14, 2024

View reviewed changes

mpstefan added this to the v1.3.0 milestone Mar 20, 2024

github-actions bot added the stale Pull requests/issues with no activity label Apr 4, 2024

kevin85421 closed this Apr 5, 2024

salonichf5 mentioned this pull request Apr 25, 2024

Add support for ResponseHeaderModifier for HTTPRouteRule objects #1880

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494

Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494

kevin85421 commented Jan 21, 2024 •

edited

Loading

kate-osborn commented Jan 24, 2024

kevin85421 commented Jan 24, 2024

kate-osborn commented Jan 24, 2024

kevin85421 commented Jan 27, 2024

kevin85421 commented Jan 28, 2024 •

edited

Loading

kate-osborn commented Jan 29, 2024

kevin85421 Feb 4, 2024

kate-osborn Feb 14, 2024

kevin85421 commented Feb 4, 2024

kevin85421 commented Feb 4, 2024

kevin85421 commented Feb 4, 2024

kate-osborn commented Feb 5, 2024

kevin85421 commented Feb 5, 2024

kate-osborn commented Feb 7, 2024

kate-osborn commented Feb 7, 2024

kevin85421 commented Feb 12, 2024

ADubhlaoich left a comment

kate-osborn commented Feb 22, 2024 •

edited

Loading

kate-osborn left a comment

kate-osborn Feb 20, 2024

kate-osborn Feb 20, 2024

kevin85421 commented Feb 25, 2024

github-actions bot commented Mar 11, 2024

kevin85421 commented Mar 11, 2024

kevin85421 Mar 11, 2024

kate-osborn Mar 14, 2024

pleshakov Mar 14, 2024

kevin85421 commented Mar 11, 2024

kate-osborn Mar 14, 2024

kate-osborn Mar 14, 2024

kate-osborn Mar 14, 2024

github-actions bot commented Apr 4, 2024

kevin85421 commented Apr 5, 2024

	name: "invalid response header modifier filter",
	name: "response header modifier filter with disallowed header name",

Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494

Add support for ResponseHeaderModifier for HTTPRouteRule objects #1494

Conversation

kevin85421 commented Jan 21, 2024 • edited Loading

Proposed changes

Checklist

kate-osborn commented Jan 24, 2024

kevin85421 commented Jan 24, 2024

kate-osborn commented Jan 24, 2024

kevin85421 commented Jan 27, 2024

kevin85421 commented Jan 28, 2024 • edited Loading

kate-osborn commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Feb 4, 2024

kevin85421 commented Feb 4, 2024

kevin85421 commented Feb 4, 2024

kate-osborn commented Feb 5, 2024

kevin85421 commented Feb 5, 2024

kate-osborn commented Feb 7, 2024

kate-osborn commented Feb 7, 2024

kevin85421 commented Feb 12, 2024

ADubhlaoich left a comment

Choose a reason for hiding this comment

kate-osborn commented Feb 22, 2024 • edited Loading

kate-osborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Feb 25, 2024

github-actions bot commented Mar 11, 2024

kevin85421 commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 4, 2024

kevin85421 commented Apr 5, 2024

kevin85421 commented Jan 21, 2024 •

edited

Loading

kevin85421 commented Jan 28, 2024 •

edited

Loading

kate-osborn commented Feb 22, 2024 •

edited

Loading