-
Notifications
You must be signed in to change notification settings - Fork 62
Added custom metrics binding credential generation and management in APIServer #376
Changes from 7 commits
d40b644
c681837
c4015fa
bfdb593
9332fbd
a915ab9
1ddc494
a5bc4f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
'use strict'; | ||
|
||
module.exports = function(sequelize, DataTypes) { | ||
var CustomMetricsCredentials = sequelize.define('credentials', { | ||
id: { | ||
type: DataTypes.STRING, | ||
field: 'id', | ||
primaryKey: true | ||
}, | ||
username: { | ||
type: DataTypes.STRING, | ||
field: 'username', | ||
allowNull: false | ||
}, | ||
password: { | ||
type: DataTypes.STRING, | ||
field: 'password', | ||
allowNull: false | ||
} | ||
},{ | ||
freezeTableName: true, | ||
timestamps: true, | ||
createdAt: false, | ||
updatedAt: 'updated_at' | ||
}); | ||
return CustomMetricsCredentials; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,196 @@ | ||
'use strict'; | ||
module.exports = function(models, credentialCache, cacheTTL) { | ||
var logger = require('../log/logger'); | ||
var HttpStatus = require('http-status-codes'); | ||
var uuidv4 = require('uuid/v4'); | ||
var bcrypt = require('bcrypt-nodejs'); | ||
var credhelper = {}; | ||
|
||
function generateHash(input) { | ||
return bcrypt.hashSync(input, bcrypt.genSaltSync(8)); | ||
} | ||
|
||
function validateHash(input, hash) { | ||
return bcrypt.compareSync(input, hash); | ||
} | ||
|
||
function validateCredentialDetails(username,usernamehash,password, passwordhash){ | ||
var isUsernameValid = validateHash(username, usernamehash); | ||
var isPasswordValid = validateHash(password, passwordhash); | ||
var isValidCred = isUsernameValid && isPasswordValid; | ||
return isValidCred; | ||
} | ||
|
||
credhelper.createOrUpdateCredentials = function(req, callback) { | ||
var username = uuidv4(); | ||
var password = uuidv4(); | ||
var appId = req.params.app_id; | ||
models.credentials.upsert({ | ||
id: req.params.app_id, | ||
username: generateHash(username), | ||
password: generateHash(password) | ||
}).then(function(createdData) { | ||
if (createdData) { | ||
logger.info('New credentials hasbeen generated successfully', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if updatedData, cache entry needs to be invalidated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hasbeen -> has been? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
'app_id': appId | ||
}); | ||
callback(null, { | ||
'statusCode': HttpStatus.CREATED, | ||
'username': username, | ||
'password': password | ||
}); | ||
} | ||
else { | ||
logger.info('Existing credentials hasbeen updated successfully', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hasbeen -> has been? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
'app_id': appId | ||
}); | ||
var deleted = credentialCache.del(appId); | ||
if (deleted != 1) { | ||
logger.info('Cache invalidation failed', { | ||
'app_id': appId | ||
}); | ||
} | ||
callback(null, { | ||
'statusCode': HttpStatus.OK, | ||
'username': username, | ||
'password': password | ||
}); | ||
} | ||
}).catch(function(error) { | ||
logger.error('Failed to create custom metrics credentials', { | ||
'app_id': appId, | ||
'error': error | ||
}); | ||
error.statusCode = HttpStatus.INTERNAL_SERVER_ERROR; | ||
callback(error); | ||
}); | ||
} | ||
|
||
credhelper.deleteCredentials = function(req, callback) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete credential will need to invalidate the entry in cache There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cache invalidation added. |
||
var appId = req.params.app_id; | ||
models.credentials.destroy({ | ||
where: { | ||
id: appId | ||
} | ||
}).then(function(result) { | ||
if (result > 0) { | ||
logger.info('Successfully deleted the custom metrics credentials for application', { | ||
'app id': appId | ||
}); | ||
var deleted = credentialCache.del(appId); | ||
if (deleted != 1) { | ||
logger.info('Cache invalidation failed', { | ||
'app_id': appId | ||
}); | ||
} | ||
callback(null, { | ||
'statusCode': HttpStatus.OK | ||
}); | ||
} | ||
else { | ||
var error = { | ||
message: 'No custom metrics credentials exists with application', | ||
statusCode: HttpStatus.NOT_FOUND | ||
} | ||
logger.error('No custom metrics credentials exists with application', { | ||
'app id': appId, | ||
error: error | ||
}); | ||
callback(error); | ||
} | ||
}).catch(function(error) { | ||
logger.error('Internal Error while deleting custom metrics credentials', { | ||
'app id': appId, | ||
'error': error | ||
}); | ||
error.statusCode = HttpStatus.INTERNAL_SERVER_ERROR; | ||
callback(error); | ||
}); | ||
}; | ||
|
||
credhelper.validateCredentials = function(req, callback) { | ||
var appId = req.params.app_id; | ||
var username = req.query["username"]; | ||
var password = req.query["password"]; | ||
var creds,isValidCred,cachedCred; | ||
if (!username || !password) { | ||
var insufficientQueryparamError = new Error(); | ||
insufficientQueryparamError.statusCode = HttpStatus.INTERNAL_SERVER_ERROR; | ||
insufficientQueryparamError.message = 'insufficient query parameters'; | ||
logger.error('Failed to validate custom metrics credentials due to insufficient query parameters', { | ||
'app_id': appId, | ||
'error': insufficientQueryparamError | ||
}); | ||
callback(insufficientQueryparamError); | ||
return; | ||
} | ||
// Try to find credentials in cache | ||
try{ | ||
creds = credentialCache.get(appId, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it better to get data from database directly when there is not data in cache rather than throwing an error and catching it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As getting creds directly from database is being already handled in catch block ..tried handling in the same way. |
||
isValidCred = validateCredentialDetails(username, creds.username, password, creds.password); | ||
// If cache contains old or invalid credentials | ||
if (!isValidCred){ | ||
logger.info('Credentials not valid', { | ||
'app_id': appId, | ||
'isValid': isValidCred | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it is not valid, we may need to check with database... and refresh cache. |
||
}); | ||
throw new Error('Invalid or old credentials found in cache'); | ||
} | ||
else { | ||
logger.info('valid credentials hasbeen found successfully in cache', { | ||
'app_id': appId, | ||
'isValid': isValidCred | ||
}); | ||
callback(null, { | ||
'statusCode': HttpStatus.OK, | ||
'isValid': isValidCred, | ||
}); | ||
} | ||
} | ||
catch(err){ | ||
// Did not find credentials in cache, lets find in database. | ||
models.credentials.find({ | ||
where: { | ||
id: appId | ||
} | ||
}).then(function(creds) { | ||
if (!creds) { | ||
var error = { | ||
message: 'No credentials found', | ||
statusCode: HttpStatus.NOT_FOUND | ||
} | ||
logger.info('No credentials found', { | ||
'app_id': appId, | ||
'error': error | ||
}); | ||
callback(error); | ||
} | ||
else { | ||
isValidCred = validateCredentialDetails(username, creds.username, password, creds.password); | ||
logger.info('Credentials hasbeen found successfully in database', { | ||
'app_id': appId, | ||
'isValid': isValidCred | ||
}); | ||
cachedCred = { | ||
'username': creds.username, | ||
'password': creds.password | ||
}; | ||
var isCached = credentialCache.set(appId, cachedCred, cacheTTL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason the cred is not cached? if it is not cached, it is not good to have log message saying "credential cached" |
||
logger.info('Credential cached',{ 'app_id':appId, 'isCached':isCached }); | ||
callback(null, { | ||
'statusCode': HttpStatus.OK, | ||
'isValid': isValidCred | ||
}); | ||
} | ||
}).catch(function(err) { | ||
logger.error('Failed to validate custom metrics credentials', { | ||
'app_id': appId, | ||
'error': err | ||
}); | ||
err.statusCode = HttpStatus.INTERNAL_SERVER_ERROR; | ||
callback(err); | ||
}); | ||
} | ||
} | ||
return credhelper; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
'use strict'; | ||
module.exports = function(models, credentialCache, cacheTTL) { | ||
|
||
var express = require('express'); | ||
var router = express.Router(); | ||
var logger = require('../log/logger'); | ||
var credHelper = require('./credentialHelper')(models, credentialCache, cacheTTL); | ||
|
||
router.post('/:app_id/credentials', function(req, resp) { | ||
var appId = req.params.app_id; | ||
logger.info('Request for credentials creation received', { | ||
'app_id': appId | ||
}); | ||
credHelper.createOrUpdateCredentials(req, function(err, result) { | ||
var responseBody = {}; | ||
var statusCode; | ||
if (err) { | ||
statusCode = err.statusCode; | ||
responseBody = { | ||
'error': err.message | ||
}; | ||
} | ||
else { | ||
statusCode = result.statusCode; | ||
responseBody = { | ||
'username': result.username, | ||
'password': result.password | ||
} | ||
} | ||
resp.status(statusCode).json(responseBody) | ||
}); | ||
}); | ||
|
||
router.delete('/:app_id/credentials', function(req, res) { | ||
logger.info('Request for credentials deletion received', { | ||
'app_id': req.params.app_id | ||
}); | ||
credHelper.deleteCredentials(req, function(err, result) { | ||
var responseBody = {}; | ||
var statusCode; | ||
if (err) { | ||
statusCode = err.statusCode; | ||
responseBody = { | ||
'error': err.message | ||
}; | ||
} | ||
else { | ||
statusCode = result.statusCode; | ||
} | ||
res.status(statusCode).json(responseBody); | ||
}); | ||
}); | ||
|
||
router.post('/:app_id/credentials/validate', function(req, resp) { | ||
var appId = req.params.app_id; | ||
logger.info('Request for credential validation received', { | ||
'app_id': appId | ||
}); | ||
credHelper.validateCredentials(req, function(err, result) { | ||
var responseBody = {}; | ||
var statusCode; | ||
if (err) { | ||
statusCode = err.statusCode; | ||
responseBody = { | ||
'error': err.message | ||
}; | ||
} | ||
else { | ||
statusCode = result.statusCode; | ||
responseBody = { | ||
'isValid': result.isValid | ||
} | ||
} | ||
resp.status(statusCode).json(responseBody) | ||
}); | ||
}); | ||
return router; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,12 +88,11 @@ var getPolicySchema = function() { | |
var getScalingRuleSchema = function() { | ||
var validOperators = getValidOperators(); | ||
var adjustmentPattern = getAdjustmentPattern(); | ||
var metricTypeEnum = getMetricTypes(); | ||
var schema = { | ||
'type': 'object', | ||
'id':'/scaling_rules', | ||
'properties' : { | ||
'metric_type':{ 'type':'string' ,'enum':metricTypeEnum }, | ||
'metric_type':{ 'type':'string' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
'breach_duration_secs':{ 'type':'number','minimum': 60,'maximum': 3600 }, | ||
'threshold':{ 'type':'number'}, | ||
'operator':{ 'type':'string','enum': validOperators }, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use appId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which one? id => appId or app_id => appId ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var appId = req.params.app_id;
appId has been defined, so better to " id: appId" ?