CNTRLPLANE-3234: health reporter writer#2318
Open
ibihim wants to merge 6 commits into
Open
Conversation
|
@ibihim: This pull request references CNTRLPLANE-3234 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 task 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
WalkthroughKMS health probe results are now converted into ChangesKMS health status publishing
Sequence Diagram(s)sequenceDiagram
participant "Config.Run" as ConfigRun
participant prober
participant buildEncryptionStatus
participant writeStatus
participant "KubeAPIServer.status.encryptionStatus" as KubeAPIServerStatus
ConfigRun->>prober: probe KMSv2 plugins
prober-->>ConfigRun: pluginHealthReport entries
ConfigRun->>buildEncryptionStatus: build KMSEncryptionStatus
buildEncryptionStatus-->>ConfigRun: KMSEncryptionStatusApplyConfiguration
ConfigRun->>writeStatus: publish status
writeStatus->>KubeAPIServerStatus: apply status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ibihim
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@go.mod`:
- Around line 144-145: Remove the replace directive that maps
github.com/openshift/client-go to the local filesystem path
/home/ibihim/go/src/github.com/openshift/client-go-worktrees/health-reporter-regen-api,
as this absolute path will not exist on other machines or CI/CD runners and will
cause build failures. If the regenerated client-go types are not yet available
in a published version, first push those types to the upstream
openshift/client-go repository, then update the github.com/openshift/client-go
version constraint in go.mod to reference the published version instead of the
local path.
- Line 21: The github.com/openshift/api dependency is using a pseudo-version
(v0.0.0-20260618083218-a3c8dea7f8bc) which is an unreleased development commit
and violates supply chain security guidelines. Replace this pseudo-version with
a stable released version of openshift/api that includes the
KMSPluginHealthStatus enum used in writer.go. If a released version is not yet
available, document and justify why the unreleased pseudo-version is necessary
as an exception to the security policy.
In `@pkg/operator/encryption/kms/health/cmd_test.go`:
- Around line 131-151: Replace the context.WithCancel call at the beginning of
the test with context.WithTimeout to add a timeout bound. Instead of relying
solely on the writeStatus callback to cancel the context, use a timeout-bound
context (such as a reasonable test timeout like 5 seconds) so that if
writeStatus is never reached and cancel() is never called, the test will fail
fast due to context timeout rather than hanging indefinitely.
In `@pkg/operator/encryption/kms/health/cmd.go`:
- Around line 135-139: Add a length validation check in the validate() method to
ensure the reporterID does not exceed Kubernetes' 128-character fieldManager
limit. The reporterID is constructed by concatenating "kms-health-reporter-" (20
characters) with o.NodeName, so add a check after the existing NodeName empty
validation that ensures o.NodeName is not longer than 108 characters (128 minus
20). Return an appropriate error if this validation fails to prevent repeated
write failures at runtime when the fieldManager length exceeds the Kubernetes
API limit.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ffa0f75-70ab-4860-83e2-6fffefb9f4fc
⛔ Files ignored due to path filters (65)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clientcredentialconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clientsecretsecretreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/customtlsprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalclaimssource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourceauthentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourcecertificateauthorityconfigmapreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourcepredicate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourcetls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/networkobservabilityspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/networkspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/oidcprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/sourcedclaimmapping.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/sourceurl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tlsprofilespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultapproleauthentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultkmspluginconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaulttlsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/alertmanagercustomconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/kubestatemetricsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/kubestatemetricsresourcelabels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexporterconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/retention.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/thanosquerierconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/thanosquerierrequestloggingconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/etcdspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmsencryptionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspluginhealthreport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kubeapiserverstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/oauthapiserverstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/openshiftapiserverstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/operator/encryption/kms/health/cmd.gopkg/operator/encryption/kms/health/cmd_test.gopkg/operator/encryption/kms/health/prober.gopkg/operator/encryption/kms/health/writer.gopkg/operator/encryption/kms/health/writer_test.go
ibihim
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ardaguclu
reviewed
Jun 19, 2026
ibihim
force-pushed
the
CNTRLPLANE-3234-health-reporter-writer
branch
from
cb0904f to
38f7db2
Compare
ibihim
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
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 `@go.mod`:
- Around line 21-23: The three github.com/openshift dependencies (api,
build-machinery-go, and client-go) on lines 21-23 are currently pinned to
pseudo-versions (v0.0.0-YYYYMMDD format) which represent development commits
rather than stable releases, violating supply-chain security guidelines. To fix
this, either upgrade each of these three dependencies to their latest stable
released versions by updating the version strings in go.mod to actual release
versions (e.g., v1.x.y), or add a clear comment in the pull request description
explaining the justification for keeping pseudo-versions (such as awaiting an
upstream stable release or requiring an unreleased bug fix).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00c0f63a-8803-4e7e-81a2-687c392838a8
⛔ Files ignored due to path filters (65)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clientcredentialconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clientsecretsecretreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/customtlsprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalclaimssource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourceauthentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourcecertificateauthorityconfigmapreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourcepredicate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/externalsourcetls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/networkobservabilityspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/networkspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/oidcprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/sourcedclaimmapping.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/sourceurl.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tlsprofilespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultapproleauthentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultkmspluginconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaulttlsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/alertmanagercustomconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/kubestatemetricsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/kubestatemetricsresourcelabels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexporterconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/retention.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/thanosquerierconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/thanosquerierrequestloggingconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/etcdspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmsencryptionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspluginhealthreport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kubeapiserverstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/oauthapiserverstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/openshiftapiserverstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/operator/encryption/kms/health/cmd.gopkg/operator/encryption/kms/health/cmd_test.gopkg/operator/encryption/kms/health/prober.gopkg/operator/encryption/kms/health/writer.gopkg/operator/encryption/kms/health/writer_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/encryption/kms/health/cmd_test.go
- pkg/operator/encryption/kms/health/writer.go
- pkg/operator/encryption/kms/health/prober.go
- pkg/operator/encryption/kms/health/writer_test.go
- pkg/operator/encryption/kms/health/cmd.go
ibihim
force-pushed
the
CNTRLPLANE-3234-health-reporter-writer
branch
from
38f7db2 to
1d303cb
Compare
ibihim
changed the title
openshift-ci
Bot
added
needs-rebase
Adds a health report writer. The construction is a bit awkward, but the lib knows how to create the healthreport-to-be-applied-config and the individual operator knows where to apply it.
ibihim
force-pushed
the
CNTRLPLANE-3234-health-reporter-writer
branch
from
496f469 to
ca23cdf
Compare
openshift-ci
Bot
removed
the
needs-rebase
ardaguclu
reviewed
Jun 25, 2026
| ) | ||
|
|
||
| // applyOpts ensures that we are the dominant fieldManager. | ||
| func applyOpts(fieldManager string) metav1.ApplyOptions { |
Member
There was a problem hiding this comment.
Do we really need this just for Force?
Contributor
Author
There was a problem hiding this comment.
no, we could remove it.
ardaguclu
reviewed
Jun 25, 2026
| return fmt.Errorf("--node-name is required") | ||
| } | ||
| if fieldManager := fieldManagerPrefix + o.NodeName; len(fieldManager) > metav1validation.FieldManagerMaxLength { | ||
| return fmt.Errorf("--node-name too long: reporter identity %q is %d chars, exceeds the %d-char fieldManager limit", fieldManager, len(fieldManager), metav1validation.FieldManagerMaxLength) |
Member
There was a problem hiding this comment.
Can we really hit this error?.
Contributor
There was a problem hiding this comment.
how would you even get out of this situation as an admin? rename your nodes?
Contributor
There was a problem hiding this comment.
with the prefix we have like 100 characters left, I think it's OK. We can also make the prefix a bit shorter?
Member
There was a problem hiding this comment.
Maybe we can just continue as is and allow kube-apiserver errors out?
Contributor
There was a problem hiding this comment.
I think he has a case with this, but we probably need a different solution here
checkout the max length we can have per node:
https://github.com/kubernetes/apimachinery/blob/master/pkg/api/validate/content/dns.go#L56-L57
Member
There was a problem hiding this comment.
I agree with Thomas. If it is possible, maybe we can use something else (uuid, hash, etc.) to remove that check. I think, other than this thread, this PR is ready to merge.
Contributor
There was a problem hiding this comment.
we can do that as a follow-up. This is probably a bit bigger because we have to rename the cmdline and add this in the lifecycle
Member
There was a problem hiding this comment.
What about cutting the static length from node name, if it exceeds?
Contributor
There was a problem hiding this comment.
you'd have to cut the unique bits from the back and hope they don't have a subdomain per node, as Krzys says
| // Unique name per plugin so the gRPC client's KMS operation metrics | ||
| // don't merge both plugins into one series. | ||
| service, err := k8senvelopekmsv2.NewGRPCService(ctx, socket, Subcommand+"-"+keyID, timeout) | ||
| service, err := k8senvelopekmsv2.NewGRPCService(ctx, socket, fieldManagerPrefix+keyID, timeout) |
Member
There was a problem hiding this comment.
Suggested change
| service, err := k8senvelopekmsv2.NewGRPCService(ctx, socket, fieldManagerPrefix+keyID, timeout) | |
| service, err := k8senvelopekmsv2.NewGRPCService(ctx, socket, fmt.Sprintf("%s-%s", Subcommand, keyID), timeout) |
Contributor
There was a problem hiding this comment.
you can probably even leave this empty, nobody is going to look at the metric this string generates
Contributor
There was a problem hiding this comment.
OR just use the Subcommand on its own, I guess the metrics can then be sliced by node/podip or so which you get for free by prometheus scraper
| // NewAuthenticationWriter writes into Authentication/cluster at | ||
| // .status.oauthAPIServer.encryptionStatus. The auth operator manages the | ||
| // oauth-apiserver. There is an extra hop the other two operators don't have. | ||
| func NewAuthenticationWriter(restConfig *rest.Config, fieldManager string) (health.EncryptionStatusWriter, error) { |
ardaguclu
reviewed
Jun 25, 2026
| require.Equal(t, want, have) | ||
| } | ||
|
|
||
| func TestMapStatus(t *testing.T) { |
ardaguclu
reviewed
Jun 25, 2026
|
|
||
| func TestBuildEncryptionStatus(t *testing.T) { | ||
| // Fixed UTC time dodges Go's monotonic clock and timezone drift. | ||
| checked := time.Unix(0, 0).UTC() |
Member
There was a problem hiding this comment.
There is a fakeclock, or something like this. Maybe we can use it
ardaguclu
reviewed
Jun 25, 2026
|
|
||
| operatorClient, err := o.newOperatorClient(restCfg) | ||
| // fieldManager is the per-node ownership identity. | ||
| fieldManager := fieldManagerPrefix + o.NodeName |
Member
There was a problem hiding this comment.
In another place we use fieldManagerPrefix + keyID but here we use fieldManagerPrefix + o.NodeName. Isn't this a discrepancy?
Contributor
Author
There was a problem hiding this comment.
+ o.NodeName distinguishes the 3 health reporters in the KAS setup. So this will be unique per writer.
+ keyID is to distinguish individual sockets within a node. This was the case in creating the service, but I dropped it to just the Subcommand according to #2318 (comment).
tjungblu
reviewed
Jun 25, 2026
| WithStatus(mapStatus(r.Status)). | ||
| WithLastCheckedTime(metav1.NewTime(r.LastChecked)) | ||
|
|
||
| // kekId/detail have MinLength=1; setting "" would fail validation. |
Contributor
There was a problem hiding this comment.
for kekId I think the behavior is correct, because we want to retain the existing value, even when we transiently can't connect to the plugin.
The Detail, I'm not so sure. What do you think?
Contributor
Author
There was a problem hiding this comment.
AFAIU this is an alternating logic...
KekID would be empty on an error.
Detail would be empty without an error.
tjungblu
reviewed
Jun 25, 2026
|
|
||
| // mapStatus defaults to Error so an unknown value never becomes an empty, | ||
| // invalid enum. | ||
| func mapStatus(s string) operatorv1.KMSPluginHealthStatus { |
Contributor
There was a problem hiding this comment.
any reason not to directly return the operator constant from pluginHealthReport?
as in, you probably can just avoid that switch and put it directly into the prober logic
Contributor
Author
There was a problem hiding this comment.
lol. This code goes through its third iteration at this point... I seem to have lost the eye for detail.
Contributor
Author
There was a problem hiding this comment.
Do we need the "pluginHealthReport" struct at all, we could use the API directly, no?
Drop intermediate struct and go immediately for the SSA read config. Address some other comments.
ibihim
commented
Jun 25, 2026
| // EncryptionStatusWriter is capable of applying the | ||
| // KMSEncryptionStatusApplyConfiguration at the correct place in the operator's | ||
| // status. | ||
| type EncryptionStatusWriter func(ctx context.Context, status *applyoperatorv1.KMSEncryptionStatusApplyConfiguration) error |
Contributor
Author
There was a problem hiding this comment.
Add this point, we could move this to cmd.go. It is small enough.
If cmd.go is considered too big, we could move out the options logic into its own file.
Member
There was a problem hiding this comment.
Just an idea; Maybe we can move functions under writers/writers.go to here?
Contributor
Author
There was a problem hiding this comment.
I liked the idea to have them as something distinct, like they would be "already on the operator side". But if you like I could move them here.
Member
There was a problem hiding this comment.
Not needed. That works for me. Thank you
Member
|
@ibihim the task that wiring of health reporter to the operators has been finished. Now you can test this PR on operators via fake bump. So that we can verify that changes here work. |
Member
|
CI Job in fake PR proves that health of kms plugins are reported; |
ardaguclu
reviewed
Jun 30, 2026
| return fmt.Errorf("--node-name is required") | ||
| } | ||
| if fieldManager := fmt.Sprintf("%s-%s", Subcommand, o.NodeName); len(fieldManager) > metav1validation.FieldManagerMaxLength { | ||
| return fmt.Errorf("--node-name too long: reporter identity %q is %d chars, exceeds the %d-char fieldManager limit", fieldManager, len(fieldManager), metav1validation.FieldManagerMaxLength) |
Member
There was a problem hiding this comment.
This one is probably the only part that requires a change.
ibihim
force-pushed
the
CNTRLPLANE-3234-health-reporter-writer
branch
2 times, most recently
from
dcfc42c to
df08a04
Compare
ardaguclu
reviewed
Jul 1, 2026
|
|
||
| // NewKubeAPIServerWriter writes into KubeAPIServer/cluster at | ||
| // .status.encryptionStatus. | ||
| func NewKubeAPIServerWriter(restConfig *rest.Config, nodeName string) (health.EncryptionStatusWriter, error) { |
Member
There was a problem hiding this comment.
In the previous version, writers were consistent. They just accept the field manager and set it accordingly. Now we leak the field manager extraction into the writer and this makes them inconsistent (some hardcodes, some send request to API Server). In my opinion, it would be better to return back to previous version by passing field manager.
Contributor
Author
There was a problem hiding this comment.
Honestly, I thought this is my master piece :D The operators themselves know best what to put in there, but based on the discussion in the other comment, we can keep it as is.
| // The writer owns its fieldManager: it turns the node name into whatever | ||
| // ownership identity its status needs (a per-node UID for the kube-apiserver, | ||
| // a constant for the single-reporter operators). | ||
| writeStatus, err := o.newWriter(restCfg, o.NodeName) |
Member
There was a problem hiding this comment.
Can't we get UID here and pass it as field manager to newWriter?. Why shouldn't we use UID for aggregated apiservers?.
Contributor
There was a problem hiding this comment.
what if we do a simpler step and just set the --node-name argument as the node.UUID instead of the node.Name from the lifecycle.
We can rename the argument later. I don't really think we should be querying node objects from here just to resolve the name.
Contributor
Author
There was a problem hiding this comment.
Yeah, I must admit, that querying node-objects is not optimal, but I didn't want to offload it to somewhere else, but we can do so. Maybe the lifecycle has it at hand already.
Contributor
Author
There was a problem hiding this comment.
I can drop the comment and we "interpret" the node-name flag creatively as something unique within bounds like an UID?
I might adjust the description for the flag, but not sure if it is worth changing the flag itself, maybe? Because of tests? WDYT?
ibihim
force-pushed
the
CNTRLPLANE-3234-health-reporter-writer
branch
from
df08a04 to
6f33b49
Compare
Contributor
|
/retest |
Contributor
|
/approve I have a bit of time at my hands to figure out the nodename hashing |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ibihim, tjungblu 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 |
Contributor
|
@ibihim: all tests passed! 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
4 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.
What
Add health report status writer.
Why
We need to write the status into the status of each individual (aggregated) apiserver's operator status for access to KMSv2 controllers.
Notes
Check usage pattern at openshift/cluster-authentication-operator#929.
Summary by CodeRabbit
New Features
Bug Fixes
Tests