kms: wire up config overrides for KMS values#914
Open
kevinrizza wants to merge 1 commit into
Open
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 ignored due to path filters (6)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a go.mod replace for library-go and updates the operator call to AddKMSPluginSidecarToPodSpec to pass ChangesKMS Plugin Configuration Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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: 2
🤖 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`:
- Line 137: The go.mod contains a replace directive pointing to a personal fork
("replace github.com/openshift/library-go => github.com/kevinrizza/library-go
..."); remove this replace before merge and instead vendor or reference the
upstream module version of github.com/openshift/library-go (pin to an exact
upstream pseudo-version or tag) and update go.sum accordingly; ensure the chosen
upstream version is justified, license-compatible, and hashes are verified per
supply-chain requirements.
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Around line 325-326: The code references an undefined variable operatorConfig
on the call that passes featureGateAccessor and UnsupportedConfigOverrides.Raw;
replace operatorConfig with the actual function parameter operatorSpec to fix
the undefined reference (update the call that currently uses operatorConfig to
use operatorSpec.Spec.UnsupportedConfigOverrides.Raw so it compiles correctly
and uses the intended value).
🪄 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: 2ea4c870-3a75-4941-9910-1965438958ec
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/sidecar.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/unsupported_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modpkg/operator/workload/sync_openshift_oauth_apiserver.go
|
|
||
| replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20251001123353-fd5b1fb35db1 | ||
|
|
||
| replace github.com/openshift/library-go => github.com/kevinrizza/library-go v0.0.0-20260609171831-1dc5b3029e36 |
There was a problem hiding this comment.
Remove the personal fork replace directive before merge.
The replace directive points to a personal fork (github.com/kevinrizza/library-go) rather than the upstream github.com/openshift/library-go. As noted in the PR description, this is WIP state and should be updated to vendor the real library-go change before merge. Merging with a personal fork violates supply chain security best practices.
As per coding guidelines: "New deps: justify need, check license compatibility" and "Pin exact versions; verify hashes where supported" from supply chain security requirements.
🤖 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 137, The go.mod contains a replace directive pointing to a
personal fork ("replace github.com/openshift/library-go =>
github.com/kevinrizza/library-go ..."); remove this replace before merge and
instead vendor or reference the upstream module version of
github.com/openshift/library-go (pin to an exact upstream pseudo-version or tag)
and update go.sum accordingly; ensure the chosen upstream version is justified,
license-compatible, and hashes are verified per supply-chain requirements.
Source: Coding guidelines
kevinrizza
force-pushed
the
kms-unsupported-overrides-log-level
branch
from
7a216a4 to
972c646
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/operator/workload/sync_openshift_oauth_apiserver.go (1)
325-326:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix unresolved undefined variable in KMS sidecar call.
Line 326 still references
operatorConfig, which is not in scope here. This is a compile-time failure; use the in-scopeoperatorSpecinstead.🐛 Minimal fix
if err := kmspluginlifecycle.AddKMSPluginSidecarToPodSpec( ctx, &required.Spec.Template.Spec, "oauth-apiserver", c.targetNamespace, fmt.Sprintf("encryption-config-%d", operatorStatus.LatestAvailableRevision), c.kubeClient.CoreV1(), c.featureGateAccessor, - operatorConfig.Spec.UnsupportedConfigOverrides.Raw); err != nil { + operatorSpec.UnsupportedConfigOverrides.Raw); err != nil { return nil, fmt.Errorf("failed to add KMS plugin to pod spec: %w", err) }🤖 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/workload/sync_openshift_oauth_apiserver.go` around lines 325 - 326, The call to the KMS sidecar still uses the out-of-scope variable operatorConfig (operatorConfig.Spec.UnsupportedConfigOverrides.Raw); replace that reference with the in-scope operatorSpec by changing operatorConfig.Spec.UnsupportedConfigOverrides.Raw to operatorSpec.Spec.UnsupportedConfigOverrides.Raw (i.e., update the KMS sidecar invocation to use operatorSpec and keep the same field access), ensuring you only alter the variable name used in the call where c.featureGateAccessor and the overrides are passed.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Around line 325-326: The call to the KMS sidecar still uses the out-of-scope
variable operatorConfig (operatorConfig.Spec.UnsupportedConfigOverrides.Raw);
replace that reference with the in-scope operatorSpec by changing
operatorConfig.Spec.UnsupportedConfigOverrides.Raw to
operatorSpec.Spec.UnsupportedConfigOverrides.Raw (i.e., update the KMS sidecar
invocation to use operatorSpec and keep the same field access), ensuring you
only alter the variable name used in the call where c.featureGateAccessor and
the overrides are passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cc366ad-41d9-4b7b-9fc3-5d33acfaa35f
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/sidecar.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/unsupported_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modpkg/operator/workload/sync_openshift_oauth_apiserver.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
kevinrizza
force-pushed
the
kms-unsupported-overrides-log-level
branch
from
972c646 to
c1766d3
Compare
pass log level along to vault WIP - this commit is using a replaces directive for my library-go fork. needs to be updated to vendor real change in library-go
kevinrizza
force-pushed
the
kms-unsupported-overrides-log-level
branch
from
c1766d3 to
e2a1a1d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/operator/workload/sync_openshift_oauth_apiserver.go (1)
326-326:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix undefined variable reference.
Line 326 references
operatorConfigwhich does not exist in scope. The function parameter is namedoperatorSpec(line 224). This will cause a compilation failure.🐛 Proposed fix
- operatorConfig.Spec.UnsupportedConfigOverrides.Raw); err != nil { + operatorSpec.UnsupportedConfigOverrides.Raw); err != 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/workload/sync_openshift_oauth_apiserver.go` at line 326, The code references an undefined variable operatorConfig; replace that reference with the actual function parameter operatorSpec and ensure the field access matches the parameter's type (e.g., use operatorSpec.UnsupportedConfigOverrides.Raw or operatorSpec.Spec.UnsupportedConfigOverrides.Raw as appropriate) where UnsupportedConfigOverrides.Raw is used so the compiler finds the correct symbol.
🤖 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.
Duplicate comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Line 326: The code references an undefined variable operatorConfig; replace
that reference with the actual function parameter operatorSpec and ensure the
field access matches the parameter's type (e.g., use
operatorSpec.UnsupportedConfigOverrides.Raw or
operatorSpec.Spec.UnsupportedConfigOverrides.Raw as appropriate) where
UnsupportedConfigOverrides.Raw is used so the compiler finds the correct symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c80514f-3519-4cbb-bb68-a7511df8e985
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/sidecar.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/unsupported_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/pluginlifecycle/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modpkg/operator/workload/sync_openshift_oauth_apiserver.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Contributor
|
@kevinrizza: 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. |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
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. |
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
1 participant
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.
pass log level along to vault
WIP - this commit is using a replaces directive for my library-go fork. needs to be updated to vendor real change in library-go
Summary by CodeRabbit