fix: propagate permissions issues portforward #712

SWITCHin2 · 2025-12-04T19:28:57Z

fixes: #540

SWITCHin2 · 2025-12-05T06:10:41Z

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 · 2025-12-05T10:08:01Z

Codecov Report

❌ Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.50%. Comparing base (87063fc) to head (e3f26d7).
⚠️ Report is 286 commits behind head on main.

Files with missing lines	Patch %	Lines
kr8s/_portforward.py	22.22%	7 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jacobtomlinson

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

jacobtomlinson · 2025-12-05T10:11:30Z

kr8s/_portforward.py

+                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


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.

Ummm yes thats makes sense, sure will do that

SWITCHin2 · 2025-12-06T03:23:40Z

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

Sure, actually I had an testing script which does something like this will add the same in to /test dir

for more information, see https://pre-commit.ci

SWITCHin2 · 2025-12-06T04:10:08Z

hey @jacobtomlinson

have handled premature error throwing case as you mentioned
modified my test script as per you said , pushed them and also fixed some pre checks

jacobtomlinson

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

jacobtomlinson · 2025-12-08T15:45:40Z

kr8s/tests/test_portforward_auth.py

+# 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()


I assume you did this for local development? In CI we need to this run against the kind cluster.

yes I was using this for my local dev test,
sure have incorporated that

jacobtomlinson · 2025-12-08T15:48:31Z

kr8s/tests/test_portforward_auth.py

+    # Delete if exists
+    if await pod.exists():
+        await pod.delete()
+        await pod.wait("condition=Deleted")
+
+    await pod.create()


Does the Pod actually need to exist? It never gets used other than to create pod_invalid.

Yes that makes sense

jacobtomlinson · 2025-12-08T15:49:26Z

kr8s/tests/test_portforward_auth.py

+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()))


In future instead of this you could just run pytest -k test_portforward_invalid_token and pytest will find the test and run it

for more information, see https://pre-commit.ci

SWITCHin2 · 2025-12-11T07:34:27Z

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

@jacobtomlinson i have tried to sanitise the test file and in-corporate your review comments, please verify and let me know if everything seems fine now.

jacobtomlinson

A couple more comments on the tests, but otherwise this looks good to go.

Could I also ask you to put these tests in the existing test_objects.py file next to the other port forward tests instead of creating a new one. AI code tools love to create new test files unnecessarily.

jacobtomlinson · 2025-12-11T10:13:40Z

kr8s/tests/test_portforward_auth.py

+    if k8s_cluster.kubeconfig_path.exists():
+        kubeconfig_data = yaml.safe_load(k8s_cluster.kubeconfig_path.read_text())
+    else:
+        pytest.skip("Kubeconfig file not found.")


We don't need to do this. If the kubeconfig doesn't exist then the whole test suite will be broken. You're safe to assume that it will exist.

jacobtomlinson · 2025-12-11T10:14:10Z

kr8s/tests/test_portforward_auth.py

+        pass
+
+    # Initialize a new API with the invalid kubeconfig
+    from kr8s._api import Api


Can we put all the imports at the top of the file?

jacobtomlinson · 2025-12-11T10:16:31Z

kr8s/tests/test_portforward_auth.py

+    original_check_version = Api._check_version
+
+    async def mock_check_version(self):
+        return
+
+    Api._check_version = mock_check_version
+
+    try:
+        api_invalid = await kr8s.asyncio.api(kubeconfig=kubeconfig_data)
+    finally:
+        Api._check_version = original_check_version


Using the mock patch context manager would hugely simplify this.

SATYAsasini and others added 2 commits December 5, 2025 00:53

feat: raise exception for unathenticated port-forward

3a3e9e6

Merge branch 'kr8s-org:main' into main

e3f26d7

github-actions bot added the kr8s label Dec 4, 2025

jacobtomlinson reviewed Dec 5, 2025

View reviewed changes

fix: incorporate review comment and add test script

6ff44f3

github-actions bot added the tests label Dec 6, 2025

pre-commit-ci bot and others added 3 commits December 6, 2025 03:45

[pre-commit.ci] auto fixes from pre-commit.com hooks

5771a2f

for more information, see https://pre-commit.ci

merge conflict resolve

518544c

fix: add end comment in test script

5ac75cd

jacobtomlinson requested changes Dec 8, 2025

View reviewed changes

SATYAsasini and others added 4 commits December 11, 2025 12:37

fix: remove unneccesary codes in test file

1771398

[pre-commit.ci] auto fixes from pre-commit.com hooks

ccc82b0

for more information, see https://pre-commit.ci

fix: linting isue and unused import

15f6178

[pre-commit.ci] auto fixes from pre-commit.com hooks

3602d95

for more information, see https://pre-commit.ci

jacobtomlinson reviewed Dec 11, 2025

View reviewed changes

fix: propagate permissions issues portforward #712

Are you sure you want to change the base?

fix: propagate permissions issues portforward #712

Uh oh!

Conversation

SWITCHin2 commented Dec 4, 2025

Uh oh!

SWITCHin2 commented Dec 5, 2025

Uh oh!

codecov bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SWITCHin2 commented Dec 6, 2025

Uh oh!

SWITCHin2 commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SWITCHin2 commented Dec 11, 2025

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 5, 2025 •

edited

Loading

SWITCHin2 commented Dec 6, 2025 •

edited

Loading