Skip to content

archlinux: fix: NO_PROXY=127.0.0.1#189

Merged
marmarek merged 1 commit into
QubesOS:mainfrom
3nprob:fix-localhost-proxying
Mar 21, 2025
Merged

archlinux: fix: NO_PROXY=127.0.0.1#189
marmarek merged 1 commit into
QubesOS:mainfrom
3nprob:fix-localhost-proxying

Conversation

@3nprob

@3nprob 3nprob commented Mar 20, 2025

Copy link
Copy Markdown

otherwise requests to repos on localhost will be passed via the proxy, which we generally don't want.

Related:

@codecov

codecov Bot commented Mar 20, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.88%. Comparing base (73cbfd4) to head (f539987).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   74.69%   74.88%   +0.18%     
==========================================
  Files          51       51              
  Lines        5865     5945      +80     
==========================================
+ Hits         4381     4452      +71     
- Misses       1484     1493       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

# Update archlinux keyring first so that Archlinux can be updated even after a long time
chroot_cmd /bin/sh -c \
"http_proxy='${REPO_PROXY}' pacman -Sy --noconfirm --noprogressbar archlinux-keyring"
"no_proxy='${NO_PROXY}' http_proxy='${REPO_PROXY}' pacman -Sy --noconfirm --noprogressbar archlinux-keyring"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What this no_proxy variable is for, and where NO_PROXY is set? If NO_PROXY is set already, that should be good enough for pacman (at least when it uses curl) already, no?

@3nprob 3nprob Mar 21, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

By default, it's not set to anything, meaning all connections (including 127.0.0.1, localhost, ::1), will be passed over it.

It's even more non-standard and inconsistently interpreted vs http_proxy/https_proxy/all_proxy. For example, AFAIK there is no way to configure this for PackageKit at all while pacman respects no_proxy (via libcurl, the same way it gets http_proxy).

https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy

https://superuser.com/questions/944958/are-http-proxy-https-proxy-and-no-proxy-environment-variables-standard

As this script is often run in a dispvm or container, overriding it can be non-trivial (why else is REPO_PROXY here at all? ;)). I figure setting this sane default while allowing the manual override would make sense?

Is there any use-case for proxying localhost connections? Currently the Qubes proxy is configured to 403 any attempts at doing that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@3nprob 3nprob Mar 21, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

FWIW, forcing it to use my local apt-cacher-ng works by modifying that, save for the issue resolved by this patch, and still getting spurious 503 mentioned in #188 when running cold.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But still, something needs to set NO_PROXY, no?

@3nprob 3nprob Mar 21, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I guess at this stage it's left as "excercise for the reader" here.. If you need it, at least it's one less package you'll have to patch. (If I understood the question right?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean, why not set it by default to 127.0.0.1 like in the other place?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, right, sure, I don't see why not. It even aligns better with PR description 😅 Pushed!

otherwise requests to repos on localhost will be passed via the proxy,
which we generally don't want.
@3nprob
3nprob force-pushed the fix-localhost-proxying branch from 73a0daa to f539987 Compare March 21, 2025 03:04
@marmarek

Copy link
Copy Markdown
Member

PipelineRetry

@marmarek
marmarek merged commit 099f4f6 into QubesOS:main Mar 21, 2025
@3nprob
3nprob deleted the fix-localhost-proxying branch March 21, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants