Skip to content

[WIP]Adding KMS TestKMSEncryptionOnOff test#2018

Closed
gangwgr wants to merge 2 commits intoopenshift:mainfrom
gangwgr:kms-e2e-full-test
Closed

[WIP]Adding KMS TestKMSEncryptionOnOff test#2018
gangwgr wants to merge 2 commits intoopenshift:mainfrom
gangwgr:kms-e2e-full-test

Conversation

@gangwgr
Copy link
Copy Markdown
Contributor

@gangwgr gangwgr commented Jan 29, 2026

Adding KMS TestKMSEncryptionOnOff test

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 29, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Excluded labels (none allowed) (1)
  • do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

  • 🔍 Trigger a full review
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2026
@openshift-ci openshift-ci bot requested review from p0lyn0mial and wangke19 January 29, 2026 11:50
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Jan 29, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gangwgr
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

go.mod Outdated
github.com/miekg/dns v1.1.61
github.com/onsi/ginkgo/v2 v2.21.0
github.com/openshift-eng/openshift-tests-extension v0.0.0-20250804142706-7b3ab438a292
github.com/openshift/api v0.0.0-20251111013132-5c461e21bdb7
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move "vendor" changes to a separate PR.

t.Log("KMS encryption on/off test placeholder - CI job validation")
ctx := context.Background()

// Get clients for deploying KMS plugin
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm this line.

// Get clients for deploying KMS plugin
clientSet := library.GetClients(t)

// Step 1: Deploy the mock KMS plugin
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm this line.

clientSet := library.GetClients(t)

// Step 1: Deploy the mock KMS plugin
t.Log("Deploying mock KMS plugin...")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm this log - we added logs to the deployer.


// Step 1: Deploy the mock KMS plugin
t.Log("Deploying mock KMS plugin...")
cleanup := kms.DeployUpstreamMockKMSPlugin(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be changed to:

t.Cleanup(kms.DeployUpstreamMockKMSPlugin(...))

)
defer cleanup()

// Step 2-10: Run the encryption on/off test
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm this line.

defer cleanup()

// Step 2-10: Run the encryption on/off test
t.Log("Running KMS encryption on/off test...")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need these logs ?

@gangwgr gangwgr force-pushed the kms-e2e-full-test branch 2 times, most recently from 4820d14 to db52c11 Compare January 29, 2026 12:50
@ardaguclu
Copy link
Copy Markdown
Member

Better to wait #2015 to be merged.

t.Cleanup(kms.DeployUpstreamMockKMSPlugin(
ctx,
t,
clientSet.Kube,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we library.GetClients(t).Kube ?

"github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/operatorclient"
operatorencryption "github.com/openshift/cluster-kube-apiserver-operator/test/library/encryption"
library "github.com/openshift/library-go/test/library/encryption"
"github.com/openshift/library-go/test/library/encryption/kms"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

librarykms

// Get clients for deploying KMS plugin
clientSet := library.GetClients(t)

t.Cleanup(kms.DeployUpstreamMockKMSPlugin(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you make a one liner instead ?

@p0lyn0mial
Copy link
Copy Markdown
Contributor

we must also pull openshift/library-go#2086 in

@gangwgr gangwgr force-pushed the kms-e2e-full-test branch 3 times, most recently from bc16b88 to 75d17e3 Compare January 30, 2026 06:03
if err := encryptionkms.AddKMSPluginVolumeAndMountToPodSpec(&required.Spec, "kube-apiserver", featureGateAccessor); err != nil {
return nil, false, fmt.Errorf("failed to add KMS encryption volumes: %w", err)
// Add KMS plugin volume mount if the KMS encryption feature gate is enabled
if err := kms.AddKMSPluginVolumeAndMountToPodSpec(&required.Spec, "kube-apiserver", featureGateAccessor); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need these changes as well, master branch should include them already.

go.sum Outdated
github.com/fvbommel/sortorder v1.1.0/go.mod h1:uk88iVf1ovNn1iLfgUVU2F9o5eO30ui720w+kxuqRs0=
github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sapM=
github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ=
github.com/gangwgr/library-go v0.0.0-20260129150807-18fba7769367 h1:dz9NGDpWK+clJaUfXP94lG5okB38qppI6lnqB/rSe0s=
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the changes in openshift/library-go#2086

@ardaguclu
Copy link
Copy Markdown
Member

/testwith gangwgr/cluster-kube-apiserver-operator/kms-e2e-full-test openshift/cluster-openshift-apiserver-operator#643

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Feb 2, 2026

@ardaguclu, testwith: Error processing request. ERROR:

could not determine job runs: requested job is invalid. needs to be formatted like: <org>/<repo>/<branch>/<variant?>/<job>. instead it was: gangwgr/cluster-kube-apiserver-operator/kms-e2e-full-test

@ardaguclu
Copy link
Copy Markdown
Member

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643

@ardaguclu
Copy link
Copy Markdown
Member

/testwith abort
it is known that this run will fail

@ardaguclu
Copy link
Copy Markdown
Member

To sum up, once we sort out the KMS plugin issue, we can run this command;

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented Feb 2, 2026

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

@ardaguclu
Copy link
Copy Markdown
Member

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

This adds TestKMSEncryptionOnOff which tests the full KMS encryption
on/off cycle using the mock KMS plugin from library-go.
@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented Feb 3, 2026

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented Feb 3, 2026

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented Feb 3, 2026

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented Feb 3, 2026

/testwith openshift/cluster-kube-apiserver-operator/main/e2e-gcp-operator-encryption-kms openshift/cluster-openshift-apiserver-operator#643 openshift/cluster-authentication-operator#832

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Feb 3, 2026

@gangwgr: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-operator-encryption-single-node 780b8a7 link false /test e2e-gcp-operator-encryption-single-node
ci/prow/e2e-aws-ovn-serial-1of2 780b8a7 link true /test e2e-aws-ovn-serial-1of2

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

// 11. Cleans up the KMS plugin
func TestKMSEncryptionOnOff(t *testing.T) {
t.Log("KMS encryption on/off test placeholder - CI job validation")
t.Cleanup(librarykms.DeployUpstreamMockKMSPlugin(context.Background(), t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s also add a comment stating that this step is only required for v1. In the future, the platform will manage the plugins, and this code will no longer be needed.

// 11. Cleans up the KMS plugin
func TestKMSEncryptionOnOff(t *testing.T) {
t.Log("KMS encryption on/off test placeholder - CI job validation")
t.Cleanup(librarykms.DeployUpstreamMockKMSPlugin(context.Background(), t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, once openshift/library-go#2113 merges t.Cleanup won't be needed.

@gangwgr
Copy link
Copy Markdown
Contributor Author

gangwgr commented Feb 6, 2026

/close

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2026
@openshift-merge-robot
Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot closed this Feb 6, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Feb 6, 2026

@gangwgr: Closed this PR.

Details

In response to this:

/close

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 kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants