Fixes #971 Adds initial capability to add a gRPC Proxy to Tyk using bundler or direct API Def config #1111

lonelycode · 2017-09-12T20:18:28Z

Fixes #971

…or grpc and trace as grpc plugins fail hard because of type mismatches. Needs more investigation.

…conflicts *except* golang.org/x/net/trace, which keeps generating duplicate paths on /debug/trace in the muxer. The only solution is to compile tyk with that vendored lib removed - then plugins work.

…lugins and reload

…we can retain vendoring and plugin functionality

…vendor by renaming and make the vendor dir the GOPATH to guarantee the same build path and versions for plugins

mvdan

first pass before bed, will take a more careful look another day.

mvdan · 2017-09-12T20:20:44Z

.travis.yml

@@ -25,6 +25,9 @@ services:
  - redis-server

 install:
+  # gRPC Proxy as plugins means we can't have these vendored
+  - go get "github.com/grpc-ecosystem/grpc-gateway/runtime"
+  - go get "golang.org/x/net/trace"


This is now fixed and they're vendored - remove?

mvdan · 2017-09-12T20:21:09Z

api_loader.go

+		if spec.CustomMiddlewareBundle != "" {
+			pathPrefix = path.Join(getTykBundlePath(), spec.APIID+"-"+spec.CustomMiddlewareBundle)
+		}
+		filePath := path.Join(pathPrefix, pluginPath)


filepath.Join

mvdan · 2017-09-12T20:21:28Z

api_loader.go

+		pluginPath := spec.CustomMiddleware.GRPCProxy.Path
+		pathPrefix := globalConf.MiddlewarePath
+		if spec.CustomMiddlewareBundle != "" {
+			pathPrefix = path.Join(getTykBundlePath(), spec.APIID+"-"+spec.CustomMiddlewareBundle)


filepath.Join

mvdan · 2017-09-12T20:22:45Z

grpcproxy/grpcproxy.go

+	"google.golang.org/grpc"
+)
+
+type conf struct{}


this global name throws me off a bit - reading its declaration, I have no idea what it's for.

mvdan · 2017-09-12T20:23:02Z

grpcproxy/grpcproxy.go

+	var p *plugin.Plugin
+
+	// Load our plugin
+	if p, err = plugin.Open(path); err != nil {


Replace these three lines with

p, err := ... if err != nil {

mvdan · 2017-09-12T20:23:30Z

grpcproxy/grpcproxy.go

+	tmapObj, err := p.Lookup("Types")
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "error: failed to lookup type map: %v\n", err)
+		os.Exit(1)


A func that errors but may also exit? (here and below)

mvdan · 2017-09-12T20:24:24Z

grpcproxy/grpcproxy.go

+	modGo := lib.NewModule("register")
+
+	ctx := context.Background()
+	ctx, _ = context.WithCancel(ctx)


What's the point of this line if you discard the cancel func?

mvdan · 2017-09-12T20:24:42Z

grpcproxy/grpcproxy.go

+type v2Config struct {
+	ctx  context.Context
+	mux  *runtime.ServeMux
+	e    string


e is a bit too short to know what it is

mvdan · 2017-09-12T20:25:38Z

plugin_build/README.md

+```sh
+go get -u github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
+go get -u github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
+go get -u github.com/golang/protobuf/protoc-gen-go


I thought we vendored them?

Not sure my last comment saved - if building the pb.go files manually you need to do this otherwise you can't actually generate the templates

I thought we would rely on our vendored copies - otherwise plenty of things can go wrong, e.g. if those libs ever have any breaking changes

For example, we could tell people to use our vendor/ as their GOPATH

It doesn't affect these instructions, this command generates the plugins needed by protoc in the previous step to do the generating, so the stuff in our vendor dir won;t be part of the go bin path.

They would need to change their GOPATH after when they actually build the plugin.

mvdan

A better review, now that I'm awake.

mvdan · 2017-09-13T09:09:05Z

grpcproxy/grpcproxy.go

+	// register the plug-in's modules
+	tmapObj, err := p.Lookup("Types")
+	if err != nil {
+		return fmt.Errorf("error: failed to lookup type map: %v", err)


All the errors have an error: prefix, that can be removed

mvdan · 2017-09-13T09:09:35Z

grpcproxy/grpcproxy.go

+
+	// assert that the type map pointer is not nil
+	if tmapPtr == nil {
+		return fmt.Errorf("error: nil type map: type=%[1]T val=%[1]v", tmapPtr)


This looks like bad formatting, even github highlights the % chars in red

mvdan · 2017-09-13T09:11:51Z

grpcproxy/grpcproxy.go

+}
+
+// Get returns the value for the specified key
+func (c *v2Config) Get(ctx context.Context, key string) interface{} {


Have you come up with the Get/Set interface? If so, why not make it type-safe, like:

func (c *v2Config) Ctx() *context.Context { return c.ctx }

(or just exporting the fields, which is even easier)

See the last comment, this mechanism solves a whole bunch of type conversion headaches. I'm not confident enough to redesign it, so if you want to take a shot, be my guest.

mvdan · 2017-09-13T09:12:32Z

plugin_build/README.md

+```sh
+go get -u github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
+go get -u github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
+go get -u github.com/golang/protobuf/protoc-gen-go


I thought we would rely on our vendored copies - otherwise plenty of things can go wrong, e.g. if those libs ever have any breaking changes

For example, we could tell people to use our vendor/ as their GOPATH

mvdan · 2017-09-13T09:13:28Z

plugin_build/wrap.go.tmpl

+}
+
+// Do not edit this function
+func changeMe(ctx context.Context, mux *runtime.ServeMux, endpoint string, opts []grpc.DialOption) (err error) {


changeMe that says do not edit? :)

mvdan · 2017-09-13T09:20:53Z

plugin_build/README.md

+
+Copy these to the same directory as your stubs.
+
+Open the *.pb.gw.go and *.pb.go files, you must change the package name from whatever was generated, to `main`, so the


whenever you use * you should quote it, otherwise the format gets fucked up

mvdan · 2017-09-13T09:21:03Z

plugin_build/README.md

+
+It will generate a reverse proxy `path/to/your_service.pb.gw.go`.
+
+Note: After generating the code for each of the stubs, in order to build the code, you will want to run ```go get .``` from the directory containing the stubs.


I think for a single line, a single ` is enough?

mvdan · 2017-09-13T09:26:45Z

grpcproxy/grpcproxy.go

+	// Instantiate a copy of the module registered by the plug-in.
+	modGo := lib.NewModule("register")
+
+	ctx := context.Background()


Related to the question below on the Get/Set interface - why is a context in the mix? We don't seem to make any use of it. And it seems unnecessary for getter/setters.

I really don't want to mess with these because it's part of the plugin fix for type passing - it's literally a lift and shift from the github.com/akutz/gpds/lib code.

I've had a look and it seems like context isn't used there at all. Are we going to make use of it? If not, I'd leave it out of the API.

mvdan · 2017-09-13T09:29:06Z

plugin_build/README.md

@@ -0,0 +1,162 @@
+# GRPC Proxy How To


I understand plugin_build is just a guide. Is this going to move to the docs?

Eventually, this is alpha, so keeping a readme here is a good idea until we formalize

mvdan · 2017-09-13T09:29:27Z

plugin_build/README.md

+docker image that will make this process more automated**
+
+*Edit* To make this easier, use the `tykio/bakery` container, it will generate a plugin file from your proto
+files without any additional steps.


These two notes seem to contradict each other a bit.

…ges were mirrored in docker hub

lonelycode · 2017-09-13T22:44:27Z

@mvdan If you can suggest a good alternative to httptest.NewRecorder() would be happy to change it.

mvdan · 2017-09-14T18:58:12Z

reverse_proxy.go

+		log.Debug("====> Calling: ", outreq.URL.String())
+		if breakerEnforced {
+			if breakerConf.CB.Ready() {
+				rec := httptest.NewRecorder()


So if we're using the grpc proxy, we're buffering the entire response? That seems pretty bad to me. If someone wishes to use a grpc API that writes tons of data or writes data incrementally or over time, this won't work at all.

I assume you did this because you only have ServeHTTP to use from grpc. We should be calling that directly with the user's ResponseWriter, instead of reading the whole response into memory first like this.

Note that this is different from what RoundTrip does - that one does the request, but it doesn't buffer the entire response body into memory.

mvdan · 2017-09-14T18:58:31Z

plugin_build/README.md

+
+Now you must edit the `opts.go` file:
+
+1. For specific gRPC options, edit the `getOpts()`` return value with the gRPC Options you need for your gateway


one too many ` this time I think

mvdan · 2017-09-14T18:58:55Z

plugin_build/README.md

+1. For specific gRPC options, edit the `getOpts()`` return value with the gRPC Options you need for your gateway
+2. Change the name of `var doRegister = changeMe` to whatever is in the `*.pb.gw.go` file, it will be something like:
+
+    ```RegisterYourServiceHandlerFromEndpoint```


If you indent something with four spaces or more, it's already a blockquote - no need for the quotes

mvdan · 2017-09-14T19:01:40Z

The httptest thing seems like a release blocker to me, but otherwise this looks okay to me - you probably want another set of eyes as well, like Leon's.

mvdan · 2017-09-14T20:02:45Z

Also, I just noticed no tests? I realise that a plugin test would be a nightmare, but you could decouple the plugin loading stuff from the actual important code and test that part.

… on the duymmy wrapper

lonelycode · 2017-09-15T09:15:49Z

Which ones in particular? Some I won't fix (like the context).

mvdan · 2017-09-15T09:16:54Z

Which ones in particular? Some I won't fix (like the context).

The markdown formatting ones :)

lonelycode · 2017-09-19T07:59:24Z

Bump

mvdan

LGTM now, but I'd still like @buger's eyes on this before it's merged.

Also, conflicts - feel free to just merge master into this, and we can squash-merge at the end.

mvdan · 2017-09-19T09:51:53Z

vendor/vendor.json

@@ -6,9 +6,7 @@
 			"checksumSHA1": "9BfDgevpBgdiP4B6DT9SvcuVyLs=",
 			"path": "github.com/Jeffail/gabs",


you moved the vendor copy from Jeffail to jeffail, so update this too

mvdan · 2017-09-19T09:55:10Z

grpcproxy/grpcproxy_test.go

+	pName := fmt.Sprintf("%v.so", uuid.NewV4().String())
+	fileArg := fmt.Sprintf("-o=%v", pName)
+
+	cmd := exec.Command("go", "build", "--tags='dummy'", "--buildmode=plugin", fileArg)


This isn't run in a shell, so remove the quotes around dummy (otherwise you really are using 'dummy' with quotes as the build tag)

buger · 2017-09-20T14:11:54Z

Looks good!

However, I'm not sure what "Step 3" with adding custom option does, and why it is needed https://github.com/TykTechnologies/tyk/blob/f07bd7ab0c7312b52258a3755875ef5712d1698e/plugin_build/README.md#step-3-add-a-custom-option-to-the-proto-file
It defines mapping between gRPC method and proxied URL, right?

If so, seems like bakery don't do it, and readme should mention that this step needed even with bakery.

lonelycode · 2017-09-20T18:43:50Z

That's the gRPC proxy specific code annotations to have the proto file generate a proxy in the first place.

You're right though the readme should be clear that yu need to do this with the bakery.

lonelycode · 2017-09-22T03:09:07Z

Bump

buger · 2017-09-22T04:19:19Z

@matiasinsaurralde pls do the QA of this task. Once you confirm that everything works, can be merged.

mvdan · 2017-09-29T14:34:55Z

bump @matiasinsaurralde

lonelycode · 2017-10-09T06:48:36Z

We may need to drop this for v2.4, happy to do so, it's hyper experimental.

stale · 2020-03-25T14:38:21Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs, please add comments to this ticket if you would like it to stay open. Thank you for your contributions.

lonelycode and others added 13 commits September 4, 2017 19:11

Initial plugin loader (via bundler) works as expected. Had to de-vend…

577d20b

…or grpc and trace as grpc plugins fail hard because of type mismatches. Needs more investigation.

There's now a process for compiling plugins safely without vendoring …

fa3d72c

…conflicts *except* golang.org/x/net/trace, which keeps generating duplicate paths on /debug/trace in the muxer. The only solution is to compile tyk with that vendored lib removed - then plugins work.

