feat(vsphere): per-component credential tooling (story #41)#16
feat(vsphere): per-component credential tooling (story #41)#16splatypus-bot wants to merge 2 commits into
Conversation
- upi/vsphere/per-component-credentials/create-roles.sh: govc script creates four vCenter roles with exact canonical privilege sets (19/6/3/2) - upi/vsphere/per-component-credentials/create-roles.ps1: PowerCLI equivalent - upi/vsphere/per-component-credentials/generate-credentials.sh: generates ~/.vsphere/credentials template with 0600/0700 permissions - docs/user/vsphere/per-component-credentials-troubleshooting.md: covers all four error scenarios with govc commands and UI paths - 15 tests: 10 script content (AC1) + 5 guide structural (AC3), all pass Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds comprehensive infrastructure for vSphere per-component credential management: a user-facing troubleshooting guide covering four error scenarios, bash and PowerShell scripts to create per-component vCenter roles with specific privilege sets, a credential template generator, and Go tests validating both the guide content and role creation scripts. ChangesvSphere Per-Component Credentials Tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
upi/vsphere/per-component-credentials/roles_test.go (1)
138-145: ⚡ Quick winExtend PowerCLI privilege assertions beyond machineAPI.
Current PowerCLI privilege validation only checks machineAPI; regressions in csiDriver, cloudController, or diagnostics privileges would pass unnoticed.
Proposed direction
func TestPowerCLIScript_MachineAPI_AllPrivilegesPresent(t *testing.T) { content := readScript(t, powercliScript) for _, priv := range machineAPIPrivileges { if !strings.Contains(content, priv) { t.Errorf("PowerCLI script missing machineAPI privilege %q", priv) } } + + for _, priv := range csiDriverPrivileges { + if !strings.Contains(content, priv) { + t.Errorf("PowerCLI script missing csiDriver privilege %q", priv) + } + } + for _, priv := range cloudControllerPrivileges { + if !strings.Contains(content, priv) { + t.Errorf("PowerCLI script missing cloudController privilege %q", priv) + } + } + for _, priv := range diagnosticsPrivileges { + if !strings.Contains(content, priv) { + t.Errorf("PowerCLI script missing diagnostics privilege %q", priv) + } + } }🤖 Prompt for 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. In `@upi/vsphere/per-component-credentials/roles_test.go` around lines 138 - 145, The test TestPowerCLIScript_MachineAPI_AllPrivilegesPresent only asserts machineAPI privileges; extend it (or add similar tests) to also validate csiDriverPrivileges, cloudControllerPrivileges, and diagnosticsPrivileges by reading the PowerCLI script via readScript(powercliScript) and asserting that each privilege string in those slices is contained in the script (same pattern used for machineAPIPrivileges); reference the existing test and the privilege slice names (machineAPIPrivileges, csiDriverPrivileges, cloudControllerPrivileges, diagnosticsPrivileges) and reuse its error message pattern when a privilege is missing.upi/vsphere/per-component-credentials/create-roles.ps1 (1)
11-13: ⚡ Quick winRemove or wire
$Server; it is currently unused.The
$Serverparameter (line 12) is never referenced in the script. The usage comment (line 8) shows users should first callConnect-VIServermanually, but the declared parameter serves no purpose and should be removed unless it's intended for future use.🤖 Prompt for 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. In `@upi/vsphere/per-component-credentials/create-roles.ps1` around lines 11 - 13, The declared parameter $Server is unused; either remove the param declaration or wire it into the script by using $Server when establishing or validating a vCenter connection (e.g., call Connect-VIServer -Server $Server or check Get-VIServer -Server $Server) so the parameter is meaningful; update the param block (remove [string]$Server = $env:GOVC_URL) or add connection logic referencing $Server and adjust usage comments accordingly.
🤖 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 `@docs/user/vsphere/per-component-credentials-troubleshooting.md`:
- Around line 12-14: The fenced error-example code blocks in
docs/user/vsphere/per-component-credentials-troubleshooting.md are missing
language tags; update each triple-backtick block that contains plain error text
(for example the blocks containing "Credential validation failed for machineAPI
on vcenter1.example.com: missing privileges: [VirtualMachine.Inventory.Create]",
"authentication error: 535 5.7.8 Error: authentication credentials invalid",
"credentials file ~/.vsphere/credentials has insecure permissions 0644; expected
0600", and "install-config.yaml: componentCredentials.machineAPI is set but
componentCredentials.csiDriver is missing") to use a language tag (e.g.,
```text) so markdownlint MD040 is satisfied and CI passes.
In `@upi/vsphere/per-component-credentials/generate-credentials.sh`:
- Around line 32-36: The validation currently checks VCENTER_LIST[0] before
trimming, so inputs like ",vc1.example.com" or whitespace-only tokens slip
through; update the code that builds VCENTER_LIST from VSPHERE_HOSTNAMES to trim
whitespace from each token and remove empty tokens (filter out zero-length
entries) after splitting, then validate that the filtered VCENTER_LIST has at
least one entry before proceeding; apply the same trim/filter-and-then-validate
fix to the other similar block referenced (lines 45-48) so both
credential-generation paths use the cleaned VCENTER_LIST.
---
Nitpick comments:
In `@upi/vsphere/per-component-credentials/create-roles.ps1`:
- Around line 11-13: The declared parameter $Server is unused; either remove the
param declaration or wire it into the script by using $Server when establishing
or validating a vCenter connection (e.g., call Connect-VIServer -Server $Server
or check Get-VIServer -Server $Server) so the parameter is meaningful; update
the param block (remove [string]$Server = $env:GOVC_URL) or add connection logic
referencing $Server and adjust usage comments accordingly.
In `@upi/vsphere/per-component-credentials/roles_test.go`:
- Around line 138-145: The test
TestPowerCLIScript_MachineAPI_AllPrivilegesPresent only asserts machineAPI
privileges; extend it (or add similar tests) to also validate
csiDriverPrivileges, cloudControllerPrivileges, and diagnosticsPrivileges by
reading the PowerCLI script via readScript(powercliScript) and asserting that
each privilege string in those slices is contained in the script (same pattern
used for machineAPIPrivileges); reference the existing test and the privilege
slice names (machineAPIPrivileges, csiDriverPrivileges,
cloudControllerPrivileges, diagnosticsPrivileges) and reuse its error message
pattern when a privilege is missing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cdc6efaf-9235-45fc-a748-666244452d8b
📒 Files selected for processing (6)
docs/user/vsphere/per-component-credentials-troubleshooting.mddocs/user/vsphere/troubleshooting_guide_test.goupi/vsphere/per-component-credentials/create-roles.ps1upi/vsphere/per-component-credentials/create-roles.shupi/vsphere/per-component-credentials/generate-credentials.shupi/vsphere/per-component-credentials/roles_test.go
… tooling - Add language tags to fenced error-example code blocks in per-component-credentials-troubleshooting.md to satisfy MD040 - Fix generate-credentials.sh: build a trimmed/filtered vCenter list before validating, so leading commas and whitespace-only tokens are handled correctly instead of producing a false error or silent empty credentials body Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
💻 dev — 2026-05-11T14:35:43ZAddressed CodeRabbit feedback (commit a0426c4): Fixed: Markdown lint (MD040) — added Fixed: Script validation bug —
|
💻 dev — 2026-05-11T14:51:34ZAgentic Review Summary — CodeRabbit Feedback Pass (a0426c4)Change reviewed: Markdown language tags + generate-credentials.sh validation order fix Assumptions surfaced:
Adversarial cases verified:
Delete Test result: N/A — this is a shell script; the fix is in validation logic Risk surface gaps: None — the fix is strictly additive (more input validation, better filtering) Verdict: APPROVE — validate-after-filter ordering is the correct approach; error message is more descriptive |
💻 dev — $(date -u +%Y-%m-%dT%H:%M:%SZ)Final Verification — All CodeRabbit Feedback AddressedReviewed both inline comments from @coderabbitai[bot]: ✅ MD040 markdown lint (per-component-credentials-troubleshooting.md)
✅ Validation order bug (generate-credentials.sh)
No new feedback since 2026-05-11T14:51:35Z. Story openshift#41 is complete and closed. |
|
Story ✅ Actions performedComments resolved. Approval is disabled; enable |
💻 dev — 2026-05-12T19:03:06Z✅ All CodeRabbit feedback addressed Verified both inline review comments from @coderabbitai[bot] (2026-05-09T04:24:39Z):
CodeRabbit confirmed both fixes. Story openshift#41 remains closed (completed 2026-05-09). |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Implement credential parsing, validation, and privilege verification for component-specific vCenter credentials. The installer now validates credentials before provisioning begins and fails early with detailed error messages. Implementation: - Define exact privilege requirements for all 5 components - Installer: 49 privileges (comprehensive provisioning) - Machine API: 35 privileges (VM lifecycle management) - Storage: 13 privileges (CSI driver volume operations) - Cloud Controller: 10 privileges (read-only node discovery) - Diagnostics: 16 privileges (vSphere Problem Detector validation) - Implement credential parsing (componentcredentials.go): - ParseComponentCredentials(): parse from install-config - GetCredentialsForVCenter(): multi-vCenter credential lookup - Support single-vCenter (direct credentials) and multi-vCenter (secretRef) - Implement privilege validation (componentvalidation.go): - ValidateComponentCredentials(): validate all components across all vCenters - ValidatePrivileges(): check required privileges per component - FormatValidationReport(): human-readable validation report - ValidationError type with detailed context (component, vCenter, missing privilege) - Comprehensive test coverage: - 7 credential parsing unit tests - 14 validation unit tests - 9 integration test stubs (require govcsim infrastructure) Total: ~704 lines (code + tests) Acceptance criteria: ✅ Parse credentials for all components from install-config.yaml ✅ Validate credential format and connectivity to each vCenter ✅ Check required privileges for each component against each vCenter ✅ Clear error messages with component, vCenter, and missing privilege ✅ Detailed validation report before provisioning ✅ Detect missing privileges during validation ✅ No partial cluster state created on validation failure Dependencies: - Story #16 (API Extensions): Provides ComponentCredentials types ✅ - Integration: Wire into installer pre-flight checks (Story #18) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add comprehensive test stubs for: - Infrastructure CR componentCredentials API validation - Install-config componentCredentials schema validation - Multi-vCenter secret format creation and parsing Test files: - vendor/github.com/openshift/api/config/v1/types_infrastructure_test.go - pkg/types/vsphere/validation/platform_test.go - pkg/asset/manifests/vsphere_secrets_test.go All tests marked with t.Skip() pending implementation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 participant
Summary
Test plan
Closes openshift#41
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests