From 59ce337d32553a9d925a653951134a37ee16b6fa Mon Sep 17 00:00:00 2001 From: Zack Robinson Date: Wed, 22 May 2024 10:30:54 -0400 Subject: [PATCH] fix: app names with non-alphanumeric characters in position 63 break syncs (issue #18237) (#18256) * Ensure truncated app label does not end in a special character Signed-off-by: Zack Robinson * Move regex to global variable and add out of bounds check Signed-off-by: Zack Robinson * Add test for out-of-bounds check Signed-off-by: Zack Robinson --------- Signed-off-by: Zack Robinson --- util/argo/resource_tracking.go | 11 ++++++ util/argo/resource_tracking_test.go | 54 +++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/util/argo/resource_tracking.go b/util/argo/resource_tracking.go index bc58172397ffe..b76cd3c8e214f 100644 --- a/util/argo/resource_tracking.go +++ b/util/argo/resource_tracking.go @@ -1,7 +1,9 @@ package argo import ( + "errors" "fmt" + "regexp" "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -21,6 +23,7 @@ const ( var WrongResourceTrackingFormat = fmt.Errorf("wrong resource tracking format, should be :/:/") var LabelMaxLength = 63 +var OkEndPattern = regexp.MustCompile("[a-zA-Z0-9]$") // ResourceTracking defines methods which allow setup and retrieve tracking information to resource type ResourceTracking interface { @@ -155,6 +158,14 @@ func (rt *resourceTracking) SetAppInstance(un *unstructured.Unstructured, key, v } if len(val) > LabelMaxLength { val = val[:LabelMaxLength] + // Prevent errors if the truncated name ends in a special character. + // See https://github.com/argoproj/argo-cd/issues/18237. + for !OkEndPattern.MatchString(val) { + if len(val) <= 1 { + return errors.New("failed to set app instance label: unable to truncate label to not end with a special character") + } + val = val[:len(val)-1] + } } err = argokube.SetAppInstanceLabel(un, key, val) if err != nil { diff --git a/util/argo/resource_tracking_test.go b/util/argo/resource_tracking_test.go index 479dcb98da098..b0452bbf6d1c1 100644 --- a/util/argo/resource_tracking_test.go +++ b/util/argo/resource_tracking_test.go @@ -62,6 +62,60 @@ func TestSetAppInstanceAnnotationAndLabel(t *testing.T) { assert.Equal(t, "my-app", app) } +func TestSetAppInstanceAnnotationAndLabelLongName(t *testing.T) { + yamlBytes, err := os.ReadFile("testdata/svc.yaml") + assert.Nil(t, err) + var obj unstructured.Unstructured + err = yaml.Unmarshal(yamlBytes, &obj) + assert.Nil(t, err) + + resourceTracking := NewResourceTracking() + + err = resourceTracking.SetAppInstance(&obj, common.LabelKeyAppInstance, "my-app-with-an-extremely-long-name-that-is-over-sixty-three-characters", "", TrackingMethodAnnotationAndLabel) + assert.Nil(t, err) + + // the annotation should still work, so the name from GetAppName should not be truncated + app := resourceTracking.GetAppName(&obj, common.LabelKeyAppInstance, TrackingMethodAnnotationAndLabel) + assert.Equal(t, "my-app-with-an-extremely-long-name-that-is-over-sixty-three-characters", app) + + // the label should be truncated to 63 characters + assert.Equal(t, obj.GetLabels()[common.LabelKeyAppInstance], "my-app-with-an-extremely-long-name-that-is-over-sixty-three-cha") +} + +func TestSetAppInstanceAnnotationAndLabelLongNameBadEnding(t *testing.T) { + yamlBytes, err := os.ReadFile("testdata/svc.yaml") + assert.Nil(t, err) + var obj unstructured.Unstructured + err = yaml.Unmarshal(yamlBytes, &obj) + assert.Nil(t, err) + + resourceTracking := NewResourceTracking() + + err = resourceTracking.SetAppInstance(&obj, common.LabelKeyAppInstance, "the-very-suspicious-name-with-precisely-sixty-three-characters-with-hyphen", "", TrackingMethodAnnotationAndLabel) + assert.Nil(t, err) + + // the annotation should still work, so the name from GetAppName should not be truncated + app := resourceTracking.GetAppName(&obj, common.LabelKeyAppInstance, TrackingMethodAnnotationAndLabel) + assert.Equal(t, "the-very-suspicious-name-with-precisely-sixty-three-characters-with-hyphen", app) + + // the label should be truncated to 63 characters, AND the hyphen should be removed + assert.Equal(t, obj.GetLabels()[common.LabelKeyAppInstance], "the-very-suspicious-name-with-precisely-sixty-three-characters") +} + +func TestSetAppInstanceAnnotationAndLabelOutOfBounds(t *testing.T) { + yamlBytes, err := os.ReadFile("testdata/svc.yaml") + assert.Nil(t, err) + var obj unstructured.Unstructured + err = yaml.Unmarshal(yamlBytes, &obj) + assert.Nil(t, err) + + resourceTracking := NewResourceTracking() + + err = resourceTracking.SetAppInstance(&obj, common.LabelKeyAppInstance, "----------------------------------------------------------------", "", TrackingMethodAnnotationAndLabel) + // this should error because it can't truncate to a valid value + assert.EqualError(t, err, "failed to set app instance label: unable to truncate label to not end with a special character") +} + func TestSetAppInstanceAnnotationNotFound(t *testing.T) { yamlBytes, err := os.ReadFile("testdata/svc.yaml") assert.Nil(t, err)