VirtualService fails to create due to Istio 1.4's regex engine check if ksvc has a little longer domain

[nak3]

In what area(s)?

/area networking /area test-and-release /kind spec

What version of Knative?

Istio 1.4.x or later.

Actual Behavior

As I mentioned https://github.com/knative/serving/issues/6039#issuecomment-554630836, Istio 1.4 introduced Regex EngineChanges / https://github.com/istio/istio/pull/18539 so most of E2E tests fail due to exceed the size of regex (=only 100 bytes) in VirtualService.

e.g:

    match:
    - authority:
        regex: ^service-to-service-call-via-activator-both-disabled-rlzfmjaj\.serving-tests\.example\.com(?::\d{1,5})?$

Then, we will get following error and VirtualService is not created. {"level":"error","ts":"2019-11-16T08:13:54.839Z","logger":"istiocontroller.ingress-controller","caller":"controller/controller.go:368","msg":"Reconcile error","commit":"f961c64","knative.dev/controller":"ingress-controller","error":"failed to create VirtualService: admission webhook \"pilot.validation.istio.io\" denied the request: configuration is invalid: regex match '^service-to-service-call-via-activator-both-disabled-rlzfmjaj\\.serving-tests(\\.svc(\\.cluster\\.local)?)?(?::\\d{1,5})?$' cannot be greater than 100 bytes","stacktrace":"knative.dev/serving/vendor/knative.dev/pkg/controller.(*Impl).handleErr\n\t/home/knakayam/.go/src/knative.dev/serving/vendor/knative.dev/pkg/controller/controller.go:368\nknative.dev/serving/vendor/knative.dev/pkg/controller.(*Impl).processNextWorkItem\n\t/home/knakayam/.go/src/knative.dev/serving/vendor/knative.dev/pkg/controller/controller.go:354\nknative.dev/serving/vendor/knative.dev/pkg/controller.(*Impl).Run.func2\n\t/home/knakayam/.go/src/knative.dev/serving/vendor/knative.dev/pkg/controller/controller.go:302"}

Steps to Reproduce the Problem

1. Install Istio 1.4.x
2. Run E2E test or deploy ksvc with long name.

[nak3]

It looks like we have a few options.

A. Modify ObjectNameForTest() to create shorter ksvc name for tests. B. Use PILOT_ENABLE_UNSAFE_REGEX=true for Istio Pilot.

I am thinking that we need optionA rather than B, but is there any other idea?

cc @tcnghia @mattmoor

[vagababov]

Also we can use pkg/kmeta/ChildName. I wonder how does that surface in the ksvc/revision status? I.e. if users actually use this long names, how can they understand that this is the cause?

On Sun, Nov 17, 2019 at 6:09 PM Kenjiro Nakayama notifications@github.com wrote:

It looks like we have a few options.

A. Modify ObjectNameForTest() https://github.com/knative/serving/blob/e552aaa9d7142a242b83ff4729a7157730262e92/vendor/knative.dev/pkg/test/helpers/name.go#L49-L52 to create shorter ksvc name for tests. B. Use PILOT_ENABLE_UNSAFE_REGEX=true for Istio Pilot.

I am thinking that we need optionA rather than B, but is there any other idea?

cc @tcnghia https://github.com/tcnghia @mattmoor https://github.com/mattmoor

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/knative/serving/issues/6058?email_source=notifications&email_token=AAF2WX4OTMPPCYGGJ3H2J3TQUH2N5A5CNFSM4JONAY22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEI6IQY#issuecomment-554820675, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF2WX6QDGQPVNMVOLN7ITTQUH2N5ANCNFSM4JONAY2Q .

[nak3]

Also we can use pkg/kmeta/ChildName.

Oh, I see. Thank you.

I wonder how does that surface in the ksvc/revision status?

For now, users can notice it by Ingress's status field (if https://github.com/knative/serving/pull/6048 is merged) and event log (current event log is already reveals this error).

c.f. ingress's status field by adding https://github.com/knative/serving/pull/6048.

  conditions:
  - lastTransitionTime: "2019-11-16T11:41:46Z"
    message: 'failed to create VirtualService: admission webhook "pilot.validation.istio.io"
      denied the request: configuration is invalid: regex match ''^service-to-service-call-via-activator-both-disabled-vktmmxev\.serving-tests(\.svc(\.cluster\.local)?)?(?::\d{1,5})?$''
      cannot be greater than 100 bytes'
    reason: ReconcileVirtualServiceFailed
    status: "False"

ksvc will just say IngressNotConfigured or something, though.

[nak3]

One note, this error does not happen with istio-learn for now. https://github.com/istio/istio/pull/18539 implements for Galley validation so following setting is needed:

(e.g) 1.3.3 does not have the validation, but just as a config sample. https://github.com/knative/serving/blob/6c8bbf02e13576014fd4527a0c31c374a94d3fba/third_party/istio-1.3.3/istio.yaml#L196

[ZhiminXiang]

It looks like we have a few options.

A. Modify ObjectNameForTest() to create shorter ksvc name for tests.

This option seems like hiding the real issue in our E2E tests as users could create ksvc with long name.

[nak3]

@ZhiminXiang Thanks. Yes, I realized that it is not good option and so filed https://github.com/knative/serving/pull/6088 to use prefix instead of regex. (prefix does not have the limit.)

[duglin]

should using

---

B. Use PILOT_ENABLE_UNSAFE_REGEX=true for Istio Pilot.

---

work around it? It doesn't seem to work for me

[nak3]

@duglin Hmm... If it does not work, I think that it is an Istio's bug. The config is mentioned in the Istio's release note.

https://istio.io/news/releases/1.4.x/announcing-1.4/upgrade-notes/#regex-engine-changes

If you depend on specific behavior of the old regex engine, you can opt out of this change by adding the environment variable PILOT_ENABLE_UNSAFE_REGEX=true to the Pilot deployment. Note: this will be removed in future releases.

Anyway, https://github.com/knative/serving/pull/6088 is merged so Knative does not need it.

[duglin]

I'll try it again. Was just hoping it would work then I could apply that patch to our v0.10 stuff :-(

[nak3]

I am feeling that Istio needs https://github.com/istio/istio/pull/19089/files (it is NOT included in Istio 1.4 branch?!) But anyway, it would be an Istio topic rather than Knative.

[duglin]

I'm guessing that PR isn't part of Istio 1.4 Bummer