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

Add cache issuer directory #15

Merged
merged 6 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions src/bindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { R2Bucket } from '@cloudflare/workers-types/2023-07-01';

export interface Bindings {
// variables and secrets
DIRECTORY_CACHE_MAX_AGE_SECONDS: string;
ENVIRONMENT: string;
LOGGING_SHIM_TOKEN: string;
SENTRY_ACCESS_CLIENT_ID: string;
Expand Down
22 changes: 14 additions & 8 deletions src/context/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,33 @@ export class MetricsRegistry {
registry: RegistryType;
options: RegistryOptions;

requestsTotal: CounterType;
directoryCacheMissTotal: CounterType;
erroredRequestsTotal: CounterType;
issuanceRequestTotal: CounterType;
keyRotationTotal: CounterType;
keyClearTotal: CounterType;
issuanceRequestTotal: CounterType;
requestsTotal: CounterType;
signedTokenTotal: CounterType;

constructor(options: RegistryOptions) {
this.options = options;
this.registry = new Registry();

this.requestsTotal = this.registry.create('counter', 'requests_total', 'total requests');
this.directoryCacheMissTotal = this.registry.create(
'counter',
'directory_cache_miss_total',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the metric counts cache miss. It is also possible to devise a metric directory_cache_request_total{status="hit|miss"}. However, I feel this would add uneccessary weight.

'Number of requests for private token issuer directory which are not served by the cache.'
);
this.erroredRequestsTotal = this.registry.create(
'counter',
'errored_requests_total',
'Errored requests served to eyeball'
);
this.issuanceRequestTotal = this.registry.create(
'counter',
'issuance_request_total',
'Number of requests for private token issuance.'
Copy link
Contributor

Choose a reason for hiding this comment

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

successful?

Suggested change
'Number of requests for private token issuance.'
'Number of successful requests for private token issuance.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not necessary successful. the metric is increased at the start of handleTokenRequest, no matter the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

side: this is also not a new metrics. The metrics have been sorted by alphabetical orders, and it's part of this change.

);
this.keyRotationTotal = this.registry.create(
'counter',
'key_rotation_total',
Expand All @@ -40,11 +50,7 @@ export class MetricsRegistry {
'key_clear_total',
'Number of key clear performed.'
);
this.issuanceRequestTotal = this.registry.create(
'counter',
'issuance_request_total',
'Number of requests for private token issuance.'
);
this.requestsTotal = this.registry.create('counter', 'requests_total', 'total requests');
this.signedTokenTotal = this.registry.create(
'counter',
'signed_token_total',
Expand Down
36 changes: 34 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,24 @@ export const handleTokenRequest = async (ctx: Context, request: Request) => {
});
};

const getDirectoryCache = async (): Promise<Cache> => {
return caches.open('response/issuer-directory');
};

const FAKE_DOMAIN_CACHE = 'cache.local';
const DIRECTORY_CACHE_REQUEST = new Request(
`https://${FAKE_DOMAIN_CACHE}${PRIVATE_TOKEN_ISSUER_DIRECTORY}`
);

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const handleTokenDirectory = async (ctx: Context, request?: Request) => {
const cache = await getDirectoryCache();
const cachedResponse = await cache.match(DIRECTORY_CACHE_REQUEST);
if (cachedResponse) {
return cachedResponse;
}
ctx.metrics.directoryCacheMissTotal.inc({ env: ctx.env.ENVIRONMENT });

const keys = await ctx.env.ISSUANCE_KEYS.list({ include: ['customMetadata'] });

if (keys.objects.length === 0) {
Expand All @@ -95,14 +112,23 @@ export const handleTokenDirectory = async (ctx: Context, request?: Request) => {
})),
};

return new Response(JSON.stringify(directory), {
const response = new Response(JSON.stringify(directory), {
headers: {
'content-type': MediaType.PRIVATE_TOKEN_ISSUER_DIRECTORY,
'cache-control': 'public, max-age=86400',
'cache-control': `public, max-age=${ctx.env.DIRECTORY_CACHE_MAX_AGE_SECONDS}`,
},
});
ctx.waitUntil(cache.put(DIRECTORY_CACHE_REQUEST, response.clone()));

return response;
};

const clearDirectoryCache = async (): Promise<boolean> => {
const cache = await getDirectoryCache();
return cache.delete(DIRECTORY_CACHE_REQUEST);
};

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const handleRotateKey = async (ctx: Context, request?: Request) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the linter exception and prepend variable's name with _.

Suggested change
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const handleRotateKey = async (ctx: Context, request?: Request) => {
export const handleRotateKey = async (ctx: Context, _request?: Request) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rule triggered a warning in both case. I've added a rule configuration to ignore ^_, which _request is.

ctx.metrics.keyRotationTotal.inc({ env: ctx.env.ENVIRONMENT });

Expand Down Expand Up @@ -148,9 +174,12 @@ export const handleRotateKey = async (ctx: Context, request?: Request) => {
customMetadata: metadata,
});

ctx.waitUntil(clearDirectoryCache());

return new Response(`New key ${publicKeyEnc}`, { status: 201 });
};

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const handleClearKey = async (ctx: Context, request?: Request) => {
ctx.metrics.keyClearTotal.inc({ env: ctx.env.ENVIRONMENT });
const keys = await ctx.env.ISSUANCE_KEYS.list();
Expand All @@ -169,6 +198,9 @@ const handleClearKey = async (ctx: Context, request?: Request) => {
}
const toDeleteArray = [...toDelete];
await ctx.env.ISSUANCE_KEYS.delete(toDeleteArray);

ctx.waitUntil(clearDirectoryCache());

return new Response(`Keys cleared: ${toDeleteArray.join('\n')}`, { status: 201 });
};

Expand Down
38 changes: 37 additions & 1 deletion test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { jest } from '@jest/globals';

import { Context } from '../src/context';
import { handleTokenRequest, default as workerObject } from '../src/index';
import { IssuerConfigurationResponse } from '../src/types';
import { b64ToB64URL, u8ToB64 } from '../src/utils/base64';
import { ExecutionContextMock, getContext, getEnv } from './mocks';
import { ExecutionContextMock, MockCache, getContext, getEnv } from './mocks';
import { RSABSSA } from '@cloudflare/blindrsa-ts';
import {
MediaType,
Expand Down Expand Up @@ -147,3 +149,37 @@ describe('rotate and clear key', () => {
expect(directory['token-keys']).toHaveLength(1);
});
});

describe('cache directory response', () => {
const rotateURL = `${sampleURL}/admin/rotate`;
const directoryURL = `${sampleURL}${PRIVATE_TOKEN_ISSUER_DIRECTORY}`;

const initializeKeys = async (numberOfKeys = 1): Promise<void> => {
const rotateRequest = new Request(rotateURL, { method: 'POST' });

for (let i = 0; i < numberOfKeys; i += 1) {
await workerObject.fetch(rotateRequest, getEnv(), new ExecutionContextMock());
}
};

it('should cache the directory response', async () => {
await initializeKeys();

const mockCache = new MockCache();
const spy = jest.spyOn(caches, 'open').mockResolvedValue(mockCache);
const directoryRequest = new Request(directoryURL);

let response = await workerObject.fetch(directoryRequest, getEnv(), new ExecutionContextMock());
expect(response.ok).toBe(true);
expect(Object.entries(mockCache.cache)).toHaveLength(1);

const [cachedURL, _] = Object.entries(mockCache.cache)[0];
const cachedResponse = new Response('cached response');
mockCache.cache[cachedURL] = cachedResponse;

response = await workerObject.fetch(directoryRequest, getEnv(), new ExecutionContextMock());
expect(response.ok).toBe(true);
expect(response).toBe(cachedResponse);
spy.mockClear();
});
});
25 changes: 25 additions & 0 deletions test/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,31 @@ import { Context, WaitUntilFunc } from '../src/context';
import { ConsoleLogger, Logger } from '../src/context/logging';
import { MetricsRegistry } from '../src/context/metrics';

export class MockCache implements Cache {
public cache: Record<string, Response> = {};

async match(info: RequestInfo, options?: CacheQueryOptions): Promise<Response | undefined> {
if (options) {
throw new Error('CacheQueryOptions not supported');
}
const url = new URL(info instanceof Request ? info.url : info).href;
return this.cache[url];
}

async delete(info: RequestInfo, options?: CacheQueryOptions): Promise<boolean> {
if (options) {
throw new Error('CacheQueryOptions not supported');
}
const url = new URL(info instanceof Request ? info.url : info).href;
return delete this.cache[url];
}

async put(info: RequestInfo, response: Response): Promise<void> {
const url = new URL(info instanceof Request ? info.url : info).href;
this.cache[url] = response;
}
}

export class ExecutionContextMock implements ExecutionContext {
waitUntils: Promise<any>[] = [];
passThrough = false;
Expand Down
1 change: 1 addition & 0 deletions wrangler.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ route = { pattern = "pp-issuer.example.test", custom_domain=true }
crons = ["0 0 * * *"]

[env.production.vars]
DIRECTORY_CACHE_MAX_AGE_SECONDS = "86400"
ENVIRONMENT = "production"
SENTRY_SAMPLE_RATE = "0" # Between 0-1 if you log errors on Sentry. 0 disables Sentry logging. Configuration is done through Workers Secrets

Expand Down
Loading