Preserve SHOTGUN_API_CACERTS precedence in shotgun_api3 cert patch#1122
Preserve SHOTGUN_API_CACERTS precedence in shotgun_api3 cert patch#1122stevelittlefish wants to merge 2 commits into
Conversation
_patch_shotgun_api3_certs() unconditionally returned the Core-extracted cacert.pem whenever ca_certs wasn't explicitly passed, silently ignoring the documented SHOTGUN_API_CACERTS environment variable. When the extracted cert lives on a slow network share, this made every cold ShotGrid connection pay a large one-time filesystem cost (~20-30s observed in Nuke 17) that a local cert copy avoids. Restore shotgun_api3's documented precedence: explicit ca_certs -> SHOTGUN_API_CACERTS -> extracted Core cert fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
carlos-villavicencio-adsk
left a comment
carlos-villavicencio-adsk
left a comment
There was a problem hiding this comment.
LGTM. Just one comment to be considered.
| environment_certs = os.environ.get("SHOTGUN_API_CACERTS") | ||
| if environment_certs: |
There was a problem hiding this comment.
Once we support Python 3.9, I wonder if it would be a good idea to start using the walrus operator 🤔
There was a problem hiding this comment.
I prefer not to. Not because there's anything wrong with it, but it's just a weird bit of syntax that lots of developers are not familiar with.
| # Preserve shotgun_api3's documented SHOTGUN_API_CACERTS override. | ||
| # Without this branch, a network-hosted extracted cert_file can be | ||
| # dramatically slower to open than a local override (see SG-44256). | ||
| environment_certs = os.environ.get("SHOTGUN_API_CACERTS") |
There was a problem hiding this comment.
Do you think it's a good idea to validate if the env var value is an actual file path instead of anything?
There was a problem hiding this comment.
No, I'd leave it unvalidated. Two reasons:
- Consistency with upstream. Look at shotgun_api3's own original _get_certs_file: when ca_certs is passed explicitly, it also returns it as-is with zero validation (if ca_certs is not None: return ca_certs). Our env-var branch is meant to restore that same documented precedence, not add stricter behavior than the thing it's mimicking.
- Silent fallback would hide misconfiguration. If we validated and silently fell back to the extracted cert on a bad path, someone who typos SHOTGUN_API_CACERTS would get no error — just the slow network-cert behavior again, with no signal as to why their override "isn't working." Letting a bad path flow through means SSL fails loudly at connection time, pointing straight at the actual problem.
If we wanted to help catch typos without changing the fallback semantics, a logger.warning() when the path doesn't exist (but still returning it, not falling back) would be a reasonable middle ground — but that's a scope increase beyond this bug fix, and this module doesn't have a logger wired in already. I'd leave it as-is unless you want me to add that warning.
Human version - this is consistent with everything else, and handling the validation is probably going to cause more problems than it solves.
There was a problem hiding this comment.
Pull request overview
This pull request restores shotgun_api3’s documented CA cert precedence in tank_vendor._patch_shotgun_api3_certs() so that SHOTGUN_API_CACERTS is honored when no explicit ca_certs argument is provided, avoiding unnecessary slow reads of the Core-extracted cert in some environments.
Changes:
- Update the
shotgun_api3cert patch to respectSHOTGUN_API_CACERTSwhenca_certsis not explicitly provided. - Add a new test suite validating precedence across: explicit
ca_certsarg, env var (including empty string), and extracted cert fallback.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/core_tests/test_tank_vendor.py | Adds a dedicated test class to validate cert precedence behavior for the shotgun_api3 cert patch. |
| python/tank_vendor/init.py | Adjusts _patched_get_certs_file() to preserve SHOTGUN_API_CACERTS precedence before falling back to the extracted cert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Snapshot the method currently installed (already patched once at | ||
| # module import time) so we can restore it after each test, and so | ||
| # repeated patch applications in this test class don't stack. | ||
| self._original_get_certs_file = shotgun_api3.Shotgun._get_certs_file | ||
|
|
setUp() captured shotgun_api3.Shotgun._get_certs_file via attribute access, which unwraps the staticmethod descriptor to a plain function. Restoring that plain function in tearDown re-assigned it as a regular instance method, so any Shotgun() constructed by a later test in the same process passed both self and ca_certs into a function that only accepts ca_certs, breaking unrelated tests (e.g. util_tests/test_shotgun_connect.py) with: TypeError: _patched_get_certs_file() takes from 0 to 1 positional arguments but 2 were given Snapshot the raw descriptor from Shotgun.__dict__ instead, so the staticmethod wrapper is preserved on restore. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| # turn _get_certs_file into a bound instance method for every | ||
| # Shotgun() call afterward, breaking unrelated tests with a | ||
| # "takes 0 to 1 positional arguments but 2 were given" TypeError. | ||
| self._original_get_certs_file = shotgun_api3.Shotgun.__dict__[ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1122 +/- ##
=======================================
Coverage 80.10% 80.10%
=======================================
Files 203 203
Lines 19529 19529
=======================================
Hits 15643 15643
Misses 3886 3886
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
_patch_shotgun_api3_certs()intank_vendor/__init__.pycurrently returns the Core-extractedcacert.pemunconditionally wheneverca_certsisn't explicitly passed, silently bypassing the documentedSHOTGUN_API_CACERTSenvironment variable.shotgun_api3's own documented precedence: explicitca_certsargument →SHOTGUN_API_CACERTSenv var → extracted Core cert fallback.Test plan
TestShotgunAPI3CertPatchPrecedenceintests/core_tests/test_tank_vendor.py, calling_patch_shotgun_api3_certs()directly against a throwaway fakepkgs.zip/certs/layout so behavior can be verified without depending on the real build-time layout:SHOTGUN_API_CACERTSset → wins over the extracted cert (the regression this fixes)ca_certsargument → still outranks everything, including the env varcore_testssuite (510 tests) passes with the fix applied, no regressions.Follow-up fix (review feedback)
CI/review caught that the new test's
tearDownwas leaking global state across test modules when the full suite ran in one process:Root cause:
setUpcapturedshotgun_api3.Shotgun._get_certs_filevia plain attribute access, which unwraps the installedstaticmethoddescriptor down to a raw function.tearDownthen reassigned that raw function directly as the class attribute, turning_get_certs_fileback into a regular instance method. AnyShotgun(...)constructed by a later test in the same process (e.g.tests/util_tests/test_shotgun_connect.py) then calledself._get_certs_file(ca_certs), which passed bothselfandca_certsinto a function that only acceptsca_certs.Fix: snapshot the raw descriptor via
shotgun_api3.Shotgun.__dict__["_get_certs_file"]instead of attribute access, so thestaticmethodwrapper is preserved on restore.core_tests.test_tank_vendor+util_tests.test_shotgun_connectin the same process (matches how the full suite runs).