-
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
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.
So, do we move forward by simply printing an error and relying on an end-to-end job to cover this?
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.
as @JorTurFer mentioned, the ScaledObject
will be patched with a truncated label now but getting metrics may probably not work because of how it's implemented in the provider.go
. I don't have any good ideas how to work around that unless we refactor the internals to not use the name
but uid
for example.
perhaps a good start is to create an e2e test covering the case of ScaledObject
with a long name so we can see if our assumption of a failure is correct or no. To be extra safe, we may want to also add an e2e test where two ScaledObjects
in the same namespace have the same truncated name.
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 like @wozniakjan's suggestion
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: