feat(copyoffload): add storage_secret_extra for array-specific Secret keys#535
Open
prabinovRedhat wants to merge 2 commits into
Open
feat(copyoffload): add storage_secret_extra for array-specific Secret keys#535prabinovRedhat wants to merge 2 commits into
prabinovRedhat wants to merge 2 commits into
Conversation
Contributor
WalkthroughAdds support for extra Kubernetes Secret ChangesStorage Secret Extra Configuration
Sequence DiagramsequenceDiagram
participant User
participant GenerateWizard as mtv-api-tests generate
participant GatherFunc as gather_storage_secret_extra()
participant Fixture as copyoffload_storage_secret fixture
participant MergeFunc as merge_storage_secret_extra()
participant Secret as Kubernetes Secret
User->>GenerateWizard: run generate
GenerateWizard->>GatherFunc: prompt for extra fields
GatherFunc->>User: request key/value pairs
User->>GatherFunc: provide extras or skip
GatherFunc->>GenerateWizard: return storage_secret_extra dict
GenerateWizard->>Fixture: build copyoffload config
Fixture->>MergeFunc: merge_storage_secret_extra(secret_data, config)
MergeFunc->>MergeFunc: parse env & resolve precedence
MergeFunc->>Fixture: return updated secret_data
Fixture->>Secret: create Secret with merged fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd storage_secret_extra for array-specific Secret keys in copy-offload
WalkthroughsDescription• Add storage_secret_extra config option for array-specific Kubernetes Secret keys • Support COPYOFFLOAD_STORAGE_SECRET_EXTRA environment variable override (JSON object) • Extend mtv-api-tests generate wizard with interactive prompts for extra keys • Merge extra secret entries after base and vendor-specific fields in storage secret fixture • Document usage in copy-offload guide, README, and .providers.json.example Diagramflowchart LR
A["providers.json<br/>storage_secret_extra"] -->|merge| C["Storage Secret<br/>stringData"]
B["COPYOFFLOAD_STORAGE_SECRET_EXTRA<br/>env var"] -->|override| C
D["Base + Vendor<br/>fields"] -->|base| C
E["generate wizard<br/>prompts"] -->|populate| A
File Changes1. cli/mtv_api_tests/common.py
|
Code Review by Qodo
1. get_storage_secret_extra() docstring lacks Raises
|
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Collaborator
Author
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit c0e7462 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@utilities/copyoffload_migration.py`:
- Around line 142-143: The type-check for storage_secret_extra currently raises
ValueError when config_extra is not a dict; change this to raise TypeError
instead. Locate the conditional that checks "if not isinstance(config_extra,
dict)" (in the function handling storage_secret_extra/config_extra) and replace
the raised exception from ValueError(...) to TypeError(...) while keeping the
existing error message text so callers receive a TypeError for incorrect types.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 204b675a-3d80-4036-ae8d-7135afae95b2
📒 Files selected for processing (7)
.providers.json.exampleREADME.mdcli/mtv_api_tests/common.pycli/mtv_api_tests/generate.pyguides/copyoffload/how-to-run-copyoffload-tests.mdtests/copyoffload/conftest.pyutilities/copyoffload_migration.py
… keys Allow extra Forklift stringData keys via providers JSON and COPYOFFLOAD_STORAGE_SECRET_EXTRA env override. Includes generate wizard prompts and copy-offload guide documentation. Co-authored-by: Cursor <cursoragent@cursor.com>
prabinovRedhat
force-pushed
the
feat/copyoffload-storage-secret-extra
branch
from
c0e7462 to
76de4da
Compare
Comment on lines
+259
to
+266
| secret_key = Prompt.ask(" Secret key (empty to finish)") | ||
| if not secret_key.strip(): | ||
| break | ||
| value = Prompt.ask(f" Value for {secret_key.strip()}") | ||
| if not value.strip(): | ||
| console.print(" [yellow]Skipping empty value[/yellow]") | ||
| continue | ||
| result[secret_key.strip()] = value.strip() |
There was a problem hiding this comment.
2. Wizard echoes secret values 🐞 Bug ⛨ Security
gather_storage_secret_extra() reads extra Secret values with Prompt.ask() without password masking, so values are visible on-screen while typing. This can leak credentials/tokens intended to go into Kubernetes Secret stringData.
Agent Prompt
### Issue description
`gather_storage_secret_extra()` collects values for Kubernetes Secret `stringData`, but it prompts the value in plaintext. This can expose sensitive values via shoulder-surfing or screen recording.
### Issue Context
Other credential prompts in this CLI already use masked input (`password=True`) for secret values.
### Fix Focus Areas
- cli/mtv_api_tests/common.py[245-269]
### Suggested fix
- Change the value prompt to use `Prompt.ask(..., password=True)` (or prompt for whether to mask, defaulting to masked).
- Keep the key prompt unmasked.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
There was a problem hiding this comment.
Done — secret extra values are now masked during prompt entry.
Comment on lines
157
to
163
| # Storage vendor and fields | ||
| vendor = select_vendor() | ||
| vendor_fields = gather_vendor_fields(vendor) | ||
| storage_secret_extra = gather_storage_secret_extra() | ||
|
|
||
| # Storage credentials | ||
| storage_host, storage_user, storage_pass = get_storage_credentials() |
There was a problem hiding this comment.
3. Extras printed in summary 🐞 Bug ⛨ Security
mtv-api-tests generate prints a configuration summary after collecting storage_secret_extra, but mask_passwords() only masks keys containing "password"/"pwd". As a result, storage_secret_extra values can be printed in plaintext even though they are destined for a Kubernetes Secret.
Agent Prompt
### Issue description
The generate wizard prints a JSON "Configuration Summary" to stdout. With the new `storage_secret_extra` feature, arbitrary Secret `stringData` values may appear in that summary, but `mask_passwords()` does not mask them unless the *key name* contains "password"/"pwd".
### Issue Context
`storage_secret_extra` is explicitly for Kubernetes Secret `stringData` keys, so values should be treated as sensitive by default.
### Fix Focus Areas
- cli/mtv_api_tests/generate.py[136-200]
- cli/mtv_api_tests/generate.py[324-333]
- cli/mtv_api_tests/common.py[767-784]
### Suggested fix
- Update `mask_passwords()` to special-case `storage_secret_extra` (and mask all values under it as `"***"`).
- Example behavior: if `key == "storage_secret_extra"` and `value` is a dict, return `{k: "***" for k in value}`.
- Alternatively (or additionally), omit `storage_secret_extra` from the summary output entirely.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
There was a problem hiding this comment.
Done — all keys under storage_secret_extra are masked in the wizard summary.
Add missing docstring sections, use TypeError for type validation, mask wizard secret extra values in prompts and config summary. Co-authored-by: Cursor <cursoragent@cursor.com>
redhat-qe-bot
added
commented-prabinovRedhat
and removed
commented-qodo-code-review[bot]
labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cli/mtv_api_tests/common.py`:
- Around line 778-779: The masking branch for storage_secret_extra uses a dict
comprehension; replace it with dict.fromkeys to be more idiomatic: in the block
where key == "storage_secret_extra" and isinstance(value, dict) (which currently
assigns masked[key] = {secret_key: "***" for secret_key in value}), set
masked[key] = dict.fromkeys(value, "***") so keys remain the same and all values
are "***".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 697e5969-ec62-4fd6-8880-31193ee2a5cb
📒 Files selected for processing (7)
.providers.json.exampleREADME.mdcli/mtv_api_tests/common.pycli/mtv_api_tests/generate.pyguides/copyoffload/how-to-run-copyoffload-tests.mdtests/copyoffload/conftest.pyutilities/copyoffload_migration.py
Collaborator
|
/lgtm |
redhat-qe-bot
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
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
6 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
storage_secret_extrain.providers.jsoncopyoffloadblock for Forklift SecretstringDatakeys not covered by built-in vendor fields (e.g. PowerMaxPOWERMAX_PORT_GROUP_NAME,STORAGE_SKIP_SSL_VERIFICATION).COPYOFFLOAD_STORAGE_SECRET_EXTRAenv var (JSON object) to override matching keys at runtime.mtv-api-tests generatewizard with an optional prompt for extra keys..providers.json.example.If
storage_secret_extrais missing, empty, ornull, behavior is unchanged.Test plan
TestCopyoffloadThinMigrationonocp-edge97(ONTAP) — 6/6 passed (multiple runs)storage_secret_extra(QE_TEST_EXTRA_KEY,ANOTHER_NON_VENDOR_KEY) plusSTORAGE_*andONTAP_SVMpre-commit run --all-filespassed on changed filesSummary by CodeRabbit
New Features
storage_secret_extrain provider config or the corresponding environment variable; wizard prompts can collect these interactively.Documentation
Bug Fixes