Add support for envoy gateway#5150
Conversation
c4a1b87 to
28aad07
Compare
|
Naming thing that bugs me across the whole PR, the chart is wire-ingress, the file is ingress-envoy.yaml the route is nginz-websockets. None of thes have any Ingress in them. My point of view, name it like the replacement wire-gateway , gateway-envoy.yaml etc... not blocking when I open a file called ingress-envoy.yaml and the first thing I see is kind: gateway. I have to stop and double check. |
|
A BackendTrafficPolicy is missing for WebSockets, the default timeouts will terminate long-lived connections, I didn't check the default value, https://gateway.envoyproxy.io/latest/api/extension_types/#backendtrafficpolicy |
8e81e3f to
83b86e7
Compare
I changed the |
Thank you! Added the policy. We need to run QA tests against this on staging to see if it really works |
| name: {{ printf "%s/%s/https" .Release.Namespace $gatewayName | quote }} | ||
| operation: | ||
| op: add | ||
| path: "/filter_chains/0/filters/0/typed_config/strip_trailing_host_dot" |
There was a problem hiding this comment.
Why don't you use the same approach las in charts/wire-ingress/templates/envoypatchpolicy-federator.yaml:36-41
There was a problem hiding this comment.
ah, this is a leftover from a previous solution. I switched to the new solution in 05e23ad
| controllerName: gateway.envoyproxy.io/gatewayclass-controller | ||
| ``` | ||
|
|
||
| You need to refer to this object in the `gateway.className` paramter. |
| | `config.dns.base` | Only used for CSP header rendering, which is a multi-ingress feature | | ||
| | `tls.verify_depth` | Envoy Gateway `ClientTrafficPolicy` does not expose a direct verify-depth knob; the CA chain itself controls this | | ||
| | `tls.enabled` | Removed — had no effect; all routes are always TLS-terminated | | ||
| | `secrets.tlsClientCA` | No longer supplied via values. The `federator-ca` ConfigMap is created by the wire-server chart and referenced directly. | |
There was a problem hiding this comment.
Same as previous comment
f8ead45 to
a59a6c7
Compare
fisx
left a comment
There was a problem hiding this comment.
i've read all the non-helm code (3 files), LGTM!
| CHARTS_DIR="${TOP_LEVEL}/.local/charts" | ||
| HELM_PARALLELISM=${HELM_PARALLELISM:-1} | ||
|
|
||
| changed_files=$(git --no-pager diff-tree --no-commit-id -r --name-only HEAD) |
There was a problem hiding this comment.
so this guard only works if i don't commit my changes first, before running tests. i guess that's a lot better than no guard. you could compute a sha256sum from all of nginx-ingress-services i guess, and compare against a stored copy of the hash. if there is a mismatch, the error will instruct you to commit the changes and update the hash in the script.
i'm not saying you should do that, this is just my brain having ideas.
There was a problem hiding this comment.
there was a misunderstanding, which @fisx resolved in 1on1: the guard actually compares against the merge-base of the PR in the ci pipeline, so it effectively prevents any changes
This PR:
Introduces a new Helm chart
wire-ingressthat targets Envoy Gateway. It is intended as a replacement for thenginx-ingress-serviceschart, which uses ingress-nginx. Thewire-ingresschart is not production-ready yet.Changes the integration test suite: all tests now run against the
wire-ingresschart. The ingress solution can be selected via theWIRE_INGRESS_MODEenvironment variable. The federation domains change in Envoy mode — see comments in the code. Changes to thefederatorandintegrationcharts are made to accommodate both variants fortesting. As a consequence, any changes to
nginx-ingress-serviceswill be untested once this PR is merged. I've added a checkintegration-setup-federation.shthat prevents any changes tonginx-ingress-servicesto avoid this being overlooked.Changes the temporary filenames used in the integration test suite. This fixes issues with filenames that were too long for
nginzto handle.Deletes the unused file
hack/helmfile-federation-v0.yaml.gotmpl.Add a
post-upgradeto all objects needed for testing. This makes running tests on the cluster manually more convenientChecklist
.envrcbefore merging !!!!changelog.d