Open
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-ci-robot
added
the
jira/valid-reference
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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. |
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
29af27f to
5249404
Compare
Member
Author
|
@jianping-shu this passed e2e-vsphere, so I reckon it's probably ready for you to take another stab at it! |
Member
Author
|
/hold Looks like I missed refactoring the preflight auth check for the new creds shape. |
openshift-ci
bot
added
the
do-not-merge/hold
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
5249404 to
39cf13e
Compare
Member
Author
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
dlom
reviewed
Feb 18, 2026
dlom
reviewed
Feb 18, 2026
dlom
reviewed
Feb 18, 2026
dlom
reviewed
Feb 18, 2026
Contributor
|
The new multi-creds changes LGTM. My only concern (as noted in review comments) is that there are some additional fields (at least |
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
39cf13e to
896e851
Compare
Member
Author
|
/hold for QE |
openshift-ci
bot
added
the
do-not-merge/hold
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
896e851 to
7996e22
Compare
Member
Author
|
/test e2e security e2e: infra flake |
Member
Author
|
Allowable to override |
pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
Outdated
Show resolved
Hide resolved
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
7996e22 to
066a4e4
Compare
jianping-shu
reviewed
Mar 5, 2026
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
ca2765c to
0746f91
Compare
Member
Author
|
/test e2e Actual test passed; infra flake during must-gather. security is same old same old, override when needed. |
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
0746f91 to
aa9dcc3
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates Hive vSphere config from single-vCenter, field-based settings to multi-vCenter, installer-style Infrastructure. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant API_Server
participant Controller
participant vsphereutils
participant InstallerTypes
participant CredsSecret
participant Deprovision
User->>CLI: Run create-cluster (--vsphere-platform-spec-json)
CLI->>API_Server: create ClusterDeployment (Platform.Infrastructure populated)
API_Server->>Controller: ADDED/RECONCILE event
Controller->>vsphereutils: if deprecated fields present -> ConvertDeprecatedFields
vsphereutils->>InstallerTypes: build installer InstallConfig.Platform (vSphere)
vsphereutils-->Controller: return Infrastructure populated
Controller->>CredsSecret: load vcenters secret (per-vCenter creds)
Controller->>InstallerTypes: build MachineSets using Infrastructure topology
User->>Deprovision: request deprovision
Deprovision->>InstallerTypes: read metadata.VCenters for cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
Updated for the issue @jianping-shu found where we were using the same Latest push also includes a trivial rebase. |
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/creds/vsphere/vsphere.go (1)
18-20:⚠️ Potential issue | 🟡 MinorDocstring is outdated.
The docstring still mentions "configures VSphere credential environment variables" but the implementation now populates
metadata.VSphere.VCentersinstead.📝 Suggested docstring update
-// ConfigureCreds loads secrets designated by the environment variables CLUSTERDEPLOYMENT_NAMESPACE, -// CREDS_SECRET_NAME, and CERTS_SECRET_NAME and configures VSphere credential environment variables -// and config files accordingly. +// ConfigureCreds loads secrets designated by the environment variables CLUSTERDEPLOYMENT_NAMESPACE, +// CREDS_SECRET_NAME, and CERTS_SECRET_NAME. It populates metadata.VSphere.VCenters with credentials +// and installs VSphere certificates accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/creds/vsphere/vsphere.go` around lines 18 - 20, Update the ConfigureCreds docstring to reflect the current behavior: instead of saying it "configures VSphere credential environment variables," state that ConfigureCreds loads secrets and populates metadata.VSphere.VCenters (and any related metadata fields) with the vCenter connection information; mention which environment variables it reads (CLUSTERDEPLOYMENT_NAMESPACE, CREDS_SECRET_NAME, CERTS_SECRET_NAME) and that it writes into metadata.VSphere.VCenters rather than setting env vars so readers can locate the changed behavior in the ConfigureCreds implementation.
🟡 Minor comments (6)
pkg/install/generate.go-892-903 (1)
892-903:⚠️ Potential issue | 🟡 MinorConsider handling the case where both VCenters and DeprecatedVCenter are empty.
If
VCentersis an empty slice andDeprecatedVCenteris also empty,joinedVCenterswill be an empty string, resulting in--vsphere-vcenters ""being passed to the deprovision command. This may cause unexpected behavior or unclear error messages downstream.🛡️ Suggested validation
if legacy { var joinedVCenters string if len(req.Spec.Platform.VSphere.VCenters) == 0 && req.Spec.Platform.VSphere.DeprecatedVCenter != "" { joinedVCenters = req.Spec.Platform.VSphere.DeprecatedVCenter } else { // StringSliceVar accepts comma-delimited joinedVCenters = strings.Join(req.Spec.Platform.VSphere.VCenters, ",") } + if joinedVCenters == "" { + // This shouldn't happen if the ClusterDeprovision was created correctly, + // but log a warning for debugging purposes + // Consider returning an error or using a sensible default + } args = []string{ "deprovision", "vsphere", "--vsphere-vcenters", joinedVCenters, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/install/generate.go` around lines 892 - 903, The code builds joinedVCenters from req.Spec.Platform.VSphere.VCenters / DeprecatedVCenter and always appends "--vsphere-vcenters", joinedVCenters to args; handle the case where both are empty by either returning a validation error or omitting the flag: check req.Spec.Platform.VSphere.VCenters and req.Spec.Platform.VSphere.DeprecatedVCenter and if both are empty do one of two things (preferred: return an error from the generator indicating missing vCenter info; alternative: only append "--vsphere-vcenters" to args when joinedVCenters != ""), updating the logic around joinedVCenters and the args slice (referencing joinedVCenters, req.Spec.Platform.VSphere.VCenters, DeprecatedVCenter, and args) so we never pass an empty "--vsphere-vcenters" value.docs/hiveutil.md-108-115 (1)
108-115:⚠️ Potential issue | 🟡 MinorThe required-parameters table still reflects the pre-JSON flow.
The example below puts
apiVIPsandingressVIPsinside--vsphere-platform-spec-json, but this table still marks--vsphere-api-vipand--vsphere-ingress-vipas required. Please remove those rows or clarify they are only needed when the JSON omits the VIPs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/hiveutil.md` around lines 108 - 115, The table in docs/hiveutil.md incorrectly lists --vsphere-api-vip and --vsphere-ingress-vip as required even though the JSON flow (VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json) can contain apiVIPs and ingressVIPs; update the table to either remove the two VIP rows or add a clarifying note that --vsphere-api-vip and --vsphere-ingress-vip are only required when VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json does not include apiVIPs/ingressVIPs, referencing the exact flags (--vsphere-platform-spec-json, --vsphere-api-vip, --vsphere-ingress-vip) so readers understand the conditional requirement.config/crds/hive.openshift.io_clusterpools.yaml-1345-1348 (1)
1345-1348:⚠️ Potential issue | 🟡 MinorFix the migration targets in the deprecation text.
These hints point users to
FailureDomains.Topology.NetworkandVCenters.Username, but the schema here exposesfailureDomains[].topology.networksandvcenters[].user. Since this text is surfaced bykubectl explain, the current wording sends users to fields that do not exist.Also applies to: 1427-1430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crds/hive.openshift.io_clusterpools.yaml` around lines 1345 - 1348, Update the deprecation descriptions to reference the actual schema fields exposed: change the Network deprecation target from FailureDomains.Topology.Network to failureDomains[].topology.networks, and change the vcenter username deprecation target from VCenters.Username to vcenters[].user; locate the description blocks for the Network field (currently mentioning FailureDomains.Topology.Network) and the vcenter username field (the block that mentions VCenters.Username) and replace the incorrect dotted paths with the exact field names shown above so kubectl explain points to existing fields.hack/app-sre/saas-template.yaml-2948-2953 (1)
2948-2953:⚠️ Potential issue | 🟡 MinorFix the deprecated field path in the description.
The migration target says
FailureDomains.Topology.Network, but the schema definestopology.networks. As written,kubectl explainpoints users at a field that does not exist.Also applies to: 6925-6930
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/app-sre/saas-template.yaml` around lines 2948 - 2953, Update the deprecated field path in the description for the network field: change the migration target from "FailureDomains.Topology.Network" to the actual schema path "FailureDomains.Topology.networks" so kubectl explain points to the correct field; locate the description text for the network field (symbol: network) and replace the incorrect path, and make the same change for the other occurrence referenced (the second network description block).config/crds/hive.openshift.io_clusterdeployments.yaml-1684-1692 (1)
1684-1692:⚠️ Potential issue | 🟡 Minor
format: ipannotation is misplaced on array type (same asapiVIPs).Same issue as
apiVIPs- the format annotation should be on the items type, not the array container.Suggested fix
ingressVIPs: description: |- IngressVIPs contains the VIP(s) for ingress. In dual stack clusters it contains an IPv4 and IPv6 address, otherwise only one VIP - format: ip items: type: string + format: ip maxItems: 2 type: array🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crds/hive.openshift.io_clusterdeployments.yaml` around lines 1684 - 1692, The schema for ingressVIPs wrongly applies "format: ip" to the array node; move the format annotation to the array items like done for apiVIPs so each string item has format: ip, i.e., remove or relocate the top-level format under ingressVIPs and add format: ip under ingressVIPs.items (the items.type: string node) to validate each IP entry correctly.config/crds/hive.openshift.io_clusterdeployments.yaml-1329-1337 (1)
1329-1337:⚠️ Potential issue | 🟡 MinorMove
format: ipfrom array to items schema level.In OpenAPI 3.0, format validation for array items must be specified in the
itemsschema, not on the array container. This issue originates in the Go source annotation and needs to be fixed there via kubebuilder markers.Source fix location
In
vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go(lines 92-98), the kubebuilder annotation+kubebuilder:validation:Format=ipon theAPIVIPs []stringfield should be adjusted so controller-gen generates the correct schema structure with format inside the items definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crds/hive.openshift.io_clusterdeployments.yaml` around lines 1329 - 1337, The array-level `+kubebuilder:validation:Format=ip` must be applied to the element type so the generated OpenAPI puts format inside `items`; fix by defining a new named element type (e.g., type APIVIP string) with the kubebuilder annotation `+kubebuilder:validation:Format=ip` on that type, then change the field signature from `APIVIPs []string` to `APIVIPs []APIVIP` (keep existing min/max item annotations on the slice); update any imports/usage accordingly so controller-gen emits `items.format: ip`.
🧹 Nitpick comments (8)
pkg/controller/utils/vsphereutils/vsphereutils_test.go (1)
12-84: Add a case for the pre-populatedInfrastructurepath.
ConvertDeprecatedFieldsis also a no-op whenplatform.Infrastructureis already set, and that is the path native zonal configs will take. This test only exercises the legacy-conversion branch, so a future change could accidentally overwrite existing Infrastructure without any test failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/utils/vsphereutils/vsphereutils_test.go` around lines 12 - 84, Add a new subtest in TestConvertDeprecatedFields that pre-populates hivevsphere.Platform.Infrastructure with a non-nil installervsphere.Platform value and asserts ConvertDeprecatedFields does not modify it: construct a platform with Infrastructure set (instead of only deprecated fields), set Expected to the same installervsphere.Platform instance, call ConvertDeprecatedFields(platform), require.NoError, and compare platform.Infrastructure to Expected to ensure the function is a no-op when platform.Infrastructure is already present (reference symbols: TestConvertDeprecatedFields, ConvertDeprecatedFields, hivevsphere.Platform.Infrastructure, installervsphere.Platform).apis/hive/v1/clusterdeprovision_types.go (1)
135-140: Consider addingomitemptyto the deprecated field.The
DeprecatedVCenterfield lacksomitempty, which means it will always be serialized even when empty. For cleaner JSON output on new resources, consider:📝 Suggested change
// DeprecatedVCenter is the vSphere vCenter hostname. // Deprecated: use VCenters instead. // +optional - DeprecatedVCenter string `json:"vCenter"` + DeprecatedVCenter string `json:"vCenter,omitempty"` // VCenters are potentially multiple vCenter hostnames. Prefer this field over VCenter. - VCenters []string `json:"vCenters"` + VCenters []string `json:"vCenters,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/hive/v1/clusterdeprovision_types.go` around lines 135 - 140, The DeprecatedVCenter struct field currently uses `json:"vCenter"` and will always be serialized even when empty; update its JSON tag to include `omitempty` (i.e., change the tag for DeprecatedVCenter on the struct to `json:"vCenter,omitempty"`) so empty deprecated values are omitted from JSON output while leaving VCenters unchanged; locate the DeprecatedVCenter field in the same struct definition (symbol: DeprecatedVCenter) and update only the tag to preserve backward compatibility.apis/hive/v1/vsphere/platform.go (1)
14-16: Minor documentation inconsistency in deprecation comments.The deprecation comments reference
Platform.ConvertDeprecatedFields, but the actual function isvsphereutils.ConvertDeprecatedFields(inpkg/controller/utils/vsphereutils/). Consider updating to reference the correct location for clarity.📝 Suggested documentation fix
// VCenter is the domain name or IP address of the vCenter. // Deprecated: Please use Platform.Infrastructure instead - // See also: Platform.ConvertDeprecatedFields + // See also: vsphereutils.ConvertDeprecatedFields // +optional DeprecatedVCenter string `json:"vCenter,omitempty"`Apply similar changes to the other deprecated field comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/hive/v1/vsphere/platform.go` around lines 14 - 16, Update the deprecation comment to reference the correct function name/location: replace the incorrect reference to Platform.ConvertDeprecatedFields with vsphereutils.ConvertDeprecatedFields (the actual function in pkg/controller/utils/vsphereutils) in the comment above the deprecated field; apply the same fix to any other deprecated-field comments that mention Platform.ConvertDeprecatedFields so they consistently point to vsphereutils.ConvertDeprecatedFields.pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go (1)
1901-1926: Consider makingexpectedAllowed: falseexplicit in test cases.Several test cases (lines 1901-1906, 1907-1916, 1917-1926) rely on the default zero value of
expectedAllowedbeingfalse. While functionally correct, making this explicit improves test readability and makes the expected behavior immediately clear.📝 Suggested clarification
{ name: "vsphere platform cannot be de-zonal-ified", oldObject: validVSphereClusterDeployment(), newObject: deprecatedVSphereClusterDeployment(), operation: admissionv1beta1.Update, + expectedAllowed: false, }, { name: "vsphere platform immutable if not being zonal-ified (deprecated shape)", oldObject: func() *hivev1.ClusterDeployment { cd := deprecatedVSphereClusterDeployment() cd.Spec.Platform.VSphere.CredentialsSecretRef.Name = "changed" return cd }(), newObject: deprecatedVSphereClusterDeployment(), operation: admissionv1beta1.Update, + expectedAllowed: false, }, { name: "vsphere platform immutable if not being zonal-ified (zonal shape)", oldObject: func() *hivev1.ClusterDeployment { cd := validVSphereClusterDeployment() cd.Spec.Platform.VSphere.Infrastructure.ClusterOSImage = "changed" return cd }(), newObject: validVSphereClusterDeployment(), operation: admissionv1beta1.Update, + expectedAllowed: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go` around lines 1901 - 1926, Add explicit expectedAllowed: false to the three test case entries named "vsphere platform cannot be de-zonal-ified", "vsphere platform immutable if not being zonal-ified (deprecated shape)", and "vsphere platform immutable if not being zonal-ified (zonal shape)" in the test table in clusterdeployment_validating_admission_hook_test.go; locate each case (they use oldObject/newObject with validVSphereClusterDeployment() or deprecatedVSphereClusterDeployment() and operation admissionv1beta1.Update) and add the field expectedAllowed: false to make the intended failure expectation explicit and improve readability.pkg/clusterresource/builder_test.go (1)
145-175: Assert the generated vSphereInfrastructuretoo.This fixture now exercises the new
VSphereCloudBuilder.Infrastructurepath, but the paired test still only checks Secret refs. A regression that drops or rewritescd.Spec.Platform.VSphere.Infrastructurewould still pass. Please add at least one assertion for the propagatedVCenters,FailureDomains, and VIPs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/clusterresource/builder_test.go` around lines 145 - 175, Add assertions in the test that verify the generated ClusterDeployment/ClusterSpec Platform.VSphere.Infrastructure was populated from the fixture: assert cd.Spec.Platform.VSphere.Infrastructure.VCenters contains an entry with Server "test", Port 123 and Datacenters ["test"]; assert Infrastructure.FailureDomains contains a domain with Name "test", Region "test", Zone "test" and Topology.Datacenter "test" (optionally check other Topology fields like ComputeCluster); and assert Infrastructure.APIVIPs and IngressVIPs equal ["192.168.0.2"] and ["192.168.0.3"] respectively. Locate the assertions near the existing Secret-ref checks that currently validate the VSphereCloudBuilder.Infrastructure path (use the test's cd or cluster result variable) and add strict equality checks for these fields.contrib/pkg/createcluster/create.go (2)
772-831: Redundant credential defaulting loop.Lines 824-831 duplicate the credential defaulting logic from lines 773-780. When using the JSON platform spec path, both loops execute on the same
platform.VCentersslice. When using the legacy path,platform.VCentersis empty, making the second loop a no-op.Consider removing one of the loops to avoid confusion.
♻️ Suggested consolidation
if len(platformBytes) > 0 { err = json.Unmarshal(platformBytes, &platform) if err != nil { return nil, fmt.Errorf("error decoding platform %s: %w", o.VSpherePlatformSpecJSON, err) } - - // Set credentials on VCenters if using new structure - for i := range platform.VCenters { - if platform.VCenters[i].Username == "" { - platform.VCenters[i].Username = vsphereUsername - } - if platform.VCenters[i].Password == "" { - platform.VCenters[i].Password = vspherePassword - } - } } else { // ... legacy flags handling ... } + // Set credentials on VCenters (applies to JSON path; no-op for legacy) for i := range platform.VCenters { if platform.VCenters[i].Username == "" { platform.VCenters[i].Username = vsphereUsername } if platform.VCenters[i].Password == "" { platform.VCenters[i].Password = vspherePassword } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/createcluster/create.go` around lines 772 - 831, There are two identical loops setting platform.VCenters[i].Username/Password from vsphereUsername and vspherePassword (iterating platform.VCenters) which is duplicated; remove the redundant loop and keep a single credential-defaulting loop for platform.VCenters (use the existing loop that runs when the JSON platform spec is processed or consolidate into one after the platform population logic), ensuring the code still sets Username/Password for each entry in platform.VCenters and that legacy fields (platform.Deprecated*) remain untouched.
833-838: Single-element VIP slices may be incomplete for multi-VIP scenarios.When
platform.APIVIPsorplatform.IngressVIPsare empty, the code defaults to a single-element slice from the legacy flags. This works for single-VIP setups but may not align with zonal deployments requiring multiple VIPs.Consider whether this defaulting behavior should warn users or validate VIP count against failure domains.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/createcluster/create.go` around lines 833 - 838, The existing fallback that sets platform.APIVIPs = []string{o.VSphereAPIVIP} and platform.IngressVIPs = []string{o.VSphereIngressVIP} can produce an insufficient single-VIP configuration for multi-VIP (zonal) setups; update the code where platform.APIVIPs/IngressVIPs are defaulted to first legacy VIP to first check the configured failure domain count and either (a) log a clear warning (using the repo logger) when only one VIP is being injected but multiple failure domains exist, or (b) fail validation by returning an error indicating the user must supply one VIP per failure domain; reference platform.APIVIPs, platform.IngressVIPs, o.VSphereAPIVIP and o.VSphereIngressVIP in your change and perform the count comparison against the cluster failure domains before accepting the single-VIP fallback.pkg/clusterresource/vsphere.go (1)
35-44:NewVSphereCloudBuilderFromSecretdoes not initialize Infrastructure.This constructor only sets
Username,Password, andCACert, leavingInfrastructurenil. Callers using this constructor will need to setInfrastructureseparately, otherwiseGetCloudPlatformandaddInstallConfigPlatformwill produce incomplete configurations.Consider documenting this expectation or adding an
Infrastructureparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/clusterresource/vsphere.go` around lines 35 - 44, NewVSphereCloudBuilderFromSecret currently returns a VSphereCloudBuilder with Username, Password and CACert but leaves Infrastructure nil; change the constructor to accept an Infrastructure parameter (e.g. add infrastructure *configv1.Infrastructure to NewVSphereCloudBuilderFromSecret’s signature) and set the returned VSphereCloudBuilder.Infrastructure = infrastructure (or otherwise populate Infrastructure appropriately) so callers of GetCloudPlatform and addInstallConfigPlatform receive a fully-initialized VSphereCloudBuilder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/crds/hive.openshift.io_clusterdeployments.yaml`:
- Around line 1637-1662: The CRD schema incorrectly sets format: ipv6 for the
gateway, ipAddrs, and nameservers properties (symbols: gateway, ipAddrs,
nameservers) which rejects IPv4; fix by removing the single format: ipv6 entries
for these fields so no single-format validation is enforced, or replace them
with a pattern-based validation that matches both IPv4 and IPv6/CIDR formats
(use a single regex for addresses/CIDRs), and/or adjust controller-gen
configuration so it does not collapse multiple +kubebuilder:validation:Format
annotations into a single format; update the YAML for the three properties
accordingly to allow both IPv4 and IPv6.
In `@config/crds/hive.openshift.io_clusterdeprovisions.yaml`:
- Around line 355-368: The schema currently marks vCenters as required which
blocks legacy objects that only set vCenter; make vCenters optional in the CRD
so backward-compatible ClusterDeprovision objects using the legacy vCenter field
are accepted, and move the validation that requires either a legacy vCenter or a
non-empty vCenters array out of the CRD and into higher-level validation logic
(controller/validator) that checks "vCenter OR non-empty vCenters" at runtime;
update/remove vCenters from the required list and ensure the controller enforces
the combined validation.
In `@config/crds/hive.openshift.io_clusterpools.yaml`:
- Around line 1263-1288: The schema for the host-networking fields is too
restrictive: update the gateway and nameservers properties to accept both IPv4
and IPv6 by replacing their single "format: ipv6" with an anyOf combining
"format: ipv4" and "format: ipv6" for the string items (refer to the gateway and
nameservers property definitions), and change ipAddrs from "format: ipv6" to a
CIDR-aware format by using "format: cidr" for the array items (refer to the
ipAddrs property) so the schema matches the documented examples and accepts
IPv4, IPv6 and CIDR values.
- Around line 955-963: The OpenAPI `format: ip` is applied to the array node for
apiVIPs and ingressVIPs so individual entries aren’t validated; update the CRD
source markers for spec.platform.vsphere.infrastructure.apiVIPs and
spec.platform.vsphere.infrastructure.ingressVIPs so that `format: ip` is moved
from the array level into the `items` schema (i.e., ensure items declares type:
string and format: ip) and remove the top-level format from the array
declaration so controller-gen emits per-element IP validation.
In `@contrib/pkg/deprovision/vsphere.go`:
- Around line 50-72: After calling vspherecreds.ConfigureCreds(client,
o.metadata) ensure the legacy-path guard: check
o.metadata.ClusterPlatformMetadata.VSphere (or o.metadata.VSphere) and that
o.metadata.VSphere.VCenters has length > 0; if not, return a clear error (e.g.,
wrap with "no vCenter configured: provide --vsphere-vcenters or use new-style
creds Secret") so the command fails fast when the legacy credentials path
produced an empty VCenters slice; place this validation immediately after
ConfigureCreds() and reference the
o.vCenters/ConfigureCreds/metadata.VSphere.VCenters symbols when making the
change.
In `@hack/app-sre/saas-template.yaml`:
- Around line 3083-3129: The vCenter credentials are being stored directly in
the CR spec via infrastructure.vcenters[*].user and
infrastructure.vcenters[*].password which leaks secrets; remove those fields
from the schema and instead reference an existing credentialsSecretRef (or add a
credentialsSecretRef field if missing) so the CR only contains a reference to a
Kubernetes Secret; update the vcenters schema (the object under vcenters → items
→ properties) to delete or mark user/password as deprecated and document using
credentialsSecretRef, and update any validation/required lists (and related
ClusterDeployment/ClusterPool references) to ensure credentials are retrieved
from the referenced Secret rather than stored in the CR.
- Around line 2605-2627: The failureDomains array allows duplicate names which
makes hosts[].failureDomain (and VSpherePlatformDeploymentZone references)
ambiguous; change the failureDomains list to a keyed map by adding Kubernetes
list-type metadata (e.g. add x-kubernetes-list-type: "map" and
x-kubernetes-list-map-keys: ["name"] on the failureDomains schema) so entries
are keyed by the name property, or alternatively add an explicit uniqueness
constraint keyed on name; apply the same change to the other occurrences you
noted (around the other specified ranges) so all
VSpherePlatformFailureDomainSpec definitions enforce unique name keys.
- Around line 4958-4972: The schema currently marks vCenters as required which
breaks legacy manifests that only set the deprecated vCenter; update the schema
so validation accepts either the singular vCenter or the plural vCenters (e.g.,
remove vCenters from the required list or replace the required array with a
oneOf/anyOf constraint that allows either vCenter (string) or vCenters (array of
string)); ensure you reference and adjust the vCenter and vCenters property
definitions and the required list/validation block so older ClusterDeprovision
manifests validate successfully.
- Around line 2835-2873: The CRD schema currently restricts gateway, ipAddrs
items, and nameservers items to format: ipv6 only; change each of those string
schemas (gateway, the items schema under ipAddrs, and the items schema under
nameservers) to accept both IPv4 and IPv6 by replacing the single "format: ipv6"
with an anyOf/oneOf that allows format: ipv4 and format: ipv6 (e.g., anyOf: -
type: string; format: ipv4 - type: string; format: ipv6), and apply the same
change to the second occurrence noted in the comment so validation accepts
either family while keeping examples intact.
In `@pkg/controller/clusterdeployment/installconfigvalidation.go`:
- Around line 67-72: The credential check is inconsistent: hasCreds is set true
when either ic.Platform.VSphere.DeprecatedUsername OR DeprecatedPassword is
non-empty (using ||) but for each ic.Platform.VSphere.VCenters it requires both
vcenter.Username AND vcenter.Password (using &&); change the deprecated-path
check to require both DeprecatedUsername and DeprecatedPassword (use &&) so
hasCreds is true only when both credentials are present, matching the VCenters
validation logic (refer to hasCreds, ic.Platform.VSphere.DeprecatedUsername,
DeprecatedPassword, ic.Platform.VSphere.VCenters, vcenter.Username,
vcenter.Password).
In `@pkg/controller/clusterpool/clusterpool_controller.go`:
- Around line 322-331: The migration updates deprecated vSphere fields which
changes the value hashed by calculatePoolVersion(), causing legacy members to
appear stale; fix by normalizing vSphere fields before any pool-version is
computed or by rewriting member annotations during the migration: either (A)
ensure vsphereutils.ConvertDeprecatedFields(clp.Spec.Platform.VSphere) is
invoked (on a DeepCopy of the ClusterPool as you already do) prior to any call
to calculatePoolVersion() so existing hashing uses the normalized Platform
(search for calculatePoolVersion()), or (B) after r.Update(ctx, clp) iterate the
ClusterPool members and overwrite their pool-version annotations with the
recalculated hash (use the same calculatePoolVersion() and the same annotation
key used elsewhere) so members and pool stay consistent.
In `@pkg/controller/machinepool/vsphereactuator.go`:
- Around line 51-65: The code builds an installertypes.InstallConfig named ic by
assigning cd.Spec.Platform.VSphere.Infrastructure directly to ic.VSphere, then
mutates ic.VSphere.FailureDomains which also mutates the shared
ClusterDeployment object; to fix, replace the direct assignment with a deep copy
of cd.Spec.Platform.VSphere.Infrastructure (use the type's DeepCopy method or
create a copy struct) before assigning to ic.VSphere so subsequent mutations to
failureDomain.Topology.ResourcePool and failureDomain.Topology.TagIDs (based on
pool.Spec.Platform.VSphere.ResourcePool and pool.Spec.Platform.VSphere.TagIDs)
only affect the local ic and not cd.
In `@pkg/controller/utils/credentials.go`:
- Around line 84-117: The code currently succeeds if it validates only the
vcenters it could materialize; change the logic in the validation path that
builds the vcenters slice so the function fails if the resolved credential set
is incomplete: after constructing vcenters (both the "vcenters" yaml path and
the legacy projection path), ensure vcenters is non-empty and contains
credentials for every entry in cd.Spec.Platform.VSphere.Infrastructure.VCenters
(match by Server/VCenter name), returning an error if any configured VCenter is
missing or if required secret keys
(constants.UsernameSecretKey/constants.PasswordSecretKey) are absent; only then
iterate and call validateVSphereCredentials for each entry and return success.
In `@pkg/creds/vsphere/vsphere.go`:
- Around line 34-42: The old-style credentials branch assings username/password
into metadata.VSphere.VCenters without checking metadata.VSphere for nil,
risking a panic; update the code handling the old-style creds (the block that
reads credsSecret.Data[constants.UsernameSecretKey] and
constants.PasswordSecretKey and loops over metadata.VSphere.VCenters) to first
verify metadata.VSphere is non-nil (and if it is nil either return a clear error
or initialize metadata.VSphere with an appropriate VCenters slice, matching the
post-zonal defensiveness), then proceed to set each VCenter's Username and
Password.
In `@pkg/installmanager/installmanager.go`:
- Around line 739-752: The code iterates over
cd.Spec.Platform.VSphere.Infrastructure.VCenters without checking that
Infrastructure is non-nil which can panic; update the block that builds vcenters
(where vcenters is made and populated and where installertypes.ClusterMetadata /
installertypesvsphere.Metadata are constructed) to first check if
cd.Spec.Platform.VSphere.Infrastructure != nil and only range over
Infrastructure.VCenters when non-nil, otherwise leave vcenters as an empty slice
(or set VCenters to nil/empty) so creation of installertypes.ClusterMetadata and
installertypesvsphere.Metadata is safe.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go`:
- Around line 671-678: The current change overwrites
oldObject.Spec.Platform.VSphere with the incoming struct which masks unrelated
immutable edits; instead, create a normalized copy of the old vSphere struct
(e.g., normalizedOldVSphere := oldObject.Spec.Platform.VSphere.DeepCopy() or
equivalent), set only its Infrastructure field to
cd.Spec.Platform.VSphere.Infrastructure, and pass that normalized copy into the
immutability check (hasChangedImmutableField or whatever call uses the old
value) rather than assigning cd.Spec.Platform.VSphere back onto
oldObject.Spec.Platform.VSphere; this preserves original credential/certificate
and other immutable fields for comparison while allowing the zonal
Infrastructure migration.
- Around line 535-540: The validation currently conflates legacy (pre-zonal) and
new shaped vSphere specs; change the logic so that when vsphere.Infrastructure
is nil or empty you validate the legacy topology fields (e.g. require
vsphere.DeprecatedVCenter and its legacy fields), and only when
vsphere.Infrastructure is provided/has entries enforce the new shape (require
vsphere.Infrastructure.VCenters). Update the conditional around
vsphere.DeprecatedVCenter / vsphere.Infrastructure to branch: if Infrastructure
== nil or len(VCenters)==0 -> run legacy-required checks (using
vsphere.DeprecatedVCenter and legacy field paths via vspherePath), else ->
require vsphere.Infrastructure.VCenters; keep references to
vsphere.DeprecatedVCenter, vsphere.Infrastructure,
vsphere.Infrastructure.VCenters and vspherePath to locate and adjust the checks.
In `@pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go`:
- Around line 237-245: The early return inside the HIVE-2391 block
short-circuits update validation and skips validateClusterPoolPlatform; instead,
after detecting the legacy→zonal transition
(oldObject.Spec.Platform.VSphere.Infrastructure == nil &&
newObject.Spec.Platform.VSphere.Infrastructure != nil) keep the debug log but
call validateClusterPoolPlatform for the new object and return its
AdmissionResponse (or combine its result) rather than immediately returning
Allowed: true — update the logic around the HIVE-2391 check so
validateClusterPoolPlatform(newObject, ...) is invoked before allowing the
change.
---
Outside diff comments:
In `@pkg/creds/vsphere/vsphere.go`:
- Around line 18-20: Update the ConfigureCreds docstring to reflect the current
behavior: instead of saying it "configures VSphere credential environment
variables," state that ConfigureCreds loads secrets and populates
metadata.VSphere.VCenters (and any related metadata fields) with the vCenter
connection information; mention which environment variables it reads
(CLUSTERDEPLOYMENT_NAMESPACE, CREDS_SECRET_NAME, CERTS_SECRET_NAME) and that it
writes into metadata.VSphere.VCenters rather than setting env vars so readers
can locate the changed behavior in the ConfigureCreds implementation.
---
Minor comments:
In `@config/crds/hive.openshift.io_clusterdeployments.yaml`:
- Around line 1684-1692: The schema for ingressVIPs wrongly applies "format: ip"
to the array node; move the format annotation to the array items like done for
apiVIPs so each string item has format: ip, i.e., remove or relocate the
top-level format under ingressVIPs and add format: ip under ingressVIPs.items
(the items.type: string node) to validate each IP entry correctly.
- Around line 1329-1337: The array-level `+kubebuilder:validation:Format=ip`
must be applied to the element type so the generated OpenAPI puts format inside
`items`; fix by defining a new named element type (e.g., type APIVIP string)
with the kubebuilder annotation `+kubebuilder:validation:Format=ip` on that
type, then change the field signature from `APIVIPs []string` to `APIVIPs
[]APIVIP` (keep existing min/max item annotations on the slice); update any
imports/usage accordingly so controller-gen emits `items.format: ip`.
In `@config/crds/hive.openshift.io_clusterpools.yaml`:
- Around line 1345-1348: Update the deprecation descriptions to reference the
actual schema fields exposed: change the Network deprecation target from
FailureDomains.Topology.Network to failureDomains[].topology.networks, and
change the vcenter username deprecation target from VCenters.Username to
vcenters[].user; locate the description blocks for the Network field (currently
mentioning FailureDomains.Topology.Network) and the vcenter username field (the
block that mentions VCenters.Username) and replace the incorrect dotted paths
with the exact field names shown above so kubectl explain points to existing
fields.
In `@docs/hiveutil.md`:
- Around line 108-115: The table in docs/hiveutil.md incorrectly lists
--vsphere-api-vip and --vsphere-ingress-vip as required even though the JSON
flow (VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json) can
contain apiVIPs and ingressVIPs; update the table to either remove the two VIP
rows or add a clarifying note that --vsphere-api-vip and --vsphere-ingress-vip
are only required when VSPHERE_INSTALLER_PLATFORM_SPEC_JSON /
--vsphere-platform-spec-json does not include apiVIPs/ingressVIPs, referencing
the exact flags (--vsphere-platform-spec-json, --vsphere-api-vip,
--vsphere-ingress-vip) so readers understand the conditional requirement.
In `@hack/app-sre/saas-template.yaml`:
- Around line 2948-2953: Update the deprecated field path in the description for
the network field: change the migration target from
"FailureDomains.Topology.Network" to the actual schema path
"FailureDomains.Topology.networks" so kubectl explain points to the correct
field; locate the description text for the network field (symbol: network) and
replace the incorrect path, and make the same change for the other occurrence
referenced (the second network description block).
In `@pkg/install/generate.go`:
- Around line 892-903: The code builds joinedVCenters from
req.Spec.Platform.VSphere.VCenters / DeprecatedVCenter and always appends
"--vsphere-vcenters", joinedVCenters to args; handle the case where both are
empty by either returning a validation error or omitting the flag: check
req.Spec.Platform.VSphere.VCenters and
req.Spec.Platform.VSphere.DeprecatedVCenter and if both are empty do one of two
things (preferred: return an error from the generator indicating missing vCenter
info; alternative: only append "--vsphere-vcenters" to args when joinedVCenters
!= ""), updating the logic around joinedVCenters and the args slice (referencing
joinedVCenters, req.Spec.Platform.VSphere.VCenters, DeprecatedVCenter, and args)
so we never pass an empty "--vsphere-vcenters" value.
---
Nitpick comments:
In `@apis/hive/v1/clusterdeprovision_types.go`:
- Around line 135-140: The DeprecatedVCenter struct field currently uses
`json:"vCenter"` and will always be serialized even when empty; update its JSON
tag to include `omitempty` (i.e., change the tag for DeprecatedVCenter on the
struct to `json:"vCenter,omitempty"`) so empty deprecated values are omitted
from JSON output while leaving VCenters unchanged; locate the DeprecatedVCenter
field in the same struct definition (symbol: DeprecatedVCenter) and update only
the tag to preserve backward compatibility.
In `@apis/hive/v1/vsphere/platform.go`:
- Around line 14-16: Update the deprecation comment to reference the correct
function name/location: replace the incorrect reference to
Platform.ConvertDeprecatedFields with vsphereutils.ConvertDeprecatedFields (the
actual function in pkg/controller/utils/vsphereutils) in the comment above the
deprecated field; apply the same fix to any other deprecated-field comments that
mention Platform.ConvertDeprecatedFields so they consistently point to
vsphereutils.ConvertDeprecatedFields.
In `@contrib/pkg/createcluster/create.go`:
- Around line 772-831: There are two identical loops setting
platform.VCenters[i].Username/Password from vsphereUsername and vspherePassword
(iterating platform.VCenters) which is duplicated; remove the redundant loop and
keep a single credential-defaulting loop for platform.VCenters (use the existing
loop that runs when the JSON platform spec is processed or consolidate into one
after the platform population logic), ensuring the code still sets
Username/Password for each entry in platform.VCenters and that legacy fields
(platform.Deprecated*) remain untouched.
- Around line 833-838: The existing fallback that sets platform.APIVIPs =
[]string{o.VSphereAPIVIP} and platform.IngressVIPs =
[]string{o.VSphereIngressVIP} can produce an insufficient single-VIP
configuration for multi-VIP (zonal) setups; update the code where
platform.APIVIPs/IngressVIPs are defaulted to first legacy VIP to first check
the configured failure domain count and either (a) log a clear warning (using
the repo logger) when only one VIP is being injected but multiple failure
domains exist, or (b) fail validation by returning an error indicating the user
must supply one VIP per failure domain; reference platform.APIVIPs,
platform.IngressVIPs, o.VSphereAPIVIP and o.VSphereIngressVIP in your change and
perform the count comparison against the cluster failure domains before
accepting the single-VIP fallback.
In `@pkg/clusterresource/builder_test.go`:
- Around line 145-175: Add assertions in the test that verify the generated
ClusterDeployment/ClusterSpec Platform.VSphere.Infrastructure was populated from
the fixture: assert cd.Spec.Platform.VSphere.Infrastructure.VCenters contains an
entry with Server "test", Port 123 and Datacenters ["test"]; assert
Infrastructure.FailureDomains contains a domain with Name "test", Region "test",
Zone "test" and Topology.Datacenter "test" (optionally check other Topology
fields like ComputeCluster); and assert Infrastructure.APIVIPs and IngressVIPs
equal ["192.168.0.2"] and ["192.168.0.3"] respectively. Locate the assertions
near the existing Secret-ref checks that currently validate the
VSphereCloudBuilder.Infrastructure path (use the test's cd or cluster result
variable) and add strict equality checks for these fields.
In `@pkg/clusterresource/vsphere.go`:
- Around line 35-44: NewVSphereCloudBuilderFromSecret currently returns a
VSphereCloudBuilder with Username, Password and CACert but leaves Infrastructure
nil; change the constructor to accept an Infrastructure parameter (e.g. add
infrastructure *configv1.Infrastructure to NewVSphereCloudBuilderFromSecret’s
signature) and set the returned VSphereCloudBuilder.Infrastructure =
infrastructure (or otherwise populate Infrastructure appropriately) so callers
of GetCloudPlatform and addInstallConfigPlatform receive a fully-initialized
VSphereCloudBuilder.
In `@pkg/controller/utils/vsphereutils/vsphereutils_test.go`:
- Around line 12-84: Add a new subtest in TestConvertDeprecatedFields that
pre-populates hivevsphere.Platform.Infrastructure with a non-nil
installervsphere.Platform value and asserts ConvertDeprecatedFields does not
modify it: construct a platform with Infrastructure set (instead of only
deprecated fields), set Expected to the same installervsphere.Platform instance,
call ConvertDeprecatedFields(platform), require.NoError, and compare
platform.Infrastructure to Expected to ensure the function is a no-op when
platform.Infrastructure is already present (reference symbols:
TestConvertDeprecatedFields, ConvertDeprecatedFields,
hivevsphere.Platform.Infrastructure, installervsphere.Platform).
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go`:
- Around line 1901-1926: Add explicit expectedAllowed: false to the three test
case entries named "vsphere platform cannot be de-zonal-ified", "vsphere
platform immutable if not being zonal-ified (deprecated shape)", and "vsphere
platform immutable if not being zonal-ified (zonal shape)" in the test table in
clusterdeployment_validating_admission_hook_test.go; locate each case (they use
oldObject/newObject with validVSphereClusterDeployment() or
deprecatedVSphereClusterDeployment() and operation admissionv1beta1.Update) and
add the field expectedAllowed: false to make the intended failure expectation
explicit and improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 945ad764-0420-42da-ad48-2747a8dae741
⛔ Files ignored due to path filters (17)
apis/go.sumis excluded by!**/*.sumapis/hive/v1/vsphere/zz_generated.deepcopy.gois excluded by!**/zz_generated*apis/hive/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*apis/vendor/github.com/openshift/installer/LICENSEis excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/NOTICEis excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/pkg/types/vsphere/OWNERSis excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/pkg/types/vsphere/doc.gois excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/pkg/types/vsphere/machinepool.gois excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/pkg/types/vsphere/metadata.gois excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.gois excluded by!**/vendor/**apis/vendor/github.com/openshift/installer/pkg/types/vsphere/zz_generated.deepcopy.gois excluded by!**/vendor/**,!**/zz_generated*apis/vendor/modules.txtis excluded by!**/vendor/**vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hive/apis/hive/v1/vsphere/machinepools.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hive/apis/hive/v1/vsphere/platform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*
📒 Files selected for processing (33)
apis/go.modapis/hive/v1/clusterdeprovision_types.goapis/hive/v1/vsphere/machinepools.goapis/hive/v1/vsphere/platform.goconfig/crds/hive.openshift.io_clusterdeployments.yamlconfig/crds/hive.openshift.io_clusterdeprovisions.yamlconfig/crds/hive.openshift.io_clusterpools.yamlconfig/crds/hive.openshift.io_machinepools.yamlcontrib/pkg/createcluster/create.gocontrib/pkg/deprovision/vsphere.godocs/hiveutil.mddocs/using-hive.mdgo.modhack/app-sre/saas-template.yamlhack/e2e-common.shpkg/clusterresource/builder_test.gopkg/clusterresource/vsphere.gopkg/controller/clusterdeployment/clusterdeployment_controller.gopkg/controller/clusterdeployment/installconfigvalidation.gopkg/controller/clusterdeployment/installconfigvalidation_test.gopkg/controller/clusterpool/clusterpool_controller.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/controller/utils/credentials.gopkg/controller/utils/vsphereutils/vsphere.gopkg/controller/utils/vsphereutils/vsphereutils_test.gopkg/creds/vsphere/vsphere.gopkg/install/generate.gopkg/installmanager/installmanager.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.gopkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.gopkg/validating-webhooks/hive/v1/machinepool_validating_admission_hook_test.go
pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
Show resolved
Hide resolved
pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go
Outdated
Show resolved
Hide resolved
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
aa9dcc3 to
8ab7797
Compare
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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. |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/machinepool/machinepool_controller_test.go (1)
1415-1418:⚠️ Potential issue | 🔴 CriticalInverted assertion logic causes tests to return early on success.
The condition
if assert.NoError(t, err, ...)returnstruewhen there is NO error, causing the test toreturnand skip all subsequent validations when reconciliation succeeds. This should beif !assert.NoError(...)to return early only on error.🐛 Proposed fix
- if assert.NoError(t, err, "unexpected error: %v", err) { + if !assert.NoError(t, err, "unexpected error: %v", err) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/machinepool/machinepool_controller_test.go` around lines 1415 - 1418, The test contains inverted assertion logic: the block using assert.NoError(t, err, "unexpected error: %v", err) returns when there is no error, skipping subsequent checks; change the conditional to return only on failure by negating the assertion (i.e., use !assert.NoError(...)) so the test proceeds when reconciliation succeeds and exits early only when err is non-nil, updating the conditional that currently wraps the return in the test function in pkg/controller/machinepool/machinepool_controller_test.go.config/crds/hive.openshift.io_clusterdeprovisions.yaml (1)
322-368:⚠️ Potential issue | 🟠 Major
ClusterDeprovisionvSphere schema is still flat; addinfrastructure-based path.This update deprecates
vCenterand addsvCenters, but it still does not expose thespec.platform.vsphere.infrastructureobject needed for the zonal/multi-vCenter migration path.As per coding guidelines, "Replace single-vCenter fields with a multi-vCenter approach via the Infrastructure object. Use Infrastructure.VCenters (list of vCenter connections) for credentials and topology across zones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crds/hive.openshift.io_clusterdeprovisions.yaml` around lines 322 - 368, The vsphere block is still flat and must expose an infrastructure-based path for multi-vCenter/zonal migration; add an infrastructure object under the existing vsphere schema that mirrors the per-VCenter connection structure and uses a VCenters array (Infrastructure.VCenters) containing items with name, certificatesSecretRef and credentialsSecretRef (and any zone/topology fields required by the infra model), keep vCenter deprecated and vCenters for backward compatibility but ensure code consumers prefer spec.platform.vsphere.infrastructure.VCenters for credentials/topology; update required/description text accordingly and reuse the existing certificatesSecretRef and credentialsSecretRef property shapes to avoid duplication.
♻️ Duplicate comments (8)
contrib/pkg/deprovision/vsphere.go (1)
72-74:⚠️ Potential issue | 🟠 MajorAdd validation for empty vCenters after credential configuration.
If using old-style credentials without
--vsphere-vcenters, theVCentersslice remains empty afterConfigureCreds(). This causes a cryptic "dial tcp: missing address" error later invsphere.New()instead of a clear error message.🐛 Proposed fix to add validation
vspherecreds.ConfigureCreds(client, o.metadata) + if o.metadata.VSphere == nil || len(o.metadata.VSphere.VCenters) == 0 { + return errors.New("must provide at least one vCenter via --vsphere-vcenters or the credentials secret") + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/deprovision/vsphere.go` around lines 72 - 74, After calling vspherecreds.ConfigureCreds(client, o.metadata) ensure you validate that the resulting vCenter list is non-empty and return a clear error if it is empty; specifically check the VCenters slice (from the configured metadata/credential object used by vsphere.New) and, when empty, return an explicit error like "no vCenter endpoints configured; provide --vsphere-vcenters or valid credentials" instead of proceeding to vsphere.New(), so callers get a helpful message rather than a "dial tcp: missing address" failure.pkg/controller/clusterdeployment/installconfigvalidation.go (1)
67-72:⚠️ Potential issue | 🟠 MajorInconsistent credential validation between deprecated and new paths.
The deprecated path on line 67 uses
||(either username or password is enough), while the VCenters path on lines 69-71 correctly requires both username AND password with&&. This allows incomplete deprecated credentials to pass validation.🐛 Proposed fix for consistent validation
- hasCreds := ic.Platform.VSphere.DeprecatedUsername != "" || ic.Platform.VSphere.DeprecatedPassword != "" + hasCreds := ic.Platform.VSphere.DeprecatedUsername != "" && ic.Platform.VSphere.DeprecatedPassword != "" for _, vcenter := range ic.Platform.VSphere.VCenters { if vcenter.Username != "" && vcenter.Password != "" { hasCreds = true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/clusterdeployment/installconfigvalidation.go` around lines 67 - 72, The credential check is inconsistent: ic.Platform.VSphere.DeprecatedUsername != "" || ic.Platform.VSphere.DeprecatedPassword != "" allows incomplete deprecated creds; change it to require both (use &&) so hasCreds is set only when DeprecatedUsername and DeprecatedPassword are both non-empty, matching the VCenters loop that checks vcenter.Username != "" && vcenter.Password != "" in the same function (installconfigvalidation.go).pkg/creds/vsphere/vsphere.go (1)
34-43:⚠️ Potential issue | 🔴 CriticalPotential nil pointer dereference in old-style credentials path.
The old-style credentials branch accesses
metadata.VSphere.VCenters(line 39) without checking ifmetadata.VSphereis nil. The new-style path (lines 28-30) defensively handles this case, but this branch lacks the same protection.🐛 Proposed fix to add nil check
} else { // Old style creds Secret with flat username/password: copy the creds across all vcenters username := strings.TrimSpace(string(credsSecret.Data[constants.UsernameSecretKey])) password := strings.TrimSpace(string(credsSecret.Data[constants.PasswordSecretKey])) + if metadata.VSphere == nil { + log.Fatal("old-style credentials require metadata.VSphere to be pre-populated with VCenter names") + } // Caller pre-populated this with just the VCenter names for i := range metadata.VSphere.VCenters {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/creds/vsphere/vsphere.go` around lines 34 - 43, The old-style creds branch in the function handling metadata dereferences metadata.VSphere.VCenters without checking metadata.VSphere; add a nil check for metadata.VSphere before using VCenters (the same defensive approach used in the new-style branch) and either initialize/return an error or skip copying credentials if metadata.VSphere is nil so you avoid a nil pointer dereference when populating metadata.VSphere.VCenters with username/password.pkg/controller/utils/credentials.go (1)
107-116:⚠️ Potential issue | 🟠 MajorValidation succeeds with empty vcenters slice in legacy path.
If
cd.Spec.Platform.VSphere.Infrastructure.VCentersis empty in the legacy path, the loop produces zero entries, the subsequent validation loop is skipped entirely, and the function returnstrue, nilwithout validating any credentials.🐛 Proposed fix to validate non-empty vcenters
} + if len(vcenters) == 0 { + return false, errors.New("no vCenter credentials resolved from credentials Secret") + } for i, vcenter := range vcenters { if vcenter.VCenter == "" { return false, fmt.Errorf("missing or invalid vCenter in index %d of vcenter list", i)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/utils/credentials.go` around lines 107 - 116, The validation loop currently skips when the vcenters slice is empty, causing legacy path to incorrectly succeed; add an explicit check after vcenters is populated to return an error if len(vcenters) == 0 (e.g., return false, fmt.Errorf("no vCenters provided in Platform.VSphere.Infrastructure.VCenters")) before the for i, vcenter range so validateVSphereCredentials is invoked for at least one entry; update the same function that declares vcenters (referencing the vcenters variable, the for loop using validateVSphereCredentials, and existing logger/rootCAFiles parameters) to perform this non-empty check and return a failing validation when empty.pkg/controller/clusterpool/clusterpool_controller.go (1)
322-333:⚠️ Potential issue | 🟠 MajorThis migration changes the pool-version hash for every legacy vSphere pool.
calculatePoolVersion()at line 556 hashesclp.Spec.Platform, so addingInfrastructurehere changes the hash even when the effective config is unchanged. Existing pool members keep their old pool-version annotation and will be treated as stale on the next reconcile.Consider either:
- Normalizing vSphere fields before hashing (apply
ConvertDeprecatedFieldson a copy before computing the hash), or- Rewriting member annotations as part of the migration
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/clusterpool/clusterpool_controller.go` around lines 322 - 333, The migration adds an Infrastructure field to vSphere pools which alters the hash computed by calculatePoolVersion() because it hashes clp.Spec.Platform; to fix, normalize legacy vSphere fields before hashing by applying vsphereutils.ConvertDeprecatedFields on a DeepCopy of clp.Spec.Platform (or call ConvertDeprecatedFields on a copy of clp before any call to calculatePoolVersion) so the computed pool-version remains stable for unchanged effective config; alternatively, if you prefer migration-on-write, after performing the in-place update in the shown block also iterate pool members and rewrite their pool-version annotations to the new hash (use the same calculatePoolVersion result) so members aren’t treated as stale—ensure you reference ConvertDeprecatedFields, calculatePoolVersion, and the reconciliation update path when implementing the fix.pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go (2)
671-678:⚠️ Potential issue | 🟠 MajorDon’t overwrite the entire old vSphere spec to permit zonal migration.
Replacing
oldObject.Spec.Platform.VSpherewith the incoming struct suppresses immutability checks for unrelated vSphere fields (credentials/certs/other immutable fields) in the same update.💡 Suggested fix
if oldObject.Spec.Platform.VSphere != nil && cd.Spec.Platform.VSphere != nil { // HIVE-2391: Moving from a non-zonal to a zonal shape is permitted. // NOTE: Existing deprecated fields may be left populated, but will be ignored. if oldObject.Spec.Platform.VSphere.Infrastructure == nil && cd.Spec.Platform.VSphere.Infrastructure != nil { contextLogger.Debug("Allowing vsphere zonal conversion") - // copy over the value to spoof the immutability checker - oldObject.Spec.Platform.VSphere = cd.Spec.Platform.VSphere + normalizedOld := oldObject.Spec.Platform.VSphere.DeepCopy() + normalizedOld.Infrastructure = cd.Spec.Platform.VSphere.Infrastructure + oldObject.Spec.Platform.VSphere = normalizedOld } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go` around lines 671 - 678, The current code replaces oldObject.Spec.Platform.VSphere with cd.Spec.Platform.VSphere to permit zonal conversion, which silently mutates unrelated immutable fields; instead, only copy the Infrastructure subfield from cd.Spec.Platform.VSphere into oldObject.Spec.Platform.VSphere (i.e., set oldObject.Spec.Platform.VSphere.Infrastructure = cd.Spec.Platform.VSphere.Infrastructure) while leaving all other fields (credentials/certs/etc.) untouched so immutability checks still run for those fields; update the block around oldObject.Spec.Platform.VSphere / cd.Spec.Platform.VSphere and the conditional that detects nil -> non-nil Infrastructure accordingly.
535-540:⚠️ Potential issue | 🟠 MajorLegacy/new vSphere shape validation is still too permissive (and reports an invalid field path).
This allows incomplete legacy specs through when
Infrastructureis absent/empty (e.g., missing legacy topology fields), deferring failure to later reconcile. Also, the reported path...vsphere.vSphere.vcenters[0]is not a valid spec path.💡 Suggested fix
- if vsphere.DeprecatedVCenter == "" && (vsphere.Infrastructure == nil || len(vsphere.Infrastructure.VCenters) == 0) { - allErrs = append(allErrs, field.Required(vspherePath.Child("vSphere").Child("vcenters").Index(0), "must specify at least one vSphere vCenter")) - } + if vsphere.Infrastructure == nil || len(vsphere.Infrastructure.VCenters) == 0 { + // legacy shape validation + if vsphere.DeprecatedVCenter == "" { + allErrs = append(allErrs, field.Required(vspherePath.Child("vCenter"), "must specify vCenter")) + } + if vsphere.DeprecatedDatacenter == "" { + allErrs = append(allErrs, field.Required(vspherePath.Child("datacenter"), "must specify datacenter")) + } + if vsphere.DeprecatedDefaultDatastore == "" { + allErrs = append(allErrs, field.Required(vspherePath.Child("defaultDatastore"), "must specify default datastore")) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go` around lines 535 - 540, The current check lets incomplete legacy vSphere specs slip through and reports an invalid field path; update the validation so it rejects when neither a modern Infrastructure.VCenters entry exists nor the required legacy topology fields are present, and fix the field path used in the error: use vspherePath.Child("vcenters").Index(0) (not vspherePath.Child("vSphere").Child("vcenters").Index(0)); additionally, when vsphere.DeprecatedVCenter is non-empty add explicit checks for the required legacy topology fields (the legacy topology field names used elsewhere in this file) and append field.Required errors using the corrected path and the existing field.Required/field.Path helpers.pkg/controller/machinepool/vsphereactuator.go (1)
54-65:⚠️ Potential issue | 🟠 MajorDeep-copy
Infrastructurebefore applying MachinePool overrides.
ic.VSpherecurrently points at the same object ascd.Spec.Platform.VSphere.Infrastructure. Updating failure-domainResourcePool/TagIDshere mutates shared ClusterDeployment state and can bleed overrides across pools.💡 Suggested fix
- ic := &installertypes.InstallConfig{ + infra := cd.Spec.Platform.VSphere.Infrastructure.DeepCopy() + ic := &installertypes.InstallConfig{ Platform: installertypes.Platform{ - VSphere: cd.Spec.Platform.VSphere.Infrastructure, + VSphere: infra, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/machinepool/vsphereactuator.go` around lines 54 - 65, The code mutates shared ClusterDeployment state by assigning ic.VSphere to cd.Spec.Platform.VSphere.Infrastructure and then modifying failureDomain.Topology; instead deep-copy the Infrastructure object before applying MachinePool overrides so changes don’t bleed to cd. Replace the direct assignment (where ic.VSphere is set from cd.Spec.Platform.VSphere.Infrastructure) with a deep copy of that structure (use the type’s DeepCopy/DeepCopyInto or a manual copy) and then apply overrides to ic.VSphere.FailureDomains/failureDomain.Topology when handling pool.Spec.Platform.VSphere.ResourcePool and TagIDs.
🧹 Nitpick comments (3)
pkg/controller/clusterdeployment/installconfigvalidation_test.go (1)
276-285: Add one case for canonicalvsphere.infrastructure.vcentersinstall-config shape.This updated test validates missing credentials with
Infrastructure: &installervsphere.Platform{}, but it still feedstestvSphereIC(legacyvCenterkey). Adding a case that uses the newinfrastructure.vcentersYAML shape would better guard the migration path and prevent regressions in the zonal/multi-vCenter flow.Proposed test extension
+const testvSphereInfrastructureIC = ` +apiVersion: v1 +baseDomain: example.com +compute: +- name: worker +controlPlane: + name: master +metadata: + name: testcluster-vsphere +platform: + vsphere: + infrastructure: + vcenters: + - server: 10.0.0.1 +pullSecret: "" +` ... + { + name: "test install config no vSphere credentials (infrastructure shape)", + cd: cdBuilder.Build( + func(cd *hivev1.ClusterDeployment) { + cd.Spec.Platform.VSphere = &hivev1vpshere.Platform{ + Infrastructure: &installervsphere.Platform{}, + } + }, + ), + ic: testvSphereInfrastructureIC, + expectedError: missingvSphereCredentialsErr, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/clusterdeployment/installconfigvalidation_test.go` around lines 276 - 285, Add a new test case alongside the existing "test install config no vSphere credentials" that exercises the canonical infrastructure.vcenters install-config shape: build a ClusterDeployment with cdBuilder.Build setting cd.Spec.Platform.VSphere = &hivev1vpshere.Platform{Infrastructure: &installervsphere.Platform{}} (same as current), but supply an install-config that uses the new "infrastructure.vcenters" schema instead of testvSphereIC (i.e., a test install config variable representing the vcenters list with no credentials). Expect the same missingvSphereCredentialsErr; reference the existing test case and variables (cdBuilder.Build, installervsphere.Platform, testvSphereIC, missingvSphereCredentialsErr) when adding the new case to ensure the migration path for zonal/multi-vCenter flows is covered.vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go (1)
135-141: Optional fields missingomitemptyin JSON tags.Both
DeprecatedVCenterandVCentersare marked+optionalbut their JSON tags lackomitempty. This means empty values will be serialized (empty string and empty array), which may cause issues with consumers expecting absent fields for unset optional values.♻️ Proposed fix to add omitempty
// DeprecatedVCenter is the vSphere vCenter hostname. // Deprecated: use VCenters instead. // +optional - DeprecatedVCenter string `json:"vCenter"` + DeprecatedVCenter string `json:"vCenter,omitempty"` // VCenters are potentially multiple vCenter hostnames. Prefer this field over VCenter. // +optional - VCenters []string `json:"vCenters"` + VCenters []string `json:"vCenters,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go` around lines 135 - 141, The JSON tags for the optional fields DeprecatedVCenter and VCenters currently omit `omitempty`, causing empty values to be serialized; update the struct field tags for DeprecatedVCenter (`DeprecatedVCenter string`) and VCenters (`VCenters []string`) to include `omitempty` in their `json:"..."` tags so that unset/empty values are omitted during serialization (e.g., change `json:"vCenter"` and `json:"vCenters"` to include `omitempty`).pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go (1)
1901-1926: Optional clarity refactor: set negative expectations explicitly.These cases currently depend on zero-value
expectedAllowed == false. Making it explicit improves readability and future-proofing.Proposed test clarity tweak
{ name: "vsphere platform cannot be de-zonal-ified", oldObject: validVSphereClusterDeployment(), newObject: deprecatedVSphereClusterDeployment(), operation: admissionv1beta1.Update, + expectedAllowed: false, }, { name: "vsphere platform immutable if not being zonal-ified (deprecated shape)", oldObject: func() *hivev1.ClusterDeployment { cd := deprecatedVSphereClusterDeployment() cd.Spec.Platform.VSphere.CredentialsSecretRef.Name = "changed" return cd }(), newObject: deprecatedVSphereClusterDeployment(), operation: admissionv1beta1.Update, + expectedAllowed: false, }, { name: "vsphere platform immutable if not being zonal-ified (zonal shape)", oldObject: func() *hivev1.ClusterDeployment { cd := validVSphereClusterDeployment() cd.Spec.Platform.VSphere.Infrastructure.ClusterOSImage = "changed" return cd }(), newObject: validVSphereClusterDeployment(), operation: admissionv1beta1.Update, + expectedAllowed: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go` around lines 1901 - 1926, Add an explicit negative expectation to the three failing test cases so they don't rely on the zero-value; for the test entries with name "vsphere platform cannot be de-zonal-ified", "vsphere platform immutable if not being zonal-ified (deprecated shape)", and "vsphere platform immutable if not being zonal-ified (zonal shape)" in clusterdeployment_validating_admission_hook_test.go, add expectedAllowed: false (and optionally expectedDeniedReason if your test harness asserts reasons) to each test case struct so the intent is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/vendor/github.com/openshift/installer/pkg/types/vsphere/metadata.go`:
- Around line 13-14: The VCenters field in the Metadata struct is missing a JSON
tag and currently serializes as "VCenters"; update the Metadata struct's
VCenters field (VCenters []VCenters) to include the same tag used in platform.go
(json:"vcenters,omitempty") so it serializes/deserializes consistently when
Metadata is embedded in ClusterMetadata and matches the platform types.
In `@apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go`:
- Around line 275-280: The TagIDs field in platform.go documents a maximum of 10
tags but lacks a validation marker; add a kubebuilder validation marker (e.g.,
+kubebuilder:validation:MaxItems=10) immediately above the TagIDs []string
`json:"tagIDs,omitempty"` declaration to enforce the limit at schema/admission
time and keep the existing +optional and example markers intact so the field
(TagIDs) is validated to at most 10 entries.
In `@apis/vendor/modules.txt`:
- Around line 28-30: Replace the pre-GA nightly vendored module version of
github.com/openshift/installer (currently referenced as
v1.4.21-pre2.0.20260112230456-1c2444827f23) with a stable or later corrected
release to avoid known issues; update the modules.txt entry for
github.com/openshift/installer to either the fixed nightly (e.g.
4.21.0-0.nightly-2026-02-10-112229) or the OpenShift 4.21 GA installer release,
and run your dependency vendor/update command to regenerate lockfiles so the new
version is applied everywhere.
In `@contrib/pkg/createcluster/create.go`:
- Around line 833-838: The code currently assigns platform.APIVIPs =
[]string{o.VSphereAPIVIP} and platform.IngressVIPs =
[]string{o.VSphereIngressVIP} even when those flag strings are empty, producing
[]string{""}; change the logic in the block handling
platform.APIVIPs/platform.IngressVIPs to only set the slice when the
corresponding option string is non-empty (check o.VSphereAPIVIP != "" before
assigning to platform.APIVIPs and o.VSphereIngressVIP != "" before assigning to
platform.IngressVIPs), leaving the slices nil/empty otherwise so you don't write
empty-string entries.
- Around line 814-822: The legacy-flag branch only sets deprecated fields on
platform (DeprecatedUsername, DeprecatedPassword, DeprecatedNetwork,
DeprecatedDatacenter, DeprecatedDefaultDatastore, DeprecatedVCenter,
DeprecatedFolder, DeprecatedCluster) but leaves platform.VCenters empty, causing
webhook validation failures; fix by constructing a vSphere VCentre entry from
those legacy values and assigning it to platform.VCenters (e.g., create a
VSphereVCenter/VCenter struct populated from
vsphereUsername/vspherePassword/vSphereNetwork/vSphereDatacenter/vSphereDatastore/vSphereVCenter
and o.VSphereFolder and append/set platform.VCenters = []{thatEntry}) so the
resulting Platform has a webhook-valid vSphere shape while still keeping the
deprecated fields.
In `@docs/hiveutil.md`:
- Line 110: Remove the internal trailing space inside the markdown code span for
the environment variable name `GOVC_USERNAME` so it reads `GOVC_USERNAME` (no
trailing space); update the table cell containing the code span (the entry
currently shown as `GOVC_USERNAME `) to eliminate the extra space to satisfy
markdownlint MD038.
- Around line 108-116: Update the required-flags table to clarify that
--vsphere-api-vip and --vsphere-ingress-vip are optional when VIPs are provided
inside VSPHERE_INSTALLER_PLATFORM_SPEC_JSON (or passed via
--vsphere-platform-spec-json); specifically change the table entries for `N/A |
--vsphere-api-vip` and `N/A | --vsphere-ingress-vip` to note "Optional — used if
not present in VSPHERE_INSTALLER_PLATFORM_SPEC_JSON" (or alternatively add a
short note under the table explaining that VIPs can be supplied either via the
two CLI flags (--vsphere-api-vip, --vsphere-ingress-vip) or embedded inside the
JSON provided to VSPHERE_INSTALLER_PLATFORM_SPEC_JSON /
--vsphere-platform-spec-json).
In `@docs/using-hive.md`:
- Around line 229-230: Update the prose that describes the Secret payload so its
key names match the example payload: replace the phrase listing `server,
username, password` with the same keys used in the example (`vCenter, username,
password`) and ensure the Secret key name `vcenters` and the example structure
are described consistently (also apply the same edit to the other occurrences
covering the example block referenced around the subsequent lines).
- Around line 593-600: Update the install-config vcenters example to use the
correct key name: replace the YAML key "username" with "user" in the vcenters
block so the example matches the vSphere platform schema used by install-config;
locate the vcenters example that currently contains "username: vsphereuser1" and
"username: vsphereuser2" and change them to "user" to avoid validation failures.
In `@pkg/controller/clusterdeployment/clusterdeployment_controller.go`:
- Around line 450-458: The conversion guard only triggers when
cd.Spec.Platform.VSphere.Infrastructure == nil but should also run for an
existing but empty legacy-shaped Infrastructure (e.g., no VCenters). Update the
condition in the ClusterDeployment reconcile block to check the Infrastructure
pointer and its VCenters length before skipping conversion (for example assign
infra := cd.Spec.Platform.VSphere.Infrastructure and use infra == nil ||
len(infra.VCenters) == 0), then call
vsphereutils.ConvertDeprecatedFields(cd.Spec.Platform.VSphere) and r.Update(ctx,
cd) as before; keep existing cd.DeepCopy(), cdLog.WithError(...), and
reconcile.Result return paths unchanged.
In `@pkg/controller/machinepool/machinepool_controller.go`:
- Around line 1443-1451: The current conditional that allows zero-sized
autoscaling (checking cd.Spec.Platform.VSphere) must be gated by the cluster
OpenShift version (require OCP >= 4.8 for vSphere); update the conditional in
the machinepool controller (the block that currently checks
cd.Spec.Platform.AWS/ Azure/ GCP/ OpenStack/ VSphere) so that VSphere is only
treated as allowing minReplicas=0 when the cluster version is >= 4.8. Locate the
code that reads cd (cd.Spec.Platform.*) and the validation that permits
zero-sized autoscaling (e.g., the if-block shown) and add a version check
against the cluster/desired version (use the project’s existing helper or
ClusterVersion info used elsewhere in this controller) so VSphere follows the
same version-gating pattern as OpenStack (4.7) and AWS/Azure/GCP (4.5).
In `@pkg/controller/utils/credentials.go`:
- Around line 96-106: The code iterates
cd.Spec.Platform.VSphere.Infrastructure.VCenters without guarding against a nil
Infrastructure pointer, which can panic for legacy ClusterDeployment shapes; add
a nil check for cd.Spec.Platform.VSphere.Infrastructure before accessing
Infrastructure.VCenters (in the same block where you build vcenters with
vsphere.VCenters) and if Infrastructure is nil either skip the loop or read the
legacy fields (whichever matches existing backwards-compat logic), logging a
clear message; ensure the change references the same symbols:
cd.Spec.Platform.VSphere.Infrastructure, Infrastructure.VCenters, and the
vcenters slice population using vsphere.VCenters.
In `@pkg/controller/utils/vsphereutils/vsphere.go`:
- Around line 10-33: Add unit tests for ConvertDeprecatedFields that cover empty
and partially-populated deprecated fields to prevent creation of
semantically-invalid Infrastructure; specifically, write tests that call
ConvertDeprecatedFields with a hivevsphere.Platform where
DeprecatedVCenter/Datacenter/DefaultDatastore/Folder/Cluster/Network are empty
or partially filled and assert that either an error is returned or the resulting
platform.Infrastructure.VSphere.VCenters[0].Server and required topology fields
are non-empty and valid. If tests reveal invalid outputs, update
ConvertDeprecatedFields to validate the converted
dummyInstallConfig.Platform.VSphere (e.g., ensure VCenters length>0 and
VCenters[0].Server != "" and required topology/datastore fields are present) and
return an error instead of assigning platform.Infrastructure, so the webhook
logic in clusterdeployment_validating_admission_hook.go continues to catch
invalid configs. Ensure tests reference the ConvertDeprecatedFields function and
inspect platform.Infrastructure.VSphere structures to verify correct behavior.
In `@pkg/install/generate.go`:
- Around line 892-903: Add a guard that fails fast when neither
req.Spec.Platform.VSphere.VCenters nor
req.Spec.Platform.VSphere.DeprecatedVCenter is provided: check if
len(req.Spec.Platform.VSphere.VCenters) == 0 &&
req.Spec.Platform.VSphere.DeprecatedVCenter == "" and return an error (or
otherwise surface a validation failure) instead of constructing args with an
empty joinedVCenters; update the code path that builds joinedVCenters and the
args slice (the variables joinedVCenters and args in generate.go) to perform
this validation and return the error before creating "--vsphere-vcenters" with
an empty value.
---
Outside diff comments:
In `@config/crds/hive.openshift.io_clusterdeprovisions.yaml`:
- Around line 322-368: The vsphere block is still flat and must expose an
infrastructure-based path for multi-vCenter/zonal migration; add an
infrastructure object under the existing vsphere schema that mirrors the
per-VCenter connection structure and uses a VCenters array
(Infrastructure.VCenters) containing items with name, certificatesSecretRef and
credentialsSecretRef (and any zone/topology fields required by the infra model),
keep vCenter deprecated and vCenters for backward compatibility but ensure code
consumers prefer spec.platform.vsphere.infrastructure.VCenters for
credentials/topology; update required/description text accordingly and reuse the
existing certificatesSecretRef and credentialsSecretRef property shapes to avoid
duplication.
In `@pkg/controller/machinepool/machinepool_controller_test.go`:
- Around line 1415-1418: The test contains inverted assertion logic: the block
using assert.NoError(t, err, "unexpected error: %v", err) returns when there is
no error, skipping subsequent checks; change the conditional to return only on
failure by negating the assertion (i.e., use !assert.NoError(...)) so the test
proceeds when reconciliation succeeds and exits early only when err is non-nil,
updating the conditional that currently wraps the return in the test function in
pkg/controller/machinepool/machinepool_controller_test.go.
---
Duplicate comments:
In `@contrib/pkg/deprovision/vsphere.go`:
- Around line 72-74: After calling vspherecreds.ConfigureCreds(client,
o.metadata) ensure you validate that the resulting vCenter list is non-empty and
return a clear error if it is empty; specifically check the VCenters slice (from
the configured metadata/credential object used by vsphere.New) and, when empty,
return an explicit error like "no vCenter endpoints configured; provide
--vsphere-vcenters or valid credentials" instead of proceeding to vsphere.New(),
so callers get a helpful message rather than a "dial tcp: missing address"
failure.
In `@pkg/controller/clusterdeployment/installconfigvalidation.go`:
- Around line 67-72: The credential check is inconsistent:
ic.Platform.VSphere.DeprecatedUsername != "" ||
ic.Platform.VSphere.DeprecatedPassword != "" allows incomplete deprecated creds;
change it to require both (use &&) so hasCreds is set only when
DeprecatedUsername and DeprecatedPassword are both non-empty, matching the
VCenters loop that checks vcenter.Username != "" && vcenter.Password != "" in
the same function (installconfigvalidation.go).
In `@pkg/controller/clusterpool/clusterpool_controller.go`:
- Around line 322-333: The migration adds an Infrastructure field to vSphere
pools which alters the hash computed by calculatePoolVersion() because it hashes
clp.Spec.Platform; to fix, normalize legacy vSphere fields before hashing by
applying vsphereutils.ConvertDeprecatedFields on a DeepCopy of clp.Spec.Platform
(or call ConvertDeprecatedFields on a copy of clp before any call to
calculatePoolVersion) so the computed pool-version remains stable for unchanged
effective config; alternatively, if you prefer migration-on-write, after
performing the in-place update in the shown block also iterate pool members and
rewrite their pool-version annotations to the new hash (use the same
calculatePoolVersion result) so members aren’t treated as stale—ensure you
reference ConvertDeprecatedFields, calculatePoolVersion, and the reconciliation
update path when implementing the fix.
In `@pkg/controller/machinepool/vsphereactuator.go`:
- Around line 54-65: The code mutates shared ClusterDeployment state by
assigning ic.VSphere to cd.Spec.Platform.VSphere.Infrastructure and then
modifying failureDomain.Topology; instead deep-copy the Infrastructure object
before applying MachinePool overrides so changes don’t bleed to cd. Replace the
direct assignment (where ic.VSphere is set from
cd.Spec.Platform.VSphere.Infrastructure) with a deep copy of that structure (use
the type’s DeepCopy/DeepCopyInto or a manual copy) and then apply overrides to
ic.VSphere.FailureDomains/failureDomain.Topology when handling
pool.Spec.Platform.VSphere.ResourcePool and TagIDs.
In `@pkg/controller/utils/credentials.go`:
- Around line 107-116: The validation loop currently skips when the vcenters
slice is empty, causing legacy path to incorrectly succeed; add an explicit
check after vcenters is populated to return an error if len(vcenters) == 0
(e.g., return false, fmt.Errorf("no vCenters provided in
Platform.VSphere.Infrastructure.VCenters")) before the for i, vcenter range so
validateVSphereCredentials is invoked for at least one entry; update the same
function that declares vcenters (referencing the vcenters variable, the for loop
using validateVSphereCredentials, and existing logger/rootCAFiles parameters) to
perform this non-empty check and return a failing validation when empty.
In `@pkg/creds/vsphere/vsphere.go`:
- Around line 34-43: The old-style creds branch in the function handling
metadata dereferences metadata.VSphere.VCenters without checking
metadata.VSphere; add a nil check for metadata.VSphere before using VCenters
(the same defensive approach used in the new-style branch) and either
initialize/return an error or skip copying credentials if metadata.VSphere is
nil so you avoid a nil pointer dereference when populating
metadata.VSphere.VCenters with username/password.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go`:
- Around line 671-678: The current code replaces oldObject.Spec.Platform.VSphere
with cd.Spec.Platform.VSphere to permit zonal conversion, which silently mutates
unrelated immutable fields; instead, only copy the Infrastructure subfield from
cd.Spec.Platform.VSphere into oldObject.Spec.Platform.VSphere (i.e., set
oldObject.Spec.Platform.VSphere.Infrastructure =
cd.Spec.Platform.VSphere.Infrastructure) while leaving all other fields
(credentials/certs/etc.) untouched so immutability checks still run for those
fields; update the block around oldObject.Spec.Platform.VSphere /
cd.Spec.Platform.VSphere and the conditional that detects nil -> non-nil
Infrastructure accordingly.
- Around line 535-540: The current check lets incomplete legacy vSphere specs
slip through and reports an invalid field path; update the validation so it
rejects when neither a modern Infrastructure.VCenters entry exists nor the
required legacy topology fields are present, and fix the field path used in the
error: use vspherePath.Child("vcenters").Index(0) (not
vspherePath.Child("vSphere").Child("vcenters").Index(0)); additionally, when
vsphere.DeprecatedVCenter is non-empty add explicit checks for the required
legacy topology fields (the legacy topology field names used elsewhere in this
file) and append field.Required errors using the corrected path and the existing
field.Required/field.Path helpers.
---
Nitpick comments:
In `@pkg/controller/clusterdeployment/installconfigvalidation_test.go`:
- Around line 276-285: Add a new test case alongside the existing "test install
config no vSphere credentials" that exercises the canonical
infrastructure.vcenters install-config shape: build a ClusterDeployment with
cdBuilder.Build setting cd.Spec.Platform.VSphere =
&hivev1vpshere.Platform{Infrastructure: &installervsphere.Platform{}} (same as
current), but supply an install-config that uses the new
"infrastructure.vcenters" schema instead of testvSphereIC (i.e., a test install
config variable representing the vcenters list with no credentials). Expect the
same missingvSphereCredentialsErr; reference the existing test case and
variables (cdBuilder.Build, installervsphere.Platform, testvSphereIC,
missingvSphereCredentialsErr) when adding the new case to ensure the migration
path for zonal/multi-vCenter flows is covered.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go`:
- Around line 1901-1926: Add an explicit negative expectation to the three
failing test cases so they don't rely on the zero-value; for the test entries
with name "vsphere platform cannot be de-zonal-ified", "vsphere platform
immutable if not being zonal-ified (deprecated shape)", and "vsphere platform
immutable if not being zonal-ified (zonal shape)" in
clusterdeployment_validating_admission_hook_test.go, add expectedAllowed: false
(and optionally expectedDeniedReason if your test harness asserts reasons) to
each test case struct so the intent is explicit.
In `@vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go`:
- Around line 135-141: The JSON tags for the optional fields DeprecatedVCenter
and VCenters currently omit `omitempty`, causing empty values to be serialized;
update the struct field tags for DeprecatedVCenter (`DeprecatedVCenter string`)
and VCenters (`VCenters []string`) to include `omitempty` in their `json:"..."`
tags so that unset/empty values are omitted during serialization (e.g., change
`json:"vCenter"` and `json:"vCenters"` to include `omitempty`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1858f9f0-388a-4ecf-bc3a-23f249f35e59
⛔ Files ignored due to path filters (1)
apis/go.sumis excluded by!**/*.sum
📒 Files selected for processing (51)
apis/go.modapis/hive/v1/clusterdeprovision_types.goapis/hive/v1/vsphere/machinepools.goapis/hive/v1/vsphere/platform.goapis/hive/v1/vsphere/zz_generated.deepcopy.goapis/hive/v1/zz_generated.deepcopy.goapis/vendor/github.com/openshift/installer/LICENSEapis/vendor/github.com/openshift/installer/NOTICEapis/vendor/github.com/openshift/installer/pkg/types/vsphere/OWNERSapis/vendor/github.com/openshift/installer/pkg/types/vsphere/doc.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/machinepool.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/metadata.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/zz_generated.deepcopy.goapis/vendor/modules.txtconfig/crds/hive.openshift.io_clusterdeployments.yamlconfig/crds/hive.openshift.io_clusterdeprovisions.yamlconfig/crds/hive.openshift.io_clusterpools.yamlconfig/crds/hive.openshift.io_machinepools.yamlcontrib/pkg/createcluster/create.gocontrib/pkg/deprovision/vsphere.godocs/hiveutil.mddocs/using-hive.mdgo.modhack/app-sre/saas-template.yamlhack/e2e-common.shpkg/clusterresource/builder_test.gopkg/clusterresource/vsphere.gopkg/controller/clusterdeployment/clusterdeployment_controller.gopkg/controller/clusterdeployment/installconfigvalidation.gopkg/controller/clusterdeployment/installconfigvalidation_test.gopkg/controller/clusterpool/clusterpool_controller.gopkg/controller/machinepool/machinepool_controller.gopkg/controller/machinepool/machinepool_controller_test.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/controller/utils/credentials.gopkg/controller/utils/vsphereutils/vsphere.gopkg/controller/utils/vsphereutils/vsphereutils_test.gopkg/creds/vsphere/vsphere.gopkg/install/generate.gopkg/installmanager/installmanager.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.gopkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.gopkg/validating-webhooks/hive/v1/machinepool_validating_admission_hook_test.govendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/machinepools.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/platform.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.govendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go
✅ Files skipped from review due to trivial changes (3)
- apis/vendor/github.com/openshift/installer/LICENSE
- apis/vendor/github.com/openshift/installer/NOTICE
- apis/vendor/github.com/openshift/installer/pkg/types/vsphere/OWNERS
🚧 Files skipped from review as they are similar to previous changes (6)
- apis/hive/v1/vsphere/machinepools.go
- pkg/controller/utils/vsphereutils/vsphereutils_test.go
- pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go
- apis/hive/v1/clusterdeprovision_types.go
- pkg/installmanager/installmanager.go
- go.mod
apis/vendor/github.com/openshift/installer/pkg/types/vsphere/metadata.go
Show resolved
Hide resolved
apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go
Show resolved
Hide resolved
pkg/controller/clusterdeployment/clusterdeployment_controller.go
Outdated
Show resolved
Hide resolved
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
8ab7797 to
f28d497
Compare
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (12)
pkg/installmanager/installmanager.go (1)
739-752:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference on
Infrastructurefield.The code accesses
cd.Spec.Platform.VSphere.Infrastructure.VCenterswithout verifying thatInfrastructureis non-nil. While line 730 checkscd.Spec.Platform.VSphere != nil, there's no guard for theInfrastructurefield itself. If a ClusterDeployment uses deprecated fields and hasn't been migrated, this will panic.🛡️ Proposed fix to add nil check
vSpherePassword := os.Getenv(constants.VSpherePasswordEnvVar) if vSpherePassword == "" { return fmt.Errorf("no %s env var set, cannot proceed", constants.VSpherePasswordEnvVar) } + if cd.Spec.Platform.VSphere.Infrastructure == nil { + return errors.New("VSphere ClusterDeployment missing Infrastructure configuration") + } + vcenters := make([]installertypesvsphere.VCenters, 0, len(cd.Spec.Platform.VSphere.Infrastructure.VCenters)) for _, vcenter := range cd.Spec.Platform.VSphere.Infrastructure.VCenters {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/installmanager/installmanager.go` around lines 739 - 752, The code currently dereferences cd.Spec.Platform.VSphere.Infrastructure.VCenters without checking that Infrastructure is non-nil; update the block that builds vcenters (the loop creating vcenters and the creation of installertypes.ClusterMetadata with installertypesvsphere.Metadata) to first verify cd.Spec.Platform.VSphere.Infrastructure != nil and only iterate over Infrastructure.VCenters when non-nil (otherwise leave vcenters empty or use a safe fallback), ensuring no nil pointer access when cd.Spec.Platform.VSphere is present but Infrastructure is absent.docs/hiveutil.md (2)
110-110:⚠️ Potential issue | 🟡 MinorRemove the trailing space in the
GOVC_USERNAMEcode span.Line 110 has
`GOVC_USERNAME `with an internal trailing space.Proposed doc edit
-| `GOVC_USERNAME ` | Must be provided as environment variable. | +| `GOVC_USERNAME` | Must be provided as environment variable. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/hiveutil.md` at line 110, The markdown code span for GOVC_USERNAME includes an internal trailing space; edit the docs/hiveutil.md line containing the `GOVC_USERNAME ` span and remove the extra space so the span reads `GOVC_USERNAME`, ensuring the environment variable name has no trailing whitespace in the rendered docs and matches other occurrences of GOVC_USERNAME.
106-115:⚠️ Potential issue | 🟠 MajorRequired-parameters table still conflicts with the vSphere example.
Lines 114–115 list
--vsphere-api-vipand--vsphere-ingress-vipas required, but Line 119 demonstrates embedded VIPs in--vsphere-platform-spec-jsonwithout those flags. Please clarify these rows as optional when VIPs are present in the JSON.Proposed doc edit
-| N/A | `--vsphere-api-vip` | -| N/A | `--vsphere-ingress-vip` | +| N/A | `--vsphere-api-vip` (optional; used if not set in `--vsphere-platform-spec-json`) | +| N/A | `--vsphere-ingress-vip` (optional; used if not set in `--vsphere-platform-spec-json`) |Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/hiveutil.md` around lines 106 - 115, The table incorrectly marks --vsphere-api-vip and --vsphere-ingress-vip as required even though VIPs can be provided inside VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json; update the table text to indicate those two command-line parameters are optional when VIPs are embedded in the platform spec JSON (e.g., change their “Environment Variable / Command line parameter” cells to note “Optional if VIPs are included in VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json”), and ensure the surrounding paragraph clarifies that either the JSON (VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json) or the explicit flags (--vsphere-api-vip and --vsphere-ingress-vip) may be used but both are not required simultaneously.pkg/controller/utils/credentials.go (1)
84-117:⚠️ Potential issue | 🔴 CriticalFail closed on credential resolution completeness, and guard legacy nil infrastructure.
Line 99 dereferences
cd.Spec.Platform.VSphere.Infrastructurewithout a nil guard, and the resolvedvcenterslist is not verified against configuredInfrastructure.VCenters. This can panic in legacy mode and can also pass preflight with partial credentials.🐛 Proposed hardening
// Account for both possible shapes of the creds Secret vcenters := []vsphere.VCenters{} + expectedVCenters := map[string]struct{}{} + if cd.Spec.Platform.VSphere.Infrastructure != nil { + for _, configured := range cd.Spec.Platform.VSphere.Infrastructure.VCenters { + expectedVCenters[configured.Server] = struct{}{} + } + } if b, ok := secret.Data["vcenters"]; ok && len(b) > 0 { // New shape: ["vcenters"] contains a yaml blob with a slice of metadata VCenters logger.Info("validating vsphere credentials with vcenters list") if err := yaml.Unmarshal(b, &vcenters); err != nil { logger.WithError(err).Error("failed to unmarshal vcenters from credentials Secret") return false, err } if len(vcenters) < 1 { return false, errors.New("empty or invalid vcenters list in credentials Secret") } } else { // Legacy shape: flat username & password. Project those same creds out to all configured VCenters. logger.Info("validating vsphere credentials with username/password") + if cd.Spec.Platform.VSphere.Infrastructure == nil { + return false, errors.New("legacy credentials require Infrastructure.VCenters to be configured") + } + username, uok := secret.Data[constants.UsernameSecretKey] + password, pok := secret.Data[constants.PasswordSecretKey] + if !uok || !pok || len(username) == 0 || len(password) == 0 { + return false, errors.New("legacy credentials secret must include non-empty username and password") + } for _, vcenter := range cd.Spec.Platform.VSphere.Infrastructure.VCenters { vcenters = append(vcenters, vsphere.VCenters{ VCenter: vcenter.Server, - Username: string(secret.Data[constants.UsernameSecretKey]), - Password: string(secret.Data[constants.PasswordSecretKey]), + Username: string(username), + Password: string(password), }) } } + if len(vcenters) == 0 { + return false, errors.New("no vCenter credentials resolved from credentials Secret") + } + seen := map[string]struct{}{} for i, vcenter := range vcenters { if vcenter.VCenter == "" { return false, fmt.Errorf("missing or invalid vCenter in index %d of vcenter list", i) } + seen[vcenter.VCenter] = struct{}{} valid, err := validateVSphereCredentials(vcenter.VCenter, vcenter.Username, vcenter.Password, rootCAFiles, logger) if err != nil || valid == false { return false, err } } + for server := range expectedVCenters { + if _, ok := seen[server]; !ok { + return false, fmt.Errorf("missing credentials for configured vCenter %q", server) + } + } return true, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/utils/credentials.go` around lines 84 - 117, The legacy branch can panic by dereferencing cd.Spec.Platform.VSphere.Infrastructure and can allow partial/missing credentials because the constructed vcenters slice isn't checked against the configured Infrastructure.VCenters; before iterating use a nil guard on cd.Spec.Platform.VSphere.Infrastructure (and return an error if nil), ensure cd.Spec.Platform.VSphere.Infrastructure.VCenters is non-empty, and when projecting legacy creds from constants.UsernameSecretKey/constants.PasswordSecretKey validate those keys exist and are non-empty for every configured vcenter (return an error if any are missing) so that the final vcenters slice length matches cd.Spec.Platform.VSphere.Infrastructure.VCenters length; keep calling validateVSphereCredentials for each vcenter and propagate errors as before.pkg/creds/vsphere/vsphere.go (1)
34-47:⚠️ Potential issue | 🔴 CriticalGuard
metadata.VSpherebefore old-style assignment and final VCenters check.Line 39 and Line 46 can dereference
metadata.VSpherewhen it is nil, leading to a panic in old-style or missing-secret flows.🐛 Proposed fix
} else { // Old style creds Secret with flat username/password: copy the creds across all vcenters username := strings.TrimSpace(string(credsSecret.Data[constants.UsernameSecretKey])) password := strings.TrimSpace(string(credsSecret.Data[constants.PasswordSecretKey])) + if metadata.VSphere == nil { + log.Fatal("old-style credentials require metadata.VSphere to be pre-populated with vCenter names") + } // Caller pre-populated this with just the VCenter names for i := range metadata.VSphere.VCenters { metadata.VSphere.VCenters[i].Username = username metadata.VSphere.VCenters[i].Password = password } } } // If we get here and there are no VCenters, something is wrong - if len(metadata.VSphere.VCenters) == 0 { + if metadata.VSphere == nil || len(metadata.VSphere.VCenters) == 0 { log.Fatal("Can't construct credenttials with no VCenters!") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/creds/vsphere/vsphere.go` around lines 34 - 47, The code dereferences metadata.VSphere and metadata.VSphere.VCenters in the old-style credential branch and in the final length check; guard against a nil metadata.VSphere before using it by checking if metadata.VSphere == nil and either initializing metadata.VSphere = &SomeVSphereStruct{VCenters: []VCenter{}} (or the appropriate zero value used in your codebase) or returning/logging a clear error; update the block that assigns Username/Password to iterate only when metadata.VSphere != nil and change the final check to safely handle a nil metadata.VSphere (e.g., treat nil as zero VCenters) so references to metadata.VSphere and metadata.VSphere.VCenters (used in the loop and the len(...) check) cannot panic.pkg/install/generate.go (1)
892-903:⚠️ Potential issue | 🟡 MinorGuard legacy vSphere deprovision from empty vCenter input.
When both
req.Spec.Platform.VSphere.VCentersandreq.Spec.Platform.VSphere.DeprecatedVCenterare empty, Line [902] still passes--vsphere-vcenters "", which fails later at runtime. Add an explicit validation guard before invoking the vSphere completion path.🛠️ Proposed fix
func GenerateUninstallerJobForDeprovision( @@ switch { @@ case req.Spec.Platform.VSphere != nil: + if legacy && len(req.Spec.Platform.VSphere.VCenters) == 0 && req.Spec.Platform.VSphere.DeprecatedVCenter == "" { + return nil, errors.New("legacy vSphere deprovision requires at least one vCenter in spec.platform.vsphere.vcenters or spec.platform.vsphere.vCenter") + } completeVSphereDeprovisionJob(req, job, args)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/install/generate.go` around lines 892 - 903, The current vSphere deprovision path always sets --vsphere-vcenters even when both req.Spec.Platform.VSphere.VCenters and req.Spec.Platform.VSphere.DeprecatedVCenter are empty, which results in a runtime failure; update the logic around joinedVCenters to validate that at least one vCenter value exists and if both are empty either return an error (validation) or omit the --vsphere-vcenters flag from args before constructing args for the deprovision vsphere command (referencing joinedVCenters, req.Spec.Platform.VSphere.VCenters, req.Spec.Platform.VSphere.DeprecatedVCenter and the args slice used for "deprovision", "vsphere").pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go (1)
671-678:⚠️ Potential issue | 🟠 MajorDon’t overwrite the entire old vSphere struct during zonal migration.
This still masks unrelated immutable edits (e.g., credentials/certs) in the same update. Normalize only
Infrastructurebefore immutable diffing.🛠️ Suggested fix
if oldObject.Spec.Platform.VSphere != nil && cd.Spec.Platform.VSphere != nil { // HIVE-2391: Moving from a non-zonal to a zonal shape is permitted. // NOTE: Existing deprecated fields may be left populated, but will be ignored. if oldObject.Spec.Platform.VSphere.Infrastructure == nil && cd.Spec.Platform.VSphere.Infrastructure != nil { contextLogger.Debug("Allowing vsphere zonal conversion") - // copy over the value to spoof the immutability checker - oldObject.Spec.Platform.VSphere = cd.Spec.Platform.VSphere + normalizedOld := oldObject.Spec.Platform.VSphere.DeepCopy() + normalizedOld.Infrastructure = cd.Spec.Platform.VSphere.Infrastructure + oldObject.Spec.Platform.VSphere = normalizedOld } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go` around lines 671 - 678, The code currently replaces the entire oldObject.Spec.Platform.VSphere with cd.Spec.Platform.VSphere during zonal conversion, which hides unrelated immutable changes; instead, update only the Infrastructure field: when oldObject.Spec.Platform.VSphere.Infrastructure is nil and cd.Spec.Platform.VSphere.Infrastructure is non-nil, set oldObject.Spec.Platform.VSphere.Infrastructure = cd.Spec.Platform.VSphere.Infrastructure (leave other fields like credentials/certs unchanged) before running the immutable diff in the same function where you handle the vsphere zonal conversion.pkg/controller/clusterdeployment/clusterdeployment_controller.go (1)
448-458:⚠️ Potential issue | 🟠 MajorBroaden vSphere up-conversion guard to handle empty Infrastructure.
Only checking
Infrastructure == nilmisses legacy-shaped specs whereInfrastructureexists butVCentersis empty. That can flow downstream into empty vCenter metadata/deprovision data.🛠️ Suggested fix
if getClusterPlatform(cd) == constants.PlatformVSphere { - if cd.Spec.Platform.VSphere.Infrastructure == nil { + infra := cd.Spec.Platform.VSphere.Infrastructure + if infra == nil || (len(infra.VCenters) == 0 && cd.Spec.Platform.VSphere.DeprecatedVCenter != "") { r.logger.WithField("gvk", cd.GroupVersionKind().String()).WithField("name", cd.Name).WithField("namespace", cd.Namespace).Info("Updating deprecated vSphere fields on ClusterDeployment object") cd = cd.DeepCopy() if err := vsphereutils.ConvertDeprecatedFields(cd.Spec.Platform.VSphere); err != nil { cdLog.WithError(err).Log(controllerutils.LogLevel(err), "failed to update deprecated vSphere fields") return reconcile.Result{}, err } return reconcile.Result{}, r.Update(ctx, cd) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/clusterdeployment/clusterdeployment_controller.go` around lines 448 - 458, The vSphere up-conversion currently only runs when cd.Spec.Platform.VSphere.Infrastructure == nil, which misses legacy objects where Infrastructure exists but has an empty VCenters slice; update the guard so it triggers when Infrastructure is nil OR when Infrastructure is non-nil but len(Infrastructure.VCenters) == 0, taking care to nil-check Infrastructure before reading VCenters; keep the existing DeepCopy, call to vsphereutils.ConvertDeprecatedFields(cd.Spec.Platform.VSphere), logging via cdLog.WithError(err) and the r.Update(ctx, cd) behavior unchanged.docs/using-hive.md (2)
593-602:⚠️ Potential issue | 🟡 MinorUse
user(notusername) in the install-configvcentersexample.The vSphere platform schema uses
useras the YAML/JSON key for the username field in VCenter entries. Lines 594 and 599 showusername, which will cause install-config validation failures.- server: myvcenter1.devcluster.openshift.com - username: vsphereuser1 + user: vsphereuser1 password: secretpassword1 datacenters: - datacenter1 - server: myvcenter2.devcluster.openshift.com - username: vsphereuser2 + user: vsphereuser2 password: secretpassword2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/using-hive.md` around lines 593 - 602, The vSphere install-config example in the vcenters entries uses the wrong key name: replace each occurrence of the YAML key "username" with "user" in the vcenters examples (the two VCenter blocks shown) so the field matches the vSphere platform schema and avoids install-config validation failures.
229-240:⚠️ Potential issue | 🟡 MinorAlign prose key naming with the example payload.
Line 229 describes the secret as containing "server, username, and password", but the example at lines 234-239 uses
vCenter,username, andpasswordas keys. Update the prose to match the example:-The Secret should contain the key `vcenters` whose value is a YAML (or JSON) document listing each VCenter's server, username, and password. +The Secret should contain the key `vcenters` whose value is a YAML (or JSON) document listing each VCenter's vCenter, username, and password.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/using-hive.md` around lines 229 - 240, Update the prose that describes the Secret to use the same key names as the example payload: change "server, username, and password" to "vCenter, username, and password" so it matches the `vcenters` secret and the example keys (e.g., `vCenter`, `username`, `password`).contrib/pkg/createcluster/create.go (2)
833-838:⚠️ Potential issue | 🟠 MajorAvoid defaulting VIP arrays to empty-string entries.
When flags are omitted, this writes
[]string{""}forAPIVIPs/IngressVIPs, which can fail schema/IP validation. Only assign VIPs when the flag values are non-empty:Proposed fix
- if len(platform.APIVIPs) == 0 { + if len(platform.APIVIPs) == 0 && o.VSphereAPIVIP != "" { platform.APIVIPs = []string{o.VSphereAPIVIP} } - if len(platform.IngressVIPs) == 0 { + if len(platform.IngressVIPs) == 0 && o.VSphereIngressVIP != "" { platform.IngressVIPs = []string{o.VSphereIngressVIP} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/createcluster/create.go` around lines 833 - 838, The current assignment blindly sets platform.APIVIPs = []string{o.VSphereAPIVIP} and platform.IngressVIPs = []string{o.VSphereIngressVIP}, which creates []string{""} when flags are omitted; change the logic in the create function to only set platform.APIVIPs and platform.IngressVIPs when o.VSphereAPIVIP and o.VSphereIngressVIP are non-empty (e.g., check strings.TrimSpace(...) != "" before assigning), leaving the slices nil/unchanged otherwise to avoid producing empty-string entries that break validation.
814-822:⚠️ Potential issue | 🟠 MajorLegacy-flag mode should emit a webhook-valid vSphere shape.
This path only populates deprecated installer fields (
DeprecatedUsername,DeprecatedPassword,DeprecatedNetwork, etc.) but leavesplatform.VCentersempty. With current admission behavior, this can produce ClusterDeployments that fail validation despite using legacy flags.Consider constructing a minimal
VCentersentry from the legacy values:Proposed fix
platform.DeprecatedUsername = vsphereUsername platform.DeprecatedPassword = vspherePassword platform.DeprecatedNetwork = vSphereNetwork platform.DeprecatedDatacenter = vSphereDatacenter platform.DeprecatedDefaultDatastore = vSphereDatastore platform.DeprecatedVCenter = vSphereVCenter platform.DeprecatedFolder = o.VSphereFolder platform.DeprecatedCluster = o.VSphereCluster + + // Also populate VCenters for webhook compatibility + platform.VCenters = []installervsphere.VCenter{ + { + Server: vSphereVCenter, + Datacenters: []string{vSphereDatacenter}, + Username: vsphereUsername, + Password: vspherePassword, + }, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/createcluster/create.go` around lines 814 - 822, The legacy-flag branch currently only sets Deprecated* fields (platform.DeprecatedUsername, DeprecatedPassword, DeprecatedNetwork, DeprecatedDatacenter, DeprecatedDefaultDatastore, DeprecatedVCenter, DeprecatedFolder, DeprecatedCluster) but leaves platform.VCenters empty, causing webhook validation failures; update the code to, when platform.VCenters is nil/empty and legacy values are present, construct and append a minimal vSphere VCenter entry into platform.VCenters using those legacy values (map DeprecatedVCenter -> VCenter.Server/Name, DeprecatedUsername/DeprecatedPassword -> credentials in the VCenter entry, DeprecatedDatacenter -> Datacenters, DeprecatedDefaultDatastore -> Datacenters[].DefaultDatastore or datastore field, DeprecatedNetwork -> Networks or relevant network field, DeprecatedFolder/DeprecatedCluster -> the folder/cluster fields), and ensure you only add this synthetic VCenter if no VCenters already exist to avoid duplication.
🧹 Nitpick comments (4)
vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go (1)
135-141: Consider addingomitemptyto JSON tags for optional fields.Both
DeprecatedVCenterandVCentersare marked+optionalbut their JSON tags lackomitempty:DeprecatedVCenter string `json:"vCenter"` VCenters []string `json:"vCenters"`This means empty values will be serialized (empty string and empty array respectively). While this matches the existing pattern in
VSphereClusterDeprovisionfor the required fields, optional fields typically useomitemptyto avoid serializing zero values.♻️ Suggested change
- DeprecatedVCenter string `json:"vCenter"` + DeprecatedVCenter string `json:"vCenter,omitempty"` // VCenters are potentially multiple vCenter hostnames. Prefer this field over VCenter. // +optional - VCenters []string `json:"vCenters"` + VCenters []string `json:"vCenters,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go` around lines 135 - 141, The JSON tags for the optional fields DeprecatedVCenter and VCenters currently omit `omitempty`, causing zero-values to be serialized; update the struct field tags for DeprecatedVCenter and VCenters to include `omitempty` (e.g., json:"vCenter,omitempty" and json:"vCenters,omitempty") so empty string/empty slice values are omitted from serialization while leaving the field names and comments unchanged.pkg/controller/machinepool/machinepool_controller.go (1)
1443-1451: Remove the unresolved “? for VSphere” note in the version comment.Line [1446] leaves ambiguous documentation in a decision gate. Please replace it with a concrete statement or simplify the comment to list supported platforms without version placeholders.
♻️ Suggested cleanup
- // Zero-sized minReplicas for autoscaling are allowed since OCP: - // - 4.5 for AWS, Azure, and GCP - // - 4.7 for OpenStack - // - ? for VSphere + // Zero-sized minReplicas for autoscaling are allowed for AWS, Azure, GCP, + // OpenStack, and VSphere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/machinepool/machinepool_controller.go` around lines 1443 - 1451, The comment above the platform check in machinepool_controller.go contains an unresolved placeholder ("? for VSphere") which is ambiguous; update the comment to remove the question mark and either state the concrete version for VSphere or simplify to list only the platforms that allow zero-sized minReplicas (e.g., "Zero-sized minReplicas for autoscaling are allowed since OCP: 4.5 for AWS, Azure, and GCP; 4.7 for OpenStack; and VSphere as supported" or simply "supported on AWS, Azure, GCP, OpenStack, and VSphere"), then keep the if condition that checks cd.Spec.Platform.AWS/Azure/GCP/OpenStack/VSphere unchanged so the logic matches the clarified comment.pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go (1)
1887-1926: Add one regression case for zonal migration with an extra immutable edit.Please add a case where legacy→zonal conversion is attempted and another immutable vSphere field changes in the same update; expected result should be reject. This prevents future regressions around immutability masking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go` around lines 1887 - 1926, Add a new test case to the existing table of tests that uses oldObject: deprecatedVSphereClusterDeployment(), newObject: a modified validVSphereClusterDeployment() that both represents the zonal shape and includes an extra immutable vSphere edit (eg. change cd.Spec.Platform.VSphere.CredentialsSecretRef.Name or cd.Spec.Platform.VSphere.Infrastructure.ClusterOSImage), set operation: admissionv1beta1.Update and expectedAllowed: false; this ensures a legacy→zonal conversion combined with an unrelated immutable vSphere field change is rejected (refer to deprecatedVSphereClusterDeployment, validVSphereClusterDeployment, and admissionv1beta1.Update to locate where to insert the case).contrib/pkg/createcluster/create.go (1)
824-831: Redundant credential defaulting loop.This loop duplicates the logic from lines 773-780. When using the JSON platform spec, credentials are already set in the first loop. When using legacy flags,
platform.VCentersis empty, so this loop has no effect. Consider removing this redundant block:Proposed fix
- for i := range platform.VCenters { - if platform.VCenters[i].Username == "" { - platform.VCenters[i].Username = vsphereUsername - } - if platform.VCenters[i].Password == "" { - platform.VCenters[i].Password = vspherePassword - } - } - if len(platform.APIVIPs) == 0 && o.VSphereAPIVIP != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/createcluster/create.go` around lines 824 - 831, Remove the redundant credential-defaulting loop that iterates over platform.VCenters and assigns vsphereUsername/verspherePassword (the block using platform.VCenters[i].Username and platform.VCenters[i].Password), since the same logic is already applied earlier when parsing the JSON platform spec; simply delete this second loop to avoid duplication and keep the initial loop (the first handling of platform.VCenters credentials) as the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go`:
- Around line 338-346: The IPAddrs field's comment says "one or more" but the
schema lacks a minimum cardinality; update the IPAddrs field declaration
(IPAddrs []string `json:"ipAddrs"`) to include the kubebuilder validation tag
enforcing at least one element by adding the comment annotation //
+kubebuilder:validation:MinItems=1 immediately above the IPAddrs field,
alongside the existing Format/Example/Required tags so the generated CRD matches
the documented intent.
- Around line 325-327: The kubebuilder enum for the Role field currently
includes an empty string which allows role: "" despite
+kubebuilder:validation:Required; edit the Role field's kubebuilder tag (the
line with +kubebuilder:validation:Enum="";bootstrap;control-plane;compute) to
remove the "" option so the enum is only bootstrap;control-plane;compute,
leaving the +kubebuilder:validation:Required intact; this ensures the Role
string field (Role) cannot be empty and must be one of the actual role values.
- Around line 334-336: Replace the duplicate +kubebuilder:validation:Format
annotations on the Gateway, ipAddrs, and nameservers fields with CEL-based
XValidation rules: for Gateway (string) use
+kubebuilder:validation:XValidation:rule="isIP(self)",message="must be a valid
IPv4 or IPv6 address"; for ipAddrs (string or items that can be IP or CIDR) use
+kubebuilder:validation:XValidation:rule="isIP(self) ||
isCIDR(self)",message="must be a valid IPv4/IPv6 address or CIDR"; and for
nameservers (array) move validation to the items level using
+kubebuilder:validation:items:XValidation:rule="isIP(self)",message="must be a
valid IPv4 or IPv6 address" so OR semantics are expressed correctly via CEL
rather than repeated Format tags.
- Around line 211-225: The FailureDomain invariants (RegionType, ZoneType,
hostGroup) are enforced at runtime in validateFailureDomains() but need
XValidation rules added to the FailureDomain CRD schema so the API server fails
fast; add appropriate +kubebuilder:validation:XValidation expressions on the
FailureDomain struct to require mutual presence/absence and valid combinations
(RegionType=Datacenter ⇔ ZoneType=ComputeCluster, RegionType=ComputeCluster ⇔
ZoneType=HostGroup) and to require hostGroup when ZoneType is HostGroup,
mirroring the existing Go checks; also fix the incomplete placeholder error
message emitted by validateFailureDomains() (replace the "something
something..." text with a clear message describing the violated invariant) so
the function returns a concrete, descriptive error.
In `@contrib/pkg/deprovision/vsphere.go`:
- Around line 61-65: The loop that populates o.metadata.VSphere.VCenters from
o.vCenters must validate each entry is non-empty/whitespace before use: inside
the for i, vCenter := range o.vCenters loop, run strings.TrimSpace(vCenter) and
if it is empty return a clear error (e.g., fmt.Errorf or errors.New) indicating
an invalid/empty --vsphere-vcenters entry; otherwise set
o.metadata.VSphere.VCenters[i] = typesvsphere.VCenters{VCenter: vCenter}. Add
the strings import if missing and ensure the function returns the error so
invalid entries fail fast with an actionable message.
In `@docs/hiveutil.md`:
- Around line 99-100: Update the sentence that describes
VSPHERE_INSTALLER_PLATFORM_SPEC_JSON: replace “should be hold a json (NOT yaml)”
with “should hold a JSON (NOT YAML) blob containing a vSphere platform spec” and
ensure “etc” is written as “etc.” (with a period) so the line reads clearly and
uses proper capitalization for JSON/YAML; locate the string
VSPHERE_INSTALLER_PLATFORM_SPEC_JSON in docs/hiveutil.md to make these edits.
In `@pkg/controller/machinepool/vsphereactuator_test.go`:
- Around line 32-38: The "deprecated vsphere fields" table-driven test currently
only checks expectedErr=true; update the test to assert the error reason is the
deprecated-field validation by checking the actual error value from the
actuation call (e.g., the error returned when using
testDeprecatedVSphereClusterDeployment(), testVSpherePool(),
testVSphereMachine("master0","master")) and verifying err != nil and that
err.Error() contains a specific substring or error type tied to deprecated
fields (for example "deprecated" or the exact validation message). Replace the
loose expectedErr boolean check with a targeted assertion (strings.Contains or
Assert.ErrorContains/Require.ErrorContains depending on test helpers) so
unrelated errors won't pass this case.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go`:
- Around line 535-540: The error uses an incorrect field path by inserting an
extra "vSphere" child under vspherePath; update the field.Required call to use
the actual path under spec.platform.vsphere by removing .Child("vSphere") and
referencing vspherePath.Child("vcenters").Index(0) (matching the
Infrastructure.VCenters check) so the validation error reports the correct field
path.
---
Duplicate comments:
In `@contrib/pkg/createcluster/create.go`:
- Around line 833-838: The current assignment blindly sets platform.APIVIPs =
[]string{o.VSphereAPIVIP} and platform.IngressVIPs =
[]string{o.VSphereIngressVIP}, which creates []string{""} when flags are
omitted; change the logic in the create function to only set platform.APIVIPs
and platform.IngressVIPs when o.VSphereAPIVIP and o.VSphereIngressVIP are
non-empty (e.g., check strings.TrimSpace(...) != "" before assigning), leaving
the slices nil/unchanged otherwise to avoid producing empty-string entries that
break validation.
- Around line 814-822: The legacy-flag branch currently only sets Deprecated*
fields (platform.DeprecatedUsername, DeprecatedPassword, DeprecatedNetwork,
DeprecatedDatacenter, DeprecatedDefaultDatastore, DeprecatedVCenter,
DeprecatedFolder, DeprecatedCluster) but leaves platform.VCenters empty, causing
webhook validation failures; update the code to, when platform.VCenters is
nil/empty and legacy values are present, construct and append a minimal vSphere
VCenter entry into platform.VCenters using those legacy values (map
DeprecatedVCenter -> VCenter.Server/Name, DeprecatedUsername/DeprecatedPassword
-> credentials in the VCenter entry, DeprecatedDatacenter -> Datacenters,
DeprecatedDefaultDatastore -> Datacenters[].DefaultDatastore or datastore field,
DeprecatedNetwork -> Networks or relevant network field,
DeprecatedFolder/DeprecatedCluster -> the folder/cluster fields), and ensure you
only add this synthetic VCenter if no VCenters already exist to avoid
duplication.
In `@docs/hiveutil.md`:
- Line 110: The markdown code span for GOVC_USERNAME includes an internal
trailing space; edit the docs/hiveutil.md line containing the `GOVC_USERNAME `
span and remove the extra space so the span reads `GOVC_USERNAME`, ensuring the
environment variable name has no trailing whitespace in the rendered docs and
matches other occurrences of GOVC_USERNAME.
- Around line 106-115: The table incorrectly marks --vsphere-api-vip and
--vsphere-ingress-vip as required even though VIPs can be provided inside
VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json; update the
table text to indicate those two command-line parameters are optional when VIPs
are embedded in the platform spec JSON (e.g., change their “Environment Variable
/ Command line parameter” cells to note “Optional if VIPs are included in
VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json”), and
ensure the surrounding paragraph clarifies that either the JSON
(VSPHERE_INSTALLER_PLATFORM_SPEC_JSON / --vsphere-platform-spec-json) or the
explicit flags (--vsphere-api-vip and --vsphere-ingress-vip) may be used but
both are not required simultaneously.
In `@docs/using-hive.md`:
- Around line 593-602: The vSphere install-config example in the vcenters
entries uses the wrong key name: replace each occurrence of the YAML key
"username" with "user" in the vcenters examples (the two VCenter blocks shown)
so the field matches the vSphere platform schema and avoids install-config
validation failures.
- Around line 229-240: Update the prose that describes the Secret to use the
same key names as the example payload: change "server, username, and password"
to "vCenter, username, and password" so it matches the `vcenters` secret and the
example keys (e.g., `vCenter`, `username`, `password`).
In `@pkg/controller/clusterdeployment/clusterdeployment_controller.go`:
- Around line 448-458: The vSphere up-conversion currently only runs when
cd.Spec.Platform.VSphere.Infrastructure == nil, which misses legacy objects
where Infrastructure exists but has an empty VCenters slice; update the guard so
it triggers when Infrastructure is nil OR when Infrastructure is non-nil but
len(Infrastructure.VCenters) == 0, taking care to nil-check Infrastructure
before reading VCenters; keep the existing DeepCopy, call to
vsphereutils.ConvertDeprecatedFields(cd.Spec.Platform.VSphere), logging via
cdLog.WithError(err) and the r.Update(ctx, cd) behavior unchanged.
In `@pkg/controller/utils/credentials.go`:
- Around line 84-117: The legacy branch can panic by dereferencing
cd.Spec.Platform.VSphere.Infrastructure and can allow partial/missing
credentials because the constructed vcenters slice isn't checked against the
configured Infrastructure.VCenters; before iterating use a nil guard on
cd.Spec.Platform.VSphere.Infrastructure (and return an error if nil), ensure
cd.Spec.Platform.VSphere.Infrastructure.VCenters is non-empty, and when
projecting legacy creds from
constants.UsernameSecretKey/constants.PasswordSecretKey validate those keys
exist and are non-empty for every configured vcenter (return an error if any are
missing) so that the final vcenters slice length matches
cd.Spec.Platform.VSphere.Infrastructure.VCenters length; keep calling
validateVSphereCredentials for each vcenter and propagate errors as before.
In `@pkg/creds/vsphere/vsphere.go`:
- Around line 34-47: The code dereferences metadata.VSphere and
metadata.VSphere.VCenters in the old-style credential branch and in the final
length check; guard against a nil metadata.VSphere before using it by checking
if metadata.VSphere == nil and either initializing metadata.VSphere =
&SomeVSphereStruct{VCenters: []VCenter{}} (or the appropriate zero value used in
your codebase) or returning/logging a clear error; update the block that assigns
Username/Password to iterate only when metadata.VSphere != nil and change the
final check to safely handle a nil metadata.VSphere (e.g., treat nil as zero
VCenters) so references to metadata.VSphere and metadata.VSphere.VCenters (used
in the loop and the len(...) check) cannot panic.
In `@pkg/install/generate.go`:
- Around line 892-903: The current vSphere deprovision path always sets
--vsphere-vcenters even when both req.Spec.Platform.VSphere.VCenters and
req.Spec.Platform.VSphere.DeprecatedVCenter are empty, which results in a
runtime failure; update the logic around joinedVCenters to validate that at
least one vCenter value exists and if both are empty either return an error
(validation) or omit the --vsphere-vcenters flag from args before constructing
args for the deprovision vsphere command (referencing joinedVCenters,
req.Spec.Platform.VSphere.VCenters, req.Spec.Platform.VSphere.DeprecatedVCenter
and the args slice used for "deprovision", "vsphere").
In `@pkg/installmanager/installmanager.go`:
- Around line 739-752: The code currently dereferences
cd.Spec.Platform.VSphere.Infrastructure.VCenters without checking that
Infrastructure is non-nil; update the block that builds vcenters (the loop
creating vcenters and the creation of installertypes.ClusterMetadata with
installertypesvsphere.Metadata) to first verify
cd.Spec.Platform.VSphere.Infrastructure != nil and only iterate over
Infrastructure.VCenters when non-nil (otherwise leave vcenters empty or use a
safe fallback), ensuring no nil pointer access when cd.Spec.Platform.VSphere is
present but Infrastructure is absent.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go`:
- Around line 671-678: The code currently replaces the entire
oldObject.Spec.Platform.VSphere with cd.Spec.Platform.VSphere during zonal
conversion, which hides unrelated immutable changes; instead, update only the
Infrastructure field: when oldObject.Spec.Platform.VSphere.Infrastructure is nil
and cd.Spec.Platform.VSphere.Infrastructure is non-nil, set
oldObject.Spec.Platform.VSphere.Infrastructure =
cd.Spec.Platform.VSphere.Infrastructure (leave other fields like
credentials/certs unchanged) before running the immutable diff in the same
function where you handle the vsphere zonal conversion.
---
Nitpick comments:
In `@contrib/pkg/createcluster/create.go`:
- Around line 824-831: Remove the redundant credential-defaulting loop that
iterates over platform.VCenters and assigns vsphereUsername/verspherePassword
(the block using platform.VCenters[i].Username and
platform.VCenters[i].Password), since the same logic is already applied earlier
when parsing the JSON platform spec; simply delete this second loop to avoid
duplication and keep the initial loop (the first handling of platform.VCenters
credentials) as the single source of truth.
In `@pkg/controller/machinepool/machinepool_controller.go`:
- Around line 1443-1451: The comment above the platform check in
machinepool_controller.go contains an unresolved placeholder ("? for VSphere")
which is ambiguous; update the comment to remove the question mark and either
state the concrete version for VSphere or simplify to list only the platforms
that allow zero-sized minReplicas (e.g., "Zero-sized minReplicas for autoscaling
are allowed since OCP: 4.5 for AWS, Azure, and GCP; 4.7 for OpenStack; and
VSphere as supported" or simply "supported on AWS, Azure, GCP, OpenStack, and
VSphere"), then keep the if condition that checks
cd.Spec.Platform.AWS/Azure/GCP/OpenStack/VSphere unchanged so the logic matches
the clarified comment.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go`:
- Around line 1887-1926: Add a new test case to the existing table of tests that
uses oldObject: deprecatedVSphereClusterDeployment(), newObject: a modified
validVSphereClusterDeployment() that both represents the zonal shape and
includes an extra immutable vSphere edit (eg. change
cd.Spec.Platform.VSphere.CredentialsSecretRef.Name or
cd.Spec.Platform.VSphere.Infrastructure.ClusterOSImage), set operation:
admissionv1beta1.Update and expectedAllowed: false; this ensures a legacy→zonal
conversion combined with an unrelated immutable vSphere field change is rejected
(refer to deprecatedVSphereClusterDeployment, validVSphereClusterDeployment, and
admissionv1beta1.Update to locate where to insert the case).
In `@vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go`:
- Around line 135-141: The JSON tags for the optional fields DeprecatedVCenter
and VCenters currently omit `omitempty`, causing zero-values to be serialized;
update the struct field tags for DeprecatedVCenter and VCenters to include
`omitempty` (e.g., json:"vCenter,omitempty" and json:"vCenters,omitempty") so
empty string/empty slice values are omitted from serialization while leaving the
field names and comments unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00e8ebed-65ec-4749-abe2-62f0faf7efbd
⛔ Files ignored due to path filters (1)
apis/go.sumis excluded by!**/*.sum
📒 Files selected for processing (51)
apis/go.modapis/hive/v1/clusterdeprovision_types.goapis/hive/v1/vsphere/machinepools.goapis/hive/v1/vsphere/platform.goapis/hive/v1/vsphere/zz_generated.deepcopy.goapis/hive/v1/zz_generated.deepcopy.goapis/vendor/github.com/openshift/installer/LICENSEapis/vendor/github.com/openshift/installer/NOTICEapis/vendor/github.com/openshift/installer/pkg/types/vsphere/OWNERSapis/vendor/github.com/openshift/installer/pkg/types/vsphere/doc.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/machinepool.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/metadata.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.goapis/vendor/github.com/openshift/installer/pkg/types/vsphere/zz_generated.deepcopy.goapis/vendor/modules.txtconfig/crds/hive.openshift.io_clusterdeployments.yamlconfig/crds/hive.openshift.io_clusterdeprovisions.yamlconfig/crds/hive.openshift.io_clusterpools.yamlconfig/crds/hive.openshift.io_machinepools.yamlcontrib/pkg/createcluster/create.gocontrib/pkg/deprovision/vsphere.godocs/hiveutil.mddocs/using-hive.mdgo.modhack/app-sre/saas-template.yamlhack/e2e-common.shpkg/clusterresource/builder_test.gopkg/clusterresource/vsphere.gopkg/controller/clusterdeployment/clusterdeployment_controller.gopkg/controller/clusterdeployment/installconfigvalidation.gopkg/controller/clusterdeployment/installconfigvalidation_test.gopkg/controller/clusterpool/clusterpool_controller.gopkg/controller/machinepool/machinepool_controller.gopkg/controller/machinepool/machinepool_controller_test.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/controller/utils/credentials.gopkg/controller/utils/vsphereutils/vsphere.gopkg/controller/utils/vsphereutils/vsphereutils_test.gopkg/creds/vsphere/vsphere.gopkg/install/generate.gopkg/installmanager/installmanager.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.gopkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.gopkg/validating-webhooks/hive/v1/machinepool_validating_admission_hook_test.govendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/machinepools.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/platform.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.govendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go
✅ Files skipped from review due to trivial changes (1)
- apis/vendor/github.com/openshift/installer/NOTICE
🚧 Files skipped from review as they are similar to previous changes (15)
- hack/e2e-common.sh
- pkg/controller/utils/vsphereutils/vsphere.go
- vendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go
- pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go
- pkg/controller/clusterdeployment/installconfigvalidation.go
- pkg/controller/machinepool/machinepool_controller_test.go
- apis/hive/v1/vsphere/platform.go
- pkg/controller/utils/vsphereutils/vsphereutils_test.go
- apis/vendor/github.com/openshift/installer/pkg/types/vsphere/machinepool.go
- apis/hive/v1/clusterdeprovision_types.go
- apis/hive/v1/vsphere/machinepools.go
- vendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.go
- apis/vendor/modules.txt
- apis/vendor/github.com/openshift/installer/pkg/types/vsphere/zz_generated.deepcopy.go
- apis/vendor/github.com/openshift/installer/pkg/types/vsphere/metadata.go
apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go
Show resolved
Hide resolved
apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go
Show resolved
Hide resolved
apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go
Show resolved
Hide resolved
apis/vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go
Show resolved
Hide resolved
pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
Show resolved
Hide resolved
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
f28d497 to
5e115f3
Compare
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
contrib/pkg/deprovision/vsphere.go (1)
63-69:⚠️ Potential issue | 🟡 MinorTrim
--vsphere-vcentersvalues before validating them.Whitespace-only entries still pass this check and later behave like an empty vCenter host.
🛠️ Suggested fix
for i, vCenter := range o.vCenters { - if vCenter == "" { - return fmt.Errorf("received an empty vcenter parameter") + vCenter = strings.TrimSpace(vCenter) + if vCenter == "" { + return fmt.Errorf("--vsphere-vcenters[%d] cannot be empty", i) } o.metadata.VSphere.VCenters[i] = typesvsphere.VCenters{ VCenter: vCenter, } }Also add
stringsto the import block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/deprovision/vsphere.go` around lines 63 - 69, Loop over o.vCenters currently treats whitespace-only entries as valid; call strings.TrimSpace on each vCenter before validating and assigning to o.metadata.VSphere.VCenters (i.e., replace uses of the loop variable vCenter with trimmed := strings.TrimSpace(vCenter)), then return fmt.Errorf("received an empty vcenter parameter") if trimmed == "" and assign VCenter: trimmed into typesvsphere.VCenters; also add "strings" to the import block.docs/using-hive.md (1)
229-230:⚠️ Potential issue | 🟡 MinorDon't describe the Secret payload as
server/username/password.This Secret now follows the metadata-style keys shown below (
vCenter,username,password), not the install-configvcentersshape (server,user,password). As written, the prose contradicts the example and nudges readers toward the wrong payload.📝 Suggested wording
-The Secret should contain the key `vcenters` whose value is a YAML (or JSON) document listing each VCenter's server, username, and password. -The values should match those in your install-config.yaml. +The Secret should contain the key `vcenters` whose value is a YAML (or JSON) document listing each entry's `vCenter`, `username`, and `password`. +Use the same vCenter hostnames and credentials that you would use in your install-config.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/using-hive.md` around lines 229 - 230, The prose incorrectly instructs readers to use the install-config vcenters payload (`server/user/password`) whereas the example and actual Secret use metadata-style keys (`vCenter`, `username`, `password`); update the sentence to describe the Secret as containing the key `vcenters` whose value is a YAML/JSON document listing each entry with `vCenter`, `username`, and `password` (matching the example), not `server/user/password`, so the wording aligns with the example and avoids suggesting the old install-config shape.pkg/controller/utils/credentials.go (1)
84-121:⚠️ Potential issue | 🟠 MajorFail closed when the resolved vCenter set is incomplete.
This path returns success after validating only the entries it could materialize. If the Secret omits one of
cd.Spec.Platform.VSphere.Infrastructure.VCenters, or the legacy projection yields zero entries, the preflight check can still pass even though install/deprovision will depend on those servers.🛠️ Suggested hardening
// Account for both possible shapes of the creds Secret vcenters := []vsphere.VCenters{} + expected := map[string]struct{}{} + if infra := cd.Spec.Platform.VSphere.Infrastructure; infra != nil { + for _, configured := range infra.VCenters { + expected[configured.Server] = struct{}{} + } + } if b, ok := secret.Data["vcenters"]; ok && len(b) > 0 { // New shape: ["vcenters"] contains a yaml blob with a slice of metadata VCenters logger.Info("validating vsphere credentials with vcenters list") @@ - for i, vcenter := range vcenters { + if len(vcenters) == 0 { + return false, errors.New("no vCenter credentials resolved from credentials Secret") + } + + seen := map[string]struct{}{} + for i, vcenter := range vcenters { if vcenter.VCenter == "" { return false, fmt.Errorf("missing or invalid vCenter in index %d of vcenter list", i) } + seen[vcenter.VCenter] = struct{}{} valid, err := validateVSphereCredentials(vcenter.VCenter, vcenter.Username, vcenter.Password, rootCAFiles, logger) if err != nil || valid == false { return false, err } } + for server := range expected { + if _, ok := seen[server]; !ok { + return false, fmt.Errorf("missing credentials for configured vCenter %q", server) + } + } return true, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/utils/credentials.go` around lines 84 - 121, Ensure we fail closed when the resolved vcenters slice is incomplete: after building vcenters (the []vsphere.VCenters) check that it is non-empty and that it contains an entry for every configured server in cd.Spec.Platform.VSphere.Infrastructure.VCenters (for legacy projection verify the projected count matches len(cd.Spec.Platform.VSphere.Infrastructure.VCenters); for the "vcenters" yaml shape ensure all expected servers are present), and return an error if any configured vCenter is missing or if vcenters is empty (instead of proceeding to validation). Keep the existing validateVSphereCredentials loop but only run it when the above completeness checks pass; reference vcenters, cd.Spec.Platform.VSphere.Infrastructure.VCenters, validateVSphereCredentials, and the legacy use of constants.UsernameSecretKey/constants.PasswordSecretKey.docs/hiveutil.md (1)
108-115:⚠️ Potential issue | 🟡 MinorClarify that the VIP flags are only fallbacks.
contrib/pkg/createcluster/create.go, Lines 856-866 only requires--vsphere-api-vipand--vsphere-ingress-vipwhenapiVIPs/ingressVIPsare absent from--vsphere-platform-spec-json, so this table still overstates the requirement and conflicts with the example below.Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/hiveutil.md` around lines 108 - 115, Update the docs table to indicate that the CLI flags --vsphere-api-vip and --vsphere-ingress-vip are fallbacks only when the corresponding apiVIPs / ingressVIPs fields are missing from the JSON provided to --vsphere-platform-spec-json; reference the logic in contrib/pkg/createcluster/create.go (look at the code paths that check apiVIPs and ingressVIPs) and change the table entries (and the repeated row at line ~119) to say “Optional; required only if apiVIPs/ingressVIPs are not present in --vsphere-platform-spec-json” so the table matches the example and the code behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/pkg/createcluster/create.go`:
- Around line 767-782: The code validates GOVC_USERNAME/GOVC_PASSWORD too early,
causing fully populated --vsphere-platform-spec-json payloads with per-vCenter
creds to still require env vars; update create.go so you first unmarshal
platformBytes into installervsphere.Platform (platform) and only then perform
the GOVC_USERNAME/GOVC_PASSWORD presence check if (a) the legacy path is in use
or (b) any platform.VCenters entry has an empty Username or Password that would
need to fall back to vsphereUsername/vspherePassword; in short, move or defer
the env-var validation until after parsing platformBytes and scanning
platform.VCenters (and keep the current fallback assignment loop for
platform.VCenters), so per-vCenter credentials in the JSON are accepted without
global GOVC_* env vars.
In `@pkg/clusterresource/vsphere.go`:
- Around line 32-33: GetCloudPlatform currently returns the Infrastructure
pointer (Infrastructure) which still contains VCenter credentials; instead, make
GetCloudPlatform create and return a deep copy of p.Infrastructure and scrub the
VCenter credentials on that copy (set Username and Password fields in each
VCenter entry to empty) so the original Infrastructure pointer with credentials
remains used only by addInstallConfigPlatform / install-config and secret
generation while the ClusterDeployment gets a credential-free copy.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go`:
- Around line 677-678: The zonal-conversion guard only checks for nil vs non-nil
VSphere Infrastructure and misses the legacy empty-struct case; in
validateCreate (in clusterdeployment_validating_admission_hook.go) update the
condition that inspects oldObject.Spec.Platform.VSphere.Infrastructure and
cd.Spec.Platform.VSphere.Infrastructure to treat an empty Infrastructure struct
the same as nil (legacy). Implement a check that considers oldInfra empty (e.g.
reflect.DeepEqual(oldInfra, &hivev1.VSpherePlatform{}) or use
apiequality.Semantic.DeepEqual) or use a helper isEmptyInfra(oldInfra) so the
branch allows conversion when old infra is nil OR empty and new infra is
non-nil/non-empty. Ensure you reference
oldObject.Spec.Platform.VSphere.Infrastructure and
cd.Spec.Platform.VSphere.Infrastructure and update the conditional accordingly.
- Around line 671-680: The current zonal conversion code in validateUpdate()
overwrites oldObject.Spec.Platform.VSphere with cd.Spec.Platform.VSphere to
bypass immutability, but never revalidates vSphere required fields so an update
can succeed with empty credentialsSecretRef.name or certificatesSecretRef.name;
update the block that detects oldObject.Spec.Platform.VSphere == nil &&
cd.Spec.Platform.VSphere != nil to explicitly validate that
cd.Spec.Platform.VSphere contains non-empty required fields (at least
credentialsSecretRef.name and certificatesSecretRef.name) and reject the update
(return an admission error) if they are missing, or alternatively copy only the
allowed fields and then invoke the existing platform validation routine used
elsewhere so platform validation is performed after the spoofing.
---
Duplicate comments:
In `@contrib/pkg/deprovision/vsphere.go`:
- Around line 63-69: Loop over o.vCenters currently treats whitespace-only
entries as valid; call strings.TrimSpace on each vCenter before validating and
assigning to o.metadata.VSphere.VCenters (i.e., replace uses of the loop
variable vCenter with trimmed := strings.TrimSpace(vCenter)), then return
fmt.Errorf("received an empty vcenter parameter") if trimmed == "" and assign
VCenter: trimmed into typesvsphere.VCenters; also add "strings" to the import
block.
In `@docs/hiveutil.md`:
- Around line 108-115: Update the docs table to indicate that the CLI flags
--vsphere-api-vip and --vsphere-ingress-vip are fallbacks only when the
corresponding apiVIPs / ingressVIPs fields are missing from the JSON provided to
--vsphere-platform-spec-json; reference the logic in
contrib/pkg/createcluster/create.go (look at the code paths that check apiVIPs
and ingressVIPs) and change the table entries (and the repeated row at line
~119) to say “Optional; required only if apiVIPs/ingressVIPs are not present in
--vsphere-platform-spec-json” so the table matches the example and the code
behavior.
In `@docs/using-hive.md`:
- Around line 229-230: The prose incorrectly instructs readers to use the
install-config vcenters payload (`server/user/password`) whereas the example and
actual Secret use metadata-style keys (`vCenter`, `username`, `password`);
update the sentence to describe the Secret as containing the key `vcenters`
whose value is a YAML/JSON document listing each entry with `vCenter`,
`username`, and `password` (matching the example), not `server/user/password`,
so the wording aligns with the example and avoids suggesting the old
install-config shape.
In `@pkg/controller/utils/credentials.go`:
- Around line 84-121: Ensure we fail closed when the resolved vcenters slice is
incomplete: after building vcenters (the []vsphere.VCenters) check that it is
non-empty and that it contains an entry for every configured server in
cd.Spec.Platform.VSphere.Infrastructure.VCenters (for legacy projection verify
the projected count matches
len(cd.Spec.Platform.VSphere.Infrastructure.VCenters); for the "vcenters" yaml
shape ensure all expected servers are present), and return an error if any
configured vCenter is missing or if vcenters is empty (instead of proceeding to
validation). Keep the existing validateVSphereCredentials loop but only run it
when the above completeness checks pass; reference vcenters,
cd.Spec.Platform.VSphere.Infrastructure.VCenters, validateVSphereCredentials,
and the legacy use of constants.UsernameSecretKey/constants.PasswordSecretKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce093492-06a3-4710-baf7-b48df673f007
📒 Files selected for processing (28)
apis/hive/v1/clusterdeprovision_types.goapis/hive/v1/vsphere/machinepools.goapis/hive/v1/vsphere/zz_generated.deepcopy.goconfig/crds/hive.openshift.io_clusterdeprovisions.yamlconfig/crds/hive.openshift.io_machinepools.yamlcontrib/pkg/createcluster/create.gocontrib/pkg/deprovision/vsphere.godocs/hiveutil.mddocs/using-hive.mdhack/app-sre/saas-template.yamlpkg/clusterresource/builder_test.gopkg/clusterresource/vsphere.gopkg/controller/clusterdeployment/clusterdeployment_controller.gopkg/controller/clusterdeployment/installconfigvalidation.gopkg/controller/clusterdeployment/installconfigvalidation_test.gopkg/controller/machinepool/machinepool_controller.gopkg/controller/machinepool/machinepool_controller_test.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/controller/utils/credentials.gopkg/creds/vsphere/vsphere.gopkg/install/generate.gopkg/installmanager/installmanager.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.govendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/machinepools.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controller/machinepool/machinepool_controller_test.go
- pkg/installmanager/installmanager.go
- pkg/controller/machinepool/machinepool_controller.go
- apis/hive/v1/clusterdeprovision_types.go
- pkg/install/generate.go
pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
Show resolved
Hide resolved
pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
Show resolved
Hide resolved
Contributor
|
@2uasimojo The regression tests passed on the latest commit, the PR can be merged if no further comment to be addressed. |
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
5e115f3 to
167c4e7
Compare
|
@2uasimojo: This pull request references HIVE-2391 which is a valid jira issue. DetailsIn response to this:
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. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
docs/hiveutil.md (1)
106-115:⚠️ Potential issue | 🟡 MinorThe vSphere requirements table still overstates what is mandatory.
create.gostill supports the legacy-flag path when--vsphere-platform-spec-jsonis omitted, and in JSON mode the VIP flags are only fallbacks when the payload omitted them. The implementation also accepts per-vCenter credentials inside the JSON itself, so presentingGOVC_USERNAME/GOVC_PASSWORDas universally required env vars is misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/hiveutil.md` around lines 106 - 115, The vSphere requirements table misstates mandatory inputs; update docs/hiveutil.md to reflect that create.go supports both legacy-flag and JSON payload modes: note that GOVC_USERNAME and GOVC_PASSWORD are not universally required because per-vCenter credentials can be supplied inside the VSPHERE_INSTALLER_PLATFORM_SPEC_JSON payload, that GOVC_TLS_CA_CERTS is only needed when using GOVC-based auth (mention it as conditional), and that --vsphere-api-vip and --vsphere-ingress-vip are fallbacks used only if those IPs are absent from the JSON (when VSPHERE_INSTALLER_PLATFORM_SPEC_JSON is provided); reference the flags --vsphere-platform-spec-json, --vsphere-api-vip, --vsphere-ingress-vip and file create.go when editing the table and surrounding text to make optionality and precedence clear.pkg/clusterresource/vsphere.go (1)
45-50:⚠️ Potential issue | 🔴 CriticalThe vCenter scrub loop still leaks secrets into the ClusterDeployment copy.
GetCloudPlatform()relies onInfrastructure(true)to sanitize the installer platform before serializing it, butfor _, v := range i.VCentersiterates by value. Line 49 clears only a copy, so everyVCenters[*].Passwordsurvives.DeprecatedUsernameandVCenters[*].Usernameare also left intact.🔒 Proposed fix
- // Scrub credentials - i := b.infrastructure.DeepCopy() - i.DeprecatedPassword = "" - for _, v := range i.VCenters { - v.Password = "" - } - return i + // Scrub credentials + infra := b.infrastructure.DeepCopy() + infra.DeprecatedUsername = "" + infra.DeprecatedPassword = "" + for idx := range infra.VCenters { + infra.VCenters[idx].Username = "" + infra.VCenters[idx].Password = "" + } + return infraExpected result:
Platform.VCentersresolves to a[]VCenterslice, so the range variable is a copy and the current loop cannot mutate the backing elements.#!/bin/bash set -euo pipefail sed -n '41,52p' pkg/clusterresource/vsphere.go rg -n -C2 'type VCenter struct|VCenters \[\]VCenter' --type go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/clusterresource/vsphere.go` around lines 45 - 50, The scrub loop in the Infrastructure deep-copy does not actually clear vCenter credentials because it ranges over the slice by value; update the scrubbing in the DeepCopy result (i := b.infrastructure.DeepCopy()) to clear DeprecatedPassword and DeprecatedUsername and to mute each VCenters element by index (e.g., for idx := range i.VCenters { i.VCenters[idx].Password = ""; i.VCenters[idx].Username = "" }) or otherwise assign back into the slice so Password/Username are removed; ensure this change affects the same variables used by GetCloudPlatform()/Infrastructure(true) so the serialized Platform.VCenters contains no secrets.
🧹 Nitpick comments (2)
contrib/pkg/createcluster/create.go (1)
776-781: Normalize JSON-supplied platform specs before readingplatform.VCenters.Only the legacy-flag branch runs
conversion.ConvertInstallConfig, but the remainder of the method assumes the canonicalplatform.VCentersshape. Moving the conversion below the branch point would keep legacy flags and older JSON blobs on the same path.♻️ Proposed refactor
if len(platformBytes) > 0 { err = json.Unmarshal(platformBytes, &platform) if err != nil { return nil, fmt.Errorf("error decoding platform %s: %w", o.VSpherePlatformSpecJSON, err) } - // We'll inject credentials from env vars later, if unset in the json blob } else { o.log.Info("Platform spec not provided, trying legacy flags") @@ - dummyInstallConfig := &installertypes.InstallConfig{ - Platform: installertypes.Platform{ - VSphere: &platform, - }, - } - if err := conversion.ConvertInstallConfig(dummyInstallConfig); err != nil { - return nil, fmt.Errorf("failed to update deprecated vSphere fields") - } } + + dummyInstallConfig := &installertypes.InstallConfig{ + Platform: installertypes.Platform{ + VSphere: &platform, + }, + } + if err := conversion.ConvertInstallConfig(dummyInstallConfig); err != nil { + return nil, fmt.Errorf("failed to update deprecated vSphere fields: %w", err) + }Also applies to: 823-830
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/pkg/createcluster/create.go` around lines 776 - 781, The JSON-parsed VSphere platform (from platformBytes into variable platform) must be normalized before any code reads platform.VCenters: run conversion.ConvertInstallConfig (or the equivalent normalization step) immediately after successful json.Unmarshal of platformBytes so both legacy-flag and non-legacy paths see the canonical platform.VCenters shape; update the logic around the legacy-flag branch (the code handling legacy flags and the later block that expects platform.VCenters) so conversion happens once right after unmarshalling (also apply the same change to the similar block around the other occurrence noted at lines 823-830), ensuring subsequent credential injection still runs after normalization.pkg/controller/utils/credentials.go (1)
117-118: Minor: Prefer!validover== false.Idiomatic Go uses
!validrather thanvalid == false.♻️ Suggested change
- if err != nil || valid == false { + if err != nil || !valid { return false, err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/utils/credentials.go` around lines 117 - 118, Replace the non-idiomatic comparison in the conditional that checks validation status: in the branch using "err" and "valid" (the line with "if err != nil || valid == false { return false, err }"), change the second check to use the idiomatic negation operator (!valid) so the condition reads "if err != nil || !valid { return false, err }".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/pkg/createcluster/create.go`:
- Around line 815-816: The calls to setCredValue (e.g., setCredValue("username",
&platform.DeprecatedUsername, vsphereUsername) and setCredValue("password",
&platform.DeprecatedPassword, vspherePassword)) currently ignore its returned
error, allowing missing credentials to be silently accepted; update each call
(including the other occurrences in the block around the 833-847 range) to
capture and propagate the error instead of discarding it—i.e., check the
returned error from setCredValue and return or surface it from the enclosing
function (or aggregate/handle it consistently with surrounding error handling)
so credential validation failures are not suppressed.
In `@docs/hiveutil.md`:
- Around line 118-120: The example CLI payload for the create-cluster command is
incomplete because create.go no longer backfills
datacenter/datastore/cluster/network from legacy flags; update the sample (or
annotate it as schematic) so copy/paste users get a valid vSpherePlatform spec:
either (A) expand the vCenters entry to include datacenters, datastores,
clusters and network entries (fill in realistic example values for datacenter,
datastore, cluster, and network) so the JSON is runnable, or (B) add a clear
note next to the bin/hiveutil create-cluster sample explaining it is schematic
and that create.go requires explicit datacenter/datastore/cluster/network in the
vCenters entries and will not auto-populate them. Reference create-cluster and
create.go and the vCenters/datacenters/datastore/cluster/network fields when
making the change.
- Around line 101-104: The fenced code snippet shows shell prompts (`$`) which
triggers markdownlint MD014; edit the snippet in the docs/hiveutil.md fenced
block so the lines contain the commands only (remove the leading `$` prompts) or
include corresponding output lines if you want prompts kept, e.g. replace "$
export VSPHERE_INSTALLER_PLATFORM_SPEC_JSON=$(</path/to/file)" and "$ echo
$VSPHERE_INSTALLER_PLATFORM_SPEC_JSON" with "export
VSPHERE_INSTALLER_PLATFORM_SPEC_JSON=$(</path/to/file)" and "echo
$VSPHERE_INSTALLER_PLATFORM_SPEC_JSON" to eliminate the lint warning.
In `@pkg/controller/clusterdeployment/installconfigvalidation_test.go`:
- Around line 413-416: In the test YAML blocks that define vcenters, the field
name is incorrectly set to "user" — change every occurrence of the "user" key
inside vcenters entries to "username" so the tests use the expected field name;
search for YAML blocks containing the "vcenters:" key and update each child map
(e.g., the entries currently showing "- user: foo") to "- username: foo" and
ensure all similar test cases in the file are updated consistently.
In `@pkg/controller/utils/credentials.go`:
- Around line 86-95: The new-shape branch that unmarshals
secret.Data["vcenters"] into vcenters must also verify it covers all configured
vCenters from cd.Spec.Platform.VSphere.Infrastructure.VCenters; after
yaml.Unmarshal(b, &vcenters) iterate the configured list and ensure each
configured VCenter (by name/ID as used in the legacy path) is present in the
unmarshaled vcenters slice, and if any are missing return false with an error
(or at minimum logger.WithFields(...).Warnf(...) if you prefer a warning);
update the validation in the function that currently handles the new-shape path
to mirror the legacy-path coverage check so incomplete blobs do not pass
validation.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go`:
- Around line 1901-1937: The table-driven tests for ClusterDeployment admission
cases (the entries with names "vsphere platform can be zonal-ified but
creds/certs refs still required", "vsphere platform cannot be de-zonal-ified",
"vsphere platform immutable if not being zonal-ified (deprecated shape)", and
"vsphere platform immutable if not being zonal-ified (zonal shape)") are missing
the explicit expectedAllowed field; update each test case struct (the literal
that includes name, oldObject, newObject, operation) to include expectedAllowed:
false so the intent is explicit and won’t rely on the bool zero value during
test runs.
In `@vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go`:
- Around line 138-141: The DeprecatedVCenter and VCenters struct fields in
clusterdeprovision_types.go are marked optional but their json tags lack
omitempty; update the json tags for DeprecatedVCenter and VCenters (the
DeprecatedVCenter string field and the VCenters []string field) to include
",omitempty" so zero values (empty string and nil/empty slice) are omitted from
serialized output, matching the rest of the optional fields in this file.
---
Duplicate comments:
In `@docs/hiveutil.md`:
- Around line 106-115: The vSphere requirements table misstates mandatory
inputs; update docs/hiveutil.md to reflect that create.go supports both
legacy-flag and JSON payload modes: note that GOVC_USERNAME and GOVC_PASSWORD
are not universally required because per-vCenter credentials can be supplied
inside the VSPHERE_INSTALLER_PLATFORM_SPEC_JSON payload, that GOVC_TLS_CA_CERTS
is only needed when using GOVC-based auth (mention it as conditional), and that
--vsphere-api-vip and --vsphere-ingress-vip are fallbacks used only if those IPs
are absent from the JSON (when VSPHERE_INSTALLER_PLATFORM_SPEC_JSON is
provided); reference the flags --vsphere-platform-spec-json, --vsphere-api-vip,
--vsphere-ingress-vip and file create.go when editing the table and surrounding
text to make optionality and precedence clear.
In `@pkg/clusterresource/vsphere.go`:
- Around line 45-50: The scrub loop in the Infrastructure deep-copy does not
actually clear vCenter credentials because it ranges over the slice by value;
update the scrubbing in the DeepCopy result (i := b.infrastructure.DeepCopy())
to clear DeprecatedPassword and DeprecatedUsername and to mute each VCenters
element by index (e.g., for idx := range i.VCenters { i.VCenters[idx].Password =
""; i.VCenters[idx].Username = "" }) or otherwise assign back into the slice so
Password/Username are removed; ensure this change affects the same variables
used by GetCloudPlatform()/Infrastructure(true) so the serialized
Platform.VCenters contains no secrets.
---
Nitpick comments:
In `@contrib/pkg/createcluster/create.go`:
- Around line 776-781: The JSON-parsed VSphere platform (from platformBytes into
variable platform) must be normalized before any code reads platform.VCenters:
run conversion.ConvertInstallConfig (or the equivalent normalization step)
immediately after successful json.Unmarshal of platformBytes so both legacy-flag
and non-legacy paths see the canonical platform.VCenters shape; update the logic
around the legacy-flag branch (the code handling legacy flags and the later
block that expects platform.VCenters) so conversion happens once right after
unmarshalling (also apply the same change to the similar block around the other
occurrence noted at lines 823-830), ensuring subsequent credential injection
still runs after normalization.
In `@pkg/controller/utils/credentials.go`:
- Around line 117-118: Replace the non-idiomatic comparison in the conditional
that checks validation status: in the branch using "err" and "valid" (the line
with "if err != nil || valid == false { return false, err }"), change the second
check to use the idiomatic negation operator (!valid) so the condition reads "if
err != nil || !valid { return false, err }".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67bcd4a6-a9de-400c-99c7-5dd0a8323a15
📒 Files selected for processing (29)
apis/hive/v1/clusterdeprovision_types.goapis/hive/v1/vsphere/machinepools.goapis/hive/v1/vsphere/zz_generated.deepcopy.goconfig/crds/hive.openshift.io_clusterdeprovisions.yamlconfig/crds/hive.openshift.io_machinepools.yamlcontrib/pkg/createcluster/create.gocontrib/pkg/deprovision/vsphere.godocs/hiveutil.mddocs/using-hive.mdhack/app-sre/saas-template.yamlpkg/clusterresource/builder_test.gopkg/clusterresource/vsphere.gopkg/controller/clusterdeployment/clusterdeployment_controller.gopkg/controller/clusterdeployment/installconfigvalidation.gopkg/controller/clusterdeployment/installconfigvalidation_test.gopkg/controller/clusterpool/clusterpool_controller.gopkg/controller/machinepool/machinepool_controller.gopkg/controller/machinepool/machinepool_controller_test.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/controller/utils/credentials.gopkg/creds/vsphere/vsphere.gopkg/install/generate.gopkg/installmanager/installmanager.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.govendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/machinepools.govendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (17)
- apis/hive/v1/clusterdeprovision_types.go
- vendor/github.com/openshift/hive/apis/hive/v1/vsphere/machinepools.go
- pkg/installmanager/installmanager.go
- pkg/controller/machinepool/machinepool_controller.go
- docs/using-hive.md
- pkg/controller/clusterpool/clusterpool_controller.go
- config/crds/hive.openshift.io_machinepools.yaml
- pkg/clusterresource/builder_test.go
- vendor/github.com/openshift/hive/apis/hive/v1/vsphere/zz_generated.deepcopy.go
- pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
- config/crds/hive.openshift.io_clusterdeprovisions.yaml
- apis/hive/v1/vsphere/machinepools.go
- pkg/install/generate.go
- contrib/pkg/deprovision/vsphere.go
- pkg/controller/clusterdeployment/clusterdeployment_controller.go
- pkg/creds/vsphere/vsphere.go
- pkg/controller/machinepool/vsphereactuator.go
Comment on lines
+413
to
+416
| vcenters: | ||
| - user: foo | ||
| password: bar | ||
| - {} |
There was a problem hiding this comment.
Same field name issue: user should be username.
Multiple test cases have the same issue where user: is used instead of username: in the vcenters YAML.
🐛 Suggested fix for lines 413-416
vcenters:
- - user: foo
+ - username: foo
password: bar
- {}🐛 Suggested fix for lines 443-446
vcenters:
- - user: foo
+ - username: foo
password: bar
- {}Also applies to: 443-446
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/clusterdeployment/installconfigvalidation_test.go` around
lines 413 - 416, In the test YAML blocks that define vcenters, the field name is
incorrectly set to "user" — change every occurrence of the "user" key inside
vcenters entries to "username" so the tests use the expected field name; search
for YAML blocks containing the "vcenters:" key and update each child map (e.g.,
the entries currently showing "- user: foo") to "- username: foo" and ensure all
similar test cases in the file are updated consistently.
Member
Author
There was a problem hiding this comment.
Is that right? The schema for that field is coming from here, isn't it?
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the actual struct definition in the vendor file.
🏁 Script executed:
#!/bin/bash
# Check the VCenter struct definition in the vendor file
grep -n -A 20 "type VCenter struct" vendor/github.com/openshift/installer/pkg/types/vsphere/platform.go
Length of output: 1001
@2uasimojo, you're absolutely right — my review comment was incorrect. The VCenter struct at line 306 has:
Username string `json:"user"`The JSON/YAML key is indeed user, not username. The test cases are correct as written. I apologize for the noise.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.go
Show resolved
Hide resolved
vendor/github.com/openshift/hive/apis/hive/v1/clusterdeprovision_types.go
Show resolved
Hide resolved
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
167c4e7 to
7d878a6
Compare
Followon addressing review from openshift#2731. MachinePool: - Removed `Topology` override - Restored ResourcePool and TagIDs overrides - Removed `osImage` detected from arbitrary master; using whatever's passed through from FD Topology (which defaults sanely if unset). Deprovision: - Changed `--vsphere-vcenter` StringVar to `--vsphere-vcenters` StringSliceVar Platform Creds: - Redesigned to take `vcenters`, a list of vcenter server/username/password, matching (and unmarshaling into) the corresponding chunk of metadata.json. Docs: - Updated install-config sample to zonal shape. - Documented new creds shape.
2uasimojo
force-pushed
the
HIVE-2391/vsphere-zonal
branch
from
7d878a6 to
183ef5e
Compare
Member
Author
|
Security fail: https://redhat.atlassian.net/browse/HIVE-3128 |
Contributor
|
@2uasimojo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
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
4 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.
Co-Authored-By: @dlom
Summary by CodeRabbit
New Features
Deprecations
Documentation