Skip to content

Conversation

youngnick
Copy link
Contributor

This PR fixes the sorting logic for Ingress (and, by extension, Gateway API), by fixing the model translation output step so that ImplementationSpecific (that is, regex) matches always come before Prefix matches, but after Exact matches.

It also updates the testing to ensure that changes here are caught, and updates some wording in the godoc to clarify the algorithm used for translation out of the model to Envoy config.

Fixes: #28852

`ImplementationSpecific` Ingress paths (which for Cilium Ingress means regex path matches) are now sorted correctly in between `Exact` and `Prefix` matches.

@youngnick youngnick requested a review from a team as a code owner November 27, 2023 08:27
@youngnick youngnick requested a review from meyskens November 27, 2023 08:27
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 27, 2023
@youngnick youngnick added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Nov 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 27, 2023
@youngnick youngnick force-pushed the ingress-path-testing branch 2 times, most recently from 08804f3 to 8f7ad45 Compare November 27, 2023 08:36
@sayboras
Copy link
Member

/test

@youngnick
Copy link
Contributor Author

Looks like the clustermesh tests failed to pull the images, retrying.

@youngnick
Copy link
Contributor Author

/test ci-clustermesh

Copy link
Contributor

@meyskens meyskens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 29, 2023
@youngnick youngnick added this pull request to the merge queue Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023
@youngnick youngnick added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@youngnick youngnick added this pull request to the merge queue Nov 30, 2023
@youngnick youngnick removed this pull request from the merge queue due to a manual request Nov 30, 2023
@youngnick youngnick added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@youngnick youngnick added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@youngnick youngnick force-pushed the ingress-path-testing branch 2 times, most recently from d4362e5 to 9f2bafe Compare November 30, 2023 23:23
@youngnick
Copy link
Contributor Author

/test

This commit makes a few changes:

Adds a new test for ingestion, that tests how multiple path matches in the
same Ingress work

Updates translation logic to sort into the order Exact, Regex, Prefix,
which means that regex matches that overlap with prefixes will now
route traffic.

Adds more cases to the unit tests for SortableRoute.Less(), which ensures
that more options are covered.

Also refactors the TestSharedIngressTranslator_getEnvoyHTTPRouteConfiguration test
to also check the contents of the path and cluster matches, as well as just the
listener name and virtual host name.

This was quite a bit more complex, because just comparing protobufs produces
very unhelpful failure outputs. This test design makes it more straightforward
to correct errors in the expected results.

Signed-off-by: Nick Young <nick@isovalent.com>
@youngnick youngnick force-pushed the ingress-path-testing branch from 9f2bafe to b3f99db Compare December 1, 2023 02:34
@mhofstetter mhofstetter added kind/bug This is a bug in the Cilium logic. area/servicemesh GH issues or PRs regarding servicemesh labels Dec 1, 2023
@aanm
Copy link
Member

aanm commented Dec 1, 2023

/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-ingress kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingress: incompatible with cert-manager ACME HTTP-01
5 participants