Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend ReturnOverrides #763

Merged
merged 5 commits into from
Jun 5, 2017
Merged

Conversation

matiasinsaurralde
Copy link
Contributor

See #762.

As a sample plugin (test_app.py)

from tyk.decorators import *

@Hook
def MyCustomMiddleware(request, session, spec):
    print("my_middleware: MyCustomMiddleware")
    request.object.return_overrides.headers['content-type'] = 'application/json'
    request.object.return_overrides.response_code = 200
    request.object.return_overrides.response_error = "{\"key\": \"value\"}\n"
    return request, session

With manifest:

{
  "file_list": [
    "test_app.py"
  ],
  "custom_middleware": {
    "post": [
      {"name": "MyCustomMiddleware"}
    ],
    "driver": "python"
  }
}

Curl output:

% curl localhost:8080/test-api/headers -v
*   Trying ::1...
* Connected to localhost (::1) port 8080 (#0)
> GET /test-api/headers HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.43.0
> Accept: */*
> 
< HTTP/1.1 200
< Content-Type: application/json
< Date: Wed, 24 May 2017 08:32:50 GMT
< Content-Length: 17
< 
{"key": "value"}
* Connection #0 to host localhost left intact

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but worried about no test. Maybe it's time we add coprocess tests? The more logic we add without tests, the worse off we'll be in the long run.

}
w.WriteHeader(int(returnObject.Request.ReturnOverrides.ResponseCode))
w.Write([]byte(returnObject.Request.ReturnOverrides.ResponseError))
return nil, 666
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does 666 mean here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

666 is a magic return code that makes the gateway halt MW processing and respond

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see we have 1666 also - we should use constants for these to make them more obvious, like return nil, mwStatusRespond. I'll ticket that up to do once this is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fantastic idea, let's do that! So much clearer

@matiasinsaurralde
Copy link
Contributor Author

@mvdan Just updated the tests, let's wait for #764 to replace the integers with constants.
I'm also extending the rest of the CP tests, trying to enhance them, will be a separate PR.

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, but I'm not very familiar with this code so I'd have another set of eyes on it before merging.


recorder := httptest.NewRecorder()

req, err := http.NewRequest("GET", "/headers", strings.NewReader(""))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a nil body

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nil body panics 😮

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is buggy somewhere... forget I said anything for now :)

@@ -125,6 +125,7 @@ func TestTykTriggerEvent(eventName, eventPayload string) {

//export applyTestHooks
func applyTestHooks(objectPtr unsafe.Pointer) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rogue empty line?

@mvdan mvdan requested a review from buger May 30, 2017 09:24
@buger
Copy link
Member

buger commented May 30, 2017

@matiasinsaurralde can we fix tests?

@mvdan
Copy link
Contributor

mvdan commented Jun 1, 2017

@buger tests are fixed, FYI

@buger
Copy link
Member

buger commented Jun 1, 2017

I'm right that this is similar to what we have now with Virtual Endpoints in our JS middleware?
Previously we already had option to add new headers, but with this functionality it is now possible to write any logic. Right?

Also, does it affect only Coprocess (Lua, Python) plugins, or gRPC ones too (because I see that you changed protobuffers files as well)?

@matiasinsaurralde
Copy link
Contributor Author

I'm not very familiar with the virtual endpoint feature. Yes, basically it's possible to take control of the request flow and return a custom body.
There is an existing feature to add headers from the CP middleware, but it's for request headers (before they hit the upstream API). The funcionality covered by this PR is specific to the response object, and the headers that are specified here are received by the client.
It affects all the plugins and will work as long as you're using the latest definitions and/or bindings provided by us.

buger pushed a commit that referenced this pull request Jun 2, 2017
buger pushed a commit that referenced this pull request Jun 2, 2017
buger pushed a commit that referenced this pull request Jun 2, 2017
buger pushed a commit that referenced this pull request Jun 2, 2017
buger pushed a commit that referenced this pull request Jun 2, 2017
@buger buger merged commit 5c1a032 into TykTechnologies:master Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants