Set up watchable to communicate between components#193
Set up watchable to communicate between components#193arkodg merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Luke Shumaker <[email protected]>
…ol-plane Signed-off-by: Luke Shumaker <[email protected]>
So I notice that I forgot to set the env-var when running the server manually. Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: danehans <[email protected]>
danehans
added
the
area/message-service
| "name", request.Name) | ||
| // TODO: Delete gateway from the IR. | ||
| // xref: https://github.com/envoyproxy/gateway/issues/38 | ||
| for namespacedName := range r.resourceTable.Gateways.LoadAll() { |
There was a problem hiding this comment.
why are we deleting all the gateways here instead of just the one thats missing a valid gateway class ?
There was a problem hiding this comment.
I think the idea is that if there's no GatewayClass with a matching controller string that has a condition of "Accepted: true", then we should not be programming anything, therefore, we need to ensure all Gateways have been removed from the resource table.
| Namespace: acceptedGateways[i].GetNamespace(), | ||
| } | ||
| r.resourceTable.Gateways.Store(key, &acceptedGateways[i]) | ||
| if key == request.NamespacedName { |
There was a problem hiding this comment.
wouldn't it be easier to reset r.resourceTable.Gateways and fill it up based on the contents of acceptedGateways ?
There was a problem hiding this comment.
Yes, let's fix in a follow-up PR.
There was a problem hiding this comment.
Agree with ^^, that should also better handle the case where the accepted GatewayClass changes (i.e. there used to be gc-1 and gc-2, with gc-1 being accepted, and then gc-1 gets deleted, now we should be processing gc-2) and we now need to remove all the Gateways for the old GatewayClass, and add the new ones.
skriss
left a comment
skriss
left a comment
There was a problem hiding this comment.
The basic structure LGTM, I did have some comments on the logic within the gatewayclass and gateway controllers, but if you want to get this in to get the structure established and then follow up on the exact insertion/deletion logic, that's fine w me.
| "name", request.Name) | ||
| // TODO: Delete gateway from the IR. | ||
| // xref: https://github.com/envoyproxy/gateway/issues/38 | ||
| for namespacedName := range r.resourceTable.Gateways.LoadAll() { |
There was a problem hiding this comment.
I think the idea is that if there's no GatewayClass with a matching controller string that has a condition of "Accepted: true", then we should not be programming anything, therefore, we need to ensure all Gateways have been removed from the resource table.
| Namespace: acceptedGateways[i].GetNamespace(), | ||
| } | ||
| r.resourceTable.Gateways.Store(key, &acceptedGateways[i]) | ||
| if key == request.NamespacedName { |
There was a problem hiding this comment.
Agree with ^^, that should also better handle the case where the accepted GatewayClass changes (i.e. there used to be gc-1 and gc-2, with gc-1 being accepted, and then gc-1 gets deleted, now we should be processing gc-2) and we now need to remove all the Gateways for the old GatewayClass, and add the new ones.
| } | ||
|
|
||
| updater := func(gc *gwapiv1b1.GatewayClass, accepted bool) error { | ||
| r.resourceTable.GatewayClasses.Store(gc.GetName(), gc) |
There was a problem hiding this comment.
Here I think we need to store the GC that is "Accepted: true", and remove all the other GCs with a matching controller that are "Accepted: false".
Note from past experience implementing this -- think about what happens if you have multiple GatewayClasses matching the controller string, and then the one that's currently "Accepted: true" gets deleted. Now we need to mark a new GatewayClass as "Accepted: true" (even though it itself has not changed or triggered a reconcile), and then also ensure all of the old GatewayClass's Gateways are removed from our internal model, and all of the Gateways for the new GatewayClass get added (even though the Gateways themselves may not have changed so are not triggering reconciles).
|
merged this one to unblock other PRs, hoping the comments can be address in follow up PRs |
5 participants
This PR pulls in https://github.com/telepresenceio/watchable/ to use for communication between components. This is PR #191 from @LukeShu with a commit to fix the failing Kube provider test.
Fixes: #34
Fixes: #38