Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions controllers/keda/hpa.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ import (
"fmt"
"sort"
"strconv"
"strings"
"unicode"

"github.com/go-logr/logr"
autoscalingv2 "k8s.io/api/autoscaling/v2"
Expand Down Expand Up @@ -82,21 +80,16 @@ func (r *ScaledObjectReconciler) newHPAForScaledObject(ctx context.Context, logg
}

// label can have max 63 chars
labelName := getHPAName(scaledObject)
if len(labelName) > 63 {
labelName = labelName[:63]
labelName = strings.TrimRightFunc(labelName, func(r rune) bool {
return !unicode.IsLetter(r) && !unicode.IsNumber(r)
})
}
labelName := kedacontrollerutil.Truncate(getHPAName(scaledObject), 63)
scaleObjectName := kedacontrollerutil.Truncate(scaledObject.Name, 63)
labels := map[string]string{
"app.kubernetes.io/name": labelName,
"app.kubernetes.io/version": version.Version,
"app.kubernetes.io/part-of": scaledObject.Name,
"app.kubernetes.io/part-of": scaleObjectName,
"app.kubernetes.io/managed-by": "keda-operator",
}
for key, value := range scaledObject.ObjectMeta.Labels {
labels[key] = value
labels[key] = kedacontrollerutil.Truncate(value, 63)
}

minReplicas := scaledObject.GetHPAMinReplicas()
Expand Down Expand Up @@ -221,6 +214,7 @@ func (r *ScaledObjectReconciler) getScaledObjectMetricSpecs(ctx context.Context,
}

metricSpecs := cache.GetMetricSpecForScaling(ctx)
scaledObjectName := kedacontrollerutil.Truncate(scaledObject.Name, 63)

for _, metricSpec := range metricSpecs {
if metricSpec.Resource != nil {
Expand All @@ -230,12 +224,12 @@ func (r *ScaledObjectReconciler) getScaledObjectMetricSpecs(ctx context.Context,
if metricSpec.External != nil {
externalMetricName := metricSpec.External.Metric.Name
if kedacontrollerutil.Contains(externalMetricNames, externalMetricName) {
return nil, fmt.Errorf("metricName %s defined multiple times in ScaledObject %s", externalMetricName, scaledObject.Name)
return nil, fmt.Errorf("metricName %s defined multiple times in ScaledObject %s", externalMetricName, scaledObjectName)
}

// add the scaledobject.keda.sh/name label. This is how the MetricsAdapter will know which scaledobject a metric is for when the HPA queries it.
metricSpec.External.Metric.Selector = &metav1.LabelSelector{MatchLabels: make(map[string]string)}
metricSpec.External.Metric.Selector.MatchLabels[kedav1alpha1.ScaledObjectOwnerAnnotation] = scaledObject.Name
metricSpec.External.Metric.Selector.MatchLabels[kedav1alpha1.ScaledObjectOwnerAnnotation] = scaledObjectName
externalMetricNames = append(externalMetricNames, externalMetricName)
}
}
Expand Down Expand Up @@ -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},
Copy link
Member

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 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kedacore/keda-contributors

Copy link
Member

@wozniakjan wozniakjan Dec 2, 2024

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 :)

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

@wozniakjan wozniakjan Dec 4, 2024

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.

Copy link
Member

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

},
},
Target: correctHpaTarget,
Expand Down
11 changes: 6 additions & 5 deletions controllers/keda/scaledobject_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (r *ScaledObjectReconciler) Reconcile(ctx context.Context, req ctrl.Request
conditions.SetFallbackCondition(metav1.ConditionFalse, "NoFallbackFound", "No fallbacks are active on this scaled object")
}

metricscollector.RecordScaledObjectPaused(scaledObject.Namespace, scaledObject.Name, conditions.GetPausedCondition().Status == metav1.ConditionTrue)
metricscollector.RecordScaledObjectPaused(scaledObject.Namespace, kedacontrollerutil.Truncate(scaledObject.Name, 63), conditions.GetPausedCondition().Status == metav1.ConditionTrue)

if err := kedastatus.SetStatusConditions(ctx, r.Client, reqLogger, scaledObject, &conditions); err != nil {
r.EventEmitter.Emit(scaledObject, req.NamespacedName.Namespace, corev1.EventTypeWarning, eventingv1alpha1.ScaledObjectFailedType, eventreason.ScaledObjectUpdateFailed, err.Error())
Expand Down Expand Up @@ -319,17 +319,18 @@ func (r *ScaledObjectReconciler) reconcileScaledObject(ctx context.Context, logg
// ensureScaledObjectLabel ensures that scaledobject.keda.sh/name=<scaledObject.Name> label exist in the ScaledObject
// This is how the MetricsAdapter will know which ScaledObject a metric is for when the HPA queries it.
func (r *ScaledObjectReconciler) ensureScaledObjectLabel(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) error {
scaledObjectNameTruncated := kedacontrollerutil.Truncate(scaledObject.Name, 63)
if scaledObject.Labels == nil {
scaledObject.Labels = map[string]string{kedav1alpha1.ScaledObjectOwnerAnnotation: scaledObject.Name}
scaledObject.Labels = map[string]string{kedav1alpha1.ScaledObjectOwnerAnnotation: scaledObjectNameTruncated}
} else {
value, found := scaledObject.Labels[kedav1alpha1.ScaledObjectOwnerAnnotation]
if found && value == scaledObject.Name {
if found && value == scaledObjectNameTruncated {
return nil
}
scaledObject.Labels[kedav1alpha1.ScaledObjectOwnerAnnotation] = scaledObject.Name
scaledObject.Labels[kedav1alpha1.ScaledObjectOwnerAnnotation] = scaledObjectNameTruncated
}

logger.V(1).Info("Adding \"scaledobject.keda.sh/name\" label on ScaledObject", "value", scaledObject.Name)
logger.V(1).Info("Adding \"scaledobject.keda.sh/name\" label on ScaledObject", "value", scaledObjectNameTruncated)
return r.Client.Update(ctx, scaledObject)
}

Expand Down
32 changes: 32 additions & 0 deletions controllers/keda/util/string.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
Copyright 2021 The KEDA Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"strings"
"unicode"
)

func Truncate(s string, threshold int) string {
if len(s) > threshold {
s = s[:threshold]
s = strings.TrimRightFunc(s, func(r rune) bool {
return !unicode.IsLetter(r) && !unicode.IsNumber(r)
})
}
return s
}
58 changes: 58 additions & 0 deletions controllers/keda/util/string_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
Copyright 2021 The KEDA Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"testing"
)

func TestTruncate(t *testing.T) {
tests := []struct {
name string
input string
threshold int
want string
}{
{
name: "string shorter than threshold",
input: "hello",
threshold: 10,
want: "hello",
},
{
name: "string longer than threshold ending with special chars",
input: "abc---def---ghi---",
threshold: 5,
want: "abc",
},
{
name: "63 character limit case",
input: "this-is-64-characters-name-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
threshold: 63,
want: "this-is-64-characters-name-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"[:63],
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := Truncate(tt.input, tt.threshold)
if got != tt.want {
t.Errorf("Truncuate() = %v, want %v", got, tt.want)
}
})
}
}
Loading