Add tests to verify TCP filter sends connection.id in Check() and Rep… by JimmyCYJ · Pull Request #3739 · istio/istio

JimmyCYJ · 2018-02-23T22:13:24Z

Add tests to verify TCP filter sends connection.id in Check() and Report() calls.

qiwzhang · 2018-02-23T22:29:59Z

/lgtm
/approve

istio-merge-robot · 2018-02-23T22:32:25Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @JimmyCYJ @qiwzhang

JimmyCYJ · 2018-02-26T18:01:38Z

/test istio-presubmit

douglas-reid · 2018-02-26T20:46:08Z

For the dep ensure bits, do we need to update the istio-contrib repo? @ldemailly can you advise?

ldemailly · 2018-02-26T20:57:39Z

you can't change Gopkg.lock without following the steps in
https://github.com/istio/istio/wiki/Vendor-FAQ#how-do-i-add--change-a-dependency

ldemailly

need to make an vendor-istio PR first (or undo your .lock changes - are they required?)

JimmyCYJ · 2018-02-27T01:18:17Z

@ldemailly I am not sure if I need to change .lock file.
In istio.deps, istio.io/api is using SHA 9d978d, which includes istio/api@ec13aa that defines message NetworkFailPolicy in api/mixer/v1/config/client/client_config.proto.

When I update proxy SHA in istio.deps, I break test istio.io/istio/mixer/test/client/network_policy/network_policy_test.go, because it does not use the new NetworkFailPolicy config.

I modified mixer/test/client/env/v2_config.go in this PR to use NetworkFailPolicy, but I got compile failure.
"env/v2_config.go:109:36: undefined: client.NetworkFailPolicy
env/v2_config.go:111:33: v2.Transport.NetworkFailPolicy.Policy undefined (type client.TransportConfig_NetworkFailPolicy has no field or method Policy)
env/v2_config.go:113:33: v2.Transport.NetworkFailPolicy.Policy undefined (type client.TransportConfig_NetworkFailPolicy has no field or method Policy)
"
Seems that I need to update istio.io/istio/vendor/istio.io/api/mixer/v1/config/client/client_config.pb.go

ldemailly · 2018-02-27T03:50:56Z

can you follow
https://github.com/istio/istio/wiki/Vendor-FAQ#how-do-i-add--change-a-dependency
and let me know if you encounter any issue

…ort() calls.

ldemailly · 2018-02-27T18:14:33Z

this PR isn't using your vendor PR (yet) please rev it up, but as mentioned in istio/old_vendor-istio_repo#14 I think you need to make a branch, not a fork

the build will gate the pr

JimmyCYJ · 2018-02-27T18:27:08Z

/retest

ldemailly · 2018-02-27T18:43:22Z

build+tests are green, merging istio/old_vendor-istio_repo#15

matching istio pr : istio/istio#3739

ldemailly · 2018-02-27T18:46:55Z

last update: can you cd vendor; git checkout master; git pull
and then cd ..; git diff vendor

should point to new head/master sha: 1c8002038e1782d4a38e22f8e3c328b51b764664

git add vendor; update the PR

and we can merge this :-)

codecov · 2018-02-27T18:49:35Z

Codecov Report

Merging #3739 into master will decrease coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #3739    +/-   ##
=======================================
- Coverage      77%     77%   -<1%     
=======================================
  Files         291     291            
  Lines       26596   26414   -182     
=======================================
- Hits        20267   20117   -150     
+ Misses       5057    5046    -11     
+ Partials     1272    1251    -21

Impacted Files	Coverage Δ
mixer/adapter/stackdriver/stackdriver.go	`55% <0%> (-15%)`	⬇️
mixer/adapter/servicecontrol/servicecontrol.go	`63% <0%> (-5%)`	⬇️
mixer/adapter/redisquota/redisquota.go	`86% <0%> (-3%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`68% <0%> (-2%)`	⬇️
mixer/adapter/stdio/zap.go	`97% <0%> (-1%)`	⬇️
mixer/adapter/rbac/rbacStore.go	`83% <0%> (-1%)`	⬇️
mixer/adapter/list/list.go	`96% <0%> (ø)`	⬇️
mixer/adapter/circonus/circonus.go	`71% <0%> (ø)`	⬇️
mixer/adapter/denier/denier.go	`93% <0%> (ø)`	⬇️
mixer/adapter/memquota/rollingWindow.go	`100% <0%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151c69e...dbea205. Read the comment docs.

JimmyCYJ · 2018-02-27T18:51:51Z

@ldemailly Thanks a lot for helping me waling through this vendor update.
git diff vendor points to new commit 1c8002038e1782d4a38e22f8e3c328b51b764664.
I have added vendor into this PR.

ldemailly · 2018-02-27T18:52:15Z

/lgtm

istio-merge-robot · 2018-02-27T18:52:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly, qiwzhang

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

Details

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

~~OWNERS~~ [ldemailly]

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 · 2018-02-27T19:11:53Z

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

ldemailly · 2018-02-27T19:16:48Z

filled #3810 for the weird test failure

JimmyCYJ requested review from a team and qiwzhang February 23, 2018 22:13

googlebot added the cla: yes label Feb 23, 2018

istio-testing assigned qiwzhang Feb 23, 2018

istio-testing added the lgtm label Feb 23, 2018

istio-merge-robot removed the lgtm label Feb 23, 2018

ldemailly previously requested changes Feb 26, 2018

View reviewed changes

JimmyCYJ added 3 commits February 27, 2018 09:53

Add tests to verify TCP filter sends connection.id in Check() and Rep…

1961caa

…ort() calls.

Fix test.

55edd9f

Update test config set up function.

cba26be

JimmyCYJ mentioned this pull request Feb 27, 2018

Update vendor. istio/old_vendor-istio_repo#14

Closed

ldemailly mentioned this pull request Feb 27, 2018

Update dependency. #3805

Closed

Update Gopkg.* and vendor

3a55e5e

JimmyCYJ mentioned this pull request Feb 27, 2018

Update vendor. istio/old_vendor-istio_repo#15

Merged

add comment.

19df1e5

ldemailly added a commit to istio/old_vendor-istio_repo that referenced this pull request Feb 27, 2018

Merge pull request #15 from istio/update-vendor

1c80020

matching istio pr : istio/istio#3739

Update vendor.

dbea205

istio-testing assigned ldemailly Feb 27, 2018

istio-testing added the lgtm label Feb 27, 2018

ldemailly approved these changes Feb 27, 2018

View reviewed changes

istio-merge-robot added the approved label Feb 27, 2018

ldemailly merged commit 26d71df into istio:master Feb 27, 2018

JimmyCYJ deleted the tcp-filter-connection-id branch February 27, 2018 19:18

Conversation

JimmyCYJ commented Feb 23, 2018

Uh oh!

qiwzhang commented Feb 23, 2018

Uh oh!

istio-merge-robot commented Feb 23, 2018

Uh oh!

JimmyCYJ commented Feb 26, 2018

Uh oh!

douglas-reid commented Feb 26, 2018

Uh oh!

ldemailly commented Feb 26, 2018

Uh oh!

ldemailly left a comment • 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!

JimmyCYJ commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimmyCYJ commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 27, 2018

Uh oh!

codecov bot commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimmyCYJ commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 27, 2018

Uh oh!

istio-merge-robot commented Feb 27, 2018

Uh oh!

istio-merge-robot commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ldemailly left a comment •

edited

Loading

JimmyCYJ commented Feb 27, 2018 •

edited

Loading

ldemailly commented Feb 27, 2018 •

edited

Loading

codecov bot commented Feb 27, 2018 •

edited

Loading