-
Notifications
You must be signed in to change notification settings - Fork 1.2k
CKS: generate a random UUID as password of CKS user in project #11639
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
CKS: generate a random UUID as password of CKS user in project #11639
Conversation
@blueorangutan package |
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.
clgtm
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
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, but how about when the min pw length is set beyond a UUID length?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11639 +/- ##
============================================
- Coverage 16.17% 16.17% -0.01%
+ Complexity 13298 13297 -1
============================================
Files 5656 5656
Lines 498151 498221 +70
Branches 60441 60452 +11
============================================
- Hits 80589 80581 -8
- Misses 408591 408671 +80
+ Partials 8971 8969 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
then #11626 appears 😆 this is just a quick workaround. generate a more strong password matching the policy, or ignore the password policy for CKS users |
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.
clgtm ( but a real solution should be created in a call that generates a password from a password policy in the end )
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 15014 |
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.
change looks good but maybe for a complete solution we can bypass PasswordPolicy here. Kubernetes project account is created with System account call context so maybe we can bypass policy validations when System is creating accounts
thanks @shwstppr |
@weizhouapache best case would have been creating a password compliant to the password policy. But because of regex config it would be difficult and error-prone. Next best is bypassing policy and creating a stronger password (which would never be used though). So I think maybe we keep this usage of complete UUID and then bypass the policy for system created account (which I feel would be more generic than adding a User.Source.CKS) |
thanks @shwstppr |
@blueorangutan package |
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15179 |
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.
@weizhouapache I've not tested but I guess we didn't need all the changes to pass the caller.
We could use CallContext.getCallingAccount(). KubernetesClusterManager was already using the system callcontext at line 1550
thanks @shwstppr , good point |
…y system account" This reverts commit 2493a59.
reverted the last commit, added a simple check on the caller tested ok
thanks @shwstppr , good advise |
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
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.
cool, thanks for the testing @shwstppr |
@blueorangutan package |
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15234 |
@blueorangutan test |
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-14487)
|
merging based on approvals and manua test, thanks @shwstppr |
Description
This PR fixes the password CKS user in project
UuidUtils.first
returns only the first part of UUID , which contains 8 letters/digits in total.If the min length is set to a value more than 8, the CKS cluster cannot be created in project
To be consistent with CKS user in non-project, use UUID instead of first 8 chars of UUID as password of CKS user in project
This fixes #11626
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?