test/kms: add reusable Vault KMS helpers and KMSTestProvider abstraction#2225
test/kms: add reusable Vault KMS helpers and KMSTestProvider abstraction#2225gangwgr wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughThis PR introduces a setup-hook mechanism for encryption test providers. It adds an ChangesEncryption Provider Setup Hook Infrastructure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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: gangwgr 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 |
gangwgr
force-pushed
the
update-kms-provider
branch
from
514f7c1 to
0af0751
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/library/encryption/kms/vault.go`:
- Around line 80-83: Before creating the target secret, validate that creds.Data
contains non-empty "role-id" and "secret-id" values: check creds.Data["role-id"]
and creds.Data["secret-id"] exist and have length > 0 (or non-empty string/byte
slice), and if either is missing/empty, fail early (return an error or call
t.Fatalf in the test) instead of proceeding to build the Data map used in the
secret write; update the code around the Data: map[string][]byte{...}
construction in vault.go to perform this validation on creds and only populate
roleID/secretID when valid.
- Around line 85-88: When creating the secret via
cs.Kube.CoreV1().Secrets(DefaultAppRoleTargetNamespace).Create(...,
appRoleSecret, ...), don't blindly call Update on any error; instead check if
the error is an AlreadyExists error
(k8s.io/apimachinery/pkg/api/errors.IsAlreadyExists(err)); if so, fetch the
existing secret with
cs.Kube.CoreV1().Secrets(DefaultAppRoleTargetNamespace).Get(ctx,
appRoleSecret.Name, metav1.GetOptions{}), copy its ResourceVersion into
appRoleSecret.ObjectMeta.ResourceVersion, then call Update; for other create
errors return/fail as before. Ensure you use the same ctx and metav1 types and
handle Get/Update errors with require.NoError where appropriate.
🪄 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: d5651548-77ce-4c42-b3f5-f47dbe458712
📒 Files selected for processing (2)
test/library/encryption/kms/provider.gotest/library/encryption/kms/vault.go
gangwgr
force-pushed
the
update-kms-provider
branch
2 times, most recently
from
4834028 to
aef73c8
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| const ( | ||
| DefaultVaultNamespace = "vault-kms" | ||
| DefaultVaultCredentialsSecret = "vault-credentials" | ||
| DefaultVaultAppRoleSecretName = "vault-approle-secret" |
There was a problem hiding this comment.
| DefaultVaultAppRoleSecretName = "vault-approle-secret" | |
| DefaultAppRoleSecretName = "vault-approle-secret" |
There was a problem hiding this comment.
Better to have this DefaultVaultAppRoleSecretName, because all are specific to vault
|
|
||
| roleID, ok := creds.Data["role-id"] | ||
| require.Truef(t, ok && len(roleID) > 0, "missing or empty key %q in %s/%s", "role-id", DefaultVaultNamespace, DefaultVaultCredentialsSecret) | ||
| secretID, ok := creds.Data["secret-id"] |
There was a problem hiding this comment.
We should be careful to not leak
There was a problem hiding this comment.
Validatio uses require.Contains which only prints key names, never secret values
| if apierrors.IsAlreadyExists(err) { | ||
| existing, getErr := cs.Kube.CoreV1().Secrets(DefaultAppRoleTargetNamespace).Get(ctx, DefaultVaultAppRoleSecretName, metav1.GetOptions{}) | ||
| require.NoError(t, getErr, "failed to get existing AppRole secret for update") | ||
| appRoleSecret.ResourceVersion = existing.ResourceVersion |
There was a problem hiding this comment.
Why do we need ResourceVersion?
| existing, getErr := cs.Kube.CoreV1().Secrets(DefaultAppRoleTargetNamespace).Get(ctx, DefaultVaultAppRoleSecretName, metav1.GetOptions{}) | ||
| require.NoError(t, getErr, "failed to get existing AppRole secret for update") | ||
| appRoleSecret.ResourceVersion = existing.ResourceVersion | ||
| _, err = cs.Kube.CoreV1().Secrets(DefaultAppRoleTargetNamespace).Update(ctx, appRoleSecret, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
updated, if secret already exists, just log and return
gangwgr
force-pushed
the
update-kms-provider
branch
from
aef73c8 to
afa6db9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/kms/vault.go (1)
78-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate AppRole credential values are non-empty before creating the target secret.
require.Containsonly verifies key presence; emptyrole-id/secret-idvalues still get written and defer failure to later test stages.Proposed fix
require.Contains(t, creds.Data, "role-id", "vault-credentials secret is missing the role-id key") require.Contains(t, creds.Data, "secret-id", "vault-credentials secret is missing the secret-id key") + roleID := creds.Data["role-id"] + secretID := creds.Data["secret-id"] + require.NotEmpty(t, roleID, "vault-credentials secret has empty role-id") + require.NotEmpty(t, secretID, "vault-credentials secret has empty secret-id") @@ Data: map[string][]byte{ - "roleID": creds.Data["role-id"], - "secretID": creds.Data["secret-id"], + "roleID": roleID, + "secretID": secretID, }, }🤖 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/library/encryption/kms/vault.go` around lines 78 - 90, The test currently only checks for presence of "role-id" and "secret-id" keys via require.Contains but may still allow empty values; before constructing appRoleSecret (the Secret created with Name DefaultVaultAppRoleSecretName in namespace DefaultAppRoleTargetNamespace using creds.Data), assert that creds.Data["role-id"] and creds.Data["secret-id"] are non-empty (e.g. require.NotEmpty or require.True on len > 0) and/or trim whitespace, and fail the test early if either value is empty so the subsequent creation of appRoleSecret with keys "roleID"/"secretID" cannot proceed with blank credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/library/encryption/kms/vault.go`:
- Around line 78-90: The test currently only checks for presence of "role-id"
and "secret-id" keys via require.Contains but may still allow empty values;
before constructing appRoleSecret (the Secret created with Name
DefaultVaultAppRoleSecretName in namespace DefaultAppRoleTargetNamespace using
creds.Data), assert that creds.Data["role-id"] and creds.Data["secret-id"] are
non-empty (e.g. require.NotEmpty or require.True on len > 0) and/or trim
whitespace, and fail the test early if either value is empty so the subsequent
creation of appRoleSecret with keys "roleID"/"secretID" cannot proceed with
blank credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f66e9580-a1e7-49f1-82d8-c656413eb3eb
📒 Files selected for processing (2)
test/library/encryption/kms/provider.gotest/library/encryption/kms/vault.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/encryption/kms/provider.go
| // KMSTestProvider pairs a KMS encryption config with its prerequisite setup. | ||
| // This allows the same test cases to run against any KMS provider (Vault, AWS, etc.) | ||
| // by simply iterating over a slice of providers. | ||
| type KMSTestProvider struct { |
There was a problem hiding this comment.
how will this provider be used ? why do we need it ?
There was a problem hiding this comment.
Right now KAS-O tests we only have Vault KMS. But whenwe have a second provider (e.g. AWS KMS) and want to add add that, we want to run same cases
Run the same TestKMSEncryptionOnOff test against each provider
Run TestEncryptionProvidersMigration across all providers
Each provider needs its own setup (Vault needs AppRole secret, AWS would need IAM credentials, etc.)
KMSTestProvider groups the config + setup together so the operator test just does:
var allProviders = []kms.KMSTestProvider{
kms.DefaultVaultTestProvider,
// future: kms.DefaultAWSTestProvider,
}
func testKMSEncryptionOnOff(t testing.TB) {
kms.SetupAll(t, allProviders)
for _, p := range allProviders {
library.TestEncryptionTurnOnAndOff(t, library.OnOffScenario{
EncryptionProvider: p.EncryptionConfig,
...
})
}
}
There was a problem hiding this comment.
then let's extend all providers, not only KMS.
for example
this field could be extended by a struct that holds configv1.APIServerEncryption and a custom setup function. WDYT
| // EnsureVaultAppRoleSecret reads credentials from the vault-credentials secret | ||
| // (created by a CI step) and creates the AppRole secret in openshift-config. | ||
| // If the secret already exists it is left untouched. | ||
| func EnsureVaultAppRoleSecret(t testing.TB) { |
There was a problem hiding this comment.
will this function create an AppRole Secret for the default configuration ?
if yes, then maybe we should rename to EnsureDefaultVaultAppRoleSecret
| Authentication: configv1.VaultAuthentication{ | ||
| Type: configv1.VaultAuthenticationTypeAppRole, | ||
| AppRole: configv1.VaultAppRoleAuthentication{ | ||
| Secret: configv1.VaultSecretReference{Name: DefaultVaultAppRoleSecretName}, |
There was a problem hiding this comment.
could we just simply use DefaultVaultCredentialsSecret ? why do we need a new secret ?
There was a problem hiding this comment.
We enforce the users to create a Secret under openshift-config namespace. Vault is deployed to dedicated vault-kms namespace. So we need this action
gangwgr
force-pushed
the
update-kms-provider
branch
from
afa6db9 to
fa3f16b
Compare
| creds, err := cs.Kube.CoreV1().Secrets(DefaultVaultNamespace).Get(ctx, DefaultVaultCredentialsSecret, metav1.GetOptions{}) | ||
| require.NoError(t, err, "failed to read %s/%s secret (was the vault-install CI step run?)", DefaultVaultNamespace, DefaultVaultCredentialsSecret) | ||
|
|
||
| require.Contains(t, creds.Data, "role-id", "vault-credentials secret is missing the role-id key") |
There was a problem hiding this comment.
I assume that require.Contains is smart enough to check that creds.Data map contains role-id key.
There was a problem hiding this comment.
do we have to compare the content ? or is it ok to assume the default will not change ?
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/kms/vault.go (1)
79-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate AppRole credential values are non-empty before creating the target secret.
Line 79 and Line 80 only verify key presence. Empty
role-id/secret-idstill pass and produce an invalidopenshift-configsecret, shifting failure downstream.Suggested fix
- require.Contains(t, creds.Data, "role-id", "vault-credentials secret is missing the role-id key") - require.Contains(t, creds.Data, "secret-id", "vault-credentials secret is missing the secret-id key") + roleID, ok := creds.Data["role-id"] + require.Truef(t, ok && len(roleID) > 0, "vault-credentials secret has missing/empty %q", "role-id") + secretID, ok := creds.Data["secret-id"] + require.Truef(t, ok && len(secretID) > 0, "vault-credentials secret has missing/empty %q", "secret-id") @@ Data: map[string][]byte{ - "roleID": creds.Data["role-id"], - "secretID": creds.Data["secret-id"], + "roleID": roleID, + "secretID": secretID, }, }🤖 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/library/encryption/kms/vault.go` around lines 79 - 91, The test currently only asserts the presence of "role-id" and "secret-id" keys in creds.Data but does not validate their values; update the test to assert that creds.Data["role-id"] and creds.Data["secret-id"] are non-empty (e.g. using require.NotEmpty or equivalent) before constructing appRoleSecret, so that the appRoleSecret.Data entries ("roleID"/"secretID") are guaranteed to contain valid bytes; reference creds.Data, DefaultVaultAppRoleSecretName, DefaultAppRoleTargetNamespace and appRoleSecret when adding these additional checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/library/encryption/kms/vault.go`:
- Around line 79-91: The test currently only asserts the presence of "role-id"
and "secret-id" keys in creds.Data but does not validate their values; update
the test to assert that creds.Data["role-id"] and creds.Data["secret-id"] are
non-empty (e.g. using require.NotEmpty or equivalent) before constructing
appRoleSecret, so that the appRoleSecret.Data entries ("roleID"/"secretID") are
guaranteed to contain valid bytes; reference creds.Data,
DefaultVaultAppRoleSecretName, DefaultAppRoleTargetNamespace and appRoleSecret
when adding these additional checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b2807e54-e4fc-4c0b-b944-e4d6b6930e5e
📒 Files selected for processing (2)
test/library/encryption/kms/provider.gotest/library/encryption/kms/vault.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/encryption/kms/provider.go
gangwgr
force-pushed
the
update-kms-provider
branch
2 times, most recently
from
dcb9f25 to
3833b5b
Compare
|
|
||
| func TestEncryptionTurnOnAndOff(t testing.TB, scenario OnOffScenario) { | ||
| if scenario.EncryptionProvider.Setup != nil { | ||
| scenario.EncryptionProvider.Setup(t) |
There was a problem hiding this comment.
IIRC all tests use SetAndWaitForEncryptionType. So I think this should be the only place that calls the setup fn.
| DefaultVaultNamespace = "vault-kms" | ||
| DefaultVaultCredentialsSecret = "vault-credentials" | ||
| DefaultVaultAppRoleSecretName = "vault-approle-secret" | ||
| DefaultVaultKMSPluginImage = "quay.io/openshifttest/mock-kms-plugin@sha256:03bb07a2c08b509653c4c70217a06a4b389c10b4d87922f50ee5eac82db5e140" |
There was a problem hiding this comment.
is this a Vault img ? or it will be in the future ?
There was a problem hiding this comment.
it is vault fake image which we are current using, in future we will update with real vault plugin image
| // If the secret already exists it is left untouched. | ||
| func EnsureDefaultVaultAppRoleSecret(t testing.TB) { | ||
| t.Helper() | ||
| ctx := context.Background() |
gangwgr
force-pushed
the
update-kms-provider
branch
2 times, most recently
from
795bd67 to
5b7f45c
Compare
| func SetAndWaitForEncryptionType(t testing.TB, ep EncryptionProvider, defaultTargetGRs []schema.GroupResource, namespace, labelSelector string) ClientSet { | ||
| t.Helper() | ||
|
|
||
| if ep.Setup != nil { |
There was a problem hiding this comment.
I think this should moved down around the line 77, no ?
| creds, err := cs.Kube.CoreV1().Secrets(DefaultVaultNamespace).Get(ctx, DefaultVaultCredentialsSecret, metav1.GetOptions{}) | ||
| require.NoError(t, err, "failed to read %s/%s secret (was the vault-install CI step run?)", DefaultVaultNamespace, DefaultVaultCredentialsSecret) | ||
|
|
||
| require.Contains(t, creds.Data, "role-id", "vault-credentials secret is missing the role-id key") |
There was a problem hiding this comment.
do we have to compare the content ? or is it ok to assume the default will not change ?
Move the Vault KMS encryption config and AppRole secret setup from cluster-kube-apiserver-operator into library-go so any operator can reuse them. Add KMSTestProvider type that pairs an encryption config with its setup function, enabling the same test cases to run against multiple KMS providers (Vault, AWS, etc.) without duplicating test logic.
gangwgr
force-pushed
the
update-kms-provider
branch
from
5b7f45c to
4aaba6e
Compare
|
#2225 (comment) |
|
@gangwgr: 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. |
| func SetAndWaitForEncryptionType(t testing.TB, ep EncryptionProvider, defaultTargetGRs []schema.GroupResource, namespace, labelSelector string) ClientSet { | ||
| t.Helper() | ||
|
|
||
| provider := ep.APIServerEncryption |
| type UpdateUnsupportedConfigFunc func(raw []byte) error | ||
|
|
||
| func SetAndWaitForEncryptionType(t testing.TB, provider configv1.APIServerEncryption, defaultTargetGRs []schema.GroupResource, namespace, labelSelector string) ClientSet { | ||
| func SetAndWaitForEncryptionType(t testing.TB, ep EncryptionProvider, defaultTargetGRs []schema.GroupResource, namespace, labelSelector string) ClientSet { |
| "secretID": creds.Data["secret-id"], | ||
| } | ||
|
|
||
| existing, err := cs.Kube.CoreV1().Secrets(DefaultAppRoleTargetNamespace).Get(ctx, DefaultVaultAppRoleSecretName, metav1.GetOptions{}) |
There was a problem hiding this comment.
could this be replaced by resourceapply.ApplySecret ?
3 participants
Move the Vault KMS encryption config and AppRole secret setup from cluster-kube-apiserver-operator into library-go so any operator can reuse them.
Add KMSTestProvider type that pairs an encryption config with its setup function, enabling the same test cases to run against multiple KMS provider without duplicating test logic.
Summary by CodeRabbit