CNTRLPLANE-3236: kms: fix args passed to the vault mock plugin

[bertinatto]

Previously, -vault-namespace and -transit-mount reflected whatever values their counterparts api fields had. However, those fields are optional in the api, but since they are not pointers, their values could be be empty strings. As a result, this patch checks if those fields contain empty strings and optionally pass them through to the KMS plugin via CLI args.

In addition, this patch also plumbs through the role-id param, which is a required api field.

Summary by CodeRabbit

Improvements

• Vault KMS plugin sidecar container arguments are now dynamically constructed based on your specific configuration settings
• Optional Vault configuration parameters (vault-namespace, transit-mount) are included only when you explicitly configure them
• Refined vault-kms-plugin sidecar command-line argument ordering to ensure proper initialization sequence during deployment

[openshift-ci-robot]

@bertinatto: This pull request references CNTRLPLANE-3236 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.

Details

In response to this:

Previously, -vault-namespace and -transit-mount reflected whatever values their counterparts api fields had. However, those fields are optional in the api, but since they are not pointers, their values could be be empty strings. As a result, this patch checks if those fields contain empty strings and optionally pass them through to the KMS plugin via CLI args.

In addition, this patch also plumbs through the role-id param, which is a required api field.

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 06350669-541a-4f10-bbd8-d0a2164a1d91

📥 Commits

Reviewing files that changed from the base of the PR and between d0c25a0 and 5b132b9.

📒 Files selected for processing (3)

• pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
• pkg/operator/encryption/kms/pluginlifecycle/vault.go
• pkg/operator/encryption/kms/pluginlifecycle/vault_test.go

---

Walkthrough

The BuildSidecarContainer method refactored to dynamically construct the Vault KMS sidecar container's argument list, with required parameters built first and optional vault-namespace and transit-mount flags conditionally appended based on configuration. All corresponding tests updated to match the new argument ordering.

Changes

Vault Sidecar Argument Construction

Layer / File(s)	Summary
Dynamic argument construction pkg/operator/encryption/kms/pluginlifecycle/vault.go	BuildSidecarContainer now builds Args slice dynamically: required flags (-listen-address, -vault-address, -transit-key, approle parameters) are initialized first, then optional flags (-vault-namespace, -transit-mount) are conditionally appended when their config values are non-empty.
Unit test updates pkg/operator/encryption/kms/pluginlifecycle/vault_test.go	Test cases updated to verify the new argument ordering and conditional inclusion of optional flags; "empty optional fields" test explicitly configures VaultNamespace and TransitMount as empty strings while providing TransitKey.
Integration test updates pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go	Both single-provider and two-provider (migration) test cases updated to expect the new sidecar container argument ordering with -vault-address appearing before -transit-key and approle parameters.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

approved, lgtm

Suggested reviewers

• dgrisonnet
• p0lyn0mial

🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Test Structure And Quality	⚠️ Warning	Assertions lack meaningful failure messages. All require.NoError() and require.Equal() calls have no descriptive messages, hindering test failure diagnosis.	Add message arguments to assertions. Example: require.NoError(t, err, "failed to build Vault sidecar container") and require.Equal(t, expected, actual, "pod spec should match expected for %s", tt.name)

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the main change: fixing arguments passed to the Vault KMS plugin, with a reference to the JIRA ticket.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	All test names in vault_test.go and sidecar_test.go are static and deterministic. No dynamic content, generated identifiers, or changing values found. Tests use descriptive static string literals.
Microshift Test Compatibility	✅ Passed	This PR does not add any Ginkgo e2e tests. Only standard Go unit tests are modified; no Ginkgo declarations found.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests are added in this PR. The changes are updates to standard Go unit tests in the KMS operator package that test configuration building logic, not cluster topology assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	The PR modifies KMS plugin sidecar container argument construction, not scheduling logic. No scheduling constraints, pod affinity, topology assumptions, or deployment manifests are introduced.
Ote Binary Stdout Contract	✅ Passed	No stdout writes in process-level code. All fmt usage is fmt.Sprintf() for string formatting in methods, not entry points like main() or init().
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	Modified files contain unit tests using Go's standard testing package, not Ginkgo e2e tests. Custom check applies only to new Ginkgo e2e tests (It(), Describe(), etc.). Not applicable to this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[ardaguclu]

Changes look good to me
/lgtm
/hold
feel free to unhold if tests work

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, bertinatto

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/operator/encryption/OWNERS [bertinatto]

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

[bertinatto]

/retest

[ardaguclu]

/hold cancel

[openshift-ci]

@bertinatto: all tests passed!

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.