CNTRLPLANE-3237: Error out when encryption-config Secret contains mismatching KMS providers - #2202
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
WalkthroughFromEncryptionState now returns (*Config, error) and Config gains a KMSProviders map. The conversion aggregates and validates KMS provider configs by key ID (erroring on mismatches). Call sites and tests were updated to handle the error return. ChangesKMS Provider aggregation, validation, and error propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
87fe32b to
1cc15b6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/operator/encryption/controllers/state_controller_test.go (2)
715-1173: ⚡ Quick winAdd a negative regression case for the new mismatch check.
These new KMS scenarios only cover matching provider configs or different key IDs. The PR’s core behavior is rejecting the same key ID when different resources carry different provider configs, so that failure path still is not exercised here. A scenario that expects
Sync()to error and the degraded condition to flip would lock the new behavior down.🤖 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/controllers/state_controller_test.go` around lines 715 - 1173, Add a negative regression test case to the scenarios slice in state_controller_test.go that exercises the new "mismatch" rejection: create initialResources using the same KMS key ID (e.g., key "1") across two resources/namespaces but supply different provider configs (use encryptiontesting.CreateEncryptionKeySecretWithKMSConfig for one and CreateEncryptionKeySecretWithCustomKMSConfig for the other) so Sync() should return an error and flip degraded; set expectedActions to include the normal reads but expect no create/update of the encryption-config secret, and implement validateFunc to assert that the controller returned an error (or that degraded condition was set) instead of creating/updating the secret so the failure path for the mismatch check is covered.
1130-1143: ⚡ Quick winReuse the shared default KMS fixture here.
This block re-inlines the same Vault config that the rest of the file already pulls from
encryptiontesting.DefaultKMSProviderConfig. Reusing the shared fixture would avoid one-off drift when the default test config changes.🤖 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/controllers/state_controller_test.go` around lines 1130 - 1143, The test is re-inlining a Vault KMS config that already exists as a shared fixture; replace the inlined map entry in KMSProviders with the shared fixture by using encryptiontesting.DefaultKMSProviderConfig (instead of constructing a configv1.VaultKMSConfig block), so the map becomes e.g. KMSProviders: map[string]*configv1.KMSConfig{"2": encryptiontesting.DefaultKMSProviderConfig} to avoid duplication and drift.pkg/operator/encryption/testing/helpers.go (1)
101-118: ⚡ Quick winAvoid sharing a mutable default KMS config across tests.
DefaultKMSProviderConfigis now a package-global*configv1.KMSConfigthat gets threaded through multiple fixtures. One accidental mutation in a scenario will leak into unrelated tests. A small constructor that returns a fresh config for each call would make these helpers much safer.🤖 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/testing/helpers.go` around lines 101 - 118, Replace the package-global pointer DefaultKMSProviderConfig with a constructor function (e.g., NewDefaultKMSProviderConfig) that returns a freshly allocated *configv1.KMSConfig each call; update CreateEncryptionKeySecretWithKMSConfig to call this constructor (or callers that rely on the default) and pass the returned value into CreateEncryptionKeySecretWithCustomKMSConfig so tests get isolated, non-shared configs and accidental mutations cannot leak across tests.
🤖 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.
Nitpick comments:
In `@pkg/operator/encryption/controllers/state_controller_test.go`:
- Around line 715-1173: Add a negative regression test case to the scenarios
slice in state_controller_test.go that exercises the new "mismatch" rejection:
create initialResources using the same KMS key ID (e.g., key "1") across two
resources/namespaces but supply different provider configs (use
encryptiontesting.CreateEncryptionKeySecretWithKMSConfig for one and
CreateEncryptionKeySecretWithCustomKMSConfig for the other) so Sync() should
return an error and flip degraded; set expectedActions to include the normal
reads but expect no create/update of the encryption-config secret, and implement
validateFunc to assert that the controller returned an error (or that degraded
condition was set) instead of creating/updating the secret so the failure path
for the mismatch check is covered.
- Around line 1130-1143: The test is re-inlining a Vault KMS config that already
exists as a shared fixture; replace the inlined map entry in KMSProviders with
the shared fixture by using encryptiontesting.DefaultKMSProviderConfig (instead
of constructing a configv1.VaultKMSConfig block), so the map becomes e.g.
KMSProviders: map[string]*configv1.KMSConfig{"2":
encryptiontesting.DefaultKMSProviderConfig} to avoid duplication and drift.
In `@pkg/operator/encryption/testing/helpers.go`:
- Around line 101-118: Replace the package-global pointer
DefaultKMSProviderConfig with a constructor function (e.g.,
NewDefaultKMSProviderConfig) that returns a freshly allocated
*configv1.KMSConfig each call; update CreateEncryptionKeySecretWithKMSConfig to
call this constructor (or callers that rely on the default) and pass the
returned value into CreateEncryptionKeySecretWithCustomKMSConfig so tests get
isolated, non-shared configs and accidental mutations cannot leak across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a8cc2d5f-0da9-4d15-8291-d92890b790ab
📒 Files selected for processing (13)
pkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/migration_controller.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/config_test.gopkg/operator/encryption/encryptiondata/secret.gopkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/statemachine/transition_test.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
1cc15b6 to
9d684bf
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
| } | ||
| if _, exists := kmsProviders[key.Key.Name]; !exists { | ||
| if provider, exists := kmsProviders[key.Key.Name]; exists { | ||
| if !reflect.DeepEqual(provider, key.KMSConfig.Provider) { |
There was a problem hiding this comment.
since we're comparing api types, should we use the equality package from upstream? There's also maps.Equal, but I think equality is more appropriate
There was a problem hiding this comment.
Updated to use equality.Semantic.DeepEqual.
| if _, exists := kmsProviders[key.Key.Name]; !exists { | ||
| if provider, exists := kmsProviders[key.Key.Name]; exists { | ||
| if !reflect.DeepEqual(provider, key.KMSConfig.Provider) { | ||
| return nil, fmt.Errorf("KMS provider config mismatch for keyID %s: configs from different resources must be identical", key.Key.Name) |
There was a problem hiding this comment.
in what scenarios can this happen? When the user manually change the secrets? Or when there's a bug in our code?
There was a problem hiding this comment.
When the user manually change the secrets?
I think mostly this can cause
There was a problem hiding this comment.
Or when there's a bug in our code?
that too, that's why we want to add an extra check.
| } | ||
| if _, exists := kmsProviders[key.Key.Name]; !exists { | ||
| if provider, exists := kmsProviders[key.Key.Name]; exists { | ||
| if !reflect.DeepEqual(provider, key.KMSConfig.Provider) { |
There was a problem hiding this comment.
please add a comment explaining why we need an extra check.
9d684bf to
ae1be62
Compare
| return make([]byte, 16) | ||
| } | ||
|
|
||
| func TestFromEncryptionStateKMSProviderConfigMismatch(t *testing.T) { |
There was a problem hiding this comment.
nit: could you rework to table tests ?
There was a problem hiding this comment.
Updated for table tests
|
LGTM |
|
I'm not tagging so that @bertinatto has a chance to look at the PR. |
ae1be62 to
c93db8d
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/encryption/encryptiondata/config_test.go`:
- Around line 718-737: The test "matching provider configs across resources"
currently reuses the same *configv1.KMSConfig pointer via
encryptiontesting.DefaultKMSProviderConfig for both resources; create two
separate but equal provider config objects instead (e.g., construct a new
apiserverconfigv1.KMSConfiguration/Provider object for the secrets entry and a
distinct one with the same fields for the configmaps entry) so that
state.KMSConfig.KMSConfig pointers are different while values match; update the
encryptionState entries' KMSConfig.Provider fields to reference these two
distinct instances rather than the single shared pointer.
🪄 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: d87cdd7f-08ce-4738-a10e-33623919b8cc
📒 Files selected for processing (5)
pkg/operator/encryption/controllers/migration_controller.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/config_test.gopkg/operator/encryption/statemachine/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/encryption/controllers/migration_controller.go
- pkg/operator/encryption/statemachine/transition_test.go
- pkg/operator/encryption/encryptiondata/config.go
| name: "matching provider configs across resources", | ||
| encryptionState: map[schema.GroupResource]state.GroupResourceState{ | ||
| {Resource: "secrets"}: { | ||
| ReadKeys: []state.KeyState{{ | ||
| Key: apiserverconfigv1.Key{Name: "1", Secret: "AAAAAAAAAAAAAAAAAAAAAA=="}, | ||
| Mode: state.KMS, | ||
| KMSConfig: &state.KMSConfig{ | ||
| Encryption: &apiserverconfigv1.KMSConfiguration{APIVersion: "v2", Name: "1", Endpoint: "unix:///var/run/kmsplugin/kms-1.sock"}, | ||
| Provider: encryptiontesting.DefaultKMSProviderConfig, | ||
| }, | ||
| }}, | ||
| }, | ||
| {Resource: "configmaps"}: { | ||
| ReadKeys: []state.KeyState{{ | ||
| Key: apiserverconfigv1.Key{Name: "1", Secret: "AAAAAAAAAAAAAAAAAAAAAA=="}, | ||
| Mode: state.KMS, | ||
| KMSConfig: &state.KMSConfig{ | ||
| Encryption: &apiserverconfigv1.KMSConfiguration{APIVersion: "v2", Name: "1", Endpoint: "unix:///var/run/kmsplugin/kms-1.sock"}, | ||
| Provider: encryptiontesting.DefaultKMSProviderConfig, | ||
| }, |
There was a problem hiding this comment.
Use distinct-but-equal provider configs in the “matching” test case.
Line 726 and Line 736 currently reuse the same *configv1.KMSConfig pointer. That can let a pointer-identity bug pass. Please construct two separate but equivalent provider config objects for the two resources.
🤖 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/encryptiondata/config_test.go` around lines 718 -
737, The test "matching provider configs across resources" currently reuses the
same *configv1.KMSConfig pointer via encryptiontesting.DefaultKMSProviderConfig
for both resources; create two separate but equal provider config objects
instead (e.g., construct a new apiserverconfigv1.KMSConfiguration/Provider
object for the secrets entry and a distinct one with the same fields for the
configmaps entry) so that state.KMSConfig.KMSConfig pointers are different while
values match; update the encryptionState entries' KMSConfig.Provider fields to
reference these two distinct instances rather than the single shared pointer.
|
@ardaguclu: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, bertinatto 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
4 participants
As promised in #2163 (comment), this PR errors out when there is a discrepancy between same KMS providers in different resources.
Summary by CodeRabbit
New Features
Bug Fixes
Tests