CLeaned up the code a bit, removed unecessary logs, tested multiple p…

4130ec5

…lugins and reload

Had to re-vendor trace, but cleared the init(), not great, but means …

94625f8

…we can retain vendoring and plugin functionality

Added more vendored packages

ffa8af0

Vendoring and plugins suck.

b2b004c

force merged vendor with master

ae02778

Vendor directory is complete, added a shell script that will disable …

4cf3061

…vendor by renaming and make the vendor dir the GOPATH to guarantee the same build path and versions for plugins

links back to normal

9a9afed

revert symlinks to master

7ad7fca

Made it so wrap tempaltes are not compiled

70bc363

gofmt

ce2f624

imports

1261158

lonelycode requested a review from mvdan September 12, 2017 20:18

mvdan reviewed Sep 12, 2017

View reviewed changes

lonelycode added 8 commits September 13, 2017 16:12

Review changes made, fixed a minor issue in the grpc build script

39fe6d2

Remove broken vendor

1c92fc5

Fixed missing ggrpc gateway dep

7c63a6f

Fixed remaining deps

12ebaea

Mover vendoring shenanigans

5406a8d

Vendooooriiiiing

bce5391

fmt

376a37f

imports

c293ada

mvdan reviewed Sep 13, 2017

View reviewed changes

Addressed review fixes where needed, also fixed bakery to ensure chan…

27308e6

…ges were mirrored in docker hub

mvdan reviewed Sep 14, 2017

View reviewed changes

Added a test for the gRPC loader, generates a plugin on the fly based…

861f185

… on the duymmy wrapper

lonelycode added 9 commits September 16, 2017 09:52

Modified test to be simpler, fixed README

b0e1197

Vixed Tunny

f30df36

Switched tunny

a75287f

Stupid dependency

6d163ae

Removed lowercase jeffail

0eaa45e

Added jeffail lowercase back in

82b441d

Fuck

b54a439

Fixed jeff's FAIL

01f4d12

An unecessary fix

10b794f

mvdan reviewed Sep 19, 2017

View reviewed changes

lonelycode added 5 commits September 20, 2017 08:50

Addressed review

c91941f

Merge master

7d69668

Merge masteragain, fix conflicts

29a0da4

fmt

1812a1f

Fucking global config

f07bd7a

Updated readme for proto proxy

19bfda3

matiasinsaurralde mentioned this pull request Oct 16, 2017

Fix bundle directory creation #1225

Merged

stale bot added the wontfix label Mar 25, 2020

stale bot closed this Apr 24, 2020

furkansenharputlu deleted the grpc-proxy branch May 25, 2021 09:17


		Copy these to the same directory as your stubs.

		Open the .pb.gw.go and .pb.go files, you must change the package name from whatever was generated, to `main`, so the


		It will generate a reverse proxy `path/to/your_service.pb.gw.go`.

		Note: After generating the code for each of the stubs, in order to build the code, you will want to run ```go get .``` from the directory containing the stubs.


		Now you must edit the `opts.go` file:

		1. For specific gRPC options, edit the `getOpts()`` return value with the gRPC Options you need for your gateway

		@@ -6,9 +6,7 @@
		"checksumSHA1": "9BfDgevpBgdiP4B6DT9SvcuVyLs=",
		"path": "github.com/Jeffail/gabs",

Fixes #971 Adds initial capability to add a gRPC Proxy to Tyk using bundler or direct API Def config #1111

Fixes #971 Adds initial capability to add a gRPC Proxy to Tyk using bundler or direct API Def config #1111

Conversation

lonelycode commented Sep 12, 2017 • edited by buger Loading

mvdan left a 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

mvdan left a 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

lonelycode commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvdan commented Sep 14, 2017

mvdan commented Sep 14, 2017

lonelycode commented Sep 15, 2017

mvdan commented Sep 15, 2017

lonelycode commented Sep 19, 2017

mvdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buger commented Sep 20, 2017

lonelycode commented Sep 20, 2017

lonelycode commented Sep 22, 2017

buger commented Sep 22, 2017

mvdan commented Sep 29, 2017

lonelycode commented Oct 9, 2017

stale bot commented Mar 25, 2020

lonelycode commented Sep 12, 2017 •

edited by buger

Loading