simpler, additional e2e functional test + auth test #833

ldemailly · 2017-09-16T02:27:33Z

This is for a couple things:

a) an e2e test (using the e2e framework) that has minimum dependencies and fast start which can be use as a starting point for new tests that do not rely on bookinfo
b) exercise fortio images, ingress rule svc2sc
c) detect whether auth is working or not
d) bug fix in fortio when server errors out
e) faster test start and end (and updated README with the faster instructions)

background/justification
We all spent a lot of time debugging complex end2end test over the last few weeks,
Also people have been complaining that writing new tests is hard (which is partially true as I learned through writing one), and we also don't have test verifying auth is really on

This PR makes progress toward solving all 3 problems, introducing a simpler (than mixer/bookinfo) test.

A simpler test lets us quickly smoke test basic features.

This is additional to tests that exists in pilot (as this is about end2end with all the actual components in place, same code as we release)

e2e is not exclusively "website tasks testing" and should have as many sane tests as we can have

A separate effort - though this simpler test does help already - is to optimize the runtime and probably split test into "every PR" tests and "nightly"... etc...

New simpler e2e functional tests, including an auth test.

ldemailly · 2017-09-16T02:28:43Z

istio.VERSION

 export PILOT_HUB="gcr.io/istio-testing"
 export PILOT_TAG="3ac357f84576f881d9f57ed76e4e14a052ac5333"
+export ISTIOCTL_URL="https://storage.googleapis.com/istio-artifacts/pilot/3ac357f84576f881d9f57ed76e4e14a052ac5333/artifacts/istioctl"


moved after PILOT_TAG so we can eventually change it to
export ISTIOCTL_URL="https://storage.googleapis.com/istio-artifacts/pilot/$PILOT_TAG/artifacts/istioctl"

ldemailly · 2017-09-16T02:29:37Z

istio.VERSION

-
+export PROXY_DEBIAN_URL="https://storage.googleapis.com/istio-artifacts/proxy//artifacts/debs"
+export FORTIO_HUB="gcr.io/istio-testing"
+export FORTIO_TAG="a9b5395d6fe2e8cbcbe9b0835d0b48e88620b878"


this tag is from a bot presubmit, the artefact building is missing so far from postsubmit (separate pr)

ldemailly · 2017-09-16T02:30:13Z

tests/e2e/tests/simple/servicesToBeInjected.yaml

+  rules:
+  - http:
+      paths:
+      - path: /.*


