-
Notifications
You must be signed in to change notification settings - Fork 37
Add EllipsizeName utility to handle Kubernetes object name limits #1408
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
Open
s3rj1k
wants to merge
1
commit into
projectsveltos:main
Choose a base branch
from
s3rj1k:i118
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package utils | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "hash/fnv" | ||
| ) | ||
|
|
||
| // EllipsizeName ensures a Kubernetes object name is <= 63 characters. | ||
| // If the name exceeds 63 characters, it truncates and appends an 8-character FNV-32 hash | ||
| // for uniqueness. | ||
| func EllipsizeName(name string) string { | ||
| const maxLength = 63 | ||
| if len(name) <= maxLength { | ||
| return name | ||
| } | ||
|
|
||
| // Generate 8-char FNV-32 hex suffix | ||
| h := fnv.New32a() | ||
| h.Write([]byte(name)) | ||
| hash := fmt.Sprintf("%08x", h.Sum32()) | ||
|
|
||
| // Reserve 9 chars: 8 for hash + 1 for separator | ||
| truncateLength := maxLength - 9 | ||
| truncated := name[:truncateLength] | ||
|
|
||
| return truncated + "-" + hash | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| package utils | ||
|
|
||
| import ( | ||
| "regexp" | ||
| "strings" | ||
| "testing" | ||
| "unicode" | ||
| ) | ||
|
|
||
| func TestEllipsizeName(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| wantSame bool // true if we expect output == input | ||
| }{ | ||
| { | ||
| name: "short name unchanged", | ||
| input: "short", | ||
| wantSame: true, | ||
| }, | ||
| { | ||
| name: "exactly 63 chars unchanged", | ||
| input: strings.Repeat("a", 63), | ||
| wantSame: true, | ||
| }, | ||
| { | ||
| name: "64 chars gets ellipsized", | ||
| input: strings.Repeat("a", 64), | ||
| wantSame: false, | ||
| }, | ||
| { | ||
| name: "very long name gets ellipsized", | ||
| input: strings.Repeat("a", 100), | ||
| wantSame: false, | ||
| }, | ||
| { | ||
| name: "real cluster summary name", | ||
| input: "very-long-profile-name-that-exceeds-limits-capi-very-long-cluster-name-that-also-exceeds", | ||
| wantSame: false, | ||
| }, | ||
| { | ||
| name: "real cluster report name", | ||
| input: "p--very-long-profile-name-that-exceeds-limits--capi--very-long-cluster-name-that-also-exceeds", | ||
| wantSame: false, | ||
| }, | ||
| } | ||
|
|
||
| hashSuffixPattern := regexp.MustCompile(`-[0-9a-f]{8}$`) | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := EllipsizeName(tt.input) | ||
|
|
||
| // Check if result matches expectation | ||
| if tt.wantSame && result != tt.input { | ||
| t.Errorf("EllipsizeK8sName() = %q, want same as input %q", result, tt.input) | ||
| } | ||
| if !tt.wantSame && result == tt.input { | ||
| t.Errorf("EllipsizeK8sName() = %q, expected different from input %q", result, tt.input) | ||
| } | ||
|
|
||
| // Check length constraint | ||
| if len(result) > 63 { | ||
| t.Errorf("EllipsizeK8sName() result length = %d, want <= 63", len(result)) | ||
| } | ||
|
|
||
| // For ellipsized names, check hash suffix format | ||
| if len(tt.input) > 63 && tt.input != "" { | ||
| if len(result) != 63 { | ||
| t.Errorf("EllipsizeK8sName() ellipsized result length = %d, want 63", len(result)) | ||
| } | ||
|
|
||
| // Should end with -<8hexchars> pattern | ||
| if !hashSuffixPattern.MatchString(result) { | ||
| t.Errorf("EllipsizeK8sName() result %q should end with -<8hexchars>", result) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func isValidObjectName(name string) bool { | ||
| if name == "" || len(name) > 63 { | ||
| return false | ||
| } | ||
|
|
||
| isAlphanumeric := func(c rune) bool { | ||
| return unicode.IsLower(c) || unicode.IsDigit(c) | ||
| } | ||
|
|
||
| if !isAlphanumeric(rune(name[0])) || !isAlphanumeric(rune(name[len(name)-1])) { | ||
| return false | ||
| } | ||
|
|
||
| for _, c := range name { | ||
| if !isAlphanumeric(c) && c != '-' && c != '.' { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| func FuzzEllipsizeObjectNameHashCollisions(f *testing.F) { | ||
| f.Add("very-long-profile-name-capi-cluster1" + strings.Repeat("x", 30)) | ||
| f.Add("very-long-profile-name-capi-cluster2" + strings.Repeat("x", 30)) | ||
| f.Add(strings.Repeat("a", 80)) | ||
| f.Add(strings.Repeat("b", 80)) | ||
| f.Add("p--profile-name--capi--cluster-name" + strings.Repeat("z", 50)) | ||
|
|
||
| results := make(map[string]string) | ||
|
|
||
| f.Fuzz(func(t *testing.T, input string) { | ||
| // Only care about inputs that will get hashed (longer than 63 chars) | ||
| if len(input) <= 63 { | ||
| t.Skip("Input will not be hashed") | ||
| } | ||
|
|
||
| if !isValidObjectName(input) { | ||
| t.Skip("Input contains invalid characters for Kubernetes names") | ||
| } | ||
|
|
||
| result := EllipsizeName(input) | ||
|
|
||
| // Check for hash collisions | ||
| if existing, exists := results[result]; exists && existing != input { | ||
| t.Errorf("Hash collision detected: inputs %q and %q both produced %q", input, existing, result) | ||
| } | ||
|
|
||
| results[result] = input | ||
| }) | ||
| } |
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.
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.
This is great. Thank you.
We need to handle upgrade scenario. Since we are changing the name, if ClusterSummary instance already exists before upgrade, we need to handle that. I am thinking we need to first check if clusterSummary with old name already exists, if so return that name. Otherwise return the new name.
this method getManagementClusterClient returns the client to the management cluster.
Also, even when we use the name ellipsized name, we need to handle conflicts. So when we generate the ellipsized name we probably need to:
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.
We are changing behavior only in case of actually hitting an edge case for long object name, at least the idea in change was like this, don't change names for cases where inject name is valid, ellipsize name for cases when object name was already invalid and it would be impossible to create it.
There shouldn't be any name conflict, I mean the probability of this happening when we add FNV hash as suffix is quite low (checkout fuzz test).
Problem with fetching a list of existing clusterSummary objects before generating unique name is a possibility to have a race condition and we would need to add a retry logic in every place where we create this objects, this will be more bigger/impactful change.
If we do decide to go with fetch list approach, please let me know if hash suffix is acceptable as object name differentiator in that case, we would still need to create unique object names in case of conflict and adding something like an index number as suffix would probbaly increase a chance of races (at least I think it will)
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. Sorry i missed that we are doing this only when length is over 63 characters. So then yes you are right we don't have to worry about upgrade.
Regarding the conflict, let me think a little more. Maybe instead of fetching, we can keep in memory map. Key could be the generated name and the value profile/cluster names. That will prevent race conditions. And we can rebuild map on restart and postpone profile reconciliation till that is complete
Even if rare I feel we should cover it. Overriding a clustersummary might cause stale resources or missing applications.
What do you think?
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.
I guess the only question is, how that would impact memory footprint in general, probably not noticeable. Generally speaking in-mem map would simplify things a lot, avoiding retry logic in a lot of places.
On a side note, if we can say that our clusters won't go back in time, we can use unixtime as object suffix and completely avoid messing with hashing, on downside that would mean names will always be unique, not predictable per each input. In our usecase I think this can be acceptable.
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. Yes we would need to store only names when exceeding 63 chars. So it won't impact.
But good point. We can safely say clusters won't go back on time. But I am not sure that will work. Those methods to get name are invoked also after clustersummary instances are created. For instance clusterprofile reconciliation invokes it for every matching clusters at every reconciliation
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.
This makes me think that we need everything to be deterministic, so any kind of counter approach at suffix won't work (counter, unixtime, ...)
Let's assume we implement a in-mem map for tracking objects, and we use some very prone to collisions hash function, we can detect collision via map key (same would apply to a case of direct k8s object lookup) but what we do next is unclear, how do we recover from collision and still keep names deterministic?
So I am thinking we need to invest a bit more time into that fuzz test, basically limiting the upper-bound string length to what we actual use (~ 63*4 + 5), ensure that it generates okeish looking strings, run it against current hash func for 24h and see if we have collisions, if we do, increase hash size and/or replace hash function, run again and repeat until we are not getting collision detection, use that as solution and put into docs that for larger cluster profile and cluster names we use hashing to create predictable object names, hashing has a low level of collusion chances but still not zero so if you ever hit that kind of a problem, please report that and rename your profile.
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. Can we do this:
On restart we can rebuild this in memory map before we reconcile any clusterProfile/profile
I feel your PR already took care of most, we just need to handle those corners cases.
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.
I guess we can but keep in mind that in case of collision and appending index we can get to a situation when some other object that created collision gets removed and we will have none-deterministic name for new object (that was created with index), basically reusing same name.
(this is what I was referencing before)
And if we already talking about adding indexes we might as well do in-tree map + unixtime and remove a need of hash and additional hash+index
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.
Correct but when a clustersummary is deleted, if name was allocated via this new mechanism, we can remove from map.
Also what we want to solve is to make sure clustersummary name is always valid. Today if ClusterProfile name is really long and the cluster name is also long (exceeding the 253 chars) creation of a clustersummary will fail