Fix: correect header retrieval for urllib3 compatibility#2479
Fix: correect header retrieval for urllib3 compatibility#2479franck-ada wants to merge 1 commit intokubernetes-client:masterfrom
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: franck-ada The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR fixes header retrieval in the RESTResponse class to support urllib3 v2.x compatibility. The PR addresses issue #2280 by adding compatibility checks that prefer the newer headers attribute (available in urllib3 v2.x) before falling back to the deprecated getheaders() and getheader() methods.
Note: The PR title contains a typo: "correect" should be "correct".
Key changes:
- Added
hasattr()checks to detect the presence of theheadersattribute on urllib3 response objects - Updated
getheaders()to useheaders.items()when available, falling back togetheaders()for older versions - Updated
getheader()to useheaders.get()when available, falling back togetheader()for older versions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -41,10 +41,14 @@ def __init__(self, resp): | |||
|
|
|||
| def getheaders(self): | |||
| """Returns a dictionary of the response headers.""" | |||
There was a problem hiding this comment.
The docstring states "Returns a dictionary of the response headers" but the method returns headers.items() which returns an iterable of tuples (key-value pairs), not a dictionary. Consider updating the docstring to accurately reflect the return type, such as "Returns an iterable of header key-value pairs" or "Returns header items as tuples".
| """Returns a dictionary of the response headers.""" | |
| """Returns an iterable of header key-value pairs (tuples).""" |
- add support for headers
a602e3f to
7a59580
Compare
|
Here is another occurrence of python/kubernetes/client/exceptions.py Line 91 in a49d85d From my sniff testing, this needs to be changed as well. |
I was looking at it. this |
@franck-ada I was testing an ansible playbook with urllib3 2.6.0 patched like this: When trying to delete a configmap that doesn't exist through ansible's I had to add this additional patch to get past this error: Unfortunately, I didn't find a way yet to have ansible print the full stack trace. |
|
the file kubernetes/client/rest.py is generated, the fix needs to be done somewhere else, otherwise, it will be overwritten during next release. |
|
Thanks for working on this! Few things that might help: The repo already has scripts/rest_urllib_headers.diff which patches exceptions.py to use .headers instead of .getheaders(). Gets applied via apply-hotfixes.sh. But theres still the RESTResponse.getheaders() method in rest.py (~line 60) that calls self.urllib3_response.getheaders() which will also fail with urllib3 >= 2.6.0. This PR addresses it but as yliaog mentioned rest.py is auto-generated so changes get overwritten. Maybe we could add another patch file like rest_getheaders_compat.diff? Same pattern as existing hotfixes and survives regeneration. Something like: --- a/kubernetes/client/rest.py
This is blocking us from updating to urllib3 >= 2.6.3 which has security fixes. Happy to help test or submit a followup PR for the patch file approach if useful |
Disregard. I found discussion in #2497. |
What type of PR is this?
/kind bug
/kind deprecation
What this PR does / why we need it:
Correct issue link to upgrade of urlib3
Which issue(s) this PR fixes:
Fixes #2280 / #2477
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE