address review comments

Signed-off-by: everettraven <everettraven@gmail.com>
operator-framework · Nov 6, 2024 · d3a0ec1 · d3a0ec1
1 parent 5b081f0
commit d3a0ec1
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 38 deletions.
diff --git a/api/core/v1alpha1/clustercatalog_types.go b/api/core/v1alpha1/clustercatalog_types.go
@@ -206,21 +206,19 @@ type ClusterCatalogStatus struct {
 // +kubebuilder:validation:XValidation:rule="isURL(self.base)",message="base must be a valid URL"
 // +kubebuilder:validation:XValidation:rule="isURL(self.base) ? (url(self.base).getScheme() == \"http\" || url(self.base).getScheme() == \"https\") : true",message="base is invalid, scheme must be one of [http, https]"
 type ClusterCatalogURLs struct {
-	// base is a cluster-internal URL that provides REST API endpoints for
+	// base is a cluster-internal URL that provides endpoints for
 	// accessing the content of the catalog.
 	//
-	// It is expected that users append the path for the REST API endpoint they wish
-	// to access. The general format expected for a request to a REST API endpoint is
-	// {base}/api/{version}/{endpoint}
+	// It is expected that client append the path for the endpoint they wish
+	// to access.
 	//
-	// Currently, only a single version of the REST API is served and is accessible at the path
+	// Currently, only a single version of the is served and is accessible at the path
 	// /api/v1.
 	//
-	// The endpoints served for v1 of the REST API are:
+	// The endpoints served for v1 of the are:
 	//   - /all - this endpoint returns the entirety of the catalog contents in the FBC format
 	//
-	// As the needs of users and clients of the REST API evolve, new versions of the
-	// REST API may be added.
+	// As the needs of users and clients of the evolve, new endpoints may be added.
 	//
 	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:MaxLength:=525
@@ -279,6 +277,9 @@ type ResolvedCatalogSource struct {
 // Check for the existence of a valid name
 // +kubebuilder:validation:XValidation:rule="self.ref.find('(\\\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?((\\\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?)+)?)') != \"\"",message="ref is invalid, a valid name is required. the name must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters."
 //
+// Check for the existence of a digest reference at the end of the image reference
+// +kubebuilder:validation:XValidation:rule="self.ref.find('(@.*:)') != \"\"",message="ref is invalid, must end with a digest"
+//
 // If we find a digest in the reference, check that the digest algorithm is valid
 // +kubebuilder:validation:XValidation:rule="self.ref.find('(@.*:)') != \"\" ? self.ref.find('(@.*:)').matches('(@[A-Za-z][A-Za-z0-9]*([-_+.][A-Za-z][A-Za-z0-9]*)*[:])') : true",message="ref is invalid, digest algorithm is not valid. the algorithm must start with an uppercase or lowercase alpha character followed by alphanumeric characters and may contain the \"-\", \"_\", \"+\", and \".\" characters."
 //

diff --git a/api/core/v1alpha1/clustercatalog_types_test.go b/api/core/v1alpha1/clustercatalog_types_test.go
@@ -95,7 +95,7 @@ func TestImageSourceCELValidationRules(t *testing.T) {
 		},
 		"invalid tag based image ref, tag too long": {
 			spec: ImageSource{
-				Ref: fmt.Sprintf("docker.io/foo/bar:%s", strings.Repeat("x", 200)),
+				Ref: fmt.Sprintf("docker.io/foo/bar:%s", strings.Repeat("x", 128)),
 			},
 			wantErrs: []string{
 				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, tag is invalid. the tag must not be more than 127 characters",
@@ -156,7 +156,7 @@ func TestImageSourceCELValidationRules(t *testing.T) {
 
 func TestResolvedImageSourceCELValidation(t *testing.T) {
 	validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
-	pth := "openAPIV3Schema.properties.spec.properties.source.properties.image"
+	pth := "openAPIV3Schema.properties.status.properties.resolvedSource.properties.image"
 	validator, found := validators["v1alpha1"][pth]
 	require.True(t, found)
 
@@ -175,55 +175,64 @@ func TestResolvedImageSourceCELValidation(t *testing.T) {
 				Ref: "-quay+docker/foo/bar@sha256:abcdef123456789abcdef123456789abc",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, it must start with a valid domain. the domain must be alphanumeric characters (lowercase and uppercase) separated by the \".\" character.",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, it must start with a valid domain. the domain must be alphanumeric characters (lowercase and uppercase) separated by the \".\" character.",
 			},
 		},
 		"invalid digest based image ref, invalid name": {
 			spec: ImageSource{
 				Ref: "docker.io/FOO/BAR@sha256:abcdef123456789abcdef123456789abc",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, a valid name is required. the name must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, a valid name is required. the name must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
 			},
 		},
 		"invalid digest based image ref, invalid digest algorithm": {
 			spec: ImageSource{
 				Ref: "docker.io/foo/bar@99-problems:abcdef123456789abcdef123456789abc",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, digest algorithm is not valid. the algorithm must start with an uppercase or lowercase alpha character followed by alphanumeric characters and may contain the \"-\", \"_\", \"+\", and \".\" characters.",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, digest algorithm is not valid. the algorithm must start with an uppercase or lowercase alpha character followed by alphanumeric characters and may contain the \"-\", \"_\", \"+\", and \".\" characters.",
 			},
 		},
 		"invalid digest based image ref, too short digest encoding": {
 			spec: ImageSource{
 				Ref: "docker.io/foo/bar@sha256:abcdef123456789",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, digest is not valid. the encoded string must be at least 32 characters",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, digest is not valid. the encoded string must be at least 32 characters",
 			},
 		},
 		"invalid digest based image ref, invalid characters in digest encoding": {
 			spec: ImageSource{
 				Ref: "docker.io/foo/bar@sha256:XYZxy123456789abcdef123456789abc",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, digest is not valid. the encoded string must only contain hex characters (A-F, a-f, 0-9)",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, digest is not valid. the encoded string must only contain hex characters (A-F, a-f, 0-9)",
 			},
 		},
 		"invalid image ref, no digest": {
 			spec: ImageSource{
 				Ref: "docker.io/foo/bar",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, must end with a digest or a tag",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, must end with a digest",
 			},
 		},
 		"invalid image ref, only domain with port": {
 			spec: ImageSource{
 				Ref: "docker.io:8080",
 			},
 			wantErrs: []string{
-				"openAPIV3Schema.properties.spec.properties.source.properties.image: Invalid value: \"object\": ref is invalid, a valid name is required. the name must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, a valid name is required. the name must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters.",
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, must end with a digest",
+			},
+		},
+		"invalid image ref, tag-based ref": {
+			spec: ImageSource{
+				Ref: "docker.io/foo/bar:latest",
+			},
+			wantErrs: []string{
+				"openAPIV3Schema.properties.status.properties.resolvedSource.properties.image: Invalid value: \"object\": ref is invalid, must end with a digest",
 			},
 		},
 	} {

diff --git a/config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml b/config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml
@@ -362,6 +362,8 @@ spec:
                         by the ".", "_", "__", "-" characters.
                       rule: self.ref.find('(\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?((\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?)+)?)')
                         != ""
+                    - message: ref is invalid, must end with a digest
+                      rule: self.ref.find('(@.*:)') != ""
                     - message: ref is invalid, digest algorithm is not valid. the
                         algorithm must start with an uppercase or lowercase alpha
                         character followed by alphanumeric characters and may contain
@@ -402,21 +404,19 @@ spec:
                 properties:
                   base:
                     description: |-
-                      base is a cluster-internal URL that provides REST API endpoints for
+                      base is a cluster-internal URL that provides endpoints for
                       accessing the content of the catalog.
 
-                      It is expected that users append the path for the REST API endpoint they wish
-                      to access. The general format expected for a request to a REST API endpoint is
-                      {base}/api/{version}/{endpoint}
+                      It is expected that client append the path for the endpoint they wish
+                      to access.
 
-                      Currently, only a single version of the REST API is served and is accessible at the path
+                      Currently, only a single version of the is served and is accessible at the path
                       /api/v1.
 
-                      The endpoints served for v1 of the REST API are:
+                      The endpoints served for v1 of the are:
                         - /all - this endpoint returns the entirety of the catalog contents in the FBC format
 
-                      As the needs of users and clients of the REST API evolve, new versions of the
-                      REST API may be added.
+                      As the needs of users and clients of the evolve, new endpoints may be added.
                     maxLength: 525
                     type: string
                 required:

diff --git a/internal/controllers/core/clustercatalog_controller.go b/internal/controllers/core/clustercatalog_controller.go
@@ -29,6 +29,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -330,8 +331,7 @@ func updateStatusServing(status *v1alpha1.ClusterCatalogStatus, result source.Re
 		status.URLs = &v1alpha1.ClusterCatalogURLs{}
 	}
 	status.URLs.Base = baseURL
-	lastUnpackedTime := metav1.NewTime(result.UnpackTime)
-	status.LastUnpacked = &lastUnpackedTime
+	status.LastUnpacked = ptr.To(metav1.NewTime(result.UnpackTime))
 	meta.SetStatusCondition(&status.Conditions, metav1.Condition{
 		Type:               v1alpha1.TypeServing,
 		Status:             metav1.ConditionTrue,

diff --git a/internal/controllers/core/clustercatalog_controller_test.go b/internal/controllers/core/clustercatalog_controller_test.go
@@ -775,6 +775,7 @@ func TestPollingRequeue(t *testing.T) {
 	for name, tc := range map[string]struct {
 		catalog              *catalogdv1alpha1.ClusterCatalog
 		expectedRequeueAfter time.Duration
+		lastPollTime         metav1.Time
 	}{
 		"ClusterCatalog with tag based image ref without any poll interval specified, requeueAfter set to 0, ie polling disabled": {
 			catalog: &catalogdv1alpha1.ClusterCatalog{
@@ -792,6 +793,7 @@ func TestPollingRequeue(t *testing.T) {
 				},
 			},
 			expectedRequeueAfter: time.Second * 0,
+			lastPollTime:         metav1.Now(),
 		},
 		"ClusterCatalog with tag based image ref with poll interval specified, requeueAfter set to wait.jitter(pollInterval)": {
 			catalog: &catalogdv1alpha1.ClusterCatalog{
@@ -810,6 +812,7 @@ func TestPollingRequeue(t *testing.T) {
 				},
 			},
 			expectedRequeueAfter: time.Minute * 5,
+			lastPollTime:         metav1.Now(),
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
@@ -823,7 +826,7 @@ func TestPollingRequeue(t *testing.T) {
 							Ref: "my.org/someImage@someSHA256Digest",
 						},
 					},
-					LastSuccessfulPollAttempt: metav1.Now(),
+					LastSuccessfulPollAttempt: tc.lastPollTime,
 				}},
 				Storage:        &MockStore{},
 				storedCatalogs: map[string]storedCatalogData{},

diff --git a/internal/source/containers_image.go b/internal/source/containers_image.go
@@ -173,22 +173,18 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa
 			Type: catalogdv1alpha1.SourceTypeImage,
 			Image: &catalogdv1alpha1.ResolvedImageSource{
 				Ref: canonicalRef.String(),
-
-				// We truncate to the second because metav1.Time is serialized
-				// as RFC 3339 which only has second-level precision. When we
-				// use this result in a comparison with what we deserialized
-				// from the Kubernetes API server, we need it to match.
 			},
 		},
-		State:                     StateUnpacked,
-		Message:                   fmt.Sprintf("unpacked %q successfully", canonicalRef),
-		LastSuccessfulPollAttempt: metav1.NewTime(time.Now().Truncate(time.Second)),
+		State:   StateUnpacked,
+		Message: fmt.Sprintf("unpacked %q successfully", canonicalRef),
 
-		// We truncate to the second because metav1.Time is serialized
+		// We truncate both the unpack time and last successful poll attempt
+		// to the second because metav1.Time is serialized
 		// as RFC 3339 which only has second-level precision. When we
 		// use this result in a comparison with what we deserialized
 		// from the Kubernetes API server, we need it to match.
-		UnpackTime: lastUnpacked.Truncate(time.Second),
+		UnpackTime:                lastUnpacked.Truncate(time.Second),
+		LastSuccessfulPollAttempt: metav1.NewTime(time.Now().Truncate(time.Second)),
 	}
 }