Discuss: refactor the out files under internal/xds/translator/testdata/out/xds-ir

[muyuan0]

Description:

Describe the issue.

The current xds translator test doesn't have corresponding requireXXX settings (though it has requireSecrets): https://github.com/envoyproxy/gateway/blob/73e38e0a2811146671d503bc53ac363a81d29d50/internal/xds/translator/translator_test.go#L148-L156

For each test case, all four xds types need to be specific. This causes lots of duplicate files and a heavier burden to update them later.

lots of duplicate files

Using rdfind shows us a result like that:

DUPTYPE_FIRST_OCCURRENCE 23 0 1205 2080 207885 1 internal/xds/translator/testdata/out/xds-ir/http-route-direct-response.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 212938 1 internal/xds/translator/testdata/out/xds-ir/http-route-redirect.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 212346 1 internal/xds/translator/testdata/out/xds-ir/http-route-mirror.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 606945 1 internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-root-path-url-prefix.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 217152 1 internal/xds/translator/testdata/out/xds-ir/http-route-regex.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 218598 1 internal/xds/translator/testdata/out/xds-ir/http-route-request-headers.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 224496 1 internal/xds/translator/testdata/out/xds-ir/http-route-response-add-headers.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 242400 1 internal/xds/translator/testdata/out/xds-ir/http-route-response-add-remove-headers.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 252551 1 internal/xds/translator/testdata/out/xds-ir/http-route.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 252294 1 internal/xds/translator/testdata/out/xds-ir/http-route-weighted-invalid-backend.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 252051 1 internal/xds/translator/testdata/out/xds-ir/http-route-weighted-backend.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 243504 1 internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-url-prefix.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 243422 1 internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-url-host.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 242764 1 internal/xds/translator/testdata/out/xds-ir/http-route-rewrite-url-fullpath.listeners.yaml
DUPTYPE_WITHIN_SAME_TREE -23 0 1205 2080 242518 1 internal/xds/translator/testdata/out/xds-ir/http-route-response-remove-headers.listeners.yaml

We can find out that almost every HTTPFilter test cases share the same listener xds as the http-route case. It's expected as the HTTPFilter doesn't affect the listener.

a heavier burden to update them later

For example, https://github.com/envoyproxy/gateway/pull/1095 needs to update over ten out files. As the number of test cases is growing, maybe the next time we have to update tens of files.

If the refactoring is permitted, I will take on it.

[optional Relevant Links:]

Any extra documentation required to understand the issue.

[arkodg]

Pros of the current testdata/*yaml style of unit tests

• very easy for devs to understand intent
• very easy to share this intent as a file to non devs

Cons

• change in one field can can cause a change in 100 files

Having felt the pain of the changing 100 files, which does take hours to fix, I still prefer the yaml approach because its very easy to share the envoy config with someone so they can repro the entire setup on envoy very easily

[arkodg]

is it crazy if we add a utility in gateway-api and xds-translator to write out the expected out yaml 🙃 ? I say this while Im changing 100 files in gateway-api thanks to https://github.com/envoyproxy/gateway/pull/1153 atm

[muyuan0]

Yes. We can have the Pros of the current testdata/*yaml style while reducing the Cons, by splitting them according to the listen/route/cluster and more sections. So if a test only wants to check a specific section, it can only write the section into the expected file. In the future, we can provide an expected struct to only compare part of the output data.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.