Fix jobsink name generator + add unit and fuzz tests

Signed-off-by: Pierangelo Di Pilato <[email protected]>
knative · Nov 18, 2024 · f88177f · f88177f
1 parent c9fc3cf
commit f88177f
Show file tree

Hide file tree

Showing 2 changed files with 102 additions and 12 deletions.
diff --git a/cmd/jobsink/main.go b/cmd/jobsink/main.go
@@ -18,8 +18,10 @@ package main
 
 import (
 	"context"
-	"crypto/md5" //nolint:gosec
+	"crypto/md5"
+	//nolint:gosec
 	"crypto/tls"
+	"encoding/hex"
 	"fmt"
 	"log"
 	"net/http"
@@ -231,11 +233,11 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	id := toIdHashLabelValue(event.Source(), event.ID())
-	logger.Debug("Getting job for event", zap.String("URI", r.RequestURI), zap.String("id", id))
+	jobName := toJobName(ref.Name, event.Source(), event.ID())
+	logger.Debug("Getting job for event", zap.String("URI", r.RequestURI), zap.String("jobName", jobName))
 
 	jobs, err := h.k8s.BatchV1().Jobs(js.GetNamespace()).List(r.Context(), metav1.ListOptions{
-		LabelSelector: jobLabelSelector(ref, id),
+		LabelSelector: jobLabelSelector(ref, jobName),
 		Limit:         1,
 	})
 	if err != nil {
@@ -256,16 +258,14 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	jobName := kmeta.ChildName(ref.Name, id)
-
 	logger.Debug("Creating job for event", zap.String("URI", r.RequestURI), zap.String("jobName", jobName))
 
 	job := js.Spec.Job.DeepCopy()
 	job.Name = jobName
 	if job.Labels == nil {
 		job.Labels = make(map[string]string, 4)
 	}
-	job.Labels[sinks.JobSinkIDLabel] = id
+	job.Labels[sinks.JobSinkIDLabel] = jobName
 	job.Labels[sinks.JobSinkNameLabel] = ref.Name
 	job.OwnerReferences = append(job.OwnerReferences, metav1.OwnerReference{
 		APIVersion:         sinksv.SchemeGroupVersion.String(),
@@ -332,7 +332,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			Name:      jobName,
 			Namespace: ref.Namespace,
 			Labels: map[string]string{
-				sinks.JobSinkIDLabel:   id,
+				sinks.JobSinkIDLabel:   jobName,
 				sinks.JobSinkNameLabel: ref.Name,
 			},
 			OwnerReferences: []metav1.OwnerReference{
@@ -396,8 +396,7 @@ func (h *Handler) handleGet(ctx context.Context, w http.ResponseWriter, r *http.
 	eventSource := parts[6]
 	eventID := parts[8]
 
-	id := toIdHashLabelValue(eventSource, eventID)
-	jobName := kmeta.ChildName(ref.Name, id)
+	jobName := toJobName(ref.Name, eventSource, eventID)
 
 	job, err := h.k8s.BatchV1().Jobs(ref.Namespace).Get(r.Context(), jobName, metav1.GetOptions{})
 	if err != nil {
@@ -450,6 +449,7 @@ func jobLabelSelector(ref types.NamespacedName, id string) string {
 	return fmt.Sprintf("%s=%s,%s=%s", sinks.JobSinkIDLabel, id, sinks.JobSinkNameLabel, ref.Name)
 }
 
-func toIdHashLabelValue(source, id string) string {
-	return utils.ToDNS1123Subdomain(fmt.Sprintf("%s", md5.Sum([]byte(fmt.Sprintf("%s-%s", source, id))))) //nolint:gosec
+func toJobName(js string, source, id string) string {
+	h := md5.Sum([]byte(source+id))
+	return kmeta.ChildName(js, "-" + utils.ToDNS1123Subdomain(hex.EncodeToString(h[:])))
 }
diff --git a/cmd/jobsink/main_test.go b/cmd/jobsink/main_test.go
@@ -0,0 +1,90 @@
+package main
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/api/validation"
+
+	"knative.dev/eventing/pkg/utils"
+)
+
+type testCase struct {
+	JobSinkName string
+	Source      string
+	Id          string
+}
+
+func TestToJobName(t *testing.T) {
+	testcases := []testCase{
+		{
+			JobSinkName: "job-sink-success",
+			Source:      "mysource3/myservice",
+			Id:          "2234-5678",
+		},
+		{
+			JobSinkName: "a",
+			Source:      "0",
+			Id:          "0",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.JobSinkName + "_" + tc.Source + "_" + tc.Id, func(t *testing.T) {
+			if errs := validation.NameIsDNS1035Label(tc.JobSinkName, false); len(errs) != 0 {
+				t.Errorf("Invalid JobSinkName: %v", errs)
+			}
+
+			name := toJobName(tc.JobSinkName, tc.Source, tc.Id)
+			doubleName := toJobName(tc.JobSinkName, tc.Source, tc.Id)
+			if name != doubleName {
+				t.Errorf("Before: %q, after: %q", name, doubleName)
+			}
+
+			if got := utils.ToDNS1123Subdomain(name); got != name {
+				t.Errorf("ToDNS1123Subdomain(Want) returns a different result, Want: %q, Got: %q", name, got)
+			}
+
+			if errs := validation.NameIsDNS1035Label(name, false); len(errs) != 0 {
+				t.Errorf("toJobName produced invalid name %q given %q, %q, %q: errors: %#v", name, tc.JobSinkName, tc.Source, tc.Id, errs)
+			}
+		})
+	}
+}
+
+func FuzzToJobName(f *testing.F) {
+	testcases := []testCase{
+		{
+			JobSinkName: "job-sink-success",
+			Source:      "mysource3/myservice",
+			Id:          "2234-5678",
+		},
+		{
+			JobSinkName: "a",
+			Source:      "0",
+			Id:          "0",
+		},
+	}
+
+	for _, tc := range testcases {
+		f.Add(tc.JobSinkName, tc.Source, tc.Id) // Use f.Add to provide a seed corpus
+	}
+	f.Fuzz(func(t *testing.T, js, source, id string) {
+		if errs := validation.NameIsDNS1035Label(js, false); len(errs) != 0 {
+			t.Skip("Prerequisite: invalid jobsink name")
+		}
+
+		name := toJobName(js, source, id)
+		doubleName := toJobName(js, source, id)
+		if name != doubleName {
+			t.Errorf("Before: %q, after: %q", name, doubleName)
+		}
+
+		if got := utils.ToDNS1123Subdomain(name); got != name {
+			t.Errorf("ToDNS1123Subdomain(Want) returns a different result, Want: %q, Got: %q", name, got)
+		}
+
+		if errs := validation.NameIsDNS1035Label(name, false); len(errs) != 0 {
+			t.Errorf("toJobName produced invalid name %q given %q, %q, %q: errors: %#v", name, js, source, id, errs)
+		}
+	})
+}