WIP: ⚠️ updates from api audit #1404
+101
−62
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,8 +25,8 @@ import ( | |
| var ClusterExtensionKind = "ClusterExtension" | ||
|
|
||
| type ( | ||
| UpgradeConstraintPolicy string | ||
| CRDUpgradeSafetyEnforcement string | ||
| ) | ||
|
|
||
| const ( | ||
|
|
@@ -58,6 +58,7 @@ type ClusterExtensionSpec struct { | |
| // catalog: | ||
| // packageName: example-package | ||
| // | ||
| // +kubebuilder:validation:Required | ||
| Source SourceConfig `json:"source"` | ||
|
|
||
| // install is a required field used to configure the installation options | ||
|
|
@@ -69,6 +70,7 @@ type ClusterExtensionSpec struct { | |
| // namespace: example-namespace | ||
| // serviceAccount: | ||
| // name: example-sa | ||
| // +kubebuilder:validation:Required | ||
| Install ClusterExtensionInstallConfig `json:"install"` | ||
| } | ||
|
|
||
|
|
@@ -80,14 +82,16 @@ const SourceTypeCatalog = "Catalog" | |
| type SourceConfig struct { | ||
|
There was a problem hiding this comment. I believe we need to update the CEL validation here to match the changes in https://github.com/operator-framework/catalogd/pull/443/files#diff-d446855f2187e6d61c4719c3202fcb21e4aea9b09e09f9e1d01419f341d8fc9fR233 It looks like Joel had requested this as well: https://github.com/openshift/api/pull/2067/files#r1812412946 |
||
| // sourceType is a required reference to the type of install source. | ||
| // | ||
| // Allowed values are "Catalog" | ||
| // | ||
| // When this field is set to "Catalog", information for determining the | ||
| // appropriate bundle of content to install will be fetched from | ||
| // ClusterCatalog resources existing on the cluster. | ||
| // When using the Catalog sourceType, the catalog field must also be set. | ||
| // | ||
| // +unionDiscriminator | ||
| // +kubebuilder:validation:Enum:="Catalog" | ||
| // +kubebuilder:validation:Required | ||
| SourceType string `json:"sourceType"` | ||
|
|
||
| // catalog is used to configure how information is sourced from a catalog. This field must be defined when sourceType is set to "Catalog", | ||
|
There was a problem hiding this comment. Should probably update this to reflect any updates we make to the CEL validation following my comment on this structs validations |
||
|
|
@@ -130,6 +134,7 @@ type ClusterExtensionInstallConfig struct { | |
| //+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ | ||
|
There was a problem hiding this comment. Should we use CEL here instead of pattern validation? |
||
| //+kubebuilder:validation:MaxLength:=63 | ||
| //+kubebuilder:validation:XValidation:rule="self == oldSelf",message="namespace is immutable" | ||
| //+kubebuilder:validation:Required | ||
| Namespace string `json:"namespace"` | ||
|
There was a problem hiding this comment. There were a few comments from Joel related to the GoDoc for this field that I don't see reflected here. Did we decide against accepting those suggestions or making adjustments to clarify some of the questions asked? Ref: |
||
|
|
||
| // serviceAccount is a required reference to a ServiceAccount that exists | ||
|
|
@@ -140,6 +145,7 @@ type ClusterExtensionInstallConfig struct { | |
| // the ServiceAccount provided via this field should be configured with the | ||
| // appropriate permissions to perform the necessary operations on all the | ||
| // resources that are included in the bundle of content being applied. | ||
| //+kubebuilder:validation:Required | ||
| ServiceAccount ServiceAccountReference `json:"serviceAccount"` | ||
|
|
||
| // preflight is an optional field that can be used to configure the preflight checks run before installation or upgrade of the content for the package specified in the packageName field. | ||
|
There was a problem hiding this comment. Do we need to be more descriptive about what the default preflight checks are? Context: https://github.com/openshift/api/pull/2067/files#r1812435988 |
||
|
|
@@ -181,6 +187,7 @@ type CatalogSource struct { | |
| //+kubebuilder:validation:MaxLength:=253 | ||
| //+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ | ||
|
There was a problem hiding this comment. Should this also be CEL validation? |
||
| //+kubebuilder:validation:XValidation:rule="self == oldSelf",message="packageName is immutable" | ||
| //+kubebuilder:validation:Required | ||
| PackageName string `json:"packageName"` | ||
|
There was a problem hiding this comment. Similar question regarding Joel having comments on the GoDoc for this field - are we intentionally not changing the GoDoc? There was a problem hiding this comment. Is has one, and I thought that the crux of confusion lay in the pattern getting barfed back to users on errors. |
||
|
|
||
| // version is an optional semver constraint (a specific version or range of versions). When unspecified, the latest version available will be installed. | ||
|
|
@@ -258,14 +265,14 @@ type CatalogSource struct { | |
| // For more information on semver, please see https://semver.org/ | ||
| // | ||
| //+kubebuilder:validation:MaxLength:=64 | ||
| //+kubebuilder:validation:XValidation:rule="self.matches(\"^(\\\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\\\^)\\\\s*(v?(0|[1-9]\\\\d*|[x|X|\\\\*])(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*]))?(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*))?(-([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?(\\\\+([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?)\\\\s*)((?:\\\\s+|,\\\\s*|\\\\s*\\\\|\\\\|\\\\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\\\\^)\\\\s*(v?(0|[1-9]\\\\d*|x|X|\\\\*])(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*))?(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*]))?(-([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?(\\\\+([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?)\\\\s*)*$\")",message="invalid version expression in the catalog source" | ||
| //+optional | ||
| Version string `json:"version,omitempty"` | ||
|
|
||
| // channels is an optional reference to a set of channels belonging to | ||
|
There was a problem hiding this comment. There is an open-question on if we should set a max number of items in the list of channels: https://github.com/openshift/api/pull/2067/files#r1812455558 I think it is reasonable to have a max number of entries in the slice and Joe's suggested 256 seems fine to me as well. |
||
| // the package specified in the packageName field. | ||
| // | ||
| // A "channel" is a package-author-defined stream of updates for an extension. | ||
| // | ||
| // When specified, it is used to constrain the set of installable bundles and | ||
| // the automated upgrade path. This constraint is an AND operation with the | ||
|
|
@@ -322,7 +329,7 @@ type CatalogSource struct { | |
| // the upgrade path(s) defined in the catalog are enforced for the package | ||
| // referenced in the packageName field. | ||
| // | ||
| // Allowed values are: "CatalogProvided" or "SelfCertified". | ||
| // | ||
| // When this field is set to "CatalogProvided", automatic upgrades will only occur | ||
| // when upgrade constraints specified by the package author are met. | ||
|
|
@@ -373,6 +380,7 @@ type ServiceAccountReference struct { | |
| //+kubebuilder:validation:MaxLength:=253 | ||
| //+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ | ||
| //+kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| //+kubebuilder:validation:Required | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
|
|
@@ -386,28 +394,30 @@ type PreflightConfig struct { | |
| // consequences of upgrading a CRD, such as data loss. | ||
| // | ||
| // This field is required if the spec.install.preflight field is specified. | ||
| //+kubebuilder:validation:Required | ||
| CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"` | ||
| } | ||
|
|
||
| // CRDUpgradeSafetyPreflightConfig is the configuration for CRD upgrade safety preflight check. | ||
| type CRDUpgradeSafetyPreflightConfig struct { | ||
| // enforcement is used to configure the state of the CRD Upgrade Safety pre-flight check. | ||
| // | ||
| // This field is required when the spec.install.preflight.crdUpgradeSafety field is | ||
| // specified. | ||
|
Comment on lines
405
to
406
There was a problem hiding this comment. I think I saw a comment from Joel about not needing the conditional statement here. We can just say "enforcement is required" unconditionally (it is implicit that it being required is conditional on the parent struct being specified). |
||
| // | ||
| // Allowed values are "None" or "Strict". The default value is "Strict". | ||
| // | ||
| // When set to "None", the CRD Upgrade Safety pre-flight check will be skipped | ||
| // when performing an upgrade operation. This should be used with caution as | ||
| // unintended consequences such as data loss can occur. | ||
| // | ||
| // When set to "Strict", the CRD Upgrade Safety pre-flight check will be run when | ||
| // performing an upgrade operation. | ||
| // | ||
| //+kubebuilder:validation:Enum:="None";"Strict" | ||
| //+kubebuilder:default:=Strict | ||
| //+kubebuilder:validation:Required | ||
| Enforcement CRDUpgradeSafetyEnforcement `json:"enforcement"` | ||
| } | ||
|
|
||
| const ( | ||
|
|
@@ -428,8 +438,10 @@ const ( | |
| ReasonBlocked = "Blocked" | ||
| ReasonRetrying = "Retrying" | ||
|
|
||
| // None will not perform CRD upgrade safety checks. | ||
| CRDUpgradeSafetyEnforcementNone CRDUpgradeSafetyEnforcement = "None" | ||
| // Strict will enforce the CRD upgrade safety check and block the upgrade if the CRD would not pass the check. | ||
| CRDUpgradeSafetyEnforcementStrict CRDUpgradeSafetyEnforcement = "Strict" | ||
| ) | ||
|
|
||
| func init() { | ||
|
|
@@ -455,9 +467,11 @@ func init() { | |
| type BundleMetadata struct { | ||
| // name is a required field and is a reference | ||
| // to the name of a bundle | ||
| //+kubebuilder:validation:Required | ||
| Name string `json:"name"` | ||
| // version is a required field and is a reference | ||
| // to the version that this bundle represents | ||
| //+kubebuilder:validation:Required | ||
| Version string `json:"version"` | ||
| } | ||
|
|
||
|
|
@@ -496,6 +510,7 @@ type ClusterExtensionStatus struct { | |
| // +patchStrategy=merge | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +optional | ||
| Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` | ||
| } | ||
|
|
||
|
|
@@ -504,6 +519,7 @@ type ClusterExtensionInstallStatus struct { | |
| // | ||
| // A "bundle" is a versioned set of content that represents the resources that | ||
| // need to be applied to a cluster to install a package. | ||
| //+kubebuilder:validation:Required | ||
| Bundle BundleMetadata `json:"bundle"` | ||
| } | ||
|
|
||
|
|
@@ -516,7 +532,9 @@ type ClusterExtension struct { | |
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
|
||
| //+optional | ||
| Spec ClusterExtensionSpec `json:"spec,omitempty"` | ||
| //+optional | ||
| Status ClusterExtensionStatus `json:"status,omitempty"` | ||
| } | ||
|
|
||
|
|
@@ -525,8 +543,10 @@ type ClusterExtension struct { | |
| // ClusterExtensionList contains a list of ClusterExtension | ||
| type ClusterExtensionList struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| //+optional | ||
| metav1.ListMeta `json:"metadata,omitempty"` | ||
| //+kubebuilder:validation:Required | ||
| Items []ClusterExtension `json:"items"` | ||
| } | ||
|
|
||
| func init() { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an extra new line above the Required comment marker?