[WIP] CNTRLPLANE-3234: health reporter writer#2318
Open
ibihim wants to merge 3 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
WalkthroughThe PR introduces an ChangesKMS Health Status Writer Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ibihim 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 |
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
| github.com/opencontainers/go-digest v1.0.0 | ||
| github.com/opencontainers/selinux v1.13.0 | ||
| github.com/openshift/api v0.0.0-20260521125114-09730f85d883 | ||
| github.com/openshift/api v0.0.0-20260618083218-a3c8dea7f8bc |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is there a released version of github.com/openshift/api with KMSPluginHealthStatus enum?
💡 Result:
No—based on the current openshift/api repository contents, I did not find a released/defined version of github.com/openshift/api that contains the KMSPluginHealthStatus enum. In the surfaced openshift/api changes, the KMS-related API work focuses on KMS encryption/configuration validation (vault KMS plugin configuration, including fields like kmsPluginImage, vaultAddress, approleSecret, etc.) rather than a KMSPluginHealthStatus enum being added or referenced [1][2][3]. One of the related efforts includes adding a KMS “health” subcommand in cluster-openshift-apiserver-operator, but that is in a different repository and does not indicate a KMSPluginHealthStatus enum within openshift/api [4]. Caveat: this conclusion is based on search results for the exact symbol name “KMSPluginHealthStatus” within openshift/api; if the enum exists under a different name (or in a generated protobuf/types file not indexed by the search), it may not have appeared in these results.
Citations:
- 1: CNTRLPLANE-2711: add vault kms plug configuration api api#2805
- 2: CNTRLPLANE-3361: update vault kms plugin validation api#2836
- 3: https://github.com/openshift/api/blob/f14ed7c0/config/v1/types_kmsencryption.go
- 4: CNTRLPLANE-3363: Register health monitor as command cluster-openshift-apiserver-operator#713
🏁 Script executed:
#!/bin/bash
# Check if the enum exists in the bumped version
go mod download github.com/openshift/api@v0.0.0-20260618083218-a3c8dea7f8bc 2>/dev/null && \
grep -r "KMSPluginHealthStatus" \
"$HOME/go/pkg/mod/github.com/openshift/api@v0.0.0-20260618083218-a3c8dea7f8bc/" || echo "Enum not found"Repository: openshift/library-go
Length of output: 951
🏁 Script executed:
# Check the actual usage of KMSPluginHealthStatus in writer.go
find . -name "writer.go" -type f | head -5Repository: openshift/library-go
Length of output: 309
🏁 Script executed:
# Check what versions of openshift/api are available and whether any released version has KMSPluginHealthStatus
git ls-files go.modRepository: openshift/library-go
Length of output: 71
🏁 Script executed:
# Search for how KMSPluginHealthStatus is used in this repository
rg "KMSPluginHealthStatus" -A 2 -B 2Repository: openshift/library-go
Length of output: 6496
🏁 Script executed:
# Check go.mod git history to see what version was previously used
git log --oneline -20 go.mod | head -20Repository: openshift/library-go
Length of output: 103
🏁 Script executed:
# Check what the previous version was for openshift/api
git diff HEAD~1 go.mod | grep -A 2 -B 2 openshift/api || echo "No git history available"Repository: openshift/library-go
Length of output: 294
🏁 Script executed:
# Check if there are any released tags for openshift/api with KMSPluginHealthStatus
# by looking at go.sum for any stable versions
grep "github.com/openshift/api" go.sum | grep -v "v0.0.0" | head -10Repository: openshift/library-go
Length of output: 46
Replace pseudo-version with a released version or justify the necessity.
The openshift/api version v0.0.0-20260618083218-a3c8dea7f8bc is a pseudo-version (unreleased development commit), which violates the supply chain security guideline: "No pre-release or yanked versions in production."
The KMSPluginHealthStatus enum used in writer.go does exist and is compatible in the target version. However, using unreleased commits introduces maintenance and security risks. Either wait for openshift/api to release a stable version with this enum, or provide justification if this dependency must remain unreleased.
🤖 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 `@go.mod` at 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.
Source: Coding guidelines
Comment on lines
+131
to
+151
| ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| var have *applyoperatorv1.KMSEncryptionStatusApplyConfiguration | ||
| c := &Config{ | ||
| nodeName: "node-1", | ||
| interval: time.Hour, // never reached; cancelled after the first tick | ||
| writeTimeout: time.Second, | ||
| prober: &prober{ | ||
| plugins: []pluginClient{ | ||
| {keyID: "1", service: &fakeService{resp: &kmsservice.StatusResponse{Healthz: "ok", KeyID: "kek-abc"}}}, | ||
| }, | ||
| now: func() time.Time { return time.Unix(0, 0).UTC() }, | ||
| }, | ||
| writeStatus: func(_ context.Context, status *applyoperatorv1.KMSEncryptionStatusApplyConfiguration) error { | ||
| have = status | ||
| cancel() | ||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| require.NoError(t, c.Run(ctx)) |
There was a problem hiding this comment.
Add a timeout-bound context to prevent stuck CI on regressions.
At Line [131], using context.WithCancel means c.Run(ctx) at Line [151] can block forever if writeStatus is never reached. A timeout makes this test fail fast instead of hanging.
💡 Proposed fix
- ctx, cancel := context.WithCancel(context.Background())
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ t.Cleanup(cancel)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ctx, cancel := context.WithCancel(context.Background()) | |
| var have *applyoperatorv1.KMSEncryptionStatusApplyConfiguration | |
| c := &Config{ | |
| nodeName: "node-1", | |
| interval: time.Hour, // never reached; cancelled after the first tick | |
| writeTimeout: time.Second, | |
| prober: &prober{ | |
| plugins: []pluginClient{ | |
| {keyID: "1", service: &fakeService{resp: &kmsservice.StatusResponse{Healthz: "ok", KeyID: "kek-abc"}}}, | |
| }, | |
| now: func() time.Time { return time.Unix(0, 0).UTC() }, | |
| }, | |
| writeStatus: func(_ context.Context, status *applyoperatorv1.KMSEncryptionStatusApplyConfiguration) error { | |
| have = status | |
| cancel() | |
| return nil | |
| }, | |
| } | |
| require.NoError(t, c.Run(ctx)) | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| t.Cleanup(cancel) | |
| var have *applyoperatorv1.KMSEncryptionStatusApplyConfiguration | |
| c := &Config{ | |
| nodeName: "node-1", | |
| interval: time.Hour, // never reached; cancelled after the first tick | |
| writeTimeout: time.Second, | |
| prober: &prober{ | |
| plugins: []pluginClient{ | |
| {keyID: "1", service: &fakeService{resp: &kmsservice.StatusResponse{Healthz: "ok", KeyID: "kek-abc"}}}, | |
| }, | |
| now: func() time.Time { return time.Unix(0, 0).UTC() }, | |
| }, | |
| writeStatus: func(_ context.Context, status *applyoperatorv1.KMSEncryptionStatusApplyConfiguration) error { | |
| have = status | |
| cancel() | |
| return nil | |
| }, | |
| } | |
| require.NoError(t, c.Run(ctx)) |
🤖 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 `@pkg/operator/encryption/kms/health/cmd_test.go` around lines 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.
Comment on lines
+135
to
139
| // reporterID is the per-node ownership identity. The naming convention lives | ||
| // here, not in the caller's writer, so all three operators stay uniform. | ||
| reporterID := "kms-health-reporter-" + o.NodeName | ||
| writeStatus, err := o.newWriter(restCfg, reporterID) | ||
| if err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Reporter ID creation and flow
rg -n -C3 'reporterID|NewEncryptionStatusWriterFunc|newWriter\(' --type go
# 2) Writer implementations using field manager semantics
rg -n -C3 'FieldManager|fieldManager|ApplyStatus|Apply\(' --type go
# 3) Existing guards/truncation for reporter identity length
rg -n -C2 'maxFieldManager|len\\(.*reporterID|truncate|hash' --type goRepository: openshift/library-go
Length of output: 50517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Examine the validate() method to see what validation already exists
echo "=== Validate function ==="
rg -n -A 20 'func \(o \*options\) validate' pkg/operator/encryption/kms/health/cmd.go
# Find where reporterID flows after being created
echo -e "\n=== reporterID usage context ==="
rg -n -B 2 -A 5 'reporterID :=' pkg/operator/encryption/kms/health/cmd.go
# Search for the writer implementation file
echo -e "\n=== EncryptionStatusWriter interface and implementations ==="
rg -n 'type EncryptionStatusWriter' pkg/operator/encryption/kms/health/ -A 3
rg -n 'newWriter' pkg/operator/encryption/kms/health/ | head -20Repository: openshift/library-go
Length of output: 2067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full validate() function
echo "=== Full validate() function ==="
rg -n -A 30 'func \(o \*options\) validate' pkg/operator/encryption/kms/health/cmd.go
# Check writer.go for how reporterID is used
echo -e "\n=== writer.go content ==="
cat pkg/operator/encryption/kms/health/writer.go
# Search for fieldManager usage in the file
echo -e "\n=== fieldManager usage in kms/health ==="
rg -n 'fieldManager\|FieldManager' pkg/operator/encryption/kms/health/Repository: openshift/library-go
Length of output: 2808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where NewEncryptionStatusWriterFunc is implemented in the codebase
echo "=== Searching for writer factory implementations ==="
rg -n 'NewEncryptionStatusWriterFunc' --type go | grep -v vendor | grep -v test
# Search for where this factory creates the actual writer that uses fieldManager
echo -e "\n=== Searching for Patch/Apply calls that might use fieldManager with reporterID ==="
rg -n 'Patch|Apply|fieldManager' pkg/operator/encryption/kms/ --type go | head -40
# Check if there are any tests or examples showing how the writer uses reporterID
echo -e "\n=== Looking for test files or references to understand how reporterID is consumed ==="
fd -e go -path '*/kms/*' | xargs grep -l 'reporterID' 2>/dev/null || echo "No additional references found"Repository: openshift/library-go
Length of output: 1922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the writer factory is actually implemented (likely in parent or sibling package)
echo "=== Searching for writer factory implementations in kms and parent packages ==="
rg -n 'func.*NewEncryptionStatusWriterFunc' --type go -g '!vendor' -g '!*test*'
# Check for Patch method calls that might use fieldManager
echo -e "\n=== Searching for Patch/Apply patterns with fieldManager parameter ==="
rg -n 'Patch.*fieldManager\|\.Patch\(' pkg/operator/encryption/ --type go -B 2 -A 2 | head -50
# Look for where reporterID gets passed to Patch or Apply calls
echo -e "\n=== Searching for context where reporterID might be used in Patch/Apply ==="
rg -n 'reporterID|fieldManager' pkg/operator/encryption/ --type go | grep -v test | grep -v vendor
# Check the actual implementation or tests in cmd_test.go
echo -e "\n=== Check test file for clues about how writer uses reporterID ==="
rg -n -B 5 -A 15 'NewEncryptionStatusWriterFunc' pkg/operator/encryption/kms/health/cmd_test.go 2>/dev/null | head -50Repository: openshift/library-go
Length of output: 356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the entire pkg for who implements or creates NewEncryptionStatusWriterFunc
echo "=== Searching entire pkg/operator for writer factory implementations ==="
rg -n 'NewEncryptionStatusWriterFunc' pkg/operator/ --type go | grep -v '/health/'
# Look for Apply or Patch calls with fieldManager in the broader operator package
echo -e "\n=== Searching for Patch/Apply with fieldManager usage across operator ==="
rg -n 'fieldManager.*=' pkg/operator/ --type go -B 2 -A 2 | head -80
# Search for Kubernetes fieldManager length constraints in documentation or code
echo -e "\n=== Check if there are any comments about fieldManager length limits ==="
rg -n 'fieldManager.*128\|maxFieldManager\|fieldManager.*len\|fieldManager.*limit' --type go
# Look for how server-side apply is used with fieldManager
echo -e "\n=== Server-side apply patterns with fieldManager ==="
rg -n '\.Patch\(|\.Apply\(' pkg/operator/encryption/ --type go -B 3 -A 3 | head -60Repository: openshift/library-go
Length of output: 137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# The writer factory is injected, but let's look at the test to see how it's mocked
echo "=== Examining cmd_test.go for writer implementation hints ==="
cat pkg/operator/encryption/kms/health/cmd_test.go | head -200
# Search for where NewCommand is called to see what factory is passed
echo -e "\n=== Where NewCommand is used (to find actual factory) ==="
rg -n 'NewCommand' pkg/ --type go -B 3 -A 3 | head -60Repository: openshift/library-go
Length of output: 8607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at what the Config struct contains and how writeStatus is used
echo "=== Looking at Config struct definition and usage ==="
rg -n 'type Config struct' pkg/operator/encryption/kms/health/ -A 20
# Check how Config.Run uses writeStatus
echo -e "\n=== How Config.Run uses writeStatus ==="
rg -n 'func.*\(c \*Config\) Run' pkg/operator/encryption/kms/health/ -A 50 | head -70
# Search for any Kubernetes fieldManager documentation or constraints
echo -e "\n=== Checking for Kubernetes API constraints in vendor or imports ==="
rg -n 'fieldManager' vendor/k8s.io/apimachinery/ --type go | grep -i 'maxlen\|length\|constraint\|128\|limit' | head -20
# Look for where the writer is actually created in integration or e2e tests
echo -e "\n=== Search for actual writer factory implementations ==="
rg -n 'func.*NewEncryptionStatusWriterFunc\|reporterID.*fieldManager' --type go -g '!vendor'Repository: openshift/library-go
Length of output: 7814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact FieldManagerMaxLength constant
echo "=== Finding FieldManagerMaxLength constant ==="
rg -n 'FieldManagerMaxLength' vendor/k8s.io/ --type go -B 2 -A 2
# Verify the ValidateFieldManager function to confirm the constraint
echo -e "\n=== ValidateFieldManager function details ==="
rg -n -A 10 'func.*ValidateFieldManager' vendor/k8s.io/apimachinery/ --type goRepository: openshift/library-go
Length of output: 3738
Add length validation for reporterID in the options validator.
At line 137, reporterID is constructed by concatenating "kms-health-reporter-" (20 characters) with o.NodeName and passed to the writer as the fieldManager (line 138). Kubernetes enforces a 128-character limit on fieldManager values, which means any NodeName exceeding 108 characters will cause every status write to fail with a validation error from the Kubernetes API.
Add a length check in the validate() method to catch this during startup rather than experiencing repeated write failures:
Proposed fix
func (o *options) validate() error {
+ const reporterPrefix = "kms-health-reporter-"
+ const maxFieldManagerLen = 128
+
if len(o.KMSSockets) == 0 {
return fmt.Errorf("--kms-sockets is required, at least one")
}Add this check after the NodeName empty check:
if o.NodeName == "" {
return fmt.Errorf("--node-name is required")
}
+ if len(reporterPrefix+o.NodeName) > maxFieldManagerLen {
+ return fmt.Errorf("--node-name is too long for reporter identity (%d > %d)", len(reporterPrefix+o.NodeName), maxFieldManagerLen)
+ }🤖 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 `@pkg/operator/encryption/kms/health/cmd.go` around lines 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.
ardaguclu
reviewed
Jun 19, 2026
| Kubeconfig string | ||
|
|
||
| newOperatorClient func(*rest.Config) (v1helpers.OperatorClient, error) | ||
| newWriter NewEncryptionStatusWriterFunc |
Member
There was a problem hiding this comment.
Before discussing the details, I think it would be very useful to have a PoC PR in respective operators to see how callers will call this (https://github.com/openshift/cluster-kube-apiserver-operator/blob/4e7619777cb6e5bb7853836d07656da3e1eb14df/cmd/cluster-kube-apiserver-operator/main.go#L70-L74)
Signed-off-by: Krzysztof Ostrowski <kostrows@redhat.com>
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
Comment on lines
+21
to
+23
| github.com/openshift/api v0.0.0-20260618083218-a3c8dea7f8bc | ||
| github.com/openshift/build-machinery-go v0.0.0-20250530140348-dc5b2804eeee | ||
| github.com/openshift/client-go v0.0.0-20260512113608-deb4dc54551a | ||
| github.com/openshift/client-go v0.0.0-20260618131434-17fd91ed6167 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the local filesystem replace directive has been removed
# Check for any replace directives in go.mod
echo "=== Checking for replace directives ==="
rg -n "^replace " go.mod || echo "No replace directives found"
# Check all github.com/openshift dependencies for pseudo-versions
echo ""
echo "=== Checking all openshift dependency versions ==="
rg -n "github.com/openshift" go.mod | grep -E "^\s*github.com/openshift"
# Extract the actual versions being used
echo ""
echo "=== Extracting exact versions ==="
grep "github.com/openshift" go.mod | awk '{print $1, $2}'Repository: openshift/library-go
Length of output: 459
Replace pseudo-version dependencies with stable released versions or explicitly justify their necessity.
The local filesystem replace directive that was flagged in a previous review has been removed—that issue is resolved.
However, the three github.com/openshift/* dependencies (lines 21–23) remain on pseudo-versions (v0.0.0-YYYYMMDD...), which represent development commits rather than official releases. Per the supply-chain security guidelines, production dependencies must use stable pinned released versions.
Either:
- Upgrade these three dependencies to their latest stable released versions, or
- Add a comment in the pull description justifying why pseudo-versions are currently necessary (e.g., awaiting a stable release, bug fix not yet shipped).
🤖 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 `@go.mod` around lines 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).
Source: Coding guidelines
Contributor
|
@ibihim: 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. |
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
3 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.
Summary by CodeRabbit
Bug Fixes
Tests