Proxy ElasticSearch service #868

mjgiarlo · 2019-07-02T19:17:00Z

Fixes #630

Includes:

Add middleware to Express server to proxy ElasticSearch requests
- Allow only GET and POST requests to a known path
Add new config entry for index (i.e., ElasticSearch) URL, backed by an environment variable
- Add environment variable to the necessary spots to ensure it is properly fixed at build time (per README)
Handle a FIXME in server.js, preferring .find() over .forEach()

Fixes #630 Includes: * Add middleware to Express server to proxy ElasticSearch requests * Allow only GET and POST requests to a known path * Add new config entry for index (*i.e.*, ElasticSearch) URL, backed by an environment variable * Add environment variable to the necessary spots to ensure it is properly fixed at build time (per README) * Handle a FIXME in server.js, preferring `.find()` over `.forEach()`

mjgiarlo · 2019-07-02T19:17:47Z

.circleci/config.yml

-            docker build -t ld4p/sinopia_editor:dev --build-arg TRELLIS_BASE_URL=$TRELLIS_BASE_URL_DEV --build-arg SINOPIA_URI=$SINOPIA_URI_DEV --build-arg AWS_COGNITO_DOMAIN=$AWS_COGNITO_DOMAIN_DEV --build-arg COGNITO_CLIENT_ID=$COGNITO_CLIENT_ID_DEV --build-arg COGNITO_USER_POOL_ID=$COGNITO_USER_POOL_ID_DEV .
-            docker build -t ld4p/sinopia_editor:stage --build-arg TRELLIS_BASE_URL=$TRELLIS_BASE_URL_STAGE --build-arg SINOPIA_URI=$SINOPIA_URI_STAGE --build-arg AWS_COGNITO_DOMAIN=$AWS_COGNITO_DOMAIN_STAGE --build-arg COGNITO_CLIENT_ID=$COGNITO_CLIENT_ID_STAGE --build-arg COGNITO_USER_POOL_ID=$COGNITO_USER_POOL_ID_STAGE .
-            docker build -t ld4p/sinopia_editor:prod --build-arg TRELLIS_BASE_URL=$TRELLIS_BASE_URL_PROD --build-arg SINOPIA_URI=$SINOPIA_URI_PROD --build-arg AWS_COGNITO_DOMAIN=$AWS_COGNITO_DOMAIN_PROD --build-arg COGNITO_CLIENT_ID=$COGNITO_CLIENT_ID_PROD --build-arg COGNITO_USER_POOL_ID=$COGNITO_USER_POOL_ID_PROD .
+            docker build -t ld4p/sinopia_editor:dev --build-arg TRELLIS_BASE_URL=$TRELLIS_BASE_URL_DEV --build-arg SINOPIA_URI=$SINOPIA_URI_DEV --build-arg AWS_COGNITO_DOMAIN=$AWS_COGNITO_DOMAIN_DEV --build-arg COGNITO_CLIENT_ID=$COGNITO_CLIENT_ID_DEV --build-arg COGNITO_USER_POOL_ID=$COGNITO_USER_POOL_ID_DEV --build-arg INDEX_URL=$INDEX_URL_DEV .


FYI, I have already tracked down these values and added new env-specific variables to the CircleCI build.

mjgiarlo · 2019-07-02T21:03:00Z

package.json

@@ -116,6 +116,7 @@
    "react-router-dom": "^4.3.1",
    "redux": "^4.0.1",
    "redux-thunk": "^2.3.0",
+    "request": "^2.88.0",


It seems in Javascriptland there are a million options here, and it's not clear to me which is the best fit or the most idiomatic. Happy to consider alternatives.

This module request is fine.

mjgiarlo · 2019-07-02T21:04:38Z

server.js

+app.use(express.json())
+
+app.use('/api/search', (req, res) => {
+  if (!['GET', 'POST'].includes(req.method)) {


We only allow the editor to hit the search part of ElasticSearch's API, which means either GET or POST could be used. Throw a 400 error if any other HTTP action is attempted.

mjgiarlo · 2019-07-02T21:06:03Z

server.js

+
+  // Hard-coded for now since we only have the one use case for proxying ES,
+  // i.e., full-text search of resources
+  if (req.path !== '/sinopia_resources/sinopia/_search') {


I don't love this approach but I was loath to get into regex or otherwise parsing the request path, lest I leave some wrinkle that could be exploited. I am open to validating requests some other way, to prevent our ElasticSearch instance from getting owned or hosed.

mjgiarlo · 2019-07-02T21:07:27Z

server.js

 const versoSpoof = require('./src/versoSpoof')

 const port = 8000
 const app = express()

-app.use(express.static(`${__dirname}/`))
+// Required for ElasticSearch proxy middleware to parse response body as JSON


Note that server.js doesn't have test coverage. I'm not sure exactly how or what I'd test. I could consider extracting the middleware elsewhere so it's easier to test but that's not super clear to me at the moment.

jermnelson · 2019-07-02T22:29:25Z

Hi @mjgiarlo, I am getting this error both when I rebuild the Editor container and try to run with docker-compose up editor as well as trying to run npm start directly to test this route:

sinopia_editor/src/Config.js:109
export default Config
^^^^^^
SyntaxError: Unexpected token export

I'm hoping this my user error :-).

mjgiarlo · 2019-07-02T22:31:08Z

Hi @mjgiarlo, I am getting this error both when I rebuild the Editor container and try to run with docker-compose up editor as well as trying to run npm start directly to test this route:
sinopia_editor/src/Config.js:109
export default Config
^^^^^^
SyntaxError: Unexpected token export  
I'm hoping this my user error :-).

@jermnelson That does not ring a bell. Weird! Happy to pair in the morning.

ndushay · 2019-07-03T00:37:39Z

server.js

-        }
-      })
-      res.status(200).type('application/json').send(json)
+      const json = versoSpoof.owlOntUrl2JsonMappings.find(el => reqUri === el.url).json


huzzah for getting rid of FIXME. 👍

ndushay

LGTM; I have a couple of questions

ndushay · 2019-07-03T00:38:02Z

server.js

 })

