twosigma / envoy-viz

Apache License 2.0
11 stars 1 forks source link

Link more envoy protos into the binary or fail-open on unknown types #6

Open howardjohn opened 3 years ago

howardjohn commented 3 years ago
$ envoy-viz --admin http://localhost:15000
panic: proto: (line 1008:25): unable to resolve "type.googleapis.com/envoy.extensions.filters.http.router.v3.Router": "not found"

goroutine 1 [running]:
main.main()
        /tmp/tmp.8sgS3WIhPc/envoy-viz/envoy-viz.go:47 +0x494

Basically looks like we don't have the protos imported?

One thing our project does is autogen an import for everything in go-control-plane: https://github.com/istio/istio/blob/c191dfdbf4d226b481a34687e01cfe49b8250774/pkg/config/xds/filter_types.gen.go#L21

This still won't work if users have custom types though - to be more robust you would probably want to ingore unknown types. I think https://github.com/istio/istio/blob/88a2bfb3bd8e0010229ee66befc4e56b326c2a3a/istioctl/pkg/util/configdump/wrapper.go#L30 does this?

blevz commented 3 years ago

Makes sense, will look into doing that. We also probably want to think how custom types can integrate and provide integration points for custom types to provide "custom graphing rules"

oschaaf commented 3 years ago

naive thought probably, but why not treat everything as a custom type? (there would be more custom graphing rules to teach the tool visualising stock Envoy configs, but OTOH there would be a single way in which this gets accomplished instead of two, and maybe it would uncouple a bit from leaning on specific api/Envoy versions)

blevz commented 3 years ago

That makes intuitive sense to me and would probably solve some of the complexity introduced here: https://github.com/twosigma/envoy-viz/blob/main/graph/build.go#L84-L151

It seems like most of the custom stuff we'd want to support is in the routing portion of the envoy config? Is that a reasonable view of things? Are there a lot of custom listeners and clusters or do we really just need to target the wiring between those two?

oschaaf commented 3 years ago

Well, thinking out loud some more:

A lot of things are extensible within Envoy; maybe what is wise depends on what this intends to support and where it is heading.

If Envoy support is to be constrained to the vanilla version, then the current way of doing things seems pragmatic to me, I think going generic might not make things easier / less complex. But it might help in the long run depending on the above.

As an example, assisting users with debugging Envoy configs aside, I could see this potentially being useful to visualise configurations in documentation, in which case the specific Envoy flavored repo consuming it could be served by its own custom rules to render specifics (e.g. rebranding, or handling extensions we didn't know about). Or maybe some consumer wants to visualise things around transport socket extensions, but we don't care about that here.

esnible commented 3 years ago

More important that extensibility is a mechanism to filter out unimportant configuration.

I have struggled to visualize the Envoy configuration generated by Istio. The amount of data is overwhelming in the usual case. Typically a microservice has two or three dependencies. Unless the admin has carefully hinted the mesh, Istio doesn't know the dependencies of a pod, so it provides a great deal of config to Envoy. Except 0.5MB of configuration.

When I wrote my own tool I found to get the output down to something manageable I needed to exclude "unused" configuration. To determine the unused configurations I looked at Envoy's /clusters, and excluded those with rq_total=0, and then excluded routes and listeners and endpoints that only went through those clusters.

blevz commented 3 years ago

I haven't cleaned up the code to opensource but we are enriching our visualizations using info from the admin endpoints internally (mostly displaying stats as a tooltip on the node). Once that bit is available it seems reasonable to add the ability to filter out unused components of the graph

blevz commented 3 years ago

Is there a protojson equivalent to https://github.com/istio/istio/blob/88a2bfb3bd8e0010229ee66befc4e56b326c2a3a/istioctl/pkg/util/configdump/wrapper.go#L30 I think we will want a combo of the filter_types.gen.go approach + a fail-open resolver.

Additionally, I went a bit down the rabbit hole trying to get the testdata/istio-httpbin.json to parse. I ended up getting stuck here trying to go get the istio envoy filter types:

$ go get istio.io/istio/pkg/envoy/config/filter/http/alpn/v2alpha1
go get: istio.io/istio@none updating to
        istio.io/istio@v0.0.0-20210331005500-1c16be5d1744 requires
        helm.sh/helm/v3@v3.5.3 requires
        github.com/deislabs/oras@v0.10.0 requires
        github.com/docker/distribution@v0.0.0-00010101000000-000000000000: invalid version: unknown revision 000000000000
howardjohn commented 3 years ago

Hm yeah importing istio/istio is a pain. I have been hoping to move our protos out to istio/api. If you want to make it work you may need to copy over the replace directives in our go.mod unfortunately...

However, I would have expected with the custom resolver you can just ignore the Istio filters? They don't contribute to any routing or anything so we probably don't need to visualize them at all.

On Tue, Mar 30, 2021, 7:29 PM Ben Levy @.***> wrote:

Is there a protojson equivalent to https://github.com/istio/istio/blob/88a2bfb3bd8e0010229ee66befc4e56b326c2a3a/istioctl/pkg/util/configdump/wrapper.go#L30 I think we will want a combo of the filter_types.gen.go approach + a fail-open resolver.

Additionally, I went a bit down the rabbit hole trying to get the testdata/istio-httpbin.json to parse. I ended up getting stuck here https://github.com/twosigma/envoy-viz/commit/960f6d6d27e9091a0c19860c2267d760fe7b4014 trying to go get the istio envoy filter types:

$ go get istio.io/istio/pkg/envoy/config/filter/http/alpn/v2alpha1 go get: @. updating to @. requires @. requires @. requires @.***: invalid version: unknown revision 000000000000

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/twosigma/envoy-viz/issues/6#issuecomment-810710335, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXMHPNUORKKEJ5QZYD3TGKCH7ANCNFSM42AKWLYA .

blevz commented 3 years ago

I assume it would work w/ the custom resolver but couldn't find an equivalent custom resolver that fits into the protojson api (the linked one targets jsonpb and fulfills a different interface). Will probably try implementing the equivalent custom resolver if I can't hunt down an example somewhere

blevz commented 3 years ago

Works w/ that custom resolver code. Pushed an initial version here. Gonna clean it up and add an actual test (right now it just tests that it can parse the json).