Skip to content

Commit

Permalink
fix: copy credentials in internal config (#1052)
Browse files Browse the repository at this point in the history
  • Loading branch information
kjin authored Jun 26, 2019
1 parent 178c2a9 commit 8930df3
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 21 deletions.
16 changes: 12 additions & 4 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,6 @@ function initConfig(userConfig: Forceable<Config>): TopLevelConfig {
},
writerConfig: {
[FORCE_NEW]: forceNew,
projectId: lastOf<string | undefined>(
mergedConfig.projectId,
process.env.GCLOUD_PROJECT
),
onUncaughtException: mergedConfig.onUncaughtException,
bufferSize: mergedConfig.bufferSize,
flushDelaySeconds: mergedConfig.flushDelaySeconds,
Expand All @@ -153,6 +149,18 @@ function initConfig(userConfig: Forceable<Config>): TopLevelConfig {
process.env.GAE_MINOR_VERSION
),
},
/**
* Our TypeScript interface suggests that only credentials, keyFilename,
* and projectId are accepted, but by passing the entire object to the
* Trace Writer, we can allow users to supply other fields that are
* publicly supported by the Google Auth Library.
*/
authOptions: Object.assign({}, mergedConfig, {
projectId: lastOf<string | undefined>(
mergedConfig.projectId,
process.env.GCLOUD_PROJECT
),
}),
},
pluginLoaderConfig: {
[FORCE_NEW]: forceNew,
Expand Down
6 changes: 3 additions & 3 deletions src/trace-writer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ const SCOPES: string[] = ['https://www.googleapis.com/auth/trace.append'];
/* The API endpoint of the Stackdriver Trace service */
const TRACE_API_ENDPOINT = 'cloudtrace.googleapis.com';

export interface TraceWriterConfig extends common.GoogleAuthOptions {
projectId?: string;
export interface TraceWriterConfig {
authOptions: common.GoogleAuthOptions;
onUncaughtException: string;
bufferSize: number;
flushDelaySeconds: number;
Expand Down Expand Up @@ -124,7 +124,7 @@ export class TraceWriter extends common.Service {
baseUrl: `https://${TRACE_API_ENDPOINT}/v1`,
scopes: SCOPES,
},
config
config.authOptions
);

this.logger = logger;
Expand Down
4 changes: 2 additions & 2 deletions src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ export class Tracing implements Component {
.activate();

if (
typeof this.config.writerConfig.projectId !== 'string' &&
typeof this.config.writerConfig.projectId !== 'undefined'
typeof this.config.writerConfig.authOptions.projectId !== 'string' &&
typeof this.config.writerConfig.authOptions.projectId !== 'undefined'
) {
this.logger.error(
'StackdriverTracer#start: config.projectId, if provided, must be a string.',
Expand Down
4 changes: 2 additions & 2 deletions test/test-config-priority.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,13 @@ describe('should respect config load order', () => {
it('should respect GCLOUD_PROJECT', () => {
traceTestModule.start();
const config = getCapturedConfig();
assert.strictEqual(config.writerConfig.projectId, '1729');
assert.strictEqual(config.writerConfig.authOptions.projectId, '1729');
});

it('should prefer env to config', () => {
traceTestModule.start({ projectId: '1927' });
const config = getCapturedConfig();
assert.strictEqual(config.writerConfig.projectId, '1729');
assert.strictEqual(config.writerConfig.authOptions.projectId, '1729');
});
});
});
36 changes: 36 additions & 0 deletions test/test-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import * as testTraceModule from './trace';
import { TopLevelConfig } from '../src/tracing';
import { StackdriverTracer } from '../src/trace-api';
import {Logger} from '../src/logger';
import { TraceWriterConfig } from '../src/trace-writer';

describe('Behavior set by config for CLS', () => {
const useAH = semver.satisfies(process.version, '>=8');
Expand Down Expand Up @@ -164,3 +165,38 @@ describe('Behavior set by config for TracePolicy', () => {
});
});

describe('Behavior set by config for TraceWriter', () => {
let capturedConfig: TraceWriterConfig|null;

class CaptureConfigTestWriter extends testTraceModule.TestTraceWriter {
constructor(config: TraceWriterConfig, logger: Logger) {
super(config, logger);
// Capture the config object passed into this constructor.
capturedConfig = config;
}
}

beforeEach(() => {
capturedConfig = null;
});

before(() => {
testTraceModule.setTraceWriterForTest(CaptureConfigTestWriter);
});

after(() => {
testTraceModule.setTraceWriterForTest(testTraceModule.TestTraceWriter);
});

it('should set auth variables passed to TraceWriter as authOptions', () => {
const credentials = { private_key: 'abc' };
testTraceModule.start({
keyFilename: 'a',
credentials
});
assert.ok(capturedConfig);
assert.strictEqual(capturedConfig!.authOptions.keyFilename, 'a');
assert.deepStrictEqual(capturedConfig!.authOptions.credentials, credentials);
});
});

2 changes: 1 addition & 1 deletion test/test-trace-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('Trace Interface', () => {
return traceWriter
.create(
Object.assign(
{ [FORCE_NEW]: true, projectId: 'project-1' },
{ [FORCE_NEW]: true, authOptions: { projectId: 'project-1' } },
defaultConfig
),
logger
Expand Down
25 changes: 17 additions & 8 deletions test/test-trace-writer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ function createDummyTrace(numSpans: number): Trace {
describe('Trace Writer', () => {
const pjson = require('../../package.json');
const DEFAULT_CONFIG: TraceWriterConfig = {
authOptions: {},
onUncaughtException: 'ignore',
bufferSize: Infinity,
flushDelaySeconds: 3600,
Expand Down Expand Up @@ -174,17 +175,21 @@ describe('Trace Writer', () => {
it('should use the keyFilename field of the config object', async () => {
const expectedCredentials: TestCredentials = require('./fixtures/gcloud-credentials.json');
const actualCredentials = await captureCredentialsForConfig({
projectId: 'my-project',
keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json'),
authOptions: {
projectId: 'my-project',
keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json'),
},
});
assert.deepStrictEqual(actualCredentials, expectedCredentials);
});

it('should use the credentials field of the config object', async () => {
const expectedCredentials: TestCredentials = require('./fixtures/gcloud-credentials.json');
const actualCredentials = await captureCredentialsForConfig({
projectId: 'my-project',
credentials: expectedCredentials,
authOptions: {
projectId: 'my-project',
credentials: expectedCredentials,
},
});
assert.deepStrictEqual(actualCredentials, expectedCredentials);
});
Expand All @@ -197,9 +202,11 @@ describe('Trace Writer', () => {
type: 'authorized_user',
};
const actualCredentials = await captureCredentialsForConfig({
projectId: 'my-project',
keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json'),
credentials: expectedCredentials,
authOptions: {
projectId: 'my-project',
keyFilename: path.join('test', 'fixtures', 'gcloud-credentials.json'),
credentials: expectedCredentials,
},
});
assert.deepStrictEqual(actualCredentials, expectedCredentials);
});
Expand All @@ -216,7 +223,9 @@ describe('Trace Writer', () => {

it(`doesn't call Service#getProjectId if project ID is passed`, async () => {
const writer = new TraceWriter(
Object.assign({ projectId: 'my-project' }, DEFAULT_CONFIG),
Object.assign({}, DEFAULT_CONFIG, {
authOptions: { projectId: 'my-project' },
}),
logger
);
getProjectIdOverride = () => Promise.resolve('my-different-project');
Expand Down
2 changes: 1 addition & 1 deletion test/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export class TestLogger extends testLogger.TestLogger {
export class TestTraceWriter extends TraceWriter {
constructor(config: TraceWriterConfig, logger: logger.Logger) {
super(config, logger);
this.getConfig().projectId = '0';
this.getConfig().authOptions.projectId = '0';
}

async initialize(): Promise<void> {}
Expand Down

0 comments on commit 8930df3

Please sign in to comment.