CNTRLPLANE-3237: Carry referenced Secret data from API to plugin lifecycle for KMS#2212
CNTRLPLANE-3237: Carry referenced Secret data from API to plugin lifecycle for KMS#2212ardaguclu wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@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:
WalkthroughThe PR adds KMS plugin secret data fetching and propagation to the encryption operator. The controller now retrieves referenced secrets from the ChangesKMS Secret Data State and Configuration
KMS Plugin Secret Fetching and Validation
Testing and Validation
Sequence DiagramsequenceDiagram
participant Ctrl as Key Controller
participant API as Kubernetes API<br/>(openshift-config)
participant KMS as KMS State
participant Config as Encryption Config
participant Secret as Encryption Secret
Ctrl->>Ctrl: checkAndCreateKeys
Ctrl->>Ctrl: generateKeySecret(ctx, mode=KMS)
Ctrl->>API: GET vault-approle-secret<br/>(from openshift-config)
API-->>Ctrl: Secret with role-id, secret-id
Ctrl->>Ctrl: Validate required keys present
Ctrl->>KMS: Populate KMS.SecretData<br/>[role-id, secret-id]
Ctrl->>Secret: FromKeyState(ks)
Secret->>Secret: Write secret data with<br/>kms-plugin-secret- prefix
Secret->>Config: Config includes KMSSecretData
Config->>Config: Round-trip validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR involves coordinated changes across multiple layers (controller fetching, state storage, config serialization) with new validation logic in the controller and helpers. The heterogeneous changes span state definitions, serialization logic, controller operations, and comprehensive test updates, requiring careful verification of the round-tripping behavior and KMS integration points. Suggested labels
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ardaguclu 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 |
|
/uncc @dgrisonnet |
openshift-ci
Bot
requested review from
bertinatto
and removed request for
dgrisonnet
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/encryption/controllers/key_controller.go (1)
231-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the actual create error in warning log
At Line 236, the warning logs
err, but the failure being handled iscreateErr. This can log<nil>or the wrong error, which hurts debugging.💡 Suggested fix
- syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, err) + syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, createErr)🤖 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/key_controller.go` around lines 231 - 237, The warning log after attempting to create the secret uses the wrong error variable (`err`) instead of the actual creation error (`createErr`); update the error passed to syncContext.Recorder().Warningf in the code handling c.secretClient.Secrets(...).Create (and the surrounding block where createErr is checked) so it reports createErr (e.g., replace the err argument with createErr) and ensure the returned error is createErr.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/key_controller_test.go (1)
333-406: ⚡ Quick winAdd a KMS credential failure-path scenario
Please add one test case where the referenced AppRole secret is missing or missing required keys, and assert degraded/error behavior. The PR changes now hard-depend on this read path, so guarding that path in unit tests would prevent regressions.
Also applies to: 422-435, 517-530
🤖 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/key_controller_test.go` around lines 333 - 406, Add a new table-driven test case in key_controller_test.go alongside the existing KMS-success case that simulates a missing/invalid AppRole secret: create a test entry whose initialObjects omits encryptiontesting.CreateVaultAppRoleSecret (or includes a secret missing the "role-id"/"secret-id" keys), set apiServerObjects to apiServerWithKMS, and set expectedActions to include any attempted gets but not a successful create of the managed secret; in the validateFunc for that case assert that the controller records degraded/error behavior (e.g., no create:secrets action, presence of an event creation action like "create:events:kms" or a status update indicating degradation) and that the code path handling credentials (the blocks that read actualSecret.Data keys and annotations) is not reached or reports the appropriate error; mirror this pattern for the other referenced ranges (422-435, 517-530).
🤖 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 `@test/e2e-encryption/encryption_test.go`:
- Around line 490-503: The test currently hard-codes "vault-approle-secret"
which can collide across parallel runs; change the secret name to a unique
variable (e.g. secretName := fmt.Sprintf("%s-vault-approle-secret-%s",
component, util.RandomSuffix()) or similar) and use that variable everywhere
instead of the string literal: in the kubeClient.CoreV1().Secrets(...).Create
call, the deferred Delete call, the Patch payload passed to
fakeApiServerClient.Patch (the JSON must reference the variable value for
"name"), and any later assertions that reference the credential name; update the
code around the Create/Deferred Delete and the Patch invocation (and the other
occurrences noted in the review) to interpolate the new secretName variable so
tests are isolated.
---
Outside diff comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 231-237: The warning log after attempting to create the secret
uses the wrong error variable (`err`) instead of the actual creation error
(`createErr`); update the error passed to syncContext.Recorder().Warningf in the
code handling c.secretClient.Secrets(...).Create (and the surrounding block
where createErr is checked) so it reports createErr (e.g., replace the err
argument with createErr) and ensure the returned error is createErr.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 333-406: Add a new table-driven test case in
key_controller_test.go alongside the existing KMS-success case that simulates a
missing/invalid AppRole secret: create a test entry whose initialObjects omits
encryptiontesting.CreateVaultAppRoleSecret (or includes a secret missing the
"role-id"/"secret-id" keys), set apiServerObjects to apiServerWithKMS, and set
expectedActions to include any attempted gets but not a successful create of the
managed secret; in the validateFunc for that case assert that the controller
records degraded/error behavior (e.g., no create:secrets action, presence of an
event creation action like "create:events:kms" or a status update indicating
degradation) and that the code path handling credentials (the blocks that read
actualSecret.Data keys and annotations) is not reached or reports the
appropriate error; mirror this pattern for the other referenced ranges (422-435,
517-530).
🪄 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: f17da15d-8491-4079-ad4e-9411ca9a23b9
📒 Files selected for processing (13)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/config_test.gopkg/operator/encryption/encryptiondata/secret.gopkg/operator/encryption/kms/credentials.gopkg/operator/encryption/kms/credentials_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.gotest/e2e-encryption/encryption_test.go
| t.Logf("Create vault AppRole credential secret") | ||
| _, err = kubeClient.CoreV1().Secrets("openshift-config").Create(ctx, &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "vault-approle-secret", Namespace: "openshift-config"}, | ||
| Data: map[string][]byte{ | ||
| "role-id": []byte("test-role-id"), | ||
| "secret-id": []byte("test-secret-id"), | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| }, metav1.CreateOptions{}) | ||
| require.NoError(t, err) | ||
| defer kubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, "vault-approle-secret", metav1.DeleteOptions{}) | ||
|
|
||
| t.Logf("Switch to KMS") | ||
| _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"type":"KMS","kms":{"type":"Vault","vault":{"kmsPluginImage":"registry.example.com/kms-plugin@sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890","vaultAddress":"https://vault.example.com","authentication":{"type":"AppRole","appRole":{"secret":{"name":"vault-approle-secret"}}},"transitKey":"test-transit-key"}}}}}`), metav1.PatchOptions{}) |
There was a problem hiding this comment.
Avoid hard-coded AppRole secret name in e2e flow
Using a global static name (vault-approle-secret) risks collisions across parallel runs or leftover state in shared clusters. This can produce flaky failures unrelated to the feature.
Consider deriving a unique name from component, and reusing that variable in:
- secret creation,
- APIServer KMS patches,
- credential key assertions.
💡 Suggested direction
+ vaultSecretName := fmt.Sprintf("vault-approle-secret-%s", component)
- ObjectMeta: metav1.ObjectMeta{Name: "vault-approle-secret", Namespace: "openshift-config"},
+ ObjectMeta: metav1.ObjectMeta{Name: vaultSecretName, Namespace: "openshift-config"},
- defer kubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, "vault-approle-secret", metav1.DeleteOptions{})
+ defer kubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, vaultSecretName, metav1.DeleteOptions{})
- ... "appRole":{"secret":{"name":"vault-approle-secret"}} ...
+ ... fmt.Sprintf(`... "appRole":{"secret":{"name":"%s"}} ...`, vaultSecretName) ...
- secrets.EncryptionSecretKMSCredentialPrefix+"-vault-approle-secret-role-id"
+ secrets.EncryptionSecretKMSCredentialPrefix+"-"+vaultSecretName+"-role-id"Also applies to: 543-543, 572-572, 338-339
🤖 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 `@test/e2e-encryption/encryption_test.go` around lines 490 - 503, The test
currently hard-codes "vault-approle-secret" which can collide across parallel
runs; change the secret name to a unique variable (e.g. secretName :=
fmt.Sprintf("%s-vault-approle-secret-%s", component, util.RandomSuffix()) or
similar) and use that variable everywhere instead of the string literal: in the
kubeClient.CoreV1().Secrets(...).Create call, the deferred Delete call, the
Patch payload passed to fakeApiServerClient.Patch (the JSON must reference the
variable value for "name"), and any later assertions that reference the
credential name; update the code around the Create/Deferred Delete and the Patch
invocation (and the other occurrences noted in the review) to interpolate the
new secretName variable so tests are isolated.
ardaguclu
force-pushed
the
carry-kms-credentials
branch
2 times, most recently
from
36a0adb to
ca04a82
Compare
|
/retest |
ardaguclu
force-pushed
the
carry-kms-credentials
branch
from
ca04a82 to
715692d
Compare
|
/retitle CNTRLPLANE-3237: Carry KMS credentials from referenced Secret |
openshift-ci
Bot
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| credSecret, err := secretClient.Secrets("openshift-config").Get(ctx, secretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get credential secret %s in openshift-config: %w", secretName, err) | ||
| } |
There was a problem hiding this comment.
instead of returning an error right away, should we give the user time to create the secret, once they set the KMS fields in the api?
There was a problem hiding this comment.
I think, it is prerequisite for cluster-admin to create secret beforehand. We should degrade if it is not found. This error blocks the new key creation. So this won't impact on cluster's stability.
| value, ok := credSecret.Data[key] | ||
| if !ok { | ||
| return nil, fmt.Errorf("credential secret %s in openshift-config is missing required key %q", secretName, key) | ||
| } |
There was a problem hiding this comment.
yeah, if the secret is malformed, we should probably error out
| if !ok { | ||
| return nil, fmt.Errorf("credential secret %s in openshift-config is missing required key %q", secretName, key) | ||
| } | ||
| credentials[secrets.EncryptionSecretKMSCredentialPrefix+"-"+secretName+"-"+key] = string(value) |
There was a problem hiding this comment.
the credentials secret is not subject to the validation done at admission time, like the KMS fields in the cluster/apiserver CR. I think we should probably validate that the user is setting valid values for these 2 secret keys, wdyt?
There was a problem hiding this comment.
Key controller can not have semantic knowledge about what values should be. Preflight checker will validate this.
| } | ||
|
|
||
| func (k *KeyState) HasKMSCredentials() bool { | ||
| return k != nil && k.KMS != nil && len(k.KMS.Credentials) > 0 |
There was a problem hiding this comment.
for vault this would actually need to be 2, but I think it's fine as is because you're currently checking that in fetchVaultCredentials
There was a problem hiding this comment.
yeah, I wanted to keep here generic.
| return nil, fmt.Errorf("vault AppRole secret reference name is empty") | ||
| } | ||
|
|
||
| credSecret, err := secretClient.Secrets("openshift-config").Get(ctx, secretName, metav1.GetOptions{}) |
There was a problem hiding this comment.
this secret shouldn't change a lot, so I wonder if we should use a cached client (lister) instead
There was a problem hiding this comment.
Good idea. Let me update it.
There was a problem hiding this comment.
However, this requires adding openshift-config to the key controller's informer watch and store SecretLister in keyController struct. If you are ok with this signature change, I can update.
There was a problem hiding this comment.
I think we agreed that we will use secretClient?
| } | ||
| return short | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: maybe keep these functions together with their plugin-config counterparts in helpers.go
p0lyn0mial
left a comment
p0lyn0mial
left a comment
There was a problem hiding this comment.
I did a first pass and left a few comments. Let me know what you think. Thanks.
| // Credentials stores fetched credential values from the referenced secret | ||
| // in openshift-config. Keys use the format | ||
| // "encryption.apiserver.operator.openshift.io-kms-secret-{secretName}-{keyName}". | ||
| Credentials map[string]string |
There was a problem hiding this comment.
wait, didn't we want to store Secrets/ConfigMap data fields ? xref: https://github.com/openshift/enhancements/blob/master/enhancements/kube-apiserver/kms-encryption-foundations.md#proposal
There was a problem hiding this comment.
That is correct. Credentials store key-value map in KMSState which will be converted to each data field.
| // Credentials stores fetched credential values from the referenced secret | ||
| // in openshift-config. Keys use the format | ||
| // "encryption.apiserver.operator.openshift.io-kms-secret-{secretName}-{keyName}". | ||
| Credentials map[string]string |
There was a problem hiding this comment.
why not to make it more generic:
SecretData map[string][]byte
| Plugin: apiServerEncryption.KMS, | ||
| } | ||
|
|
||
| credentials, err := kms.FetchCredentials(ctx, c.secretClient, apiServerEncryption.KMS) |
There was a problem hiding this comment.
by making the data struct more generic e.g. SecretData we make this code more generic too:
if secretName := referencedSecretName(apiServerEncryption.KMS); len(secretName) > 0 {
refSecret, err := c.secretClient.Secrets("openshift-config").Get(ctx, secretName, metav1.GetOptions{})
...
sd := make(map[string][]byte, len(refSecret.Data))
for k, v := range refSecret.Data {
sd[k] = v
}
ks.KMS.SecretData = sd
}
There was a problem hiding this comment.
where referencedSecretName could be private and simple:
func referencedSecretName(plugin configv1.KMSPluginConfig) string {
switch plugin.Type {
case configv1.VaultKMSProvider:
return plugin.Vault.Authentication.AppRole.Secret.Name
default:
return ""
}
}
There was a problem hiding this comment.
do we already have a const for "openshift-config" namespace ?
There was a problem hiding this comment.
Updated with one difference. We can not carry all the data fields in the given Secret. We need to carry only the selective set of data keys. So referencedSecretName function will also return an array for this. Please let me know your thoughts.
| @@ -0,0 +1,90 @@ | |||
| package kms | |||
There was a problem hiding this comment.
mhm, not sure if we need a new file, seems all methods could be private and close to the source.
| // KMSCredentials maps keyID to credential data carried from | ||
| // Key Secrets into the encryption-config Secret. | ||
| // Inner map keys use the short form (e.g. "app-role-role-id"). | ||
| KMSCredentials map[string]map[string]string |
There was a problem hiding this comment.
KMSSecretData map[string]map[string][]byte
| // EncryptionSecretKMSCredentialPrefix is the data field key prefix for credential values | ||
| // fetched from the referenced secret in openshift-config. The full data key is | ||
| // constructed by appending "-{secretName}-{dataKey}" to this prefix. | ||
| EncryptionSecretKMSCredentialPrefix = "encryption.apiserver.operator.openshift.io-kms-secret" |
There was a problem hiding this comment.
maybe this should be .....kms-plugin-secret" to align with "encryption.apiserver.operator.openshift.io-kms-plugin-config"
| Plugin: apiServerEncryption.KMS, | ||
| } | ||
|
|
||
| credentials, err := kms.FetchCredentials(ctx, c.secretClient, apiServerEncryption.KMS) |
There was a problem hiding this comment.
it seems like we miss some dynamic validation, for example we should go degraded when the src secret is removed but referenced.
There was a problem hiding this comment.
Tracking the current secret references, keeping their content up-to-date, degrading if they are removed will be handled in another PR. This PR only focuses on carrying this data. If Secret is not present, or one one of the selected set of data fields are missing, we will return error (I assume that means we'll degrade).
| Plugin: apiServerEncryption.KMS, | ||
| } | ||
|
|
||
| credentials, err := kms.FetchCredentials(ctx, c.secretClient, apiServerEncryption.KMS) |
There was a problem hiding this comment.
seems like we miss an informer that would trigger the sync loop.
There was a problem hiding this comment.
Added a TODO for informer.
| return state.KeyState{}, fmt.Errorf("%s can not be empty, when mode is KMS", EncryptionSecretKMSPluginConfig) | ||
| } | ||
| for dataKey, value := range s.Data { | ||
| if strings.HasPrefix(dataKey, EncryptionSecretKMSCredentialPrefix) { |
There was a problem hiding this comment.
i'd like to experiment a bit with how we store data here, e.g. with our without the prefix.
There was a problem hiding this comment.
Sounds good to me. Thanks for spending time on this and review.
ardaguclu
force-pushed
the
carry-kms-credentials
branch
from
715692d to
c10b32e
Compare
|
/retitle CNTRLPLANE-3237: Carry referenced Secret data from API to plugin lifecycle for KMS |
openshift-ci
Bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/operator/encryption/secrets/types.go (1)
60-64:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove trailing dash from the constant for consistency.
The constant
EncryptionSecretKMSSecretDataPrefixhas a trailing dash ("...-kms-plugin-secret-"), but the comment on line 63 states "appending-{secretName}-{dataKey}" (implying a leading dash should be added during concatenation). However, the actual usage insecrets.goline 173 directly concatenatesprefix + dataKeywithout adding an intermediate dash.This creates confusion:
- If the prefix keeps its trailing dash and code does direct concatenation, the final key has only one dash after "secret" (e.g.,
"...-kms-plugin-secret-vault-approle-secret-role-id").- But the test on line 338 of
encryption_test.goadds an explicit dash (prefix + "-vault-approle-secret-role-id"), expecting a double dash.For consistency with
EncryptionSecretKMSPluginConfig(line 59, no trailing dash) and to match the past review suggestion, remove the trailing dash from the constant and update the concatenation sites to add dashes explicitly as needed.Proposed fix
- EncryptionSecretKMSSecretDataPrefix = "encryption.apiserver.operator.openshift.io-kms-plugin-secret-" + EncryptionSecretKMSSecretDataPrefix = "encryption.apiserver.operator.openshift.io-kms-plugin-secret"Then update usage in
secrets.goline 173 to:- s.Data[EncryptionSecretKMSSecretDataPrefix+dataKey] = value + s.Data[EncryptionSecretKMSSecretDataPrefix+"-"+dataKey] = valueAnd in
secrets.goline 92 to:- if rawKey, found := strings.CutPrefix(dataKey, EncryptionSecretKMSSecretDataPrefix); found && len(rawKey) > 0 { + if rawKey, found := strings.CutPrefix(dataKey, EncryptionSecretKMSSecretDataPrefix+"-"); found && len(rawKey) > 0 {And fix the comment on line 63 to:
-// constructed by appending "-{secretName}-{dataKey}" to this prefix. +// constructed by appending "-{secretName}-{dataKey}" after this prefix (with a separator dash).As per coding guidelines from past review comment: "maybe this should be
.....kms-plugin-secret"to align with"encryption.apiserver.operator.openshift.io-kms-plugin-config""🤖 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/secrets/types.go` around lines 60 - 64, Remove the trailing dash from the EncryptionSecretKMSSecretDataPrefix constant value (make it "...-kms-plugin-secret") and update all call sites that currently do direct concatenation with that prefix (e.g., where code builds keys by using EncryptionSecretKMSSecretDataPrefix + dataKey in secrets.go) to explicitly insert the needed dash(s) when concatenating (e.g., prefix + "-" + secretName + "-" + dataKey or prefix + "-" + dataKey as appropriate). Also update the comment above EncryptionSecretKMSSecretDataPrefix to describe that callers must append "-{secretName}-{dataKey}", and ensure tests that build expected keys are changed to use the new explicit-dash concatenation rather than relying on a trailing dash in the constant; keep EncryptionSecretKMSPluginConfig unchanged.
🧹 Nitpick comments (2)
pkg/operator/encryption/controllers/key_controller.go (1)
287-291: 💤 Low valueConsider using a constant for the openshift-config namespace.
The code hardcodes the
"openshift-config"namespace string. For consistency and maintainability, consider defining a constant (e.g.,OpenshiftConfigNamespace) that can be reused across the codebase.Example pattern
const OpenshiftConfigNamespace = "openshift-config" // Then use: refSecret, err := c.secretClient.Secrets(OpenshiftConfigNamespace).Get(ctx, secretName, metav1.GetOptions{})As per past review comment: "do we already have a const for
"openshift-config"namespace ?"🤖 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/key_controller.go` around lines 287 - 291, The code currently hardcodes "openshift-config" when calling c.secretClient.Secrets(...) inside the referencedSecretName(apiServerEncryption.KMS) handling; define a package-level constant (e.g., const OpenshiftConfigNamespace = "openshift-config") and replace the literal with that constant so calls like c.secretClient.Secrets(OpenshiftConfigNamespace).Get(...) use the new constant; update any other occurrences in this file to use the same constant for consistency.test/e2e-encryption/encryption_test.go (1)
490-500: ⚡ Quick winConsider using a unique secret name to avoid test collisions.
The test creates a Secret with the hardcoded name
"vault-approle-secret"in theopenshift-confignamespace. If multiple test runs execute in parallel against the same cluster, this could cause collisions or flaky failures due to leftover state.Consider generating a unique name (e.g., including the
componentvariable or a random suffix) and using that name consistently throughout the test—in the Secret creation, the APIServer KMS patch payload, and the credential key assertions.Example approach
+ vaultSecretName := fmt.Sprintf("vault-approle-secret-%s", component) t.Logf("Create vault AppRole vault AppRole secret") _, err = kubeClient.CoreV1().Secrets("openshift-config").Create(ctx, &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "vault-approle-secret", Namespace: "openshift-config"}, + ObjectMeta: metav1.ObjectMeta{Name: vaultSecretName, Namespace: "openshift-config"}, Data: map[string][]byte{ "role-id": []byte("test-role-id"), "secret-id": []byte("test-secret-id"), }, Type: corev1.SecretTypeOpaque, }, metav1.CreateOptions{}) require.NoError(t, err) - defer kubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, "vault-approle-secret", metav1.DeleteOptions{}) + defer kubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, vaultSecretName, metav1.DeleteOptions{})Then update the Patch payload on line 503 to interpolate
vaultSecretNameinstead of the hardcoded string, and adjust lines 332-333 and 338-339 to use the dynamic name.As per past review comment: "Using a global static name (
vault-approle-secret) risks collisions across parallel runs or leftover state in shared clusters."🤖 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 `@test/e2e-encryption/encryption_test.go` around lines 490 - 500, The test uses a hardcoded Secret name "vault-approle-secret" which can collide across parallel runs; replace that literal with a generated unique name (e.g., vaultSecretName := fmt.Sprintf("vault-approle-%s-%d", component, rand.Intn(100000))) and use vaultSecretName when calling kubeClient.CoreV1().Secrets(...).Create and .Delete, update the APIServer KMS patch payload to interpolate vaultSecretName instead of the static string, and update the credential/key assertions to reference vaultSecretName so all uses of the secret are consistent and unique per test run.
🤖 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/controllers/key_controller.go`:
- Line 121: The informer setup currently only watches
"openshift-config-managed"; add an informer for the "openshift-config" namespace
so KMS plugin Secrets updates trigger reconciliation. Specifically, call
kubeInformersForNamespaces.InformersFor("openshift-config").Core().V1().Secrets().Informer()
and include that informer alongside the existing
kubeInformersForNamespaces.InformersFor("openshift-config-managed").Core().V1().Secrets().Informer()
(the same place where shared informers are assembled/started), ensuring the new
informer is registered with the controller and started so updates to Secrets in
openshift-config will enqueue reconciliations for the key controller.
In `@test/e2e-encryption/encryption_test.go`:
- Around line 338-339: The test builds the map key with an extra "-" causing a
double-dash mismatch with how the code stores secrets; update the assertions to
use the same concatenation as the production code by removing the explicit dash
so they read
keySecret.Data[secrets.EncryptionSecretKMSSecretDataPrefix+"vault-approle-secret-role-id"]
and
keySecret.Data[secrets.EncryptionSecretKMSSecretDataPrefix+"vault-approle-secret-secret-id"]
(i.e. match the s.Data[EncryptionSecretKMSSecretDataPrefix+dataKey] behavior),
keeping the existing keyID usage in the error messages.
---
Duplicate comments:
In `@pkg/operator/encryption/secrets/types.go`:
- Around line 60-64: Remove the trailing dash from the
EncryptionSecretKMSSecretDataPrefix constant value (make it
"...-kms-plugin-secret") and update all call sites that currently do direct
concatenation with that prefix (e.g., where code builds keys by using
EncryptionSecretKMSSecretDataPrefix + dataKey in secrets.go) to explicitly
insert the needed dash(s) when concatenating (e.g., prefix + "-" + secretName +
"-" + dataKey or prefix + "-" + dataKey as appropriate). Also update the comment
above EncryptionSecretKMSSecretDataPrefix to describe that callers must append
"-{secretName}-{dataKey}", and ensure tests that build expected keys are changed
to use the new explicit-dash concatenation rather than relying on a trailing
dash in the constant; keep EncryptionSecretKMSPluginConfig unchanged.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 287-291: The code currently hardcodes "openshift-config" when
calling c.secretClient.Secrets(...) inside the
referencedSecretName(apiServerEncryption.KMS) handling; define a package-level
constant (e.g., const OpenshiftConfigNamespace = "openshift-config") and replace
the literal with that constant so calls like
c.secretClient.Secrets(OpenshiftConfigNamespace).Get(...) use the new constant;
update any other occurrences in this file to use the same constant for
consistency.
In `@test/e2e-encryption/encryption_test.go`:
- Around line 490-500: The test uses a hardcoded Secret name
"vault-approle-secret" which can collide across parallel runs; replace that
literal with a generated unique name (e.g., vaultSecretName :=
fmt.Sprintf("vault-approle-%s-%d", component, rand.Intn(100000))) and use
vaultSecretName when calling kubeClient.CoreV1().Secrets(...).Create and
.Delete, update the APIServer KMS patch payload to interpolate vaultSecretName
instead of the static string, and update the credential/key assertions to
reference vaultSecretName so all uses of the secret are consistent and unique
per test run.
🪄 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: e45cd9d6-1c06-461d-b26a-99e572d57f4f
📒 Files selected for processing (11)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/config_test.gopkg/operator/encryption/encryptiondata/secret.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.gotest/e2e-encryption/encryption_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/encryption/encryptiondata/config_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/encryption/testing/helpers.go
- pkg/operator/encryption/secrets/secrets_test.go
- pkg/operator/encryption/controllers/key_controller_test.go
ardaguclu
force-pushed
the
carry-kms-credentials
branch
from
c10b32e to
9309879
Compare
p0lyn0mial
left a comment
p0lyn0mial
left a comment
There was a problem hiding this comment.
did another pass, haven't finished. seems like there is a potential of a collision when there is more than one secret in the future ....
| if !ok { | ||
| return nil, fmt.Errorf("secret %s in openshift-config is missing required key %q", secretName, key) | ||
| } | ||
| sd[secretName+"-"+key] = v |
There was a problem hiding this comment.
what if secretName already contains - ? is this a problem ?
what if key already contains - ? is this a problem ?
There was a problem hiding this comment.
From the encryption controller's perspective this is not a problem. It concatenates secretName and key and propagates to the plugin lifecycle as is.
But this collision can be a problem in plugin lifecycle side. I'd like to discuss more.
| if !ok { | ||
| return nil, fmt.Errorf("secret %s in openshift-config is missing required key %q", secretName, key) | ||
| } | ||
| sd[secretName+"-"+key] = v |
There was a problem hiding this comment.
what if:
secretName="vault-approle", key="secret-role-id" gives "vault-approle-secret-role-id"
secretName="vault-approle-secret", key="role-id" gives "vault-approle-secret-role-id"
There was a problem hiding this comment.
in the future we might ref more than one secret.
| func referencedSecretName(plugin configv1.KMSPluginConfig) (string, []string) { | ||
| switch plugin.Type { | ||
| case configv1.VaultKMSProvider: | ||
| return plugin.Vault.Authentication.AppRole.Secret.Name, []string{"role-id", "secret-id"} |
There was a problem hiding this comment.
it is important that we document that we only care respect []string{"role-id", "secret-id"}
| require.NotEmpty(t, secretData, "expected non-empty secret data for keyID %s", keyID) | ||
|
|
||
| // Verify actual values match the source secret | ||
| require.Equal(t, "test-role-id", secretData["vault-approle-secret-role-id"], "role-id secret data mismatch for keyID %s", keyID) |
There was a problem hiding this comment.
does the comparison work ? we are comparing []byte with string. i guess it works because in go string actually is []byte
| for _, key := range expectedKeys { | ||
| v, ok := refSecret.Data[key] | ||
| if !ok { | ||
| return nil, fmt.Errorf("secret %s in openshift-config is missing required key %q", secretName, key) |
There was a problem hiding this comment.
worth adding unit tests to covert his scenario ?
| // KMSSecretData maps keyID to secret data carried from | ||
| // Key Secrets into the encryption-config Secret. | ||
| // Inner map keys use the short form (e.g. "app-role-role-id"). | ||
| KMSSecretData map[string]map[string][]byte |
There was a problem hiding this comment.
maybe we should rename to KMSPluginsSecretData ?
| // (e.g. "kms-plugin-secret-app-role-role-id-1"). KeyIDFromSecretDataKey | ||
| // returns the keyID (e.g. "1") and the secretDataKey (e.g. "app-role-role-id"). | ||
| var kmsSecretData map[string]map[string][]byte | ||
| for key, value := range encryptionConfigSecret.Data { |
There was a problem hiding this comment.
do we have tests that cover changes in this file ?
There was a problem hiding this comment.
seems like not everything is covered.
| } | ||
|
|
||
| // Write secret data entries to the encryption-config Secret. | ||
| // Each secret dataKey (e.g. "app-role-role-id") is combined with the keyID |
There was a problem hiding this comment.
what if the keyID=123 ?
There was a problem hiding this comment.
WDYM?. It will be kms-plugin-secret-app-role-role-id-123?
| } | ||
|
|
||
| // Write secret data entries to the encryption-config Secret. | ||
| // Each secret dataKey (e.g. "app-role-role-id") is combined with the keyID |
There was a problem hiding this comment.
what if dataKey=token-123 ?
There was a problem hiding this comment.
please add unit test to cover all edge cases.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| s.Data[dataKey] = value |
There was a problem hiding this comment.
if we have the same dataKey from two different secrets then I think we have a collision here.
| EncryptionConfSecretKey = "encryption-config" | ||
| // encryptionConfigSecretDataPrefix is the data key prefix for KMS plugin secret | ||
| // data entries in the encryption-config Secret. Full key: "kms-plugin-secret-{secretDataKey}-{keyID}". | ||
| encryptionConfigSecretDataPrefix = "kms-plugin-secret-" |
There was a problem hiding this comment.
worth updating the KEP.
|
@ardaguclu: 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. |
|
|
||
| // SecretData stores fetched values from the referenced secret | ||
| // in openshift-config. Keys use the format "{secretName}-{keyName}". | ||
| SecretData map[string][]byte |
There was a problem hiding this comment.
maybe this should be:
SecretData map[string]map[string][]byte // secretName to dataKey to value
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
4 participants
This PR carries Vault credentials (according to the new format in openshift/api#2836 --
role-id,secret-idinstead ofroleID,secretID) based on https://github.com/openshift/enhancements/blob/master/enhancements/kube-apiserver/kms-encryption-foundations.md.This PR mimics from 5221c82 commit.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes