Skip to content

Conversation

jglogan
Copy link
Contributor

@jglogan jglogan commented Oct 6, 2025

  • Closes [Bug]: image fetch honors HTTP_PROXY but not other standard variables. #255.
  • Fixes ProxyUtils so that the environment variable to be used for proxy selection is determined by the request scheme.
  • RegistryClient uses ProxyUtils to get the proxy URL used by the HTTPClient.
  • Tweak hostname resolution error message to avoid misleading output if the proxy hostname cannot be resolved.

@jglogan jglogan marked this pull request as draft October 6, 2025 20:37
Copy link
Contributor

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

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

With all the changes it feels pretty leaky all over the codebase. Maybe we can chat on this one real-time and figure something out. I'll spend some cycles on it today.

@jglogan jglogan force-pushed the image-proxy branch 2 times, most recently from 94823fd to c57fe5e Compare October 7, 2025 23:03
@jglogan jglogan changed the title Fixes proxy access for image registry types. Fixes proxy logic in RegistryClient. Oct 7, 2025
@jglogan jglogan marked this pull request as ready for review October 7, 2025 23:05
@jglogan jglogan requested a review from crosbymichael October 7, 2025 23:05
// proxy configuration assumes all client requests will go to `base` URL
self.proxyURL = ProxyUtils.proxyFromEnvironment(scheme: scheme, host: host)
if let proxyURL = self.proxyURL, let proxyHost = proxyURL.host {
httpConfiguration.proxy = HTTPClient.Configuration.Proxy.server(host: proxyHost, port: proxyURL.port ?? 8080)
Copy link
Contributor

Choose a reason for hiding this comment

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

is 8080 correct here or should it be 80?

Copy link
Contributor Author

@jglogan jglogan Oct 8, 2025

Choose a reason for hiding this comment

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

That should be 80 given that I changed other things to be go-like. I'll rebase and fix.

Originally that's what I thought curl used and I was doing curl-style semantics. But 8080 is wrong there as well – curl actually defaults to 1080 🤷 (though the docs aren't clear whether this applies to just socks proxies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

443 for https scheme, 80 otherwise now

- Fixes ProxyUtils so that the environment
  variable to be used for proxy selection
  is determined by the request scheme.
- RegistryClient uses ProxyUtils to
  get the proxy URL used by the HTTPClient.
- Tweak hostname resolution error message
  to avoid misleading output if the proxy
  hostname cannot be resolved.
@crosbymichael crosbymichael merged commit 2443a24 into apple:main Oct 9, 2025
2 checks passed
@jglogan jglogan deleted the image-proxy branch October 9, 2025 18:03
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.

[Bug]: image fetch honors HTTP_PROXY but not other standard variables.

2 participants