Update outbound policy watches when routes change parents #13315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alex Leong <[email protected]>
Signed-off-by: Alex Leong <[email protected]>
olix0r
approved these changes
Nov 13, 2024
| } | ||
|
|
||
| // We must send the route update to all namespace indexes in case this | ||
| // route's parent_refs have changed and this route must be removed be |
There was a problem hiding this comment.
Suggested change
| // route's parent_refs have changed and this route must be removed be | |
| // route's parent_refs have changed and this route must be removed by |
| } | ||
|
|
||
| // We must send the route update to all namespace indexes in case this | ||
| // route's parent_refs have changed and this route must be removed be |
There was a problem hiding this comment.
Suggested change
| // route's parent_refs have changed and this route must be removed be | |
| // route's parent_refs have changed and this route must be removed by |
| } | ||
|
|
||
| // We must send the route update to all namespace indexes in case this | ||
| // route's parent_refs have changed and this route must be removed be |
There was a problem hiding this comment.
Suggested change
| // route's parent_refs have changed and this route must be removed be | |
| // route's parent_refs have changed and this route must be removed by |
| } | ||
|
|
||
| // We must send the route update to all namespace indexes in case this | ||
| // route's parent_refs have changed and this route must be removed be |
There was a problem hiding this comment.
Suggested change
| // route's parent_refs have changed and this route must be removed be | |
| // route's parent_refs have changed and this route must be removed by |
Comment on lines
1447
to
1450
| let (kind, group) = match resource_port.kind { | ||
| ResourceKind::Service => ("Service", "core"), | ||
| ResourceKind::EgressNetwork => ("EgressNetwork", "policy.linkerd.io"), | ||
| }; |
There was a problem hiding this comment.
It would be ideal to use the type-oriented helpers
Suggested change
| let (kind, group) = match resource_port.kind { | |
| ResourceKind::Service => ("Service", "core"), | |
| ResourceKind::EgressNetwork => ("EgressNetwork", "policy.linkerd.io"), | |
| }; | |
| let (kind, group) = match resource_port.kind { | |
| ResourceKind::Service => (Service::kind(&()), Service::group(&())), | |
| ResourceKind::EgressNetwork => (linkerd_k8s_api::EgressNetwork::kind(&()), linkerd_k8s_api::EgressNetwork::group(&())), | |
| }; |
So that we have more typo-proof static checking.
Signed-off-by: Alex Leong <[email protected]>
zaharidichev
approved these changes
Nov 14, 2024
| @@ -217,6 +217,49 @@ pub async fn await_route_status( | |||
| route_status | |||
| } | |||
|
|
|||
| // Waits until an HttpRoute with the given namespace and name has a status set | |||
| // on it, then returns the generic route status representation. | |||
| pub async fn await_gateway_route_status( | |||
There was a problem hiding this comment.
Question: Is this change directly related or have you introduced this method in order to minimize flakiness that you have observed. I am asking because I myself am experiencing some flakiness because of delay in status setting, especially when the status controller has some more work to do.
There was a problem hiding this comment.
To resolve flakiness that was causing tests to fail. I don't think it's directly related to this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13280
When an xRoute resource is updated to change its parent_refs, the route may attach to new parents or become unattached to parents it was previously attached to. However, in the policy-controller, the xRoute update will only be sent to the parents on the new version of the route resource and any existing watches on parents that the route was unattached from will not be updated.
Unfortunately, the kube-rs watch interface only provides the new version of a resource when it is updated, and not the previous state. This means that we cannot know which parents the route might have been unattached to when it was updated. Therefore, we send the route update to all NamespaceIndexes and each one removes that route from each parent if that parent is not currently an accepted parent of the route.
We also add integration tests for this behavior.
This issue applies to all xRoutes: policy HttpRoute, gateway HttpRoute, GrpcRoute, TlsRoute, and TcpRoute.