put matches first when building routes #5926
Conversation
|
/ok-to-test |
| cr.Match = createMatchRequest(route) | ||
| vs.Http = append([]*networking.HTTPRoute{cr}, vs.Http...) | ||
| } else { | ||
| vs.Http = append(vs.Http, cr) |
There was a problem hiding this comment.
aren't this line and line 97 duplicates? I think we can rewrite this whole block to:
cr := createRoute(route)
cr.Route[0].Destination.Subset = route.GetCapiProcessGuid()
if route.GetPath() != "" {
cr.Match = createMatchRequest(route)
}
vs.Http = append(vs.Http, cr)Or was the goal to differentiate append(cr, vs.Http...) vs append(vs.Http, cr)? If the goal is to fix that discrepancy, then I think you'd be better off sorting the HTTP routes by these conditions before returning it, rather than keeping the ordering as we process the list (IMO it's more obvious, and it'll be easier to change later if match ordering needs to change for some other reason down the line).
There was a problem hiding this comment.
We considered this but then you have the mess of what is potentially a nil pointer (there may not be a match for the route you are looking at).
There was a problem hiding this comment.
IMO sorting is a cleaner/better way to implement this because it consolidates the logic into a single location and it's fairly obvious what's happening. The impl as-is is a big tripping hazard IMO: that logic diff is subtle and easy to change later. Either way, doesn't warrant blocking the PR, though I would recommend adding a comment describing what's happening here.
There was a problem hiding this comment.
cool, added a comment in cc2229b
hoping to refactor that soon. thanks for the input @ZackButcher
| @@ -70,7 +70,7 @@ func TestCloudFoundrySnapshot(t *testing.T) { | |||
| g.Expect(virtualService.Gateways).To(gomega.ConsistOf([]string{"some-gateway", "some-other-gateway"})) | |||
|
|
|||
| g.Expect(virtualService.Http).To(gomega.HaveLen(2)) | |||
| g.Expect(virtualService.Http).To(gomega.ConsistOf([]*networking.HTTPRoute{ | |||
| g.Expect(virtualService.Http).To(gomega.Equal([]*networking.HTTPRoute{ | |||
There was a problem hiding this comment.
Equal enforces element ordering where ConsistOf doesn't, I assume?
|
/test istio-pilot-e2e |
2 similar comments
|
/test istio-pilot-e2e |
|
/test istio-pilot-e2e |
* use Gateway and VirtualService to expose helloworld service * readme fix
istio#5873) * Update Jaeger version and add limit on the number of traces held in memory * Use tag 1 to pick up latest stable versions * Use tag 1.5
otherwise, pilot will not enumerate the routes past the base route
cc2229b to
a30139a
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: utako Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@utako: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
otherwise, pilot will not enumerate the routes past the base route.
thanks all.