From d3a0ec142c963f260d3512365efb05394ad4e9cf Mon Sep 17 00:00:00 2001 From: everettraven Date: Wed, 6 Nov 2024 12:03:13 -0500 Subject: [PATCH] address review comments Signed-off-by: everettraven --- api/core/v1alpha1/clustercatalog_types.go | 17 ++++++------ .../v1alpha1/clustercatalog_types_test.go | 27 ++++++++++++------- ....operatorframework.io_clustercatalogs.yaml | 16 +++++------ .../core/clustercatalog_controller.go | 4 +-- .../core/clustercatalog_controller_test.go | 5 +++- internal/source/containers_image.go | 16 +++++------ 6 files changed, 47 insertions(+), 38 deletions(-) diff --git a/api/core/v1alpha1/clustercatalog_types.go b/api/core/v1alpha1/clustercatalog_types.go index 0d52ad52..1f24ddd1 100644 --- 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 index 0e62d178..4ba10ba0 100644 --- 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,7 +175,7 @@ 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": { @@ -183,7 +183,7 @@ func TestResolvedImageSourceCELValidation(t *testing.T) { 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": { @@ -191,7 +191,7 @@ func TestResolvedImageSourceCELValidation(t *testing.T) { 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": { @@ -199,7 +199,7 @@ func TestResolvedImageSourceCELValidation(t *testing.T) { 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": { @@ -207,7 +207,7 @@ func TestResolvedImageSourceCELValidation(t *testing.T) { 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": { @@ -215,7 +215,7 @@ func TestResolvedImageSourceCELValidation(t *testing.T) { 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": { @@ -223,7 +223,16 @@ func TestResolvedImageSourceCELValidation(t *testing.T) { 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 index b28d63ba..3852e5db 100644 --- 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 index 72ad6b6f..1e7fe453 100644 --- 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 index 50f43a4c..93a2cbee 100644 --- 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 index 2a808de1..7f3035f5 100644 --- 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)), } }