Respect proxy settings when streaming the download and extraction of the CodeQL bundle #2624

NlightNFotis · 2024-11-28T10:50:23Z

Description

This PR is adding support for respecting proxy settings while downloading the CodeQL bundle in streaming mode.

Fixes #2600

Guidance

The code changes required to support the feature have been added in 0dc76a9.
It's introducing a justfile, which is a script interpreted by https://github.com/casey/just, a command action runner. It allows us to create an python virtual environment and run the sync.py to update dependencies in one step, which is very useful for someone trying to interact with it the first time.
- If just is not present, the file is irrelevant (it's just a text file) but can act as documentation for the steps to invoke manually.
I have initially tested this works locally, by setting tinyproxy and writing a function that invoked downloadAndExtractZstdWithStreaming and observing that we go through the proxy.
- This function was subsequently thrown away after the first tests locally validated the design.
For CI testing, I have made changes to the test_proxy.yml workflow, so that we download the latest nightly bundle, which by default has the extension .tar.zst which goes through the code path introduced.
- This can be seen working here (latest CI run at the time of writing).
This required some changes to the sync.py file in 9c48c8b in order to support a step that is being used to initialise the container image (because the Ubuntu container is leaned out and doesn't come with the list of pre-installed software the runners usually come prepared with).
The diff (~31k lines) is mostly comprised of a couple of packages added in 8f2cb3a. The actual code changes introduced by this PR are in the other commits, and are much smaller in terms of loc changed.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

src/tools-download.ts

NlightNFotis · 2024-12-04T15:20:48Z

The streaming of the bundle using the Proxy worked fine in the last CI run: https://github.com/github/codeql-action/actions/runs/12146133664/job/33869336618?pr=2624#step:7:33

I'm now in the process of tidying this up (lots of experimental commits), and incorporating conflicting changes from main.

…reaming

…ng with proxy

pr-checks/test-proxy.patch

NlightNFotis · 2024-12-05T17:33:15Z

Something that I forgot to mention: this PR is not removing the feature flag: there are 2 references in the code (after this PR gets merged) which are very easy to remove and do not impact functionality.

There are also two more references in a couple of test actions, which would require a bit more thought as to how to handle (we could just remove the environment variable reference in them, or just delete them given that this is more or less tested in the new test_proxy workflow).

I wanted to do any changes to the feature flag/env var as a different PR, and keep the changes here scoped down to the proxy settings and their handling by the streaming download handler.

henrymercer

Thanks for this fix! I've added a couple of questions / suggestions:

CHANGELOG.md

pr-checks/checks/test-proxy.yml

henrymercer · 2024-12-05T18:15:26Z

pr-checks/checks/test-proxy.yml

@@ -1,10 +1,20 @@
 name: "Proxy test"
 description: "Tests using a proxy specified by the https_proxy environment variable"
-versions: ["linked"]
+versions: ["linked", "nightly-latest"]


This test was passing before when we were streaming zstd bundles with a CodeQL Action version that wasn't respecting proxy settings, presumably because the CodeQL Action could still access the bundle without the proxy.

Can we update the test to ensure that we're downloading the bundle via the proxy?

In the latest test added as part of this PR, I'm fairly confident we're downloading the bundle through the proxy.

The reason it worked before is that it didn't test the streaming of the bundle, because when linked is passed as the version, the prepare-step action was choosing the .tar.gz bundle by default - as example the latest run from a PR merged recently.

I am not sure though how it makes that selection between the .tar.gz and the .tar.zst one, because in this PR, the same job downloaded the .tar.zst one. But previously, we didn't go through this code because the same job was opting for the .tar.gz when downloading.

Will look more into that and report back any interesting findings.

henrymercer · 2024-12-05T18:17:26Z

src/tools-download.ts

+    core.warning(
+      `Failed to download and extract CodeQL bundle using streaming. Falling back to downloading the bundle before extracting.`,


Have you tested this fallback? For instance, what happens if we partially extract the CodeQL bundle while streaming, but something goes wrong? Is it worth cleaning up the destination directory in this case or is that handled elsewhere?

Yes, I'm quoting from a previous message of mine that probably got lost because of the volume of messages here:

I'm confident we are good without a flag to manage the risk.

To explain why: because the conflicts introduced by a couple of other PRs were extensive, and I couldn't sort out a clean commit history, I opted to instead just manually recreate the PR on top of main (my code changes are small, so easy to manually replay).

In doing so, I made a mistake: I initialised the HTTP client (the one that respects the proxy settings) and passed it to the headers by accident (instead of the http.get options). What happened is here, but short story, because of the streaming download now being inside of a fallback try/catch, the failure to download the CodeQL pack with streaming resulted in falling back to downloading it and then extracting it.

I've since fixed this, but this little mistake validated the fallback mechanism working as intended, so I believe that this won't cause an incident like last time - if it fails, worst case scenario is that the mechanism won't work as expected and fall back to the previous mechanism (download and then extract), with the impact being that we lose a few seconds per run (compared to a failed run).

Does this answer your question satisfactorily?

Thank you, that answers the first part of my question. I still wonder if it is worth cleaning up the destination directory as part of the catch in case we managed to extract some of the CodeQL bundle but not a complete CodeQL bundle.

I agree this is a good idea, and something prudent for us to do before we retry.

Added in 4c20d4f.

pr-checks/checks/test-proxy.yml

pr-checks/justfile

Co-authored-by: Henry Mercer <henrymercer@github.com>

…ep for the container

…e extraction

NlightNFotis · 2024-12-09T19:14:59Z

Thank you for your reviews @aeisenberg @henrymercer.

I have now implemented all outstanding items on this PR. Would it be possible to have another look at this?

aeisenberg · 2024-12-09T19:56:20Z

CHANGELOG.md

@@ -7,7 +7,8 @@ Note that the only difference between `v2` and `v3` of the CodeQL Action is the
 ## [UNRELEASED]

 - We are rolling out a change in December 2024 that will extract the CodeQL bundle directly to the toolcache to improve performance. [#2631](https://github.com/github/codeql-action/pull/2631)
- We have added support for respecting the proxy settings present in environment variables in a runner when streaming the download and extraction of the CodeQL bundle. [#2624](https://github.com/github/codeql-action/pull/2624)
+- Fixed an issue where streaming the download and extraction of the CodeQL bundle did not respect proxy settings. [#2624](https://github.com/github/codeql-action/pull/2624)
+- Update default CodeQL bundle version to 2.20.0. [#2636](https://github.com/github/codeql-action/pull/2636)


Was this line added due to a bad merge?

Hm, I will have a look.

This was reported as a conflict in the GitHub UI, but when I did a git merge locally, it had no problem automatically resolving the conflict. Perhaps it did a weird thing because of my .gitconfig.

I cannot find precisely what went wrong here, as the merge log is past my terminal character limit. I remember it saying that it resolved it automatically using the ort method, but it apparently went wrong there.

This is now fixed in 9e8cd42.

Going forward I will be more careful validating these merges went right.

aeisenberg · 2024-12-09T19:58:53Z

src/tools-download.ts

@@ -127,7 +127,11 @@ export async function downloadAndExtract(
    core.warning(
      `Failed to download and extract CodeQL bundle using streaming. Falling back to downloading the bundle before extracting.`,
    );
-    core.warning(getErrorMessage(e));
+    core.warning(`Error: ${getErrorMessage(e)}`);


Don't prefix a warning with Error.

Suggested change

core.warning(`Error: ${getErrorMessage(e)}`);

core.warning(`Warning: ${getErrorMessage(e)}`);

But, do we even need the prefix at all?

Maybe reverting this is better:

Suggested change

core.warning(`Error: ${getErrorMessage(e)}`);

core.warning(getErrorMessage(e));

Thank you for catching this.

Seeing it reported now, I agree it looks a bit silly.

My rationale for changing this was that this was rendering an error that was generated in the process of unpacking as a simple warning, with no other context whatsoever, which could be confusing to a user when interpreting the runner log (example).

I'm still motivated to decorate it somehow, as reverting may not satisfy this concern, but I will think about a cleaner decoration that doesn't look weird/silly.

Okay, this is what I came up with: 88bcf64

I've reworded the warnings to flow a bit more naturally for me, and to allow me to embed the error message in an indicative but non-invasive way.

Is this workable for you @aeisenberg ? Or do you have a strong preference for the previous state of affairs?

aeisenberg · 2024-12-09T19:59:22Z

pr-checks/readme.md

+
+This folder contains the code supporting the workflows run when a PR is created.
+
+## Update


I think we can simplify this. I can give some suggestions, but not quite right now.

See here for an alternative readme: https://github.com/github/codeql-action/compare/NlightNFotis/detect_use_proxy_when_streaming...aeisenberg/justfile-readme?quick_pull=1#diff-b90d48e45a6813fad7195d74075f05bdf05919291a02116fe8a6b67691b1266a

Thank you for this! I agree it looks cleaner.

Will cherry-pick this in.

henrymercer · 2024-12-10T17:23:39Z

I've merged in main since we just did a release, and I want to make sure the merge strategy puts the changelog entry in the right place.

…extraction

aeisenberg

Nice.

NlightNFotis force-pushed the NlightNFotis/detect_use_proxy_when_streaming branch 2 times, most recently from c888d44 to f742d6b Compare December 3, 2024 16:10

aeisenberg reviewed Dec 3, 2024

View reviewed changes

src/tools-download.ts Show resolved Hide resolved

NlightNFotis force-pushed the NlightNFotis/detect_use_proxy_when_streaming branch from 326b0f1 to 28dd147 Compare December 3, 2024 17:06

NlightNFotis force-pushed the NlightNFotis/detect_use_proxy_when_streaming branch 3 times, most recently from 99f0c0d to 2ff0598 Compare December 4, 2024 18:32

NlightNFotis added 3 commits December 4, 2024 19:29

feat: use http agent that respects environment proxy settings when st…

0dc76a9

…reaming

build: add undici to node_modules folder - CI fails without it

8f2cb3a

ci: add configuration tto run proxy with zstd package to test streami…

78be2f1

…ng with proxy

NlightNFotis force-pushed the NlightNFotis/detect_use_proxy_when_streaming branch from 2ff0598 to 78be2f1 Compare December 4, 2024 19:29

aeisenberg reviewed Dec 4, 2024

View reviewed changes

pr-checks/test-proxy.patch Outdated Show resolved Hide resolved

NlightNFotis added 2 commits December 5, 2024 15:32

ci: add capability to handle container initialisation steps to sync.py

9c48c8b

ci: remove patch file and cleanup justfile

b4bc093

NlightNFotis marked this pull request as ready for review December 5, 2024 17:00

NlightNFotis requested a review from a team as a code owner December 5, 2024 17:00

NlightNFotis requested review from aeisenberg, henrymercer and zutshisunakshi December 5, 2024 17:00

NlightNFotis changed the title ~~feat: detect and adjust CodeQL bundle download URL if proxy settings present~~ Respect proxy settings when streaming the download and extraction of the CodeQL bundle Dec 5, 2024

chore: add changelog entry

b706e37

NlightNFotis self-assigned this Dec 5, 2024

henrymercer reviewed Dec 5, 2024

View reviewed changes

aeisenberg reviewed Dec 5, 2024

View reviewed changes

pr-checks/checks/test-proxy.yml Show resolved Hide resolved

pr-checks/justfile Show resolved Hide resolved

NlightNFotis and others added 4 commits December 6, 2024 18:37

chore: reword description in CHANGELOG.md

182c5e7

Co-authored-by: Henry Mercer <henrymercer@github.com>

docs: add readme for justfile/pr-checks update

a89fbc8

ci: remove dns for container running proxy steps (experimental)

c901aee

Merge branch 'main' into NlightNFotis/detect_use_proxy_when_streaming

3951a82

NlightNFotis added 2 commits December 9, 2024 18:59

docs: add documentation for the reason why we need to have a setup st…

51e71f8

…ep for the container

feat: cleanup destination directory if we fail during streaming of th…

4c20d4f

…e extraction

aeisenberg reviewed Dec 9, 2024

View reviewed changes

Merge branch 'main' into NlightNFotis/detect_use_proxy_when_streaming

c6454d5

aeisenberg and others added 4 commits December 11, 2024 11:27

Update PR checks readme instructions

9323695

feat: change wording of error reporting when a failure occurs during …

88bcf64

…extraction

fix: move changelog entry to appropriate place

9e8cd42

Merge branch 'main' into NlightNFotis/detect_use_proxy_when_streaming

1e5b591

aeisenberg approved these changes Dec 11, 2024

View reviewed changes

NlightNFotis merged commit c4bbe15 into main Dec 11, 2024
266 checks passed

NlightNFotis deleted the NlightNFotis/detect_use_proxy_when_streaming branch December 11, 2024 16:26

This was referenced Dec 12, 2024

Merge main into releases/v3 #2645

Merged

Merge releases/v3 into releases/v2 #2647

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect proxy settings when streaming the download and extraction of the CodeQL bundle #2624

Respect proxy settings when streaming the download and extraction of the CodeQL bundle #2624

NlightNFotis commented Nov 28, 2024 •

edited

Loading

NlightNFotis commented Dec 4, 2024

NlightNFotis commented Dec 5, 2024

henrymercer left a comment

henrymercer Dec 5, 2024

NlightNFotis Dec 6, 2024

henrymercer Dec 5, 2024

NlightNFotis Dec 6, 2024

henrymercer Dec 9, 2024

NlightNFotis Dec 9, 2024

NlightNFotis commented Dec 9, 2024

aeisenberg Dec 9, 2024

NlightNFotis Dec 11, 2024

NlightNFotis Dec 11, 2024

aeisenberg Dec 9, 2024

NlightNFotis Dec 11, 2024

NlightNFotis Dec 11, 2024

aeisenberg Dec 9, 2024

aeisenberg Dec 9, 2024

NlightNFotis Dec 11, 2024

henrymercer commented Dec 10, 2024

aeisenberg left a comment

		core.warning(
		`Failed to download and extract CodeQL bundle using streaming. Falling back to downloading the bundle before extracting.`,

	core.warning(`Error: ${getErrorMessage(e)}`);
	core.warning(`Warning: ${getErrorMessage(e)}`);

	core.warning(`Error: ${getErrorMessage(e)}`);
	core.warning(getErrorMessage(e));


		This folder contains the code supporting the workflows run when a PR is created.

		## Update

Respect proxy settings when streaming the download and extraction of the CodeQL bundle #2624

Respect proxy settings when streaming the download and extraction of the CodeQL bundle #2624

Conversation

NlightNFotis commented Nov 28, 2024 • edited Loading

Description

Guidance

Merge / deployment checklist

NlightNFotis commented Dec 4, 2024

NlightNFotis commented Dec 5, 2024

henrymercer left a 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

NlightNFotis commented Dec 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

henrymercer commented Dec 10, 2024

aeisenberg left a comment

Choose a reason for hiding this comment

NlightNFotis commented Nov 28, 2024 •

edited

Loading