Added CORS headers to enable Front-End inputs on Go Engine #1952

ericandrewmeadows · 2020-06-15T16:54:29Z

Description

Added CORS headers to all requests
- All front-end web requests were getting rejected
Added OPTIONS Request type, which is done via CORS Preflight
- All OPTIONS requests, typical in CORS Preflight, were being rejected

Fixes # CORS issue in Slack

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unit Tests
Integration Tests
Production deployment (at MLB)

Test Configuration:

Go version: go version go1.14.3 darwin/amd64

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

seldondev · 2020-06-15T16:54:46Z

Hi @ericandrewmeadows. Thanks for your PR.

I'm waiting for a SeldonIO member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

executor/api/rest/middlewares.go

executor/api/rest/server.go

SachinVarghese · 2020-06-16T08:09:41Z

executor/api/rest/middlewares.go

@@ -16,6 +17,13 @@ const (

 	contentTypeOptsHeader = "X-Content-Type-Options"
 	contentTypeOptsValue  = "nosniff"
+
+	corsAllowOriginHeader  = "Access-Control-Allow-Origin"
+	corsAllowOriginValue   = "*"


The default values for these headers are very lax from the security perspective. Maybe these values should be set from a custom config and the defaults should be made more strict.

👍 - I agree.

adriangonz

Thanks for your contribution @ericandrewmeadows!

I think that the code looks good, besides what @SachinVarghese mentioned (which could be a blocker in some use cases). Maybe we could read Access-Control-Allow-Origin from an environment variable?

I've also noticed that gorilla/mux seems to provide support for CORS out of the box. Would it be possible to change it to use this implementation? It seems that it also takes care of adding the OPTIONS handler.

adriangonz · 2020-06-16T08:28:44Z

/ok-to-test
/test integration

seldondev · 2020-06-16T08:30:09Z

Tue Jun 16 08:30:07 UTC 2020
The logs for [lint] [3] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/3.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=3

seldondev · 2020-06-16T08:30:21Z

Tue Jun 16 08:30:20 UTC 2020
The logs for [pr-build] [1] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/1.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=1

seldondev · 2020-06-16T08:30:21Z

Tue Jun 16 08:30:20 UTC 2020
The logs for [integration] [2] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/2.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=2

seldondev · 2020-06-16T18:42:50Z

Tue Jun 16 18:42:46 UTC 2020
The logs for [lint] [7] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/7.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=7

seldondev · 2020-06-16T18:44:01Z

Tue Jun 16 18:44:00 UTC 2020
The logs for [pr-build] [6] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/6.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=6

ericandrewmeadows · 2020-06-16T23:54:31Z

@adriangonz - after diving in very deep on Gorilla's CORS implementation, it isn't actually setting the headers, even though the library looks as if it would, rendering it unusable. This was tried a multitude of ways - I'm happy to chat through this on Slack tomorrow - But I went deep into testing and found that the library is just insufficient.

See gorilla/handlers#191

ericandrewmeadows · 2020-06-17T02:26:49Z

The only Gorilla method that works properly is r.Router.Use(mux.CORSMethodMiddleware(r.Router)), but this results in a return type that doesn't seem befitting of a quality web backend, so I would caution against using the Gorilla here as well because there should be only a single instance of each method in the return. It works, but yields the following:

Access-Control-Allow-Methods: OPTIONS,POST,OPTIONS,POST,OPTIONS,POST

I have returned my code that handles a hard-coded list of the types because the request presentation won't confuse any devs going forward.

In comparison - my result:

curl -v -X OPTIONS localhost:8000/api/v0.1/predictions -d '{"data":{"ndarray":[[1.0,2.0]]}}'
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8000 (#0)
> OPTIONS /api/v0.1/predictions HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.64.1
> Accept: */*
> Content-Length: 32
> Content-Type: application/x-www-form-urlencoded
> 
* upload completely sent off: 32 out of 32 bytes
< HTTP/1.1 200 OK
< Access-Control-Allow-Headers: Accept, Accept-Encoding, Authorization, Content-Length, Content-Type, X-CSRF-Token
< Access-Control-Allow-Methods: GET, OPTIONS, POST
< Access-Control-Allow-Origin: *
< Seldon-Puid: 2a0d139f-e856-4fd1-9bd1-947467e0144c
< X-Content-Type-Options: nosniff
< Date: Wed, 17 Jun 2020 02:27:12 GMT
< Content-Length: 0
< 
* Connection #0 to host localhost left intact
* Closing connection 0

adriangonz · 2020-06-17T09:07:10Z

/test integration

Those are great insights on gorilla/mux @ericandrewmeadows! It's good to be aware of those limitations.

From my side, the changes look great now. I think this is ready to land once the integration tests pass.

seldondev · 2020-06-17T09:09:36Z

Wed Jun 17 09:09:34 UTC 2020
The logs for [integration] [8] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/8.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=8

adriangonz · 2020-06-17T13:19:08Z

/test integration

seldondev · 2020-06-17T13:20:31Z

Wed Jun 17 13:20:30 UTC 2020
The logs for [integration] [9] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/9.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=9

adriangonz · 2020-06-17T15:52:10Z

There are two failed tests, but TBH they just seem flaky (i.e. maybe waiting for Ambassador to come up). The core ones seem to pass.

TestBatchWorker.test_batch_worker:

seldon-core/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/9.log

Lines 1617 to 1668 in 9339683

    
           =================================== FAILURES =================================== 
        
           ______________________ TestBatchWorker.test_batch_worker _______________________ 
        
           [gw2] linux -- Python 3.6.10 /usr/local/bin/python 
        
           self = <test_batch_processor.TestBatchWorker object at 0x7fa5abd84f28> 
        
           namespace = 'test-batch-worker' 
        
               def test_batch_worker(self, namespace): 
        
                   spec = "../../servers/sklearnserver/samples/iris.yaml" 
        
                   retry_run(f"kubectl apply -f {spec} -n {namespace}") 
        
                   wait_for_status("sklearn", namespace) 
        
                   wait_for_rollout("sklearn", namespace) 
        
                   time.sleep(1) 
        
                   batch_size = 1000 
        
                   input_data_path = "input-data.txt" 
        
                   output_data_path = "output-data.txt" 
        
                   with open(input_data_path, "w") as f: 
        
                       for i in range(batch_size): 
        
                           f.write("[[1,2,3,4]]\n") 
        
                   start_multithreaded_batch_worker( 
        
                       "sklearn", 
        
                       "istio", 
        
                       namespace, 
        
                       API_ISTIO_GATEWAY, 
        
                       "rest", 
        
                       "data", 
        
                       "ndarray", 
        
                       100, 
        
                       3, 
        
                       input_data_path, 
        
                       output_data_path, 
        
                       "predict", 
        
                       "debug", 
        
                       True, 
        
                       str(uuid.uuid1()), 
        
                   ) 
        
                   with open(output_data_path, "r") as f: 
        
                       for line in f: 
        
                           output = json.loads(line) 
        
                           # Ensure all requests are successful 
        
           >               assert output.get("data", {}).get("ndarray", False) 
        
           E               AssertionError: assert False 
        
           E                +  where False = <built-in method get of dict object at 0x7fa5abd528b8>('ndarray', False) 
        
           E                +    where <built-in method get of dict object at 0x7fa5abd528b8> = {}.get 
        
           E                +      where {} = <built-in method get of dict object at 0x7fa5abda7168>('data', {}) 
        
           E                +        where <built-in method get of dict object at 0x7fa5abda7168> = {'meta': {'tags': {'batch_id': '907316de-b0a1-11ea-bc88-3e5a72e7b2db', 'batch_index': 2, 'batch_instance_id': '90747394-b0a1-11ea-aa11-3e5a72e7b2db'}}, 'status': {'info': 'FAILURE', 'reason': '', 'status': 1}}.get 
        
           test_batch_processor.py:58: AssertionError

TestPrepack.test_tfserving:

seldon-core/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/9.log

Lines 1690 to 1712 in 9339683

    
           __________________________ TestPrepack.test_tfserving __________________________ 
        
           [gw1] linux -- Python 3.6.10 /usr/local/bin/python 
        
           self = <test_prepackaged_servers.TestPrepack object at 0x7f8ac6e274e0> 
        
           namespace = 'test-tfserving' 
        
               def test_tfserving(self, namespace): 
        
                   spec = "../../servers/tfserving/samples/mnist_rest.yaml" 
        
                   retry_run(f"kubectl apply -f {spec}  -n {namespace}") 
        
                   wait_for_status("tfserving", namespace) 
        
                   wait_for_rollout("tfserving", namespace) 
        
                   time.sleep(1) 
        
                   logging.warning("Initial request") 
        
                   r = initial_rest_request( 
        
                       "tfserving", 
        
                       namespace, 
        
                       data=[create_random_data(784)[1].tolist()], 
        
                       dtype="ndarray", 
        
                   ) 
        
           >       assert r.status_code == 200 
        
           E       AttributeError: 'NoneType' object has no attribute 'status_code' 
        
           test_prepackaged_servers.py:63: AttributeError

@axsaucedo do you think that the test_batch_worker one could be just due to flakiness?

ericandrewmeadows · 2020-06-17T16:15:50Z

@adriangonz - I was seeing those fail, and wasn't fully sure why. We do intercept and add the headers to all requests, so I don't perceive that causing a failure here.

axsaucedo · 2020-06-19T15:09:28Z

Yes, it seems that we may need to increase the number of retries in the batch processor.
/test integration

seldondev · 2020-06-19T15:10:39Z

Fri Jun 19 15:10:38 UTC 2020
The logs for [integration] [10] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1952/10.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1952 --build=10

adriangonz · 2020-06-19T16:05:00Z

/approve

@axsaucedo thanks for clarifying that point! In that case, I think it's safe to land this one.

seldondev · 2020-06-19T16:05:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriangonz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [adriangonz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

seldondev · 2020-06-19T16:54:21Z

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

Test name	Commit	Details	Rerun command
integration	`5d2a533`	link	`/test integration`

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.

axsaucedo · 2020-06-19T16:58:48Z

@adriangonz yes batch test passed, currently failed test is TestTagsPythonS2iK8s.test_model_combiner_grpc which i'm not sure why exactly is the case but it's not related to this at all. All other tests pass so shoudl eb good to go. I'll add an issue to increase retries on batch processor test

seldondev · 2021-05-13T06:29:51Z

failed to trigger Pull Request pipeline

failed to create agent
failed to calculate in repo config
failed to load trigger config for repository SeldonIO/seldon-core for ref 5d2a533
failed to find any lighthouse configuration files in repo SeldonIO/seldon-core at sha 5d2a533
failed to process repo SeldonIO/seldon-core refref 5d2a533
failed to list files in directory /var/tmp/gitrepo660600678/.lighthouse
open /var/tmp/gitrepo660600678/.lighthouse
no such file or directory

Eric Meadows added 2 commits June 15, 2020 12:41

Fixed CORS

a3ab865

Added docstring; more verbose function name

4140a11

seldondev added the needs-ok-to-test label Jun 15, 2020

seldondev added the size/M label Jun 15, 2020

ericandrewmeadows commented Jun 15, 2020

View reviewed changes

executor/api/rest/middlewares.go Show resolved Hide resolved

ericandrewmeadows commented Jun 15, 2020

View reviewed changes

executor/api/rest/server.go Show resolved Hide resolved

SachinVarghese reviewed Jun 16, 2020

View reviewed changes

adriangonz suggested changes Jun 16, 2020

View reviewed changes

seldondev assigned adriangonz Jun 16, 2020

seldondev added ok-to-test and removed needs-ok-to-test labels Jun 16, 2020

Environment variable setting enabled

65dfd7e

seldondev added size/L and removed size/M labels Jun 16, 2020

fixed merge conflict

5d2a533

ericandrewmeadows requested a review from adriangonz June 17, 2020 02:30

seldondev added the approved label Jun 19, 2020

axsaucedo merged commit 79ca088 into SeldonIO:master Jun 19, 2020

axsaucedo unassigned adriangonz May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added CORS headers to enable Front-End inputs on Go Engine #1952

Added CORS headers to enable Front-End inputs on Go Engine #1952

ericandrewmeadows commented Jun 15, 2020 •

edited

Loading

seldondev commented Jun 15, 2020

SachinVarghese Jun 16, 2020

ericandrewmeadows Jun 16, 2020

adriangonz left a comment

adriangonz commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

ericandrewmeadows commented Jun 16, 2020 •

edited

Loading

ericandrewmeadows commented Jun 17, 2020 •

edited

Loading

adriangonz commented Jun 17, 2020

seldondev commented Jun 17, 2020

adriangonz commented Jun 17, 2020

seldondev commented Jun 17, 2020

adriangonz commented Jun 17, 2020

ericandrewmeadows commented Jun 17, 2020

axsaucedo commented Jun 19, 2020

seldondev commented Jun 19, 2020

adriangonz commented Jun 19, 2020

seldondev commented Jun 19, 2020

seldondev commented Jun 19, 2020

axsaucedo commented Jun 19, 2020

seldondev commented May 13, 2021

Added CORS headers to enable Front-End inputs on Go Engine #1952

Added CORS headers to enable Front-End inputs on Go Engine #1952

Conversation

ericandrewmeadows commented Jun 15, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

seldondev commented Jun 15, 2020

SachinVarghese Jun 16, 2020

Choose a reason for hiding this comment

ericandrewmeadows Jun 16, 2020

Choose a reason for hiding this comment

adriangonz left a comment

Choose a reason for hiding this comment

adriangonz commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

seldondev commented Jun 16, 2020

ericandrewmeadows commented Jun 16, 2020 • edited Loading

ericandrewmeadows commented Jun 17, 2020 • edited Loading

adriangonz commented Jun 17, 2020

seldondev commented Jun 17, 2020

adriangonz commented Jun 17, 2020

seldondev commented Jun 17, 2020

adriangonz commented Jun 17, 2020

ericandrewmeadows commented Jun 17, 2020

axsaucedo commented Jun 19, 2020

seldondev commented Jun 19, 2020

adriangonz commented Jun 19, 2020

seldondev commented Jun 19, 2020

seldondev commented Jun 19, 2020

axsaucedo commented Jun 19, 2020

seldondev commented May 13, 2021

ericandrewmeadows commented Jun 15, 2020 •

edited

Loading

ericandrewmeadows commented Jun 16, 2020 •

edited

Loading

ericandrewmeadows commented Jun 17, 2020 •

edited

Loading