feat(vsphere): Add VSphereCredentialsMode and ComponentCredentials to VSpherePlatformSpec#2
feat(vsphere): Add VSphereCredentialsMode and ComponentCredentials to VSpherePlatformSpec#2splatypus-bot wants to merge 1 commit into
Conversation
…ls to VSpherePlatformSpec Adds VSphereCredentialsMode (Passthrough/PerComponent) and VSphereComponentCredentials/VSphereComponentSecretRef types to VSpherePlatformSpec in the Infrastructure CR API, enabling per-component vCenter credential isolation for story openshift#36. - VSphereCredentialsModePassthrough (zero value = "") preserves backward compatibility: existing clusters that do not set credentialsMode automatically get Passthrough behavior - VSphereCredentialsModePerComponent enables per-component secret refs for machineAPI, csiDriver, cloudController, and vsphereProblemDetector - DeepCopy methods added to zz_generated.deepcopy.go for the new types - 14 unit tests covering all 3 acceptance criteria and adversarial cases Resolves: openshift-splat-team/splat-team#36 Parent: openshift-splat-team/splat-team#33
📝 WalkthroughWalkthroughThis PR extends the vSphere infrastructure API with credential mode selection and per-component secret references. It introduces ChangesVSphere Credentials API Extension
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 docstrings
🧪 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: 1
🧹 Nitpick comments (2)
config/v1/vsphere_credentials_test.go (2)
60-72: 💤 Low value
TestVSpherePlatformSpec_EmptyStringCredentialsModeis a duplicate ofTestVSpherePlatformSpec_JSONRoundTrip_ExplicitPassthrough.Both tests unmarshal the identical JSON string (
{"credentialsMode": ""}) and assert the same condition (spec.CredentialsMode == VSphereCredentialsModePassthrough). The only difference is the error message wording. One of them should be removed — this inflates the stated test count of 14 by one redundant case and could mislead reviewers into thinking the adversarial section covers a distinct scenario.♻️ Proposed fix: remove the redundant adversarial test
-func TestVSpherePlatformSpec_EmptyStringCredentialsMode(t *testing.T) { - raw := `{"credentialsMode": ""}` - var spec VSpherePlatformSpec - if err := json.Unmarshal([]byte(raw), &spec); err != nil { - t.Fatalf("unmarshal error: %v", err) - } - if spec.CredentialsMode != VSphereCredentialsModePassthrough { - t.Errorf("empty credentialsMode = %q; want Passthrough", spec.CredentialsMode) - } -}Also applies to: 176-185
🤖 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 `@config/v1/vsphere_credentials_test.go` around lines 60 - 72, Remove the redundant test that duplicates behavior of TestVSpherePlatformSpec_JSONRoundTrip_ExplicitPassthrough: locate the duplicate test function TestVSpherePlatformSpec_EmptyStringCredentialsMode (also present around the other occurrence noted) and delete it so only one test asserts that unmarshalling `{"credentialsMode": ""}` yields VSphereCredentialsModePassthrough; ensure there are no remaining duplicate assertions or error-message-only differences after removal.
187-200: ⚡ Quick win
TestVSpherePlatformSpec_ComponentCredentials_NilPointerSafetyhas no meaningful assertion and is effectively a no-op.Two issues:
Trivially-true assertion (lines 192–194):
ComponentCredentialswas just assignednilon line 190, sospec.ComponentCredentials != nilis always false andt.Fatalfis unreachable. This check never catches anything.Dead-code safe-access pattern (lines 195–199): The
if spec.ComponentCredentials != nilbranch is always false, somachineAPICredsis always nil._ = machineAPICredsdiscards the result with no assertion — the nil-safety claim in the test name is never exercised.A more meaningful version would at least assert
machineAPICreds == nilafter the guarded access to confirm the pattern yields the expected result:♻️ Proposed fix: add a meaningful assertion
func TestVSpherePlatformSpec_ComponentCredentials_NilPointerSafety(t *testing.T) { spec := VSpherePlatformSpec{ CredentialsMode: VSphereCredentialsModePassthrough, ComponentCredentials: nil, } - if spec.ComponentCredentials != nil { - t.Fatalf("expected nil ComponentCredentials") - } var machineAPICreds *VSphereComponentSecretRef if spec.ComponentCredentials != nil { machineAPICreds = spec.ComponentCredentials.MachineAPI } - _ = machineAPICreds + if machineAPICreds != nil { + t.Errorf("expected nil machineAPICreds when ComponentCredentials is nil; got %+v", machineAPICreds) + } }🤖 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 `@config/v1/vsphere_credentials_test.go` around lines 187 - 200, The test TestVSpherePlatformSpec_ComponentCredentials_NilPointerSafety currently contains unreachable checks and no real assertion; update it so after initializing spec := VSpherePlatformSpec{ComponentCredentials: nil} you perform the guarded access (if spec.ComponentCredentials != nil { machineAPICreds = spec.ComponentCredentials.MachineAPI }) and then assert that machineAPICreds == nil (e.g. t.Fatalf or t.Errorf on non-nil) to validate the nil-safe access pattern; remove the trivially-true/unreachable t.Fatalf and reference VSpherePlatformSpec, ComponentCredentials, MachineAPI and VSphereComponentSecretRef when locating the code to change.
🤖 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 `@config/v1/types_infrastructure.go`:
- Around line 1674-1680: The Name and Namespace fields in the secret reference
struct are marked Required but lack a minimum-length constraint, so add
kubebuilder validation to enforce non-empty values: add the annotation `//
+kubebuilder:validation:MinLength=1` above both the `Name string
\`json:"name"\`` and `Namespace string \`json:"namespace"\`` declarations in
types_infrastructure.go (the struct fields named Name and Namespace) so the CRD
rejects empty strings.
---
Nitpick comments:
In `@config/v1/vsphere_credentials_test.go`:
- Around line 60-72: Remove the redundant test that duplicates behavior of
TestVSpherePlatformSpec_JSONRoundTrip_ExplicitPassthrough: locate the duplicate
test function TestVSpherePlatformSpec_EmptyStringCredentialsMode (also present
around the other occurrence noted) and delete it so only one test asserts that
unmarshalling `{"credentialsMode": ""}` yields
VSphereCredentialsModePassthrough; ensure there are no remaining duplicate
assertions or error-message-only differences after removal.
- Around line 187-200: The test
TestVSpherePlatformSpec_ComponentCredentials_NilPointerSafety currently contains
unreachable checks and no real assertion; update it so after initializing spec
:= VSpherePlatformSpec{ComponentCredentials: nil} you perform the guarded access
(if spec.ComponentCredentials != nil { machineAPICreds =
spec.ComponentCredentials.MachineAPI }) and then assert that machineAPICreds ==
nil (e.g. t.Fatalf or t.Errorf on non-nil) to validate the nil-safe access
pattern; remove the trivially-true/unreachable t.Fatalf and reference
VSpherePlatformSpec, ComponentCredentials, MachineAPI and
VSphereComponentSecretRef when locating the code to change.
🪄 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: fdc5f779-83b0-4001-8177-da9ef11628e9
📒 Files selected for processing (3)
config/v1/types_infrastructure.goconfig/v1/vsphere_credentials_test.goconfig/v1/zz_generated.deepcopy.go
| // name is the name of the secret. | ||
| // +kubebuilder:validation:Required | ||
| Name string `json:"name"` | ||
|
|
||
| // namespace is the namespace of the secret. | ||
| // +kubebuilder:validation:Required | ||
| Namespace string `json:"namespace"` |
There was a problem hiding this comment.
Enforce non-empty secret reference fields
Name and Namespace are marked required, but empty strings are still valid without length constraints. That allows invalid secret refs to pass schema validation and fail later at runtime. Please add MinLength=1 on both fields (Line 1676, Line 1680).
Suggested diff
type VSphereComponentSecretRef struct {
// name is the name of the secret.
// +kubebuilder:validation:Required
+ // +kubebuilder:validation:MinLength=1
Name string `json:"name"`
// namespace is the namespace of the secret.
// +kubebuilder:validation:Required
+ // +kubebuilder:validation:MinLength=1
Namespace string `json:"namespace"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // name is the name of the secret. | |
| // +kubebuilder:validation:Required | |
| Name string `json:"name"` | |
| // namespace is the namespace of the secret. | |
| // +kubebuilder:validation:Required | |
| Namespace string `json:"namespace"` | |
| // name is the name of the secret. | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinLength=1 | |
| Name string `json:"name"` | |
| // namespace is the namespace of the secret. | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinLength=1 | |
| Namespace string `json:"namespace"` |
🤖 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 `@config/v1/types_infrastructure.go` around lines 1674 - 1680, The Name and
Namespace fields in the secret reference struct are marked Required but lack a
minimum-length constraint, so add kubebuilder validation to enforce non-empty
values: add the annotation `// +kubebuilder:validation:MinLength=1` above both
the `Name string \`json:"name"\`` and `Namespace string \`json:"namespace"\``
declarations in types_infrastructure.go (the struct fields named Name and
Namespace) so the CRD rejects empty strings.
1 participant
Summary
VSphereCredentialsModetype (Passthrough/PerComponent) toconfig/v1CredentialsModeandComponentCredentialsfields toVSpherePlatformSpecVSphereComponentCredentialsandVSphereComponentSecretReftypesDeepCopymethods for new types tozz_generated.deepcopy.goVSphereCredentialsModePassthroughuses""(empty string) as its value so the Go zero value equalsPassthrough, preserving backward compatibility for existing clusters that do not setcredentialsMode.go test output
Implementation Notes
VSphereCredentialsModePassthrough = ""so the Go zero value isPassthrough;omitemptyomits it from JSON for existing clusters — no custom marshaler neededVSphereCredentialsModePerComponent = "PerComponent"VSphereProblemDetector(matching updated design doc, notDiagnostics)Closes openshift-splat-team/splat-team#36
Summary by CodeRabbit
New Features
Tests