feat: add internal token rotation for clients - #695
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughImplements configurable OIDC token lifetime and full token rotation: adds RotateToken RPC, backend handler that signs and rotates client credential secrets, Kubernetes client rotation helper, Python SDK and CLI integrations, and unit/integration/e2e tests. ChangesToken rotation feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.py (1)
75-106: ⚡ Quick winMock
asyncio.sleepin timeout test to avoid real 10s wait.This test currently waits through the full polling window; patching sleep keeps coverage while reducing CI latency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.py` around lines 75 - 106, The test test_rotate_client_token_timeout currently performs real waits; patch asyncio.sleep inside the test (before awaiting api.rotate_client_token) to a no-op async function or AsyncMock so the polling loop in ClientsV1Alpha1Api.rotate_client_token runs without real delays; ensure the mock is applied (and restored if needed) so the test still triggers the timeout logic while avoiding the real 10s sleeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/internal/config/oidc.go`:
- Around line 28-34: The TokenLifetime parsing branch allows negative or zero
durations which can produce already-expired tokens; in the block where you call
ParseDuration on config.Internal.TokenLifetime and then signer.SetTokenLifetime
(the TokenLifetime handling code using ParseDuration and
signer.SetTokenLifetime), validate the parsed lifetime and reject non-positive
values by returning an error (with a clear message) instead of setting the
signer; ensure the error is returned to the caller so callers cannot proceed
with a zero or negative TokenLifetime.
In `@controller/internal/service/client/v1/client_service.go`:
- Around line 421-423: When rotating the credential token, do not replace
secret.Data entirely; instead preserve existing keys and only set/update the
"token" entry. Locate the code using the secret variable and replace the
assignment secret.Data = map[string][]byte{ "token": []byte(token) } with logic
that ensures secret.Data is non-nil (if secret.Data == nil { secret.Data =
map[string][]byte{} }) and then updates the single key (secret.Data["token"] =
[]byte(token)), preserving any other keys before continuing with the existing
save/update call.
- Around line 395-407: The RotateToken method dereferences req and s.Signer
without nil checks which can cause panics; add a guard at the start of
RotateToken to return a descriptive error if req == nil, and check s.Signer (or
s.Signer.Token usage) before calling s.Signer.Token to return an error if the
signer is nil; ensure you still parse namespace via
utils.ParseNamespaceIdentifier and call s.AuthClient only after these guards so
the flow (namespace parsing, AuthClient, s.Signer.Token with
jclient.InternalSubject()) remains unchanged otherwise.
In `@python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate.py`:
- Around line 79-82: The code currently only catches ApiException and
ConfigException around rotate_client_token, but rotate_client_token can raise
plain Exceptions (e.g., timeout/no credential) causing raw CLI tracebacks; add a
final broad except Exception as e clause after the existing except blocks to
catch non-Kubernetes errors from rotate_client_token and handle them
gracefully—either call a new or existing error handler (e.g.,
handle_generic_rotation_exception(e) or reuse processLogger.error plus
sys.exit(1)) so the CLI logs a clear message and exits cleanly; reference
rotate_client_token, handle_k8s_api_exception, and handle_k8s_config_exception
when updating the try/except block.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/auth.py`:
- Around line 149-159: The code currently assigns and persists the rotated token
immediately (config.token = new_token; ClientConfigV1Alpha1.save(config)) before
validating it; instead, validate new_token first by ensuring it's non-empty,
that get_token_remaining_seconds(new_token) returns a non-None value and that
decode_jwt(new_token) yields an expected payload (e.g., contains "exp") before
mutating config and calling ClientConfigV1Alpha1.save; if validation fails, do
not set config.token or save, surface an error (raise or click.echo and exit)
and avoid persisting a malformed token—make these checks around the result of
rotate_token() and only persist on successful validation, referencing
rotate_token, new_token, get_token_remaining_seconds, decode_jwt, config.token,
and ClientConfigV1Alpha1.save.
In `@python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py`:
- Around line 166-168: The code in the block that calls await
self.get_client(name) dereferences client.status.credential and can raise
AttributeError if client.status is None; update this check in the method (the
caller of get_client) to first ensure client.status is not None (e.g., if
client.status is None or getattr(client.status, "credential", None) is None) and
then raise the intended Exception(f"Client '{name}' has no credential secret")
when missing; alternatively wrap the access in a safe getattr or try/except to
convert AttributeError into the same user-facing exception while referencing the
existing client, get_client and client.status symbols.
- Around line 175-180: The current polling loop swallows all exceptions (the
try/except around self.core_api.read_namespaced_secret), which hides real
errors; replace the broad except Exception with targeted handling: catch
kubernetes.client.rest.ApiException (or the specific k8s API exception type) and
only ignore/continue for expected transient statuses (e.g., 404 NotFound while
the secret is not yet created), but for any other ApiException or any other
exception, log the error (use the existing logger or raise) and re-raise so real
failures surface; keep the logic that decodes secret.data["token"] unchanged and
ensure you import the ApiException type used for discrimination.
---
Nitpick comments:
In
`@python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.py`:
- Around line 75-106: The test test_rotate_client_token_timeout currently
performs real waits; patch asyncio.sleep inside the test (before awaiting
api.rotate_client_token) to a no-op async function or AsyncMock so the polling
loop in ClientsV1Alpha1Api.rotate_client_token runs without real delays; ensure
the mock is applied (and restored if needed) so the test still triggers the
timeout logic while avoiding the real 10s sleeps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41dd6d7a-a098-4b11-b5ef-9f7dd949ee06
⛔ Files ignored due to path filters (5)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/client/v1/client.pb.gw.gois excluded by!**/*.pb.gw.gocontroller/internal/protocol/jumpstarter/client/v1/client_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/jumpstarter_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/router_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
controller/cmd/main.gocontroller/internal/config/oidc.gocontroller/internal/oidc/op.gocontroller/internal/service/client/v1/client_service.gocontroller/internal/service/controller_service.goprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/__init__.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/auth.pypython/packages/jumpstarter-cli/jumpstarter_cli/auth_test.pypython/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.pypython/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/config/client.py
bennyz
force-pushed
the
internal-secret-rotate
branch
from
May 19, 2026 17:18
cdbf6f2 to
519f658
Compare
Member
|
Thanks benny!, this is awesome, can we add a bunch of E2E tests to verify admin and client rotation?, leave docs for later as @raballew is doing a small doc refactor :) |
mangelajo
reviewed
May 20, 2026
mangelajo
left a comment
mangelajo
left a comment
Member
There was a problem hiding this comment.
can we add some E2E tests for the admin and client sides? looks awesome
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/test/e2e_test.go (1)
286-290: ⚡ Quick winStrengthen the negative-path assertion for missing client.
This test currently passes for any failure reason. Assert an expected error substring (e.g., not found) to ensure it validates the intended behavior.
Suggested test tightening
It("admin rotate fails for non-existent client", func() { ns := Namespace() - _, err := Jmp("admin", "rotate", "client", "-n", ns, "does-not-exist") + out, err := Jmp("admin", "rotate", "client", "-n", ns, "does-not-exist") Expect(err).To(HaveOccurred()) + Expect(strings.ToLower(out)).To(ContainSubstring("not found")) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test/e2e_test.go` around lines 286 - 290, The test It("admin rotate fails for non-existent client" currently only asserts that an error occurred; tighten it to assert the error message contains an expected substring (e.g., "not found") so the failure is for the missing client and not some other reason: after calling Jmp("admin", "rotate", "client", "-n", ns, "does-not-exist") update the assertion to check the error string (for example using Expect(err).To(MatchError(ContainSubstring("not found"))) or Expect(err).To(HaveOccurred()); Expect(err.Error()).To(ContainSubstring("not found"))), referencing the test case and the Jmp call to locate where to change the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/internal/oidc/op_test.go`:
- Around line 118-121: The test currently ignores the error returned by
s1.Token("subject"); update the test so you capture and assert the error from
s1.Token (e.g., check err != nil and call t.Fatalf/t.Fatal with the error)
before calling s2.Validate, so failures in s1.Token are reported clearly; refer
to s1.Token, token1, err and s2.Validate to locate and adjust the test logic.
---
Nitpick comments:
In `@e2e/test/e2e_test.go`:
- Around line 286-290: The test It("admin rotate fails for non-existent client"
currently only asserts that an error occurred; tighten it to assert the error
message contains an expected substring (e.g., "not found") so the failure is for
the missing client and not some other reason: after calling Jmp("admin",
"rotate", "client", "-n", ns, "does-not-exist") update the assertion to check
the error string (for example using
Expect(err).To(MatchError(ContainSubstring("not found"))) or
Expect(err).To(HaveOccurred()); Expect(err.Error()).To(ContainSubstring("not
found"))), referencing the test case and the Jmp call to locate where to change
the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9df606f5-d70a-49ad-9278-f77de50bca5c
⛔ Files ignored due to path filters (5)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/client/v1/client.pb.gw.gois excluded by!**/*.pb.gw.gocontroller/internal/protocol/jumpstarter/client/v1/client_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/jumpstarter_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/router_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
controller/cmd/main.gocontroller/internal/config/oidc.gocontroller/internal/oidc/op.gocontroller/internal/oidc/op_test.gocontroller/internal/service/client/v1/client_service.gocontroller/internal/service/controller_service.goe2e/test/e2e_test.goprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/__init__.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/auth.pypython/packages/jumpstarter-cli/jumpstarter_cli/auth_test.pypython/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.pypython/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/config/client.py
✅ Files skipped from review due to trivial changes (3)
- python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/init.py
- python/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py
- python/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
raballew
requested changes
May 20, 2026
Comment on lines
+183
to
+184
| except Exception: | ||
| raise |
Member
There was a problem hiding this comment.
Isnt this functionally identical to not having the handler at all?
Wire the existing but unused tokenLifetime config into Signer.Token(), add a RotateToken RPC to ClientService so clients can rotate their own internal JWT without being deleted and recreated, and add an admin path (jmp admin rotate client) that works via K8s API for expired tokens. - `jmp auth rotate` client self-rotates via gRPC (needs valid token) - `jmp admin rotate client <name> --save` admin rotates via K8s API
bennyz
force-pushed
the
internal-secret-rotate
branch
from
May 21, 2026 05:42
0f16816 to
b338136
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controller/internal/service/client/v1/client_service.go (1)
396-408:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd nil guards before request/signer dereference.
Line 397 and Line 407 can panic when
reqors.Signeris nil. Return gRPC status errors instead.Proposed fix
func (s *ClientService) RotateToken(ctx context.Context, req *cpb.RotateTokenRequest) (*cpb.RotateTokenResponse, error) { + if req == nil { + return nil, status.Error(codes.InvalidArgument, "request is required") + } + if s.Signer == nil { + return nil, status.Error(codes.FailedPrecondition, "token signer is not configured") + } + namespace, err := utils.ParseNamespaceIdentifier(req.Parent) if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/internal/service/client/v1/client_service.go` around lines 396 - 408, In RotateToken, add nil guards for the incoming request and the service signer: check that req is not nil before accessing req.Parent and return a gRPC status error (e.g., codes.InvalidArgument) if it is, and check that s.Signer is not nil before calling s.Signer.Token and return an appropriate gRPC status error (e.g., codes.FailedPrecondition or codes.Unavailable) if missing; update the beginning of RotateToken (function RotateToken, references to req.Parent and s.Signer.Token) to perform these checks and return clear status.Error responses instead of allowing a panic.
🧹 Nitpick comments (2)
python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate_test.py (1)
86-92: ⚡ Quick winStrengthen the name-only output expectation.
Line 91 only proves status text is suppressed; it doesn’t verify the promised name-only output. Assert the expected name to prevent silent regressions.
Proposed test tightening
def test_rotate_client_name_only_output(mock_rotate, _mock_kube): """--output name prints only name.""" runner = CliRunner() result = runner.invoke(rotate, ["client", "my-client", "--output", "name"]) assert result.exit_code == 0 - assert "Rotating" not in result.output + assert result.output.strip() == "my-client"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate_test.py` around lines 86 - 92, Update test_rotate_client_name_only_output to not only assert absence of status text but also verify the actual name-only output; after invoking rotate (in test_rotate_client_name_only_output and using the same runner.invoke call), add an assertion that the CLI output equals the expected resource name ("my-client") when trimmed (i.e. strip whitespace/newline) so the test fails on any extra text or missing name, while keeping the existing assert that "Rotating" is not in result.output.e2e/test/e2e_test.go (1)
286-290: ⚡ Quick winAssert the expected failure reason for the missing-client case.
Line 289 currently accepts any error, so unrelated failures can pass this test. Check for a not-found style message to verify the intended behavior.
Proposed assertion improvement
It("admin rotate fails for non-existent client", func() { ns := Namespace() - _, err := Jmp("admin", "rotate", "client", "-n", ns, "does-not-exist") + out, err := Jmp("admin", "rotate", "client", "-n", ns, "does-not-exist") Expect(err).To(HaveOccurred()) + Expect(strings.ToLower(out)).To(ContainSubstring("not found")) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test/e2e_test.go` around lines 286 - 290, The test "admin rotate fails for non-existent client" currently only checks that an error occurred; change the assertion to validate the failure reason by asserting the returned error from Jmp("admin", "rotate", "client", "-n", ns, "does-not-exist") contains a not-found message (e.g., "not found" or "does not exist"). Replace the loose Expect(err).To(HaveOccurred()) with a stricter assertion like Expect(err).To(MatchError(ContainSubstring("not found"))) or Expect(err.Error()).To(ContainSubstring("does not exist")) so the test ensures the specific missing-client error path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py`:
- Around line 172-183: In rotate_client_token, after calling
core_api.delete_namespaced_secret, change the polling logic around
core_api.read_namespaced_secret so that you do not accept the first successful
read as the new token; instead require that at least one ApiException with
status 404 has been observed after deletion before returning a Secret.token.
Update the loop in rotate_client_token (which currently uses
count/CREATE_CLIENT_COUNT and catches ApiException) to track a boolean like
sawNotFound that flips true when e.status == 404, and only decode/return
base64.b64decode(secret.data["token"]).decode("utf8") if sawNotFound is true and
secret.data contains "token"; still re-raise non-404 ApiExceptions and keep the
existing timeout/count behavior.
---
Duplicate comments:
In `@controller/internal/service/client/v1/client_service.go`:
- Around line 396-408: In RotateToken, add nil guards for the incoming request
and the service signer: check that req is not nil before accessing req.Parent
and return a gRPC status error (e.g., codes.InvalidArgument) if it is, and check
that s.Signer is not nil before calling s.Signer.Token and return an appropriate
gRPC status error (e.g., codes.FailedPrecondition or codes.Unavailable) if
missing; update the beginning of RotateToken (function RotateToken, references
to req.Parent and s.Signer.Token) to perform these checks and return clear
status.Error responses instead of allowing a panic.
---
Nitpick comments:
In `@e2e/test/e2e_test.go`:
- Around line 286-290: The test "admin rotate fails for non-existent client"
currently only checks that an error occurred; change the assertion to validate
the failure reason by asserting the returned error from Jmp("admin", "rotate",
"client", "-n", ns, "does-not-exist") contains a not-found message (e.g., "not
found" or "does not exist"). Replace the loose Expect(err).To(HaveOccurred())
with a stricter assertion like Expect(err).To(MatchError(ContainSubstring("not
found"))) or Expect(err.Error()).To(ContainSubstring("does not exist")) so the
test ensures the specific missing-client error path is exercised.
In `@python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate_test.py`:
- Around line 86-92: Update test_rotate_client_name_only_output to not only
assert absence of status text but also verify the actual name-only output; after
invoking rotate (in test_rotate_client_name_only_output and using the same
runner.invoke call), add an assertion that the CLI output equals the expected
resource name ("my-client") when trimmed (i.e. strip whitespace/newline) so the
test fails on any extra text or missing name, while keeping the existing assert
that "Rotating" is not in result.output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e287d6f-463d-4f84-80bd-38bbf87841f4
⛔ Files ignored due to path filters (5)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/client/v1/client.pb.gw.gois excluded by!**/*.pb.gw.gocontroller/internal/protocol/jumpstarter/client/v1/client_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/jumpstarter_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/router_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
controller/cmd/main.gocontroller/internal/config/oidc.gocontroller/internal/oidc/op.gocontroller/internal/oidc/op_test.gocontroller/internal/service/client/v1/client_service.gocontroller/internal/service/controller_service.goe2e/test/e2e_test.goprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/__init__.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/rotate_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/auth.pypython/packages/jumpstarter-cli/jumpstarter_cli/auth_test.pypython/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.pypython/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/config/client.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
Comment on lines
+172
to
+183
| await self.core_api.delete_namespaced_secret(secret_name, self.namespace) | ||
|
|
||
| count = 0 | ||
| while count < CREATE_CLIENT_COUNT: | ||
| try: | ||
| secret = await self.core_api.read_namespaced_secret(secret_name, self.namespace) | ||
| if secret.data and "token" in secret.data: | ||
| return base64.b64decode(secret.data["token"]).decode("utf8") | ||
| except ApiException as e: | ||
| if e.status != 404: | ||
| raise | ||
| count += 1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the referenced code with line numbers
sed -n '130,240p' python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py | nl -ba | sed -n '1,130p'
# 2) Find the function containing this loop and print a wider window for context
python - <<'PY'
import re, pathlib
path = pathlib.Path("python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py")
txt = path.read_text()
# Find likely function def near the referenced line number
lines = txt.splitlines()
for i in range(len(lines)):
if i+1 in range(160, 210) and re.match(r'\s*async\s+def\s+', lines[i-1] if i>0 else ""):
pass
# Just print all async defs around the region
for m in re.finditer(r'^(async\s+def\s+.*|def\s+.*)\s*:\s*$', txt, re.M):
start = txt[:m.start()].count("\n")+1
if 140 <= start <= 230:
print("\n=== Function starting at line", start, "===\n", m.group(0), "\n")
PY
# 3) Search for CREATE_CLIENT_COUNT / CREATE_CLIENT_DELAY usage and any secret rotation logic
rg -n "CREATE_CLIENT_COUNT|CREATE_CLIENT_DELAY|delete_namespaced_secret|read_namespaced_secret|rotate|stale token" python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes -SRepository: jumpstarter-dev/jumpstarter
Length of output: 114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py"
# 1) Print the referenced region with line numbers (avoid nl)
sed -n '130,240p' "$FILE" | cat -n
# 2) Print a wider window to capture surrounding logic (token rotation / creation)
sed -n '1,260p' "$FILE" | tail -n +110 | cat -n
# 3) Show exact definitions/uses of the retry constants and the secret read/delete loop area
rg -n "CREATE_CLIENT_COUNT|CREATE_CLIENT_DELAY|delete_namespaced_secret|read_namespaced_secret" "$FILE" -S
# 4) Look for any related rotation/secret update helpers in this package
rg -n "secret.*token|token.*secret|rotation|rotate" python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes -SRepository: jumpstarter-dev/jumpstarter
Length of output: 12782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/rotate_test.py"
# Show the whole test file (it's small enough; otherwise slice)
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -nRepository: jumpstarter-dev/jumpstarter
Length of output: 5809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py"
# show constants and rotate_client_token body precisely
sed -n '130,220p' "$FILE" | cat -n
# find if any other code handles stale token / wait for deletion elsewhere
rg -n "rotate_client_token|delete_namespaced_secret\\(|read_namespaced_secret\\(|observed_not_found" python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes -SRepository: jumpstarter-dev/jumpstarter
Length of output: 5617
Avoid returning the pre-delete (“stale”) token in rotate_client_token
rotate_client_token() deletes the Secret and then immediately polls read_namespaced_secret(), returning the first Secret that contains "token". Because Kubernetes Secret deletion is asynchronous, that first successful read can still observe the old Secret (pre-rotation token). The current tests don’t cover this race. Gate token return until at least one 404 has been observed after deletion.
Proposed fix
secret_name = client.status.credential.name
await self.core_api.delete_namespaced_secret(secret_name, self.namespace)
count = 0
+ observed_not_found = False
while count < CREATE_CLIENT_COUNT:
try:
secret = await self.core_api.read_namespaced_secret(secret_name, self.namespace)
- if secret.data and "token" in secret.data:
+ if observed_not_found and secret.data and "token" in secret.data:
return base64.b64decode(secret.data["token"]).decode("utf8")
except ApiException as e:
- if e.status != 404:
+ if e.status == 404:
+ observed_not_found = True
+ else:
raise
count += 1
await asyncio.sleep(CREATE_CLIENT_DELAY)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py`
around lines 172 - 183, In rotate_client_token, after calling
core_api.delete_namespaced_secret, change the polling logic around
core_api.read_namespaced_secret so that you do not accept the first successful
read as the new token; instead require that at least one ApiException with
status 404 has been observed after deletion before returning a Secret.token.
Update the loop in rotate_client_token (which currently uses
count/CREATE_CLIENT_COUNT and catches ApiException) to track a boolean like
sawNotFound that flips true when e.status == 404, and only decode/return
base64.b64decode(secret.data["token"]).decode("utf8") if sawNotFound is true and
secret.data contains "token"; still re-raise non-404 ApiExceptions and keep the
existing timeout/count behavior.
mangelajo
self-requested a review
May 21, 2026 08:45
raballew
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wire the existing but unused tokenLifetime config into Signer.Token(), add a RotateToken RPC to ClientService so clients can rotate their own internal JWT without being deleted and recreated, and add an admin path (jmp admin rotate client) that works via K8s API for expired tokens.
jmp auth rotateclient self-rotates via gRPC (needs valid token)jmp admin rotate client <name> --saveadmin rotates via K8s API