Ambassador should route any knative service endpoint

[maticue]

Describe the bug

Per default knative services with endpoints different than / are not routed by ambassador.

Let's suppose the following curl:

curl -v "http://event-display.default.svc.cluster.local/hello" -X POST -H "Content-Type: application/json" -d '{"foo":"bar"}'

Ambassador logs show No Route value:

ACCESS [2021-02-12T23:18:02.936Z] "POST /hello HTTP/1.1" 404 NR 0 0 0 - "10.233.88.35" "curl/7.75.0" "ad25b510-5b9e-48c2-a3de-db876fc414b5" "event-display.default.svc.cluster.local" "-"

This is because Ambassador default mapping for knative routing has:

prefix: /
prefix_regex: true

which, based on ambassador documentation:

When the prefix_regex attribute is set to true, Ambassador Edge Stack configures a regex route instead of a prefix route in Envoy. This means the entire path must match the regex specified, not only the prefix.

the scope is limited to only /.

On the other hand, knative service definition does not allow to set up path attribute which is expected by knative.py. Knative also creates a knative route + knative ingress when a knative service is created.

We can workaround this creating another knative ingress manifest different from the default one created automatically by knative.

To Reproduce
Steps to reproduce the behavior:

1. Install ambassador and knative. Configure them to work together.
2. Create any knative service like the following

apiVersion: serving.knative.dev/v1 # Current version of Knative
kind: Service
metadata:
  name: helloworld-go # The name of the app
spec:
  template:
    spec:
      containers:
        - image: gcr.io/knative-samples/helloworld-go # Reference to the image of the app
          env:
            - name: TARGET # The environment variable printed out by the sample app
              value: "Go Sample v1"

2. Wait until ambassador mapping is created
3. Try to access to GET any endpoint for the designated service different than /. Like /hello
4. Get a 404 and see the error on ambassador logs

Expected behavior
If the backend service has a service running on /hello, I should see a 2xx HTTP code.

Versions (please complete the following information):

• Ambassador: 1.11.1
• Kubernetes environment: OKE
• Version 1.18.10

Additional context
We could just set the prefix_regex option to false per default. If more specific configuration is required, then users could create their own knative ingress

[impl]

Hi @maticue,

Thanks for reporting this. It looks like our behavior indeed deviates from the Knative spec. Note that we're not working with a Knative Service/Revision here, we're rather working with an internal type, an Ingress under networking.internal.knative.dev/v1alpha1. I don't believe it's exposed in the docs anywhere as its consumption is intended for implementers of Knative gateways.

Specifically the field in question is here: https://github.com/knative/serving/blob/130f8ace745397cbee16fb549664d91f08e8baa6/vendor/knative.dev/networking/pkg/apis/networking/v1alpha1/ingress_types.go#L212-L220

One unfortunate consequence of that spec description is that because we forward this on to Envoy the regex syntax doesn't exactly line up with what they ask for (Envoy regexes are these, which are not POSIX extended regexes). I'm not sure if there's anything we can practically do about that or if it even matters for most users (probably not).

That said, it looks like the default for the path should definitely be /.* instead of /, and that is a change I would be happy to make (or if you'd like to update your PR, feel free to do so!). I won't have a chance to actually put up a new PR until some time tomorrow.

Thanks again for tracking this down so helpfully!

[impl]

Actually, a second reading of this doc comment makes me wonder if we should be appending .* to every path value for compatibility. It looks like the intention here is to be a prefix match. @kflynn what do you think?

[impl]

It looks like the Knative team views it as a prefix in their own Envoy shim, so I am going to go ahead and do the same: https://github.com/knative-sandbox/net-contour/blob/3df437f/pkg/reconciler/contour/resources/httpproxy.go#L189-L194

[impl]

After talking to Flynn and the Knative folks, it sounds like literal prefix-matching is the right way to go. We went ahead and updated the documentation on the Knative side so we'll be in conformance with the spec once the fix to this ticket lands. You closed your original PR, @maticue, but I updated mine (#3232) to match yours.

[kflynn]

Thanks, @impl and @maticue!

[impl]

@kflynn With the release of 1.13, I think this issue can be closed. 👯

[kflynn]

👍