OKD-259: Support an "OKD" featureset to be enabled by default on OKD clusters #2451
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jatinsu: This pull request references OKD-259 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 epic to target the "4.20.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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @jatinsu! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
@jatinsu: This pull request references OKD-259 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 epic to target the "4.20.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. |
openshift-merge-robot
added
the
needs-rebase
jatinsu
force-pushed
the
OKD-featuregates
branch
from
c35740c to
df29920
Compare
openshift-merge-robot
removed
the
needs-rebase
jatinsu
force-pushed
the
OKD-featuregates
branch
2 times, most recently
from
d58e1ad to
fb290e4
Compare
openshift-merge-robot
added
the
needs-rebase
jatinsu
force-pushed
the
OKD-featuregates
branch
from
7938b54 to
01da285
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/test e2e-aws-ovn-techpreview |
|
/test okd-scos-e2e-aws-ovn |
|
/test e2e-aws-ovn-techpreview |
openshift-merge-robot
added
the
needs-rebase
jatinsu
force-pushed
the
OKD-featuregates
branch
from
abf6422 to
7426300
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/test ci/prow/images ci/prow/minor-e2e-upgrade-minor |
|
@jatinsu: The specified target(s) for The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
/test images minor-e2e-upgrade-minor |
jatinsu
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
jatinsu
force-pushed
the
OKD-featuregates
branch
from
7426300 to
ac89d9f
Compare
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Please add a test somewhere that ensures that a feature is not enabled in Default without being enabled in OKD as well
Please also link the work that will prevent the OKD featureset being enabled in OCP clusters
config/v1/types_feature.go
Outdated
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'CustomNoUpgrade' ? self == 'CustomNoUpgrade' : true",message="CustomNoUpgrade may not be changed" | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'TechPreviewNoUpgrade' ? self == 'TechPreviewNoUpgrade' : true",message="TechPreviewNoUpgrade may not be changed" | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'DevPreviewNoUpgrade' ? self == 'DevPreviewNoUpgrade' : true",message="DevPreviewNoUpgrade may not be changed" | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'OKD' ? self == 'OKD' : true",message="OKD may not be changed" |
There was a problem hiding this comment.
I'm expecting the the OKD featureset is a subset of TechPreviewNoUpgrade which is also a subset of DevPreviewNoUpgrade. Have you considered allowing a transition from OKD to either TP or DP?
There was a problem hiding this comment.
Please add a test somewhere that ensures that a feature is not enabled in Default without being enabled in OKD as well
Sure thing! I'll get on that
Please also link the work that will prevent the OKD featureset being enabled in OCP clusters
Here's what we added for the validation: openshift/kubernetes@87b2e70
Do you want this featureset to be opt-in? Or on by default in all OKD clusters?
It won't be enabled by default, the feature will be opt in through the installer
I'm expecting the the OKD featureset is a subset of TechPreviewNoUpgrade which is also a subset of DevPreviewNoUpgrade. Have you considered allowing a transition from OKD to either TP or DP?
I haven't, but @Prashanth684 would this be something that we would be interested in?
There was a problem hiding this comment.
It won't be enabled by default, the feature will be opt in through the installer
This contradicts something @Prashanth684 mentioned on another thread https://github.com/openshift/api/pull/2451/files#r2461166939
I haven't, but @Prashanth684 would this be something that we would be interested in?
Prasanth also suggested the same, so lets make this change
There was a problem hiding this comment.
This contradicts something @Prashanth684 mentioned on another thread https://github.com/openshift/api/pull/2451/files#r2461166939
The more i think about this - i feel CVO should be the one dictating what featureset gets installed for OKD, not the installer. CVO should also migrate OKD clusters with default featureset to OKD featureset. I tried an implementation with some CVO changes on installs and upgrades and it worked fine. https://gist.github.com/Prashanth684/c650c14b8c2b02d820e67fe75153c669 captures the major changes needed to have OKD featureset by default on install and upgrades.
There was a problem hiding this comment.
Do folks need a way to opt out here? Are we certain forcing every OKD cluster onto this OKD featureset is the right thing to do?
jatinsu
force-pushed
the
OKD-featuregates
branch
from
282ef0e to
926ca0a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (3)
33-34: Fix grammatical error in printer column description.The description contains a double negative: "When at least one of machine is not either not updated". This should be simplified.
- - description: When at least one of machine is not either not updated or is in - the process of updating to the desired machine config. + - description: When at least one machine is not updated or is in the process of + updating to the desired machine config.
350-351: Fix typo in status schema description.Line 351 contains a typo: "ceryExpiry" should be "certExpiry".
- description: ceryExpiry contains the bundle name and the expiry + description: certExpiry contains the bundle name and the expiry
502-504: Remove double period in description.Line 504 ends with a double period ("failed.."), which should be a single period.
- degradedMachineCount represents the total number of machines marked degraded (or unreconcilable). - A node is marked degraded if applying a configuration failed.. + degradedMachineCount represents the total number of machines marked degraded (or unreconcilable). + A node is marked degraded if applying a configuration failed.
🧹 Nitpick comments (3)
payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (2)
299-306: Asymmetric validation: IBMCloud enforced, other driver types permissive.The CRD requires
driverType(line 300) and includes a CEL rule (lines 302–306) that strictly enforces the presence/absence of theibmcloudconfig block. However, other platform-specific configs (aws, azure, gcp, vSphere) have no corresponding validation—a user can setdriverType: AWSwithout providing anawsconfig block, creating an inconsistent validation model.If all driver types should require their respective config blocks, extend the CEL rule to validate all driver types. If config blocks are truly optional for non-IBMCloud platforms, document why IBMCloud alone receives strict enforcement.
210-215: GCP location pattern overly permissive.Line 214 uses pattern
^[a-zA-Z0-9\_-]+$, but the description states locations must "match an existing GCP location, or 'global'". The current pattern allows arbitrary alphanumeric strings that are not valid GCP regions. Either tighten the pattern to match known GCP regions or clarify that validation is delegated to the GCP API at runtime.features/features.go (1)
801-807: Minor formatting inconsistency.The indentation on line 802 appears off compared to the surrounding lines.
FeatureGateImageVolume = newFeatureGate("ImageVolume"). - reportProblemsToJiraComponent("Node"). + reportProblemsToJiraComponent("Node"). contactPerson("haircommander").
📜 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 (21)
config/v1/types_feature.go(2 hunks)config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml(1 hunks)features.md(1 hunks)features/features.go(24 hunks)features/okd_featureset_parity_test.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml
- features.md
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml
- payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml
- operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.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:
config/v1/types_feature.gomachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yamlfeatures/okd_featureset_parity_test.goconfig/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlfeatures/features.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml
🧬 Code graph analysis (2)
features/okd_featureset_parity_test.go (2)
features/features.go (1)
AllFeatureSets(21-34)config/v1/types_feature.go (3)
Default(41-41)OKD(58-58)FeatureGateAttributes(136-144)
features/features.go (2)
config/v1/types_feature.go (4)
Default(41-41)OKD(58-58)DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)features/util.go (2)
SelfManaged(36-36)Hypershift(35-35)
🔇 Additional comments (15)
payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (1)
1-10: Well-structured CRD with proper OKD featureset annotation.The CRD is correctly annotated with
release.openshift.io/feature-set: OKD(line 9) and includes comprehensive cloud-platform-specific configuration support with thoughtful validation constraints. The status subresource is properly enabled for read-only tracking. The file aligns with PR objectives.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (1)
5-5: Verify API approval reference PR #2255 includes MachineConfigNode scope.The api-approved annotation references PR #2255. Ensure that this PR approval covers both the Hypershift-OKD and SelfManagedHA-OKD variants of the MachineConfigNode CRD for both deployment models.
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml (2)
6-8: Verify consistency of OKD feature set deployment across all MachineConfigNode CRD variants.Both CRDs include
api.openshift.io/merged-by-featuregates: "true"andrelease.openshift.io/feature-set: OKD, indicating these CRDs are conditionally merged based on the OKD feature set being enabled. Confirm that the feature gate logic in the build/merge pipeline correctly conditionally includes both the Hypershift and SelfManagedHA variants when OKD feature set is active.
1-383: Verify generated CRD source was properly updated.These files are marked as
zz_generated.crd-manifests(auto-generated). Ensure that the source Go types or CRD generation tool was updated to produce both Hypershift-OKD and SelfManagedHA-OKD variants with the correct annotations and feature set metadata.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-122: LGTM!The CRD manifest correctly includes the
release.openshift.io/feature-set: OKDannotation and follows the established pattern for cluster-scoped configuration resources.config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml (1)
83-97: LGTM!The OKD enum addition and the transition validation rule are correctly implemented. The rule
oldSelf == 'OKD' ? self != '' : trueproperly prevents OKD from transitioning to Default while allowing transitions to TechPreviewNoUpgrade and DevPreviewNoUpgrade.features/features.go (1)
39-45: LGTM!The OKD feature set is correctly added to feature gates that are enabled in Default. This ensures OKD clusters have access to all Default features, aligning with the intended hierarchy where OKD is a superset of Default.
payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml (1)
1-366: LGTM!The manifest correctly defines the OKD feature set for SelfManagedHA clusters. The enabled/disabled feature lists align with the
enableInconfigurations infeatures/features.go. TheHyperShiftOnlyDynamicResourceAllocationis appropriately disabled since it's only enabled for the Hypershift cluster profile.config/v1/types_feature.go (2)
56-61: LGTM!The OKD FeatureSet constant is well-documented and correctly added to
AllFixedFeatureSets. The distinction from other "NoUpgrade" feature sets is clear—OKD allows upgrades but prevents transitioning back to Default.
74-79: Validation rule correctly implemented.The XValidation rule using
self != ''properly prevents OKD from transitioning to Default (which is serialized as an empty string), while still allowing transitions to TechPreviewNoUpgrade and DevPreviewNoUpgrade as discussed in prior review comments.features/okd_featureset_parity_test.go (1)
12-53: LGTM! Well-structured parity test.The test correctly verifies that the OKD feature set includes all Default feature gates, which aligns with the intended containment relationship. The implementation is clean, with clear error reporting and proper use of set operations to detect missing gates.
payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-122: LGTM! Standard generated CRD manifest for OKD feature set.This CRD manifest for Scheduler follows the expected structure and includes appropriate OKD feature-set annotations. The schema definition is complete with proper field documentation and validation constraints.
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml (1)
10-441: LGTM! Comprehensive example CRD demonstrating validation patterns.This example CRD effectively demonstrates various OpenAPI schema validation patterns including format markers, CEL validations, immutable fields, union types, and subnet validation. The OKD feature-set annotation is properly configured.
config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml (1)
1-794: LGTM! Comprehensive ClusterVersion CRD for OKD feature set.This CRD manifest properly defines the ClusterVersion resource with extensive schema coverage for cluster update management, capabilities configuration, and conditional update handling. The OKD feature-set annotation is correctly applied, and the complex validation rules appear sound.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml (1)
95-96: This validation rule comment is based on incorrect assumptions about the featureSet API."OKD" is not a valid value in the FeatureGate
spec.featureSetenum. The valid values are: Default, TechPreviewNoUpgrade, DevPreviewNoUpgrade, and CustomNoUpgrade. The ruleoldSelf == 'OKD' ? self != '' : truechecks for a state that cannot legitimately occur in the API, so the concern about unintended upgrade paths from OKD to other feature sets is not applicable.Likely an incorrect or invalid review comment.
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml
Show resolved
Hide resolved
|
/test minor-e2e-upgrade-minor |
jatinsu
force-pushed
the
OKD-featuregates
branch
from
926ca0a to
adb8abe
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (2)
350-351: Fix typo in status schema description.Line 351 contains a typo: "ceryExpiry" should be "certExpiry".
Apply this diff to correct the spelling:
- description: ceryExpiry contains the bundle name and the expiry + description: certExpiry contains the bundle name and the expiry
33-34: Fix grammatical error in additionalPrinterColumn description.Line 33-34 contains a double negative that is grammatically incorrect: "When at least one of machine is not either not updated..."
Apply this diff to fix the grammar and subject-verb agreement:
- - description: When at least one of machine is not either not updated or is in - the process of updating to the desired machine config. + - description: When at least one machine is not updated or is in the process of + updating to the desired machine config.operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml (1)
100-111: Add 'controlplanemachinesets' to the enum (spec and status).Description lists both machinesets and controlplanemachinesets, but enum only allows machinesets. Include both to avoid invalid rejections.
Apply in both places:
- enum: - - machinesets + enum: + - machinesets + - controlplanemachinesetsAlso applies to: 737-744
payload-manifests/crds/0000_10_config-operator_01_nodes-OKD.crd.yaml (1)
56-58: Fix typo and grammatical error in description.This is the same issue already flagged in config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml. Both files need the correction:
workerLatencyProfile: description: |- - workerLatencyProfile determins the how fast the kubelet is updating + workerLatencyProfile determines how fast the kubelet is updating the status and corresponding reaction of the clusterexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml (1)
5-5: Replace placeholder URL with actual PR reference or clear placeholder.The api-approved annotation contains a placeholder URL (
pull/xxx). Even for example CRDs, use either a real PR reference or a more obvious placeholder likehttps://github.com/openshift/api/pull/EXAMPLE.- api-approved.openshift.io: https://github.com/openshift/api/pull/xxx + api-approved.openshift.io: https://github.com/openshift/api/pull/EXAMPLEconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (1)
65-80: Critical: PEM validation contradictsformat: bytespecification.In Kubernetes CRD schemas,
format: bytedeclares values are base64-encoded per OpenAPI specification (RFC 4648). However, the CEL validation rules check for plaintext PEM headers (-----BEGIN CERTIFICATE-----). This creates an irreconcilable contradiction: users must violate either theformatdeclaration or the validation rules.This is a schema correctness bug that will cause validation failures. Fields affected: fulcioCAData (lines 65–80), rekorKeyData (lines 110–125), keyData (lines 143–159), and rekorKeyData (lines 160–174).
Recommended fix (Option A): Accept plaintext PEM by removing
format: byteand updating descriptions to clarify plaintext PEM is expected (not base64-encoded).fulcioCAData: description: |- - fulcioCAData is a required field contains inline base64-encoded data for the PEM format fulcio CA. + fulcioCAData is a required field containing the PEM format fulcio CA certificate. fulcioCAData must be at most 8192 characters. - format: byte maxLength: 8192 type: stringApply the same fix to rekorKeyData (lines 110–125), keyData (lines 143–159), and the other rekorKeyData (lines 160–174).
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml (1)
552-555: Doc typo: ProviderSpec.“The ProviderSe should be complete...” → “The ProviderSpec should be complete...”
📜 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 (52)
config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1/types_feature.go(2 hunks)config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)example/v1/types_stable.go(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-TechPreviewNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yaml(1 hunks)features.md(1 hunks)features/features.go(24 hunks)features/okd_featureset_parity_test.go(1 hunks)machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml(1 hunks)machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yaml(1 hunks)machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(1 hunks)operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_nodes-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml
- features/features.go
- openapi/generated_openapi/zz_generated.openapi.go
- example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yaml
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml
- example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yaml
- example/v1/types_stable.go
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.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:
config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yamlpayload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yamlconfig/v1/types_feature.gomachine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlmachine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlmachine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yamlpayload-manifests/crds/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_nodes-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlopenapi/openapi.jsonexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlfeatures.mdpayload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yamlexample/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yamloperator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yamlpayload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yamlfeatures/okd_featureset_parity_test.go
🔇 Additional comments (30)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (1)
1-383: LGTM!The CRD manifest is well-structured with comprehensive validation rules, proper metadata annotations for OKD feature set, and correct use of RFC1123 subdomain constraints. Status subresource and additionalPrinterColumns are appropriately configured.
payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (1)
1-617: Verify OKD featureset inclusion is complete.The CRD includes appropriate OKD-specific annotations (
release.openshift.io/feature-set: OKD), includes markers for self-managed-high-availability and ibm-cloud-managed deployments, and has comprehensive validation schemas. The MachineConfigPool structure aligns with machine configuration management requirements.Verify that all MachineConfigPool features required for OKD are represented in this variant and that it maintains consistency with corresponding Default and TechPreviewNoUpgrade featuresets.
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml (1)
1-383: LGTM!The SelfManagedHA variant of MachineConfigNode CRD is well-structured with consistent validation rules and appropriate metadata annotations for self-managed-high-availability deployments. The schema properly enforces RFC1123 constraints and generation monotonicity rules for pinned image sets.
payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (1)
1-383: Note: Verify intentional duplication across directories.This file is identical in content to the generated CRD manifest in
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml. If both manifests are maintained separately, ensure there's a process to keep them synchronized during future updates to avoid divergence.Confirm this is intentional and that a synchronization mechanism is in place for the duplicated manifests.
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (2)
258-272: This file is auto-generated; review concerns at the source template level.The file path prefix
zz_generatedindicates this CRD manifest is automatically generated from a source template. Any validation logic issues should be investigated in the source template, not in this file. The domain validation rule at lines 261–262 itself appears logically sound: it correctly enforces that single-component domains (with no dots) must belocalhost, while allowing multi-component domains to pass validation.
1-10: Confirm source template for auto-generated CRD.This file is auto-generated (indicated by
zz_generatedprefix). The source ImagePolicy type definition exists in the openshift/api repository under config/v1alpha1 with proper kubebuilder markers (e.g.,+kubebuilder:object:root=true,+kubebuilder:resource:path=imagepolicies,+kubebuilder:subresource:status) and field-level validation rules. Ensure the source Go struct is included in this PR and any schema changes are applied to the source template, not to this generated manifest. CRD manifests are auto-generated via controller-gen using the Makefile targets (e.g.,make manifests).payload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yaml (1)
89-94: LGTM.Schema, validations, and managementState enum look consistent with operator patterns.
Also applies to: 171-178
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yaml (1)
326-329: LGTM.Atomic list semantics for excludeSubnets are appropriate here.
config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yaml (1)
32-32: LGTM: tests updated to include OKD as supported value.The expected error strings now reflect OKD in the enum list.
Also applies to: 124-124
config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml (1)
79-85: LGTM: OKD added to enum with transition guard.Enum and XValidation preventing OKD → Default align with PR intent.
Also applies to: 96-98
example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yaml (1)
334-334: LGTM: Consistent atomic list behavior.The addition of
x-kubernetes-list-type: atomictoexcludeSubnetsaligns it with thesubnetslist (line 352), ensuring both lists are treated as atomic units during updates.features.md (1)
1-116: LGTM: Documentation updated for OKD featureset.The table correctly adds OKD columns for both Hypershift and SelfManagedHA cluster profiles. Empty cells are expected as this represents the initial OKD featureset configuration.
payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml (1)
82-96: LGTM: OKD featureset properly integrated with correct validation.The OKD enum value and transition validation are correctly implemented. The rule prevents OKD from reverting to Default (empty string) while allowing transitions to other feature sets.
features/okd_featureset_parity_test.go (1)
12-53: LGTM: Parity test ensures OKD includes all Default gates.The test logic is sound and provides clear error messages with sorted lists of any missing gates. The use of sets for difference calculation is efficient and appropriate.
config/v1/types_feature.go (2)
56-62: LGTM: OKD featureset properly defined with correct validation.The OKD constant, its inclusion in
AllFixedFeatureSets, and the transition validation are correctly implemented. The XValidation rule properly prevents transition to Default (empty string) while allowing transitions to other feature sets.
74-78: LGTM: Enum and validation rules correctly configured.The enum includes OKD and the XValidation rule uses the correct empty string comparison for Default. This aligns with the generated CRD manifests.
openapi/openapi.json (1)
14328-14328: Verify consistency of atomic list annotations across the schema.The addition of
"x-kubernetes-list-type": "atomic"is a valid Kubernetes OpenAPI extension that ensures list fields are replaced atomically rather than merged. However, only a single hunk is visible in this review. Please verify that:
- This annotation has been consistently applied to all relevant list-type fields in the affected schema sections.
- The change aligns with the PR's schema enrichment objectives and doesn't introduce conflicting or incomplete annotations.
- Existing fields with similar list semantics elsewhere in the schema are treated uniformly.
config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml (1)
1-10: CRD structure and OKD integration look good.The ClusterVersion CRD is well-structured with comprehensive schema definitions, proper OKD feature-set annotations, and appropriate status subresource configuration.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml (1)
1-10: APIServer CRD structure is well-designed with comprehensive configuration options.The schema is properly organized with clear descriptions of audit profiles, TLS configuration, encryption options, and other API server settings. OKD feature-set marker is correctly applied.
operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml (1)
210-212: CEL validation rules for operator state management are properly designed.The validations correctly enforce monotonic increases for revision tracking and prevent invalid state transitions (e.g., cannot unset currentRevision once set, only one node can have nonzero targetRevision). These constraints are appropriate for operator reconciliation logic.
Also applies to: 230-232, 274-285, 290-293
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yaml (1)
1-10: MachineSet CRD structure follows established patterns with proper lifecycle management.The schema correctly implements MachineSet semantics with provider flexibility, lifecycle hooks for maintenance coordination, and comprehensive status tracking. OKD feature-set marker is correctly applied and scale subresource is properly configured.
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yaml (2)
122-124: Verify list-type marker consistency across lifecycle hooks, conditions, and taints.The file mixes
x-kubernetes-list-type: map(for lifecycleHooks, conditions, ownerReferences) andx-kubernetes-list-type: atomic(for taints, addresses). Ensure this distinction is intentional: map-type lists should have correspondingx-kubernetes-list-map-keys(which are present here), while atomic-type lists represent unordered sets where mutation semantics differ.For example:
- Lines 122–124: lifecycleHooks preDrain uses
maptype withnamekey — correct for keyed ordering.- Line 342: taints uses
atomictype — acceptable if taints order does not matter and they should not be merged in strategic merge patches.Verify this aligns with the upstream Kubernetes API conventions for these resource types.
Also applies to: 158-160, 342-342, 365-365, 411-413
1-12: Confirm OKD feature-set annotation and capability alignment.Line 10 correctly sets
release.openshift.io/feature-set: OKD, and line 6 setsapi.openshift.io/merged-by-featuregates: "true". Verify that this Machine CRD is intended to be bundled with the OKD featureset and not conflict with existing Default or TechPreview variants.payload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (1)
305-337: Validate scopes field CEL rules for correctness.Lines 321–334 contain complex CEL expressions for validating image scope format. While the intent is clear (enforce fully qualified domain names, wildcard restrictions, and repository namespace rules), the nested
.split()and.matches()operations are intricate.Recommend testing these rules with sample scopes (including edge cases like wildcards, localhost, multi-level paths) to ensure they behave as intended, particularly:
- Wildcard matching rule (lines 326–330):
self.contains('*') ? self.matches(...) : true- Repository validation rule (lines 331–334): complex multi-line regex with alternation
operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml (1)
307-310: Verify complex CEL validation rule for nodeStatuses.Line 309 contains a complex CEL expression:
size(self.filter(status, status.?targetRevision.orValue(0) != 0)) <= 1. This rule enforces at most one node status with a non-zero targetRevision. While the intent is clear, verify that:
- The
.?targetRevision.orValue(0)syntax correctly handles optional fields in CEL.- The filter logic properly evaluates the condition across all elements.
This is a critical constraint for ensuring serialized updates, so correctness is important.
operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (1)
1-9: Approve ClusterCSIDriver CRD structure and OKD feature-set annotation.CRD is properly annotated with
release.openshift.io/feature-set: OKD(line 9) andapi.openshift.io/merged-by-featuregates: "true"(line 6). The metadata.name enum constrains driver names to known CSI drivers, which is appropriate for cluster stability.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-10: Approve Scheduler CRD for OKD featureset.CRD correctly includes
release.openshift.io/feature-set: OKDannotation (line 9) and bootstrap-required flag (line 8). The schema is straightforward with sensible defaults (mastersSchedulable defaults to false, profile defaults to LowNodeUtilization). No issues identified.machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml (3)
94-101: Verify CEL function usage in machineNamePrefix validation.Line 100 uses
!format.dns1123Subdomain().validate(self).hasValue()to validate the machineNamePrefix. This CEL syntax appears unconventional; standard CEL for DNS validation typically uses string matching or direct format validation. Verify that:
- The
format.dns1123Subdomain()function is available in the CEL environment used by OpenShift CRD validation.- The
.validate(self).hasValue()chaining is the correct pattern for this validation context.If this is not supported, consider using an explicit regex pattern (as done elsewhere in the file) for clarity and portability.
478-502: Verify cross-platform failureDomain validation rules for all platforms.Lines 478–502 contain platform-specific CEL validation rules that enforce required configurations:
- AWS requires
has(self.aws)whenplatform == 'AWS'- Similar rules for Azure, GCP, OpenStack, vSphere, Nutanix
These rules ensure that only the relevant platform configuration is present. Verify:
- All supported platforms have corresponding validation rules.
- The ternary logic (required if platform matches, forbidden otherwise) is consistent across all rules.
Spot-check: vSphere rule on lines 495–498 appears correct. All rules follow the same pattern.
529-543: Strict metadata label validation enforces cluster-api label requirements.Lines 529–543 enforce three required labels on template metadata via CEL rules:
machine.openshift.io/cluster-api-machine-rolemust equal'master'machine.openshift.io/cluster-api-machine-typemust equal'master'machine.openshift.io/cluster-api-clustermust existThese constraints ensure that control plane machines are properly labeled for cluster API discovery and management. This is appropriate for safety. However, verify that:
- These label requirements are documented in user-facing docs or release notes.
- Tooling/controllers that create ControlPlaneMachineSets are updated to set these labels correctly.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml
Show resolved
Hide resolved
...pha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml
Show resolved
Hide resolved
...ig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml
Show resolved
Hide resolved
...oad-manifests/crds/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml
Show resolved
Hide resolved
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Can you push a DNM commit that shows that this test fails our CI?
jatinsu
force-pushed
the
OKD-featuregates
branch
from
adb8abe to
37a99c2
Compare
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml (1)
5-5: Placeholder URL in api-approved annotation needs to be replaced.The
api-approved.openshift.ioannotation contains a placeholderpull/xxxwhich should be updated to a real PR reference or a clearer placeholder.
🧹 Nitpick comments (1)
features/features.go (1)
39-947: OKD enablement across feature gates looks coherent; consider reducing repetitionThe updates systematically add
configv1.OKDwherever features are enabled forconfigv1.Default(and, where appropriate, for the preview sets), matching the stated intent that Default gates are a subset of OKD. The SelfManaged and HypershiftenableForClusterProfileexpansions for OKD also line up with the new feature matrix.Given how many gates now use the same four-feature-set combination (
Default,OKD,DevPreviewNoUpgrade,TechPreviewNoUpgrade), you may eventually want a small helper or shared constant to represent that common set to reduce repetition and the chance of future omissions, but that’s optional and your new parity test should already catch most drift.
📜 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 (21)
config/v1/types_feature.go(2 hunks)config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml(1 hunks)features.md(1 hunks)features/features.go(25 hunks)features/okd_featureset_parity_test.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- features/okd_featureset_parity_test.go
- payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.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:
config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yamlpayload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yamlconfig/v1/types_feature.gofeatures/features.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yamlfeatures.md
🔇 Additional comments (24)
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml (1)
1-441: CRD structure and schema validation look comprehensive.The StableConfigType CRD is well-structured for the OKD feature-set integration:
- OKD feature-set annotation correctly applied (line 9)
- Comprehensive OpenAPI v3 schema with proper CEL validations for union types and field constraints
- Consistent application of
x-kubernetes-list-typemarkers on collection fields (atomic and set types)- Format field descriptions appropriately include security warnings for CVE-2021-29923 and CVE-2024-24790 related to IP validation
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (2)
1-11: Verify the API approval reference and featureset annotation consistency.These CRD manifests are correctly annotated with
release.openshift.io/feature-set: OKD(line 8), but the PR reference for API approval should be validated. Additionally, ensure this matches the approval state of corresponding non-OKD variants if they exist.Please verify:
- That the referenced API approval PR (#2255) is valid and covers this CRD variant
- That the OKD featureset labeling aligns with the overall featureset hierarchy (OKD → Default with TechPreviewNoUpgrade features)
377-379: Cross-field validation looks sound.The constraint
metadata.name == spec.node.name(lines 377-379) ensures referential integrity between resource identity and spec configuration, which is a good practice for machine config node management.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (3)
1-11: Verify API approval reference and featureset labeling consistency.The MachineConfigPool CRD is correctly labeled with
release.openshift.io/feature-set: OKD(line 9) and includes deployment model targeting (lines 8, 11). The API approval reference should be validated. Ensure the approval PR (#1453) is appropriate for the OKD featureset variant.Please verify that PR #1453 serves as the appropriate API approval for this OKD-scoped variant.
577-588: Sophisticated constraint validations on poolSynchronizersStatus are well-designed.The per-synchronizer status object includes comprehensive cross-field validations (lines 578–588) enforcing invariants like
machineCount >= updatedMachineCount,machineCount >= availableMachineCount, andavailableMachineCount >= readyMachineCount. These constraints prevent invalid state representations and improve observability.
233-249: maxUnavailable correctly uses x-kubernetes-int-or-string pattern.The
anyOfpattern (lines 234–236) withx-kubernetes-int-or-string: true(line 249) properly allows both integer and percentage-based values, following Kubernetes conventions for disruption budgets. The documentation is clear about the interaction with thepausedproperty.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml (1)
1-11: Deployment-model-specific annotations are appropriately targeted.The SelfManagedHA variant correctly includes
include.release.openshift.io/self-managed-high-availability: "true"(line 7) alongside the OKD featureset label (line 8), distinguishing it from the Hypershift variant. The schema is identical to its Hypershift counterpart, which is appropriate.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml (4)
879-899: Schema validation rule for deploymentMode customization looks correct.The validation at lines 895–899 correctly enforces:
customConfigis required whendeploymentModeisCustomConfig, and forbidden otherwise. The CEL rule uses proper quantity comparison syntax.
175-177: List type markers are correctly applied.The file consistently uses appropriate
x-kubernetes-list-typemarkers (map,set,atomic) for different list semantics, which aligns with Kubernetes API conventions and the PR objective of updating OpenAPI schemas.Also applies to: 209-210, 259-261
50-50: Required field constraints are logically sound.The spec requires at least one configuration section (minProperties: 1), and nested config objects have their own required fields (deploymentMode within alertmanagerConfig, mode within userDefined). Constraint hierarchy is correct.
Also applies to: 892-894, 1305-1307
1-20: Verify CRD conflicts with existing ClusterMonitoring definitions in the repository.This is a generated OKD-specific CRD manifest. Confirm:
- No duplicate or conflicting ClusterMonitoring CRD definitions exist in non-OKD manifests
- The feature-set annotation correctly gates this version to OKD clusters only
- The OKD variant properly extends or overrides the default variant (not creates a duplicate resource type)
config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml (1)
79-97: OKD enum and transition validation look consistent and correctOKD is added to the enum and the validation correctly prohibits transitioning from OKD to the default (
"") feature set while leaving other existing rules unchanged. This matches the payload featuregates CRD and aligns with the described OKD behavior.payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-122: Scheduler OKD SelfManagedHA CRD matches existing patternThis CRD’s annotations, schema, and status subresource mirror the existing Scheduler definition while scoping it to SelfManagedHA + OKD via annotations. Nothing stands out as inconsistent or risky.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yaml (1)
1-218: DevPreviewNoUpgrade Images CRD is consistent with the Default variantThe DevPreviewNoUpgrade-scoped Images CRD matches the known Images schema and uses the feature-set annotation to scope it correctly. The registrySources validation and list markers are intact and consistent.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-122: Generated Scheduler OKD CRD aligns with payload manifestThe generated schedulers.config.openshift.io CRD for SelfManagedHA + OKD matches the payload variant in annotations and schema, keeping type and payload in sync. No issues spotted.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yaml (1)
4-11: Default Images CRD feature-set annotation is appropriateAdding
release.openshift.io/feature-set: Defaultto the Images CRD metadata matches the pattern used for other feature-set-specific CRDs and should aid correct payload selection without altering schema behavior.features.md (1)
1-116: OKD columns in feature matrix match the code wiringThe expanded table correctly adds OKD columns and, on spot check, the Enabled cells for OKD align with the gates you’ve wired to OKD in
features.go(e.g., AdminNetworkPolicy, GatewayAPI, MachineConfigNodes, ManagedBootImages*, UserNamespaces*, ImageVolume, etc.). This keeps the docs in step with the implementation.payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml (1)
78-97: OKD featureSet enum and transition rule are now consistent and correctOKD is added to the
featureSetenum, and the new validation correctly blocks transitioning from OKD to the default feature set by checking against the empty string. This aligns with the generated CRD, resolves the earlier “Default vs empty string” issue, and matches the intended OKD behavior.config/v1/types_feature.go (1)
56-78: LGTM - Critical validation issue properly fixed.The OKD feature set integration is correct. The XValidation rule at line 78 now properly compares against the empty string (
self != '') instead of the string literal'Default', which aligns with the fact thatDefault FeatureSet = ""at line 41. This correctly prevents OKD from transitioning back to Default while allowing transitions to TechPreviewNoUpgrade, DevPreviewNoUpgrade, or CustomNoUpgrade.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml (1)
82-96: LGTM - Generated CRD correctly reflects type definitions.The OKD enum value and validation rule are properly generated. The YAML escaping of the empty string (
'''') correctly represents the transition constraint preventing OKD from reverting to Default.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yaml (1)
1-218: LGTM - Well-structured Image CRD with appropriate validations.The TechPreviewNoUpgrade variant of the Image CRD is properly structured. The mutual exclusivity validation for
blockedRegistriesandallowedRegistries(lines 167-171) correctly prevents conflicting configurations, and the comprehensive schema provides clear API boundaries for image registry configuration.config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml (1)
200-206: LGTM - Correct cross-field validations for update configuration.The CEL validations properly enforce the desiredUpdate constraints: preventing both Architecture and Image from being set simultaneously, and requiring Version when Architecture is specified. These rules prevent invalid update configurations.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yaml (1)
1-218: LGTM - Consistent CustomNoUpgrade Image CRD variant.The CustomNoUpgrade variant maintains the same well-structured schema and validations as other Image CRD variants, ensuring consistency across feature sets.
operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (1)
299-306: LGTM - Correct platform-specific configuration enforcement.The CEL validation at lines 302-306 properly enforces that the
ibmcloudconfiguration must be set if and only ifdriverTypeis'IBMCloud'. This prevents misconfiguration where platform-specific settings are either missing or incorrectly specified for the wrong platform.
jatinsu
force-pushed
the
OKD-featuregates
branch
from
37a99c2 to
2a40ef9
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/features.go (1)
586-592: Critical: Add configv1.OKD to maintain Default/OKD parity.
FeatureGateImageStreamImportModeis enabled forconfigv1.Defaultbut missingconfigv1.OKD. This violates the stated requirement that OKD must include all Default feature gates and will cause the parity test infeatures/okd_featureset_parity_test.goto fail.Apply this diff to add OKD:
FeatureGateImageStreamImportMode = newFeatureGate("ImageStreamImportMode"). reportProblemsToJiraComponent("Multi-Arch"). contactPerson("psundara"). productScope(ocpSpecific). enhancementPR(legacyFeatureGateWithoutEnhancement). - enableIn(configv1.Default, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade). + enableIn(configv1.Default, configv1.OKD, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade). mustRegister()
♻️ Duplicate comments (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml (1)
56-58: ** Fix typo and grammatical error in description (flagged in prior review).**Line 57 contains a typo ("determins" should be "determines") and awkward grammar ("the how fast").
workerLatencyProfile: description: |- - workerLatencyProfile determins the how fast the kubelet is updating + workerLatencyProfile determines how fast the kubelet is updating the status and corresponding reaction of the clusterconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (1)
65-80: ** Fix PEM validation contradiction withformat: byte(flagged in prior review).**The schema declares
format: byte(meaning base64-encoded per OpenAPI spec), but CEL validation rules check for plaintext PEM headers (-----BEGIN CERTIFICATE-----, etc.). This creates a contradiction where neither plaintext nor base64-encoded PEM will validate.Option A (simpler, recommended): Accept plaintext PEM. Remove
format: bytefrom the four affected fields and update descriptions to clarify plaintext PEM is expected.Apply this diff to all four affected fields (
fulcioCAData,rekorKeyData, and the twokeyData/rekorKeyDatainpublicKey):fulcioCAData: description: |- - fulcioCAData is a required field contains inline base64-encoded data for the PEM format fulcio CA. + fulcioCAData is a required field containing the PEM-formatted Fulcio CA certificate. - format: byte maxLength: 8192 type: stringRepeat for
rekorKeyData(lines 110–125),keyData(lines 143–159), andpublicKey.rekorKeyData(lines 160–174).Also applies to: 110-125, 143-159, 160-174
operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml (2)
737-744: Add missing controlplanemachinesets enum value (status field).Same issue as in the spec field - the status enum is missing
controlplanemachinesets. Keep both enums synchronized.Apply this diff:
enum: - machinesets + - controlplanemachinesets
104-111: Add missing controlplanemachinesets enum value.The description states "Valid values are machinesets and controlplanemachinesets" but the enum only includes
machinesets. Add the missing value to match the documentation.Apply this diff:
enum: - machinesets + - controlplanemachinesetspayload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (1)
384-390: Fix inconsistent enum value casing for status condition.Line 389 has
Unknownwithout quotes, while lines 387–388 have quoted string values"True"and"False". In YAML, unquoted values are treated as different types. Ensure consistency:status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (1)
320-326: Fix inconsistent enum value casing for status condition (alpha1 version).Line 325 has
Unknownwithout quotes, while lines 323–324 have quoted string values"True"and"False". Ensure consistency:status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringpayload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml (1)
384-390: Fix inconsistent enum value casing for status condition (ClusterImagePolicy).Line 388 has
Unknownwithout quotes, while lines 386–387 have quoted string values"True"and"False". Ensure consistency:status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: string
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml (1)
59-878: Consider extracting reusable schema components to reduce duplication.The schemas for
resources,tolerations, andtopologySpreadConstraintsare nearly identical betweenalertmanagerConfig.customConfigandmetricsServerConfig. While valid, this verbosity creates maintenance friction—future updates to these constraints would need synchronized changes across multiple locations.If the repository uses tools for CRD code generation (e.g., controller-gen, kube-openapi), this duplication would be unavoidable. However, if this file is hand-maintained or generated from structured definitions, consider whether shared schema components could be extracted.
Also applies to: 906-1286
📜 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 (52)
config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1/types_feature.go(2 hunks)config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)example/v1/types_stable.go(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-TechPreviewNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yaml(1 hunks)features.md(1 hunks)features/features.go(24 hunks)features/okd_featureset_parity_test.go(1 hunks)machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml(1 hunks)machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yaml(1 hunks)machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(1 hunks)operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_images-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_images-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yaml
- openapi/generated_openapi/zz_generated.openapi.go
- example/v1/types_stable.go
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml
- openapi/openapi.json
- operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml
- machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yaml
- config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml
- example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-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:
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yamlfeatures.mdconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yamlmachine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yamlmachine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yamlfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_images-OKD.crd.yamlconfig/v1/types_feature.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlfeatures/okd_featureset_parity_test.gomachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yamlexample/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yamlpayload-manifests/crds/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yamlconfig/v1/tests/featuregates.config.openshift.io/AAA_ungated.yamlconfig/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_images-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_clustermonitorings-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml
🧬 Code graph analysis (2)
features/features.go (2)
config/v1/types_feature.go (4)
Default(41-41)OKD(58-58)DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)features/util.go (2)
SelfManaged(36-36)Hypershift(35-35)
features/okd_featureset_parity_test.go (2)
features/features.go (1)
AllFeatureSets(21-34)config/v1/types_feature.go (3)
Default(41-41)OKD(58-58)FeatureGateAttributes(136-144)
🔇 Additional comments (44)
payload-manifests/crds/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml (2)
4-10: OKD featureset annotation is correctly applied. The CRD metadata properly marks this resource as part of the OKD feature set, and the API pre-approval and merge-by-featuregates annotations are in place.
895-899: Cross-field validation pattern is well-implemented. The CEL rule enforcing the relationship betweendeploymentModeandcustomConfigis clear and follows Kubernetes validation best practices.payload-manifests/crds/0000_10_config-operator_01_images-CustomNoUpgrade.crd.yaml (1)
1-218: Well-structured CRD manifest with proper validation and list type markers.The file correctly defines the Image CRD variant with:
- Proper OpenAPI v3 schema with documented fields and types
- Cross-field validation enforcing mutual exclusion between
blockedRegistriesandallowedRegistries(lines 167–171)- Appropriate
x-kubernetes-list-typemarkers on all list fields (atomicfor most,setforcontainerRuntimeSearchRegistries)- Subresource configuration for status
- Comprehensive field descriptions and constraints (e.g.,
minItems: 1for search registries)However, verify the feature-set annotation alignment: this manifest is annotated with
release.openshift.io/feature-set: CustomNoUpgrade(line 10), but the PR objectives focus on introducing the OKD featureset. Confirm this CustomNoUpgrade variant is intentional and part of the parallel CRD variants being added, or if a separate OKD variant should be the primary focus for this PR.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-OKD.crd.yaml (1)
1-1319: Verify that this generated CRD manifest was produced by the correct generation tool with no errors.This is a generated file (
zz_generatedprefix); the YAML structure is well-formed and the OpenAPI v3 schema follows Kubernetes conventions (properx-kubernetes-list-typemarkers, CEL validations, field constraints). However, verify that:
- The file was generated from the correct source Go types and generation markers.
- No generation errors or mismatches occurred during creation.
- This OKD variant aligns with the base (non-OKD) ClusterMonitoring CRD structure—there should be no conflicting schemas between the two variants.
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (3)
1-22: CRD structure and feature-gate integration look solid.Metadata annotations correctly identify this as OKD-specific (
release.openshift.io/feature-set: OKD) and feature-gate-merged. Cluster scope is appropriate for MachineConfigPool.
540-588: Verify poolSynchronizersStatus cross-field validation invariants.The validation rules in lines 577–588 constrain machine count relationships. Confirm that the semantic intent is correct, particularly line 586–588 which assumes readyMachineCount ⊆ availableMachineCount. If a "ready" machine can exist in an "unavailable" state, this constraint may be too strict.
320-341: RFC-1123 subdomain validation for pinnedImageSets.name is well-formed.Pattern and constraints (minLength: 1, maxLength: 253, maxItems: 100) correctly enforce DNS-compliant names with proper segment limits.
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml (3)
1-11: Deployment-model annotation correctly flags this variant for SelfManagedHA topology.Metadata structure and feature-set annotation are properly configured. CRD will only be deployed on SelfManagedHA OKD clusters.
355-367: Cross-field CEL validation rules are logically sound.The rules use appropriate null-coalescing patterns (
has()checks) to handle optional fields. Rule 3 appropriately requires error details when a failure marker is present. Syntax appears correct, but automated CEL validation would confirm.
248-284: Status configVersion structure correctly makes desired required, current optional.Semantics align with update lifecycle: controller targets desired version, then current tracks what's actually applied. RFC1123 validations on both ensure name consistency.
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (2)
1-11: Deployment-model annotation correctly flags this variant for Hypershift (IBM Cloud Managed) topology.Annotation difference from SelfManagedHA variant (File 2) is appropriate; both CRDs have identical schema and should be deployed to their respective topologies.
77-379: Schema is consistent with SelfManagedHA variant (File 2).Both variants have identical spec/status structure and validations, differing only in deployment-model annotation—this is the correct design pattern for topology-specific CRD variants.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yaml (1)
10-10: LGTM. The metadata annotation aligns with the OKD feature-set enablement. No concerns.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml (1)
1-20: LGTM. The APIServer CRD is well-structured with appropriate schema validation, defaults, and audit configuration options. The feature-set annotation and nested validations align with OpenShift conventions.config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml (1)
200-207: LGTM. The cross-field validation rules fordesiredUpdatecorrectly enforce valid combinations of image/version/architecture. The capability constraints (marketplace requiring OperatorLifecycleManager) and overall schema structure align with Kubernetes best practices.operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (1)
123-129: LGTM. Platform-specific driver configurations are well-structured with appropriate ARN and CRN pattern validation. The CEL rule correctly enforces IBM Cloud configuration when the driver type is IBMCloud. The metadata name enum appropriately restricts to known CSI driver names.Also applies to: 244-251
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yaml (1)
84-161: LGTM. Lifecycle hooks are properly constrained, and the preserv-unknown-fields approach for platform-specific providerSpec is appropriate. The status structure comprehensively captures machine state and error information following Kubernetes conventions.machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml (1)
529-543: LGTM. Template metadata validation rules correctly enforce required cluster-related labels via CEL constraints, and the cross-field validations for failure domains appropriately enforce platform-specific configuration requirements. The scale subresource is properly configured.payload-manifests/crds/0000_10_config-operator_01_images-Default.crd.yaml (1)
10-10: LGTM!The feature-set metadata annotation correctly marks this CRD as belonging to the Default feature set, consistent with the broader OKD feature-set integration.
example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml (1)
328-328: LGTM!Adding
x-kubernetes-list-type: atomicto excludeSubnets aligns with the existing atomic list semantics used for the subnets field, ensuring consistent list behavior across related fields.config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yaml (2)
32-32: LGTM!Test expectation correctly updated to include OKD in the list of supported featureSet values for validation error messages.
124-124: LGTM!Test expectation correctly updated to include OKD in the supported featureSet values list, consistent with the first test case.
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yaml (1)
328-328: LGTM!Consistent with the atomic list addition in related CRD manifests, ensuring uniform list semantics for excludeSubnets.
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yaml (1)
333-333: LGTM!Atomic list type addition consistent with other CRD manifest variants.
features/okd_featureset_parity_test.go (1)
16-53: LGTM - Test correctly enforces OKD/Default parity.The test implementation is sound and will catch any feature gates enabled in Default but missing from OKD. The logic correctly:
- Iterates through all cluster profiles and feature sets
- Uses set operations to identify missing gates
- Provides clear error messages with sorted lists
This test should catch the ImageStreamImportMode inconsistency flagged in features.md and enforce the stated requirement that OKD must include all Default feature gates.
features/features.go (4)
43-85: LGTM - Consistent OKD integration pattern.The additions of
configv1.OKDto these feature gates correctly maintain parity with Default, ensuring OKD clusters have the same baseline features as Default clusters.
123-450: LGTM - OKD correctly added across feature gates.The pattern of adding
configv1.OKDalongsideconfigv1.Defaultis consistently applied across these feature gate definitions, properly implementing the OKD feature set as a superset of Default.
473-506: LGTM - Cluster-profile-specific OKD enablement correct.The
enableForClusterProfilecalls correctly add OKD support for SelfManaged cluster profile, aligning with the OKD feature set integration strategy.
536-940: LGTM - Remaining OKD additions follow correct pattern.The OKD feature set additions throughout the rest of the file consistently follow the established pattern, properly integrating OKD into the feature gating system.
features.md (1)
76-76: Verify the ImageStreamImportMode omission from OKD.The documentation shows ImageStreamImportMode enabled for Default (Hypershift + SelfManagedHA) and TechPreviewNoUpgrade, but NOT enabled for OKD. This appears inconsistent with the stated goal that OKD should include all Default feature gates plus selected TechPreviewNoUpgrade features. This would also fail the parity test in
features/okd_featureset_parity_test.go.Confirm whether this omission is intentional or if ImageStreamImportMode should be added to OKD.
config/v1/types_feature.go (2)
56-62: LGTM! OKD featureset properly integrated.The OKD featureset constant, comment, and inclusion in
AllFixedFeatureSetsare correctly implemented. The XValidation rule preventing transition from OKD back to Default (empty string) is correct, as confirmed by the past review fix.
74-78: LGTM! Enum and validation correctly updated.The OKD value is properly added to the featureSet enum, and the XValidation rule correctly uses the empty string (
'') to prevent transitioning from OKD to Default.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml (1)
82-96: LGTM! Generated CRD correctly reflects Go types.The OKD enum value and XValidation rule are properly generated from the source types. The validation correctly prevents transitioning from OKD to the default (empty string) featureset.
payload-manifests/crds/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yaml (1)
1-218: LGTM! Standard Image CRD with proper validations.This CRD manifest for the TechPreviewNoUpgrade feature set includes proper schema definitions, list-type annotations, and the mutual-exclusivity validation between blockedRegistries and allowedRegistries. The structure is consistent with OpenShift CRD patterns.
payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml (1)
82-96: LGTM! Payload CRD correctly includes OKD featureset.The OKD enum value and transition validation are properly included in the payload CRD, consistent with the generated manifests.
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml (1)
1-441: LGTM! Comprehensive example CRD with proper OKD feature-set annotation.This example CRD demonstrates various validation patterns (CEL, format markers, immutability, unions, etc.) and correctly includes the OKD feature-set annotation. The schema is well-structured with appropriate list types and validations.
payload-manifests/crds/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yaml (1)
1-218: LGTM! Standard Image CRD for DevPreviewNoUpgrade.This CRD manifest follows the same pattern as the TechPreviewNoUpgrade variant with proper schema definitions, validations, and list-type annotations.
payload-manifests/crds/0000_10_config-operator_01_images-OKD.crd.yaml (1)
51-176: No action needed—theimageStreamImportModefield is correctly omitted from the OKD variant.
imageStreamImportModeis a Technology Preview feature that requires the TechPreviewNoUpgrade FeatureGate to be enabled. OKD (the community OpenShift distribution) does not support Technology Preview features, so the field should not appear in its CRD. The field's presence in TechPreviewNoUpgrade and absence in OKD is intentional and correct per the architectural design of feature-gated CRD variants.Likely an incorrect or invalid review comment.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yaml (1)
1-218: LGTM! The Image CRD for DevPreviewNoUpgrade is well-structured with proper validations and mutual-exclusivity constraints on registrySources.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-OKD.crd.yaml (1)
51-154: Verify consistency of Image CRD spec across feature-set variants.The OKD variant of the Image CRD is missing the
imageStreamImportModefield that exists in the DevPreviewNoUpgrade and TechPreviewNoUpgrade variants (spec lines 105–122 in those files). This field is important for controlling image import behavior. If this omission is intentional for OKD, document why; otherwise, add this field to maintain API consistency across feature-set variants.Compare with the DevPreviewNoUpgrade variant to confirm whether this field should be included in the OKD CRD.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-122: LGTM! The Scheduler CRD has a well-defined spec with proper enums and a sensible profile default.operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml (1)
1-331: LGTM! The Etcd CRD has comprehensive status tracking with proper CEL validations enforcing deployment constraints and state transitions.config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (1)
105-144: Manual verification required: repository access unavailable.The verification cannot proceed due to inability to access the repository. The review comment claims that:
- Line 110 documents PKI as a valid
policyTypevalue- The schema properties lack a corresponding
pkifield definition- A
pkifield should be added to therootOfTrustschemaHowever, these claims cannot be verified without examining the actual CRD file and related source code. Additionally, since this is a generated file (
zz_generated.crd-manifests), any schema changes must originate from the source Go definitions, not direct edits to this YAML.Please verify the following manually:
- Check if the
pkifield is genuinely missing fromrootOfTrustproperties- Confirm the PKI policy type is documented but unsupported in the schema
- Locate the source Go CRD definitions and confirm where the fix should be applied
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yaml (1)
1-218: Generated CRD manifest is well-formed and correct.The TechPreviewNoUpgrade Image CRD is properly structured with comprehensive OpenAPI schemas, appropriate use of atomic list markers, and correct validation rules. The CEL validation rule at lines 167–171 correctly enforces mutual exclusivity between
blockedRegistriesandallowedRegistriesin both directions. No structural or schema issues identified.
| x-kubernetes-validations: | ||
| - message: a lowercase RFC 1123 subdomain must consist of lowercase | ||
| alphanumeric characters, hyphens ('-'), and periods ('.'). Each | ||
| block, separated by periods, must start and end with an alphanumeric | ||
| character. Hyphens are not allowed at the start or end of a block, | ||
| and consecutive periods are not permitted. | ||
| rule: '!format.dns1123Subdomain().validate(self).hasValue()' |
There was a problem hiding this comment.
Inverted CEL validation logic for machineNamePrefix.
Line 100 contains: !format.dns1123Subdomain().validate(self).hasValue(), which inverts the validation, requiring the field to be invalid as a DNS subdomain. This contradicts the description and the intent. The validation should succeed when the field is a valid DNS subdomain, not fail.
x-kubernetes-validations:
- message: a lowercase RFC 1123 subdomain must consist of lowercase
alphanumeric characters, hyphens ('-'), and periods ('.'). Each
block, separated by periods, must start and end with an alphanumeric
character. Hyphens are not allowed at the start or end of a block,
and consecutive periods are not permitted.
- rule: '!format.dns1123Subdomain().validate(self).hasValue()'
+ rule: 'format.dns1123Subdomain().validate(self).hasValue()'📝 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.
| x-kubernetes-validations: | |
| - message: a lowercase RFC 1123 subdomain must consist of lowercase | |
| alphanumeric characters, hyphens ('-'), and periods ('.'). Each | |
| block, separated by periods, must start and end with an alphanumeric | |
| character. Hyphens are not allowed at the start or end of a block, | |
| and consecutive periods are not permitted. | |
| rule: '!format.dns1123Subdomain().validate(self).hasValue()' | |
| x-kubernetes-validations: | |
| - message: a lowercase RFC 1123 subdomain must consist of lowercase | |
| alphanumeric characters, hyphens ('-'), and periods ('.'). Each | |
| block, separated by periods, must start and end with an alphanumeric | |
| character. Hyphens are not allowed at the start or end of a block, | |
| and consecutive periods are not permitted. | |
| rule: 'format.dns1123Subdomain().validate(self).hasValue()' |
🤖 Prompt for AI Agents
In
machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml
around lines 94 to 100 the CEL rule is inverted: it uses
'!format.dns1123Subdomain().validate(self).hasValue()' which enforces invalid
DNS names; change the rule to assert a valid DNS1123 subdomain by removing the
negation so it reads 'format.dns1123Subdomain().validate(self).hasValue()'
(i.e., ensure the rule returns true for valid subdomains).
| policyType: | ||
| description: |- | ||
| policyType is a required field specifies the type of the policy for verification. This field must correspond to how the policy was generated. | ||
| Allowed values are "PublicKey", "FulcioCAWithRekor", and "PKI". | ||
| When set to "PublicKey", the policy relies on a sigstore publicKey and may optionally use a Rekor verification. | ||
| When set to "FulcioCAWithRekor", the policy is based on the Fulcio certification and incorporates a Rekor verification. | ||
| When set to "PKI", the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI). This value is enabled by turning on the SigstoreImageVerificationPKI feature gate. | ||
| type: string | ||
| publicKey: | ||
| description: |- | ||
| publicKey defines the root of trust configuration based on a sigstore public key. Optionally include a Rekor public key for Rekor verification. | ||
| publicKey is required when policyType is PublicKey, and forbidden otherwise. | ||
| properties: | ||
| keyData: | ||
| description: |- | ||
| keyData is a required field contains inline base64-encoded data for the PEM format public key. | ||
| keyData must be at most 8192 characters. | ||
| format: byte | ||
| maxLength: 8192 | ||
| minLength: 68 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: the keyData must start with base64 encoding | ||
| of '-----BEGIN PUBLIC KEY-----'. | ||
| rule: string(self).startsWith('-----BEGIN PUBLIC KEY-----') | ||
| - message: the keyData must end with base64 encoding of | ||
| '-----END PUBLIC KEY-----'. | ||
| rule: string(self).endsWith('-----END PUBLIC KEY-----\n') | ||
| || string(self).endsWith('-----END PUBLIC KEY-----') | ||
| rekorKeyData: | ||
| description: |- | ||
| rekorKeyData is an optional field contains inline base64-encoded data for the PEM format from the Rekor public key. | ||
| rekorKeyData must be at most 8192 characters. | ||
| format: byte | ||
| maxLength: 8192 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: the rekorKeyData must start with base64 encoding | ||
| of '-----BEGIN PUBLIC KEY-----'. | ||
| rule: string(self).startsWith('-----BEGIN PUBLIC KEY-----') | ||
| - message: the rekorKeyData must end with base64 encoding | ||
| of '-----END PUBLIC KEY-----'. | ||
| rule: string(self).endsWith('-----END PUBLIC KEY-----\n') | ||
| || string(self).endsWith('-----END PUBLIC KEY-----') | ||
| required: | ||
| - keyData | ||
| type: object | ||
| required: | ||
| - policyType | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: publicKey is required when policyType is PublicKey, | ||
| and forbidden otherwise | ||
| rule: 'has(self.policyType) && self.policyType == ''PublicKey'' | ||
| ? has(self.publicKey) : !has(self.publicKey)' | ||
| - message: fulcioCAWithRekor is required when policyType is FulcioCAWithRekor, | ||
| and forbidden otherwise | ||
| rule: 'has(self.policyType) && self.policyType == ''FulcioCAWithRekor'' | ||
| ? has(self.fulcioCAWithRekor) : !has(self.fulcioCAWithRekor)' | ||
| signedIdentity: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "clusterimagepolicies.*\.crd\.yaml" -oRepository: openshift/api
Length of output: 170
🏁 Script executed:
cat -n "payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml" | head -250Repository: openshift/api
Length of output: 170
🏁 Script executed:
rg "pki" "payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml" -i -A 2 -B 2Repository: openshift/api
Length of output: 170
🏁 Script executed:
rg "fulcioCAWithRekor" "payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml" -A 2 -B 2Repository: openshift/api
Length of output: 170
🌐 Web query:
openshift clusterimagepolicies CRD policyType PKI pki field schema
💡 Result:
Summary of the ClusterImagePolicy CRD schema for policyType = "PKI" (rootOfTrust.pki):
- policyType allowed values include "PKI" (enabling this requires the SigstoreImageVerificationPKI feature gate). [1][2]
- rootOfTrust.pki (object) fields:
- caRootsData (required): inline base64-encoded PEM bytes for one or more root CA certificates (stored as bytes in API). [2][3]
- caIntermediatesData (optional): inline base64-encoded PEM bytes for intermediate CA certificate(s). [2][3]
- pkiCertificateSubject (required when pki is present): object with constraints on the expected certificate subject:
- email (string, optional)
- hostname (string, optional)
- Validation: at least one of email or hostname must be set. [2][3]
Notes:
- The PKI root-of-trust option is documented in the OpenShift ClusterImagePolicy API reference and the OpenShift API Go types. The ClusterImagePolicy CR is used by the Machine Config Operator to populate node verification configuration (/etc/containers/policy.json and sigstore registries entries). [1][2]
Sources:
[1] Red Hat / OpenShift docs — ClusterImagePolicy overview and examples (includes PKI example).
[2] OpenShift API Go types / pkg (config v1 / v1alpha1) — PKI and PKICertificateSubject struct definitions and validation rules.
[3] OpenShift API reference (ClusterImagePolicy) — .spec.policy.rootOfTrust and policyType description.
🌐 Web query:
"ClusterImagePolicy" "pki" field configuration sigstore
💡 Result:
Short answer — how to configure the PKI (BYO‑PKI) rootOfTrust in a ClusterImagePolicy for sigstore:
- Enable the PKI feature (OpenShift: SigstoreImageVerificationPKI / TechPreview as required). [2]
- Set spec.policy.rootOfTrust.policyType: "PKI" and provide the pki block with base64-encoded PEM data:
- pki.caRootsData: base64 PEM of CA root(s)
- pki.caIntermediatesData: base64 PEM of intermediate certs (optional)
- pki.pkiCertificateSubject: (optional) subject fields used to match the signing certificate (e.g. email, hostname)
- Optionally configure signedIdentity matchPolicy (MatchRepository / MatchRepoDigestOrExact / ExactRepository / RemapIdentity). [2]
- For the general ClusterImagePolicy format and authorities examples see the policy-controller docs. [1]
Minimal example (YAML fragment):
apiVersion: config.openshift.io/v1alpha1
kind: ClusterImagePolicy
spec:
policy:
rootOfTrust:
policyType: PKI
pki:
caRootsData: LS0tLS1CRUdJTiBDRVJUSUZ... # base64 PEM
caIntermediatesData: LS0tLS1CRUdJTiBDRVJ... # base64 PEM (optional)
pkiCertificateSubject:
email: [email protected]
hostname: signer.example.com
signedIdentity:
matchPolicy: MatchRepository
References:
- Sigstore policy-controller (ClusterImagePolicy fields & examples). [1]
- OpenShift / Red Hat docs explaining PKI (BYO‑PKI) fields (caRootsData, caIntermediatesData, pkiCertificateSubject) and feature gate. [2]
🌐 Web query:
openshift ImagePolicy CRD v1alpha1 pki field rootOfTrust
💡 Result:
The ImagePolicy.spec.policy.rootOfTrust "pki" option is the BYO‑PKI (bring‑your‑own PKI) root‑of‑trust used to verify cosign signatures. It requires policyType: "PKI" (feature SigstoreImageVerificationPKI must be enabled) and the pki object which typically contains:
- caRootsData — base64‑encoded PEM of root CA(s).
- caIntermediatesData — base64‑encoded PEM of intermediate CA(s) (optional/when needed).
- pkiCertificateSubject — fields from the signer certificate to match (e.g., email, hostname) used to validate the signing certificate against the supplied chain.
When applied in an ImagePolicy/ClusterImagePolicy the PKI block defines how CRI‑O will verify signatures for the listed scopes. Example and schema details are in the OpenShift docs and API reference. [1][2][3]
Sources:
- Red Hat Developer: Verify Cosign BYO‑PKI on OpenShift (example usage). [1]
- Red Hat / OpenShift docs (Nodes: Manage secure signatures with sigstore) — ImagePolicy rootOfTrust PKI description and example. [2]
- OKD / API reference and openshift/api v1alpha1 schema showing PolicyRootOfTrust.PKI and fields. [3]
Add missing pki field to cluster-scoped rootOfTrust schema.
The description correctly identifies PKI as a valid policyType value (line 136), but the schema is incomplete. When policyType: PKI, the specification requires a pki field containing:
caRootsData(required): base64-encoded PEM format root CA certificate(s)caIntermediatesData(optional): base64-encoded PEM format intermediate CA certificate(s)pkiCertificateSubject(required when pki is present): object with optionalemailandhostnamefields, with validation that at least one is set
Add the pki object definition to rootOfTrust properties and a corresponding x-kubernetes-validations entry enforcing its presence and exclusivity when policyType == 'PKI'.
🤖 Prompt for AI Agents
In
payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml
around lines 130-189, the rootOfTrust schema documents "PKI" as a policyType but
does not define the required pki object or validation; add a new properties.pki
object with required caRootsData (base64 PEM, format: byte, sensible maxLength
like 8192, minLength > 0), optional caIntermediatesData (format: byte, maxLength
similar), and pkiCertificateSubject object containing optional email and
hostname fields plus an x-kubernetes-validations rule that enforces at least one
of email or hostname is present when pkiCertificateSubject exists; mark pki as
present when policyType == 'PKI' and forbidden otherwise by adding an
x-kubernetes-validations entry alongside the existing publicKey/fulcio rules
that checks has(self.policyType) && self.policyType == 'PKI' ? has(self.pki) :
!has(self.pki).
Just did that, and the unit test failed and denotes the right failure message. Is there anything else that you would like for us to test/anything else I missed in this PR? |
|
Looks good, if you drop that DNM commit I'll tag the PR, thanks |
ran make update adding files to test updated and vendored api updated all generated files to reflect new feature gates Re-add hack/update-protobuf Add kube-api-server crds adding listType=atomic to excludeSubnets
Fix error in integration tests
jatinsu
force-pushed
the
OKD-featuregates
branch
from
2a40ef9 to
ec6894d
Compare
|
Just did. Thanks for your patience for this PR! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (16)
operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml (1)
104-111: Missingcontrolplanemachinesetsin enum (spec and status).The
resourcefield enum at both locations lists onlymachinesets, but the description documents "Valid values are machinesets and controlplanemachinesets". Update both occurrences to include both values and keep them synchronized.Apply this diff to fix the spec section (lines 109–110):
enum: - machinesets + - controlplanemachinesetsApply the same diff to the status section (lines 742–743):
enum: - machinesets + - controlplanemachinesetsAlso applies to: 737-744
payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (3)
33-34: Correct grammatical errors in printer column description.The description contains a double negative and subject-verb agreement issue: "When at least one of machine is not either not updated..." This was flagged in a prior review but remains unfixed.
351-351: Fix typo in status schema description.Line 351 contains a typo: "ceryExpiry" should be "certExpiry". This was previously flagged but remains unfixed.
- description: ceryExpiry contains the bundle name and the expiry + description: certExpiry contains the bundle name and the expiry
503-504: Remove double period in degradedMachineCount description.Line 504 ends with a double period ("failed.."). This was flagged in a prior review but remains unfixed.
- A node is marked degraded if applying a configuration failed.. + A node is marked degraded if applying a configuration failed.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml (1)
56-58: Fix typo in description.Line 57 contains a typo ("determins" should be "determines") and grammatical awkwardness ("the how" should be "how").
workerLatencyProfile: description: |- - workerLatencyProfile determins the how fast the kubelet is updating + workerLatencyProfile determines how fast the kubelet is updating the status and corresponding reaction of the clusterpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml (1)
78-82: Align managementState pattern with OpenShift conventions.The pattern
^(Managed|Force)$deviates from standard OpenShift operator practices. StandardmanagementStatevalues areManaged,Unmanaged, andRemoved. "Force" is not a recognized enum value in the OpenShift library-go operator package. Update the pattern to match standard upstream conventions:^(Managed|Unmanaged|Removed)$.payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml (2)
130-189: Add missingpkifield torootOfTrustschema.Line 136 documents PKI as a valid
policyTypevalue (enabled viaSigstoreImageVerificationPKIfeature gate), but the schema is incomplete. WhenpolicyType: PKI, the specification requires apkifield with properties:caRootsData(required, base64 PEM),caIntermediatesData(optional, base64 PEM), andpkiCertificateSubject(optional). Add the missingpkiobject definition to therootOfTrustproperties and correspondingx-kubernetes-validationsentry to enforce presence/exclusivity whenpolicyType == 'PKI'.
385-389: Fix inconsistent enum value casing for status condition.Line 389 has
Unknownwithout quotes, while lines 386–388 have quoted string values"True"and"False". For consistency and to avoid YAML type confusion, quote all enum values:enum: - "True" - "False" - - Unknown + - "Unknown"payload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (2)
130-189: Add missingpkifield torootOfTrustschema.Line 136 documents PKI as a valid
policyTypevalue (enabled viaSigstoreImageVerificationPKIfeature gate), but the schema is incomplete. WhenpolicyType: PKI, the specification requires apkifield with properties:caRootsData(required, base64 PEM),caIntermediatesData(optional, base64 PEM), andpkiCertificateSubject(optional). Add the missingpkiobject definition to therootOfTrustproperties and correspondingx-kubernetes-validationsentry to enforce presence/exclusivity whenpolicyType == 'PKI'.
385-389: Fix inconsistent enum value casing for status condition.Line 389 has
Unknownwithout quotes, while lines 386–388 have quoted string values"True"and"False". For consistency and to avoid YAML type confusion, quote all enum values:enum: - "True" - "False" - - Unknown + - "Unknown"config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (2)
105-144: Add missingpkifield torootOfTrustschema.Line 110 documents PKI as a valid
policyTypevalue (enabled viaSigstoreImageVerificationPKIfeature gate), but the schema is incomplete. WhenpolicyType: PKI, the specification requires apkifield with properties:caRootsData(required, base64 PEM),caIntermediatesData(optional, base64 PEM), andpkiCertificateSubject(optional). Add the missingpkiobject definition to therootOfTrustproperties and correspondingx-kubernetes-validationsentry to enforce presence/exclusivity whenpolicyType == 'PKI'.
320-326: Fix inconsistent enum value casing for status condition.Line 325 has
Unknownwithout quotes, while lines 323–324 have quoted string values"True"and"False". For consistency and to avoid YAML type confusion, quote all enum values:enum: - "True" - "False" - - Unknown + - "Unknown"config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml (2)
65-80: Resolve contradiction betweenformat: byteand plaintext PEM validation.The schema declares
format: byte(lines 69, 114, 147, 163), which per OpenAPI v3 specifies base64-encoded data (RFC 4648). However, CEL validation rules (lines 73–79, 118–124, 152–158, 167–173) check for plaintext PEM markers (-----BEGIN CERTIFICATE-----,-----END PUBLIC KEY-----, etc.). This creates an irreconcilable conflict: users must either provide plaintext PEM (violatingformat: byte) or base64-encoded PEM (violating the CEL rules that expect plaintext).Fix: Remove
format: bytefrom affected fields (fulcioCAData, rekorKeyData, publicKey.keyData, publicKey.rekorKeyData). Update descriptions to clarify the fields expect PEM-formatted certificates/keys with plaintext BEGIN/END markers. Keep the existing CEL header/footer validation rules intact.fulcioCAData: description: |- - fulcioCAData is a required field contains inline base64-encoded data for the PEM format fulcio CA. + fulcioCAData is a required field contains inline PEM-formatted fulcio CA certificate. fulcioCAData must be at most 8192 characters. - format: byte maxLength: 8192 type: stringAlso applies to: 110-125, 143-159, 163-174
385-389: Fix inconsistent enum value casing for status condition.Line 389 has
Unknownwithout quotes, while lines 386–388 have quoted string values"True"and"False". For consistency and to avoid YAML type confusion, quote all enum values:enum: - "True" - "False" - - Unknown + - "Unknown"config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml (1)
105-145: Note: This validation issue was previously flagged.The past review comment regarding policyType validation and PKI case handling remains valid. Since this is a generated CRD manifest (indicated by
zz_generated), the fix should be applied to the source generator, not this file directly.machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml (1)
94-100: Critical: Inverted DNS validation remains unfixed.The past review comment correctly identified that the CEL rule is inverted. Line 100 contains
!format.dns1123Subdomain().validate(self).hasValue()which requires the field to be invalid as a DNS subdomain, directly contradicting the field description. This will reject valid RFC 1123 subdomain values and accept invalid ones.Since this is a generated CRD manifest (
zz_generated), the fix must be applied to the source generator to remove the negation operator.
🧹 Nitpick comments (1)
features/okd_featureset_parity_test.go (1)
43-44: Redundant sort -sets.NewString().List()already returns sorted.The
List()method onsets.Stringreturns elements in sorted order. The explicitsort.Strings(missingList)call is unnecessary but harmless.if missingInOKD.Len() > 0 { missingList := missingInOKD.List() - sort.Strings(missingList)
📜 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 (52)
config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1/types_feature.go(2 hunks)config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml(2 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)example/v1/types_stable.go(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml(1 hunks)example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-TechPreviewNoUpgrade.crd.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yaml(1 hunks)example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yaml(1 hunks)features.md(1 hunks)features/features.go(23 hunks)features/okd_featureset_parity_test.go(1 hunks)machine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml(1 hunks)machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yaml(1 hunks)machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(1 hunks)operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_nodes-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- openapi/generated_openapi/zz_generated.openapi.go
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-OKD.crd.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-TechPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yaml
- config/v1/zz_generated.featuregated-crd-manifests/featuregates.config.openshift.io/AAA_ungated.yaml
- config/v1/tests/featuregates.config.openshift.io/AAA_ungated.yaml
- payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-Default.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml
- example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_nodes-OKD.crd.yaml
- example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml
- payload-manifests/crds/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.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:
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlfeatures.mdoperator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yamlexample/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yamlexample/v1/types_stable.goopenapi/openapi.jsonmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yamlexample/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlfeatures/features.gopayload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlexample/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yamlconfig/v1/types_feature.gofeatures/okd_featureset_parity_test.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yamlconfig/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yamlmachine/v1/zz_generated.crd-manifests/0000_10_control-plane-machine-set_01_controlplanemachinesets-OKD.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlpayload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlmachine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml
🧬 Code graph analysis (2)
features/features.go (2)
config/v1/types_feature.go (4)
Default(41-41)OKD(58-58)DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)features/util.go (2)
SelfManaged(36-36)Hypershift(35-35)
features/okd_featureset_parity_test.go (2)
features/features.go (1)
AllFeatureSets(21-34)config/v1/types_feature.go (3)
Default(41-41)OKD(58-58)FeatureGateAttributes(136-144)
🔇 Additional comments (28)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (1)
1-383: Verify relationship between zz_generated and payload-manifests paths.This file appears to define the same MachineConfigNode CRD as
payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml. Clarify whether these are intentional duplicates (e.g., one is auto-generated and the other is a source manifest) or if only one should be included.payload-manifests/crds/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (1)
1-10: Ensure API approval reference is current.Line 5 references
api-approved.openshift.io: https://github.com/openshift/api/pull/1453. Verify this PR is the appropriate approval for this CRD or update to reference the current PR if needed.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-SelfManagedHA-OKD.crd.yaml (1)
1-383: Verify deployment-model specific CRD variants are necessary.This file is nearly identical to File 1, differing only in the
include.release.openshift.ioannotations (self-managed-high-availability vs ibm-cloud-managed). Confirm whether:
- Separate CRD manifests per deployment model are required, or
- A single CRD with multiple deployment-model annotations would suffice.
Also clarify the relationship between zz_generated files (Files 1, 3) and payload-manifests files (Files 2, 4).
payload-manifests/crds/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml (1)
1-383: Confirm intentional duplication with File 1.This file is identical to File 1 (
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yaml). These files appear in different directory hierarchies (payload-manifests vs zz_generated). Verify whether:
- Both are needed (one may be auto-generated), or
- One should be removed to avoid maintenance burden and inconsistency drift.
example/v1/zz_generated.crd-manifests/0000_50_my-operator_01_stableconfigtypes-CustomNoUpgrade.crd.yaml (1)
333-333: LGTM: Consistent atomic list type applied.The addition of
x-kubernetes-list-type: atomictoexcludeSubnetsaligns its list semantics with the existing atomic handling on thesubnetslist (line 351). This ensures consistent atomic update behavior for both related CIDR list fields.example/v1/types_stable.go (1)
207-207: LGTM: Source marker for consistent atomic list type.The
+listType=atomicmarker onExcludeSubnetsensures CRD generation produces consistent atomic list semantics with theSubnetsfield (line 199). This maintains semantic consistency between related CIDR list fields during API updates.payload-manifests/crds/0000_10_config-operator_01_featuregates.crd.yaml (2)
82-82: LGTM: OKD added to featureSet enum.The addition of
OKDas a valid feature set value integrates the new OKD feature set into the cluster configuration framework as intended by the PR objectives.
95-96: Verify OKD transition semantics align with intended upgrade paths.The validation rule prevents OKD → Default but permits OKD → other feature sets (CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade). Given that the NoUpgrade feature sets have their own immutability rules, transitioning from OKD to them creates a one-way path.
Confirm this aligns with the intended containment hierarchy (TechPreviewNoUpgrade ⊃ OKD ⊃ Default) mentioned in PR discussions. If OKD should only allow upgrading to TechPreviewNoUpgrade (not to CustomNoUpgrade or DevPreviewNoUpgrade), tighten the rule accordingly.
openapi/openapi.json (1)
14292-14297: Verify consistency ofx-kubernetes-list-type: "atomic"across similar list definitions.The Kubernetes vendor extension
x-kubernetes-list-type: "atomic"is a valid and standard OpenAPI annotation for CRD schemas. It correctly specifies that list fields should be treated as atomic values (replaced as a whole unit during updates), which is the default behavior for arrays in Kubernetes APIs.However, verification requires access to the full file to ensure:
- All similar list fields in the affected types consistently receive this annotation.
- The openapi.json file's maintenance approach—whether it is manually edited, generated from upstream sources, or hybrid—to confirm changes propagate correctly.
- No partial or incomplete application of this extension across related schemas.
Without access to the repository, these scope and consistency checks cannot be completed.
payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml (1)
1-11: LGTM for APIServer OKD CRD.Schema, defaults, and validations look consistent. No blocking issues found.
Also applies to: 21-26, 226-425
config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml (1)
19-35: LGTM for ClusterVersion OKD CRD.OpenAPI schema and validations align with expectations.
Also applies to: 780-791
operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml (1)
1-11: Double-check parity with payload Etcd CRD.Avoid divergence between operator and payload copies.
Use the same script as in the payload Etcd CRD comment to diff these two files.
Also applies to: 22-29, 322-331
payload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yaml (1)
1-11: Ensure payload and operator Etcd CRDs are identical (prevent drift).Keep these manifests in lockstep to avoid rollout/skew issues.
Run:
#!/bin/bash # Show diffs between payload and operator Etcd OKD CRDs payload="payload-manifests/crds/0000_12_etcd_01_etcds-OKD.crd.yaml" operator="operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yaml" diff -u "$payload" "$operator" || trueAlso applies to: 22-29, 322-331
operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yaml (1)
79-83: Verify schema parity with the canonical KubeAPIServer CRD for managementState pattern.The OKD variant shows
pattern: ^(Managed|Force)$for managementState. Confirm this matches the non-OKD/canonical version to prevent API skew during upgrades.payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (1)
46-65: Sanity-check driver lists against supported platforms.metadata.name and driverType enums must reflect exactly the supported set to avoid rejecting valid objects. Verify the enumerated driver names at lines 46-65, 176-191, and 300-307 match the currently supported CSI driver set.
example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example.yaml (1)
332-333: LGTM - Consistent atomic list type annotation.Adding
x-kubernetes-list-type: atomictoexcludeSubnetsaligns with the existingsubnetsfield (line 351) and ensures consistent list replacement semantics across the CRD.example/v1/zz_generated.featuregated-crd-manifests/stableconfigtypes.example.openshift.io/Example+Example2.yaml (1)
333-334: LGTM - Consistent with Example.yaml.Same atomic list type annotation for
excludeSubnets, maintaining consistency across feature-gated CRD variants.features.md (1)
1-115: LGTM - Documentation correctly reflects OKD feature set.The table accurately shows OKD feature gates matching Default gates, with OKD columns appropriately mirroring Default across both Hypershift and SelfManagedHA cluster profiles.
features/okd_featureset_parity_test.go (1)
16-52: LGTM - Test correctly enforces OKD/Default parity.The test logic properly validates that all feature gates enabled in Default are also enabled in OKD for each cluster profile, aligning with the PR objective.
config/v1/types_feature.go (2)
56-61: LGTM - OKD FeatureSet correctly defined.The OKD constant and its inclusion in
AllFixedFeatureSetsare correct. Documentation clearly states OKD is for OKD clusters only.
74-79: XValidation correctly enforces OKD transition rules.The rule at line 78 properly prevents OKD → Default transition using empty string comparison (
self != ''), while still allowing OKD → TechPreviewNoUpgrade/DevPreviewNoUpgrade transitions as discussed in the PR thread.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-OKD.crd.yaml (1)
1-415: LGTM! Generated CRD manifest follows Kubernetes conventions with comprehensive OpenAPI v3 schema, proper cross-field validations, and correct metadata annotations for OKD feature-set support.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yaml (1)
82-96: LGTM! OKD featureset enum and transition validation are correctly implemented. The rule prevents downgrade from OKD to Default, maintaining consistency with other immutable featureset transitions.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yaml (1)
1-122: LGTM - Well-formed CRD manifest.The Scheduler CRD is properly structured with appropriate enum constraints, field validations, and subresource definitions. The OKD feature-set annotation is correctly applied.
features/features.go (1)
39-939: LGTM - Consistent OKD feature gate integration.The addition of
configv1.OKDto feature gate definitions is consistent across the file. The changes correctly extend OKD support to match the Default featureset, which aligns with the PR objectives and the parity test infeatures/okd_featureset_parity_test.go.operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yaml (1)
1-488: LGTM - Comprehensive CSI driver CRD with proper validations.The ClusterCSIDriver CRD includes well-defined platform-specific configurations and appropriate discriminator validations. The IBMCloud validation rule (lines 302-306) correctly enforces the presence of the
ibmcloudfield whendriverTypeis 'IBMCloud'.machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-OKD.crd.yaml (1)
1-544: LGTM - Well-structured Machine API CRD.The Machines CRD provides comprehensive lifecycle management with proper validation for lifecycle hooks, metadata, and provider-specific configurations. The use of
x-kubernetes-preserve-unknown-fieldsfor providerSpec appropriately accommodates platform-specific variations.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigpools-OKD.crd.yaml (1)
1-614: Verify that generated CRD was produced from source API types and ensure test coverage for feature-set parity.The
zz_generated.crd-manifests/pattern is intentional in OpenShift: these files are committed to version control as machine-generated artifacts. Verify that:
This OKD CRD was generated from the authoritative Go API types (not hand-edited) using the project's standard generation tooling (Makefile targets, controller-gen, or equivalent). The generated output should be reproducible.
Test or CI coverage exists to verify that the OKD variant of this CRD remains synchronized with the Default (and other feature-set) variants if they are intended to be identical, or that intentional differences are documented.
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-OKD.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_machineconfigurations-OKD.crd.yaml
Show resolved
Hide resolved
|
Scheduling tests matching the |
|
/test e2e-aws-ovn |
1 similar comment
|
/test e2e-aws-ovn |
|
@jatinsu: The following test failed, say
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. |
5 participants
This PR was largely bult on the work done by
@Prashanth684