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 |
| - type: Inline | ||
| inline: | | ||
| function envoy_on_request(request_handle) | ||
| local ssl = request_handle:connection():ssl() |
There was a problem hiding this comment.
Something feels off here, we are trusting a header that the client may be able to forge themselves. Since we do not remove it first, it looks spoofable. Is that really expected on the Federator side?
There was a problem hiding this comment.
Good catch! Addressed in 300c32e
In addition to this it would've been nice to also enforce client certificates, but this would change the protocol (change error codes). I left a comment why client certs are configured optionally
| apiVersion: gateway.envoyproxy.io/v1alpha1 | ||
| kind: EnvoyExtensionPolicy | ||
| metadata: | ||
| name: {{ include "wire-ingress.fullname" . }}-federator-cert-header |
There was a problem hiding this comment.
Other policies in this chart set namespace: {{ .Release.Namespace }} ?
| - type: Inline | ||
| inline: | | ||
| function envoy_on_request(request_handle) | ||
| local ssl = request_handle:connection():ssl() |
| 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
| @@ -0,0 +1,154 @@ | |||
| {{- if .Values.envoy.enabled }} | |||
There was a problem hiding this comment.
None of the five kinds here carry chart/release label
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: account-pages-http |
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: team-settings-http |
| kind: Service | ||
| metadata: | ||
| name: {{ .Release.Namespace }}-fed | ||
| namespace: {{ .Values.gateway.controllerNamespace }} |
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: webapp-http |
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ .Release.Namespace }}-fed |
There was a problem hiding this comment.
The -fed suffix here is also hardcoded in integration/_helpers.tpl .Both need to stay aligned renaming one side breaks federation discovery with no obvious error.
Ideally a shared helper ? otherwise at least a cross-reference comment in both files pointing to each other ? that would also explain why we don't use wire-ingress.fullname here like everywhere else
There was a problem hiding this comment.
I explored the the shared helper idea again, but it would require passing 4 values consistently in 3 helm releases to avoid this hidden assumption. I went with commenting in both places instead (7f220cc)
| # -----END PRIVATE KEY----- | ||
| # | ||
| # When federator.enabled is true, a ConfigMap named `federator-ca` with a `ca.crt` key | ||
| # must exist in the release namespace. It is created by the wire-server chart |
There was a problem hiding this comment.
This ConfigMap is created by the federator chart, not wire-server ?
There was a problem hiding this comment.
Users don't install the federator only directly, but via requirement of the wire-server chart. (federator is a subchart)
| 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
| @@ -0,0 +1,75 @@ | |||
| {{/* vim: set filetype=mustache: */}} | |||
There was a problem hiding this comment.
getCertificateSecretName, getCustomSolversSecretName, getIssuerName, getGatewayName
I would use it without get, because in Helm charts you usually use short names focused on the result but that's just my opinion
f8ead45 to
a59a6c7
Compare
This PR:
Introduces a new Helm chart
wire-ingressthat targets Envoy Gateway. It is intended as areplacement for the
nginx-ingress-serviceschart, which uses ingress-nginx.The
wire-ingresschart is not production-ready yet.Changes the integration test suite: all tests now run against the
wire-ingresschart. Theingress solution can be selected via the
WIRE_INGRESS_MODEenvironment variable.The federation domains change in Envoy mode — see comments in the code.
Changes to the
federatorandintegrationcharts are made to accommodate both variants fortesting. As a consequence, any changes to
nginx-ingress-serviceswill be untested once this PRis merged. I've added a check
integration-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