Add cache issuer directory #15

thibmeu · 2024-02-09T09:43:22Z

To reduce the number of calls to R2, with an expensive list operation, add cache in front of issuer directory endpoint.

It's worth noting that this change does not affect token issuance. The key is still retrieve for every issuance, incurring a class A operation.

Insatead of computing the issuer directory whenever requested, cache it with the same setting used by clients. This cache is clearer upon key rotation. This commit also allows cache time to be configured per deployment.

Disable eslint no-unused-variable for handler. `request` does not have to be used, and it's good to have a consistent signature for the handlers.

To enable monitoring of cache miss, a dedicated prometheus metric is added.

With the multiplication of metrics, use alphabetical ordering to ease searches.

thibmeu · 2024-02-09T09:45:08Z

src/context/metrics.ts

-		this.requestsTotal = this.registry.create('counter', 'requests_total', 'total requests');
+		this.directoryCacheMissTotal = this.registry.create(
+			'counter',
+			'directory_cache_miss_total',


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.

Lekensteyn · 2024-02-09T11:25:49Z

The response is pretty small (about 569 bytes). This PR tackles edge-side caching. Do you also want to attempt to address client-side caching?

If you include a Last-Modified and/or ETag response header, then the client can send a If-Modified-Since or If-None-Match header in its subsequent request. If there are no changes, the server can respond with a HTTP 304 Not Modified status code.

See https://gitlab.com/wireshark/ws-cloudflare-workers/-/blob/71ff179cbb71eed0242b0442eeaf571727b8de97/wiki-worker/src/index.js#L226-238 for an example if you are interested in that.

I did not see the Last-Modified or ETag headers in production, but in dev I do see that it is being ignored.

$ curl https://pp-issuer-dev.research.cloudflare.com/.well-known/private-token-issuer-directory -si
HTTP/2 200
date: Fri, 09 Feb 2024 11:10:43 GMT
content-type: application/private-token-issuer-directory
content-length: 569
cf-ray: 852baae33e79b724-AMS
cf-cache-status: HIT
accept-ranges: bytes
age: 5433
cache-control: public, max-age=86400
expires: Sat, 10 Feb 2024 11:10:43 GMT
last-modified: Fri, 09 Feb 2024 09:40:10 GMT
strict-transport-security: max-age=31536000; includeSubDomains
x-content-type-options: nosniff
access-control-allow-origin: *
server: cloudflare

{"issuer-request-uri":"/token-request","token-keys":[{"token-type":2,"token-key":"MIIBUjA9BgkqhkiG9w0BAQowMKANMAsGCWCGSAFlAwQCAqEaMBgGCSqGSIb3DQEBCDALBglghkgBZQMEAgKiAwIBMAOCAQ8AMIIBCgKCAQEA59SRdhH67WaxoDuutpAEAuTMCXxmir7e4ij17b3T4AVGq4uAYViMgr0Db_iFhbIc-SbLH02szzionlkDFv3ANz4Ody2U6uoXouMn5O4qC7QquBoYAwLfs9OcxxeD8IxbsdtMSORJ3wExXuqgmP9XH2tlzVvQLE1RreINBBys77Le0H-9uKXsATeMwMAQgWOf_x05OQlsXIYXQTw17Up83o-bBO309we4-9rC8RUhTT_E9TYurSTnB4PGSHX_S_OMraaKXKRBCREzrUaUs1f48ZBpR4gwhZ4cDjkoVuzVm_PNd96QnVRJwhsYulNCiZw0OllplT-UcfMpUIObxFv3rwIDAQAB","not-before":1706745608704}]}

$ curl https://pp-issuer-dev.research.cloudflare.com/.well-known/private-token-issuer-directory -si -Hif-modified-since:'Fri, 09 Feb 2024 09:40:10 GMT'
HTTP/2 200
date: Fri, 09 Feb 2024 11:11:02 GMT
content-type: application/private-token-issuer-directory
content-length: 569
cf-ray: 852bab57cf11b724-AMS
cf-cache-status: HIT
accept-ranges: bytes
age: 5452
cache-control: public, max-age=86400
expires: Sat, 10 Feb 2024 11:11:02 GMT
last-modified: Fri, 09 Feb 2024 09:40:10 GMT
strict-transport-security: max-age=31536000; includeSubDomains
x-content-type-options: nosniff
access-control-allow-origin: *
server: cloudflare

{"issuer-request-uri":"/token-request","token-keys":[{"token-type":2,"token-key":"MIIBUjA9BgkqhkiG9w0BAQowMKANMAsGCWCGSAFlAwQCAqEaMBgGCSqGSIb3DQEBCDALBglghkgBZQMEAgKiAwIBMAOCAQ8AMIIBCgKCAQEA59SRdhH67WaxoDuutpAEAuTMCXxmir7e4ij17b3T4AVGq4uAYViMgr0Db_iFhbIc-SbLH02szzionlkDFv3ANz4Ody2U6uoXouMn5O4qC7QquBoYAwLfs9OcxxeD8IxbsdtMSORJ3wExXuqgmP9XH2tlzVvQLE1RreINBBys77Le0H-9uKXsATeMwMAQgWOf_x05OQlsXIYXQTw17Up83o-bBO309we4-9rC8RUhTT_E9TYurSTnB4PGSHX_S_OMraaKXKRBCREzrUaUs1f48ZBpR4gwhZ4cDjkoVuzVm_PNd96QnVRJwhsYulNCiZw0OllplT-UcfMpUIObxFv3rwIDAQAB","not-before":1706745608704}]}

I originally tried HEAD to test the endpoint, but there appears to be a bug there: #16

thibmeu · 2024-02-09T14:22:56Z

@Lekensteyn I've added a commit to support HEAD, and provide an Etag. If Etag on the request matches, 304 is returned. HEAD is only stripping GET body if there is one, to make sure the headers are correct.

In addition, it adds Content-Length.

Lekensteyn · 2024-02-09T14:46:12Z

src/index.ts

@@ -179,6 +236,7 @@ export default {
 		const router = new Router();

 		router
+			.head(PRIVATE_TOKEN_ISSUER_DIRECTORY, handleHeadTokenDirectory)


I think that head method should be dropped, head should be automatically added for all GET requests by the underlying router as suggested in #16 with appropriate tests verifying that behavior.

You do not have to go our of your way to omit the body in the Response constructor. The underlying Workers runtime will skip inclusion of the the body for HEAD requests.

The advantage of using the same implementation is that your response (headers) will be the same with no extra effort.

Lekensteyn · 2024-02-09T14:47:58Z

src/index.ts

 		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}`,
+			'content-length': body.length.toString(),


Question: Isn't content-length automatically set? Or is it missing due to chunked encoding being used?

when I first tested, it did not seem to be set automatically. therefore adding it here. The question I have is if the request is compressed, I'm not sure if it's modified automatically.

Lekensteyn · 2024-02-09T14:48:46Z

src/index.ts

-			'cache-control': 'public, max-age=86400',
+			'cache-control': `public, max-age=${ctx.env.DIRECTORY_CACHE_MAX_AGE_SECONDS}`,
+			'content-length': body.length.toString(),
+			'date': new Date().toUTCString(),


Date is not that important for caching, has this been added for debugging purposes?

it's been added to help debugging if there are caching issues in the future.

Browsers might send `HEAD` request to understand if they should do a `GET` request. This commit adds an ETag on response, allowing `If-None-Match` to be handled, as well as `HEAD` request to be correctly processed.

armfazh · 2024-02-09T23:04:41Z

src/context/metrics.ts

+		this.issuanceRequestTotal = this.registry.create(
+			'counter',
+			'issuance_request_total',
+			'Number of requests for private token issuance.'


successful?

Suggested change

'Number of requests for private token issuance.'

'Number of successful requests for private token issuance.'

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

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

armfazh · 2024-02-09T23:23:41Z

src/index.ts

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


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) => {

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

The former has not been updated in 5 years.

thibmeu · 2024-02-12T10:02:05Z

once merged
closes #16

thibmeu added 4 commits February 9, 2024 10:17

Add cache for issuer directory

ceb85bd

Insatead of computing the issuer directory whenever requested, cache it with the same setting used by clients. This cache is clearer upon key rotation. This commit also allows cache time to be configured per deployment.

Fix unused variable warning

1f927bd

Disable eslint no-unused-variable for handler. `request` does not have to be used, and it's good to have a consistent signature for the handlers.

Add metrics for directory cache miss

92ef2a9

To enable monitoring of cache miss, a dedicated prometheus metric is added.

Fix metrics variable declaration to use alphabetical order

e193ae5

With the multiplication of metrics, use alphabetical ordering to ease searches.

thibmeu added the enhancement New feature or request label Feb 9, 2024

thibmeu requested review from cdrubin and armfazh February 9, 2024 09:43

thibmeu self-assigned this Feb 9, 2024

thibmeu commented Feb 9, 2024

View reviewed changes

Lekensteyn reviewed Feb 9, 2024

View reviewed changes

Add HEAD request handling and etag to issuer directory

6482ab3

Browsers might send `HEAD` request to understand if they should do a `GET` request. This commit adds an ETag on response, allowing `If-None-Match` to be handled, as well as `HEAD` request to be correctly processed.

thibmeu force-pushed the add-cache-issuer-directory branch from de8f900 to 6482ab3 Compare February 9, 2024 15:05

armfazh approved these changes Feb 9, 2024

View reviewed changes

Update eslint-config-typescript to @typescript-eslint/eslint-plugin

6249350

The former has not been updated in 5 years.

thibmeu merged commit b2b7834 into cloudflare:main Feb 13, 2024
2 checks passed

thibmeu deleted the add-cache-issuer-directory branch February 13, 2024 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache issuer directory #15

Add cache issuer directory #15

thibmeu commented Feb 9, 2024 •

edited

Loading

thibmeu Feb 9, 2024

Lekensteyn commented Feb 9, 2024 •

edited

Loading

thibmeu commented Feb 9, 2024

Lekensteyn Feb 9, 2024

thibmeu Feb 9, 2024

Lekensteyn Feb 9, 2024

thibmeu Feb 9, 2024

Lekensteyn Feb 9, 2024

thibmeu Feb 9, 2024

armfazh Feb 9, 2024

thibmeu Feb 12, 2024

thibmeu Feb 12, 2024

armfazh Feb 9, 2024

thibmeu Feb 12, 2024

thibmeu commented Feb 12, 2024

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

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

Add cache issuer directory #15

Add cache issuer directory #15

Conversation

thibmeu commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Lekensteyn commented Feb 9, 2024 • edited Loading

thibmeu commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibmeu commented Feb 12, 2024

thibmeu commented Feb 9, 2024 •

edited

Loading

Lekensteyn commented Feb 9, 2024 •

edited

Loading