kms: Add unsupported config for vault kms plugin log level#2290
kms: Add unsupported config for vault kms plugin log level#2290kevinrizza wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR threads an ChangesKMS Vault logLevel configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kevinrizza 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 `@pkg/operator/encryption/kms/pluginlifecycle/unsupported_config.go`:
- Around line 33-35: The json.Unmarshal error is being swallowed in the
unsupportedKMSConfig decoding path; update the block that calls
json.Unmarshal(jsonRaw, &config) to log the error (using the same logging
mechanism used for the YAML conversion error) before returning
unsupportedKMSConfig{}, nil so malformed JSON is observable; reference the
json.Unmarshal call, the jsonRaw variable, and the unsupportedKMSConfig return
to locate and fix the code, ensuring the log message includes context that the
unsupported KMS config JSON was invalid.
🪄 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: 870baea6-d690-47ad-a7ec-63fa950bfad0
📒 Files selected for processing (6)
pkg/operator/encryption/kms/pluginlifecycle/sidecar.gopkg/operator/encryption/kms/pluginlifecycle/sidecar_test.gopkg/operator/encryption/kms/pluginlifecycle/unsupported_config.gopkg/operator/encryption/kms/pluginlifecycle/unsupported_config_test.gopkg/operator/encryption/kms/pluginlifecycle/vault.gopkg/operator/encryption/kms/pluginlifecycle/vault_test.go
| if err := json.Unmarshal(jsonRaw, &config); err != nil { | ||
| return unsupportedKMSConfig{}, nil | ||
| } |
There was a problem hiding this comment.
Log JSON unmarshaling errors before swallowing them.
Line 34 silently ignores JSON unmarshaling errors and returns an empty config with no error. While the test shows this lenient behavior is intentional, the error should be logged (like the YAML conversion error at line 28) so users know when their unsupported config is malformed and being ignored.
As per coding guidelines, Go code should never ignore error returns. Even when errors are intentionally not propagated, they should be logged for observability.
📋 Proposed fix to add logging
config := unsupportedKMSConfig{}
if err := json.Unmarshal(jsonRaw, &config); err != nil {
+ klog.V(4).Infof("failed to unmarshal unsupported KMS config, ignoring: %v", err)
return unsupportedKMSConfig{}, nil
}🤖 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/pluginlifecycle/unsupported_config.go` around
lines 33 - 35, The json.Unmarshal error is being swallowed in the
unsupportedKMSConfig decoding path; update the block that calls
json.Unmarshal(jsonRaw, &config) to log the error (using the same logging
mechanism used for the YAML conversion error) before returning
unsupportedKMSConfig{}, nil so malformed JSON is observable; reference the
json.Unmarshal call, the jsonRaw variable, and the unsupportedKMSConfig return
to locate and fix the code, ensuring the log message includes context that the
unsupported KMS config JSON was invalid.
Source: Coding guidelines
Also wires definition of unsupported config overrides for kms sidecars
kevinrizza
force-pushed
the
unsupported-log-level-vault-config
branch
from
b115e9e to
42a0e5f
Compare
|
/retest |
|
@kevinrizza: 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
1 participant
This pull request adds support for configuring the vault kms plugin's log-level via the unsupportedconfigoverrides API.
Additionally, it adds the wiring for unsupportedconfigoverrides in general for the kms plugin sidecars, which will allow us to add additional unsupported overrides in the future.
The shape of that object is defined to look like this:
For example, you can patch the openshift aggregated apiserver's config like this through a terminal:
oc patch openshiftapiserver cluster --type=merge -p '{"spec":{"unsupportedConfigOverrides":{"encryption":{"kms":{"vault":{"logLevel":"debug-extended"}}}}}}'Summary by CodeRabbit
New Features
Refactor
Tests