-
Notifications
You must be signed in to change notification settings - Fork 353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Gateway API 1.2.0] Add gateway name label #4333
Conversation
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4333 +/- ##
==========================================
+ Coverage 65.96% 65.98% +0.01%
==========================================
Files 197 197
Lines 23959 23971 +12
==========================================
+ Hits 15805 15817 +12
Misses 7027 7027
Partials 1127 1127 ☔ View full report in Codecov by Sentry. |
@@ -87,6 +87,7 @@ infraIR: | |||
labels: | |||
gateway.envoyproxy.io/owning-gateway-name: eg | |||
gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway-system | |||
gateway.networking.k8s.io/gateway-name: eg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EG differs from the GEP in two ways
- label name
gateway.envoyproxy.io/owning-gateway-name
- generated resource is in the same ns as the Gateway
this warrants a bigger discussion, lets bring this up in the community meeting next week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR only adds the required label gateway.envoyproxy.io/owning-gateway-name
. It's compatible with the current EG implementation.
The existing gateway.envoyproxy.io/owning-gateway-name
is reserved for compatibility reasons. This label has been used as a selector to filter out services and deployments related to a Gateway.
@@ -216,6 +216,7 @@ infraIR: | |||
metadata: | |||
labels: | |||
gateway.envoyproxy.io/owning-gatewayclass: envoy-gateway-class | |||
gateway.networking.k8s.io/gateway-name: gateway-1,gateway-2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All gateway names are concatenated and stored in a single label if mergeGateways is enabled.
Probably not the best way to do this, should revisit this and come up with a better solution upstream.
This is tracked in kubernetes-sigs/gateway-api#3365
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Apart from passing the conformance test, how does the end user benefit from this change ? |
No, but we could replace the existing |
I suggest waiting until kubernetes-sigs/gateway-api#3366 is resolved which in indirectly related to the name of the label |
Signed-off-by: Huabing Zhao <[email protected]>
Implement the Gateway name label pfart of GEP-1762: In Cluster Gateway Deployments
This PR only covers the gateway name label requirement in the GEP-1762.
Please note that this label has not been added to the generated resources of the merged gateway.
Implement: #4330