CNTRLPLANE-3513: add kms health reports#2881
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@tjungblu: This pull request references CNTRLPLANE-3513 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn 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. |
📝 WalkthroughWalkthroughAdds KMS plugin health types to the config API (KMSPluginHealthStatus, KMSPluginHealthReport, APIServerEncryptionStatus.healthReports) and wires an optional v1.APIServerEncryptionStatus into operator status structs (OAuthAPIServerStatus, KubeAPIServerStatus, OpenShiftAPIServerStatus) behind KMSEncryption. Introduces CRD payload manifests for KubeAPIServer, OpenShiftAPIServer, and Authentication with OpenAPI/CEL validations (including unique (nodeName,keyId) for healthReports) and adds operator tests that accept healthy/unique reports and reject duplicates. Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Hello @tjungblu! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
config/v1/types_apiserver.go (2)
313-324: 💤 Low valueDocument maxLength constraints for optional fields.
The
KEKIdandDetailfields havemaxLength: 1024validation markers, but this constraint isn't documented in the field comments. Per API documentation guidelines, validation constraints should be documented.Consider adding length constraints to the comments:
- // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. - // This is not a cryptographic key, but a unique representation of the KEK. + // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. + // This is not a cryptographic key, but a unique representation of the KEK. + // The value must be at most 1024 characters when specified. ... - // detail contains additional error/health information; omitted when healthy + // detail contains additional error/health information; omitted when healthy. + // The value must be at most 1024 characters when specified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_apiserver.go` around lines 313 - 324, The comments for the optional fields KEKId and Detail do not document the MaxLength=1024 validation; update the field comments for KEKId and Detail to explicitly state the allowed length (e.g., "Maximum length: 1024 characters") so the validation constraint is visible in the API docs, leaving the existing kubebuilder tags (MinLength/MaxLength/optional) intact for KEKId and Detail.Source: Coding guidelines
330-338: 💤 Low valueDocument maxItems constraint for healthReports.
The
healthReportsfield hasmaxItems: 250validation but this isn't documented in the comment. Consider adding this constraint to help operators understand the limit:// healthReports contains all KMS plugin health reports for this APIServer. // When omitted, no health reports are available. // Each entry must have a unique combination of nodeName and keyId. +// The list is limited to 250 entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_apiserver.go` around lines 330 - 338, The comment for the HealthReports field does not mention the configured maximum of 250 items; update the doc comment above the HealthReports []KMSPluginHealthReport field to explicitly state the maxItems constraint (250) so operators see the limit (e.g., "Maximum of 250 entries"); keep reference to the existing uniqueness requirement and other validation notes for HealthReports and KMSPluginHealthReport unchanged.Source: Coding guidelines
payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml (1)
242-249: 💤 Low valueMinor inconsistency: enum includes empty string but minLength rejects it.
The
statusfield enum includes""(empty string) butminLength: 1prevents its use. While functionally correct (only healthy/unhealthy/error are valid), this is a documentation inconsistency. The enum value""can never pass validation.This is likely generated from the feature-gate-aware enum marker. Consider whether the empty string should be in the enum when the feature gate is enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml` around lines 242 - 249, The enum for the status field currently includes an empty string "" but minLength: 1 prevents it from ever validating; fix by either removing the "" entry from the enum on the status field (preferred) so the enum matches minLength: 1, or if the empty-string must be allowed for feature-gate behavior, change minLength to 0 to permit it; update the status enum/minLength pair accordingly and ensure any feature-gate-aware generator/config (the feature-gate-aware enum marker) is adjusted to produce the matching combination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@config/v1/types_apiserver.go`:
- Around line 313-324: The comments for the optional fields KEKId and Detail do
not document the MaxLength=1024 validation; update the field comments for KEKId
and Detail to explicitly state the allowed length (e.g., "Maximum length: 1024
characters") so the validation constraint is visible in the API docs, leaving
the existing kubebuilder tags (MinLength/MaxLength/optional) intact for KEKId
and Detail.
- Around line 330-338: The comment for the HealthReports field does not mention
the configured maximum of 250 items; update the doc comment above the
HealthReports []KMSPluginHealthReport field to explicitly state the maxItems
constraint (250) so operators see the limit (e.g., "Maximum of 250 entries");
keep reference to the existing uniqueness requirement and other validation notes
for HealthReports and KMSPluginHealthReport unchanged.
In
`@payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml`:
- Around line 242-249: The enum for the status field currently includes an empty
string "" but minLength: 1 prevents it from ever validating; fix by either
removing the "" entry from the enum on the status field (preferred) so the enum
matches minLength: 1, or if the empty-string must be allowed for feature-gate
behavior, change minLength to 0 to permit it; update the status enum/minLength
pair accordingly and ensure any feature-gate-aware generator/config (the
feature-gate-aware enum marker) is adjusted to produce the matching combination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 98ea2a13-beee-4e07-acf0-5dedda344284
⛔ Files ignored due to path filters (24)
config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (22)
config/v1/types_apiserver.gooperator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/types_authentication.gooperator/v1/types_kubeapiserver.gooperator/v1/types_openshiftapiserver.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-Default.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-OKD.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-Default.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-OKD.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
This PR adds a structured API for the kms health reports, which are currently stored as json in the operator conditions. Corresponding enhancement in openshift/enhancements#2005 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
There was a problem hiding this comment.
Looks like the changes here are only used in the operator/v1 group - any reason these changes shouldn't be placed in that group instead of this one?
There was a problem hiding this comment.
Because it is semantically correlated with the KMS and encryption configurations above.
| // +openshift:validation:FeatureGateAwareEnum:featureGate=KMSEncryption,enum="";healthy;unhealthy;error | ||
| type KMSPluginHealthStatus string | ||
|
|
||
| const ( | ||
| KMSPluginHealthStatusHealthy KMSPluginHealthStatus = "healthy" | ||
|
|
||
| KMSPluginHealthStatusUnhealthy KMSPluginHealthStatus = "unhealthy" | ||
|
|
||
| KMSPluginHealthStatusError KMSPluginHealthStatus = "error" | ||
| ) |
There was a problem hiding this comment.
Enum values should be PascalCase:
| // +openshift:validation:FeatureGateAwareEnum:featureGate=KMSEncryption,enum="";healthy;unhealthy;error | |
| type KMSPluginHealthStatus string | |
| const ( | |
| KMSPluginHealthStatusHealthy KMSPluginHealthStatus = "healthy" | |
| KMSPluginHealthStatusUnhealthy KMSPluginHealthStatus = "unhealthy" | |
| KMSPluginHealthStatusError KMSPluginHealthStatus = "error" | |
| ) | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=KMSEncryption,enum="";Healthy;Unhealthy;Error | |
| type KMSPluginHealthStatus string | |
| const ( | |
| KMSPluginHealthStatusHealthy KMSPluginHealthStatus = "Healthy" | |
| KMSPluginHealthStatusUnhealthy KMSPluginHealthStatus = "Unhealthy" | |
| KMSPluginHealthStatusError KMSPluginHealthStatus = "Error" | |
| ) |
| // status contains a health indicator for the respective KMS plugin | ||
| // The field can have three states: healthy, unhealthy, error. | ||
| // With error and unhealthy containing additional information in Detail. | ||
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=10 | ||
| // +required | ||
| Status KMSPluginHealthStatus `json:"status,omitempty"` | ||
|
|
||
| // lastChecked is a timestamp of when the probe was last checked. | ||
| // +required | ||
| LastChecked metav1.Time `json:"lastChecked,omitempty"` | ||
|
|
||
| // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. | ||
| // This is not a cryptographic key, but a unique representation of the KEK. | ||
| // +kubebuilder:validation:MinLength=0 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| KEKId *string `json:"kekId,omitempty"` | ||
|
|
||
| // detail contains additional error/health information; omitted when healthy | ||
| // +kubebuilder:validation:MinLength=0 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| Detail *string `json:"detail,omitempty"` |
There was a problem hiding this comment.
Aside from kekId, I think the fields here could be replaced by using typical status conditions (i.e a []metav1.Condition).
For example, I think this could probably all be wrapped into a single condition.
Happy path:
type: Healthy
status: True
reason: AsExpected
lastTransitionTime: ....Unhealthy scenario:
type: Healthy
status: False
reason: SomethingIsWrong
message: "error: something is wrong"
lastTransitionTime: ....While lastTransitionTime isn't necessarily as accurate as lastChecked in terms of probe state, it still signals that the status has been observed since X time and prevents unnecessary status updates to update a timestamp.
There was a problem hiding this comment.
@ibihim does that work for you to replace it?
There was a problem hiding this comment.
we would need NodeName,KeyId,KekId however - so the condition would replace only the health status, lastchecked and detail
There was a problem hiding this comment.
We do use conditions for this today and learned that it's difficult to work with. Operators end up parsing JSON field with no schema behind it. Please see for more details about the structure.
It seems that a dedicated type fits better:
-
a message isn't meant to carry structured data. It's a human readable string by convention.
-
there's no schema (or rather schema is implicit), no validation, and no generated client.
-
structured types are easier to understand and work with. With a dedicated type, someone reading the API or writing a controller can immediately see what fields exist and what their types are.
-
schema evolution seems to be simpler. if we later need to add a field a dedicated type gets a new field with proper validation markers and generated client support.
There was a problem hiding this comment.
i am pasting the conditions we use today from the kep i shared in the previous message:
status:
conditions:
- type: KMSHealthReporter_master-0
status: "True"
reason: AsExpected
message: '[{"kekID":"kek-9f2c","keyID":"2","status":"healthy","lastChecked":"2026-05-08T12:34:56Z"},{"kekID":"kek-4a17","keyID":"1","status":"healthy","lastChecked":"2026-05-08T12:34:56Z"}]'
- type: KMSHealthReporter_master-1
status: "True"
reason: AsExpected
message: '[{"kekID":"kek-9f2c","keyID":"2","status":"unhealthy","lastChecked":"2026-05-08T12:34:56Z","detail":"credential lacks decrypt permission"},{"kekID":"kek-4a17","keyID":"1","status":"healthy","lastChecked":"2026-05-08T12:34:56Z"}]'
- type: KMSHealthReporter_master-2
status: "True"
reason: AsExpected
message: '[{"keyID":"2","status":"error","lastChecked":"2026-05-08T12:34:56Z","detail":"connection refused"},{"keyID":"1","status":"error","lastChecked":"2026-05-08T12:34:56Z","detail":"connection refused"}]'
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +required | ||
| KeyId string `json:"keyId,omitempty"` |
There was a problem hiding this comment.
Does the key id follow a specific format that we can validate?
There was a problem hiding this comment.
I wouldn't make many assumptions here, this is a concatenation of some strings and integers.
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +required | ||
| NodeName string `json:"nodeName,omitempty"` |
There was a problem hiding this comment.
node names follow the DNS subdomain name format. We should use the format validation CEL library in kube for validating the input against that naming format: https://kubernetes.io/docs/reference/using-api/cel/#kubernetes-format-library
There was a problem hiding this comment.
I'm afraid this will clash with the way that the node controller reports those, I'll add the CEL though.
There was a problem hiding this comment.
actually, are you sure that this is the case?
the node metadata has no such constraints:
https://github.com/kubernetes/api/blob/master/core/v1/types.go#L7013-L7030
| // +kubebuilder:validation:MinLength=0 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| KEKId *string `json:"kekId,omitempty"` |
There was a problem hiding this comment.
What does it mean for kekId to be set to ""?
There was a problem hiding this comment.
That means there is a connection error between health monitor and kms plugin. So ideally we should expect that KMSPluginHealthStatus should be error (or unhealthy)
There was a problem hiding this comment.
@ibihim how do you intent do deal with this? are you just keeping the last value of a KEK in the conditions or are you always wiping it on error?
Otherwise we can also make this required from the start.
| // +kubebuilder:validation:MinItems=0 | ||
| // +kubebuilder:validation:MaxItems=100 | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryption,rule="self.all(x, self.exists_one(y, y.nodeName == x.nodeName && y.keyId == x.keyId))",message="each health report must have a unique nodeName/keyId combination" | ||
| // +listType=atomic |
There was a problem hiding this comment.
Any particular reason for this to be an atomic list?
There was a problem hiding this comment.
what else should it be?
| // +kubebuilder:validation:MaxItems=100 | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryption,rule="self.all(x, self.exists_one(y, y.nodeName == x.nodeName && y.keyId == x.keyId))",message="each health report must have a unique nodeName/keyId combination" | ||
| // +listType=atomic | ||
| HealthReports []KMSPluginHealthReport `json:"healthReports,omitempty"` |
There was a problem hiding this comment.
Is there a semantic difference between healthReports: [] and it being omitted?
| // +optional | ||
| // +kubebuilder:validation:MinItems=0 | ||
| // +kubebuilder:validation:MaxItems=100 | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryption,rule="self.all(x, self.exists_one(y, y.nodeName == x.nodeName && y.keyId == x.keyId))",message="each health report must have a unique nodeName/keyId combination" |
There was a problem hiding this comment.
This shouldn't need to be feature gated if it this type is only used by fields that are gated.
| EncryptionTypeKMS EncryptionType = "KMS" | ||
| ) | ||
|
|
||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=KMSEncryption,enum="";healthy;unhealthy;error |
There was a problem hiding this comment.
Doesn't need to be a feature gated enum, can just be a regular enum marker.
|
From KMS feature team POV, fields look good to me and align with what we discussed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml (1)
181-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumented uniqueness constraint not enforced across all CRD variants.
All four CRD files contain
healthReportsarrays with descriptions stating "Each entry must have a unique combination of nodeName and keyId", but none include a CEL validation rule to enforce this. Thex-kubernetes-list-type: atomicsetting does not provide uniqueness guarantees.This allows duplicate
(nodeName, keyId)entries to be persisted, violating the documented API contract. The fix should be applied to the Go type definitions inconfig/v1/types_apiserver.go(and regenerated), or directly via a CEL validation:x-kubernetes-validations: - message: "each healthReport must have a unique combination of nodeName and keyId" rule: "self.all(x, self.exists_one(y, x.nodeName == y.nodeName && x.keyId == y.keyId))"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml` around lines 181 - 239, The CRD's healthReports array documents uniqueness of (nodeName, keyId) but doesn't enforce it; add a CEL validation to the healthReports schema (or add corresponding validation tags in config/v1/types_apiserver.go and regenerate) that enforces "each entry must have a unique combination of nodeName and keyId" using a rule that checks every element has no other element with the same nodeName and keyId (attach a clear message like "each healthReport must have a unique combination of nodeName and keyId"); locate the healthReports array definition in the CRD (and the HealthReport Go type in types_apiserver.go) and add the x-kubernetes-validations/CEL rule to that schema so duplicates cannot be persisted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml`:
- Around line 181-239: The CRD's healthReports array documents uniqueness of
(nodeName, keyId) but doesn't enforce it; add a CEL validation to the
healthReports schema (or add corresponding validation tags in
config/v1/types_apiserver.go and regenerate) that enforces "each entry must have
a unique combination of nodeName and keyId" using a rule that checks every
element has no other element with the same nodeName and keyId (attach a clear
message like "each healthReport must have a unique combination of nodeName and
keyId"); locate the healthReports array definition in the CRD (and the
HealthReport Go type in types_apiserver.go) and add the
x-kubernetes-validations/CEL rule to that schema so duplicates cannot be
persisted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa05b620-e7e1-4823-bc9d-5a2d15029c03
⛔ Files ignored due to path filters (14)
config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (10)
config/v1/types_apiserver.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- config/v1/types_apiserver.go
- payload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yaml
|
@tjungblu: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
6 participants
This PR adds a structured API for the kms health reports, which are currently stored as json in the operator conditions.
Corresponding enhancement in openshift/enhancements#2005