doesn't seem to work (as I expect) with a different path e.g /fortio/.* if I call fortio/debug it doesn't work (as in it doesn't call fortio with /debug)

istio-merge-robot · 2017-09-16T03:08:57Z

@ldemailly PR needs rebase

# Conflicts: # istio.VERSION

plus todo

andraxylia · 2017-09-16T17:57:09Z

Can you add more details in the description about what it is testing?

fortio: added -echo-debug-path option to change the /debug to something else (because I can’t get rewrite rules to work yet, but it’s also a useful feature anyway) Rewrite rule seems to do nothing Also observed I can’t talk pod to pod getting 404s somehow

istio-merge-robot · 2017-09-17T00:31:37Z

@ldemailly PR needs rebase

istio-merge-robot · 2017-09-18T08:00:34Z

@ldemailly PR needs rebase

sebastienvas · 2017-09-18T16:07:51Z

devel/fortio/http.go

@@ -59,7 +59,7 @@ func init() {

 // Version is the fortio package version (TODO:auto gen/extract).
 const (
-	Version       = "0.2.2"


Do we want this to be tied to the release ? Can we do like a bookstore where people manually increase when they update it. The release process is already as complicated as it is and I don't want to add artifacts that are not essential to istio usage.

I'm fine with the internal version to be hardcoded / manually changing when I make a significant change for now

I do want the images to be built by postsubmit though

sebastienvas

Nice

ldemailly · 2017-10-03T03:53:30Z

this is ready now, PTAL

sebastienvas · 2017-10-03T16:22:45Z

tests/e2e/tests/simple/simple1_test.go

+	os.Exit(tc.RunTest(m))
+}
+
+func TestSimpleIngress(t *testing.T) {


Are the comment still valid ? Could you remove the one that is not working.

updating the comments, thanks for noticing

sebastienvas · 2017-10-03T16:25:43Z

tests/e2e/tests/simple/simple1_test.go

+	os.Exit(tc.RunTest(m))
+}
+
+func TestSimpleIngress(t *testing.T) {


Are those tests independant ? if so that s OK, but if not you might want to use setup and tear down.

it uses framework.NewCommonConfig

costinm · 2017-10-03T20:34:29Z

install/updateVersion.sh

 export PROXY_TAG="${PROXY_TAG}"
 export ISTIO_NAMESPACE="${ISTIO_NAMESPACE}"
 export AUTH_DEBIAN_URL="${AUTH_DEBIAN_URL}"
 export PILOT_DEBIAN_URL="${PILOT_DEBIAN_URL}"
 export PROXY_DEBIAN_URL="${PROXY_DEBIAN_URL}"
-
+export FORTIO_HUB="${FORTIO_HUB}"


Small comment - do we need so many hubs ? Also for fortio I think it may be ok to just use ':latest' and simplify things...

(just IMHO)

I agree it's a lot overall but I'm just following the pattern in the code where things are split by hub and tag like all the other components. a follow up PR will make the tag auto updated with the rest

costinm

/lgtm

douglas-reid · 2017-10-03T20:46:25Z

devel/fortio/http.go

@@ -644,15 +651,17 @@ func (c *BasicClient) readResponse(conn *net.TCPConn) {
 			}
 			break // we're done!
 		}
-	}
+	} // end of big for loop
 	// Figure out whether to keep or close the socket:
 	if keepAlive && c.code == http.StatusOK {
 		c.socket = conn // keep the open socket
 	} else {


nit: since this appears to be the end of the method, we could just return inside of the conditional, and avoid the else.

similarly, in the contained if-else, we could just return inside the if and avoid the else altogether.

that whole method has a big TODO refactor but I don't want to do that just before the release

douglas-reid · 2017-10-03T20:47:41Z

devel/fortio/http.go

+	go func(secure bool, port int) {
+		var err error
+		if secure {
+			Errf("Secure setup not yet supported will just close incoming connections for now")


nit: add punctuation between "supported" and "will"

douglas-reid · 2017-10-03T20:51:21Z

devel/fortio/http.go

-	go func(port int) {
-		if err := http.Serve(listener, nil); err != nil {
-			Fatalf("Unable to serve on %d: %v", port, err)
+	go func(secure bool, port int) {


question: any reason not make this a separate method (or two)?

something like:

if secure { go runSecure(port, listener) return port } go runInsecure(port, listener) return port

it was going to be a 1 liner in the then: ServeTLS except that's a 1.9 feature but maybe I can make a "closingServer()" function for now instead of inlining?

douglas-reid · 2017-10-03T20:55:18Z

tests/e2e/tests/simple/simple1_test.go

+	// with "echo debug server ..." on the /debug uri.
+	url := "http://" + tc.Kube.Ingress + "/fortio/debug"
+	glog.Infof("Fetching '%s'", url)
+	attempts := 7 // should not take more than 70s to be live...


thought: we should really consider having some readiness check for Ingress.

yes in 0.3 I hope we build up more of library functions, this is just a start using what we already have (it does add a handy getPodList() for instance)

douglas-reid · 2017-10-03T20:56:40Z

tests/e2e/tests/simple/simple1_test.go

+	}
+	// call into the service from each of the pods
+	for _, pod := range podList {
+		glog.Infof("From pod \"%s\"", pod)


why glog.Infof and not t.Logf ?

mixer_test and others seem to be using glog.* - I think we may have some capture / log parsing code that uses glog format, not sure. good question for 0.3 testing fw

douglas-reid · 2017-10-03T20:56:56Z

tests/e2e/tests/simple/simple1_test.go

+		}
+		_ = resp.Body.Close()
+		bodyStr := string(body)
+		glog.Infof("Attempt %d: reply is\n%s\n---END--", i, bodyStr)


why not use t.Logf ?

douglas-reid · 2017-10-03T20:58:51Z

tests/e2e/tests/simple/simple1_test.go

+	return nil
+}
+
+func check(err error, msg string) {


nit: this is only used twice, in main(). it might make sense to just inline the code there, as this doesn't represent a big savings and is arguably a bit of an anti-pattern in golang.

I copied this part from mixer_test :-) I think we should move it to the framework if we're going to use it a lot (or inline) ?

deleting it

Thx doug

douglas-reid · 2017-10-03T21:51:15Z

/lgtm

istio-merge-robot · 2017-10-03T21:52:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, douglas-reid

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [costinm,douglas-reid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2017-10-03T21:55:17Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-10-03T22:25:54Z

Automatic merge from submit-queue

Automatic merge from submit-queue simpler, additional e2e functional test + auth test This is for a couple things: a) an e2e test (using the e2e framework) that has minimum dependencies and fast start which can be use as a starting point for new tests that do not rely on bookinfo b) exercise fortio images, ingress rule svc2sc c) detect whether auth is working or not d) bug fix in fortio when server errors out e) faster test start and end (and updated README with the faster instructions) _background/justification_ We all spent a lot of time debugging complex end2end test over the last few weeks, Also people have been complaining that writing new tests is hard (which is partially true as I learned through writing one), and we also don't have test verifying auth is really on This PR makes progress toward solving all 3 problems, introducing a simpler (than mixer/bookinfo) test. A simpler test lets us quickly smoke test basic features. This is additional to tests that exists in pilot (as this is about end2end with all the actual components in place, same code as we release) e2e is not exclusively "website tasks testing" and should have as many sane tests as we can have A separate effort - though this simpler test does help already - is to optimize the runtime and probably split test into "every PR" tests and "nightly"... etc... ```release-note New simpler e2e functional tests, including an auth test. ``` Former-commit-id: fad04b1

