Added custom metrics binding credential generation and management in APIServer #376

paltanmoy · 2018-08-13T16:03:12Z

Custom Metrics feature related changes[For ServiceBroker and APIServer only].

cfdreddbot · 2018-08-13T16:03:13Z

Hey paltanmoy!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

cf-gitbot · 2018-08-13T16:03:13Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/159738594

The labels on this github issue will be updated when the story is started.

cdlliuy · 2018-08-16T07:00:37Z

@paltanmoy , Is that possible to break this PR with 2930 insertions into smaller ones , i.e. to handle credential generation and management only ? Regardless custom metric, credential generation is a "valid" topic in service broker and api server ..

boyang9527 · 2018-08-16T18:05:51Z

api/lib/routes/credentialHelper.js

+            'username': creds.username,
+            'password': creds.password
+        };
+        var isCached = credentialCache.set(appId, cachedCred, cacheTTL);


any reason the cred is not cached? if it is not cached, it is not good to have log message saying "credential cached"

boyang9527 · 2018-08-16T21:42:26Z

api/lib/validation/schemaValidator.js

  var schema = {
    'type': 'object',
    'id':'/scaling_rules',
    'properties' : {
-      'metric_type':{ 'type':'string' ,'enum':metricTypeEnum },
+      'metric_type':{ 'type':'string' },


we probably will need to add a prefix for custom metrics, to distinguish whether it is custom metrics or not. This will also avoid conflict with built-in metrics.

We need to call this out in doc as well.

boyang9527 · 2018-08-16T21:56:08Z

src/integration/integration_broker_api_test.go

+
+				checkResponseContent(getPolicy, appId, http.StatusOK, expected, INTERNAL)
+			})
+		})


need to add another test case when custom metrics support for unbind

Added Unbind Test case

boyang9527 · 2018-08-16T22:02:28Z

api/lib/routes/credentials.js

+  var HttpStatus = require('http-status-codes');
+  var credHelper = require('./credentialHelper')(models, credentialCache, cacheTTL);
+
+  router.get('/:app_id/creds', function(req, resp) {


should be it "POST /v1 /apps/:app_id/credentials" ?

Yes , all endpoints have been modified as suggested

boyang9527 · 2018-08-16T22:03:00Z

api/lib/routes/credentials.js

+    });
+  });
+
+  router.delete('/:app_id/creds', function(req, res) {


DELETE /v1 /apps/:app_id/credentials

boyang9527 · 2018-08-16T22:11:10Z

api/lib/routes/credentials.js

+    });
+  });
+
+  router.get('/:app_id/creds/validate', function(req, resp) {


should it be "POST /v1 /apps/:app_id/credentials/validate" ?

boyang9527 · 2018-08-16T22:16:59Z

api/lib/routes/credentialHelper.js

+    });
+  }
+
+  credhelper.deleteCredentials = function(req, callback) {


delete credential will need to invalidate the entry in cache

Cache invalidation added.

boyang9527 · 2018-08-16T22:18:12Z

api/lib/routes/credentialHelper.js

+      id: req.params.app_id,
+      username: generateHash(username),
+      password: generateHash(password)
+    }).then(function(updatedData) {


seems should be opposite.... if updatedData, then you should log the credential hasbeen updated succeesfully

Nope, rather sequelize upsert returns a promise with a boolean indicating whether the data is created or not. So renamed updatedData to createdData

boyang9527 · 2018-08-16T22:19:38Z

api/lib/routes/credentialHelper.js

+      password: generateHash(password)
+    }).then(function(updatedData) {
+      if (updatedData) {
+        logger.info('New credentials hasbeen generated successfully', {


if updatedData, cache entry needs to be invalidated

sorry I forgot there will be multiple instances of api server, so invalidate local cache will not invalidate cache of other instances. So if we don't have distributed remote cache, we will need to allow using old credentials until it expires in cache. However if the check with cache fails, we need to recheck with database to make sure we don't reject request which uses new credentials while the old credential is not expired in cache yet.

boyang9527 · 2018-08-22T22:30:56Z

api/lib/routes/credentialHelper.js

+      isValidCred = validateCredentialDetails(username, creds.username, password, creds.password);
+      logger.info('Credentials hasbeen found successfully in cache', {
+          'app_id': appId,
+          'isValid': isValidCred


if it is not valid, we may need to check with database... and refresh cache.

boyang9527 · 2018-08-24T22:47:45Z

LGTM

qibobo · 2018-08-27T03:26:05Z

api/lib/routes/credentialHelper.js

+    var password = uuidv4();
+    var appId = req.params.app_id;
+    models.credentials.upsert({
+      id: req.params.app_id,


which one? id => appId or app_id => appId ?

var appId = req.params.app_id;
appId has been defined, so better to " id: appId" ?

qibobo · 2018-08-27T03:26:46Z

api/lib/routes/credentialHelper.js

+      password: generateHash(password)
+    }).then(function(createdData) {
+      if (createdData) {
+        logger.info('New credentials hasbeen generated successfully', {


hasbeen -> has been?

qibobo · 2018-08-27T03:26:54Z

api/lib/routes/credentialHelper.js

+        });
+      }
+      else {
+        logger.info('Existing credentials hasbeen updated successfully', {


hasbeen -> has been?

qibobo · 2018-08-27T03:44:14Z

api/test/unit/config/settings_test.js

@@ -87,6 +88,7 @@ describe('config setting Test Suite', function () {
    expect(settings.cfApi).to.equal("https://" + defaultConfig.cfApi.toLowerCase());
    expect(settings.infoFilePath).to.equal(defaultConfig.infoFilePath);
    expect(settings.skipSSLValidation).to.equal(false);
+    expect(settings.cacheTTL).to.equal(defaultConfig.cacheTTL);


where is the test case for cacheTTL validation?

Test case added

qibobo · 2018-08-27T03:51:49Z

api/lib/routes/credentialHelper.js

+    }
+    // Try to find credentials in cache
+    try{
+      creds = credentialCache.get(appId, true);


Is it better to get data from database directly when there is not data in cache rather than throwing an error and catching it?

As getting creds directly from database is being already handled in catch block ..tried handling in the same way.

qibobo · 2018-08-27T05:22:36Z

servicebroker/lib/util/apiServerUtil.js

+    });
+  };
+
+  apiUtil.validateCreds = function(parameters, callback){


Is this function useful?

Nope. Removed the function.

qibobo · 2018-08-27T05:29:22Z

servicebroker/test/routes/binding_test.js

            binding.count({ where: { bindingId: bindingId } }).then(function(countRes) {
              expect(countRes).to.equal(1);
              done();
            })
          });
      });
+
+      it("failed to create a new binding because of credential generation failure", function(done) {


The binding creation only fails when enableCustomMetrics is true.

in test settings enableCustomMetrics is true only

qibobo · 2018-08-29T06:52:23Z

LGTM

* Consolidate all db maintenance of pruner to operator (#368) * Consolidate all db maintenance of pruner to operator * update per comments * wrong sample request in docs (#374) * Bug fix in liquibase change set when rename pruner to operator * Added DB schema for custom-metrics credentials management (#373) * Added DB schema for custom-metrics credentials management * Deleted binding_id column * Update api.db.changelog.yml * adapt public rest api to cf style (#371) * adapt public rest api to cf style [#159391488] * both order-direction and order are valid to compatible with old version * page * refactor router helper and update prevUrl&nextUrl method * add test cases for function getPageUrl * test case update * Instance metrics can be queried by instance index (#377) * Instance metrics can be queried by instance index [#159756219] * when instance-index is not provided, returns instance metrics of all instances * update for comments * update for comments * Added custom metrics binding credential generation and management in APIServer (#376) * Custom Metrics broker api changes * Modified Integration Test * Minor update * Incorporated Review comments * Updated Integration tests * credentials generated during binding without policy * Rechecking database for invalid credential cache entry * Incorporated Review comments * refactor config of go components (#378) * Add maven dependencies to scheduler to be compatible with JAVA9 or higher versions. (#380) [#160224877] * Check user authorization in apiserver with cc token_point instead of authorization_endpoint (#382) [#160438532] * prune non-existent application details (#381) * synchronize application details * Pruning apps only for CF-AppNotFound type of error * pruning of app is restricted to only 404 statusCode * Updated to incorporate review comments * cache metric data in metrics collector (#379) * cache metric data in metrics collector * update based on review comments * fix the race condition in test (#386) * Add http connection timeout as a configuration to all autoscaler components (#387) * Add http connection timeout as a configuration to all autoscaler components [#160241806] * rename http_request_timeout to http_client_timeout * typo * override tcp dial timeout of cfhttp httpclient (#389) * Added validation to restrict threshold value to be integer only (#392) * [auth] allow admin to access autoscaler (#393) * [auth] allow admin to access autoscaler * [auth] check token via uaa * [auth] add test cases * [auth] supplement test cases * [auth] update test * [auth] [7]string -> []string * Add db connection limitation configurations to scheduler (#395) [#161710050] * Add health endpoint for go component (#394) * Add health endpoint for go component * update for comments * Add health endpoint for scheduler (#397) [#161745454] * unify all the golang fakes (#398) [#161774071] * cut off duration of operator should be any duration rather than days (#399) * cut off duration of operator should be any duration rather than days [#161834647] * the default cut off duration is 30 days * enable cpu metric collection * Configuration enhancement for scheduler to prevent logjam vulnerability (#400) [#161873300] * Updated metric_type schema to support custom metrics (#404) * Updated metric_type schema to support custom metrics - metric_type can be alphanumeric characters and it can also have underscores * Updated apiserver policy validation * enhance integration test for bug "TLS handshake error between operator and scheduler" (#405) [#162230075]

paltanmoy added 2 commits August 13, 2018 13:57

Custom Metrics broker api changes

d40b644

Modified Integration Test

c681837

cf-gitbot added the unscheduled label Aug 13, 2018

paltanmoy mentioned this pull request Aug 13, 2018

Added custom metrics binding credential generation and management #360

Closed

paltanmoy changed the title ~~Custom metrics servicebroker api~~ Added custom metrics binding credential generation and management in APIServer Aug 13, 2018

Minor update

c4015fa

boyang9527 reviewed Aug 16, 2018

View reviewed changes

boyang9527 requested a review from qibobo August 16, 2018 22:19

Incorporated Review comments

bfdb593

paltanmoy force-pushed the custom-metrics-servicebroker-api branch from 3425464 to bfdb593 Compare August 17, 2018 15:47

paltanmoy added 2 commits August 20, 2018 14:08

Updated Integration tests

9332fbd

credentials generated during binding without policy

a915ab9

boyang9527 reviewed Aug 22, 2018

View reviewed changes

Rechecking database for invalid credential cache entry

1ddc494

boyang9527 approved these changes Aug 24, 2018

View reviewed changes

boyang9527 requested a review from cdlliuy August 24, 2018 22:48

qibobo suggested changes Aug 27, 2018

View reviewed changes

Incorporated Review comments

a5bc4f0

paltanmoy force-pushed the custom-metrics-servicebroker-api branch from 74b5e65 to a5bc4f0 Compare August 29, 2018 05:06

qibobo approved these changes Aug 29, 2018

View reviewed changes

qibobo merged commit c5c5ec3 into cloudfoundry:develop Aug 29, 2018

cf-gitbot removed the unscheduled label Aug 29, 2018

paltanmoy deleted the custom-metrics-servicebroker-api branch January 4, 2019 06:21

cf-gitbot added the delivered label Jan 5, 2019

cf-gitbot added accepted and removed delivered labels Jan 5, 2019

Added custom metrics binding credential generation and management in APIServer #376

Added custom metrics binding credential generation and management in APIServer #376

Conversation

paltanmoy commented Aug 13, 2018

cfdreddbot commented Aug 13, 2018

cf-gitbot commented Aug 13, 2018

cdlliuy commented Aug 16, 2018

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

boyang9527 Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boyang9527 commented Aug 24, 2018

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

qibobo commented Aug 29, 2018

boyang9527 Aug 22, 2018 •

edited

Loading