Use @actions/github as a wrapper around octokit in order to support proxies #100
Conversation
robertbrignull
force-pushed
the
github_proxy
branch
from
4fd9151 to
0086c2e
Compare
There was a problem hiding this comment.
Just one comment, but note the bug that CodeQL found: https://github.com/github/codeql-action/security/code-scanning/634?query=ref%3Arefs%2Fheads%2Fgithub_proxy
src/config-utils.test.ts
Outdated
| sinon.stub(api, "client").value(ok); | ||
| let client = new github.GitHub('123'); | ||
| const spyGetContents = sinon.stub(client.repos, "getContents").resolves(Promise.resolve(dummyResponse)); | ||
| sinon.stub(api, "getApiClient").value(() => client); |
There was a problem hiding this comment.
Is it worth moving these 3 lines to a function as they're repeated in 3 different places? You could take the dummyResponse as a parameter and return the spy.
There was a problem hiding this comment.
That's a good idea, and will make it easier to document what's going on there too.
There was a problem hiding this comment.
I've done this but it turned out to be harder than I expected it to be. Sinon, octokit, and toolkit/github don't make their types very easy to work with or access, so there's a lot of any floating around. I'm not hugely happy with it but I couldn't find anything better to do, and it's not any worse than it was before where there weren't any types.
There was a problem hiding this comment.
Agreed, still worth doing. Thanks for fighting with the type system for us!
That's not a bug. You can see later on there's a |
Ah, cool, thanks for clarifying. Do you want to close the alert as a false positive? |
Just done so. Thanks for the reminder. I forgot you can do that now. |
|
I think this is now good to go. I've copied the tests from https://github.com/actions/checkout/blob/master/.github/workflows/test.yml so we can test with a proxy automatically in actions. I also verified that if you remove the then all http requests fail, which is nice because it shows that all requests have to go through the proxy and thus we shouldn't fall into a degenerate case where if we simply ignore the |
| runs-on: ubuntu-latest | ||
| env: | ||
| https_proxy: http://no-such-proxy:3128 | ||
| no_proxy: api.github.com,github.com,github-production-release-asset-2e65be.s3.amazonaws.com |
There was a problem hiding this comment.
I'm not sure if I'm entirely happy with this value having to include github-production-release-asset-2e65be.s3.amazonaws.com. This is a consequence of https://github.com/actions/http-client/blob/master/proxy.ts (which we use for downloading the codeql-bundle) not supporting any form of wildcards, so the domain has to be explicit. Maybe this is fine if this domain is for all of github, but I'm worried about it changing when we update the bundle, or just at random.
The options that I see at the moment are to just not include this test, or to go with this hardcoded value and raise a PR on https://github.com/actions/http-client to improve support there.
@chrisgavin or @tibbes, do you have any thoughts here?
There was a problem hiding this comment.
It does seem a little bit dodgy.
Perhaps another option is to use mitmproxy with a script that blocks everything except S3 URLs.
There was a problem hiding this comment.
After more thought and trying to get this working, I think in fact it's more fair to say that we only partially support no_proxy. As far as I can tell there isn't really a format spec for this, but more a general convention between implementations, and the one we're using seems to be on the minimal side.
I'm going to remove this test and instead open an issue to improve this support and add the test back in.
This PR makes us use
@actions/githubwhich is a wrapper around octokit and mainly just helps with the initialisation. Some of it we were doing already, like setting the API URL, some of it we were not like handling proxies. By using this wrapper it takes care of more things for us.For info, here's where the proxy handling behaviour was introduced: actions/toolkit#314, and here's where the checkout action started using it which is how I found out about this: actions/checkout#144.
There is a slight difference in behaviour that I had to account for, which is that this new constructor fails if you don't provide it with an authentication token. This was making the tests fail because the client is constructed as a constant at the top level, so it ran and failed before we could stub it out with something else. I worked around this by changing the constant for a function to generate an API client. Constructing a new one each time is not a big loss of efficiency so I'm not too worried.
When doing the above I encountered some errors with
sinonbecause it requires you to callsinon.restore()after your tests, and if a test errors early then it wouldn't call this and would cause later tests to fail. By moving that call to a method that runs after every test we avoid this issue of interference.I haven't tested this to be sure, and I will do before merging, but I think it'll work with just these changes.
You can also see that the checkout action defines some integration tests that use a proxy so we should copy those. I will look at that, but wanted to put the PR up first.
Merge / deployment checklist