Feature/admin pipelines, endpoints, whitelist #418

DrMegavolt · 2017-09-18T19:56:38Z

connect Admin: extend API to include entire pipeline updates #416 connect Admin: extend API to include service endpoints #417 (duplicates connect Admin: extend API to include service endpoint declarations #388) connect Admin: extend API to include policy declaration #385 connect Admin: extend API to include API endpoint declarations #389

pipelines, apiEndpoints, serviceEndpoints
/pipelines, /api-enpoints, /service-endpoints
GET / -> list of all entities
GET /:name -> entity by object key
PUT /:name with body -> create\update entity with this name as key
DELETE /:name -> remove entity

whitelist
GET /policies/whitelist -> list of all allowed policies
PUT /policies/whitelist/:name - add to the list
DELETE /policies/whitelist/:name - rm from list

stanislaw-glogowski · 2017-09-19T15:58:55Z

lib/config/config.js

+  // due to fragile nature of YAWN it may not work properly.
+  updateGently (modifier) {
+    return new Promise((resolve, reject) => {
+      fs.readFile(this.gatewayConfigPath, 'utf8', (err, data) => {


Can we use util.promisify for all callback like methods?

stanislaw-glogowski · 2017-09-19T16:07:01Z

lib/config/config.js

+        if (err) {
+          return reject(err);
+        }
+        let json = yamlOrJson.load(data);


There should be const instead of let. (let is a signal that the variable may be reassigned)

eslint is usually able to catch these things - is it configured in CircleCI?

XVincentX · 2017-09-19T16:09:08Z

lib/config/config.js

@@ -1,4 +1,5 @@
 const fs = require('fs');
+// const YAWN = require('yawn-yaml/cjs');


Leftover comment?

XVincentX · 2017-09-19T16:11:09Z

lib/rest/routes/api-endpoints.js

+      json.apiEndpoints[req.params.name] = req.body;
+      return json;
+    })
+    .then(() => {


Nitpick:
.then(() => res.json({status: 'ok'}));

XVincentX · 2017-09-19T16:11:33Z

lib/rest/routes/api-endpoints.js

+    .then(() => {
+      res.json({status: 'ok'});
+    })
+    .catch(next);


XVincentX · 2017-09-19T16:12:57Z

lib/rest/routes/pipelines.js

+    })
+    .catch(next);
+  });
+  return router;


This seems to be almost the same code as api-endpoints.js (and other files as well), the only difference is the target json object. It can be probably generalized and reused in all the places you need.

no generalization, intentionally copy pasted.
The reason is further differences:
validation is one example
additional methods to manipulate policies

Having some generic monster would greatly affect maintainability

XVincentX · 2017-09-19T16:19:21Z

lib/config/config.js

+          }
+          resolve();
+        });
+      });


I would refactor this piece of code to take advantage of the util.promisify and chain the promises instead of going into a callback hell.

Something like this:
Pseudo code (it might be a bit different)

const fs = require("fs"); const readFile = util.promisify(fs.readFile); const writeFile = util.promisify(fs.writeFile); function updateGently(modifier) { return readFile(this.gatewayConfigPath, 'utf8').then((data) => { let json = yamlOrJson.load(data); let result = modifier(json); let text = yamlOrJson.dump(result); return writeFile(this.gatewayConfigPath, text); }); }

This feels more readable to me but more importantly - easier to reason about.

Can we consider switching to babel? async / await syntax really helps in working with promises.

Personally speaking I would refrain to introduce babel and a transpilation step for node programs. Promises are good enough and can handle all the use cases without sacrificing the readability, it's just matter of reorganizing the flows a bit. We can discuss that separately.

XVincentX · 2017-09-19T16:23:31Z

I can see double commits with the same message 🤔

My only "concern" is the code duplication - it can probably be extracted and reused across the routes when the action we want to propose are almost the same.

I can live with that, anyway ;)

kevinswiber · 2017-09-19T19:05:40Z

I can see double commits with the same message

It's of little consequence. We "Squash & Merge" PRs by default.

codecov · 2017-09-19T20:54:26Z

Codecov Report

Merging #418 into master will decrease coverage by 0.57%.
The diff coverage is 43.63%.

@@           Coverage Diff            @@
##           master   #418      +/-   ##
========================================
- Coverage   90.58%    90%   -0.58%     
========================================
  Files         214    219       +5     
  Lines        8888   8993     +105     
========================================
+ Hits         8051   8094      +43     
- Misses        837    899      +62

Impacted Files	Coverage Δ
lib/admin/index.js	`100% <ø> (ø)`	⬆️
lib/rest/routes/tokens.js	`81.81% <100%> (ø)`	⬆️
lib/rest/routes/apps.js	`83.33% <100%> (ø)`	⬆️
lib/rest/routes/credentials.js	`85% <100%> (ø)`	⬆️
lib/rest/routes/scopes.js	`64.7% <100%> (ø)`	⬆️
lib/rest/index.js	`93.61% <100%> (+0.59%)`	⬆️
lib/rest/routes/users.js	`83.01% <100%> (ø)`	⬆️
lib/admin/config/policies.js	`33.33% <33.33%> (ø)`
lib/rest/routes/policies.js	`33.33% <33.33%> (ø)`
lib/config/config.js	`70.42% <35.71%> (-8.53%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82bfb31...d9fd561. Read the comment docs.

DrMegavolt · 2017-09-19T21:21:32Z

lib/config/config.js

+  update (modifier) {
+    return readFile(this.gatewayConfigPath, 'utf8').then(data => {
+      let text;
+      if (this.systemConfig && this.systemConfig.preserveStructureOnUpdates) {


@kevinswiber seems to be a compromise on yawn with comments vs nuke it all approach?
by default it will be nuke it all, and if people complain it will be a doc section describing this flag
(obviously name can be better)

XVincentX · 2017-09-20T07:59:54Z

lib/rest/routes/api-endpoints.js

+    let isUpdate;
+    config.update(json => {
+      isUpdate = req.params.name in json.apiEndpoints;
+      json.apiEndpoints[req.params.name] = req.body;


I think you guys already discussed this - so bear with me if I am repetitive - but this might be harmful as fundamentally I can put everything I want without having any sort of validation.

I agree as for general idea, why it is not in this PR

I cannot do it for pipeline policies, because it depends on introducing schema for policies in plugins.

Here the only thing that is required is 'host' field. but for paths it is quite complex and I would like to move it after approach for pipelines validation is settled. (+ some intentionally undocumented fields do exist)

and by the way UI is going to add some new fields like fieldId to each entity that has nothing to do with EG, but needed for UI. so its cannot be strict schema

It makes perfectly sense - thanks a lot for the clarification 👍

XVincentX · 2017-09-20T08:01:13Z

lib/rest/routes/pipelines.js

+  router.get('/:name', function (req, res, next) {
+    res.json(config.gatewayConfig.pipelines[req.params.name]);
+  });
+  router.get('/', function (req, res, next) {


Is the next parameter needed for all these routes? This can probably be safely removed.

and what is the problem with having next ?
(req,res), or (req,res,next) are well known signatures for middleware

and it seems that there is no rule in standard js that says it is not good
https://standardjs.com/rules.html

I remember having that rule enabled in the project but I didn't like the experience, because in some cases implementation change and you need add\remove arguments constantly.

but I will agree if this will promoted as ESlint rule

The general rule I found among the project (although it's not codified) is

route: fn(req, res)
middleware: fn(req, res, next)

but it's a nitpick, it's absolutely not blocking anything.

XVincentX · 2017-09-20T08:04:59Z

lib/config/config.js

+        let result = modifier(json);
+        text = yamlOrJson.dump(result);
+      }
+      return writeFile(this.gatewayConfigPath, text);


I like this now so much 👍

XVincentX

Although I'm missing some more general picture, I think we're good to go.

XVincentX · 2017-09-25T12:01:16Z

lib/admin/config/api-endpoints.js

@@ -0,0 +1,27 @@
+module.exports = function (client) {
+  let baseUrl = `api-endpoints/`;


This can be const

stanislaw-glogowski

LGTM

DrMegavolt added 8 commits September 18, 2017 19:29

impl of REST API apiEndpoints

6940a45

impl of REST API apiEndpoints

c159800

rm yawn parsing; too buggy

0b196dc

service-enpoints; pipelines

b91685a

service-enpoints; pipelines

9754431

whitelist

f33fe5a

whitelist

876be4e

reverting to default gateway.config

36598d9

DrMegavolt self-assigned this Sep 18, 2017

DrMegavolt requested review from kevinswiber and kolbinski September 18, 2017 19:56

DrMegavolt added the in progress label Sep 18, 2017

DrMegavolt requested review from XVincentX and stanislaw-glogowski September 19, 2017 15:43

stanislaw-glogowski reviewed Sep 19, 2017

View reviewed changes

XVincentX reviewed Sep 19, 2017

View reviewed changes

yawn\yaml switch; api status codes clarification ; review fixes

d890494

api 201 vs 204 for create

1fd7336

DrMegavolt commented Sep 19, 2017

View reviewed changes

XVincentX reviewed Sep 20, 2017

View reviewed changes

DrMegavolt added 4 commits September 20, 2017 17:06

rm whitelist naming

7c8fb5d

swagger doc

73fc0b1

admin client

bc31366

admin client

3809ff9

XVincentX approved these changes Sep 25, 2017

View reviewed changes

DrMegavolt added 6 commits September 26, 2017 16:24

review fix

d11e952

api endpoint REST tests

7ebc0cb

REST: service ep tests

8c71de1

REST: policies whitelist test

0a23e4e

REST: policies whitelist test

9da6f0c

fix

d9fd561

stanislaw-glogowski approved these changes Sep 26, 2017

View reviewed changes

DrMegavolt merged commit 2746ec7 into master Sep 26, 2017

DrMegavolt deleted the feature/admin-pipelines branch September 26, 2017 16:35

DrMegavolt removed the in progress label Sep 26, 2017

DrMegavolt mentioned this pull request Jan 3, 2018

Admin: extend API to include pipelines #386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/admin pipelines, endpoints, whitelist #418

Feature/admin pipelines, endpoints, whitelist #418

DrMegavolt commented Sep 18, 2017 •

edited

Loading

stanislaw-glogowski Sep 19, 2017

stanislaw-glogowski Sep 19, 2017

XVincentX Sep 19, 2017

XVincentX Sep 19, 2017

XVincentX Sep 19, 2017

XVincentX Sep 19, 2017

XVincentX Sep 19, 2017

DrMegavolt Sep 19, 2017

XVincentX Sep 19, 2017 •

edited

Loading

stanislaw-glogowski Sep 19, 2017

XVincentX Sep 19, 2017

XVincentX commented Sep 19, 2017

kevinswiber commented Sep 19, 2017

codecov bot commented Sep 19, 2017 •

edited

Loading

DrMegavolt Sep 19, 2017

XVincentX Sep 20, 2017

DrMegavolt Sep 20, 2017

XVincentX Sep 20, 2017

XVincentX Sep 20, 2017

DrMegavolt Sep 20, 2017

XVincentX Sep 20, 2017

XVincentX Sep 20, 2017

XVincentX left a comment

XVincentX Sep 25, 2017

stanislaw-glogowski left a comment

		@@ -1,4 +1,5 @@
		const fs = require('fs');
		// const YAWN = require('yawn-yaml/cjs');

		@@ -0,0 +1,27 @@
		module.exports = function (client) {
		let baseUrl = `api-endpoints/`;

Feature/admin pipelines, endpoints, whitelist #418

Feature/admin pipelines, endpoints, whitelist #418

Conversation

DrMegavolt commented Sep 18, 2017 • edited Loading

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

XVincentX Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVincentX commented Sep 19, 2017

kevinswiber commented Sep 19, 2017

codecov bot commented Sep 19, 2017 • edited Loading

Codecov Report

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

XVincentX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanislaw-glogowski left a comment

Choose a reason for hiding this comment

DrMegavolt commented Sep 18, 2017 •

edited

Loading

XVincentX Sep 19, 2017 •

edited

Loading

codecov bot commented Sep 19, 2017 •

edited

Loading