feat: propagate gateway-name label to infra resources in default mode - #9478
feat: propagate gateway-name label to infra resources in default mode#9478mmmommm wants to merge 1 commit into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
96b0a66 to
804c7ac
Compare
|
@codex review |
|
/retest |
![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: 804c7ac888
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| labels := r.envoyLabels(extraLabels) | ||
| if !r.GatewayNamespaceMode { | ||
| if gwName := labels[gatewayapi.OwningGatewayNameLabel]; gwName != "" { | ||
| labels[gatewayapi.GatewayNameLabel] = gwName |
There was a problem hiding this comment.
This adds the gateway-name label in default mode, but default conformance still skips tests.GatewayInfrastructure and still excludes GatewayInfrastructurePropagationFeature in test/conformance/suite.go:77 and test/conformance/suite.go:92. In default-mode conformance reports this change will therefore continue to be reported as unsupported even though the PR is intended to make the feature pass; please remove/update those skips when the default-mode behavior is actually supported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The GatewayInfrastructure conformance test hardcodes ns to "gateway-conformance-infra" and lists ServiceAccounts/Pods/Services only in that namespace. In default mode, GetResourceNamespace() always returns ControllerNamespace (typically envoy-gateway-system), so the test finds no resources regardless of this label — the namespace model, not the label, is the blocker.
This is consistent with EG's own published default-mode report, where GatewayInfrastructurePropagation is under unsupportedFeatures. It's only supported in gateway-namespace-mode (where infra lives in the Gateway's namespace), which is exactly why the skip is default-mode-only today. Un-skipping would fail the suite.
Happy to file a separate issue to track default-mode support if the upstream test becomes namespace-configurable.
There was a problem hiding this comment.
Originally I intended this PR to also enable the default-mode conformance feature, but it turned out not to be feasible 😢
|
@zhaohuabing / @zirain any idea why we didnt set this for default case ? |
I guess it's because we can't set a proper owner label for mergedGateway, but we should still set it for the non-merged case. |
+1 |
|
Hi @mmmommm , thanks for working this. |
|
Sure, I’ll update it right away. Thanks for pointing that out! |
|
Thanks for the review, the current implementation already handles set gateway-name label only when non-mergedGateway mode. |
Signed-off-by: Kise Ryota <kiseryota.contact@gmail.com>
804c7ac to
3aa5fda
Compare
4 participants
What this PR does / why we need it:
In the default mode, generated proxy infrastructure resources were missing the standard Gateway API gateway.networking.k8s.io/gateway-name label.
That label was only applied in Gateway Namespace Mode, which is why the GatewayInfrastructurePropagation conformance feature passes there but not in the default mode.
This PR propagates the gateway-name label to the generated resources in the default mode as well.
Which issue(s) this PR fixes:
Fixes #9475
confirmed working
defaultMode
when above Gateway/GatewayClass resources are created in default mode, the generated Deployment, Service, ConfigMap, and ServiceAccount have the metadata label gateway.networking.k8s.io/gateway-name: eg, and that spec.selector.matchLabels does not contain gateway-name.
deployment
svc
configmap
serviceAccount
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.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.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.