Check modified request headers at the mirrored backend.

[Treenhan]

What would you like to be added: Checks if the headers are mirrored in the request sent.

Why this is needed: While developing conformance tests, we discovered an issue with the ExpectMirroredRequest method (https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/utils/http/mirror.go#L32). This method verifies that a request is mirrored properly. However, it was only checking that the path matched for mirrored requests. It did not have checks in place for the headers.

This is problematic because headers can also be modified or added to requests. To have fully conformant request mirroring, the test method needs to account for headers as well as the path.

More context: https://github.com/kubernetes-sigs/gateway-api/pull/2101#discussion_r1239819362

/kind feature /area conformance

[Treenhan]

Hi @shaneutt can I work on this one?

[shaneutt]

Seems like a helpful improvement:

/triage accepted

Please do feel free to work on this, let us know if you need help or run into any trouble.

/assign @Treenhan

[shaneutt]

@Treenhan Heya! :wave:

Just checking in, everything going OK have any questions about this one?

[Treenhan]

Hi @shaneutt thank you for asking, do you know what happens to the response from the mirrored backend? Do you know if the gateway forwards it back to the original client? If so, how can the client see this response?

[Treenhan]

I found this paragraph in the doc https://gateway-api.sigs.k8s.io/references/spec/

RequestMirror defines a schema for a filter that mirrors requests. Requests are sent to the specified destination, but responses from that destination are ignored.

which means the mirrored backend response is ignored, what are the possible ways we can extract the headers if not at the client level?

[shaneutt]

You may have stumbled upon the reason why we didn't have the test check the headers in the first place: I think we colloquially expect the headers to be sent in the mirrored request, but given that we ignore the response I think the only place we could meaningfully test would be at the mirror destination (e.g. the backend receiving the request needs to be queried (logs, e.t.c.) to verify the mirrored request). I'm not sure how easy it is to go locate and verify that request at the backend level with our tooling today, I suppose the next step is to go investigate that and see how reasonable/feasible that is :thinking:

[Treenhan]

I suggest the following changes for this problem:

• [] Update the golang code for the echo-server docker image to look for a new special Request Header called X-Header-Mirrored:True. If this Header is present, echo-server should log this request and response, before sending the request back.
• [] Once the Request and Response is present in the logs of echo-server, we can expand the existing regex so it captures the Headers in the logs.
• [] Finally, we can compare the Headers in the logs and the expected Headers to ensure that the mirrored requests have its headers modified, therefore passing the test.

@shaneutt what do you think?

[shaneutt]

This seems like a reasonable way forward to me :+1:

[Treenhan]

I had a discussion with @BenTheElder about the best way to upgrade reformat-gherkin version, and we decided to install the correct version in the source repo script (e.g. verify-gherkins.sh), and point the yaml config from test-infra to that script.

Implementation https://github.com/kubernetes-sigs/ingress-controller-conformance/pull/109

[shaneutt]

@Treenhan how are we doing here, with the other PRs done is the implementation here now pretty simple? Do you need any help moving this forward?

[Treenhan]

Hi @shaneutt, thank you for checking in.

This is blocked on https://github.com/kubernetes-sigs/ingress-controller-conformance/pull/108 which is just missing an LGTM from one of the reviewers. I think @bowei is quite busy at the moment so I wonder if there is anyone else who can LGTM that to unblock this one?

[youngnick]

Probably worth noting that #2447 is moving that code into this repo so that we can more easily make changes.

[shaneutt]

pinged Tim on https://github.com/kubernetes-sigs/ingress-controller-conformance/pull/108

[robscott]

Hey @Treenhan, sorry I missed this one, we actually moved this image into gateway-api control, do you mind making the same change here on https://github.com/kubernetes-sigs/gateway-api/tree/main/conformance/echo-basic instead?

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted