Repository navigation
Conversation
…renced by a Gateway mergeGateways is only read from the EnvoyProxy referenced by the GatewayClass (or the default EnvoyProxy spec). Setting it on an EnvoyProxy referenced by a Gateway through infrastructure.parametersRef had no effect, and nothing said so. Report it as a Warning condition with reason UnsupportedField on that EnvoyProxy's ancestor status, through the existing SetEnvoyProxyWarnings path used for deprecated and patch-disabled fields. Fixes envoyproxy#3601 Signed-off-by: Lakhan Yelgate <39281685+lyelgate@users.noreply.github.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 720190e880
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| status.SetEnvoyProxyWarnings(ep, ancestor, deprecatedFieldsUsedInEnvoyProxy(envoyproxyMap[key]), patchedFields) | ||
| status.SetEnvoyProxyWarnings(ep, ancestor, deprecatedFieldsUsedInEnvoyProxy(envoyproxyMap[key]), patchedFields, | ||
| gatewayUnsupportedFieldsUsedInEnvoyProxy(envoyproxyMap[key])) |
There was a problem hiding this comment.
When EnvoyGateway.envoyProxy.mergeGateways is true, attachEnvoyProxy skips the Gateway's parametersRef (internal/gatewayapi/contexts.go:105-114), leaving envoyProxyFromGateway false, so this new helper is never called. A referenced EnvoyProxy that sets mergeGateways—notably false to try to override the default—still gets no UnsupportedField condition even though the new docs and release note say Gateway-referenced proxies do; inspect the raw reference before attachment is suppressed (or explicitly reject it).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good point. When Gateways are already merged, the Gateway's parametersRef is ignored entirely, so that EnvoyProxy gets no status at all. I've scoped the docs and the release note to Gateways that are not merged, in 313ff34.
| status.SetEnvoyProxyWarnings(ep, ancestor, deprecatedFieldsUsedInEnvoyProxy(envoyproxyMap[key]), patchedFields, | ||
| gatewayUnsupportedFieldsUsedInEnvoyProxy(envoyproxyMap[key])) |
There was a problem hiding this comment.
When one Gateway-level EnvoyProxy is referenced by multiple Gateways and uses StrategicMerge or JSONMerge over a GatewayClass or default config, attachEnvoyProxy constructs a separate merged ep for each Gateway, and this call writes each warning only to that transient copy. newTranslateResult then deduplicates the copies by NamespacedName (internal/gatewayapi/translator.go:247-259) without merging statuses, so the published resource retains only the first Gateway's UnsupportedField warning; accumulate status on the source EnvoyProxy or merge duplicate statuses before deduplication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, but not specific to this warning: the deprecated-field and patch-disabled warnings, and the Accepted condition, are written to the same per-Gateway merged copy, so they share the same limitation when several Gateways reference one EnvoyProxy with a merge type. Merging statuses before deduplication would change how every EnvoyProxy status is aggregated, so I'd rather keep this PR scoped and handle it separately if maintainers want it.
When Gateways are already merged, a Gateway's parametersRef is ignored entirely and its EnvoyProxy gets no status, so the warning only applies when Gateways are not merged. Signed-off-by: Lakhan Yelgate <39281685+lyelgate@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 313ff3437b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| status.SetEnvoyProxyWarnings(ep, ancestor, deprecatedFieldsUsedInEnvoyProxy(envoyproxyMap[key]), patchedFields) | ||
| status.SetEnvoyProxyWarnings(ep, ancestor, deprecatedFieldsUsedInEnvoyProxy(envoyproxyMap[key]), patchedFields, | ||
| gatewayUnsupportedFieldsUsedInEnvoyProxy(envoyproxyMap[key])) |
There was a problem hiding this comment.
After this warning is published, removing parametersRef, deleting its Gateway, or enabling merged mode removes the EnvoyProxy from TranslateResult instead of invoking SetEnvoyProxyWarnings with an empty list. The runner then deletes the tracked key (internal/gatewayapi/runner/runner.go:1021-1023), but the Kubernetes status subscriber explicitly ignores delete updates (internal/provider/kubernetes/status.go:668-670), so the UnsupportedField condition already written to the CR remains indefinitely even though the reference no longer applies; publish an empty or filtered status before dropping the key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, but not specific to this warning: the deprecated-field and patch-disabled warnings, and the Accepted condition, are cleared the same way, so they also stay on the CR once the EnvoyProxy drops out of the translation result. Publishing an empty status before dropping the key would change the lifecycle of every EnvoyProxy status, so I'd rather keep this PR scoped and handle it separately if maintainers want it.
|
|
||
| This can help to efficiently utilize the infra resources in the cluster and manage them in a centralized manner, or have a single IP address for all of the listeners. | ||
| Setting the `mergeGateways` field in the EnvoyProxy resource linked to GatewayClass will result in merging all Gateway listeners under one GatewayClass resource. | ||
| The field has no effect on an EnvoyProxy referenced by a Gateway through `infrastructure.parametersRef`; that EnvoyProxy gets a `Warning` condition with reason `UnsupportedField` instead. When Gateways are already merged, the Gateway's `parametersRef` is ignored entirely, so that EnvoyProxy gets no status. |
There was a problem hiding this comment.
In the Kubernetes provider, when merging is enabled by the GatewayClass EnvoyProxy described in the preceding sentence, processGatewayParamsRef returns an error requiring infrastructure.parametersRef to be nil (internal/provider/kubernetes/controller.go:3262-3264), and its caller marks the Gateway not accepted (internal/provider/kubernetes/controller.go:2129-2139). Thus the reference is not merely ignored in this common merged-mode path; users need to be told to remove it, with the different default-spec behavior distinguished if that is intentional.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, that's correct: with merging enabled by the GatewayClass EnvoyProxy the Kubernetes provider doesn't accept a Gateway that sets parametersRef, and it's only ignored when merging comes from the default EnvoyProxy spec. The docs now say a Gateway should not set parametersRef when Gateways are merged and describe both cases, in 402a8d3.
When merging is enabled by the GatewayClass EnvoyProxy, the Kubernetes provider does not accept a Gateway that sets infrastructure.parametersRef; the reference is only ignored when merging comes from the default EnvoyProxy spec in the EnvoyGateway configuration. Signed-off-by: Lakhan Yelgate <39281685+lyelgate@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402a8d38a8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| strings.Join(patchedFields, ", "))) | ||
| } | ||
| if len(unsupportedFields) > 0 { | ||
| addWarning(EnvoyProxyReasonUnsupportedField, fmt.Sprintf( |
There was a problem hiding this comment.
When a Gateway-referenced EnvoyProxy has enough deprecated telemetry entries to fill the 32-KiB condition, and it also has ignored Kubernetes patch fields plus mergeGateways, the patch warning is first preserved at the end by mergeBoundedWarningMessages, but this later addWarning truncates that end to reserve space for UnsupportedField. The resulting Warnings condition no longer says that the patch was ignored, regressing the invariant covered by TestSetEnvoyProxyWarningKeepsLaterWarningWhenFull; reserve space for every non-deprecation warning category instead of repeatedly truncating the accumulated message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, confirmed with a test: with a deprecated-fields list that fills the message, the UnsupportedField warning cut off the patch-disabled one. The warnings are now collected first, the later ones merged together, and only then fitted after the first, so only the deprecated-fields list can be truncated. With one or two warnings the message is unchanged. Fixed in e0918a6, with TestSetEnvoyProxyWarningsKeepsLaterWarningsWhenTruncating.
Each warning was merged into the message in turn, and mergeBoundedWarningMessages cuts the existing message short to fit the next one. With three warnings and a deprecated-fields list that fills the condition message, adding the UnsupportedField warning cut off the patch-disabled warning that had been kept before it. Merge the later warnings first and then fit them after the first one, so only the first warning can be cut short. With one or two warnings the message is unchanged. Signed-off-by: Lakhan Yelgate <39281685+lyelgate@users.noreply.github.com>
What this PR does / why we need it:
mergeGatewaysis only read from the EnvoyProxy referenced by the GatewayClass, or the default EnvoyProxy spec (IsMergeGatewaysEnabledininternal/gatewayapi/helpers.go). Setting it on an EnvoyProxy referenced by a Gateway throughinfrastructure.parametersRefhas no effect. That EnvoyProxy reportsAcceptedwith no warning, so the user has no way to tell why their Gateways aren't merged.#3601 suggested CEL validation for this. CEL can't express it: whether an EnvoyProxy is referenced by a Gateway or by a GatewayClass isn't known from the EnvoyProxy object itself. The translator does know, and EnvoyProxy already has a per-ancestor
Warningcondition for this kind of problem.SetEnvoyProxyWarningsreports deprecated fields andpatchfields ignored because patching is disabled. This PR adds a third source of warnings to it, for fields that have no effect on an EnvoyProxy referenced by a Gateway, and reportsspec.mergeGatewaysthere:Details:
envoyproxyMap[key]), not the merged result, the same way deprecated fields are checked there. A GatewayClass-levelmergeGatewaysdoesn't trigger it.trueorfalse, since neither value takes effect.UnsupportedField, is defined next to the helper ininternal/gatewayapi/status, the same way the policy reasons such asPolicyReasonUnsupportedFieldare. There is no change under/api.SetEnvoyProxyWarningsnow goes through a smalladdWarningclosure instead of a nested if/else. The existing deprecated and patch-disabled behavior and messages are unchanged, and the existing unit tests pass as is (with the new argument).Changes:
internal/gatewayapi/status/envoyproxy.go:EnvoyProxyReasonUnsupportedField, and anunsupportedFieldsargument toSetEnvoyProxyWarnings.internal/gatewayapi/translator.go:gatewayUnsupportedFieldsUsedInEnvoyProxy, passed for Gateway-referenced EnvoyProxies (nilfor the GatewayClass one).internal/gatewayapi/status/envoyproxy_test.go: cases for the new warning alone and merged with the other two.internal/gatewayapi/testdata/envoyproxy-mergegateways-on-gateway.{in,out}.yaml: one Gateway whose EnvoyProxy setsmergeGateways, and one whose EnvoyProxy doesn't, as a control.site/content/en/latest/tasks/operations/deployment-mode.md: one sentence noting the field has no effect on a Gateway-referenced EnvoyProxy.release-notes/current/other_changes/3601-envoyproxy-mergegateways-on-gateway.md.Testing:
mainwithout the change and passes with it.go test ./internal/...passes.go test ./internal/gatewayapi ./internal/xds/translator --override-testdata=trueleaves no diff in existing golden files.golangci-lint(repo config, CI build tags) on./internal/gatewayapi/...,codespell,yamllintandtools/boilerplate/verify-boilerplate.share clean.Which issue(s) this PR fixes:
Fixes #3601
PR Checklist
git commit -s). See DCO: Sign your work./api.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.) Locally I ran the parts this change touches (listed under Testing); the full targets will run in CI.release-notes/current/.release-notes/current/breaking_changes/.