⚠️ types tightening (#384)

* types tightening * switch to status.lastUnpacked for sole unpacked timestamp, use CEL validation for spec.source and status.resolvedSource Signed-off-by: Ankita Thomas <[email protected]> * rename unpacker result's lastTransitionTime to unpackTime, set observedGeneration in conditions * restructure validation checks to only check for image source type Signed-off-by: Ankita Thomas <[email protected]> * fail on missing validator in types test Signed-off-by: Ankita Thomas <[email protected]> * don't skip bad image refs when testing for needsUnpack allow bad image refs to pass through to unpacker to propagate error to Progressing status Signed-off-by: Ankita Thomas <[email protected]> * compare catalog generation and condition ObservedGeneration for needsUnpack Signed-off-by: Ankita Thomas <[email protected]> --------- Signed-off-by: Ankita Thomas <[email protected]> Co-authored-by: Ankita Thomas <[email protected]>
operator-framework · Oct 2, 2024 · 29d21c7 · 29d21c7
1 parent db8617f
commit 29d21c7
Show file tree

Hide file tree

Showing 16 changed files with 277 additions and 230 deletions.
diff --git a/README.md b/README.md
@@ -24,7 +24,7 @@ Procedure steps marked with an asterisk (`*`) are likely to change with future A
  name: operatorhubio
  spec:
  source:
- type: image
+ type: Image
  image:
  ref: quay.io/operatorhubio/catalog:latest
  EOF
@@ -57,7 +57,7 @@ Procedure steps marked with an asterisk (`*`) are likely to change with future A
  Image:
  Poll Interval: 10m0s
  Ref: quay.io/operatorhubio/catalog:latest
- Type: image
+ Type: Image
  Status:
  Conditions:
  Last Transition Time: 2024-09-12T13:37:53Z
@@ -79,7 +79,7 @@ Procedure steps marked with an asterisk (`*`) are likely to change with future A
  Last Unpacked: 2024-09-12T13:37:52Z
  Ref: quay.io/operatorhubio/catalog:latest
  Resolved Ref: quay.io/operatorhubio/catalog@sha256:4453a361198d39d0390fd8c1a7f07b5a5a3ae1e8dac9979ef0c4eba46299df16
- Type: image
+ Type: Image
  Events: <none>
  ```
 

diff --git a/api/core/v1alpha1/clustercatalog_types.go b/api/core/v1alpha1/clustercatalog_types.go
@@ -25,7 +25,7 @@ import (
 type SourceType string
 
 const (
- SourceTypeImage SourceType = "image"
+ SourceTypeImage SourceType = "Image"
 
  TypeProgressing = "Progressing"
  TypeServing = "Serving"
@@ -77,7 +77,7 @@ type ClusterCatalogSpec struct {
  // Below is a minimal example of a ClusterCatalogSpec that sources a catalog from an image:
  //
  // source:
- // type: image
+ // type: Image
  // image:
  // ref: quay.io/operatorhubio/catalog:latest
  //
@@ -91,7 +91,7 @@ type ClusterCatalogSpec struct {
  // When omitted, the default priority is 0.
  // +kubebuilder:default:=0
  // +optional
- Priority int32 `json:"priority,omitempty"`
+ Priority int32 `json:"priority"`
 }
 
 // ClusterCatalogStatus defines the observed state of ClusterCatalog
@@ -128,51 +128,54 @@ type ClusterCatalogStatus struct {
  // lastUnpacked: "2024-09-10T12:22:13Z"
  // ref: quay.io/operatorhubio/catalog:latest
  // resolvedRef: quay.io/operatorhubio/catalog@sha256:c7392b4be033da629f9d665fec30f6901de51ce3adebeff0af579f311ee5cf1b
- // type: image
+ // type: Image
  // +optional
  ResolvedSource *ResolvedCatalogSource `json:"resolvedSource,omitempty"`
  // contentURL is a cluster-internal URL from which on-cluster components
  // can read the content of a catalog
  // +optional
  ContentURL string `json:"contentURL,omitempty"`
- // observedGeneration is the most recent generation observed for this ClusterCatalog. It corresponds to the
- // ClusterCatalog's generation, which is updated on mutation by the API Server.
- // +optional
- ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+
  // lastUnpacked represents the time when the
- // ClusterCatalog object was last unpacked.
+ // ClusterCatalog object was last unpacked successfully.
  // +optional
  LastUnpacked metav1.Time `json:"lastUnpacked,omitempty"`
 }
 
 // CatalogSource is a discriminated union of possible sources for a Catalog.
+// CatalogSource contains the sourcing information for a Catalog
+// +union
+// +kubebuilder:validation:XValidation:rule="self.type == 'Image' && has(self.image)",message="source type 'Image' requires image field"
 type CatalogSource struct {
  // type is a required reference to the type of source the catalog is sourced from.
  //
- // Allowed values are ["image"]
+ // Allowed values are ["Image"]
  //
- // When this field is set to "image", the ClusterCatalog content will be sourced from an OCI image.
+ // When this field is set to "Image", the ClusterCatalog content will be sourced from an OCI image.
  // When using an image source, the image field must be set and must be the only field defined for this type.
  //
  // +unionDiscriminator
- // +kubebuilder:validation:Enum:="image"
+ // +kubebuilder:validation:Enum:="Image"
  // +kubebuilder:validation:Required
  Type SourceType `json:"type"`
- // image is used to configure how catalog contents are sourced from an OCI image. This field must be set when type is set to "image" and must be the only field defined for this type.
+ // image is used to configure how catalog contents are sourced from an OCI image. This field must be set when type is set to "Image" and must be the only field defined for this type.
  // +optional
  Image *ImageSource `json:"image,omitempty"`
 }
 
 // ResolvedCatalogSource is a discriminated union of resolution information for a Catalog.
+// ResolvedCatalogSource contains the information about a sourced Catalog
+// +union
+// +kubebuilder:validation:XValidation:rule="self.type == 'Image' && has(self.image)",message="source type 'Image' requires image field"
 type ResolvedCatalogSource struct {
  // type is a reference to the type of source the catalog is sourced from.
  //
- // It will be set to one of the following values: ["image"].
+ // It will be set to one of the following values: ["Image"].
  //
- // When this field is set to "image", information about the resolved image source will be set in the 'image' field.
+ // When this field is set to "Image", information about the resolved image source will be set in the 'image' field.
  //
  // +unionDiscriminator
- // +kubebuilder:validation:Enum:="image"
+ // +kubebuilder:validation:Enum:="Image"
  // +kubebuilder:validation:Required
  Type SourceType `json:"type"`
  // image is a field containing resolution information for a catalog sourced from an image.
@@ -181,14 +184,10 @@ type ResolvedCatalogSource struct {
 
 // ResolvedImageSource provides information about the resolved source of a Catalog sourced from an image.
 type ResolvedImageSource struct {
- // ref is the reference to a container image containing Catalog contents.
+ // ref contains the resolved sha256 image ref containing Catalog contents.
  Ref string `json:"ref"`
- // resolvedRef is the resolved sha256 image ref containing Catalog contents.
- ResolvedRef string `json:"resolvedRef"`
- // lastPollAttempt is the time when the source image was last polled for new content.
- LastPollAttempt metav1.Time `json:"lastPollAttempt"`
- // lastUnpacked is the last time when the Catalog contents were successfully unpacked.
- LastUnpacked metav1.Time `json:"lastUnpacked"`
+ // lastSuccessfulPollAttempt is the time when the resolved source was last successfully polled for new content.
+ LastSuccessfulPollAttempt metav1.Time `json:"lastSuccessfulPollAttempt"`
 }
 
 // ImageSource enables users to define the information required for sourcing a Catalog from an OCI image

diff --git a/api/core/v1alpha1/clustercatalog_types_test.go b/api/core/v1alpha1/clustercatalog_types_test.go
@@ -2,6 +2,7 @@ package v1alpha1
 
 import (
  "context"
+ "fmt"
  "os"
  "testing"
  "time"
@@ -23,7 +24,7 @@ func TestPollIntervalCELValidationRules(t *testing.T) {
  validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
  pth := "openAPIV3Schema.properties.spec"
  validator, found := validators["v1alpha1"][pth]
- assert.True(t, found)
+ require.True(t, found)
 
  for name, tc := range map[string]struct {
  spec ClusterCatalogSpec
@@ -68,6 +69,84 @@ func TestPollIntervalCELValidationRules(t *testing.T) {
  }
 }
 
+func TestSourceCELValidation(t *testing.T) {
+ validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
+ pth := "openAPIV3Schema.properties.spec.properties.source"
+ validator, found := validators["v1alpha1"][pth]
+ require.True(t, found)
+ for name, tc := range map[string]struct {
+ source CatalogSource
+ wantErrs []string
+ }{
+ "image source missing required image field": {
+ source: CatalogSource{
+ Type: SourceTypeImage,
+ },
+ wantErrs: []string{
+ fmt.Sprintf("%s: Invalid value: \"object\": source type '%s' requires image field", pth, SourceTypeImage),
+ },
+ },
+ "image source with required image field": {
+ source: CatalogSource{
+ Type: SourceTypeImage,
+ Image: &ImageSource{},
+ },
+ wantErrs: []string{},
+ },
+ } {
+ t.Run(name, func(t *testing.T) {
+ obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&tc.source) //nolint:gosec
+ require.NoError(t, err)
+ errs := validator(obj, nil)
+ fmt.Println(errs)
+ require.Equal(t, len(tc.wantErrs), len(errs))
+ for i := range tc.wantErrs {
+ got := errs[i].Error()
+ assert.Equal(t, tc.wantErrs[i], got)
+ }
+ })
+ }
+}
+
+func TestResolvedSourceCELValidation(t *testing.T) {
+ validators := fieldValidatorsFromFile(t, "../../../config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml")
+ pth := "openAPIV3Schema.properties.status.properties.resolvedSource"
+ validator, found := validators["v1alpha1"][pth]
+
+ require.True(t, found)
+ for name, tc := range map[string]struct {
+ source ResolvedCatalogSource
+ wantErrs []string
+ }{
+ "image source missing required image field": {
+ source: ResolvedCatalogSource{
+ Type: SourceTypeImage,
+ },
+ wantErrs: []string{
+ fmt.Sprintf("%s: Invalid value: \"object\": source type '%s' requires image field", pth, SourceTypeImage),
+ },
+ },
+ "image source with required image field": {
+ source: ResolvedCatalogSource{
+ Type: SourceTypeImage,
+ Image: &ResolvedImageSource{},
+ },
+ wantErrs: []string{},
+ },
+ } {
+ t.Run(name, func(t *testing.T) {
+ obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&tc.source) //nolint:gosec
+ require.NoError(t, err)
+ errs := validator(obj, nil)
+ require.Equal(t, len(tc.wantErrs), len(errs))
+ for i := range tc.wantErrs {
+ got := errs[i].Error()
+ assert.Equal(t, tc.wantErrs[i], got)
+ }
+ })
+ }
+}
+
 // fieldValidatorsFromFile extracts the CEL validators by version and JSONPath from a CRD file and returns
 // a validator func for testing against samples.
 func fieldValidatorsFromFile(t *testing.T, crdFilePath string) map[string]map[string]CELValidateFunc {

diff --git a/api/core/v1alpha1/zz_generated.deepcopy.go b/api/core/v1alpha1/zz_generated.deepcopy.go
diff --git a/config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml b/config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml
@@ -68,7 +68,7 @@ spec:
  Below is a minimal example of a ClusterCatalogSpec that sources a catalog from an image:
 
  source:
- type: image
+ type: Image
  image:
  ref: quay.io/operatorhubio/catalog:latest
 
@@ -77,7 +77,7 @@ spec:
  image:
  description: image is used to configure how catalog contents are
  sourced from an OCI image. This field must be set when type
- is set to "image" and must be the only field defined for this
+ is set to "Image" and must be the only field defined for this
  type.
  properties:
  pollInterval:
@@ -107,16 +107,19 @@ spec:
  description: |-
  type is a required reference to the type of source the catalog is sourced from.
 
- Allowed values are ["image"]
+ Allowed values are ["Image"]
 
- When this field is set to "image", the ClusterCatalog content will be sourced from an OCI image.
+ When this field is set to "Image", the ClusterCatalog content will be sourced from an OCI image.
  When using an image source, the image field must be set and must be the only field defined for this type.
  enum:
- - image
+ - Image
  type: string
  required:
  - type
  type: object
+ x-kubernetes-validations:
+ - message: source type 'Image' requires image field
+ rule: self.type == 'Image' && has(self.image)
  required:
  - source
  type: object
@@ -211,15 +214,9 @@ spec:
  lastUnpacked:
  description: |-
  lastUnpacked represents the time when the
- ClusterCatalog object was last unpacked.
+ ClusterCatalog object was last unpacked successfully.
  format: date-time
  type: string
- observedGeneration:
- description: |-
- observedGeneration is the most recent generation observed for this ClusterCatalog. It corresponds to the
- ClusterCatalog's generation, which is updated on mutation by the API Server.
- format: int64
- type: integer
  resolvedSource:
  description: |-
  resolvedSource contains information about the resolved source based on the source type.
@@ -232,50 +229,42 @@ spec:
  lastUnpacked: "2024-09-10T12:22:13Z"
  ref: quay.io/operatorhubio/catalog:latest
  resolvedRef: quay.io/operatorhubio/catalog@sha256:c7392b4be033da629f9d665fec30f6901de51ce3adebeff0af579f311ee5cf1b
- type: image
+ type: Image
  properties:
  image:
  description: image is a field containing resolution information
  for a catalog sourced from an image.
  properties:
- lastPollAttempt:
- description: lastPollAttempt is the time when the source image
- was last polled for new content.
- format: date-time
- type: string
- lastUnpacked:
- description: lastUnpacked is the last time when the Catalog
- contents were successfully unpacked.
+ lastSuccessfulPollAttempt:
+ description: lastSuccessfulPollAttempt is the time when the
+ resolved source was last successfully polled for new content.
  format: date-time
  type: string
  ref:
- description: ref is the reference to a container image containing
+ description: ref contains the resolved sha256 image ref containing
  Catalog contents.
  type: string
- resolvedRef:
- description: resolvedRef is the resolved sha256 image ref
- containing Catalog contents.
- type: string
  required:
- - lastPollAttempt
- - lastUnpacked
+ - lastSuccessfulPollAttempt
  - ref
- - resolvedRef
  type: object
  type:
  description: |-
  type is a reference to the type of source the catalog is sourced from.
 
- It will be set to one of the following values: ["image"].
+ It will be set to one of the following values: ["Image"].
 
- When this field is set to "image", information about the resolved image source will be set in the 'image' field.
+ When this field is set to "Image", information about the resolved image source will be set in the 'image' field.
  enum:
- - image
+ - Image
  type: string
  required:
  - image
  - type
  type: object
+ x-kubernetes-validations:
+ - message: source type 'Image' requires image field
+ rule: self.type == 'Image' && has(self.image)
  type: object
  required:
  - metadata

diff --git a/config/base/crd/kustomization.yaml b/config/base/crd/kustomization.yaml
@@ -4,11 +4,3 @@
 resources:
 - bases/olm.operatorframework.io_clustercatalogs.yaml
 #+kubebuilder:scaffold:crdkustomizeresource
-
-patches:
-- path: patches/catalog_validation.yaml
- target:
- group: apiextensions.k8s.io
- version: v1
- kind: CustomResourceDefinition
- name: catalogs.olm.operatorframework.io
diff --git a/config/base/crd/patches/catalog_validation.yaml b/config/base/crd/patches/catalog_validation.yaml