Add support for network interfaces in requests#6207
Conversation
Fix for canonical#6205 Note: When using socket.SO_BINDTODEVICE in Python (e.g., with requests-toolbelt's SocketOptionsAdapter), the process requires the CAP_NET_RAW capability on Linux. This is because binding a socket to a specific network interface (iface) is a privileged operation.
|
I just noticed the dependency. New dependencies are strongly discouraged. Is this dependency already available in Linux distributions? Was an alternative approach considered? Why isn't it possible to do the same thing without the new dependency? |
|
Not commenting on the code itself, but I think this approach is the right one until such a time as requests handles scoped link-local addresses correctly. Given the general tone of psf/requests#6927 I wouldn't count on that happening any time soon. The additional dependency is somewhat problematic, though. Requests-toolbelt is available in Debian and Fedora, but doesn't seem to be included in Fedora-derivatives (checked Rocky 8 and 9, anyway). |
|
@nmeyerhans |
|
@xiachen-rh Can we do some testing on this change on RHEL-10 and using the same repro case we had before? Also does this fix your test case psf/requests#6735 (comment) ? |
|
@ani-sinha OK, let me test it |
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
github-actions
Bot
added
the
stale-pr
|
@ani-sinha I tested this changes on OpenStack env with ipv6 only network, cloud-init can get metadata via ipv6, so cloud-init can work without changing the code of 'requests'. |
@holmanb what are we going to do with this PR? I have not followed the fix closely myself so can't comment. Do you think we can merge it to cloud-init? |
github-actions
Bot
removed
the
stale-pr
Someone has to review |
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
github-actions
Bot
added
the
stale-pr
TheRealFalcon
removed
the
stale-pr
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
This seems like it can be a workable solution. I left some comments inline.
Additionally, I'd like to see some testing with a test case that fails without this change, but passes with the new change.
| global _DNS_REDIRECT_IP | ||
| parsed_url = parse.urlparse(url) | ||
| name = parsed_url.hostname | ||
| parsed_url = url_parser_plus(url) |
There was a problem hiding this comment.
I don't see why any of the changes to this function are needed. The call to net.is_ip_address should return true for any ipv6 address, including those that are interface scoped. Since we strip the [] before passing it in, there shouldn't be a problem here.
If that fails, then we should have an actual non-IP hostname, so the call to socket.getaddrinfo also shouldn't need any changes.
Am I missing something?
| if headers_cb: | ||
| headers = headers_cb(url) | ||
|
|
||
| headers["Host"] = parsed_url.hostname |
There was a problem hiding this comment.
Why is this line needed?
| TRUE_STRINGS = ("true", "1", "on", "yes") | ||
| FALSE_STRINGS = ("off", "0", "no", "false") | ||
|
|
||
| class URLParserPlus: |
There was a problem hiding this comment.
Rather than a whole bucket of new parsing, I'd rather we just extract the iface from already parsed host. See my comment in url_helper.py. I don't think we need this new class.
| if session is None: | ||
| session = requests.Session() | ||
|
|
||
| if iface is not None: |
There was a problem hiding this comment.
I think we can remove all of the changes to the top of this function and add something like this above the if condition here:
parsed_url = urlparse(url)
iface = parsed_url.hostname.split("%")[1] if parsed_url.hostname else None
|
@rezaf28 , also, before we can accept your change, you need to sign the CLA. Please note that Canonical's CLA process has changed recently, so if you signed a long time ago, you are required to resign in order to contribute. |
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
github-actions
Bot
added
the
stale-pr
6 participants
This change modifies the metadata retrieval logic to handle IPv6 link-local
addresses that require a specific network interface (scope_id). Previously,
cloud-init failed to connect when using addresses like [fe80::a9fe:a9fe%25{iface}] due
to improper handling in the underlying requests layer.
By introducing a scoped connection handling approach, this fix enables
cloud-init to operate correctly in IPv6-only OpenStack environments
and similar setups where metadata services are accessible via
scoped link-local addresses.
Summary of Patch Fixes:
Interface in URL: Added proper support for link-local IPv6 addresses with network interface identifiers (e.g.,
%eth0), enabling reliable socket binding usingSO_BINDTODEVICE.Enhanced URL Parsing: Improved URL parser to correctly handle hostnames, ports (with defaults), interfaces, and support for both
%and%25encoded formats across various schemes includinghttp,https,ftp,sftp, andfile.Fixes #6205