A few enhancements to istio-proxy-cfg.py by baodongli · Pull Request #3235 · istio/istio

baodongli · 2018-02-06T20:48:09Z

Made a few enhancements:

pilot_url doesn't have to be provided, but can be found out from the cluster
cache_stats can be queried. This is useful to find out who are talking to pilot for xds
podname can be a prefix of a pod's name
clarify the help strings.

Although I think podname as name.namespace.ip or name.namespace is not necessary, I kept them in.

istio-merge-robot · 2018-02-06T20:49:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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:

bin/OWNERS

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

mandarjog · 2018-02-07T02:13:52Z

bin/istio-proxy-cfg.py

We should continue separating yaml files by source(pilot or proxy). It is easy to diff 2 separate files.

I agree. I was thinking about that while working on it, but hesitating on what command arguments should be used to achieve that. Before this PR, --pilot_url is used to indicate pilot vs proxy. Now that pilot_url is not required, and there are three types of output: pilot, proxy, cache_stats, need a way to write them in different files. Is it ok to define the output argument as --output [DIR]? That way, three files will be written in the directory. Do you have any suggestions or preference? Do you think it's necessary for user to indicate the type of output in the command line?

Yes output dir sounds good

baodongli · 2018-02-13T16:59:13Z

/test circleci

baodongli · 2018-02-13T17:54:22Z

/test istio-pilot-e2e

baodongli · 2018-02-15T15:46:47Z

@mandarjog updated per comments. let me know any further comments. thanks

rshriram · 2018-02-16T21:20:27Z

bin/istio-proxy-cfg.py

nit: its json.. there is no YAML for v1

I changed the suffix to be json.

baodongli · 2018-02-28T21:18:58Z

@mandarjog let me know how to move this forward. thanks.

codecov · 2018-02-28T21:33:25Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #3235    +/-   ##
=======================================
- Coverage      76%     76%   -<1%     
=======================================
  Files         297     297            
  Lines       26725   27213   +488     
=======================================
+ Hits        20254   20536   +282     
- Misses       5203    5359   +156     
- Partials     1268    1318    +50

Impacted Files	Coverage Δ
mixer/adapter/noop/noop.go	`67% <0%> (-17%)`	⬇️
mixer/adapter/stackdriver/stackdriver.go	`58% <0%> (-12%)`	⬇️
pilot/pkg/config/monitor/monitor.go	`69% <0%> (-5%)`	⬇️
pilot/pkg/kube/inject/webhook.go	`77% <0%> (-4%)`	⬇️
mixer/adapter/circonus/circonus.go	`67% <0%> (-1%)`	⬇️
mixer/adapter/rbac/rbac.go	`10% <0%> (-1%)`	⬇️
mixer/template/sample/template.gen.go	`55% <0%> (ø)`	⬇️
pkg/cache/lruCache.go	`100% <0%> (ø)`	⬆️
mixer/adapter/dogstatsd/dogstatsd.go	`100% <0%> (ø)`	⬆️
pilot/pkg/model/controller.go	`100% <0%> (ø)`	⬆️
... and 9 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 007c793...e4fd766. Read the comment docs.

mandarjog · 2018-03-01T22:44:11Z

Running it directly gets me this error

mjog-macbookpro:tmp mjog$ ./istio-proxy-cfg.py -h
usage: istio-proxy-cfg.py [-h] [--pilot_url PILOT_URL] [--output OUTPUT]
                          [--cache_stats] [--clear_cache_stats]
                          podname

Fetch routes from Envoy or Pilot for a given pod

positional arguments:
  podname               podname must be either name.namespace.podip or
                        name.namespace or any string that is a pod's label or
                        a prefix of a pod's name. ingress, mixer, istio-ca,
                        product-page all work

optional arguments:
  -h, --help            show this help message and exit
  --pilot_url PILOT_URL
                        Often this is localhost:8080 or 15003 through a port-
                        forward. kubectl --namespace=istio-system port-forward
                        $(kubectl --namespace=istio-system get -l istio=pilot
                        pod -o=jsonpath='{.items[0].metadata.name}')
                        8080:8080. If not provided, attempt will be made to
                        find it out.
  --output OUTPUT       A directory where output files are saved. default is
                        the current directory
  --cache_stats         Fetch Pilot cache stats
  --clear_cache_stats   Clear Pilot cache stats
mjog-macbookpro:tmp mjog$ ./istio-proxy-cfg.py ingress
It seems that you are running outside the k8s cluster
Let's try to create a port-forward to access pilot
Enter local port number (default 8080, ctrl-C to abort): 8080
local port is 8080
Forwarding from 127.0.0.1:8080 -> 8080
Handling connection for 8080
Fetching from Pilot for pod istio-ingress-784f979b9b-jpdzv in istio-system namespace
http://localhost:8080/v1/listeners/istio-proxy/ingress~10.32.1.33~istio-ingress-784f979b9b-jpdzv.istio-system~istio-system.svc.cluster.local
Handling connection for 8080
http://localhost:8080/v1/routes/80/istio-proxy/ingress~10.32.1.33~istio-ingress-784f979b9b-jpdzv.istio-system~istio-system.svc.cluster.local
Handling connection for 8080
http://localhost:8080/v1/clusters/istio-proxy/ingress~10.32.1.33~istio-ingress-784f979b9b-jpdzv.istio-system~istio-system.svc.cluster.local
Handling connection for 8080
http://localhost:8080/v1/registration/grafana.istio-system.svc.cluster.local|http
Handling connection for 8080
http://localhost:8080/v1/registration/istio-pilot.istio-system.svc.cluster.local|http-discovery
Handling connection for 8080
http://localhost:8080/v1/registration/httpbin.default.svc.cluster.local|http
Handling connection for 8080
http://localhost:8080/v1/registration/echosrv2.istio.svc.cluster.local|http-echo
Handling connection for 8080
http://localhost:8080/v1/registration/prometheus.istio-system.svc.cluster.local|http-prometheus
Handling connection for 8080
http://localhost:8080/v1/registration/echosrv1.istio.svc.cluster.local|http-echo
Handling connection for 8080
http://localhost:8080/v1/registration/productpage.default.svc.cluster.local|http
Handling connection for 8080
Wrote  ./istio-ingress-784f979b9b-jpdzv/pilot_xds.json
Fetching from Envoy for pod istio-ingress-784f979b9b-jpdzv in istio-system namespace
kubectl -n istio-system exec -i -t istio-ingress-784f979b9b-jpdzv -c istio-ingress -- curl http://localhost:15000/routes
command terminated with exit code 126
Traceback (most recent call last):
  File "./istio-proxy-cfg.py", line 376, in <module>
    sys.exit(main(args))
  File "./istio-proxy-cfg.py", line 324, in main
    data = pr.routes()
  File "./istio-proxy-cfg.py", line 173, in routes
    return self.query("/routes")
  File "./istio-proxy-cfg.py", line 163, in query
    s = subprocess.check_output(cmd.split())
  File "/usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 219, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '['kubectl', '-n', 'istio-system', 'exec', '-i', '-t', 'istio-ingress-784f979b9b-jpdzv', '-c', 'istio-ingress', '--', 'curl', 'http://localhost:15000/routes']' returned non-zero exit status 126

mandarjog · 2018-03-01T22:48:25Z

curl is not present on images, so we cannot assume it. Now that is runs unprompted, the error is an issue.

ask if user wants directly from envoy
If curl is not there, don't throw and error.
start a new curl container and use it to hit envoy in question. mtls needs to be solved.

mandarjog · 2018-03-01T22:51:01Z

Would be good to replace this

Let's try to create a port-forward to access pilot
Enter local port number (default 8080, ctrl-C to abort): 8080

with "using ${some random local port} for forwarding to pilot" And make it non interactive.

baodongli · 2018-03-02T00:59:42Z

About the curl error, I realized that port-forward can be used and curl can be invoked from the host rather than from inside the container. I'll restructure the code a bit so that both sidecar query and pilot query can use the same code. For local port, I'll use a random port from the range 50000 - 60000, and get rid of the read from user.

Let's use another PR for the mtls.

Add --clear_cache_stats option

to use this command outside the k8s cluster

baodongli · 2018-03-03T03:06:10Z

/retest

baodongli · 2018-03-05T15:47:42Z

/retest

istio-testing · 2018-03-05T16:03:59Z

@baodongli: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-presubmit.sh	`e4fd766`	link	`/test istio-presubmit`

Details

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.

baodongli requested a review from a team February 6, 2018 20:48

googlebot added the cla: yes label Feb 6, 2018

baodongli changed the title ~~A few enhancements~~ A few enhancements to istio-proxy-cfg.py Feb 6, 2018

baodongli assigned mandarjog Feb 6, 2018

mandarjog reviewed Feb 7, 2018

View reviewed changes

baodongli force-pushed the proxy-cfg branch 2 times, most recently from 2d8ad7b to 0bd67a9 Compare February 13, 2018 16:27

baodongli force-pushed the proxy-cfg branch from 0bd67a9 to d3c90e8 Compare February 13, 2018 17:00

rshriram reviewed Feb 16, 2018

View reviewed changes

baodongli force-pushed the proxy-cfg branch from d3c90e8 to ce73b49 Compare February 28, 2018 21:13

Baodong (Robert) Li added 4 commits March 2, 2018 16:20

A few enhancements

d65c659

Make --output to be a directory

52b1582

Add --clear_cache_stats option

add port-forward support so that it's even easier

11c531f

to use this command outside the k8s cluster

use port-forward to download envoy config

e4fd766

baodongli force-pushed the proxy-cfg branch from ce73b49 to e4fd766 Compare March 2, 2018 21:24

mandarjog approved these changes Mar 5, 2018

View reviewed changes

mandarjog merged commit be57dc6 into istio:master Mar 5, 2018

Conversation

baodongli commented Feb 6, 2018

Uh oh!

istio-merge-robot commented Feb 6, 2018

Uh oh!

mandarjog Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

baodongli Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

mandarjog Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

baodongli commented Feb 13, 2018

Uh oh!

baodongli commented Feb 13, 2018

Uh oh!

baodongli commented Feb 15, 2018

Uh oh!

rshriram Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

baodongli Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

baodongli commented Feb 28, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mandarjog commented Mar 1, 2018

Uh oh!

mandarjog commented Mar 1, 2018

Uh oh!

mandarjog commented Mar 1, 2018

Uh oh!

baodongli commented Mar 2, 2018

Uh oh!

baodongli commented Mar 3, 2018

Uh oh!

baodongli commented Mar 5, 2018

Uh oh!

istio-testing commented Mar 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Feb 28, 2018 •

edited

Loading