Enable CRD store in mixer install pod spec #650

mandarjog · 2017-09-02T18:36:44Z

Release note:

NONE

istio-merge-robot · 2017-09-02T18:36:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: geeknoid

Assign the PR to them by writing /assign @geeknoid in a comment when ready.

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

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

douglas-reid · 2017-09-03T03:18:17Z

tests/e2e/tests/mixer/mixer_test.go

@@ -249,7 +249,7 @@ func TestDenials(t *testing.T) {
 	time.Sleep(5 * time.Second)

 	// generate several calls to the product page
-	for i := 0; i < 10; i++ {
+	for i := 0; i < 1000; i++ {


this isn't needed with latest from HEAD. It now uses v3 exclusively.

if err := replaceRouteRule(routeReviewsV3Rule); err != nil { t.Fatalf("Could not create replace reviews routing rule: %v", err) } // hope for stability time.Sleep(30 * time.Second)

andraxylia · 2017-09-03T14:56:40Z

Please update RBAC rules for mixer: create a mixer service account and add rules for the CRDs.

mandarjog · 2017-09-03T21:03:48Z

/test e2e-suite-no_rbac-no_auth

ldemailly · 2017-09-04T20:51:07Z

can you add
install/updateVersion.sh -pgcr.io/istio-testing,fb947d4c6d0902a841e0e82ffa3bee16786be0df -i https://storage.googleapis.com/istio-artifacts/pilot/fb947d4c6d0902a841e0e82ffa3bee16786be0df/artifacts/istioctl

to your pr (same as #662 ) and maybe also the CA and also more relevant to mixer: the newest mixer 49ad083ba35943880f57a16a475c50b1e037dde8 from this morning istio/old_mixer_repo@49ad083 )

edit: rebase as I updated the pilot sha to be the correct one

istio-merge-robot · 2017-09-04T21:04:18Z

@mandarjog PR needs rebase

ldemailly · 2017-09-04T21:34:51Z

istio.VERSION

-export ISTIOCTL_URL="https://storage.googleapis.com/istio-artifacts/pilot/fb947d4c6d0902a841e0e82ffa3bee16786be0df/artifacts/istioctl"
+export MIXER_HUB="gcr.io/istio-testing"
+export MIXER_TAG="2c8e76790aa1d569b739454ca2dba5e0b031bd25"
+export ISTIOCTL_URL="https://storage.googleapis.com/istio-artifacts/pilot/b5d710d658d3bfffa06506f312725d96a8737703/artifacts/istioctl"


merge error, please leave fb947d4c6d0902a841e0e82ffa3bee16786be0df
(and again, I think you should use latest mixer (49ad083ba35943880f57a16a475c50b1e037dde8) given the big change and need to debug the tests anyway)

fixed it, PTAL.

ldemailly · 2017-09-04T21:40:59Z

tests/e2e/tests/mixer/mixer_test.go

+		return err
+	}
+
+	if err = p.portForward("app=productpage", productPagePort, "80"); err != nil {


add a comment that this forward is just to test the app liveness directly, initially was worried we weren't doing e2e anymore

ldemailly · 2017-09-04T21:44:36Z

tests/e2e/tests/mixer/mixer_test.go

+	}
+	resp, err := clnt.Do(req)
+	if err != nil {
+		glog.Warningf("Error communicating with %s: %s", url, err)


nit/style (feel free to ignore): return here and no else?

ldemailly · 2017-09-04T21:47:08Z

tests/e2e/tests/mixer/mixer_test.go

+	}
+}
+
+func get(clnt *http.Client, url string, headerkv ...string) (status int, contents string, err error) {


(not for this pr probably) we could use the fortio client here too probably (and make it dump the full headers when there is an error)

That would be nice.

ldemailly · 2017-09-04T21:48:51Z

tests/e2e/tests/mixer/mixer_test.go

+		return 0, "", err
+	}
+
+	for i := 0; i < len(headerkv)/2; i++ {


might want to log an error or panic if headerkv isn't even (caller bug/error)

headers are optional, it is not an error.

0 is even
but if someone passes 1 argument it will get silently ignored which is surely not what you intend ? (or 3 or any 2N+1)

ldemailly · 2017-09-04T21:52:13Z

tests/e2e/tests/mixer/mixer_test.go

 		closeResponseBody(resp)
 	}
-	return resp.StatusCode, contents, err
+	return


how does that compile?

douglas-reid · 2017-09-04T22:28:23Z

samples/apps/bookinfo/mixer-rule-additional-telemetry.yaml

+  actions:
+  - handler: handler.prometheus.istio-config-default
+    instances:
+    - responsesize.metric.istio-config-default


have we abandoned logs? if so, we need an issue to update this Task.

douglas-reid · 2017-09-04T22:31:37Z

tests/e2e/tests/mixer/mixer_test.go

 		t.Fatalf("Test app setup failure: %v", err)
 	}

 	glog.Info("Successfully sent request(s) to /productpage; checking metrics...")
 	// must sleep to allow for prometheus scraping, etc.
 	time.Sleep(15 * time.Second)
+	dumpMixerMetrics()


i don't think this offers anything beyond the promDump() changes in the other outstanding PR. I don't think there is any evidence that the mixer->prometheus parts of this test are flaky or problematic.

sure I can get rid of this. I needed something for my debugging.

douglas-reid · 2017-09-04T22:34:09Z

tests/e2e/tests/mixer/mixer_test.go

-		t.Fatalf("Could not create required mixer rule: %v", err)
+	// deny rule will deny all requests to product page unless
+	// ["x-user"] header is set.
+	glog.Infof("Denials: block productpage if x-user header is missing")


This fundamentally changes the nature of this test. What is the reasoning here?

If we strip out everything from the tests, we have no check of any complex behavior (nor need for using something like bookinfo as the basis of a test).

This is now a denials integration test. I am going to add some labels processing once istio/old_mixer_repo#1207 in merged.

I agree that previous behavior was complex, however that complexity was orthogonal to the workings on denials. The above test (with addition of labels) tests Mixer / Pilot / Envoy all together.

It may have been orthogonal to the mixer feature. But, imho, it is central to Istio as a whole to have something like this just work.

This test was passing (and recently altered specifically to pass with new proxy logic). I'm not sure what the compelling reason to change it is.

A different approach to this discussion:

This PR is changing the fundamental way mixer is configured (affecting metrics reporting, denials enforcement, etc.). Does it make sense to remove tests of those aspects in the same PR where we are switching the logic?

Either we should do this change first, or we should do in subsequently, imho. But doing it at the same time raises questions of regression and brittleness.

We are not changing all the tests. Which tests are we removing apart from TestDenials?

TestDenials is the only test that is changing behavior, and becoming more focussed. I need a test that can test the new code path. There is also istio/old_mixer_repo#1207 that is needed to use labels which are used in the existing test. We can add those tests back.

Apart from that these changes swap metrics in-place, so the new prometheus metrics adapter generates identical metrics.

What else is brittle?

mandarjog · 2017-09-04T22:44:07Z

/retest

ldemailly

just in case my comments above would be missed :-)

