Edge services integration #1

noahdietz · 2017-04-20T22:47:25Z

This is the first pass at EdgeServices integration, many things are still in flux. I would like basic/functional code review. Integration testing is impossible currently due to outstanding permissions issues in Turbo.

To consider during review:

we don't have a default target for EdgeServices yet, thus I have a flag --edgeserviceshost so that i can toggle between running the server locally and the one in GKE. This will be removed.
this integration only works when a token is passed via --token...the other built-in option is basic auth, which Turbo doesn't support, so we need to generate a token when those options are used.
the --preserve-policies option maintains the proxy policies, and changes the node source in Trireme...should we just import a new application revision and update the deployment when using EdgeServices? Currently not supporting this
how chatty should this command be when building an image, deploying an app, etc.? default is silence, but I don't like that it just hangs for 1-2 minutes...thoughts?

* adds building of flag-based arrays

AdamMagaluk · 2017-04-21T15:22:14Z

lib/commands/deploynodeapp.js

+  },
+  runtime: {
+    name: 'Runtime to deploy the Node.js app to',
+    shortOption: 'B',


What short option as -B?

Same comment as below, there are few option shorthands left to pick from that made sense to me.

AdamMagaluk · 2017-04-21T15:22:40Z

lib/commands/deploynodeapp.js

+  },
+  'node-version': {
+    name: 'Version of node to use in EdgeServices',
+    shortOption: 'x',


There are so many options already in use, i just started picking random ones...do an apigeetool deploynodeapp --help and itll list them, plus there are parent flags. Not really sure how to pick from the remaining.

I get ya. I think if the short flag is not relevant to the long flag than we probably just leave it out. Especially for something like node-version that probably wont get set in all cases.

AdamMagaluk · 2017-04-21T15:23:50Z

lib/commands/deploynodeapp.js

+    shortOption: 'B',
+    required: false
+  },
+  stream: {


Could this be rolled into a verbose option? If verbose is set then stream?

Great idea, should've done that sooner! Thanks.

AdamMagaluk · 2017-04-21T15:27:09Z

lib/commands/deploynodeapp.js

+  // we need the XML for a HostedTarget (or whatever)...this is just a place holder that isn't a ScriptTarget
+  if (opts.runtime === EdgeServicesRuntimeOption) {
+    targetDoc = mustache.render(
+    '<TargetEndpoint name="default">' +


Won't we need the policy to get the routing key from the KVM? Not sure if this is the right place.

Agreed, this policy stuff will have to change. Because we have little idea what the policy will look like I added a place holder so I could test all the steps (sans deployment).

Sounds good. However at this point it should look pretty close to the policies the old shipyardctl used right? We are still just using a KVM and routing key in it. Keys and TargetEndpoint url will be different.

We used the ScriptContainer tag which I believe pulled the routing key from the KVM, so that will be the same functionally...will it have to target a different KVM map? I forget how that actually worked. It also targeted the LB in front of our cluster for us...not sure we can just drop it in. Is there a specific change you'd like to see here? I'm not sure what to do without an actually policy to use, but happy to do whatever.

AdamMagaluk · 2017-04-21T15:29:10Z

Overall looks great. For --preserve-policies im not sure the best approach, but that sounds like a decent plan. Deploy a new edge services incase you already had a policy that did other things but had the edge services policy already in it. I think it doesn't matter too much because at the end of the day this would be driven by Edge when the integrations are done.

noahdietz · 2017-04-21T15:50:18Z

@AdamMagaluk Agreed, its not a HUGE deal. But its also easy to implement (I think). If the routing stuff / the associated policies don't need to be changed to accommodate a new version of the application being deployed, then it should be as easy as building a revision & updating(?) a deployment.

* folds --stream into --verbose * adds better shortOption choices

jbowen93 · 2017-04-21T16:17:10Z

lib/commands/deploynodeapp.js

+function deployEdgeServicesApplication(opts, request, imported, cb) {
+  var uri = util.format('http://%s/environments/%s:%s/deployments', opts.edgeserviceshost, opts.organization, opts.environments);
+  var payload = {
+    name: opts.api,


name is optional. If we're setting name == application we might want to just leave name out.

yeah good point...I don't think we need to expose the configurable deployment name in this CLI.

So I can leave the name out of the deployment payload and it'll be set to the application?

jbowen93 · 2017-04-21T16:18:17Z

lib/commands/deploynodeapp.js

+      return cb(new Error('bad request, there was a missing parameter'));
+    } else if (res.statusCode === 409) {
+      return cb(new Error('a deployment with these parametes already exists. cancelling'));
+    } else if (res.statusCode === 200) {


Creating a deployment is going to return a 201 created not a 200 on success.

Cool! I was going off of the Swagger. I'll change it in here to check for a 201.

Good catch. I've updated the swagger in a PR https://github.com/apigee-internal/turbo/pull/138

* makes deployment check for 201 * removes --node-version shortOption

jbowen93 · 2017-04-21T16:41:46Z

LGTM

whitlockjc

There are a few things that need changes and/or explanation.

whitlockjc · 2017-05-10T22:27:45Z

lib/commands/deploynodeapp.js

 var unzip = require('node-unzip-2');
+var targz = require('tar.gz')({}, {


Are we going to still use tar.gz?

we support tar, tgz, zip and i wrote this code before we had zip support as that was a newer addition.

They will never see the temporary archive, so I don't think it matters, personally.

Good enough for me.

whitlockjc · 2017-05-10T22:29:35Z

lib/commands/deploynodeapp.js


 var DeploymentDelay = 60;
 var ProxyBase = 'apiproxy';
+var TriremeRuntimeOption = 'trireme';
+var EdgeServicesRuntimeOption = 'edgeServices'
+var DefaultNodeVersion = '7'


We should use an LTS version like 4.x or 6.x. I'd almost say use the newest version that is in LTS, which would be 6.x.

whitlockjc · 2017-05-10T22:29:52Z

lib/commands/deploynodeapp.js

@@ -72,6 +80,31 @@ var descriptor = defaults.defaultDescriptor({
    name: 'Preserve policies from previous revision',
    shortOption: 'P',
    toggle: true
+  },
+  runtime: {


Any way to set a default value and have it be node?

This is for toggling trireme | edge-services as defined in the design doc. There was offline conversation to change from backend to runtime.

Oh, I got you. Since --runtime is a term we use in Turbo for choosing your base image, maybe we could use something different. I remember liking --backend=[edge-services|trireme] but the more I think about it, we could use a flag like --edge-services as well. The more I think about it, I like the flag because it's really just a simple boolean. I don't see us having more backends for this stuff.

I like that better too, just a flag for toggling on the edge-services runtime. @AdamMagaluk @jbowen93 what do you think?

Works for me.

whitlockjc · 2017-05-10T22:30:23Z

lib/commands/deploynodeapp.js

+    shortOption: 'r',
+    required: false
+  },
+  'node-version': {


This should be runtime-version, not node-version. We don't want to have to change this when we get multi-language support.

Offline conversation resulted in a change to the DD from runtime-version to node-vesrion.

Also, this command is deploynodeapp so there should never be another Turbo supported runtime deployed through this, hence the name node-version

Oh yeah, that's right. This is a node.js-specific command. I guess I can live with it for now.

whitlockjc · 2017-05-10T22:30:49Z

lib/commands/deploynodeapp.js

+    required: false,
+    array: true
+  },
+  edgeserviceshost: { // for testing while we don't have proxy


We should make this an environment variable like get_script and others do.

whitlockjc · 2017-05-10T22:31:41Z

lib/commands/deploynodeapp.js

@@ -425,6 +517,223 @@ function proxyCreationDone(err, req, body, opts, done) {
  }
 }

+function getToken(opts, cb) {
+  var ssoURL = opts.baseuri && opts.baseuri.indexOf('e2e') > -1 ? 'https://login.e2e.apigee.net/oauth/token' : 'https://login.apigee.com/oauth/token'


Use the same approach that the get_token script does by using the SSO_LOGIN_URL environment variable.

whitlockjc · 2017-05-10T22:35:28Z

lib/commands/deploynodeapp.js

+      opts['base-path'] = (opts['base-path'] ? opts['base-path'] : '/'+opts.api);
+
+      opts['env-var'] = opts['env-var'] === undefined ? [] : opts['env-var'];
+      opts['config-var'] = opts['config-var'] === undefined ? [] : opts['config-var'];


Any time you're using an undefined check (this line and a few above) to use a default value, just use something like this:

opts['config-var'] = opts['config-var'] || [];

I realize that there are cases where the first case can pass where you might not want it to but since we own this code, that seems unlikely and this is much simpler to read. Better yet, use underscore#defaults.

whitlockjc · 2017-05-10T22:44:17Z

lib/commands/deploynodeapp.js

@@ -495,14 +804,16 @@ function handleUploadResult(err, req, fileName, itemDone) {

 // Create a target endpoint that references the Node.js script
 function createTarget(opts, request, done) {
-  var targetDoc = mustache.render(


Did you mean to remove this? Shouldn't createTarget generate a template based on the appropriate backend?

Yes it was intentional. I moved away from using this createTarget method entirely.

Instead, when edge-services is targeted, it builds the proxy bundle all at once and uploads it, rather than doing 1 file at a time like the trireme impl does here.

But the Trireme implementation isn't changed is it? That's why I asked.

Correct, the original implementation didn't change. This is a weird diff, see this earlier commit that has the removal of a control structure in this function for template rendering.

noahdietz added 2 commits April 20, 2017 10:19

adds basic EdgeServices support

695f9b6

* adds building of flag-based arrays

differentiates between env & config vars

89a06e5

noahdietz requested review from whitlockjc, AdamMagaluk and jbowen93 April 20, 2017 22:47

AdamMagaluk reviewed Apr 21, 2017

View reviewed changes

changes per review

92df498

* folds --stream into --verbose * adds better shortOption choices

jbowen93 reviewed Apr 21, 2017

View reviewed changes

more changes per review

21ee192

* makes deployment check for 201 * removes --node-version shortOption

AdamMagaluk approved these changes Apr 21, 2017

View reviewed changes

noahdietz added 7 commits May 1, 2017 16:45

adds Edge Services proxy templates

51a08e6

makes proxy creation run on each specified env

0065a7d

adds deployment update on a create 409

fd82aa2

eliminates need for main in package.json with EdgeServices

01e6ae3

fixes deploynodeapp edgeServices final output

129c135

fixes deploynodeapp edgeServices base-path option

2315feb

default base-path to proxy name

36777b5

noahdietz force-pushed the edge-services-integration branch from 848ad5d to 36777b5 Compare May 3, 2017 15:52

remove old comment

309611d

whitlockjc suggested changes May 10, 2017

View reviewed changes

address review comments

2d8480f

noahdietz closed this Sep 11, 2017

		var unzip = require('node-unzip-2');
		var targz = require('tar.gz')({}, {

Edge services integration #1

Edge services integration #1

Conversation

noahdietz commented Apr 20, 2017

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

AdamMagaluk commented Apr 21, 2017

noahdietz commented Apr 21, 2017

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

jbowen93 commented Apr 21, 2017

whitlockjc 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

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