-
Notifications
You must be signed in to change notification settings - Fork 7
feat: Add account name as label in root + sign keys #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add account name as label in root + sign keys #110
Conversation
thobiaskarlsson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but I have a couple of comments I'd like you to consider before approving.
|
@thobiaskarlsson, thank you for the review. Ready for a second pass! |
thobiaskarlsson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for contributing! You need to sign-off the commit(s) though.
henriropp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Just a minor finding in the seed lookup from secrets. Probably not very frequent scenario...
Signed-off-by: Illia Ovchynnikov <illia.ovchynnikov@gmail.com>
3105ace to
bf8b5e7
Compare
Done!
Fixed too. Thank you guys! Are there any plans for the next release anytime soon (last one was more than 3 weeks ago)? |
Yes, we can absolutely do a new release. I was hoping for #116 to be merged soon so that we can get both features in the same release. @aleksanderaleksic, what are your thoughts on this, will you be able to fix the PR within the upcoming days? |
#116 feels like a major change worth a |
@thobiaskarlsson I will work on it today and try to get it ready for another round of reviews. |
@wingsofovnia I agree that #116 is a new minor release, we are introducing a new concept even though its backwards-compatible. |
|
@thobiaskarlsson can we merge this PR or there is a reason to keep it open? |
Yes, I'll release the current changes as a patch release. 👍 Great work! |
This PR addresses the issue of orphaned secrets created during reconciliation retries #83 by implementing deterministic labeling for account secrets #60.
Problem
When the NAuth controller reconciles a new Account CR, it generates account keys and creates Kubernetes secrets before uploading the JWT to NATS. If the NATS upload fails (e.g., NATS is temporarily unavailable), the reconciliation loop retries. Since the account.nauth.io/id label hasn't been applied to the CR yet, the controller would generate a new ID and a new set of secrets on every retry, leading to a build-up of orphaned secrets.
Solution
The controller now uses the account name as a deterministic identifier to locate existing secrets before creating new ones. By labeling secrets with
account.nauth.io/name, the manager can recover and reuse previously generated keys even if the reconciliation process was interrupted.Fixes #60
Closes #83