istio-merge-robot · 2017-09-05T03:40:22Z

@mandarjog PR needs rebase

thx for the updates and reopening

mandarjog · 2017-09-05T20:24:59Z

/retest

yutongz · 2017-09-05T20:41:10Z

/test all

mandarjog · 2017-09-05T21:22:00Z

Tests passed in private gke cluster.

Tests Summary
PASSED: //tests/e2e/tests/mixer:go_default_test 
PASSED: //tests/e2e/tests/bookinfo:go_default_test

yutongz · 2017-09-05T21:32:33Z

/test all

istio-testing · 2017-09-05T21:57:25Z

@mandarjog: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-suite-rbac-no_auth.sh	`92039ea`	link	`/test e2e-suite-rbac-no_auth`
prow/e2e-suite-rbac-auth.sh	`92039ea`	link	`/test e2e-suite-rbac-auth`
prow/e2e-suite-no_rbac-auth.sh	`92039ea`	link	`/test e2e-suite-no_rbac-auth`
prow/e2e-suite-no_rbac-no_auth.sh	`92039ea`	link	`/test e2e-suite-no_rbac-no_auth`

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. I understand the commands that are listed here.

mandarjog · 2017-09-05T22:33:27Z

@douglas-reid also replicated the same test results.

ldemailly · 2017-09-05T22:36:50Z

squash and merge according to our exceptional policies until 503s are resolved separately
( mandar and doug certify this pr as passing all tests locally in 2 places )

ldemailly · 2017-09-05T22:38:26Z

it's merged. but @mandarjog can you add release notes though

Former-commit-id: b2c76f68276985f885754aa2a47b3c8b186cf0bb

* enable configStore2 * update kind names * Use new mixer2 metrics * fix linter errors * replacing 0.1 metrics with 0.2, wip * make deny test simpler * fix linter error * fix url * linter error * Add log entries and always send 'x-user' header * add additional debug info * remove linter errors * dump direct mixer metrics for debug * fix test error * fix productpage port * fix merge error * now really fix versions * introduce header type * remove extra debug * remove unused code Former-commit-id: 838bcba

Former-commit-id: c46bc5c44d9fcd278c4af1d79226530e54355a68

* enable configStore2 * update kind names * Use new mixer2 metrics * fix linter errors * replacing 0.1 metrics with 0.2, wip * make deny test simpler * fix linter error * fix url * linter error * Add log entries and always send 'x-user' header * add additional debug info * remove linter errors * dump direct mixer metrics for debug * fix test error * fix productpage port * fix merge error * now really fix versions * introduce header type * remove extra debug * remove unused code Former-commit-id: 838bcba

This should resolve the jaeger regression around ports. Additionally other features have been added to the installer. Fixes: istio#19227

googlebot added the cla: yes label Sep 2, 2017

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

douglas-reid reviewed Sep 3, 2017

View reviewed changes

mandarjog force-pushed the mixer2 branch from 59a5a5a to e3e477d Compare September 3, 2017 19:32

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

mandarjog force-pushed the mixer2 branch from 7291624 to 07ebb4d Compare September 4, 2017 21:26

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

ldemailly reviewed Sep 4, 2017

View reviewed changes

douglas-reid reviewed Sep 4, 2017

View reviewed changes

ldemailly previously requested changes Sep 4, 2017

View reviewed changes

mandarjog changed the title ~~Enable CRD store in mixer install pod spec~~ Abandoned Enable CRD store in mixer install pod spec Sep 4, 2017

mandarjog closed this Sep 4, 2017

mandarjog reopened this Sep 5, 2017

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

mandarjog and others added 8 commits September 4, 2017 21:29

enable configStore2

fd18e0c

update kind names

5e81a1f

Use new mixer2 metrics

f7a0db4

fix linter errors

141f180

replacing 0.1 metrics with 0.2, wip

ac5e34e

make deny test simpler

83e0457

fix linter error

e03aca1

fix url

a9c6d08

mandarjog and others added 8 commits September 4, 2017 21:34

remove linter errors

f108923

dump direct mixer metrics for debug

d911144

fix test error

66026e8

fix productpage port

2c7c819

fix merge error

9ca6821

now really fix versions

dc209c7

introduce header type

de4615c

remove extra debug

9c99f1a

mandarjog force-pushed the mixer2 branch from caf8734 to 9c99f1a Compare September 5, 2017 04:47

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

remove unused code

92039ea

mandarjog changed the title ~~Abandoned Enable CRD store in mixer install pod spec~~ Enable CRD store in mixer install pod spec Sep 5, 2017

ldemailly merged commit 838bcba into istio:master Sep 5, 2017

mandarjog pushed a commit to mandarjog/istio that referenced this pull request Oct 30, 2017

Updated help text. (istio#650)

d7552ed

Former-commit-id: b2c76f68276985f885754aa2a47b3c8b186cf0bb

mandarjog deleted the mixer2 branch October 30, 2017 21:07

mandarjog pushed a commit that referenced this pull request Oct 31, 2017

Updated help text. (#650)

4d273bb

Former-commit-id: c46bc5c44d9fcd278c4af1d79226530e54355a68

howardjohn pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020

This introduces the latest installer.sha (istio#650)

7e29d92

This should resolve the jaeger regression around ports. Additionally other features have been added to the installer. Fixes: istio#19227

Enable CRD store in mixer install pod spec #650

Enable CRD store in mixer install pod spec #650

Conversation

mandarjog commented Sep 2, 2017

istio-merge-robot commented Sep 2, 2017

Choose a reason for hiding this comment

andraxylia commented Sep 3, 2017

mandarjog commented Sep 3, 2017

ldemailly commented Sep 4, 2017 • edited Loading

istio-merge-robot commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandarjog commented Sep 4, 2017

ldemailly left a comment

Choose a reason for hiding this comment

istio-merge-robot commented Sep 5, 2017

mandarjog commented Sep 5, 2017

yutongz commented Sep 5, 2017

mandarjog commented Sep 5, 2017 • edited Loading

yutongz commented Sep 5, 2017

istio-testing commented Sep 5, 2017

mandarjog commented Sep 5, 2017

ldemailly commented Sep 5, 2017

ldemailly commented Sep 5, 2017

ldemailly commented Sep 4, 2017 •

edited

Loading

mandarjog commented Sep 5, 2017 •

edited

Loading