+app.use(express.static(`${__dirname}/`))


I don't get what this line is doing

@ndushay This middleware is for serving static assets to the browser, such as the stuff in src/styles/ and static/. I'm not sure how "right" this code is given how we're using it, tbh, and I didn't add it—it was shifted around by my work. This code was added 4 years ago by LC: fb13bb2#diff-dc2704ac83baf8775944bab1251b45abR9

If we think we should tighten up how we serve static assets... new issue?

how about just adding a comment:

//serve static assets to the browser, e.g. from src/styles/ and static/

And mayhaps making a ticket to move "styles" out of "src" directory??

I was/am hoping someone with more knowledge of express could write up the ticket. I'm reluctant to pile that on the backlog without a clearer sense of the problem or the solution. (Sorry, still recovering from Hyrax backlog stress, where anyone and everyone tossed issues that were ill defined and often just questions.)

ok. I can live w/o the ticket, but would appreciate the comment.

@ndushay ok.

@ndushay 💥 #887 💥

ndushay · 2019-07-03T00:39:36Z

server.js

-      res.status(200).type('application/json').send(json)
+      const json = versoSpoof.owlOntUrl2JsonMappings.find(el => reqUri === el.url).json
+
+      res.json(json)


can you say more about this line vs old line 43 ? is status 200 the default? Is application/json the default type?

@ndushay Oh, shoot, I meant to leave a comment about this line but missed it. Sorry! Sure.

We can safely shorten res.status(200).type('application/json').send(json) to res.json(json) because Express will return an HTTP 200 OK by default, and calling .json() automatically sets the content type (to application/json), sends the content to the client, and ends the request/response cycle.

Else the server will not start because src/Config.js uses ES6 and by default npm will use node vs. babel-node when running npm start.

jermnelson

LGTM; merging.

@ndushay

Requested by @ndushay in #868 Also: use ES6 import style now that server is invoked via `npx babel-node`, for consistency with the rest of the codebase.

mjgiarlo added the M4 label Jul 2, 2019

mjgiarlo added this to the M04: Production of Original Data: Create and save data in Editor milestone Jul 2, 2019

mjgiarlo added this to Needs Review in Sinopia Work-Cycle One via automation Jul 2, 2019

mjgiarlo commented Jul 2, 2019

View reviewed changes

Add the request dependency

bd34a9c

mjgiarlo commented Jul 2, 2019

View reviewed changes

mjgiarlo marked this pull request as ready for review July 2, 2019 21:07

ndushay reviewed Jul 3, 2019

View reviewed changes

Sinopia Work-Cycle One automation moved this from Needs Review to In Progress Jul 3, 2019

ndushay approved these changes Jul 3, 2019

View reviewed changes

ndushay moved this from In Progress to Needs Review in Sinopia Work-Cycle One Jul 3, 2019

Use babel-node to run Express server

70232e7

Else the server will not start because src/Config.js uses ES6 and by default npm will use node vs. babel-node when running npm start.

Sinopia Work-Cycle One automation moved this from Needs Review to In Progress Jul 3, 2019

jermnelson approved these changes Jul 3, 2019

View reviewed changes

jermnelson merged commit a897214 into master Jul 3, 2019

Sinopia Work-Cycle One automation moved this from In Progress to Done Jul 3, 2019

jermnelson deleted the proxy_elasticsearch_630 branch July 3, 2019 16:56

mjgiarlo added a commit that referenced this pull request Jul 3, 2019

Add comments to Express server

f922265

Requested by @ndushay in #868 Also: use ES6 import style now that server is invoked via `npx babel-node`, for consistency with the rest of the codebase.

mjgiarlo mentioned this pull request Jul 3, 2019

Add comments to Express server #887

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proxy ElasticSearch service #868

Proxy ElasticSearch service #868

mjgiarlo commented Jul 2, 2019

mjgiarlo Jul 2, 2019

mjgiarlo Jul 2, 2019

jermnelson Jul 3, 2019

mjgiarlo Jul 2, 2019

mjgiarlo Jul 2, 2019

mjgiarlo Jul 2, 2019

jermnelson commented Jul 2, 2019

mjgiarlo commented Jul 2, 2019

ndushay Jul 3, 2019

mjgiarlo Jul 3, 2019

ndushay left a comment

ndushay Jul 3, 2019

mjgiarlo Jul 3, 2019

ndushay Jul 3, 2019

mjgiarlo Jul 3, 2019

ndushay Jul 3, 2019

mjgiarlo Jul 3, 2019

mjgiarlo Jul 3, 2019

ndushay Jul 3, 2019

mjgiarlo Jul 3, 2019

jermnelson left a comment

		})

		app.use(express.static(`${__dirname}/`))

Proxy ElasticSearch service #868

Proxy ElasticSearch service #868

Conversation

mjgiarlo commented Jul 2, 2019

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

jermnelson commented Jul 2, 2019

mjgiarlo commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay 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

jermnelson left a comment

Choose a reason for hiding this comment