* Doc fixes. * Add high-level overview and diagrams for MCP (istio#805) * document MCP * add additional note on NACK behavior * add a note about ignoring stale nonces * Add envoy metrics service configuration to ProxyConfig (istio#803) * Add envoy metrics service configuration to ProxyConfig Similar to the statsd sink config, this change is in preparation for adding support for the envoy metrics service API. * Update proto.lock * Reorder metrics_service_address just after statsd_udp_address * Update comment in the authN API to match with 1.1 behavior (istio#808) * Rename metrics_service_address to clarify that it is for Envoy's API (istio#809) Since "metrics service" is so ambiguous, this change specifically denotes that the config is for Envoy's metrics service API. The comments also now link to detailed info about the metrics service API. * Generate separate HTML page per proto under networking (istio#811) * Generate separate pages for each networking proto Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * update Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * tweaks Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Fix interdoc references * separate doc and go gen * change dash to underscore * fix * more fixes * makefile fix Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * remove bad target * Revert "change dash to underscore" This reverts commit 7fbced66f620fb60b0d5e18af916567e313a0109. * final update Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Revert "Generate separate HTML page per proto under networking (istio#811)" This reverts commit 5f14eadd since it breaks the ability to generate reference docs in istio.io * Update to latest tools. * Revert "Update to latest tools." This reverts commit 231fadc5 which I accidentally pushed to the wrong place. * fix mcp update error diagram (istio#810) * update istio/tools (istio#815) * Revert "Revert "Generate separate HTML page per proto under networking (istio#811)"" (istio#818) * Minor updates for building docker images. (istio#817) * Add spell checking for docs. (istio#819) * Add a bunch of words to the custom dictionary. * Document exportTo restrictions (istio#820) * Doc fixes. (istio#821) * Proxy config: Add tls_certs_to_watch field (istio#824) * Proxy config: Add tls_certs_to_watch field * Address PR comment * Revert "Proxy config: Add tls_certs_to_watch field (istio#824)" (istio#825) This reverts commit e613e358ae8d4f331dd48904b0a2b176e11bf7f5. * Update gateway and sidecar hosts doc (istio#826) * Update gateway and sidecar hosts doc * regen * review comments * Update API docs (service entry, gateway, sidecar) (istio#831) * Update sidecar and gateway docs Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * gen docs Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * update service entry Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * updates Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * add configurable dns refresh rate to mesh for strict dns cluster, (istio#832) * add configurable dns refresh rate to mesh for strict dns cluster, issue istio#12181 * add lock file * Hosts are not ignored in ServiceEntry for HTTPs (istio#833) * Hosts are not ignored in ServiceEntry for HTTPs * Clarify * Reword * Generate protos * Address code review comments * Fix typo * generate * make mixer config marshal deterministic (istio#846) Signed-off-by: Kuat Yessenov <kuat@google.com> * Fix some doc errors. (istio#854) This started out to just fix a bad link to unblock work on istio.io, but I ended up fixing a bunch of typos/incorrect format throughout. * Fix two more broken links. (istio#856) * Fix CRD examples for release-1.1 (istio#859) * fix MeshNetworks demo (istio#841) * fix MeshNetworks demo * generate * update * last minute doc fixes (istio#865) Signed-off-by: Shriram Rajagopalan <rshriram@gmail.com> * Clarify comment for server_name field in Mixer oop auth config (istio#800) * add mixerclient config to istio.io reference docs (istio#827) * Envoyfilter docs fix (istio#867) * Tweaks to get the mixer client config docs to be published. (istio#868) * Change where the Mixer client docs are published.

…e"` for SIMPLE and MUTUAL TLS (istio#833) * Set alpnOverride depending on TLS Mode (istio#44918) * Set alpnOverride Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> * remove test logs Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> --------- Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> * pilot: set `istio.alpn_override: false` in subset clusters (istio#46496) (istio#46529) * Add tests for destination rules with port level settings * Add test cases for subsets * Refactor AddALPNOverrideToMetadata * Update comment for AddALPNOverrideToMetadata * Fix lint error * Add release note * Fix release note --------- Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> --------- Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> Co-authored-by: Kalya Subramanian <42158129+ksubrmnn@users.noreply.github.com>

ldemailly added 2 commits September 15, 2017 18:19

Merge branch 'master' into ldemailly-test-auth

9074617

Super basic skeleton test

ee1f41f

googlebot added the cla: yes label Sep 16, 2017

istio-merge-robot added the release-note label Sep 16, 2017

ldemailly commented Sep 16, 2017

View reviewed changes

linters

4bb9599

istio deleted a comment from istio-testing Sep 16, 2017

Need manual tag

712e8eb

istio deleted a comment from istio-testing Sep 16, 2017

Deal with having to retry

1f2da04

ldemailly requested a review from kyessenov September 16, 2017 03:08

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 16, 2017

ldemailly added 2 commits September 15, 2017 20:10

Merge branch 'master' into ldemailly-test-auth

5e89c53

# Conflicts: # istio.VERSION

Fix previous merge (I think)

83b0bb4

plus todo

istio deleted a comment from istio-testing Sep 16, 2017

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 16, 2017

ldemailly requested a review from sebastienvas September 16, 2017 03:28

ldemailly mentioned this pull request Sep 16, 2017

e2e smoke test: need dedicated node for fortio or wrk #682

Closed

Updates / wip

c2d96a9

fortio: added -echo-debug-path option to change the /debug to something else (because I can’t get rewrite rules to work yet, but it’s also a useful feature anyway) Rewrite rule seems to do nothing Also observed I can’t talk pod to pod getting 404s somehow

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 17, 2017

ldemailly mentioned this pull request Sep 17, 2017

unable to make rewrite rule work istio/old_pilot_repo#1288

Closed

merge

c00cd0b

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 17, 2017

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 18, 2017

sebastienvas reviewed Sep 18, 2017

View reviewed changes

sebastienvas suggested changes Sep 18, 2017

View reviewed changes

Typo in message

2486329

sebastienvas reviewed Oct 3, 2017

View reviewed changes

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 3, 2017

ldemailly added 2 commits October 3, 2017 09:44

Code review comments + misc comments

317417e

Merge branch 'master' into ldemailly-test-auth

00fc889

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 3, 2017

istio deleted a comment from istio-testing Oct 3, 2017

istio deleted a comment from istio-merge-robot Oct 3, 2017

costinm reviewed Oct 3, 2017

View reviewed changes

costinm approved these changes Oct 3, 2017

View reviewed changes

douglas-reid reviewed Oct 3, 2017

View reviewed changes

Code review comments

04529a6

Thx doug

douglas-reid approved these changes Oct 3, 2017

View reviewed changes

istio-testing assigned douglas-reid Oct 3, 2017

istio-testing added the lgtm label Oct 3, 2017

istio-merge-robot added the approved label Oct 3, 2017

istio-merge-robot merged commit fad04b1 into master Oct 3, 2017

ldemailly deleted the ldemailly-test-auth branch October 3, 2017 22:41

ldemailly mentioned this pull request Mar 9, 2018

Followup on EDS #4036

Merged

simpler, additional e2e functional test + auth test #833

simpler, additional e2e functional test + auth test #833

Uh oh!

Conversation

ldemailly commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Sep 16, 2017

Uh oh!

andraxylia commented Sep 16, 2017

Uh oh!

istio-merge-robot commented Sep 17, 2017

Uh oh!

istio-merge-robot commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienvas left a comment

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Oct 3, 2017

Uh oh!

sebastienvas Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglas-reid commented Oct 3, 2017

Uh oh!

istio-merge-robot commented Oct 3, 2017

ldemailly commented Sep 16, 2017 •

edited

Loading

sebastienvas Oct 3, 2017 •

edited

Loading

ldemailly Oct 3, 2017 •

edited

Loading