Merge pull request #557 from kartverket/SKIP-1159

SKIP-1159 - A better labeler

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.GetImageVersion(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

pkg/resourcegenerator/job/job.go

@@ -2,7 +2,9 @@ package job
 
 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"
@@ -31,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}
 
@@ -47,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,
@@ -68,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(logger, 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
 
@@ -131,19 +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 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) {
+func setJobLabels(logger *log.Logger, 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(logger, skipJob.Spec.Container.Image)
 }

pkg/resourcegenerator/resourceutils/helpers.go

@@ -1,9 +1,23 @@
 package resourceutils
 
 import (
+	"regexp"
+	"strings"
+
 	skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
+	"github.com/kartverket/skiperator/pkg/log"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	"strings"
+)
+
+const (
+	LabelValueMaxLength = 63
+	defaultImageVersion = "latest"
+	unknownImageVersion = "err-unknown"
+)
+
+var (
+	allowedChars            = regexp.MustCompile(`[A-Za-z0-9_.-]`)
+	allowedPrefixCharacters = regexp.MustCompile(`[a-zA-Z0-9]`)
 )
 
 func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool {
@@ -18,24 +32,54 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool {
 	return false
 }
 
-func GetImageVersion(imageVersionString string) string {
-	parts := strings.Split(imageVersionString, ":")
+// 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")
 
-	// Implicitly assume version "latest" if no version is specified
-	if len(parts) < 2 {
-		return "latest"
+	if len(imageReference) == 0 {
+		logz.Info("imageReference had length 0")
+		return unknownImageVersion
 	}
 
-	versionPart := parts[1]
+	processedImageRef := strings.Clone(imageReference)
 
-	// Remove "@sha256" from version text if version includes a hash
-	if strings.Contains(versionPart, "@") {
-		versionPart = strings.Split(versionPart, "@")[0]
+	// Find position of first "@", remove it and everything after it
+	if strings.Contains(processedImageRef, "@") {
+		processedImageRef = strings.Split(processedImageRef, "@")[0]
 	}
 
-	// Add build number to version if it is specified
-	if len(parts) > 2 {
-		return versionPart + "+" + parts[2]
+	lastColonPos := strings.LastIndex(processedImageRef, ":")
+	if lastColonPos == -1 || lastColonPos == len(processedImageRef)-1 {
+		return defaultImageVersion
 	}
+
+	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
+	versionPart = strings.Map(func(r rune) rune {
+		if allowedChars.MatchString(string(r)) {
+			return r
+		}
+		return '-'
+	}, versionPart)
+
+	// Limit label-value to 63 characters
+	if len(versionPart) > LabelValueMaxLength {
+		versionPart = versionPart[:LabelValueMaxLength]
+		logz.Info("Trimming version length because it too long", "ref", imageReference, "trimmedVersion", versionPart)
+	}
+
 	return versionPart
 }

pkg/resourcegenerator/resourceutils/helpers_test.go

@@ -1,47 +1,56 @@
 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/kartverket/skiperator/pkg/log"
+	"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", "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", "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"},
+	}
+
+	logger := log.NewLogger()
+
+	for _, tc := range testCases {
+		t.Run(tc.imageString, func(t *testing.T) {
+			actualValue := HumanReadableVersion(&logger, tc.imageString)
+			assert.Equal(t, tc.expectedValue, actualValue)
+		})
+	}
 }

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.GetImageVersion(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() {

tests/application/labels-imageversion/application-assert.yaml

@@ -2,12 +2,12 @@ apiVersion: v1
 kind: Service
 metadata:
   labels:
-    app.kubernetes.io/version: "1234567890123456"
+    app.kubernetes.io/version: "latest"
 
 ---
 
 apiVersion: v1
 kind: Pod
 metadata:
   labels:
-    app.kubernetes.io/version: "1234567890123456"
+    app.kubernetes.io/version: latest

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