Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nak3 opened this issue Nov 18, 2019 · 11 comments · Fixed by #6088
Labels
area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/bug Categorizes issue or PR as related to a bug. kind/spec Discussion of how a feature should be exposed to customers.

Comments

@nak3
Copy link
Contributor

nak3 commented Nov 18, 2019

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 #6039 (comment), Istio 1.4 introduced Regex EngineChanges / istio/istio#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 nak3 added the kind/bug Categorizes issue or PR as related to a bug. label Nov 18, 2019
@knative-prow-robot knative-prow-robot added area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/spec Discussion of how a feature should be exposed to customers. labels Nov 18, 2019
@nak3
Copy link
Contributor Author

nak3 commented Nov 18, 2019

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
Copy link
Contributor

vagababov commented Nov 18, 2019 via email

@nak3
Copy link
Contributor Author

nak3 commented Nov 18, 2019

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 #6048 is merged) and event log (current event log is already reveals this error).

c.f. ingress's status field by adding #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
Copy link
Contributor Author

nak3 commented Nov 20, 2019

One note, this error does not happen with istio-learn for now. istio/istio#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.

- name: pilot.validation.istio.io

nak3 added a commit to nak3/serving that referenced this issue Nov 22, 2019
This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```
@ZhiminXiang
Copy link

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 added a commit to nak3/serving that referenced this issue Nov 24, 2019
This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```
@nak3
Copy link
Contributor Author

nak3 commented Nov 24, 2019

@ZhiminXiang Thanks. Yes, I realized that it is not good option and so filed #6088 to use prefix instead of regex. (prefix does not have the limit.)

@duglin
Copy link

duglin commented Nov 25, 2019

should using


B. Use PILOT_ENABLE_UNSAFE_REGEX=true for Istio Pilot.


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

knative-prow-robot pushed a commit that referenced this issue Nov 25, 2019
)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in #6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
@nak3
Copy link
Contributor Author

nak3 commented Nov 25, 2019

@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, #6088 is merged so Knative does not need it.

@duglin
Copy link

duglin commented Nov 26, 2019

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

@nak3
Copy link
Contributor Author

nak3 commented Nov 26, 2019

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
Copy link

duglin commented Nov 26, 2019

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

tcnghia pushed a commit to tcnghia/serving that referenced this issue Dec 9, 2019
…ative#6088)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
tcnghia pushed a commit to tcnghia/serving that referenced this issue Dec 9, 2019
…ative#6088)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
knative-prow-robot pushed a commit that referenced this issue Dec 9, 2019
) (#6183)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in #6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
markusthoemmes pushed a commit to markusthoemmes/knative-serving that referenced this issue Dec 10, 2019
…ative#6088) (knative#6183)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
openshift-merge-robot pushed a commit to openshift/knative-serving that referenced this issue Dec 10, 2019
* squash (knative#6174) (knative#6175)

* Use prefix instead of regex for authority match in virtualservice (knative#6088) (knative#6183)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
tcnghia pushed a commit to tcnghia/net-istio that referenced this issue Mar 4, 2020
…088)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative/serving#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
tcnghia pushed a commit to tcnghia/net-istio that referenced this issue Mar 4, 2020
…088)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative/serving#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
mattmoor pushed a commit to knative-extensions/net-istio that referenced this issue Mar 5, 2020
…088)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative/serving#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/bug Categorizes issue or PR as related to a bug. kind/spec Discussion of how a feature should be exposed to customers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants