Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: changes to node-sdk-core to work with service factory feature #72

Merged
merged 10 commits into from
Nov 19, 2019
Merged

feat: changes to node-sdk-core to work with service factory feature #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0576380
feat: changes to node-sdk-core to work with service factory feature
Nov 14, 2019
5d907b9
updated test to reflect base service constructor does not call config…
Nov 14, 2019
f0030cb
fixed linter issues
Nov 14, 2019
e770cc7
added test for getting credentials from vcap
Nov 15, 2019
6d5bebb
linted test
Nov 15, 2019
8f399ce
changed function name and wrote better test descriptions
Nov 15, 2019
c068ba7
removed name and serviceVersion because they are not referenced anymore
Nov 15, 2019
afefb80
changed import to import from build
Nov 15, 2019
940ecde
added comment for vcap parsing function. removed vcap_services depend…
Nov 18, 2019
17503f9
added debug messages for when returning empty credential
Nov 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 46 additions & 4 deletions auth/utils/read-external-sources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import camelcase = require('camelcase');
import isEmpty = require('lodash.isempty');
import vcapServices = require('vcap_services');
dpopp07 marked this conversation as resolved.
Show resolved Hide resolved
import logger from '../../lib/logger';
import { readCredentialsFile } from './read-credentials-file';

/**
Expand Down Expand Up @@ -52,7 +52,7 @@ function getProperties(serviceName: string): any {
}

if (isEmpty(properties)) {
properties = getCredentialsFromCloud(serviceName);
properties = getPropertiesFromVCAP(serviceName);
}

return properties;
Expand Down Expand Up @@ -96,9 +96,51 @@ function filterPropertiesByServiceName(envObj: any, serviceName: string): any {
/**
* Pulls credentials from VCAP_SERVICES env property that IBM Cloud sets
*
* The function will first look for a service entry whose "name" field matches
* the serviceKey value. If found, return its credentials.
*
* If no match against the service entry's "name" field is found, then find the
* service list with a key matching the serviceKey value. If found, return the
* credentials of the first service in the service list.
*/
function getCredentialsFromCloud(serviceName: string): any {
const credentials = vcapServices.getCredentials(serviceName);
function getVCAPCredentialsForService(name) {
dpopp07 marked this conversation as resolved.
Show resolved Hide resolved
if (process.env.VCAP_SERVICES) {
const services = JSON.parse(process.env.VCAP_SERVICES);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be safer to wrap the JSON parse in a try/catch since it throws an error if anything about the JSON is malformatted. Not that it should be, since that env variable is generated by the cloud. So this is an optional change, up to you.

Suggested change
const services = JSON.parse(process.env.VCAP_SERVICES);
let services;
try {
services = JSON.parse(process.env.VCAP_SERVICES);
} catch () {
return {};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping the JSON parse in a try/catch might eat up/silence the error, and the user wouldn't know their VCAP_SERVICES env is not parsed. So I think it's fine to let it fail if for some reason JSON is malformatted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's a fair point. I just remembered that the core has it's own logger now. So another thing to consider would be printing an error log for a malformatted VCAP variable so the user knows about it. My only concern here is the code crashing even though the user typically cannot control the contents of VCAP_SERVICES.

for (const serviceName of Object.keys(services)) {
dpopp07 marked this conversation as resolved.
Show resolved Hide resolved
for (const instance of services[serviceName]) {
if (instance['name'] === name) {
if (instance.hasOwnProperty('credentials')) {
return instance.credentials
} else {
logger.debug('no data read from VCAP_SERVICES')
return {}
}
}
}
}
for (const serviceName of Object.keys(services)) {
if (serviceName === name) {
if (services[serviceName].length > 0) {
if (services[serviceName][0].hasOwnProperty('credentials')) {
return services[serviceName][0].credentials
} else {
logger.debug('no data read from VCAP_SERVICES')
return {}
}
return services[serviceName][0].credentials || {};
} else {
logger.debug('no data read from VCAP_SERVICES')
return {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of the logger, I think it's a good idea to add debug or verbose logging statements before an empty object is returned saying something like "no data read from vcap services"

}
}
}
}
logger.debug('no data read from VCAP_SERVICES')
return {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more place to add a debug

}

function getPropertiesFromVCAP(serviceName: string): any {
const credentials = getVCAPCredentialsForService(serviceName);
// infer authentication type from credentials in a simple manner
// iam is used as the default later
if (credentials.username || credentials.password) {
Expand Down
5 changes: 0 additions & 5 deletions lib/base-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@ export interface BaseServiceOptions extends UserOptions {
*/
export class BaseService {
static URL: string;
name: string;
serviceVersion: string;
protected baseOptions: BaseServiceOptions;
private authenticator: AuthenticatorInterface;
private requestWrapperInstance;
Expand Down Expand Up @@ -123,9 +121,6 @@ export class BaseService {
}

this.authenticator = options.authenticator;

// temp: call the configureService method to ensure compatibility
this.configureService(this.name);
dpopp07 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
Loading