Adds Support for Managing Gateway Status Addresses#352
Adds Support for Managing Gateway Status Addresses#352danehans merged 2 commits intoenvoyproxy:mainfrom
Conversation
danehans
added
provider/kubernetes
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
- Coverage 60.76% 60.06% -0.71%
==========================================
Files 32 33 +1
Lines 3232 3323 +91
==========================================
+ Hits 1964 1996 +32
- Misses 1159 1216 +57
- Partials 109 111 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/gatewayapi/translator.go
Outdated
There was a problem hiding this comment.
Doesn't the infra correspond to a GatewayClass, which could have >1 Gateways? I would think these labels should identify the GatewayClass.
There was a problem hiding this comment.
@skriss since a Gateway supports multiple addresses, each Gateway corresponds to a Service. Therefore, a Service name is constructed as envoy-${GW_NAME}-${GW_NS}, where the variables are derived from these labels.
The owning Gateway labels are required so the gateway controller can reconcile the appropriate Gateway from CRUD's of a watched Service. As you know, we need to watch the Envoy service to get the status.ingresses[].ip to populate gateway.status.addresses.
|
Added |
danehans
changed the title
danehans
force-pushed
the
issue_330
branch
2 times, most recently
from
2cf81ce to
8d71f6f
Compare
|
@danehans curious if this PR unblocks the first few GatewayAPI conformance tests |
Signed-off-by: danehans <[email protected]>
@arkodg I have not looked into the tests so I'm unsure. From our discussions during the last few community meetings, I do know this PR will unblock our ability to run conformance tests. This is becasue the test client uses |
cool, would be great if you can attach the Gateway output highlighting the updated status with the LB address, TIA |
There was a problem hiding this comment.
Mostly LGTM, one nit. I'm a bit unclear on what's going on with labels -- if we have a single service per GatewayClass, wouldn't we want to label the Service with the GatewayClass info? https://github.com/envoyproxy/gateway/blob/main/internal/gatewayapi/translator.go#L114-L121 appears to only keep labels for the last Gateway that's seen since it's continually overwriting them for each Gateway, which seems weird. In the end I'm not sure how much it matters, since for every Reconcile call we're reconciling everything, and not actually using the service's labels to look up the relevant service for a Gateway, but it may be confusing.
Signed-off-by: danehans <[email protected]>
@skriss thanks for the review. I have refactored the PR to use a GatewayClass owning label. The service watch event handler of the gateway controller now looks for the GatewayClass owning label. If found, it will load all the Gateways from the resource map and create a reconcile request for each Gateway that references a GatewayClass that matches the owning label. |
skriss
left a comment
skriss
left a comment
There was a problem hiding this comment.
This makes sense to me based on the current relationship between GatewayClass and infra/a Service, can always revisit if/when needed.
4 participants
labels.go.Requires #350
Fixes #330