fix: add support for vault_secret_key - #421
Merged
Merged
Conversation
- When creating or updating credentials with HashiCorp vault secrets, require the vault_key when users specify the vault_secret_path.
Reviewer's GuideThis PR adds explicit support for a required HashiCorp Vault key when using vault-based credentials, wiring a new --vault-key CLI option through argument parsing, validation, payload construction, tests, and documentation for both cred add and cred edit flows. Sequence diagram for cred add/edit with required vault_keysequenceDiagram
actor User
participant qpc_cred_add as qpc_cred_add
participant qpc_cred_edit as qpc_cred_edit
participant validate_vault_args as validate_vault_args
participant build_credential_payload as build_credential_payload
User->>qpc_cred_add: qpc cred add --vault-secret-path path --vault-key key
qpc_cred_add->>validate_vault_args: validate_vault_args(args, cred_type)
validate_vault_args-->>qpc_cred_add: args validated
qpc_cred_add->>build_credential_payload: build_credential_payload(args, cred_type, add_none)
build_credential_payload-->>qpc_cred_add: payload with vault_secret_path and vault_key
User->>qpc_cred_edit: qpc cred edit --vault-secret-path path --vault-key key
qpc_cred_edit->>validate_vault_args: validate_vault_args(args, cred_type)
validate_vault_args-->>qpc_cred_edit: args validated
qpc_cred_edit->>build_credential_payload: build_credential_payload(args, cred_type, add_none)
build_credential_payload-->>qpc_cred_edit: payload with vault_secret_path and vault_key
alt [vault_secret_path set and vault_key missing]
qpc_cred_add->>validate_vault_args: validate_vault_args(args, cred_type)
validate_vault_args->>validate_vault_args: logger.error(CRED_VAULT_KEY_REQUIRED)
validate_vault_args->>validate_vault_args: sys.exit(1)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
+ Coverage 96.47% 96.49% +0.01%
==========================================
Files 138 138
Lines 8690 8737 +47
==========================================
+ Hits 8384 8431 +47
Misses 306 306 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
validate_vault_args, you enforce thatvault_keyis required whenvault_secret_pathis set, but there is no reciprocal check; consider adding a guard that errors if--vault-keyis provided without--vault-secret-path(similar to the existingvault_mount_pointvalidation) to prevent inconsistent CLI usage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validate_vault_args`, you enforce that `vault_key` is required when `vault_secret_path` is set, but there is no reciprocal check; consider adding a guard that errors if `--vault-key` is provided without `--vault-secret-path` (similar to the existing `vault_mount_point` validation) to prevent inconsistent CLI usage.
## Individual Comments
### Comment 1
<location path="qpc/cred/utils.py" line_range="175-178" />
<code_context>
"""
# Get vault options (use getattr for legacy test compatibility)
vault_secret_path = getattr(args, "vault_secret_path", None)
+ vault_key = getattr(args, "vault_key", None)
vault_mount_point = getattr(args, "vault_mount_point", None)
</code_context>
<issue_to_address>
**issue (bug_risk):** Validate that --vault-key is not provided without --vault-secret-path
`validate_vault_args` currently requires `vault_key` when `vault_secret_path` is set, but not the reverse: `--vault-key` can be passed alone, leaving `vault_secret_path` falsy and bypassing validation. This permits an invalid config that contradicts the documented usage.
Please add a check such as:
```python
if vault_key and not vault_secret_path:
logger.error(_(messages.CRED_VAULT_KEY_REQUIRES_PATH))
sys.exit(1)
```
(or reuse an existing message) so `--vault-key` is rejected unless `--vault-secret-path` is also provided.
</issue_to_address>
### Comment 2
<location path="qpc/tests/cred/test_vault_cred_edit.py" line_range="144-153" />
<code_context>
+ def test_edit_vault_missing_key(
</code_context>
<issue_to_address>
**suggestion (testing):** Add an explicit test for `--vault-key` provided without `--vault-secret-path` to document and lock in the expected behavior.
You’ve covered the `--vault-secret-path` without `--vault-key` case. Please also add tests (for both `add` and `edit`) for the inverse: `--vault-key` provided without `--vault-secret-path`. That will lock in whether this should error or be ignored, and ensure the current behavior is validated via `sys.argv` and the resulting outcome (error or no-op).
Suggested implementation:
```python
assert payload["vault_mount_point"] == "custom-mount"
def test_edit_vault_missing_secret_path_with_key(
self,
capsys,
requests_mock,
):
"""Test that vault key without vault secret path fails."""
url = get_server_location() + CREDENTIAL_URI
requests_mock.get(
url,
status_code=200,
json={
"name": "vault_cred",
"type": "vault",
"vault_secret_path": "secret/data/my-creds",
"vault_key": "existing-key",
"vault_mount_point": "secret",
},
)
# Simulate CLI invocation with --vault-key but no --vault-secret-path
sys.argv = [
"qpc",
"cred",
"edit",
"--name",
"vault_cred",
"--vault-key",
"my-new-key",
]
with pytest.raises(SystemExit) as exc:
main()
captured = capsys.readouterr()
# Lock in current behavior: non-zero exit when vault-key is provided
# without a matching vault-secret-path.
assert exc.value.code != 0
# Keep this assertion loose so it doesn't over-constrain messaging,
# but still documents that the error mentions vault arguments.
assert "vault" in captured.err.lower()
def test_edit_vault_missing_key(
self,
capsys,
requests_mock,
):
"""Test that vault secret path without vault key fails."""
url = get_server_location() + CREDENTIAL_URI
requests_mock.get(
url,
status_code=200,
json={
```
1. The new test `test_edit_vault_missing_secret_path_with_key` assumes:
- `sys` is imported (e.g., `import sys`) at the top of this file.
- `pytest` is imported (e.g., `import pytest`) at the top of this file.
- `main` is the CLI entrypoint already used in other tests in this module, and is imported accordingly.
- The existing tests in this file already follow the `sys.argv` + `main()` pattern; if they use a different runner helper, adjust the invocation in this new test to match (e.g., `runner.invoke(...)` or a project-specific helper).
2. To fully address your original review comment (“for both `add` and `edit`”), a symmetric test should be added to `qpc/tests/cred/test_vault_cred_add.py`, e.g.:
```python
def test_add_vault_missing_secret_path_with_key(capsys):
"""Test that vault key without vault secret path fails for add."""
sys.argv = [
"qpc",
"cred",
"add",
"--name",
"vault_cred",
"--type",
"vault",
"--vault-key",
"my-key",
]
with pytest.raises(SystemExit) as exc:
main()
captured = capsys.readouterr()
assert exc.value.code != 0
assert "vault" in captured.err.lower()
```
Place this new test alongside the existing `add`-side Vault tests and adjust the runner/imports to match the conventions in that file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Make sure we catch the case where vault_key is specified without having secified the vault_secret_path.
ruda
approved these changes
May 13, 2026
- Preferring to use vault_secret_key instead of vault_key as to not confuse with keys uses to authenticate to the vault itself.
abellotti
changed the title
Contributor
Author
|
Merging with the ci failure as this needs the API and camayoc PRs merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Require a vault key when using HashiCorp Vault-backed credentials and propagate it through the CLI and payloads, with corresponding documentation and tests.
New Features:
Bug Fixes:
Documentation:
Tests: