From cb16c117898bdd08e8c01aa7592a9fa670b01f63 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Tue, 5 Nov 2024 12:08:18 +0100 Subject: [PATCH 01/11] Switch to non-illegal character in label-value --- pkg/resourcegenerator/resourceutils/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 19f434e1..1b3c5915 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -35,7 +35,7 @@ func GetImageVersion(imageVersionString string) string { // Add build number to version if it is specified if len(parts) > 2 { - return versionPart + "+" + parts[2] + return versionPart + "-" + parts[2] } return versionPart } From 85fe06fc9e9d5dc57fe6f4f3230e6c81bc387677 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Tue, 5 Nov 2024 12:18:40 +0100 Subject: [PATCH 02/11] Added more test-cases --- .../resourceutils/helpers_test.go | 72 +++++++++---------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/pkg/resourcegenerator/resourceutils/helpers_test.go b/pkg/resourcegenerator/resourceutils/helpers_test.go index cc77f659..0cd4b131 100644 --- a/pkg/resourcegenerator/resourceutils/helpers_test.go +++ b/pkg/resourcegenerator/resourceutils/helpers_test.go @@ -1,47 +1,39 @@ package resourceutils import ( - "github.com/stretchr/testify/assert" "testing" -) - -func TestGetImageVersionNoTag(t *testing.T) { - imageString := "image" - expectedImageString := "latest" - - actualImageString := GetImageVersion(imageString) - - assert.Equal(t, expectedImageString, actualImageString) -} - -func TestGetImageVersionLatestTag(t *testing.T) { - imageString := "image:latest" - expectedImageString := "latest" - - actualImageString := GetImageVersion(imageString) - - assert.Equal(t, expectedImageString, actualImageString) -} - -func TestGetImageVersionVersionTag(t *testing.T) { - versionImageString := "image:1.2.3" - devImageString := "image:1.2.3-dev-123abc" - expectedVersionImageString := "1.2.3" - expectedDevImageString := "1.2.3-dev-123abc" - - actualVersionImageString := GetImageVersion(versionImageString) - actualDevImageString := GetImageVersion(devImageString) - assert.Equal(t, expectedVersionImageString, actualVersionImageString) - assert.Equal(t, expectedDevImageString, actualDevImageString) - -} - -func TestGetImageVersionShaTag(t *testing.T) { - imageString := "ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8" - expectedImageString := "54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8" - - actualImageString := GetImageVersion(imageString) + "github.com/stretchr/testify/assert" +) - assert.Equal(t, expectedImageString, actualImageString) +func TestVersions(t *testing.T) { + testCases := []struct { + imageString string + expectedValue string + }{ + {"image", "latest"}, + {"image:latest", "latest"}, + {"image:1.2.3-dev-123abc", "1.2.3-dev-123abc"}, + {"image:1.2.3", "1.2.3"}, + {"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8"}, + {"ghcr.io/org/one-app:sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4"}, + {"ghcr.io/org/another-app:3fb7048", "3fb7048"}, + {"ghcr.io/org/some-team/third-app:v1.2.54", "v1.2.54"}, + {"ghcr.io/org/another-team/fourth-app:4.0.0.rc-36", "4.0.0.rc-36"}, + {"ghcr.io/org/another-team/fifth-app:4.0.0.rc-36-master-latest", "4.0.0.rc-36-master-latest"}, + {"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "latest"}, + {"nginxinc/nginx-unprivileged:1.20.0-alpine", "1.20.0-alpine"}, + {"foo/bar:1.2.3+build.4", "1.2.3-build.4"}, + {"foo/bar:1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"}, + {"foo/bar:-1.2.3", "1.2.3"}, + {"foo/bar:_1.2.3", "1.2.3"}, + {"foo/bar:.1.2.3", "1.2.3"}, + } + + for _, tc := range testCases { + t.Run(tc.imageString, func(t *testing.T) { + actualValue := GetImageVersion(tc.imageString) + assert.Equal(t, tc.expectedValue, actualValue) + }) + } } From 8e11abe6ceb5ece42311d15d95da9c3b9c7c3143 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Tue, 5 Nov 2024 13:07:52 +0100 Subject: [PATCH 03/11] - Added more test-cases (thanks @evenh) - Fixed most cases --- .../resourceutils/helpers.go | 21 +++++++++++++++---- .../resourceutils/helpers_test.go | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 1b3c5915..46628d8d 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -22,7 +22,7 @@ func GetImageVersion(imageVersionString string) string { parts := strings.Split(imageVersionString, ":") // Implicitly assume version "latest" if no version is specified - if len(parts) < 2 { + if len(parts) > 2 { return "latest" } @@ -33,9 +33,22 @@ func GetImageVersion(imageVersionString string) string { versionPart = strings.Split(versionPart, "@")[0] } - // Add build number to version if it is specified - if len(parts) > 2 { - return versionPart + "-" + parts[2] + // Replace "+" with "-" in version text if version includes one + versionPart = strings.ReplaceAll(versionPart, "+", "-") + + // Limit label-value to 63 characters + if len(versionPart) > 63 { + versionPart = versionPart[:63] } + + // While first character is not [a-z0-9A-Z] then remove it + for len(versionPart) > 0 && !((versionPart[0] >= 'a' && versionPart[0] <= 'z') || (versionPart[0] >= 'A' && versionPart[0] <= 'Z') || (versionPart[0] >= '0' && versionPart[0] <= '9')) { + versionPart = versionPart[1:] + } + + // Add build number to version if it is specified + // if len(parts) > 2 { + // return versionPart + "-" + parts[2] + // } return versionPart } diff --git a/pkg/resourcegenerator/resourceutils/helpers_test.go b/pkg/resourcegenerator/resourceutils/helpers_test.go index 0cd4b131..18e6099d 100644 --- a/pkg/resourcegenerator/resourceutils/helpers_test.go +++ b/pkg/resourcegenerator/resourceutils/helpers_test.go @@ -26,7 +26,7 @@ func TestVersions(t *testing.T) { {"foo/bar:1.2.3+build.4", "1.2.3-build.4"}, {"foo/bar:1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"}, {"foo/bar:-1.2.3", "1.2.3"}, - {"foo/bar:_1.2.3", "1.2.3"}, + {"foo/bar:__1.2.3", "1.2.3"}, {"foo/bar:.1.2.3", "1.2.3"}, } From e382b06c6d32dd28279057d21f59291c7f2422f7 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Tue, 5 Nov 2024 15:34:40 +0100 Subject: [PATCH 04/11] - Rearranged some of the imageVersionString processing to make it more robust - All tests pass now --- .../resourceutils/helpers.go | 28 +++++++++---------- .../resourceutils/helpers_test.go | 5 ++-- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 46628d8d..398d536d 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -6,6 +6,8 @@ import ( "strings" ) +const LabelValueMaxLength int = 63 + func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { replicas, err := skiperatorv1alpha1.GetStaticReplicas(jsonReplicas) if err == nil && replicas == 0 { @@ -19,26 +21,28 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { } func GetImageVersion(imageVersionString string) string { - parts := strings.Split(imageVersionString, ":") + // Find position of first "@", remove it and everything after it + if strings.Contains(imageVersionString, "@") { + imageVersionString = strings.Split(imageVersionString, "@")[0] + imageVersionString = imageVersionString + ":unknown" + } - // Implicitly assume version "latest" if no version is specified - if len(parts) > 2 { + // If no version is given, assume "latest" + if !strings.Contains(imageVersionString, ":") { return "latest" } - versionPart := parts[1] + // Split image string into parts + parts := strings.Split(imageVersionString, ":") - // Remove "@sha256" from version text if version includes a hash - if strings.Contains(versionPart, "@") { - versionPart = strings.Split(versionPart, "@")[0] - } + versionPart := parts[1] // Replace "+" with "-" in version text if version includes one versionPart = strings.ReplaceAll(versionPart, "+", "-") // Limit label-value to 63 characters - if len(versionPart) > 63 { - versionPart = versionPart[:63] + if len(versionPart) > LabelValueMaxLength { + versionPart = versionPart[:LabelValueMaxLength] } // While first character is not [a-z0-9A-Z] then remove it @@ -46,9 +50,5 @@ func GetImageVersion(imageVersionString string) string { versionPart = versionPart[1:] } - // Add build number to version if it is specified - // if len(parts) > 2 { - // return versionPart + "-" + parts[2] - // } return versionPart } diff --git a/pkg/resourcegenerator/resourceutils/helpers_test.go b/pkg/resourcegenerator/resourceutils/helpers_test.go index 18e6099d..abd9c152 100644 --- a/pkg/resourcegenerator/resourceutils/helpers_test.go +++ b/pkg/resourcegenerator/resourceutils/helpers_test.go @@ -15,13 +15,14 @@ func TestVersions(t *testing.T) { {"image:latest", "latest"}, {"image:1.2.3-dev-123abc", "1.2.3-dev-123abc"}, {"image:1.2.3", "1.2.3"}, - {"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8"}, + {"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "unknown"}, {"ghcr.io/org/one-app:sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4"}, {"ghcr.io/org/another-app:3fb7048", "3fb7048"}, {"ghcr.io/org/some-team/third-app:v1.2.54", "v1.2.54"}, {"ghcr.io/org/another-team/fourth-app:4.0.0.rc-36", "4.0.0.rc-36"}, {"ghcr.io/org/another-team/fifth-app:4.0.0.rc-36-master-latest", "4.0.0.rc-36-master-latest"}, - {"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "latest"}, + {"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "unknown"}, + {"ghcr.io/kartverket/vulnerability-disclosure-program:4.0.1@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "4.0.1"}, {"nginxinc/nginx-unprivileged:1.20.0-alpine", "1.20.0-alpine"}, {"foo/bar:1.2.3+build.4", "1.2.3-build.4"}, {"foo/bar:1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"}, From eb0ab78c250c92dd30ded8f3de13e349571087f6 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Wed, 6 Nov 2024 10:27:38 +0100 Subject: [PATCH 05/11] - First-character check now uses actual regex-matching --- pkg/resourcegenerator/resourceutils/helpers.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 398d536d..1009a565 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -3,6 +3,7 @@ package resourceutils import ( skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "regexp" "strings" ) @@ -20,6 +21,13 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { return false } +// MatchesRegex checks if a string matches a regexp +func MatchesRegex(s string, pattern string) bool { + obj, err := regexp.Match(pattern, []byte(s)) + return obj && err == nil +} + +// GetImageVersion returns the version part of an image string func GetImageVersion(imageVersionString string) string { // Find position of first "@", remove it and everything after it if strings.Contains(imageVersionString, "@") { @@ -45,8 +53,8 @@ func GetImageVersion(imageVersionString string) string { versionPart = versionPart[:LabelValueMaxLength] } - // While first character is not [a-z0-9A-Z] then remove it - for len(versionPart) > 0 && !((versionPart[0] >= 'a' && versionPart[0] <= 'z') || (versionPart[0] >= 'A' && versionPart[0] <= 'Z') || (versionPart[0] >= '0' && versionPart[0] <= '9')) { + // While first character is not part of regex [a-z0-9A-Z] then remove it + for len(versionPart) > 0 && !MatchesRegex(versionPart[:1], "[a-zA-Z0-9]") { versionPart = versionPart[1:] } From 63f32dfedc1e404a6d4972f239099bb0987abc53 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Wed, 6 Nov 2024 10:41:03 +0100 Subject: [PATCH 06/11] Don't export matchesRegex function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Martin Haram Nygård --- pkg/resourcegenerator/resourceutils/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 1009a565..9f08a451 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -22,7 +22,7 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { } // MatchesRegex checks if a string matches a regexp -func MatchesRegex(s string, pattern string) bool { +func matchesRegex(s string, pattern string) bool { obj, err := regexp.Match(pattern, []byte(s)) return obj && err == nil } From c27959e7972f006585c3630ab1e010abab756f16 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Fri, 8 Nov 2024 12:18:08 +0100 Subject: [PATCH 07/11] - More label test-cases, finally all of them pass. - Refactored function-name to better reflect what it does --- .../deployment/deployment.go | 2 +- pkg/resourcegenerator/job/job.go | 2 +- .../resourceutils/helpers.go | 53 ++++++++++--------- .../resourceutils/helpers_test.go | 19 +++++-- pkg/resourcegenerator/service/service.go | 2 +- 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/pkg/resourcegenerator/deployment/deployment.go b/pkg/resourcegenerator/deployment/deployment.go index 6e11cd06..f02f3c21 100644 --- a/pkg/resourcegenerator/deployment/deployment.go +++ b/pkg/resourcegenerator/deployment/deployment.go @@ -108,7 +108,7 @@ func Generate(r reconciliation.Reconciliation) error { } else { podTemplateLabels = util.GetPodAppSelector(application.Name) } - podTemplateLabels["app.kubernetes.io/version"] = resourceutils.GetImageVersion(application.Spec.Image) + podTemplateLabels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(application.Spec.Image) // Add annotations to pod template, safe-to-evict added due to issues // with cluster-autoscaler and unable to evict pods with local volumes diff --git a/pkg/resourcegenerator/job/job.go b/pkg/resourcegenerator/job/job.go index bb9c6921..5bb3e2e2 100644 --- a/pkg/resourcegenerator/job/job.go +++ b/pkg/resourcegenerator/job/job.go @@ -145,5 +145,5 @@ func getJobSpec(skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelec func setJobLabels(skipJob *skiperatorv1alpha1.SKIPJob, labels map[string]string) { labels["app"] = skipJob.KindPostFixedName() - labels["app.kubernetes.io/version"] = resourceutils.GetImageVersion(skipJob.Spec.Container.Image) + labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(skipJob.Spec.Container.Image) } diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 9f08a451..0583c556 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -7,7 +7,10 @@ import ( "strings" ) -const LabelValueMaxLength int = 63 +func matchesRegex(s string, pattern string) bool { + obj, err := regexp.Match(pattern, []byte(s)) + return obj && err == nil +} func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { replicas, err := skiperatorv1alpha1.GetStaticReplicas(jsonReplicas) @@ -21,42 +24,44 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { return false } -// MatchesRegex checks if a string matches a regexp -func matchesRegex(s string, pattern string) bool { - obj, err := regexp.Match(pattern, []byte(s)) - return obj && err == nil -} +// HumanReadableVersion returns the version part of an image string +func HumanReadableVersion(imageReference string) string { + const LabelValueMaxLength = 63 + + var allowedChars = regexp.MustCompile(`[A-Za-z0-9_.-]`) -// GetImageVersion returns the version part of an image string -func GetImageVersion(imageVersionString string) string { // Find position of first "@", remove it and everything after it - if strings.Contains(imageVersionString, "@") { - imageVersionString = strings.Split(imageVersionString, "@")[0] - imageVersionString = imageVersionString + ":unknown" + if strings.Contains(imageReference, "@") { + imageReference = strings.Split(imageReference, "@")[0] } - // If no version is given, assume "latest" - if !strings.Contains(imageVersionString, ":") { + lastColonPos := strings.LastIndex(imageReference, ":") + if lastColonPos == -1 || lastColonPos == len(imageReference)-1 { return "latest" } + versionPart := imageReference[lastColonPos+1:] + imageReference = imageReference[:lastColonPos] - // Split image string into parts - parts := strings.Split(imageVersionString, ":") - - versionPart := parts[1] + // While first character is not part of regex [a-z0-9A-Z] then remove it + for len(versionPart) > 0 && !matchesRegex(versionPart[:1], "[a-zA-Z0-9]") { + versionPart = versionPart[1:] + } - // Replace "+" with "-" in version text if version includes one - versionPart = strings.ReplaceAll(versionPart, "+", "-") + // For each character in versionPart, replace characters that are not allowed in label-value + var result strings.Builder + for _, c := range versionPart { + if allowedChars.MatchString(string(c)) { + result.WriteRune(c) + } else { + result.WriteRune('-') + } + } + versionPart = result.String() // Limit label-value to 63 characters if len(versionPart) > LabelValueMaxLength { versionPart = versionPart[:LabelValueMaxLength] } - // While first character is not part of regex [a-z0-9A-Z] then remove it - for len(versionPart) > 0 && !MatchesRegex(versionPart[:1], "[a-zA-Z0-9]") { - versionPart = versionPart[1:] - } - return versionPart } diff --git a/pkg/resourcegenerator/resourceutils/helpers_test.go b/pkg/resourcegenerator/resourceutils/helpers_test.go index abd9c152..877cc9fc 100644 --- a/pkg/resourcegenerator/resourceutils/helpers_test.go +++ b/pkg/resourcegenerator/resourceutils/helpers_test.go @@ -15,25 +15,38 @@ func TestVersions(t *testing.T) { {"image:latest", "latest"}, {"image:1.2.3-dev-123abc", "1.2.3-dev-123abc"}, {"image:1.2.3", "1.2.3"}, - {"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "unknown"}, + {"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "latest"}, {"ghcr.io/org/one-app:sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4"}, {"ghcr.io/org/another-app:3fb7048", "3fb7048"}, {"ghcr.io/org/some-team/third-app:v1.2.54", "v1.2.54"}, {"ghcr.io/org/another-team/fourth-app:4.0.0.rc-36", "4.0.0.rc-36"}, {"ghcr.io/org/another-team/fifth-app:4.0.0.rc-36-master-latest", "4.0.0.rc-36-master-latest"}, - {"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "unknown"}, + {"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "latest"}, {"ghcr.io/kartverket/vulnerability-disclosure-program:4.0.1@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "4.0.1"}, {"nginxinc/nginx-unprivileged:1.20.0-alpine", "1.20.0-alpine"}, {"foo/bar:1.2.3+build.4", "1.2.3-build.4"}, {"foo/bar:1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"}, + {"foo/bar:.1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXA", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXA"}, {"foo/bar:-1.2.3", "1.2.3"}, {"foo/bar:__1.2.3", "1.2.3"}, {"foo/bar:.1.2.3", "1.2.3"}, + {"foo/bar@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "latest"}, + {"foo/bar:latest@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "latest"}, + {"foo/bar:stable@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "stable"}, + {"foo/bar:unknown@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "unknown"}, + {"foo/bar:1.2.3@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "1.2.3"}, + {"foo/bar:1.2.3%suffix", "1.2.3-suffix"}, + {"foo/bar:1.2.3*suffix", "1.2.3-suffix"}, + {"foo/bar:1.2.3#suffix", "1.2.3-suffix"}, + {"foo/bar:1.2.3$suffix", "1.2.3-suffix"}, + {"foo/bar:1.2.3–suffix", "1.2.3-suffix"}, + {"foo/bar:1.2.3-suffix", "1.2.3-suffix"}, + {"registry:5000/foo/bar:1.2.3", "1.2.3"}, } for _, tc := range testCases { t.Run(tc.imageString, func(t *testing.T) { - actualValue := GetImageVersion(tc.imageString) + actualValue := HumanReadableVersion(tc.imageString) assert.Equal(t, tc.expectedValue, actualValue) }) } diff --git a/pkg/resourcegenerator/service/service.go b/pkg/resourcegenerator/service/service.go index 485879f5..03931bd9 100644 --- a/pkg/resourcegenerator/service/service.go +++ b/pkg/resourcegenerator/service/service.go @@ -37,7 +37,7 @@ func Generate(r reconciliation.Reconciliation) error { service := corev1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}} service.Labels = util.GetPodAppSelector(application.Name) - service.Labels["app.kubernetes.io/version"] = resourceutils.GetImageVersion(application.Spec.Image) + service.Labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(application.Spec.Image) ports := append(getAdditionalPorts(application.Spec.AdditionalPorts), getServicePort(application.Spec.Port, application.Spec.AppProtocol)) if r.IsIstioEnabled() { From 62f7219bf198feb68b018977da4e43245a1f27b4 Mon Sep 17 00:00:00 2001 From: Snorre Selmer Date: Fri, 8 Nov 2024 15:38:47 +0100 Subject: [PATCH 08/11] - Fixed incorrectly named Chainsaw-test that was failing --- tests/application/labels-imageversion/application-assert.yaml | 4 ++-- tests/application/labels-imageversion/application-patch.yaml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/application/labels-imageversion/application-assert.yaml b/tests/application/labels-imageversion/application-assert.yaml index db229208..adde42db 100644 --- a/tests/application/labels-imageversion/application-assert.yaml +++ b/tests/application/labels-imageversion/application-assert.yaml @@ -2,7 +2,7 @@ apiVersion: v1 kind: Service metadata: labels: - app.kubernetes.io/version: "1234567890123456" + app.kubernetes.io/version: "latest" --- @@ -10,4 +10,4 @@ apiVersion: v1 kind: Pod metadata: labels: - app.kubernetes.io/version: "1234567890123456" + app.kubernetes.io/version: latest diff --git a/tests/application/labels-imageversion/application-patch.yaml b/tests/application/labels-imageversion/application-patch.yaml index 0b20ae9a..abc5cd75 100644 --- a/tests/application/labels-imageversion/application-patch.yaml +++ b/tests/application/labels-imageversion/application-patch.yaml @@ -1,7 +1,7 @@ apiVersion: skiperator.kartverket.no/v1alpha1 kind: Application metadata: - name: imageversionshalabel + name: imageversionlabelsha spec: image: image@sha256:1234567890123456 port: 8080 From eade1cffabeffcc081a55c750322b5ddfad1a9c2 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Fri, 15 Nov 2024 15:46:25 +0100 Subject: [PATCH 09/11] Tidy up --- pkg/resourcegenerator/job/job.go | 8 +-- .../resourceutils/helpers.go | 56 ++++++++++--------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/pkg/resourcegenerator/job/job.go b/pkg/resourcegenerator/job/job.go index 5bb3e2e2..f73f9a71 100644 --- a/pkg/resourcegenerator/job/job.go +++ b/pkg/resourcegenerator/job/job.go @@ -2,6 +2,7 @@ package job import ( "fmt" + skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" "github.com/kartverket/skiperator/pkg/reconciliation" "github.com/kartverket/skiperator/pkg/resourcegenerator/gcp" @@ -136,13 +137,6 @@ func getJobSpec(skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelec return jobSpec } -//func getSkipJobVersion(skipJob *skiperatorv1alpha1.SKIPJob) string { -// if skipJob.Spec.Container.Image != "" { -// return resourceutils.GetImageVersion(skipJob.Spec.Container.Image) -// } -// return "" -//} - func setJobLabels(skipJob *skiperatorv1alpha1.SKIPJob, labels map[string]string) { labels["app"] = skipJob.KindPostFixedName() labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(skipJob.Spec.Container.Image) diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 0583c556..18d4414d 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -1,16 +1,21 @@ package resourceutils import ( - skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "regexp" "strings" + + skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" + "github.com/kartverket/skiperator/pkg/log" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) -func matchesRegex(s string, pattern string) bool { - obj, err := regexp.Match(pattern, []byte(s)) - return obj && err == nil -} +const LabelValueMaxLength = 63 + +var ( + logger = log.NewLogger().WithName("resourceutils") + allowedChars = regexp.MustCompile(`[A-Za-z0-9_.-]`) + allowedPrefixCharacters = regexp.MustCompile(`[a-zA-Z0-9]`) +) func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { replicas, err := skiperatorv1alpha1.GetStaticReplicas(jsonReplicas) @@ -26,41 +31,38 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { // HumanReadableVersion returns the version part of an image string func HumanReadableVersion(imageReference string) string { - const LabelValueMaxLength = 63 - - var allowedChars = regexp.MustCompile(`[A-Za-z0-9_.-]`) + processedImageRef := strings.Clone(imageReference) // Find position of first "@", remove it and everything after it - if strings.Contains(imageReference, "@") { - imageReference = strings.Split(imageReference, "@")[0] + if strings.Contains(processedImageRef, "@") { + processedImageRef = strings.Split(processedImageRef, "@")[0] } - lastColonPos := strings.LastIndex(imageReference, ":") - if lastColonPos == -1 || lastColonPos == len(imageReference)-1 { + lastColonPos := strings.LastIndex(processedImageRef, ":") + if lastColonPos == -1 || lastColonPos == len(processedImageRef)-1 { return "latest" } - versionPart := imageReference[lastColonPos+1:] - imageReference = imageReference[:lastColonPos] - // While first character is not part of regex [a-z0-9A-Z] then remove it - for len(versionPart) > 0 && !matchesRegex(versionPart[:1], "[a-zA-Z0-9]") { - versionPart = versionPart[1:] - } + versionPart := processedImageRef[lastColonPos+1:] + processedImageRef = processedImageRef[:lastColonPos] + + // Trim non-alphanumeric prefix + versionPart = strings.TrimLeftFunc(versionPart, func(r rune) bool { + return !allowedPrefixCharacters.MatchString(string(r)) + }) // For each character in versionPart, replace characters that are not allowed in label-value - var result strings.Builder - for _, c := range versionPart { - if allowedChars.MatchString(string(c)) { - result.WriteRune(c) - } else { - result.WriteRune('-') + versionPart = strings.Map(func(r rune) rune { + if allowedChars.MatchString(string(r)) { + return r } - } - versionPart = result.String() + return '-' + }, versionPart) // Limit label-value to 63 characters if len(versionPart) > LabelValueMaxLength { versionPart = versionPart[:LabelValueMaxLength] + logger.Info("Trimming version length because it too long", "ref", imageReference, "trimmedVersion", versionPart) } return versionPart From 80fb90dc4a8230b08e6f86b03cc1abfd104097da Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Mon, 18 Nov 2024 10:16:04 +0100 Subject: [PATCH 10/11] Use external logger --- .../deployment/deployment.go | 5 ++-- pkg/resourcegenerator/job/job.go | 21 +++++++------- .../resourceutils/helpers.go | 28 +++++++++++++++---- .../resourceutils/helpers_test.go | 5 +++- pkg/resourcegenerator/service/service.go | 5 ++-- 5 files changed, 43 insertions(+), 21 deletions(-) diff --git a/pkg/resourcegenerator/deployment/deployment.go b/pkg/resourcegenerator/deployment/deployment.go index f02f3c21..7682fbad 100644 --- a/pkg/resourcegenerator/deployment/deployment.go +++ b/pkg/resourcegenerator/deployment/deployment.go @@ -3,13 +3,14 @@ package deployment import ( goerrors "errors" "fmt" + "strings" + "github.com/kartverket/skiperator/pkg/reconciliation" "github.com/kartverket/skiperator/pkg/resourcegenerator/idporten" "github.com/kartverket/skiperator/pkg/resourcegenerator/maskinporten" "github.com/kartverket/skiperator/pkg/resourcegenerator/pod" "github.com/kartverket/skiperator/pkg/resourcegenerator/resourceutils" "github.com/kartverket/skiperator/pkg/resourcegenerator/volume" - "strings" "github.com/go-logr/logr" skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" @@ -108,7 +109,7 @@ func Generate(r reconciliation.Reconciliation) error { } else { podTemplateLabels = util.GetPodAppSelector(application.Name) } - podTemplateLabels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(application.Spec.Image) + podTemplateLabels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(&ctxLog, application.Spec.Image) // Add annotations to pod template, safe-to-evict added due to issues // with cluster-autoscaler and unable to evict pods with local volumes diff --git a/pkg/resourcegenerator/job/job.go b/pkg/resourcegenerator/job/job.go index f73f9a71..de411357 100644 --- a/pkg/resourcegenerator/job/job.go +++ b/pkg/resourcegenerator/job/job.go @@ -4,6 +4,7 @@ import ( "fmt" skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" + "github.com/kartverket/skiperator/pkg/log" "github.com/kartverket/skiperator/pkg/reconciliation" "github.com/kartverket/skiperator/pkg/resourcegenerator/gcp" "github.com/kartverket/skiperator/pkg/resourcegenerator/pod" @@ -32,7 +33,7 @@ func Generate(r reconciliation.Reconciliation) error { Labels: make(map[string]string), } - setJobLabels(skipJob, meta.Labels) + setJobLabels(&ctxLog, skipJob, meta.Labels) job := batchv1.Job{ObjectMeta: meta} cronJob := batchv1.CronJob{ObjectMeta: meta} @@ -48,17 +49,17 @@ func Generate(r reconciliation.Reconciliation) error { } if skipJob.Spec.Cron != nil { - cronJob.Spec = getCronJobSpec(skipJob, cronJob.Spec.JobTemplate.Spec.Selector, cronJob.Spec.JobTemplate.Spec.Template.Labels, r.GetIdentityConfigMap()) + cronJob.Spec = getCronJobSpec(&ctxLog, skipJob, cronJob.Spec.JobTemplate.Spec.Selector, cronJob.Spec.JobTemplate.Spec.Template.Labels, r.GetIdentityConfigMap()) r.AddResource(&cronJob) } else { - job.Spec = getJobSpec(skipJob, job.Spec.Selector, job.Spec.Template.Labels, r.GetIdentityConfigMap()) + job.Spec = getJobSpec(&ctxLog, skipJob, job.Spec.Selector, job.Spec.Template.Labels, r.GetIdentityConfigMap()) r.AddResource(&job) } return nil } -func getCronJobSpec(skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelector, podLabels map[string]string, gcpIdentityConfigMap *corev1.ConfigMap) batchv1.CronJobSpec { +func getCronJobSpec(logger *log.Logger, skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelector, podLabels map[string]string, gcpIdentityConfigMap *corev1.ConfigMap) batchv1.CronJobSpec { spec := batchv1.CronJobSpec{ Schedule: skipJob.Spec.Cron.Schedule, TimeZone: skipJob.Spec.Cron.TimeZone, @@ -69,19 +70,19 @@ func getCronJobSpec(skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelS ObjectMeta: metav1.ObjectMeta{ Labels: skipJob.GetDefaultLabels(), }, - Spec: getJobSpec(skipJob, selector, podLabels, gcpIdentityConfigMap), + Spec: getJobSpec(nil, skipJob, selector, podLabels, gcpIdentityConfigMap), }, SuccessfulJobsHistoryLimit: util.PointTo(int32(3)), FailedJobsHistoryLimit: util.PointTo(int32(1)), } // it's not a default label, maybe it could be? // used for selecting workloads by netpols, grafana etc - setJobLabels(skipJob, spec.JobTemplate.Labels) + setJobLabels(logger, skipJob, spec.JobTemplate.Labels) return spec } -func getJobSpec(skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelector, podLabels map[string]string, gcpIdentityConfigMap *corev1.ConfigMap) batchv1.JobSpec { +func getJobSpec(logger *log.Logger, skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelector, podLabels map[string]string, gcpIdentityConfigMap *corev1.ConfigMap) batchv1.JobSpec { podVolumes, containerVolumeMounts := volume.GetContainerVolumeMountsAndPodVolumes(skipJob.Spec.Container.FilesFrom) envVars := skipJob.Spec.Container.Env @@ -132,12 +133,12 @@ func getJobSpec(skipJob *skiperatorv1alpha1.SKIPJob, selector *metav1.LabelSelec // it's not a default label, maybe it could be? // used for selecting workloads by netpols, grafana etc - setJobLabels(skipJob, jobSpec.Template.Labels) + setJobLabels(logger, skipJob, jobSpec.Template.Labels) return jobSpec } -func setJobLabels(skipJob *skiperatorv1alpha1.SKIPJob, labels map[string]string) { +func setJobLabels(logger *log.Logger, skipJob *skiperatorv1alpha1.SKIPJob, labels map[string]string) { labels["app"] = skipJob.KindPostFixedName() - labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(skipJob.Spec.Container.Image) + labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(logger, skipJob.Spec.Container.Image) } diff --git a/pkg/resourcegenerator/resourceutils/helpers.go b/pkg/resourcegenerator/resourceutils/helpers.go index 18d4414d..e931e048 100644 --- a/pkg/resourcegenerator/resourceutils/helpers.go +++ b/pkg/resourcegenerator/resourceutils/helpers.go @@ -9,10 +9,13 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) -const LabelValueMaxLength = 63 +const ( + LabelValueMaxLength = 63 + defaultImageVersion = "latest" + unknownImageVersion = "err-unknown" +) var ( - logger = log.NewLogger().WithName("resourceutils") allowedChars = regexp.MustCompile(`[A-Za-z0-9_.-]`) allowedPrefixCharacters = regexp.MustCompile(`[a-zA-Z0-9]`) ) @@ -29,8 +32,21 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool { return false } -// HumanReadableVersion returns the version part of an image string -func HumanReadableVersion(imageReference string) string { +// HumanReadableVersion parses an image reference (e.g. "ghcr.io/org/some-team/some-app:v1.2.3") +// and returns a human-readable version string. If the image reference is empty, it returns a default +// unknown version string. The function removes any digest part (everything after and including "@") +// from the image reference, extracts the version part (everything after the last ":"), trims non-alphanumeric +// prefix characters from the version part, replaces any characters not allowed in Kubernetes label values +// with a hyphen ("-"), and limits the version string to a maximum length of 63 characters. If the version +// part is not found, it returns a default version string. +func HumanReadableVersion(logger *log.Logger, imageReference string) string { + logz := (*logger).GetLogger().WithValues("helper", "HumanReadableVersion") + + if len(imageReference) == 0 { + logz.Info("imageReference had length 0") + return unknownImageVersion + } + processedImageRef := strings.Clone(imageReference) // Find position of first "@", remove it and everything after it @@ -40,7 +56,7 @@ func HumanReadableVersion(imageReference string) string { lastColonPos := strings.LastIndex(processedImageRef, ":") if lastColonPos == -1 || lastColonPos == len(processedImageRef)-1 { - return "latest" + return defaultImageVersion } versionPart := processedImageRef[lastColonPos+1:] @@ -62,7 +78,7 @@ func HumanReadableVersion(imageReference string) string { // Limit label-value to 63 characters if len(versionPart) > LabelValueMaxLength { versionPart = versionPart[:LabelValueMaxLength] - logger.Info("Trimming version length because it too long", "ref", imageReference, "trimmedVersion", versionPart) + logz.Info("Trimming version length because it too long", "ref", imageReference, "trimmedVersion", versionPart) } return versionPart diff --git a/pkg/resourcegenerator/resourceutils/helpers_test.go b/pkg/resourcegenerator/resourceutils/helpers_test.go index 877cc9fc..d8512840 100644 --- a/pkg/resourcegenerator/resourceutils/helpers_test.go +++ b/pkg/resourcegenerator/resourceutils/helpers_test.go @@ -3,6 +3,7 @@ package resourceutils import ( "testing" + "github.com/kartverket/skiperator/pkg/log" "github.com/stretchr/testify/assert" ) @@ -44,9 +45,11 @@ func TestVersions(t *testing.T) { {"registry:5000/foo/bar:1.2.3", "1.2.3"}, } + logger := log.NewLogger() + for _, tc := range testCases { t.Run(tc.imageString, func(t *testing.T) { - actualValue := HumanReadableVersion(tc.imageString) + actualValue := HumanReadableVersion(&logger, tc.imageString) assert.Equal(t, tc.expectedValue, actualValue) }) } diff --git a/pkg/resourcegenerator/service/service.go b/pkg/resourcegenerator/service/service.go index 03931bd9..261d8184 100644 --- a/pkg/resourcegenerator/service/service.go +++ b/pkg/resourcegenerator/service/service.go @@ -2,6 +2,8 @@ package service import ( "fmt" + "strings" + skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" "github.com/kartverket/skiperator/api/v1alpha1/podtypes" "github.com/kartverket/skiperator/pkg/reconciliation" @@ -10,7 +12,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "strings" ) const defaultPortName = "http" @@ -37,7 +38,7 @@ func Generate(r reconciliation.Reconciliation) error { service := corev1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}} service.Labels = util.GetPodAppSelector(application.Name) - service.Labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(application.Spec.Image) + service.Labels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(&ctxLog, application.Spec.Image) ports := append(getAdditionalPorts(application.Spec.AdditionalPorts), getServicePort(application.Spec.Port, application.Spec.AppProtocol)) if r.IsIstioEnabled() { From 639e50163ad7dcdc9597e387a4bc95e2b25e9c10 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Mon, 18 Nov 2024 11:30:15 +0100 Subject: [PATCH 11/11] bugfix --- pkg/resourcegenerator/job/job.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/resourcegenerator/job/job.go b/pkg/resourcegenerator/job/job.go index de411357..56098361 100644 --- a/pkg/resourcegenerator/job/job.go +++ b/pkg/resourcegenerator/job/job.go @@ -70,7 +70,7 @@ func getCronJobSpec(logger *log.Logger, skipJob *skiperatorv1alpha1.SKIPJob, sel ObjectMeta: metav1.ObjectMeta{ Labels: skipJob.GetDefaultLabels(), }, - Spec: getJobSpec(nil, skipJob, selector, podLabels, gcpIdentityConfigMap), + Spec: getJobSpec(logger, skipJob, selector, podLabels, gcpIdentityConfigMap), }, SuccessfulJobsHistoryLimit: util.PointTo(int32(3)), FailedJobsHistoryLimit: util.PointTo(int32(1)),