WIP: NO-JIRA: add kms plugin status#2850
Open
tjungblu wants to merge 1 commit into
Open
Conversation
Contributor
|
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 explicitly references no jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds KMS encryption status reporting to the APIServer resource. It introduces new Go types in config/v1/types_apiserver.go: a KMSPluginHealthStatus enum, a KMSPluginHealthReport struct for per-node probe details, a KMSPluginRotationStatus struct for KEK lifecycle timestamps, and an APIServerEncryptionStatus aggregation. APIServerStatus gains an EncryptionStatus field under the KMSEncryption feature gate. The APIServer CRD schemas (CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade) are updated to validate status.encryptionStatus.healthReports (atomic, maxItems: 100) and status.encryptionStatus.keyRotationStatus (atomic, maxItems: 3). 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/hold
Contributor
|
Hello @tjungblu! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XL
Contributor
|
[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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@config/v1/types_apiserver.go`:
- Around line 336-339: APIServerStatus.EncryptionStatus is encoded with
omitempty but missing the +optional marker and omitted-field documentation;
update the APIServerStatus type to add a `+optional` comment above the
EncryptionStatus field and a short doc comment describing the omitted behavior
(e.g., that when EncryptionStatus is omitted the server's encryption state is
unknown/default and consumers should treat it as not set), referencing the
EncryptionStatus field and its type APIServerEncryptionStatus so readers can
find the expected values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 83c30676-7543-40be-aa25-54d34803f553
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (4)
config/v1/types_apiserver.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
tjungblu
commented
May 20, 2026
tjungblu
commented
May 20, 2026
tjungblu
commented
May 20, 2026
| // +openshift:enable:FeatureGate=KMSEncryption | ||
| type APIServerEncryptionStatus struct { | ||
| // HealthReports contains all KMS plugin health reports for this APIServer. | ||
| // +listType=atomic |
Contributor
Author
There was a problem hiding this comment.
could also be a map over (KeyId, NodeName), not sure we have support for composite keys here
Contributor
Author
There was a problem hiding this comment.
or alternatively only KeyId, that way we have everything aggregated nicely for the key rotation feature
Contributor
Author
There was a problem hiding this comment.
in the current conditions based design it is actually a map type over the NodeName.
tjungblu
force-pushed
the
kms_internal_status
branch
from
640b0f1 to
dd92146
Compare
openshift-ci
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
config/v1/types_apiserver.go (2)
281-311: 💤 Low valueDocument MaxLength constraints in field comments.
Per coding guidelines,
+kubebuilder:validation:MaxLengthmarkers should be documented. FieldskeyId(2048 chars),nodeName(1024 chars), andkekId(1024 chars) have length constraints that aren't mentioned in their comments. This helps API consumers understand validation limits.🤖 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 281 - 311, The struct KMSPluginHealthReport has +kubebuilder:validation:MaxLength annotations but the field comments don't mention those limits; update the comments for KeyId, NodeName, and KEKId to include their max lengths (KeyId: 2048, NodeName: 1024, KEKId: 1024) so API consumers see the validation constraints (edit the comments above the KeyId, NodeName and KEKId fields in KMSPluginHealthReport to append the respective max-length information).
337-339: ⚖️ Poor tradeoffConsider adding
MaxItemsconstraint onhealthReports.Unlike
keyRotationStatus(capped at 3),healthReportshas no upper bound. In large clusters with many nodes, this list could grow significantly, potentially bloating the status object. Consider adding a reasonable limit (e.g., based on max supported nodes × plugins).🤖 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 337 - 339, The HealthReports slice (HealthReports []KMSPluginHealthReport) currently has no upper bound and can grow unbounded; add a MaxItems constraint (kubebuilder/k8s validation tag) to cap the list size (choose a sensible limit informed by max supported nodes × plugins, e.g., similar rationale to keyRotationStatus which is capped at 3) so the APIServer status cannot be bloated; update the struct tag/comment for HealthReports (and document the chosen limit) to use +kubebuilder:validation:MaxItems=<N> referencing HealthReports and KMSPluginHealthReport.
🤖 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.
Inline comments:
In `@config/v1/types_apiserver.go`:
- Around line 318-319: Fix the garbled comment text "DiscovdiscoveryTimeeryTime"
by replacing it with a clear identifier such as "DiscoveryTime" (or
"discoveryTime" if following lowerCamelCase for field comments) so the comment
reads: "DiscoveryTime contains the time when the operator has detected a change
in the status key ID, determined by all nodes agreeing on the same KEKid or when
a key has been initially discovered." Update the comment near the corresponding
type/field in types_apiserver.go to match the project's comment casing
conventions.
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml`:
- Around line 741-746: The description for the discoveryTime field contains a
duplicated-typo "DiscovdiscoveryTimeeryTime"; update the discoveryTime
description to fix the typo and make the sentence readable (e.g., start with
"DiscoveryTime contains the time..." and keep the rest of the text about
operator detection of keyId/KEKid agreement and use for key convergence/cache
invalidation). Ensure you edit the description value belonging to the
discoveryTime field in the CRD so the typo is removed and punctuation/spacing
remain correct.
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml`:
- Around line 417-421: The description for the CRD field discoveryTime contains
a garbled word "DiscovdiscoveryTimeeryTime"; update the YAML description to a
correct sentence (e.g., "discoveryTime contains the time when the operator has
detected a change in the status keyId...") and also fix the originating Go
source comment/const that generated this CRD (the struct/field comment for
DiscoveryTime) so the corrected text persists when CRDs are regenerated; after
fixing the Go comment, regenerate the CRD manifests to ensure the cleaned
description replaces the garbled text.
- Around line 399-403: The schema for the "status" field in the Default and OKD
variants is missing the enum constraint present in other variants; update the
CRD definitions for the "status" property (the "status" key under the respective
spec/status sections for the Default and OKD variants) to include the same enum:
["", "healthy", "unhealthy"] as used by
CustomNoUpgrade/DevPreviewNoUpgrade/TechPreviewNoUpgrade, or alternatively
remove the encryptionStatus field consistently from the non-KMS-enabled
variants—ensure the change is applied to the "status" property in both Default
and OKD manifest blocks (matching the exact property name "status" and keeping
maxLength/type consistent).
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml`:
- Around line 741-746: Fix the typo in the CRD description for the discoveryTime
field: replace the garbled "DiscovdiscoveryTimeeryTime" with a correct, single
phrase such as "discoveryTime contains the time when the operator has detected a
change in the status keyId..." in the description of the discoveryTime property
so the text reads clearly; update the description under the discoveryTime schema
entry (symbol: discoveryTime) to the corrected string.
In `@payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml`:
- Around line 417-421: The description for the discoveryTime field contains a
duplicated typo "DiscovdiscoveryTimeeryTime"; update the description string for
the discoveryTime field (the YAML property named discoveryTime) to a single
correct word, e.g., "DiscoveryTime contains the time when the operator..." so
the sentence reads normally and remove the duplicated fragment.
- Around line 399-403: The CRD schema for the apiservers-OKD resource is missing
the enum constraint on the status property; update the status property in the
CRD (the status field under the schema block) to include the same enum array
used in the Default/feature-gated CRD so validation is consistent—add an enum:
[...] entry with the exact same status values as the Default CRD's status enum.
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml`:
- Around line 741-746: The description for the CRD field discoveryTime contains
a duplicated/garbled word "DiscovdiscoveryTimeeryTime"; update the description
of the discoveryTime field to a clean sentence (e.g., "discoveryTime contains
the time when the operator has detected a change in the status keyId..."),
preserving the rest of the text, formatting and the date-time format; modify the
description under the discoveryTime property in the CRD (field name:
discoveryTime) to remove the typo and ensure it reads correctly.
---
Nitpick comments:
In `@config/v1/types_apiserver.go`:
- Around line 281-311: The struct KMSPluginHealthReport has
+kubebuilder:validation:MaxLength annotations but the field comments don't
mention those limits; update the comments for KeyId, NodeName, and KEKId to
include their max lengths (KeyId: 2048, NodeName: 1024, KEKId: 1024) so API
consumers see the validation constraints (edit the comments above the KeyId,
NodeName and KEKId fields in KMSPluginHealthReport to append the respective
max-length information).
- Around line 337-339: The HealthReports slice (HealthReports
[]KMSPluginHealthReport) currently has no upper bound and can grow unbounded;
add a MaxItems constraint (kubebuilder/k8s validation tag) to cap the list size
(choose a sensible limit informed by max supported nodes × plugins, e.g.,
similar rationale to keyRotationStatus which is capped at 3) so the APIServer
status cannot be bloated; update the struct tag/comment for HealthReports (and
document the chosen limit) to use +kubebuilder:validation:MaxItems=<N>
referencing HealthReports and KMSPluginHealthReport.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cbb7b355-d5a7-4a82-8636-c48a980feb40
⛔ Files ignored due to path filters (11)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (6)
config/v1/types_apiserver.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
tjungblu
force-pushed
the
kms_internal_status
branch
from
dd92146 to
363baa7
Compare
openshift-ci
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (3)
config/v1/types_apiserver.go (3)
281-313: ⚡ Quick winMissing compatibility annotation and length constraint documentation.
This is a new stable v1 API type but lacks the required
+openshift:compatibility-gen:level=1annotation. Additionally, per coding guidelines,+kubebuilder:validation:MaxLengthmarkers should be documented in field comments.Suggested changes
+// +openshift:compatibility-gen:level=1 type KMSPluginHealthReport struct { - // keyId is the encryption-key-secret id (kms-{keyId}.sock), a unique identifier of the plugin on that node. - // This is not a cryptographic key used to encrypt/decrypt any resources. + // keyId is the encryption-key-secret id (kms-{keyId}.sock), a unique identifier of the plugin on that node. + // This is not a cryptographic key used to encrypt/decrypt any resources. + // The value must be at most 512 characters. // +kubebuilder:validation:MaxLength=512 // +required KeyId string `json:"keyId"` - // nodeName is the name of the node this instance of the plugin runs on. - // The combination of NodeName/KeyId makes this health report unique. + // nodeName is the name of the node this instance of the plugin runs on. + // The combination of NodeName/KeyId makes this health report unique. + // The value must be at most 512 characters. // +kubebuilder:validation:MaxLength=512 // +required NodeName string `json:"nodeName"`As per coding guidelines, "New Stable APIs (v1) must include the kubebuilder annotation
+openshift:compatibility-gen:level=1" and "Documentation for+kubebuilder:validation:MaxLengthmarkers must document character length constraints."🤖 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 281 - 313, Add the required stability annotation and document the MaxLength constraints: add the kubebuilder compatibility marker `+openshift:compatibility-gen:level=1` above the KMSPluginHealthReport type declaration, and update the field comments for KeyId, NodeName, KEKId, and Detail to explicitly state the character length limits referenced by their `+kubebuilder:validation:MaxLength` tags (e.g., "Maximum X characters.") so the MaxLength markers are documented in the field comments.
337-347: ⚡ Quick winMissing
+optionalmarkers and compatibility annotation.Both
HealthReportsandKeyRotationStatusfields useomitemptybut lack explicit+optionalmarkers and omission behavior documentation. The struct also needs the+openshift:compatibility-gen:level=1annotation for v1 stable APIs.Suggested changes
+// +openshift:compatibility-gen:level=1 type APIServerEncryptionStatus struct { // healthReports contains all KMS plugin health reports for this APIServer. + // When omitted, no health reports are available. + // +optional // +listType=atomic HealthReports []KMSPluginHealthReport `json:"healthReports,omitempty"` // keyRotationStatus contains the status of the last three key rotations that were running. - // + // When omitted, no key rotations have been recorded. + // The list is limited to the 3 most recent rotation records. + // +optional // +kubebuilder:validation:MaxItems=3 // +listType=atomic KeyRotationStatus []KMSPluginRotationStatus `json:"keyRotationStatus,omitempty"` }As per coding guidelines, "Documentation for
+optionalfields must explain the behavior when the field is omitted" and "Documentation for+kubebuilder:validation:MaxItemsmarkers must document item count ranges."🤖 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 337 - 347, Add the missing API docs and markers to APIServerEncryptionStatus: annotate the struct with +openshift:compatibility-gen:level=1, add +optional above both HealthReports and KeyRotationStatus, and update their doc comments to explain omission behavior (what happens when the field is nil/omitted) and to document the allowed item count range for KeyRotationStatus in relation to +kubebuilder:validation:MaxItems=3; locate the APIServerEncryptionStatus type and adjust the comments for HealthReports and KeyRotationStatus accordingly.
315-335: ⚡ Quick winMissing validation markers on
KEKIdand compatibility annotation.
KEKIdlacks+requiredmarker and+kubebuilder:validation:MaxLengthconstraint. For consistency withKMSPluginHealthReport.KEKId(which hasMaxLength=1024), consider adding similar validation.Missing
+openshift:compatibility-gen:level=1for this v1 stable API type.
DiscoveryTimeis marked+optionalbut doesn't explain the behavior when omitted.Suggested changes
+// +openshift:compatibility-gen:level=1 type KMSPluginRotationStatus struct { // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. // This id can change externally and tells OpenShift when to trigger a migration of the configured resources. + // +kubebuilder:validation:MaxLength=1024 + // +required KEKId string `json:"kekId"` // discoveryTime contains the time when the operator has detected a change in the status keyId, this // is determined by all nodes agreeing on the same KEKid or when a key has been initially discovered. // This can be used to give additional time for key convergence or cache invalidation before a migration is started. + // When omitted, the key discovery time has not yet been recorded. // +optional DiscoveryTime *metav1.Time `json:"discoveryTime,omitempty"`As per coding guidelines, "Documentation for
+optionalfields must explain the behavior when the field is omitted."🤖 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 315 - 335, KMSPluginRotationStatus is missing validation and compatibility annotations: add a +required marker and a +kubebuilder:validation:MaxLength=1024 tag on the KEKId field (matching KMSPluginHealthReport.KEKId), add the +openshift:compatibility-gen:level=1 annotation above the KMSPluginRotationStatus type, and update the DiscoveryTime field comment to explain behavior when omitted (e.g., that omission means no KEK change has been observed or that discovery time is unknown and will be set when a change is detected).
🤖 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 281-313: Add the required stability annotation and document the
MaxLength constraints: add the kubebuilder compatibility marker
`+openshift:compatibility-gen:level=1` above the KMSPluginHealthReport type
declaration, and update the field comments for KeyId, NodeName, KEKId, and
Detail to explicitly state the character length limits referenced by their
`+kubebuilder:validation:MaxLength` tags (e.g., "Maximum X characters.") so the
MaxLength markers are documented in the field comments.
- Around line 337-347: Add the missing API docs and markers to
APIServerEncryptionStatus: annotate the struct with
+openshift:compatibility-gen:level=1, add +optional above both HealthReports and
KeyRotationStatus, and update their doc comments to explain omission behavior
(what happens when the field is nil/omitted) and to document the allowed item
count range for KeyRotationStatus in relation to
+kubebuilder:validation:MaxItems=3; locate the APIServerEncryptionStatus type
and adjust the comments for HealthReports and KeyRotationStatus accordingly.
- Around line 315-335: KMSPluginRotationStatus is missing validation and
compatibility annotations: add a +required marker and a
+kubebuilder:validation:MaxLength=1024 tag on the KEKId field (matching
KMSPluginHealthReport.KEKId), add the +openshift:compatibility-gen:level=1
annotation above the KMSPluginRotationStatus type, and update the DiscoveryTime
field comment to explain behavior when omitted (e.g., that omission means no KEK
change has been observed or that discovery time is unknown and will be set when
a change is detected).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a879ebcf-cdc3-42be-83b3-29bd470dfc1d
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (4)
config/v1/types_apiserver.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
tjungblu
force-pushed
the
kms_internal_status
branch
2 times, most recently
from
74b0656 to
a055c87
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@config/v1/types_apiserver.go`:
- Around line 270-303: Update the documentation for the KMSPluginHealthStatus
type and the Status field on KMSPluginHealthReport to explicitly list each
allowed enum value and its meaning ("" (empty string) — allowed when the
KMSEncryption feature gate is disabled or not set; "healthy" — plugin is
operational and responding; "unhealthy" — plugin is present but not functioning
correctly; "error" — plugin returned an error state), and also document the
kubebuilder length constraints on the Status field (MinLength=1, MaxLength=10)
so the API contract is self-contained; make these changes in the comments above
the KMSPluginHealthStatus type declaration and the Status field in
KMSPluginHealthReport and reference the featureGate=KMSEncryption behavior
described by the FeatureGateAwareEnum marker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cf242cfa-4792-4dd0-bd6f-dfe1cd336e70
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (4)
config/v1/types_apiserver.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
tjungblu
force-pushed
the
kms_internal_status
branch
2 times, most recently
from
00e61ed to
8ae83ee
Compare
This API allows us to track the plugin health status and key rotation details much better than using status conditions. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu
force-pushed
the
kms_internal_status
branch
from
8ae83ee to
a0df76f
Compare
openshift-ci
Bot
added
size/XXL
Contributor
|
@tjungblu: The following tests 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This API allows us to track the plugin health status and key rotation details much better than using status conditions.
work in progress for the time being...
/hold