AGENT-1385: Adding missing status field for InternalReleaseImage #2611
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @andfasano! Some important instructions when contributing to openshift/api: |
|
@andfasano: This pull request references AGENT-1385 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
size/XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
machineconfiguration/v1alpha1/types_internalreleaseimage.go(2 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.deepcopy.go(1 hunks)machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(6 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
openapi/generated_openapi/zz_generated.openapi.gomachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlmachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yamlmachineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yamlmachineconfiguration/v1alpha1/zz_generated.deepcopy.gomachineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlopenapi/openapi.jsonmachineconfiguration/v1alpha1/types_internalreleaseimage.gomachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml
🔇 Additional comments (18)
machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml (2)
154-162: Verifyminimum: 1constraint forobservedGeneration.The
observedGenerationfield hasminimum: 1, but typicallyobservedGenerationstarts at 0 before the controller has processed the resource. A minimum of 0 is more conventional to allow the field to remain unset or zero-valued initially.Please verify this is intentional. If the field should allow a zero value when not yet observed:
observedGeneration: description: observedGeneration represents the most recent generation of the InternalReleaseImage object observed by the controller. format: int64 - minimum: 1 + minimum: 0 type: integer
91-94: Condition types documentation may be inconsistent.The description states "Valid types are: Success, Failure", but in
types_internalreleaseimage.go, onlyDegradedis defined as a constant forInternalReleaseImageStatusConditionType. Consider aligning the documented condition types with the actual constants defined in the Go types.machineconfiguration/v1alpha1/zz_generated.deepcopy.go (1)
137-143: LGTM!The auto-generated deepcopy logic correctly handles the new
Conditionsslice with proper nil-check and per-element deep copying, consistent with other types in the file.machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.go (1)
62-67: LGTM!The swagger documentation correctly reflects the new
conditionsandobservedGenerationfields added toInternalReleaseImageStatus.machineconfiguration/v1alpha1/types_internalreleaseimage.go (1)
77-81: ConsiderMinimum=0forObservedGeneration.The field has
+kubebuilder:validation:Minimum=1, but conventionallyobservedGenerationis 0 when the controller hasn't yet observed the resource. This could cause validation failures on initial resource creation.If the intent is to allow omitting the field until first observation:
// observedGeneration represents the most recent generation of the InternalReleaseImage object observed by the controller. // +kubebuilder:validation:XValidation:rule="self >= oldSelf", message="observedGeneration must not decrease" - // +kubebuilder:validation:Minimum=1 + // +kubebuilder:validation:Minimum=0 // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"`openapi/generated_openapi/zz_generated.openapi.go (2)
42496-42524: InternalReleaseImageStatus conditions/observedGeneration schema looks correctThe
conditionsarray andobservedGenerationint64 are modeled in line with standard Kubernetes patterns (list-map keyed bytypeand generation tracking). This should integrate cleanly with controllers and CRD validation.
42552-42552: Dependency on metav1.Condition is correctly wiredAdding
k8s.io/apimachinery/pkg/apis/meta/v1.Conditionto theDependenciesslice is consistent with the newconditionsfield and ensures the referenced schema is included.machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml (1)
91-161: Status fields schema is well-formed and follows Kubernetes conventions.The new
conditionsandobservedGenerationfields are properly defined with appropriate constraints (maxItems: 20 for conditions, minimum: 1 for observedGeneration, and the non-decreasing validation rule). The condition object structure correctly requires all standard fields and uses map-based deduplication by type.payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml (1)
91-161: Payload manifest is consistent with generated CRD.The schema matches the corresponding zz_generated variant; both conditions and observedGeneration are identically defined.
machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml (1)
91-161: Schema is consistent across feature-set variants.The conditions and observedGeneration fields are identically defined in DevPreviewNoUpgrade variant, ensuring API uniformity.
payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml (1)
91-161: Consistent with generated and other payload variants.Schema alignment verified across feature-set variants.
machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml (1)
91-161: TechPreviewNoUpgrade schema aligns with other variants.Conditions and observedGeneration are identically defined; uniform across all feature-set variants.
payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml (1)
91-161: All CRD variants are consistently updated.The payload manifest reflects the same schema as generated variants; uniform implementation across CustomNoUpgrade, DevPreviewNoUpgrade, and TechPreviewNoUpgrade feature sets.
openapi/openapi.json (5)
24535-24546: Schema correctly uses Kubernetes condition semantics for observability.The
conditionsfield properly follows Kubernetes conventions with list-map semantics (x-kubernetes-list-map-keys: ["type"],x-kubernetes-list-type: "map"). This allows the controller to report multiple non-overlapping condition statuses (Success, Failure) with conflict-free updates, as mentioned in the description.
24547-24551:observedGenerationfield correctly tracks controller reconciliation.The field is properly typed as
int64with clear semantics for tracking the most recent generation observed by the controller, enabling clients to detect controller lag and stale status.
24835-24856: NewOSImageStreamSetstructure is well-defined with proper validation constraints.The object correctly requires three fields (name, osImage, osExtensionsImage) and provides detailed descriptions enforcing:
- RFC 1123 subdomain compliance for
name- Strict image digest format constraints (SHA256, 64 hex chars, max 447 chars)
These constraints align with immutable, content-addressed image reference requirements.
24869-24880:availableStreamslist correctly references newOSImageStreamSettype with map semantics.The field uses proper list-map semantics (
x-kubernetes-list-map-keys: ["name"]) to enforce unique stream entries keyed by thenamefield, preventing duplicates and enabling efficient lookups.
24864-24867: Verify thatOSImageStreamStatusrequired fields match implementation.The required fields array (
availableStreams,defaultStream) should be validated against the corresponding Go type definition and any CRD manifests to ensure consistency across the generated OpenAPI, swagger docs, and validation schemas.
andfasano
force-pushed
the
iri-missing-status-fields
branch
2 times, most recently
from
3481466 to
deb59c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
machineconfiguration/v1alpha1/types_internalreleaseimage.go(2 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.deepcopy.go(1 hunks)machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(6 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml
- machineconfiguration/v1alpha1/zz_generated.deepcopy.go
- machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machineconfiguration/v1alpha1/types_internalreleaseimage.goopenapi/generated_openapi/zz_generated.openapi.gomachineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gomachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlmachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yamlopenapi/openapi.jsonpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml
🔇 Additional comments (11)
machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml (1)
91-163: LGTM! Status fields correctly added.The new
conditionsandobservedGenerationfields follow Kubernetes API conventions:
- Standard
metav1.Conditionschema with proper list-map semantics- ObservedGeneration tracking with non-decreasing validation
- Appropriate constraints (maxItems: 20, minimum: 1)
machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml (1)
90-162: LGTM! Consistent with other CRD variants.The status fields match the NoRegistryClusterInstall variant, ensuring schema consistency across feature sets.
machineconfiguration/v1alpha1/types_internalreleaseimage.go (2)
70-82: LGTM! Status fields correctly defined.The previous documentation issues have been resolved:
- The comment now correctly states "Valid types are: Degraded"
- The new
ConditionsandObservedGenerationfields follow Kubernetes conventions with appropriate validation markers
97-104: LGTM! Type and constant correctly defined.The previous typo ("Statuc" → "Status") has been fixed. The new type and constant are properly documented and follow Go naming conventions.
openapi/generated_openapi/zz_generated.openapi.go (2)
42496-42524: LGTM! Standard Kubernetes status fields correctly generated.The
conditionsandobservedGenerationfields follow standard Kubernetes API conventions:
- conditions array with proper list-map-keys for efficient reconciliation
- observedGeneration for tracking controller observations
42551-42552: LGTM! Dependency correctly added.The
metav1.Conditiondependency is properly included to reflect the schema reference in the conditions field.machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml (1)
91-163: Status.conditions and observedGeneration schema look correct and consistentThe added
status.conditionsblock follows the standardmetav1.Conditionpattern (required fields, list-map keyed bytype, sensible item caps), andstatus.observedGenerationuses the usual non-decreasingself >= oldSelfrule with an appropriate int64/minimum: 1definition. No structural or validation issues stand out here.machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.go (1)
63-66: Swagger docs correctly updated for new status fieldsThe
map_InternalReleaseImageStatusentries forconditionsandobservedGenerationaccurately mirror the CRD semantics and integrate cleanly with the existingreleasesdescription. Nothing concerning here.payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml (1)
90-162: DevPreviewNoUpgrade status schema stays in sync with the generated typeThe added
conditionsandobservedGenerationunderstatusmatch the CustomNoUpgrade CRD and the Go types: correct list-map metadata, condition fields, and non-decreasing observedGeneration rule. This keeps the DevPreview variant consistent with the API surface.payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml (1)
90-162: TechPreviewNoUpgrade CRD status fields align with other variantsThe TechPreview CRD’s new
status.conditionsandstatus.observedGenerationdefinitions are consistent with the other InternalReleaseImage CRDs and the backing Go types, including list-map behavior and the non-decreasing generation constraint. Looks good.openapi/openapi.json (1)
24535-24551: Verify schema alignment with Go types for new status fields.The new
conditionsandobservedGenerationfields follow Kubernetes conventions. Theconditionsfield is correctly defined as an array ofmetav1.Conditionwithx-kubernetes-list-type: mapandx-kubernetes-list-map-keys: ["type"], andobservedGenerationis properly typed asint64. Since this is an auto-generated schema, ensure the corresponding Go type definitions use[]metav1.Conditionandint64for these fields to maintain consistency between the source types and the generated OpenAPI specification.
| }, | ||
| "profileCustomizations": { | ||
| "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles.", | ||
| "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles. Deprecated: no longer needed, since DRA is GA starting with 4.21, and is enabled by' default in the cluster, this field will be removed in 4.24.", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenShift 4.21 DRA Dynamic Resource Allocation GA status release notes
💡 Result:
Short answer: Dynamic Resource Allocation (DRA) is not listed as GA in OpenShift 4.21 — it remains a technology-preview/feature-gated capability (the docs describe a Dynamic Resource Allocation API / DRA prerequisites and FeatureGate). [1][2]
Sources:
- OpenShift 4.20 nodes docs (DRA prerequisites / resource-claims examples). [1]
- OpenShift postinstallation/FeatureGate docs (Dynamic Resource Allocation API listed as a feature gate / not GA). [2]
🌐 Web query:
OpenShift 4.24 release schedule profileCustomizations removal
💡 Result:
Do you mean the KubeDescheduler CRD's spec.profileCustomizations being removed in OpenShift 4.24? If so, I can search OpenShift 4.24 release notes and operator change logs and report exact dates and links. Which scope do you want me to check (OpenShift release notes, Kube Descheduler Operator changelog, or Red Hat announcements)?
🌐 Web query:
OpenShift DRA deprecation timeline 4.24 removal
💡 Result:
I couldn't find any Red Hat / OpenShift documentation about a feature called "DRA" being deprecated or removed in 4.24. Do you mean one of these instead? — pick one or confirm the exact term:
- "Data Resiliency" / OpenShift Data Resiliency features (OpenShift Container Storage)
- "DRA" as Kubernetes Device Resource Assignment (DRA) / GPU DRA
- an OpenShift Operator or add-on named DRA (give the operator name)
- something else (please paste a link or error/message you saw)
If you confirm which "DRA" you mean I’ll search official Red Hat docs and release notes and return the exact deprecation/removal timeline with citations.
🏁 Script executed:
# Find context around line 10586 in openapi.json
cd openapi && head -n 10590 openapi.json | tail -n 20 && echo "---" && head -n 10600 openapi.json | tail -n 30Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Search for profileCustomizations in the codebase
rg "profileCustomizations" -B 3 -A 3Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Search for DRA references in openapi.json
rg "DRA|Dynamic Resource Allocation" openapi/ -i -B 2 -A 2Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Check file metadata and version info
head -n 50 openapi/openapi.json | grep -i "version\|title\|description" | head -20Repository: openshift/api
Length of output: 170
Deprecation message contains inaccurate DRA status claims.
The description states "DRA is GA starting with 4.21," but according to official OpenShift documentation, Dynamic Resource Allocation (DRA) remains in technology-preview/feature-gated status in OpenShift 4.21 and is not GA. The removal timeline for version 4.24 cannot be verified and appears to be based on this incorrect premise. The deprecation message must be corrected to accurately reflect DRA's actual status and supported timeline.
🤖 Prompt for AI Agents
In openapi/openapi.json around line 10586, the deprecation description
incorrectly claims "DRA is GA starting with 4.21" and asserts removal in 4.24;
update the text to remove the GA claim and any unverified removal timeline,
instead stating that Dynamic Resource Allocation (DRA) remains
technology-preview/feature-gated in 4.21 per OpenShift documentation and that
the field is deprecated pending upstream feature stabilization, or point readers
to the official OpenShift docs for current status and timelines.
| "com.github.openshift.api.machineconfiguration.v1alpha1.OSImageStreamSet": { | ||
| "type": "object", | ||
| "required": [ | ||
| "name", | ||
| "osImage", | ||
| "osExtensionsImage" | ||
| ], | ||
| "properties": { | ||
| "name": { | ||
| "description": "name is the required identifier of the stream.\n\nname is determined by the operator based on the OCI label of the discovered OS or Extension Image.\n\nMust be a valid RFC 1123 subdomain between 1 and 253 characters in length, consisting of lowercase alphanumeric characters, hyphens ('-'), and periods ('.').", | ||
| "type": "string" | ||
| }, | ||
| "osExtensionsImage": { | ||
| "description": "osExtensionsImage is a required OS Extensions Image referenced by digest.\n\nosExtensionsImage bundles the extra repositories used to enable extensions, augmenting the base operating system without modifying the underlying immutable osImage.\n\nThe format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", | ||
| "type": "string" | ||
| }, | ||
| "osImage": { | ||
| "description": "osImage is a required OS Image referenced by digest.\n\nosImage contains the immutable, fundamental operating system components, including the kernel and base utilities, that define the core environment for the node's host operating system.\n\nThe format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", | ||
| "type": "string" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for validation rules on osImage and osExtensionsImage fields
fd -e yaml -e json | head -50 | xargs rg -A 10 'osImage.*:' | head -100Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Find CRD definition files for machine configuration
fd -type f -name "*.yaml" -o -name "*.yml" -o -name "*.json" | xargs rg -l "OSImageStreamSet" | head -20Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Search for pattern validators and validation keywords in the openapi.json file around OSImageStreamSet
rg -B 5 -A 30 'OSImageStreamSet' openapi/openapi.json | head -150Repository: openshift/api
Length of output: 170
🌐 Web query:
OpenShift API OSImageStreamSet CRD validation rules pattern constraints
💡 Result:
I can help — can you share where you saw OSImageStreamSet (repo/cluster version or a CRD YAML)? I couldn't find a public CRD named "OSImageStreamSet". If you paste the CRD or a link, I'll extract the validation rules and pattern constraints.
🌐 Web query:
OpenAPI schema validation rules osImage digest format constraints OpenShift
💡 Result:
Summary of OpenShift OpenAPI validation rules for osImage / image digest formats
- Allowed format: host[:port][/namespace]/name@sha256:. The digest MUST begin with "sha256:" followed by exactly 64 lowercase hex characters (a–f, 0–9). [1]
- Overall length constraints: field length between 1 and 447 characters. [1]
- Validation rules (as implemented in the OpenShift API types):
- Suffix rule: the part after '@' must match ^sha256:[a-f0-9]{64}$. [1]
- Name rule: the part before '@' must match a host[:port][/namespace]/name‑like regex (resembles a hostname with optional port and optional single namespace path). [1]
- Practical examples: digested pull specs used in machine-config and other OpenShift components follow the form ...@sha256:<64-hex>; e.g. ostree-unverified-registry:...@sha256:3c8fc6... . [2][3]
Sources:
- OpenShift API Go type documentation (ImageDigestFormat with kubebuilder/XValidation rules). [1]
- OpenShift/OKD docs showing digested image pull specs in machine configuration examples and image APIs. [2][3]
Add explicit OpenAPI schema validation keywords for OSImageStreamSet field constraints.
The name, osImage, and osExtensionsImage fields have detailed constraints documented in descriptions (RFC 1123 format, digest pattern sha256:[a-f0-9]{64}, length 1-447 characters). These constraints are enforced via CRD validation rules in the implementation, but the OpenAPI schema should explicitly declare them using pattern, minLength, and maxLength keywords rather than relying on descriptive text alone. Add:
"pattern"for name (RFC 1123 compliance) and image digest format (^sha256:[a-f0-9]{64}$)"minLength": 1, "maxLength": 447for image pull specs
🤖 Prompt for AI Agents
In openapi/openapi.json around lines 24835-24856, update the OSImageStreamSet
properties to include explicit OpenAPI validation: add a RFC1123 subdomain regex
pattern for "name" (e.g. a pattern enforcing lowercase alphanumerics, hyphens
and dots with proper start/end chars and 1-253 length), and for "osImage" and
"osExtensionsImage" add "minLength": 1, "maxLength": 447 and a pattern that
enforces the digest format (ensure the pull spec ends with "@sha256:<digest>"
where <digest> matches ^sha256:[a-f0-9]{64}$ or equivalently ensure a trailing
@sha256:[a-f0-9]{64}$); update the schema objects to include these "pattern",
"minLength", and "maxLength" keywords accordingly.
| // observedGeneration represents the most recent generation of the InternalReleaseImage object observed by the controller. | ||
| // +kubebuilder:validation:XValidation:rule="self >= oldSelf", message="observedGeneration must not decrease" | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +optional | ||
| ObservedGeneration int64 `json:"observedGeneration,omitempty"` |
There was a problem hiding this comment.
Do you need a top-level observed generation?
Conditions can have an observed generation: https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition which is generally preferred over a top-level one
There was a problem hiding this comment.
Thanks @everettraven ! For my new MCO controller I was following the example of two already existing similar controllers, and following up your comment just noticed that both ContainerRuntimeConfigStatus and KubeletConfigStatus do not use the metav1.Condition but redefine a custom one without the ObservedGeneration field (present instead in the upper level)! It will definitely work, will update it
andfasano
force-pushed
the
iri-missing-status-fields
branch
from
deb59c4 to
14f9fa5
Compare
andfasano
force-pushed
the
iri-missing-status-fields
branch
from
14f9fa5 to
8521650
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openapi/openapi.json (2)
10583-10589: The deprecation message contains an inaccurate DRA status claim that contradicts OpenShift documentation.The description states "DRA is GA starting with 4.21," but according to official OpenShift documentation, Dynamic Resource Allocation remains in technology-preview/feature-gated status in 4.21 and is not GA. The removal timeline for version 4.24 cannot be verified against this incorrect premise. Correct the message to accurately reflect DRA's actual status and remove unverified removal timelines.
24830-24880: Add explicit OpenAPI validation keywords for OSImageStreamSet field constraints.The
name,osImage, andosExtensionsImagefields document detailed constraints in descriptions (RFC 1123 format, digest pattern, length 1–447 characters) but lack explicit OpenAPI schema validation keywords. Addpatternfor the RFC 1123 subdomain and digest format (^sha256:[a-f0-9]{64}$), andminLength/maxLengthfor image pull spec length constraints. This enables client-side and tooling validation without relying on description text.Apply this diff to add explicit validation keywords:
"name": { "description": "name is the required identifier of the stream.\n\nname is determined by the operator based on the OCI label of the discovered OS or Extension Image.\n\nMust be a valid RFC 1123 subdomain between 1 and 253 characters in length, consisting of lowercase alphanumeric characters, hyphens ('-'), and periods ('.').", - "type": "string" + "type": "string", + "pattern": "^[a-z0-9]([a-z0-9-]{0,251}[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]{0,251}[a-z0-9])?)*$", + "minLength": 1, + "maxLength": 253 }, "osExtensionsImage": { "description": "osExtensionsImage is a required OS Extensions Image referenced by digest.\n\nosExtensionsImage bundles the extra repositories used to enable extensions, augmenting the base operating system without modifying the underlying immutable osImage.\n\nThe format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", - "type": "string" + "type": "string", + "minLength": 1, + "maxLength": 447, + "pattern": ".*@sha256:[a-f0-9]{64}$" }, "osImage": { "description": "osImage is a required OS Image referenced by digest.\n\nosImage contains the immutable, fundamental operating system components, including the kernel and base utilities, that define the core environment for the node's host operating system.\n\nThe format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", - "type": "string" + "type": "string", + "minLength": 1, + "maxLength": 447, + "pattern": ".*@sha256:[a-f0-9]{64}$" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
machineconfiguration/v1alpha1/types_internalreleaseimage.go(2 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.deepcopy.go(1 hunks)machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml(1 hunks)machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(6 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- openapi/generated_openapi/zz_generated.openapi.go
- machineconfiguration/v1alpha1/types_internalreleaseimage.go
- machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machineconfiguration/v1alpha1/zz_generated.deepcopy.gomachineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gomachineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yamlmachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlopenapi/openapi.jsonmachineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml
🔇 Additional comments (10)
machineconfiguration/v1alpha1/zz_generated.deepcopy.go (1)
135-143: DeepCopy forInternalReleaseImageStatus.Conditionsis correctly implementedNil-guard, slice reallocation, and per-element
DeepCopyIntomatch existing patterns in this file and ensure no aliasing of the conditions slice.machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml (1)
91-155: Top-levelstatus.conditionsschema looks consistent with existing condition usageThe new
status.conditionsblock follows the same metav1.Condition shape and list semantics already used for per-release conditions (map keyed bytype, minItems/maxItems bounds) and keepsstatusitself optional, so it should integrate cleanly with existing controllers.machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.go (1)
62-66: Swagger docs forInternalReleaseImageStatusare now aligned with the newconditionsfieldThe map entry for
conditionsmatches the CRD description and keeps the status documentation coherent with the Go types and OpenAPI.machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests/internalreleaseimages.machineconfiguration.openshift.io/NoRegistryClusterInstall.yaml (1)
91-155:status.conditionsschema matches the non-feature-gated CRD variantThis feature-gated CRD uses the same Condition shape, validation, and list semantics as the main InternalReleaseImage CRD, which is important to avoid behavior differences across feature sets.
payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-DevPreviewNoUpgrade.crd.yaml (1)
90-154: DevPreviewNoUpgrade CRD’sstatus.conditionsis consistent with other variantsThe added
status.conditionsblock mirrors the schema and semantics used in the other InternalReleaseImage CRDs, so controllers can rely on a uniform status surface across feature sets.openapi/openapi.json (1)
24535-24546: Conditions field implementation is correct.The new
conditionsarray properly references the Kubernetesmetav1.Conditiontype, includes appropriate Kubernetes-specific metadata (x-kubernetes-list-type: map,x-kubernetes-list-map-keys), and documents valid condition types in the description. The structure aligns with standard Kubernetes condition patterns.machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yaml (3)
90-154: Status conditions schema is well‑structured and properly validated.The top-level
conditionsfield correctly captures controller state with:
- Proper
x-kubernetes-list-type: mapsemantics usingtypeas the unique key- All condition fields appropriately constrained (format specs, length bounds, required fields)
- Descriptions aligned with Kubernetes conventions
minItems: 1design choice appropriately gates empty condition arrays (either omit field or provide meaningful conditions)
165-239: Per‑release conditions schema mirrors top‑level structure with appropriate constraints.The nested conditions array in release items follows the same pattern as top‑level conditions with:
- Identical field validation (lastTransitionTime, message, observedGeneration, reason, status, type all required)
- Tighter bound (
maxItems: 5vs.20at top-level) reflecting finer granularity- Clear description of valid condition types (Mounted, Installing, Available, Removing, Degraded)
- Proper list-type map semantics
239-256: Image field validation rules are syntactically sound and enforce OCI digest format.The CEL rules properly validate:
- Digest format: requires
@sha256:prefix followed by exactly 64 lowercase hex characters- Reference format: enforces valid registry/namespace/name structure with optional port
payload-manifests/crds/0000_80_machine-config_01_internalreleaseimages-CustomNoUpgrade.crd.yaml (1)
91-155: File 2 is consistent with File 1 across all conditions and release field definitions.The CustomNoUpgrade variant properly mirrors the TechPreviewNoUpgrade variant with identical:
- Top-level conditions schema and constraints (lines 91-155)
- Per-release conditions, image, and name fields (lines 166-267)
- CEL validation rules and field descriptions
Intentional metadata differences (feature-set annotations) are appropriate.
Also applies to: 166-267
| ], | ||
| "properties": { | ||
| "name": { | ||
| "description": "name is an OCI Image referenced by digest.\n\nThe format of the image ref is: host[:port][/namespace]/name@sha256:<digest>", | ||
| "type": "string", | ||
| "default": "" | ||
| "description": "name is an OCI Image referenced by digest. The format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", | ||
| "type": "string" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add explicit OpenAPI validation for the InternalReleaseImage name field.
The name field describes format constraints in its description (OCI Image referenced by digest, format host[:port][/namespace]/name@sha256:<digest>, length 1–447 characters) but lacks explicit OpenAPI validation keywords (pattern, minLength, maxLength). Add these keywords to enforce validation at the schema level.
Apply this diff to add explicit validation:
"name": {
"description": "name is an OCI Image referenced by digest. The format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.",
- "type": "string"
+ "type": "string",
+ "minLength": 1,
+ "maxLength": 447,
+ "pattern": ".*@sha256:[a-f0-9]{64}$"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ], | |
| "properties": { | |
| "name": { | |
| "description": "name is an OCI Image referenced by digest.\n\nThe format of the image ref is: host[:port][/namespace]/name@sha256:<digest>", | |
| "type": "string", | |
| "default": "" | |
| "description": "name is an OCI Image referenced by digest. The format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", | |
| "type": "string" | |
| } | |
| } | |
| }, | |
| ], | |
| "properties": { | |
| "name": { | |
| "description": "name is an OCI Image referenced by digest. The format of the image pull spec is: host[:port][/namespace]/name@sha256:<digest>, where the digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the whole spec must be between 1 to 447 characters.", | |
| "type": "string", | |
| "minLength": 1, | |
| "maxLength": 447, | |
| "pattern": ".*@sha256:[a-f0-9]{64}$" | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In openapi/openapi.json around lines 24886 to 24893, the
InternalReleaseImage.name schema only has a descriptive string but no
enforcement; add explicit OpenAPI validation by keeping "type": "string" and
adding "minLength": 1, "maxLength": 447 and a "pattern" that enforces the OCI
pull spec with sha256 digest (host optionally with :port, optional namespaces,
name, then "@sha256:" followed by exactly 64 lowercase hex characters, e.g.
pattern roughly matching host(:port)?(/segment)*?/name@sha256:[a-f0-9]{64});
ensure the pattern uses a JSON-compatible regex and integrate these three
keywords into the same properties object.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest-required |
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
openshift-ci-robot
added
verified-later
verified
|
@andfasano: This PR has been marked to be verified later by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@andfasano: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Followup of #2510
Adding missing status fields on InternalReleaseImage resource to report the controller status.
Related to: openshift/machine-config-operator#5452