Closed Bug 1442636 Opened 7 years ago Closed 7 years ago

Add Access-Control-Max-Age header to reduce additional CORS preflight requests

Categories

(Taskcluster :: Services, enhancement)

Product:
Taskcluster
Component:
Services
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: emorley, Assigned: fiennyangeln, Mentored)

Details

Currently the CORS preflight request isn't cached, meaning that every call to https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 requires a preflight request. This can be seen by visiting https://tools.taskcluster.net/auth/roles/ and then hitting refresh. With preflight caching, the second load could avoid the OPTIONS request - but I'm presuming this affects the every 15 minute auth renewal used by tc-tools and Treeherder. In addition, when using things like custom actions, Treeherder currently makes multiple requests to login.m.net (a bug in its own right, but still), resulting in many unnecessary preflight requests: 14:59:57.820 OPTIONS XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 477ms] 14:59:57.992 OPTIONS XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 481ms] 14:59:58.007 OPTIONS XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 321ms] 14:59:58.101 OPTIONS XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 109ms] 14:59:58.117 GET XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 3500ms] 14:59:58.117 GET XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 4148ms] 14:59:58.226 GET XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 4813ms] 14:59:58.320 GET XHR https://login.taskcluster.net/v1/oidc-credentials/mozilla-auth0 [HTTP/1.1 200 OK 5265ms] As such I think it would be useful to add an `Access-Control-Max-Age` header to at least login.tc.net, and also possibly some of the other sites too (though as I understand it, the CORS headers are only cached for the exact resource URL, so URLs containing task IDs would have a low cache hit rate). Docs here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age
s/but I'm presuming this affects/and I'm presuming this also affects/
This is a good idea - thanks! We set Access-Control-* headers for all APIs here: https://github.com/taskcluster/taskcluster-lib-api/blob/master/src/api.js#L848 so this would mean adding the header Ed has described to that code. Once that's done, we can make a new release of the library and then upgrade other services to use the new version.
Mentor: dustin
Component: Login → Platform Libraries
Hi. I submitted PR for this https://github.com/taskcluster/taskcluster-lib-api/pull/83 please review. Thank youu!!
Flags: needinfo?(dustin)
Assignee: nobody → fiennyangeln
Flags: needinfo?(dustin)
Commit pushed to master at https://github.com/taskcluster/taskcluster-lib-api https://github.com/taskcluster/taskcluster-lib-api/commit/ea2cff674eaa518977aac716c8d8fbf1ffc30df4 Add Access-Control-Max Age header Add Access-Control-Max-Age header and set it to be 900 (15 minutes). It is used to reduce additional CORS Preflight requests Bug 1442636
Pushed as taskcluster-lib-api@6.1.0 -- once Travis-CI builds that it should appear at https://www.npmjs.com/package/taskcluster-lib-api Then on to updating the services!!
do you mean adding theallowedCORSOrigin to be true as option to be passed when creating API on that service? :D Because it seems like it is already defaulted to use the allowedCORSOrigin based on this https://github.com/taskcluster/taskcluster-lib-api/blob/master/src/api.js#L811 Since on the Hooks, auth, queue repo, they dont specify it as option, so the default one (the allowed cors origin) is used. Is my understanding correct ? :D or is it actually something else to be updated?
Flags: needinfo?(dustin)
You're correct. All we need to do is update the version of taskcluster-lib-api that each of those services use. It's not the most exciting part of the job since it's just running the same command in a bunch of repositories, but it's part of the job and we do it quite often. So let's pick a service -- say, secrets -- and upgrade it and then check that the new header is in place. If it is, we can do the rest of the services.
Flags: needinfo?(dustin)
So, like updating the package.json and see if the header is in place? Will do it :D
Flags: needinfo?(dustin)
Yes, but use `yarn upgrade --latest taskcluster-lib-api` as that will also change the yarn-lock file.
Flags: needinfo?(dustin)
Hi Dustin, I try to work on Hooks first to update it, there is this error AssertionError [ERR_ASSERTION]: Option 'name' must be provided which is caused by the https://github.com/fiennyangeln/taskcluster-hooks/blob/master/src/v1.js#L10 when call the api does not provide parameter name which is now required. So I added name into it. I checked using console.log that the Header is in place. Please review https://github.com/taskcluster/taskcluster-hooks/pull/86
Flags: needinfo?(dustin)
Nice job figuring out why that error occurred and fixing it :)
Flags: needinfo?(dustin)
Commit pushed to master at https://github.com/taskcluster/taskcluster-hooks https://github.com/taskcluster/taskcluster-hooks/commit/424c5ad43dc085985e1a3701cefe0865c7f6bf10 Upgrade the taskcluster-lib-api Upgrade to the latest taskcluster-lib-api, add 'name' as parameter since it is becoming a required parameter. Related to Bug 1442636
Deployed -- did it work?
Sorry but i dont understand what you mean .. When i printed out the response, I see the Access-Control-Max-Header with value of 900. Is there any other way to test it? :D should i continue upgrading the rest?
Flags: needinfo?(dustin)
That's what I meant -- sounds good! So yes, carry on!
Flags: needinfo?(dustin)
Dustin, I have upgraded most of the services. However, for some services ( events, aws, tree-herder, host-secrets, and tools ), they either have different version of Node / does not have the lib-api as dependency inside their package.json.. For pulse, it is a bit different. The test fails because we declare scopes inside it https://github.com/fiennyangeln/taskcluster-pulse/blob/master/src/v1.js#L121 which is not in the right format. This is what the scopes be, and the error is caused by [ [ 'pulse:namespace:<namespace>' ] ] being passed ( error is resulted from this line : https://github.com/fiennyangeln/taskcluster-lib-api/blob/master/src/api.js#L774) so i changed the format of scopes to be scopes: {AllOf: ['pulse:namespace:<namespace>'], }, Do you think I can do this way? Then for the rest which does not have taskcluster-lib-api as dependency do we need to upgrade them as well? (some of them use version 3 of the lib-api, which i think might be because of different Node version)
Flags: needinfo?(dustin)
(In reply to Fienny Angelina from comment #16) > Dustin, I have upgraded most of the services. However, for some services ( > events, aws, tree-herder, host-secrets, and tools ), they either have > different version of Node / does not have the lib-api as dependency inside > their package.json.. Great! Yes, that's a list of the more "oddball" services we run. Good job skipping them :) > For pulse, it is a bit different. The test fails because we declare scopes > inside it > https://github.com/fiennyangeln/taskcluster-pulse/blob/master/src/v1.js#L121 > which is not in the right format. This is what the scopes be, and the error > is caused by [ [ 'pulse:namespace:<namespace>' ] ] being passed ( error is > resulted from this line : > https://github.com/fiennyangeln/taskcluster-lib-api/blob/master/src/api. > js#L774) > > so i changed the format of scopes to be > scopes: {AllOf: > ['pulse:namespace:<namespace>'], > }, > > Do you think I can do this way? Absolutely, and I'm really happy you took the initiative to figure that out :) > Then for the rest which does not have taskcluster-lib-api as dependency do > we need to upgrade them as well? (some of them use version 3 of the lib-api, > which i think might be because of different Node version) Nope, don't worry about those. I've assigned all of the PR's for review, but really that means "please merge and carefully deploy" more than "review" since it's a one-line change. That will happen over the next few days, but there's not really anything left for you to do here other than watch while your code gets put into production all over the place :)
Flags: needinfo?(dustin)
Commit pushed to master at https://github.com/taskcluster/taskcluster-login https://github.com/taskcluster/taskcluster-login/commit/38dd230bc54e84ae3c3dd84873ac2be8ab7b764b Upgrade tc-lib-api (#87) Upgrade taskcluster-lib-api to carry Access-Control-Max-Age Related to Bug 1442636
Commit pushed to master at https://github.com/taskcluster/taskcluster-index https://github.com/taskcluster/taskcluster-index/commit/b121cdea8a130ceafe47e9df183a697e97d7086c Upgrade tc-lib-api Upgrade taskcluster-lib-api to carry Access-Control-Max-Age Related to Bug 1442636
Commit pushed to master at https://github.com/taskcluster/taskcluster-queue https://github.com/taskcluster/taskcluster-queue/commit/1e9f7be66dc93138f83b7d3177d70398fdeca387 Upgrade taskcluster-lib-api version Upgrade taskcluster-lib-api version to the newest one. It is to enable Access-Control-Max-Age Related to Bug 1442636
Commit pushed to master at https://github.com/taskcluster/taskcluster-auth https://github.com/taskcluster/taskcluster-auth/commit/18599c1ea57a2f5b617f60da4e2d52981ff938d2 Upgrade taskcluster-lib-api Upgrade taskcluster-lib-api to carry Access-Control-Max-Age Related to Bug 1442636
This is completed -- all services except aws-provisioner and ec2-manager have since been upgraded.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: Platform Libraries → Services
You need to log in before you can comment on or make changes to this bug.