CNTRLPLANE-3236: support deploying Vault mock KMS plugin#2146
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughUpdated Go module pins. Added ChangesDependency and target config controller update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labelsjira/valid-reference Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 `@go.mod`:
- Line 140: Remove the personal fork replace directive currently replacing
github.com/openshift/library-go with github.com/bertinatto/library-go (the
replace line in go.mod); delete that replace entry and restore/point to the
official github.com/openshift/library-go module (use an appropriate released
version or remove the replace so the go tool resolves the upstream tag) so the
dependency no longer references your personal fork.
🪄 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: 87628888-2677-494d-aa8c-cf27c567efad
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/migration_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/plugins/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/sidecar.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modpkg/operator/starter.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
💤 Files with no reviewable changes (1)
- pkg/operator/starter.go
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/targetconfigcontroller/targetconfigcontroller.go`:
- Line 230: managePods currently receives secretLister but never reads it, so
either wire it into the pod-rendering logic or remove it to avoid a no-op;
update the managePods declaration and its internal logic to use the passed
secretLister (e.g., call secretLister.Secrets(namespace).Get(secretName) where
pod templates or credentials are resolved) and handle not-found/errors, or if
secrets are not needed, remove the secretLister parameter from managePods and
all its call sites (including the call from targetconfigcontroller where
managePods is invoked) to keep the signature consistent.
🪄 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: b328744a-15ff-4712-9324-cf6969c77150
⛔ Files ignored due to path filters (13)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/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/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/library-go/pkg/operator/encryption/encoding/encoding.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modpkg/operator/targetconfigcontroller/targetconfigcontroller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
from
26bf24b to
a8a6f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Around line 335-337: Move the proxy env var application so it runs after the
KMS sidecar is injected: call encryptionkms.AddKMSPluginSidecarToPodSpec(ctx,
&required.Spec, ...) first, then iterate required.Spec.Containers (and
InitContainers if applicable) to add HTTP_PROXY/HTTPS_PROXY/NO_PROXY so the
newly appended sidecar also receives them; update the code around the
AddKMSPluginSidecarToPodSpec invocation and the proxy-env application logic
accordingly.
🪄 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: e8403636-f71b-4d59-b494-868ddd3ea438
⛔ Files ignored due to path filters (40)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_operator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_image.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.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/console/v1/types_console_plugin.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.featuregated-crd-manifests.yamlis 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/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/apiserverencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/kmspluginconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/registrysources.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/vsphereplatformspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/nodeexportercollectorsoftirqsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/migration_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encoding/encoding.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/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/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modpkg/operator/targetconfigcontroller/targetconfigcontroller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
from
a8a6f7f to
5f9e3a4
Compare
bertinatto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bertinatto: This pull request references CNTRLPLANE-3236 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 story 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. |
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
2 times, most recently
from
0b3ee72 to
297c81b
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-techpreview-serial |
|
@bertinatto: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f5ac7010-4ed7-11f1-9cb0-89c4ec005c5c-0 |
| return nil, false, fmt.Errorf("failed to get feature gates: %w", err) | ||
| } | ||
| if featureGates.Enabled(features.FeatureGateKMSEncryption) { | ||
| err := kmspluginlifecycle.AddKMSPluginSidecarToPodSpec(ctx, &required.Spec, "kube-apiserver", "openshift-config-managed", "encryption-config-openshift-kube-apiserver", secretClient) |
There was a problem hiding this comment.
we need to check if the ns is correct, i think the ns should be openshift-kube-apiserver, no ?
There was a problem hiding this comment.
do encryption controllers sync to openshift-config-managed and then we revision from that ns ?
There was a problem hiding this comment.
xref:
There was a problem hiding this comment.
There was a problem hiding this comment.
and it seems that the revision controller is working against openshift-kube-apiserver"
There was a problem hiding this comment.
openshift-config-managed is the namespace the state controller creates this specific secret: https://github.com/bertinatto/library-go/blob/kms-plugins-lifecycle-vault-mock/pkg/operator/encryption/controllers/state_controller.go#L167-L167
There was a problem hiding this comment.
I think we also copy that secret to openshift-kube-apiserver and name it encryption-secret, which could be more appropriate
There was a problem hiding this comment.
I agree that namespace should be openshift-kube-apiserver. Next question would be: Precondition validation will compare this with a secret under which namespace?
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
from
297c81b to
97bfb3e
Compare
|
|
||
| if err := encryptionkms.AddKMSPluginVolumeAndMountToPodSpec(&required.Spec, "kube-apiserver", featureGateAccessor); err != nil { | ||
| return nil, false, fmt.Errorf("failed to add KMS encryption volumes: %w", err) | ||
| if featureGateAccessor.AreInitialFeatureGatesObserved() { |
There was a problem hiding this comment.
this is what we do in the other places in this operator, right ? we don't rety controller when FG hasn't been observed, right ?
There was a problem hiding this comment.
besides KMS, the accessors are currently only used by config observers in this operator. When feature gates haven't been observed yet, they just report an empty config.
if the feature gates haven't been observed yet, we should probably keep the current behavior of non-TP clusters unchanged (the function inside the if clause reflects that)
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
from
97bfb3e to
ca69fff
Compare
|
/test e2e-gcp-operator-encryption-kms |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
from
08d9005 to
99e50ec
Compare
openshift-ci-robot
removed
the
verified
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
bertinatto
force-pushed
the
kms-plugins-lifecycle-vault-mock
branch
from
99e50ec to
d2f42de
Compare
|
/lgtm |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@tjungblu: This PR has been marked as verified by 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. |
| func TestPerfEncryption(tt *testing.T) { | ||
| operatorClient := operatorencryption.GetOperator(tt) | ||
| library.TestPerfEncryption(tt, library.PerfScenario{ | ||
| library.TestPerfEncryption(tt.Context(), tt, library.PerfScenario{ |
There was a problem hiding this comment.
As far as I recall tt.Context() is always empty. We should get ctx from TestPerfEncryption
| librarykms.DeployUpstreamMockKMSPlugin(context.Background(), t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage, librarykms.DefaultKMSPluginCount) | ||
| library.TestEncryptionProvidersMigration(t, library.ProvidersMigrationScenario{ | ||
| func testKMSEncryptionProvidersMigration(ctx context.Context, t testing.TB) { | ||
| librarykms.DeployUpstreamMockKMSPlugin(ctx, t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage, librarykms.DefaultKMSPluginCount) |
There was a problem hiding this comment.
We don't need this anymore. But this can/should be handled in a separate PR.
| // file | ||
| func TestEncryptionRotation(t *testing.T) { | ||
| library.TestEncryptionRotation(t, library.RotationScenario{ | ||
| library.TestEncryptionRotation(t.Context(), t, library.RotationScenario{ |
|
/lgtm |
openshift-merge-bot
Bot
merged commit d0f9b3c
into
openshift:main
|
@bertinatto: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Summary by CodeRabbit
Chores
Refactor