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

Add e2e test coverage for a partial mesh #4028

Closed
markusthoemmes opened this issue May 7, 2019 · 9 comments · Fixed by #4493
Closed

Add e2e test coverage for a partial mesh #4028

markusthoemmes opened this issue May 7, 2019 · 9 comments · Fixed by #4493
Assignees
Labels
area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Milestone

Comments

@markusthoemmes
Copy link
Contributor

#3620 added the possibility to opt-out of the mesh on the revision pod side. That'd mean that potentially the system's pods (including the activator) have sidecars while the user pods don't. It also means that different revisions can have sidecars, while others don't.

We need these tests to make sure that neither permutation breaks.

  1. Scale from 0 with sidecar disabled on revision
  2. Service A (with sidecar) to Service B (without sidecar)
  3. Service B (without sidecar) to Service A (with sidecar)
@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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 7, 2019
@yu2003w
Copy link
Contributor

yu2003w commented Jun 6, 2019

I will start working on this item.

@adrcunha
Copy link
Contributor

/assign @yu2003w

@knative-prow-robot
Copy link
Contributor

@adrcunha: GitHub didn't allow me to assign the following users: yu2003w.

Note that only knative members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @yu2003w

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tcnghia tcnghia added this to the Serving 0.8 milestone Jun 22, 2019
@tcnghia
Copy link
Contributor

tcnghia commented Jun 22, 2019

@yu2003w thanks, do you think this can be done in https://github.com/knative/serving/milestone/24 (due August 06?)

@yu2003w
Copy link
Contributor

yu2003w commented Jun 22, 2019

@tcnghia Sure, I will submit PR next week. I planned to submit this PR last week. As a little busy recently, the work delayed.

@yu2003w
Copy link
Contributor

yu2003w commented Jun 25, 2019

@markusthoemmes @mattmoor @bbrowning I submitted PR for this issue.
But for test 2, 'Service A (with sidecar) to Service B (without sidecar)', when FQDN is {route}.{namespace}.svc or {route}.{namespace}, tests failed with error as below,

        service.go:127: Waiting for Service "svc-to-svc-without-sidecar-call-short-ktanatjd" to transition to Ready.
        service.go:132: Checking to ensure Service Status is populated for Ready service svc-to-svc-without-sidecar-call-short-ktanatjd
        service.go:138: Getting latest objects Created by Service svc-to-svc-without-sidecar-call-short-ktanatjd
        service.go:141: Successfully created Service svc-to-svc-without-sidecar-call-short-ktanatjd
        service_to_service_test.go:109: Failed to start endpoint of httpproxy: response: status: 404, body: , headers: map[Content-Length:[0] Date:[Tue, 25 Jun 2019 14:59:02 GMT] Server:[envoy] X-Envoy-Upstream-Service-Time:[2] Zipkin_trace_id:[64ff216adbd75982542549abdac1d0c3]] did not pass checks: timed out waiting for the condition
FAIL
FAIL	github.com/knative/serving/test/e2e	317.912s

New added tests make use of existing service_to_service_call scenario for the tests.

httpproxy is a proxy that redirects request to internal service of helloworld app
with FQDN {route}.{namespace}.svc.cluster.local, or {route}.{namespace}.svc, or
{route}.{namespace}.

I also tried following commands within pod of http proxy,

root@filmy1:/mnt/data/gowork/src/github.com/knative/serving# kubectl -n serving-tests exec -it -c user-container svc-to-svc-without-sidecar-call-short-ktanatjd-5gqpp-deploxhdm2 nslookup svc-to-svc-without-sidecar-call-tyrqaper.serving-tests.svc
Server:		10.0.0.10
Address:	10.0.0.10#53

svc-to-svc-without-sidecar-call-tyrqaper.serving-tests.svc.cluster.local	canonical name = cluster-local-gateway.istio-system.svc.cluster.local.
Name:	cluster-local-gateway.istio-system.svc.cluster.local
Address: 10.0.186.143

It works with nslookup. In httpproxy.go, the related code is below,

func handler(w http.ResponseWriter, r *http.Request) {
	log.Print("HTTP proxy received a request.")
	// Reverse proxy does not automatically reset the Host header.
	// We need to manually reset it.
	r.Host = getTargetHostEnv()
	httpProxy.ServeHTTP(w, r)
}

I suspect that httputil.ReverseProxy.ServerHttp() couldn't handle short FQDN.
What's your opinion?

@tcnghia
Copy link
Contributor

tcnghia commented Jul 9, 2019

@yu2003w we can check in the tests for partial mesh connectivity using FQDN first, and follow up with short names.

Currently we already have some short names coverage using existing tests.

@markusthoemmes
Copy link
Contributor Author

/assign yu2003w

@yu2003w
Copy link
Contributor

yu2003w commented Jul 15, 2019

@tcnghia I updated code again and only added tests for FQDN, did not touch short names.
If I misunderstood something, please let me know. Thanks a lot.

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants