CNTRLPLANE-2247:Adding KMS TestKMSEncryptionOnOff test for AUTH #834
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-2247 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 "4.22.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. |
WalkthroughThe PR updates the openshift/library-go dependency to a newer version and implements a full end-to-end KMS encryption test, replacing a placeholder with a complete test flow that deploys a mock KMS plugin and validates token encryption and decryption workflows. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
test/e2e/network_policy.go
Outdated
| @@ -0,0 +1,264 @@ | |||
| package e2e | |||
There was a problem hiding this comment.
Why do we need this change?. Isn't this out of scope?
There was a problem hiding this comment.
my bad i added my network policy feature work in pr
gangwgr
force-pushed
the
kms-e2e-full-test-new
branch
from
82fda3b to
7927629
Compare
|
/lgtm |
![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
🤖 Fix all issues with AI agents
In `@test/e2e-encryption-kms/encryption_kms_test.go`:
- Around line 30-33: The mock KMS deployment is not cleaned up; update
DeployUpstreamMockKMSPlugin (or the test in encryption_kms_test.go) to register
a cleanup with t.Cleanup that deletes the created namespace/resources (the
namespace is librarykms.WellKnownUpstreamMockKMSPluginNamespace and deployment
is created by librarykms.DeployUpstreamMockKMSPlugin); ensure the cleanup uses
the same clients (library.GetClients(t).Kube) to remove the deployed plugin and
namespace so resources are removed after the test to prevent leakage and
cross-test interference.
| // Deploy the mock KMS plugin for testing. | ||
| // NOTE: This manual deployment is only required for KMS v1. In the future, | ||
| // the platform will manage the KMS plugins, and this code will no longer be needed. | ||
| librarykms.DeployUpstreamMockKMSPlugin(context.Background(), t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of DeployUpstreamMockKMSPlugin
rg -n -C5 'func DeployUpstreamMockKMSPlugin' -g '**/*.go'Repository: openshift/cluster-authentication-operator
Length of output: 1697
🏁 Script executed:
#!/bin/bash
# Read the full DeployUpstreamMockKMSPlugin function implementation
sed -n '54,150p' vendor/github.com/openshift/library-go/test/library/encryption/kms/k8s_mock_kms_plugin_deployer.goRepository: openshift/cluster-authentication-operator
Length of output: 3557
🏁 Script executed:
#!/bin/bash
# Check the test file to see if cleanup is registered after calling DeployUpstreamMockKMSPlugin
sed -n '20,60p' test/e2e-encryption-kms/encryption_kms_test.goRepository: openshift/cluster-authentication-operator
Length of output: 2148
🏁 Script executed:
#!/bin/bash
# Search for cleanup or teardown in the test file
rg -n 'Cleanup|cleanup|teardown|defer' test/e2e-encryption-kms/encryption_kms_test.goRepository: openshift/cluster-authentication-operator
Length of output: 67
🏁 Script executed:
#!/bin/bash
# Find and inspect TestEncryptionTurnOnAndOff to see if it handles KMS plugin cleanup
rg -n 'func TestEncryptionTurnOnAndOff' -g '**/*.go' -A 50Repository: openshift/cluster-authentication-operator
Length of output: 8979
🏁 Script executed:
#!/bin/bash
# Check if the test file has any setup/teardown or suite-level cleanup
rg -n 'TestMain|init|defer|t.Cleanup' test/e2e-encryption-kms/encryption_kms_test.goRepository: openshift/cluster-authentication-operator
Length of output: 67
🏁 Script executed:
#!/bin/bash
# Search for the WellKnownUpstreamMockKMSPluginNamespace constant
rg -n 'WellKnownUpstreamMockKMSPluginNamespace' -g '**/*.go' | head -20Repository: openshift/cluster-authentication-operator
Length of output: 688
Add explicit cleanup for the mock KMS plugin deployment.
DeployUpstreamMockKMSPlugin deploys resources to the "k8s-mock-plugin" namespace but does not register cleanup. The test comment mentions cleanup, but no cleanup code exists in the test or library function. Register t.Cleanup() in DeployUpstreamMockKMSPlugin to delete the namespace/resources, or add explicit cleanup in the test to prevent resource leakage and cross-test interference.
🤖 Prompt for AI Agents
In `@test/e2e-encryption-kms/encryption_kms_test.go` around lines 30 - 33, The
mock KMS deployment is not cleaned up; update DeployUpstreamMockKMSPlugin (or
the test in encryption_kms_test.go) to register a cleanup with t.Cleanup that
deletes the created namespace/resources (the namespace is
librarykms.WellKnownUpstreamMockKMSPluginNamespace and deployment is created by
librarykms.DeployUpstreamMockKMSPlugin); ensure the cleanup uses the same
clients (library.GetClients(t).Kube) to remove the deployed plugin and namespace
so resources are removed after the test to prevent leakage and cross-test
interference.
7927629 to
a51c1b7
Compare
gangwgr
force-pushed
the
kms-e2e-full-test-new
branch
2 times, most recently
from
2c2e899 to
8428744
Compare
|
/retest-required |
|
I think failures look same. I suspect that tls changes in library-go causing this. Investigating... |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest |
|
/lgtm |
|
/retest |
gangwgr
force-pushed
the
kms-e2e-full-test-new
branch
from
8428744 to
8dd3540
Compare
gangwgr
force-pushed
the
kms-e2e-full-test-new
branch
from
8dd3540 to
5dbb770
Compare
openshift-merge-robot
added
the
needs-rebase
gangwgr
force-pushed
the
kms-e2e-full-test-new
branch
from
5dbb770 to
d6826ab
Compare
gangwgr
force-pushed
the
kms-e2e-full-test-new
branch
from
d6826ab to
ea4537d
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/lgtm |
|
Failure of e2e-gcp-operator-encryption-kms is expected. Because controllers are not ready to handle KMS encryption type yet (i.e. openshift/library-go#2086). |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
the
verified
|
@ardaguclu: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@gangwgr: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/assign @liouk |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, gangwgr, liouk 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
openshift-merge-bot
bot
merged commit 2d8256b
into
openshift:master
5 participants
CNTRLPLANE-2247:Adding KMS TestKMSEncryptionOnOff test for AUTH