-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: truncate label values longer than 63 characters #6382
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
@@ -293,7 +287,7 @@ func (r *ScaledObjectReconciler) getScaledObjectMetricSpecs(ctx context.Context, | |||
Metric: autoscalingv2.MetricIdentifier{ | |||
Name: compMetricName, | |||
Selector: &metav1.LabelSelector{ | |||
MatchLabels: map[string]string{kedav1alpha1.ScaledObjectOwnerAnnotation: scaledObject.Name}, | |||
MatchLabels: map[string]string{kedav1alpha1.ScaledObjectOwnerAnnotation: scaledObjectName}, |
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'm not 100% sure if we should do this or just print an error. We use this label in to recover the SO name during the HPA querying and I'd say that the querying will fail if we truncate this name. But maybe I'm wrong and if we cover the scenario with and e2e test to cover it, I'm fine 😄
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.
@kedacore/keda-contributors
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.
oddly enough, HPA seems to have the label truncated since 2021, there it's probably not used that much.
For sure we should add e2e tests for trunc name cases, I'd be curious to see what is the behaviour when two SO with names 63+ chars that differ only in the last truncated characters.
Alternatively, can we refactor the code to use SO uid
field? that is guaranteed to be within the bounds but not sure if that would be a breaking change. Or just document that SOs with more than 63 chars are not allowed :)
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.
oddly enough, HPA seems to have the label #1631, there it's probably not used that much.
That applies to the HPA name, and this label is this other line -> https://github.com/kedacore/keda/pull/1631/files#diff-a43846d8ca384145be0f11e006bc0ce798b50c992df46e9b13ff0c55c290bae0L157
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.
The HPA name isn't relevant if there isn't more HPAs in conflict in the namespace, but the ScaledObject name is used to get the SO
Provide a description of what has been changed
When creating a
ScaledObject
with a name exceeding 63 characters, an error occurs due to Kubernetes' label length limitations. This PR addresses the issue by truncating theScaledObject
name to ensure it adheres to the label length requirements.Checklist
Fixes #6370
Relates to #
Made local testing:
deployment.yaml:
scaledobject.yaml:
and get:
and: