-
Notifications
You must be signed in to change notification settings - Fork 548
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
fix fallback envoy version check bug #441
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
==========================================
+ Coverage 37.24% 37.49% +0.24%
==========================================
Files 65 65
Lines 9215 9215
==========================================
+ Hits 3432 3455 +23
+ Misses 5517 5495 -22
+ Partials 266 265 -1 |
name: higress-conformance-infra-default-backend | ||
namespace: higress-conformance-infra | ||
annotations: | ||
nginx.ingress.kubernetes.io/default-backend: "foo.com/foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation value should be a service name in the same namespace, see: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#default-backend
}, | ||
Response: http.AssertionResponse{ | ||
ExpectedResponse: http.Response{ | ||
StatusCode: 200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This e2e test case doesn't seem to make sense, you should check that request is routed to the default backend service when the ingress's service doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also check the fallback via the normal ingress and the corresponding canary ingress.
For example,
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: normal
namespace: default
spec:
ingressClassName: higress
rules:
- host: test.com
http:
paths:
- backend:
service:
name: demo
port:
number: 8080
path: /header
pathType: ImplementationSpecific
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
nginx.ingress.kubernetes.io/canary: 'true'
nginx.ingress.kubernetes.io/canary-by-header: x-user-id
nginx.ingress.kubernetes.io/canary-by-header-value: "100"
name: canary
namespace: default
spec:
ingressClassName: higres
rules:
- host: test.com
http:
paths:
- backend:
service:
name: demo-canary
port:
number: 8080
path: /header
pathType: ImplementationSpecific
If the service demo-canry doesn't exist, the following request will fallback to demo.
curl -H "host: test.com" -H "x-user-id: 100" x.x.x.x/header
Ⅰ. Describe what this PR did
Our Evnoy version is 1.20, while the fallback feature requires Envoy version at lease 1.20.6.
The initial version higress-data-plane support fallback, we can remove this version check directly.
Ⅱ. Does this pull request fix one issue?
fixes #431
Ⅲ. Why don't you add test cases (unit test/integration test)?
I added test file to
test/e2e/conformance/tests
Ⅳ. Describe how to verify it
run
make higress-conformance-test
Ⅴ. Special notes for reviews