CNTRLPLANE-3237: Introduce KMSProviderConfig in KeyState - #2186
Merged
openshift-merge-bot[bot] merged 2 commits intoApr 29, 2026
Merged
openshift-merge-bot[bot] merged 2 commits into
openshift-merge-bot[bot] merged 2 commits into
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. |
|
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:
WalkthroughKMS encryption support now carries provider-specific configuration through the system: new API types for KMS provider (Vault), a helper to read provider config from a dedicated Secret, state and secret serializers were extended to persist/validate provider config, and controller/tests updated to use and verify that config. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant SecretsAPI
participant KMSSecret as "KMS Provider Secret"
participant APIHelper as "GetKMSProviderConfigFromSecret()"
participant KeyState
Controller->>SecretsAPI: Get(secret: openshift-config/<name>)
SecretsAPI-->>KMSSecret: returns Secret(with data.config)
Controller->>APIHelper: parse secret.data[config]
APIHelper-->>Controller: KMSConfigAPI (or error)
Controller->>KeyState: set ks.KMS.Provider = KMSConfigAPI
Controller->>KeyState: FromKeyState() → marshals provider JSON into secret data
KeyState-->>SecretsAPI: create/update encryption key Secret (with provider-config)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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
🧹 Nitpick comments (1)
pkg/operator/encryption/testing/helpers.go (1)
117-118: Don’t ignore marshal errors in test fixture generation.Line 117 currently discards
json.Marshalerrors; this can mask fixture regressions.Proposed fix
- providerConfigJSON, _ := json.Marshal(providerConfig) + providerConfigJSON, err := json.Marshal(providerConfig) + if err != nil { + panic(err) + } secret.Data[encryptionSecretKMSProviderConfigForTest] = providerConfigJSON🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/testing/helpers.go` around lines 117 - 118, The code currently ignores the error from json.Marshal(providerConfig) before assigning providerConfigJSON into secret.Data[encryptionSecretKMSProviderConfigForTest]; change this to capture the error (err := json.Marshal(...)), check it, and fail/propagate instead of discarding it — e.g., if this helper accepts testing.T use t.Fatalf("marshal providerConfig: %v", err), or if it returns an error propagate it up; then only assign providerConfigJSON to secret.Data[encryptionSecretKMSProviderConfigForTest] when err == nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/secrets/secrets.go`:
- Around line 78-88: The current reader in
pkg/operator/encryption/secrets/secrets.go rejects KMS secrets missing
EncryptionSecretKMSProviderConfig but other code can write secrets with a nil
ProviderConfig, causing roundtrip failures; update the parsing in the block that
handles s.Data[EncryptionSecretKMSProviderConfig] so that if the key is absent
or empty you do NOT return an error but instead leave key.KMS.ProviderConfig as
nil (or populate it with a default) and continue; when the field exists, still
json.Unmarshal into a state.KMSProviderConfig and assign to
key.KMS.ProviderConfig, and keep the existing error wrapping for malformed JSON.
---
Nitpick comments:
In `@pkg/operator/encryption/testing/helpers.go`:
- Around line 117-118: The code currently ignores the error from
json.Marshal(providerConfig) before assigning providerConfigJSON into
secret.Data[encryptionSecretKMSProviderConfigForTest]; change this to capture
the error (err := json.Marshal(...)), check it, and fail/propagate instead of
discarding it — e.g., if this helper accepts testing.T use t.Fatalf("marshal
providerConfig: %v", err), or if it returns an error propagate it up; then only
assign providerConfigJSON to
secret.Data[encryptionSecretKMSProviderConfigForTest] when err == nil.
🪄 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: a3734530-d955-4377-b024-ef9209a63bf8
📒 Files selected for processing (7)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.go
p0lyn0mial
reviewed
Apr 28, 2026
| @@ -48,6 +49,27 @@ type KeyState struct { | |||
| type KMSConfig struct { | |||
| // Encoded EncryptionConfig that stores the KMS related fields | |||
| EncryptionConfig *apiserverconfigv1.KMSConfiguration | |||
Contributor
There was a problem hiding this comment.
we could rename this to Encryption - could be a new PR.
p0lyn0mial
reviewed
Apr 28, 2026
| EncryptionConfig *apiserverconfigv1.KMSConfiguration | ||
|
|
||
| // ProviderConfig stores KMS provider specific configurations | ||
| ProviderConfig *KMSProviderConfig |
Contributor
There was a problem hiding this comment.
then this could be Provider
Member
Author
There was a problem hiding this comment.
Provider is nicer indeed.
p0lyn0mial
reviewed
Apr 28, 2026
| // TODO: These fields are mimicked from https://github.com/openshift/api/pull/2805 | ||
| // When API PR merges, *v1.KMSConfig will inherently support store fields, | ||
| // so that we can remove these internal structs. | ||
| KMSPluginImage string `json:"kmsPluginImage,omitempty"` |
Contributor
There was a problem hiding this comment.
I would wait until the API definitions land before introducing these changes.
p0lyn0mial
reviewed
Apr 28, 2026
| } | ||
|
|
||
| // KMSProviderConfig stores KMS provider details. | ||
| // This type does not only store the API definitions but also carries internally |
Contributor
There was a problem hiding this comment.
what other fields will be stored here ?
Member
Author
There was a problem hiding this comment.
Credential Secret/ConfigMap mapping will be stored additionally.
p0lyn0mial
reviewed
Apr 28, 2026
| type KMSProviderConfig struct { | ||
| // KMSProviderConfig is directly derived from the KMSConfig in o/api. | ||
| // Therefore, all the KMS provider details in o/api is represented here. | ||
| *v1.KMSConfig |
Contributor
There was a problem hiding this comment.
how about pulling in the o/api PR in this PR ?
Member
Author
There was a problem hiding this comment.
Done. Second commit uses the o/api PR
ardaguclu
force-pushed
the
kms-provider-config-keystate
branch
from
April 28, 2026 10:02
8560739 to
2a88752
Compare
Member
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
p0lyn0mial
reviewed
Apr 28, 2026
| s.Data[EncryptionSecretKMSEncryptionConfig] = kmsEncCfgJSON | ||
| } | ||
|
|
||
| if ks.KMS != nil && ks.KMS.ProviderConfig != nil { |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/key_controller_test.go (1)
366-374: KMS provider-config tests only verify{}today.Because the KMS APIServer fixture has no populated
Spec.Encryption.KMS, these checks won’t catch regressions that drop/reshape actual provider fields. Add at least one scenario with non-empty KMS config and assert roundtrip on decoded struct content.Also applies to: 435-443, 520-528, 722-778, 809-816
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/key_controller_test.go` around lines 366 - 374, The tests in key_controller_test.go only assert the KMS provider-config equals an empty JSON object by marshalling state.KMSProviderConfig{}, which misses regressions; add at least one test case where the APIServer fixture's Spec.Encryption.KMS is populated with non-empty fields, ensure the controller writes that non-empty provider config into actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"], decode/unmarshal the secret bytes back into the same struct type (state.KMSProviderConfig) and assert field-level equality (roundtrip) rather than string equality to catch reshaping; update the analogous checks at the other noted ranges (435-443, 520-528, 722-778, 809-816) to include non-empty scenarios and roundtrip assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 142: The go.mod replace directive "replace github.com/openshift/api =>
github.com/flavianmissi/api v0.0.0-20260424105406-3d4bd99d6aa0" must be removed
or replaced with an upstream commit/tag to avoid using a personal fork; edit the
replace line in go.mod (the replace directive) to either delete it entirely or
point to an official github.com/openshift/api version (a specific tag or commit
hash) and re-run go mod tidy to update go.sum, or keep the forked replace only
in a local, uncommitted workspace if this change is exploratory.
In `@pkg/operator/encryption/secrets/secrets_test.go`:
- Around line 132-138: The test is constructing state.KMSProviderConfig with
fields that don't exist; update the literal to either leave ProviderConfig
nil/empty if the test doesn't need values, or build the nested v1.KMSConfig
properly: set ProviderConfig to &state.KMSProviderConfig{KMSConfig:
&v1.KMSConfig{Type: v1.KMSProviderTypeVault, Vault:
&v1.VaultKMSConfig{KMSPluginImage: "...", VaultAddress: "...", VaultNamespace:
"...", TransitMount: "...", TransitKey: "..."}}} so the Vault-specific fields
live under Vault (v1.VaultKMSConfig) rather than directly on
state.KMSProviderConfig; apply the same change to the other occurrence
referenced in the comment.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 366-374: The tests in key_controller_test.go only assert the KMS
provider-config equals an empty JSON object by marshalling
state.KMSProviderConfig{}, which misses regressions; add at least one test case
where the APIServer fixture's Spec.Encryption.KMS is populated with non-empty
fields, ensure the controller writes that non-empty provider config into
actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"],
decode/unmarshal the secret bytes back into the same struct type
(state.KMSProviderConfig) and assert field-level equality (roundtrip) rather
than string equality to catch reshaping; update the analogous checks at the
other noted ranges (435-443, 520-528, 722-778, 809-816) to include non-empty
scenarios and roundtrip assertions.
🪄 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: 465b5ca6-2647-4900-83fe-e55592b3532a
⛔ Files ignored due to path filters (43)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois 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_dns.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/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/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/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**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/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (8)
go.modpkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/encryption/testing/helpers.go
- pkg/operator/encryption/secrets/secrets.go
| sigs.k8s.io/structured-merge-diff/v6 v6.3.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/openshift/api => github.com/flavianmissi/api v0.0.0-20260424105406-3d4bd99d6aa0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the go.mod file around line 142
head -150 go.mod | tail -20Repository: openshift/library-go
Length of output: 1082
🏁 Script executed:
# Also get the full replace directives to understand the context
grep -n "^replace" go.modRepository: openshift/library-go
Length of output: 167
🏁 Script executed:
# Check if there are any comments or documentation about why this fork is used
rg -i "flavianmissi|openshift/api" --type go --type md -C 2Repository: openshift/library-go
Length of output: 50377
🏁 Script executed:
# Check git history to see if there's context about this change
git log --oneline -20 -- go.mod 2>/dev/null || echo "Git history not available"Repository: openshift/library-go
Length of output: 106
🏁 Script executed:
# Check if there are any comments or documentation about the Vault KMS change
rg -i "vault|kms|flavianmissi" --type go --type md -B 2 -A 2 | head -50Repository: openshift/library-go
Length of output: 3111
🏁 Script executed:
# Check recent PR or commit context
git log --oneline --all | grep -i "vault\|kms\|api\|flavian" | head -20Repository: openshift/library-go
Length of output: 1342
Remove the fork override or pin it to a specific upstream commit.
The replace directive redirects github.com/openshift/api to a personal fork (github.com/flavianmissi/api), which introduces supply-chain and reproducibility risk in a shared branch. While the version is pinned to a pseudoversion rather than floating master, the use of a personal fork as a core API dependency requires clear justification. The commit message "Fake bump o/api to get Vault KMS" suggests this is exploratory work that shouldn't be merged. Either use an upstream github.com/openshift/api commit/tag or keep such overrides local and uncommitted during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 142, The go.mod replace directive "replace
github.com/openshift/api => github.com/flavianmissi/api
v0.0.0-20260424105406-3d4bd99d6aa0" must be removed or replaced with an upstream
commit/tag to avoid using a personal fork; edit the replace line in go.mod (the
replace directive) to either delete it entirely or point to an official
github.com/openshift/api version (a specific tag or commit hash) and re-run go
mod tidy to update go.sum, or keep the forked replace only in a local,
uncommitted workspace if this change is exploratory.
Member
Author
|
/retitle WIP: CNTRLPLANE-3237: Introduce KMSProviderConfig in KeyState |
openshift-ci
Bot
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
ardaguclu
force-pushed
the
kms-provider-config-keystate
branch
3 times, most recently
from
April 28, 2026 13:17
20ff99b to
eb6d1f3
Compare
Member
Author
|
/retitle CNTRLPLANE-3237: Introduce KMSProviderConfig in KeyState |
Member
Author
|
/hold cancel |
openshift-ci
Bot
changed the title
openshift-ci
Bot
removed
do-not-merge/work-in-progress
ardaguclu
force-pushed
the
kms-provider-config-keystate
branch
from
April 28, 2026 13:20
eb6d1f3 to
80735b3
Compare
p0lyn0mial
reviewed
Apr 28, 2026
| @@ -0,0 +1,284 @@ | |||
| // TODO: This API definitions are copied from https://github.com/openshift/api/pull/2805 | |||
Contributor
There was a problem hiding this comment.
nit: could we create a new api pkg for it ? for example pkg/operator/encryption/api ?
Member
Author
There was a problem hiding this comment.
It would be indeed better
p0lyn0mial
reviewed
Apr 29, 2026
| } | ||
|
|
||
| func (c *keyController) getCurrentModeAndExternalReason(ctx context.Context) (state.Mode, string, error) { | ||
| func (c *keyController) getCurrentModeAndExternalReason(ctx context.Context) (state.Mode, string, *v1.APIServerEncryption, error) { |
Contributor
There was a problem hiding this comment.
getCurrentModeReasonAndEncryptionConfig ?
p0lyn0mial
reviewed
Apr 29, 2026
| return "", "", nil, err | ||
| } | ||
|
|
||
| encryption := &apiServer.Spec.Encryption |
Contributor
There was a problem hiding this comment.
do we have to return a pointer ?
Member
Author
There was a problem hiding this comment.
No we don't have to. Updated.
p0lyn0mial
reviewed
Apr 29, 2026
| reason := encryptionConfig.Encryption.Reason | ||
| switch currentMode := state.Mode(apiServer.Spec.Encryption.Type); currentMode { | ||
| switch currentMode := state.Mode(encryption.Type); currentMode { | ||
| case state.AESCBC, state.AESGCM, state.KMS, state.Identity: // secretbox is disabled for now |
Contributor
There was a problem hiding this comment.
could we check encryption.KMS for nil here ?
p0lyn0mial
reviewed
Apr 29, 2026
| ExternalReason: externalReason, | ||
| } | ||
| if currentMode == state.KMS { | ||
| if apiServerEncryption == nil || apiServerEncryption.KMS == nil { |
Contributor
There was a problem hiding this comment.
then we could drop this checks.
p0lyn0mial
reviewed
Apr 29, 2026
|
|
||
| func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext factory.SyncContext, encryptedGRs []schema.GroupResource) error { | ||
| currentMode, externalReason, err := c.getCurrentModeAndExternalReason(ctx) | ||
| currentMode, externalReason, apiServerEncryption, err := c.getCurrentModeAndExternalReason(ctx) |
Contributor
There was a problem hiding this comment.
encryptionConfiguration ?
Contributor
There was a problem hiding this comment.
apiEncryptionConfiguration ?
Member
Author
There was a problem hiding this comment.
Updated for apiEncryptionConfiguration
ardaguclu
force-pushed
the
kms-provider-config-keystate
branch
from
April 29, 2026 12:58
8bb5034 to
87b5035
Compare
Member
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
ardaguclu
force-pushed
the
kms-provider-config-keystate
branch
from
April 29, 2026 13:14
87b5035 to
db7cc6f
Compare
bertinatto
reviewed
Apr 29, 2026
| if err := json.Unmarshal(v, providerConfig); err != nil { | ||
| return state.KeyState{}, fmt.Errorf("secret %s/%s has invalid %s data: %w", s.Namespace, s.Name, EncryptionSecretKMSProviderConfig, err) | ||
| } | ||
| // KMSConfig is non-nil here; initialized by the encryption config branch above |
Member
There was a problem hiding this comment.
If the secret has a EncryptionSecretKMSProviderConfig key, but lacks a EncryptionSecretKMSEncryptionConfig key, then KMSConfig could be nil (and we'd panic here).
Probably not something that can happen, but maybe we can be defensive about it and initialize key.KMSConfig before the if-else clauses - and add a comment that the allocation would be wasted if the secret doesn't have the KMS keys (which is fine).
Member
There was a problem hiding this comment.
Something like:
diff --git a/pkg/operator/encryption/secrets/secrets.go b/pkg/operator/encryption/secrets/secrets.go
index f16198107..aa0fa417d 100644
--- a/pkg/operator/encryption/secrets/secrets.go
+++ b/pkg/operator/encryption/secrets/secrets.go
@@ -64,12 +64,14 @@ func ToKeyState(s *corev1.Secret) (state.KeyState, error) {
case state.AESCBC, state.AESGCM, state.SecretBox, state.Identity:
key.Mode = keyMode
case state.KMS:
+ // This allocation will be wasted if KMS keys are not present in the secret, but that should not happen
+ key.KMSConfig = &state.KMSConfig{}
if v, ok := s.Data[EncryptionSecretKMSEncryptionConfig]; ok && len(v) > 0 {
kmsConfiguration := &apiserverconfigv1.KMSConfiguration{}
if err := json.Unmarshal(v, kmsConfiguration); err != nil {
return state.KeyState{}, fmt.Errorf("secret %s/%s has invalid %s data: %w", s.Namespace, s.Name, EncryptionSecretKMSEncryptionConfig, err)
}
- key.KMSConfig = &state.KMSConfig{Encryption: kmsConfiguration}
+ key.KMSConfig.Encryption = kmsConfiguration
} else {
// encryption.apiserver.operator.openshift.io-kms-encryption-config data field is required for KMS
// encryption mode.
@@ -80,7 +82,6 @@ func ToKeyState(s *corev1.Secret) (state.KeyState, error) {
if err := json.Unmarshal(v, providerConfig); err != nil {
return state.KeyState{}, fmt.Errorf("secret %s/%s has invalid %s data: %w", s.Namespace, s.Name, EncryptionSecretKMSProviderConfig, err)
}
- // KMSConfig is non-nil here; initialized by the encryption config branch above
key.KMSConfig.Provider = providerConfig
} else {
// encryption.apiserver.operator.openshift.io-kms-provider-config data field is required for KMS
Member
Author
There was a problem hiding this comment.
That would be better. I'll make changes after https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_library-go/2186/pull-ci-openshift-library-go-master-e2e-aws-encryption/2049479863575777280 finishes
bertinatto
reviewed
Apr 29, 2026
| if ks.HasKMSProvider() { | ||
| providerJSON, err := json.Marshal(ks.KMSConfig.Provider) | ||
| if err != nil { | ||
| return nil, err |
Member
There was a problem hiding this comment.
nit: we could annotate the error here and on the check above (for the EncryptionConfiguration) to say what exactly failed to be serialized
bertinatto
reviewed
Apr 29, 2026
|
|
||
| if ks.Mode == state.KMS && (!ks.HasKMSEncryption() || !ks.HasKMSProvider()) { | ||
| return nil, fmt.Errorf("%s or %s can not be empty, when mode is KMS", EncryptionSecretKMSEncryptionConfig, EncryptionSecretKMSProviderConfig) | ||
| } |
Member
There was a problem hiding this comment.
nit: we could move this up and avoid some processing if we're in a bad state
Member
Author
There was a problem hiding this comment.
Updated. Please let me know about the state.
bertinatto
reviewed
Apr 29, 2026
| } | ||
|
|
||
| func (c *keyController) getCurrentModeAndExternalReason(ctx context.Context) (state.Mode, string, error) { | ||
| func (c *keyController) getCurrentModeReasonAndEncryptionConfig(ctx context.Context) (state.Mode, string, v1.APIServerEncryption, error) { |
Member
There was a problem hiding this comment.
do we have a reason for this not to be a pointer?
ardaguclu
force-pushed
the
kms-provider-config-keystate
branch
from
April 29, 2026 14:44
2c40672 to
635b6b7
Compare
Contributor
|
/lgtm |
Member
Author
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, bertinatto, p0lyn0mial 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
Contributor
|
@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. |
Member
Author
|
It failed but this requires changes in test step. I will fix it in a followup PR |
openshift-ci
Bot
removed
the
do-not-merge/hold
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
4 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.
This PR solely focuses on populating the kms-provider-config into Key Secret as well as
KeyStatefor the work that is described openshift/enhancements#1960Currently kms-provider-config is not wired to encryption-config Secret that to be used to carry the data to plugin lifecycle.
Summary by CodeRabbit
New Features
Tests