feat: add proxy_ssl_context to Configuration for a separate proxy TLS context - #2658
Conversation
kubernetes-prow
Bot
added
do-not-merge/invalid-commit-message
kubernetes-prow
Bot
added
cncf-cla: yes
|
Welcome @emmanuel-adu! |
kubernetes-prow
Bot
added
the
size/L
… context Adds a proxy_ssl_context parameter to Configuration and threads it through RESTClientObject to urllib3.ProxyManager, so the HTTPS proxy's own TLS certificate can be validated independently of the destination (Kubernetes API server) TLS settings - the "HTTPS Proxy + HTTPS Destination" scenario urllib3 documents at: https://urllib3.readthedocs.io/en/stable/advanced-usage.html#https-proxy-https-destination kubernetes/client is generated from kubernetes-client/gen; this applies the equivalent of the companion gen-repo patch directly to the already-generated files, so the feature is usable now rather than waiting on the next full regeneration. Added an end-to-end test (kubernetes/e2e_test) using a mock HTTPS proxy and a mock HTTPS destination server, each with an independent self-signed CA - it doesn't require a live cluster, only openssl on PATH. Verified: without proxy_ssl_context, connecting through a proxy with a different CA than the destination fails with CERTIFICATE_VERIFY_FAILED; with it set to trust the proxy's CA, the connection succeeds and the destination is still validated against its own separate CA.
6985e88 to
f526d68
Compare
kubernetes-prow
Bot
added
release-note
|
/kind feature |
kubernetes-prow
Bot
added
kind/feature
|
|
||
|
|
||
| import copy | ||
| import ssl |
There was a problem hiding this comment.
This file is generated by upstream openapi-generator. Please send a PR to update the upstream generator to properly support this feature.
There was a problem hiding this comment.
Opened kubernetes-client/gen#308 to add proxy_ssl_context for the sync client, following the existing pattern, and this PR applies that generated change now.
Or are you asking for this to be implemented in OpenAPITools/openapi-generator itself instead?
There was a problem hiding this comment.
better to add to OpenAPITools/openapi-generator
modules/openapi-generator/src/main/resources/python-pydantic-v1/configuration.mustache
modules/openapi-generator/src/main/resources/python/configuration.mustache
There was a problem hiding this comment.
Okay we'll look into doing that
There was a problem hiding this comment.
Opened and merged OpenAPITools/openapi-generator#24539 - adds proxy_ssl_context to both templates you flagged, gated the same way as tls_server_name/disable_strict_ssl_verification.
Given that, do we want to merge this PR now to ship the fix, or hold off and pick it up later via a full regen (bump the pinned generator commit in gen, rerun autoupdate.sh)? Either way works cleanly - if you go the regen route later, this PR's changes just get superseded by that regen with no conflict.
|
sounds good, thanks. please add the corresponding e2e test for asyncio client. |
|
How would you like to proceed?
|
| connection_pool_maxsize: Optional[int]=None, | ||
| proxy: Optional[str]=None, | ||
| proxy_headers: Optional[Any]=None, | ||
| proxy_ssl_context: Optional[ssl.SSLContext]=None, |
There was a problem hiding this comment.
does it apply to aio client? the upstream openapi generator PR says it does not
There was a problem hiding this comment.
Correct - OpenAPITools/openapi-generator#24539 only touches the python and python-pydantic-v1 (urllib3/sync) templates, not the aiohttp templates, so a future regen wouldn't produce this for the aio client on its own.
The aio support in this PR (configuration.py field + the TCPConnector override in rest.py) is hand-written, not generator-templated, because aiohttp has no public equivalent to urllib3's proxy_ssl_context - it required overriding a private TCPConnector method to give the proxy leg its own SSL context.
So it's the one piece of this PR not backed by the generator - it'd need to be manually reapplied (or ported to an aiohttp template upstream) after any future regen. Happy to drop it and keep this PR scoped to the sync client if you'd rather not carry hand-maintained code here - your call.
|
@emmanuel-adu regen does not have the test, right? i think we want to merge this PR, please take a look at the question about aio, if it is needed. |
|
@yliaog Correct - regen wouldn't add the e2e test on its own, e2e tests are hand-maintained in this repo, not templated by the generator (same as the rest of the test suite). Replied inline on the aio question - the upstream generator PR doesn't touch the aiohttp templates, so the aio changes here are hand-written and not backed by regen. Left it to you whether to keep or drop that part. |
|
/close |
|
@yliaog: Closed this PR. DetailsIn response to this:
Instructions 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. |
|
/reopen |
|
@yliaog: Reopened this PR. DetailsIn response to this:
Instructions 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. |
|
@emmanuel-adu we can merge the first commit, could you please drop the other 3 commits in the PR? so we can focus on the fix for sync client. |
b61be45 to
f526d68
Compare
|
@yliaog Done - dropped the asyncio commits, this PR is now just the sync client fix ( |
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: emmanuel-adu, yliaog The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
6c710e4
into
kubernetes-client:master
3 participants
Fixes #2387.
Configurationgainsproxy_ssl_context: Optional[ssl.SSLContext] = None, passed through tourllib3.ProxyManagerinRESTClientObject. This lets the HTTPS proxy's TLS cert be validated independently of the destination (K8s API server) settings - the "HTTPS Proxy + HTTPS Destination" scenario urllib3 already supports natively.kubernetes/clientis generated from kubernetes-client/gen; companion PR kubernetes-client/gen#308 adds the same patch there. This PR applies it to the already-generated files so it's usable now.Verification:
kubernetes/e2e_test/test_issue_2387_proxy_ssl_context.pyspins up a mock HTTPS proxy and destination server with independent self-signed CAs (no live cluster needed). Withoutproxy_ssl_context: fails withCERTIFICATE_VERIFY_FAILED. With it set to trust the proxy's CA: succeeds, destination still validated separately.