fix: More fixes to preflight result messages and fields (#1240)

**What problem does this PR solve?**:
I identified more mistakes in preflight check messages and fields. The
fixes are in separate commits.

**Which issue(s) this PR fixes**:
Fixes #

**How Has This Been Tested?**:
<!--
Please describe the tests that you ran to verify your changes.
Provide output from the tests and any manual steps needed to replicate
the tests.
-->

**Special notes for your reviewer**:
<!--
Use this to provide any additional information to the reviewers.
This may include:
- Best way to review the PR.
- Where the author wants the most review attention on.
- etc.
-->

Author: dlipovetsky

pkg/webhook/preflight/generic/spec.go

@@ -50,7 +50,7 @@ func newConfigurationCheck(
 					carenv1.ClusterConfigVariableName,
 					err,
 				),
-				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].genericClusterConfigSpec",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]",
 			},
 		)
 	}

pkg/webhook/preflight/generic/spec_test.go

@@ -107,7 +107,7 @@ func TestNewConfigurationCheck(t *testing.T) {
 				Causes: []preflight.Cause{
 					{
 						Message: "Failed to unmarshal cluster variable \"clusterConfig\": failed to unmarshal json: invalid character 'i' looking for beginning of object key string. Review the Cluster.", ///nolint:lll // The message is long.
-						Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].genericClusterConfigSpec",
+						Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"]",
 					},
 				},
 			},

pkg/webhook/preflight/nutanix/credentials.go

@@ -222,6 +222,8 @@ func newCredentialsCheck(
 				"Failed to validate credentials: %s. This is usually a temporary error. Please retry.", ///nolint:lll // Message is long.
 				err,
 			),
+			// We do not add ".url" or ".credentials.secretRef" to the field, because we do not know
+			// if the error is related to the URL, or the credentials.
 			Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint",
 		},
 	)

pkg/webhook/preflight/nutanix/failuredomain.go

@@ -88,7 +88,7 @@ func (fdc *failureDomainCheck) Run(ctx context.Context) preflight.CheckResult {
 			result.Allowed = false
 			result.Causes = append(result.Causes, preflight.Cause{
 				Message: fmt.Sprintf(
-					"NutanixFailureDomain %q referenced in cluster was not found in the management cluster. Please create it and retry.", //nolint:lll // Message is long.
+					"NutanixFailureDomain %q was not found in the management cluster. Please create it and retry.", //nolint:lll // Message is long.
 					fdc.failureDomainName,
 				),
 				Field: fdc.field,
@@ -100,7 +100,7 @@ func (fdc *failureDomainCheck) Run(ctx context.Context) preflight.CheckResult {
 		result.InternalError = true
 		result.Causes = append(result.Causes, preflight.Cause{
 			Message: fmt.Sprintf(
-				"Failed to get NutanixFailureDomain %q: %v This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
+				"Failed to get NutanixFailureDomain %q: %s This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
 				fdc.failureDomainName,
 				err,
 			),
@@ -118,7 +118,7 @@ func (fdc *failureDomainCheck) Run(ctx context.Context) preflight.CheckResult {
 		result.InternalError = true
 		result.Causes = append(result.Causes, preflight.Cause{
 			Message: fmt.Sprintf(
-				"Failed to check if the Prism Element cluster %q, referenced by Failure Domain %q, exists: %v This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
+				"Failed to check if the Prism Element cluster %q, referenced by Failure Domain %q, exists: %s. This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
 				peIdentifier,
 				fdc.failureDomainName,
 				err,
@@ -149,7 +149,7 @@ func (fdc *failureDomainCheck) Run(ctx context.Context) preflight.CheckResult {
 			result.InternalError = true
 			result.Causes = append(result.Causes, preflight.Cause{
 				Message: fmt.Sprintf(
-					"Failed to get subnet %q referenced by the Failure Domain %q: %v This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
+					"Failed to get subnet %q referenced by the Failure Domain %q: %s. This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
 					id,
 					fdc.failureDomainName,
 					err,

pkg/webhook/preflight/nutanix/specs.go

@@ -56,7 +56,7 @@ func newConfigurationCheck(
 					carenv1.ClusterConfigVariableName,
 					err,
 				),
-				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]",
 			},
 		)
 	}
@@ -88,7 +88,7 @@ func newConfigurationCheck(
 				)
 				if workerConfigVar != nil {
 					workerConfigFieldPath = fmt.Sprintf(
-						"$.spec.topology.workers.machineDeployments[[email protected]==%q].variables[[email protected]=='%s'].value.nutanix",
+						"$.spec.topology.workers.machineDeployments[[email protected]==%q].variables[[email protected]=='%s']",
 						md.Name,
 						carenv1.WorkerConfigVariableName,
 					)
@@ -101,7 +101,7 @@ func newConfigurationCheck(
 				)
 				if workerConfigVar != nil {
 					workerConfigFieldPath = fmt.Sprintf(
-						"$.spec.topology.variables[[email protected]=='%s'].value.nutanix",
+						"$.spec.topology.variables[[email protected]=='%s']",
 						carenv1.WorkerConfigVariableName,
 					)
 				}

pkg/webhook/preflight/nutanix/specs_test.go

@@ -138,7 +138,7 @@ func TestNewConfigurationCheck(t *testing.T) {
 				Causes: []preflight.Cause{
 					{
 						Message: "Failed to unmarshal cluster variable \"clusterConfig\": failed to unmarshal json: invalid character 'i' looking for beginning of object key string. Review the Cluster.", ///nolint:lll // The message is long.
-						Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix",
+						Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"]",
 					},
 				},
 			},
@@ -185,7 +185,7 @@ func TestNewConfigurationCheck(t *testing.T) {
 				Causes: []preflight.Cause{
 					{
 						Message: "Failed to unmarshal variable \"workerConfig\": failed to unmarshal json: invalid character 'i' looking for beginning of object key string. Review the Cluster.", ///nolint:lll // The message is long.
-						Field:   "$.spec.topology.workers.machineDeployments[[email protected]==\"md-0\"].variables[[email protected]=='workerConfig'].value.nutanix",                                                 ///nolint:lll // The field is long.
+						Field:   "$.spec.topology.workers.machineDeployments[[email protected]==\"md-0\"].variables[[email protected]=='workerConfig']",                                                               ///nolint:lll // The field is long.
 					},
 				},
 			},

pkg/webhook/preflight/nutanix/storagecontainer.go

@@ -47,42 +47,44 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 		result.Allowed = false
 		result.Causes = append(result.Causes, preflight.Cause{
 			Message: "Nutanix CSI Provider configuration is missing storage class configurations. Review the Cluster.", //nolint:lll // Message is long.
-			Field:   c.field,
+			Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.addons.csi.providers.nutanix",        //nolint:lll // The field is long.
 		})
 
 		return result
 	}
 
 	// Get cluster identifier based on whether failure domain is configured
 	var clusterIdentifier *capxv1.NutanixResourceIdentifier
-	var err error
 
 	if c.failureDomainName != "" {
-		// Get cluster identifier from failure domain
-		clusterIdentifier, err = c.getClusterIdentifierFromFailureDomain(ctx)
+		// Use cluster identifier from failure domain
+		fdObj := &capxv1.NutanixFailureDomain{}
+		fdKey := ctrlclient.ObjectKey{Name: c.failureDomainName, Namespace: c.namespace}
+		err := c.kclient.Get(ctx, fdKey, fdObj)
 		if err != nil {
 			result.Allowed = false
 			if errors.IsNotFound(err) {
 				result.Causes = append(result.Causes, preflight.Cause{
 					Message: fmt.Sprintf(
-						"NutanixFailureDomain %q referenced in cluster was not found in the management cluster. Please create it and retry.", //nolint:lll // Message is long.
+						"NutanixFailureDomain %q was not found in the management cluster. Please create it and retry.", //nolint:lll // Message is long.
 						c.failureDomainName,
 					),
-					Field: c.field,
+					Field: c.field + ".failureDomain",
 				})
 			} else {
 				result.InternalError = true
 				result.Causes = append(result.Causes, preflight.Cause{
 					Message: fmt.Sprintf(
-						"Failed to get cluster identifier from NutanixFailureDomain %q: %v This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
+						"Failed to get NutanixFailureDomain %q: %s. This is usually a temporary error. Please retry.", //nolint:lll // Message is long.
 						c.failureDomainName,
 						err,
 					),
-					Field: c.field,
+					Field: c.field + ".failureDomain",
 				})
 			}
 			return result
 		}
+		clusterIdentifier = &fdObj.Spec.PrismElementCluster
 	} else {
 		// Use cluster identifier from machine spec
 		clusterIdentifier = c.machineSpec.Cluster
@@ -118,7 +120,7 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 					clusterIdentifier,
 					err,
 				),
-				Field: c.field,
+				Field: c.field + ".cluster",
 			})
 			continue
 		}
@@ -131,7 +133,7 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 					len(clusters),
 					clusterIdentifier,
 				),
-				Field: c.field,
+				Field: c.field + ".cluster",
 			})
 			continue
 		}
@@ -150,7 +152,7 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 					clusterIdentifier,
 					err,
 				),
-				Field: c.field,
+				Field: c.field + ".cluster",
 			})
 			continue
 		}
@@ -167,7 +169,7 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 					clusterIdentifier,
 					clusterIdentifier,
 				),
-				Field: c.field,
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.addons.csi.providers.nutanix.storageClassConfigs.volume.parameters.storageContainer", //nolint:lll // The field is long.
 			})
 		case 1:
 			continue
@@ -184,27 +186,14 @@ func (c *storageContainerCheck) Run(ctx context.Context) preflight.CheckResult {
 					storageContainerName,
 					clusterIdentifier,
 				),
-				Field: c.field,
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.addons.csi.providers.nutanix.storageClassConfigs.volume.parameters.storageContainer", //nolint:lll // The field is long.
 			})
 		}
 	}
 
 	return result
 }
 
-// getClusterIdentifierFromFailureDomain fetches the failure domain and returns the cluster identifier.
-func (c *storageContainerCheck) getClusterIdentifierFromFailureDomain(
-	ctx context.Context,
-) (*capxv1.NutanixResourceIdentifier, error) {
-	fdObj := &capxv1.NutanixFailureDomain{}
-	fdKey := ctrlclient.ObjectKey{Name: c.failureDomainName, Namespace: c.namespace}
-	if err := c.kclient.Get(ctx, fdKey, fdObj); err != nil {
-		return nil, err
-	}
-
-	return &fdObj.Spec.PrismElementCluster, nil
-}
-
 func newStorageContainerChecks(cd *checkDependencies) []preflight.Check {
 	checks := []preflight.Check{}
 

pkg/webhook/preflight/nutanix/storagecontainer_test.go

@@ -398,6 +398,7 @@ func TestStorageContainerCheck(t *testing.T) {
 		expectedAllowed      bool
 		expectedError        bool
 		expectedCauseMessage string
+		expectedField        string
 		failureDomainName    string
 		kclient              ctrlclient.Client
 		namespace            string
@@ -414,7 +415,7 @@ func TestStorageContainerCheck(t *testing.T) {
 			nclient:              nil,
 			expectedAllowed:      false,
 			expectedError:        false,
-			expectedCauseMessage: "Nutanix CSI Provider configuration is missing storage class configurations",
+			expectedCauseMessage: "Nutanix CSI Provider configuration is missing storage class configurations. Review the Cluster.", //nolint:lll // The message is long.
 		},
 		{
 			name: "storage class config without parameters",
@@ -1154,7 +1155,8 @@ func TestStorageContainerCheck(t *testing.T) {
 			namespace:            "test-ns",
 			expectedAllowed:      false,
 			expectedError:        false,
-			expectedCauseMessage: "NutanixFailureDomain \"non-existent-fd\" referenced in cluster was not found in the management cluster. Please create it and retry.", //nolint:lll // The message is long.
+			expectedCauseMessage: "NutanixFailureDomain \"non-existent-fd\" was not found in the management cluster. Please create it and retry.", //nolint:lll // The message is long.
+			expectedField:        field + ".failureDomain",
 		},
 		{
 			name: "error getting failure domain",
@@ -1183,7 +1185,8 @@ func TestStorageContainerCheck(t *testing.T) {
 			namespace:            "test-ns",
 			expectedAllowed:      false,
 			expectedError:        true,
-			expectedCauseMessage: "Failed to get cluster identifier from NutanixFailureDomain \"fd-with-error\": kube API error This is usually a temporary error. Please retry.", //nolint:lll // The message is long.
+			expectedCauseMessage: "Failed to get NutanixFailureDomain \"fd-with-error\": kube API error. This is usually a temporary error. Please retry.", //nolint:lll // The message is long.
+			expectedField:        field + ".failureDomain",
 		},
 	}
 
@@ -1208,12 +1211,16 @@ func TestStorageContainerCheck(t *testing.T) {
 			assert.Equal(t, tc.expectedAllowed, result.Allowed)
 			assert.Equal(t, tc.expectedError, result.InternalError)
 
-			if tc.expectedCauseMessage != "" {
+			if !tc.expectedAllowed {
 				require.NotEmpty(t, result.Causes)
-				assert.Contains(t, result.Causes[0].Message, tc.expectedCauseMessage)
-				assert.Equal(t, field, result.Causes[0].Field)
-			} else {
-				assert.Empty(t, result.Causes)
+
+				if tc.expectedCauseMessage != "" {
+					assert.Equal(t, tc.expectedCauseMessage, result.Causes[0].Message)
+				}
+
+				if tc.expectedField != "" {
+					assert.Equal(t, tc.expectedField, result.Causes[0].Field)
+				}
 			}
 		})
 	}