fix: propagate permissions issues portforward #712
Conversation
|
Hi @jacobtomlinson , I’ve attempted to fix the issue mentioned in the PR description. Could you please take a look when you get a chance? Thank you |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 94.61% 94.50% -0.12%
==========================================
Files 29 33 +4
Lines 3141 5095 +1954
==========================================
+ Hits 2972 4815 +1843
- Misses 169 280 +111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobtomlinson
left a comment
jacobtomlinson
left a comment
There was a problem hiding this comment.
This looks good thanks!
Could I ask you to add a test that verifies this behaviour. I would start by finding a test that creates a temporary copy of the kubeconfig copy that. Then change token in the kubeconfig to something invalid and then trying to open a port forward
kr8s/_portforward.py
Outdated
| if ( | ||
| isinstance(e, httpx_ws.WebSocketUpgradeError) | ||
| and hasattr(e, "response") | ||
| and e.response.status_code in (401, 403) | ||
| ): | ||
| error_message = f"Permission denied: {e.response.status_code}" | ||
| with suppress(httpx.StreamClosed, httpx.ResponseNotRead): | ||
| await e.response.aread() | ||
| error_message = e.response.text | ||
| raise ServerError(error_message, response=e.response) from e |
There was a problem hiding this comment.
This bypasses the connection attempts. There may be cases where auth tokens expire and open_websocket will rotate them, so we don't want to raise here prematurely. Can you move this block inside the if connection_attempts > 5: section. That way we raise a ServerError if we have one, but a more generic ConnectionClosedError if we don't.
There was a problem hiding this comment.
Ummm yes thats makes sense, sure will do that
Sure, actually I had an testing script which does something like this will add the same in to /test dir |
|
hey @jacobtomlinson
|
jacobtomlinson
left a comment
jacobtomlinson
left a comment
There was a problem hiding this comment.
Thanks for pushing the test. I left a few comments but overall there is a lot of unnecessary code in there.
I think you just need to:
- Create the temporary credentials with bad token
- Create the
api_invalid - Create the invalid Pod object (but not actually create the resource)
- Try to call the port forward and assert that it raises the
ServerError
| # Override the k8s_cluster fixture to avoid creating a Kind cluster | ||
| @pytest.fixture | ||
| def k8s_cluster(): | ||
| class MockCluster: | ||
| @property | ||
| def kubeconfig_path(self): | ||
| return Path(os.environ.get("KUBECONFIG", "~/.kube/config")).expanduser() | ||
|
|
||
| return MockCluster() |
There was a problem hiding this comment.
I assume you did this for local development? In CI we need to this run against the kind cluster.
| # Delete if exists | ||
| if await pod.exists(): | ||
| await pod.delete() | ||
| await pod.wait("condition=Deleted") | ||
|
|
||
| await pod.create() |
There was a problem hiding this comment.
Does the Pod actually need to exist? It never gets used other than to create pod_invalid.
| if __name__ == "__main__": | ||
|
|
||
| class MockCluster: | ||
| @property | ||
| def kubeconfig_path(self): | ||
| return Path(os.environ.get("KUBECONFIG", "~/.kube/config")).expanduser() | ||
|
|
||
| asyncio.run(test_portforward_invalid_token(MockCluster())) |
There was a problem hiding this comment.
In future instead of this you could just run pytest -k test_portforward_invalid_token and pytest will find the test and run it
3 participants
fixes: #540