kubernetes-sigs / ingress-controller-conformance

Repository for a compliance specification of ingress-controllers.
Apache License 2.0
43 stars 36 forks source link

Remove prefix path rule scenario #85

Closed aledbf closed 3 years ago

aledbf commented 3 years ago

According to the KEP, this is the expected behavior:

A path element refers is the list of labels in the path split by the '/' separator. A request is a match for path p if every p is an element-wise prefix of p of the request path. Note that if the last element of the path is a substring of the last element in request path, it is not a match (e.g. /foo/bar matches /foo/bar/baz, but does not match /foo/barbaz)

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190125-ingress-api-group.md#prefix-match

Now, imposing this behavior in the conformance suite means (at least for ingress-nginx) a breaking change that does not add any benefits. Can we remove this scenario?

cc @bowei @robscott

Note: other ingress controllers like Kong are also impacted by this scenario. this was an mistake

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aledbf To complete the pull request process, please assign thockin after the PR has been reviewed. You can assign the PR to them by writing /assign @thockin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-sigs/ingress-controller-conformance/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
hbagdi commented 3 years ago

Note: other ingress controllers like Kong are also impacted by this scenario.

Kong does follow that spec for Ingress v1 resource: https://github.com/Kong/kubernetes-ingress-controller/blob/8c9484644823191b533ee3493afd755a3052c034/internal/ingress/controller/parser/translate.go#L525

A /foo prefix match is translated into nginx as /foo$ and /foo/ to ensure that it doesn't match /foobar.

cc @mflendrich @rainest

aledbf commented 3 years ago

A /foo prefix match is translated into nginx as /foo$ and /foo/ to ensure that it doesn't match /foobar.

@hbagdi so you generate two locations for that scenario? What if the user also defines an PathTypeExact?

hbagdi commented 3 years ago

match is translated into nginx

As @mflendrich pointed out out of band, I meant Kong here.

@hbagdi so you generate two locations for that scenario?

Kong doesn't generate location blocks in nginx directly. We populate two paths in a single Route for this use case.

What if the user also defines an PathTypeExact?

In that case, only a single path is populated /foo$.

mflendrich commented 3 years ago

What if the user also defines an PathTypeExact?

To add to @hbagdi's response above:

If a user defines an Exact rule and a Prefix rule that both translate to /foo$ on the Kong side, then the following KEP rule:

Exact match is preferred to a Prefix match.

is followed because Kong ingress controller defines them in Kong with different priorities.