Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore proxy for cloud metadata retrieval #2700

Closed

Conversation

mhashizume
Copy link
Contributor

Prior to this commit when a user set the http_proxy environment variable, the Net::HTTP#new in the HTTP resolver would use it when establishing new connections. While this is the desired behavior in most cases, this also interfered with fetching cloud metadata for services like AWS and GCP.

Facter had previously dealt with this behavior in earlier versions (2.x) starting with 23bf7e5, but that was lost during Facter's transition from C++ back to Ruby.

This commit sets Net::HTTP#new's p_addr (proxy address) argument to nil when Facter detects that the http_proxy environment variable is present when attempting to resolve cloud service metadata.

@mhashizume mhashizume added the bug Something isn't working label Apr 3, 2024
@mhashizume mhashizume force-pushed the FACT-636/main/cloud-proxy branch 2 times, most recently from fb1a025 to 46e45e7 Compare April 4, 2024 21:49
@mhashizume mhashizume force-pushed the FACT-636/main/cloud-proxy branch 2 times, most recently from e33a9bc to 3922edc Compare April 8, 2024 21:53
This commit updates method documentation into YARD formatting.
Prior to this commit when a user set the http_proxy environment
variable, the Net::HTTP#new in the HTTP resolver would use it when
establishing new connections. While this is the desired behavior in
most cases, this also interfered with fetching cloud metadata for
services like AWS, GCP, and Azure.

Facter had previously dealt with this behavior in earlier versions (2.x)
starting with 23bf7e5, but that was lost during Facter's transition to
C++ then back to Ruby.

This commit exposes Net::HTTP#new's p_addr (proxy address) argument and,
by necessity, the preceding port positional argument, and configures
the AWS, GCE, and Azure resolvers to always pass nil as the proxy
address.
@mhashizume
Copy link
Contributor Author

I think this is introducing some unwanted behavior related to positional and keyword arguments.

Some of the methods being called here use a hash as what's intended to be a positional argument but, with the addition of keyword arguments in this PR, older Rubies seem to now misinterpret the keys in that hash as individual keyword arguments.

For example in the ec2 resolver, the headers and timeouts arguments now seem to get wrongly interpreted as keyword arguments:

def get_data_from(url)
headers = {}
headers['X-aws-ec2-metadata-token'] = v2_token if v2_token
Facter::Util::Resolvers::Http.get_request(url, headers, { session: determine_session_timeout })
end

I think we can resolve this by using strings instead of symbols in these positional hash arguments and being explicit about the hash syntax({ 'foo' => 'bar' } instead of { foo: 'bar'}), but that may have other implications. I'll do more testing and research on this.

@mhashizume
Copy link
Contributor Author

When attempting to add new keyword arguments to the get_request and put_request methods, I'm running into issues with different versions of Ruby and how they deal with the separation of positional and keyword arguments.

The last argument of these methods (timeouts) is an optional hash that uses symbols as keys. In Ruby < 3.0, Ruby interprets the keys in the timeouts hash as keyword arguments if we add any keyword arguments to the get_request/put_request methods.

For example, running this rspec test on Ruby 2.7.8:

  1) Facter::Util::Resolvers::Http when windows #get_request behaves like a http request on windows when timeout is configured uses the desired value
     Failure/Error:
       def get_request(url, headers = {}, timeouts = {}, http_port: 80, proxy_addr: DEFAULT_HOST)
         make_request(url, headers, timeouts, 'GET', http_port, proxy_addr)

     ArgumentError:
       unknown keyword: :connection
     Shared Example Group: "a http request on windows" called from ./spec/facter/util/resolvers/http_spec.rb:163
     # ./lib/facter/util/resolvers/http.rb:32:in `get_request'
     # ./spec/facter/util/resolvers/http_spec.rb:131:in `block (4 levels) in <top (required)>'

Ruby >= 3.0 properly separates positional and keyword arguments:

$ bundle exec rspec ./spec/facter/util/resolvers/http_spec.rb:131
Run options: include {:locations=>{"./spec/facter/util/resolvers/http_spec.rb"=>[131]}}
..

Finished in 0.19973 seconds (files took 8.66 seconds to load)
2 examples, 0 failures

But because we still support Facter on Ruby >= 2.5, we need to account for Ruby < 3.0.

We could solve this issue by making sure that the keys in the timeouts hash are strings intead of symbols. However, other parts of the code access values in the hash by looking for symbol keys specifically (like in here) and get_request/put_request are public methods. The fact that those methods are public mean that while I think additive changes like new keyword arguments are probably okay, changes or removals risk breaking things for any users who may rely on those methods.

I'm going to instead try to implement this with a boolean positional argument instead which, while isn't as clean, seems easier to implement across Ruby versions and retains compatibility with these public methods.

See also: https://www.fastruby.io/blog/fix-sneaky-argument-error-when-upgrading-ruby.html

@mhashizume
Copy link
Contributor Author

Closing in favor of #2704

@mhashizume mhashizume closed this Apr 11, 2024
@mhashizume mhashizume deleted the FACT-636/main/cloud-proxy branch September 16, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants