Skip to content

Conversation

@mangelajo
Copy link
Member

@mangelajo mangelajo commented Nov 7, 2025

Enable Client/Exporter migration across clusters by using name-only authentication tokens. Usernames() now returns both portable format (new) and namespace/UUID format (legacy) to maintain compatibility with existing tokens.

This allows copying objects and their token secrets between clusters without token regeneration or without setting a migrated-uid annotation.

This will only work for newly generated tokens, but does not work to migrate existing tokens.

Related-Issue: #188 188

Summary by CodeRabbit

  • Refactor
    • Updated client identifier formatting to support both portable and legacy formats while maintaining backward compatibility.
    • Simplified exporter identifier format with dual-format support for portable and legacy identifiers.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 7, 2025

Walkthrough

The changes simplify InternalSubject() to return a portable format using only the name, while expanding Usernames() to emit both portable and legacy formats followed by any explicit override username. These modifications apply consistently to both client and exporter helper files.

Changes

Cohort / File(s) Summary
Subject and username format modernization
api/v1alpha1/client_helpers.go, api/v1alpha1/exporter_helpers.go
InternalSubject() simplified to return portable format (name only). Usernames() now constructs dual formats: portable format and legacy format (namespace:name:UID), followed by optional Spec.Username override.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify portable and legacy format construction is correct in both files
  • Ensure the optional username override behavior is preserved and tested
  • Confirm backward compatibility with existing code depending on these helper methods

Possibly related PRs

Poem

🐰 A rabbit hops through formats new,
Portable names and legacy too—
Dual paths where subjects flow,
Simplicity helps the code to grow! 🌱

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main change: introducing portable authentication tokens while maintaining backward compatibility with existing tokens.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch portable-auth

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d344e5 and bbbf9ed.

📒 Files selected for processing (2)
  • api/v1alpha1/client_helpers.go (1 hunks)
  • api/v1alpha1/exporter_helpers.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/v1alpha1/client_helpers.go (1)
api/v1alpha1/client_types.go (1)
  • Client (44-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: lint-go
  • GitHub Check: tests
  • GitHub Check: deploy-kind
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: e2e-tests-release-0-7
  • GitHub Check: e2e-test-operator
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
🔇 Additional comments (4)
api/v1alpha1/client_helpers.go (2)

9-20: Consistent dual-format implementation.

The implementation mirrors the exporter approach, maintaining backward compatibility while enabling portability. The ordering (portable, legacy, explicit) is consistent across both resource types.


5-7: This review comment is incorrect and should be dismissed.

The concern about namespace collision does not apply. The InternalSubject() Subject claim is not used to identify which namespace's Client resource to access. Instead, the namespace is extracted from gRPC request metadata (separate from the token), and Kubernetes RBAC enforces namespace isolation when the authorization layer performs the actual resource lookup via client.Get(ctx, ObjectKey{Namespace: ns, Name: name}).

The simplified format is safe because:

  • The JWT Subject claim is just a certificate CN field (informational)
  • Resource identification comes from namespace-aware request metadata + RBAC validation
  • Even if an attacker provides false namespace metadata, authorization fails against the actual Kubernetes resource
  • The dual format in Usernames() exists for legacy compatibility, not safety
api/v1alpha1/exporter_helpers.go (2)

12-14: Review comment is incorrect — namespace isolation is properly enforced.

The concern about cross-namespace collisions overlooks how the authorization actually works. The exporter is always fetched using a namespaced ObjectKey that includes the namespace context from attributes.GetNamespace() (line 34–36). Even if exporters with the same name exist in different namespaces, each lookup is scoped to a specific namespace, so the validation happens on the correct exporter object in the correct namespace. InternalSubject() is intentionally simplified for the portable token format, while Usernames() includes the full legacy format for backward compatibility. Namespace isolation is correctly enforced at the authorization layer through Kubernetes' standard namespaced object lookups, not through the subject string itself.

Likely an incorrect or invalid review comment.


16-27: Dual-format approach maintains backward compatibility effectively.

The implementation correctly emits both portable and legacy formats, ensuring existing tokens continue to work while enabling portability for new tokens.

However, verify that the authentication layer properly handles the portable format's namespace-agnostic nature. Run this script to check how usernames are validated:


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mangelajo mangelajo requested a review from NickCao November 7, 2025 11:08
Enable Client/Exporter migration across clusters by using name-only
authentication tokens. Usernames() now returns both portable format
(new) and namespace/UUID format (legacy) to maintain compatibility
with existing tokens.

This allows copying objects and their token secrets between clusters
without token regeneration or without setting a migrated-uuid
annotation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants