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.

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

Fixes source token expiration by token refresh #5198

Merged
merged 10 commits into from Nov 4, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -3,3 +3,4 @@
- Changes `superstatic` dependency to `v8`, addressing Hosting emulator issues on Windows.
- Fixes internal library that was not being correctly published.
- Adds `--disable-triggers` flag to RTDB write commands.
- Fixes source token expiration issue by refreshing source token.
7 changes: 5 additions & 2 deletions src/deploy/functions/release/fabricator.ts
Expand Up @@ -210,9 +210,11 @@ export class Fabricator {
if (apiFunction.httpsTrigger) {
apiFunction.httpsTrigger.securityLevel = "SECURE_ALWAYS";
}
apiFunction.sourceToken = await scraper.tokenPromise();
// apiFunction.sourceToken = await scraper.tokenPromise();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - stray comment?

const resultFunction = await this.functionExecutor
.run(async () => {
// try to get the source token right before deploying
apiFunction.sourceToken = await scraper.getToken();
const op: { name: string } = await gcf.createFunction(apiFunction);
return poller.pollOperation<gcf.CloudFunction>({
...gcfV1PollerOptions,
Expand Down Expand Up @@ -374,9 +376,10 @@ export class Fabricator {
throw new Error("Precondition failed");
}
const apiFunction = gcf.functionFromEndpoint(endpoint, sourceUrl);
apiFunction.sourceToken = await scraper.tokenPromise();

const resultFunction = await this.functionExecutor
.run(async () => {
apiFunction.sourceToken = await scraper.getToken();
const op: { name: string } = await gcf.updateFunction(apiFunction);
return await poller.pollOperation<gcf.CloudFunction>({
...gcfV1PollerOptions,
Expand Down
51 changes: 40 additions & 11 deletions src/deploy/functions/release/sourceTokenScraper.ts
@@ -1,28 +1,55 @@
import { logger } from "../../../logger";

enum TokenState {
"NONE",
"VALID",
"INVALID",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

tangent: enums are controversial in TS for some reason.

https://blog.logrocket.com/why-typescript-enums-suck/

Internally, I think we prefer string literals:

type TokenState = "none" | "valid" | "invalid"


/**
* GCF v1 deploys support reusing a build between function deploys.
* This class will return a resolved promise for its first call to tokenPromise()
* and then will always return a promise that is resolved by the poller function.
*/
export class SourceTokenScraper {
private firstCall = true;
private resolve!: (token: string) => void;
private tokenValidDurationMs; // in ms
private resolve!: (token?: string) => void;
private promise: Promise<string | undefined>;
private expiration: bigint | undefined;
private tokenState: TokenState;

constructor() {
constructor(validDurationMs = 1500000) {
this.tokenValidDurationMs = validDurationMs;
this.promise = new Promise((resolve) => (this.resolve = resolve));
this.tokenState = TokenState.NONE;
}

async getToken(): Promise<string | undefined> {
if (this.tokenState === TokenState.NONE) {
this.tokenState = TokenState.INVALID;
return undefined;
} else if (this.tokenState === TokenState.INVALID) {
Copy link
Contributor

@taeold taeold Nov 1, 2022

Choose a reason for hiding this comment

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

I found that token states as described here bit confusing. tokenState == INVALID makes me think that token is invalid, but token doesn't actually exist at this point.

What about another name that closely maps to the state of the scraper, like tokenFetchState instead of tokenState and FETCHING instead of INVALID?

return this.promise; // wait until we get a source token
} else if (this.tokenState === TokenState.VALID) {
if (this.checkTokenExpired()) {
this.tokenState = TokenState.INVALID;
this.promise = new Promise((resolve) => (this.resolve = resolve));
return undefined;
}
return this.promise;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw an error here instead of silently succeeding? Can see argument for either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By throwing an error, do you mean throwing an error and then catching it in an enclosing try { await getToken(); ... } catch (expiredErr) {...} block, and then retrying?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that in theory we should never reach this codepath - fetchState can only be in one of three states, and we have if condition for all of them. So we could throw (which could happen if we misunderstood this code for somee reason) or just return the promise (which might leave the CLI in an undefined state). I have slight preference for the former.

}
}

// Token Promise will return undefined for the first caller
// (because we presume it's this function's source token we'll scrape)
// and then returns the promise generated from the first function's onCall
tokenPromise(): Promise<string | undefined> {
if (this.firstCall) {
this.firstCall = false;
return Promise.resolve(undefined);
checkTokenExpired(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit* I usually name my boolean function with "is" like isTokenExpired

if (this.expiration === undefined) {
throw new Error();
Copy link
Contributor

Choose a reason for hiding this comment

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

This error is user visible - we should try our best to describe the situation and also call out what users should do when they see the issue.

}
return this.promise;
return process.hrtime.bigint() >= this.expiration;
}

calculateTokenExpiry(): bigint {
const now = process.hrtime.bigint();
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think using hrtime with its bigint silliness is unnecessarily precise, especially if the time range we care about is in MINUTES not MICROSECONDS.

I'd suggest we stick to Date.now() and forget about big ints.

return now + BigInt(this.tokenValidDurationMs) * BigInt(1e6);
}

get poller() {
Expand All @@ -32,6 +59,8 @@ export class SourceTokenScraper {
op.metadata?.target?.split("/") || [];
logger.debug(`Got source token ${op.metadata?.sourceToken} for region ${region as string}`);
this.resolve(op.metadata?.sourceToken);
this.tokenState = TokenState.VALID;
this.expiration = this.calculateTokenExpiry(); // calculate expiration
}
};
}
Expand Down
26 changes: 21 additions & 5 deletions src/deploy/functions/release/timer.ts
@@ -1,14 +1,30 @@
/** Measures the time taken from construction to the call to stop() */
export class Timer {
private readonly start: bigint;
private readonly start: bigint; // nanoseconds
taeold marked this conversation as resolved.
Show resolved Hide resolved
private readonly expiry?: bigint; // nanoseconds

constructor() {
/**
* Constructor for Timer object.
* @param expirationSeconds timer expiration in minutes
*/
constructor(expirationMs?: number) {
this.start = process.hrtime.bigint();
if (expirationMs !== undefined) {
this.expiry = BigInt(expirationMs) * BigInt(1e6);
}
}

stop(): number {
expired(): boolean {
const elapsedNanos = this.elapsedNanos();
return this.expiry !== undefined && elapsedNanos >= this.expiry;
}

elapsedNanos(): bigint {
const stop = process.hrtime.bigint();
const elapsedNanos = stop - this.start;
return Number(elapsedNanos / BigInt(1e6));
return stop - this.start;
}

stop(): number {
return Number(this.elapsedNanos() / BigInt(1e6));
}
}
68 changes: 59 additions & 9 deletions src/test/deploy/functions/release/sourceTokenScraper.spec.ts
Expand Up @@ -2,23 +2,27 @@ import { expect } from "chai";

import { SourceTokenScraper } from "../../../../deploy/functions/release/sourceTokenScraper";

describe("SourcTokenScraper", () => {
it("immediately provides the first result", async () => {
describe("SourceTokenScraper", () => {
const timeout = (duration: number): Promise<void> => {
return new Promise<void>((resolve) => setTimeout(resolve, duration));
};

it.only("immediately provides the first result", async () => {
const scraper = new SourceTokenScraper();
await expect(scraper.tokenPromise()).to.eventually.be.undefined;
await expect(scraper.getToken()).to.eventually.be.undefined;
});

it("provides results after the firt operation completes", async () => {
it("provides results after the first operation completes", async () => {
const scraper = new SourceTokenScraper();
// First result comes right away;
await expect(scraper.tokenPromise()).to.eventually.be.undefined;
await expect(scraper.getToken()).to.eventually.be.undefined;

let gotResult = false;
const timeout = new Promise((resolve, reject) => {
setTimeout(() => reject(new Error("Timeout")), 10);
});
const getResult = (async () => {
await scraper.tokenPromise();
await scraper.getToken();
gotResult = true;
})();
await expect(Promise.race([getResult, timeout])).to.be.rejectedWith("Timeout");
Expand All @@ -28,17 +32,63 @@ describe("SourcTokenScraper", () => {
await expect(getResult).to.eventually.be.undefined;
});

it("provides tokens from an operation", async () => {
it.only("provides tokens from an operation", async () => {
const scraper = new SourceTokenScraper();
// First result comes right away
await expect(scraper.tokenPromise()).to.eventually.be.undefined;
await expect(scraper.getToken()).to.eventually.be.undefined;

scraper.poller({
metadata: {
sourceToken: "magic token",
target: "projects/p/locations/l/functions/f",
},
});
await expect(scraper.tokenPromise()).to.eventually.equal("magic token");
await expect(scraper.getToken()).to.eventually.equal("magic token");
});

it.only("refreshes token after timer expires", async () => {
const scraper = new SourceTokenScraper(10);
await expect(scraper.getToken()).to.eventually.be.undefined;
scraper.poller({
metadata: {
sourceToken: "magic token",
target: "projects/p/locations/l/functions/f",
},
});
await expect(scraper.getToken()).to.eventually.equal("magic token");
await timeout(50);
Copy link
Member

Choose a reason for hiding this comment

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

In the future, try to instrument code without sleeping in your test. You can inject a clock for example.

await expect(scraper.getToken()).to.eventually.be.undefined;
scraper.poller({
metadata: {
sourceToken: "magic token #2",
target: "projects/p/locations/l/functions/f",
},
});
await expect(scraper.getToken()).to.eventually.equal("magic token #2");
});

it.only("concurrent requests for source token", async () => {
const scraper = new SourceTokenScraper();

const promises = [];
for (let i = 0; i < 3; i++) {
promises.push(scraper.getToken());
}
scraper.poller({
metadata: {
sourceToken: "magic token",
target: "projects/p/locations/l/functions/f",
},
});

let successes = 0;
const tokens = await Promise.all(promises);
for (const tok of tokens) {
if (tok === "magic token") {
successes++;
}
}
expect(tokens.includes(undefined)).to.be.true;
expect(successes).to.equal(2);
});
});