fix: Misc. fixes to preflight check results (#1202)

**What problem does this PR solve?**:
Fixes some issues with the check results, including:
- Make the field more precise
- Capitalize messages
- Use valid JSONPath in fields

Also:
- Remove code that is never expected to run
- Do not treat invalid credentials as an internal error

**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.
-->
All changes covered by unit 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/registry.go

@@ -58,7 +58,7 @@ func defaultRegClientGetter(opts ...regclient.Opt) regClientPinger {
 }
 
 func pingFailedReasonString(registryURL string, err error) string {
-	return fmt.Sprintf("failed to ping registry %s with err: %s", registryURL, err.Error())
+	return fmt.Sprintf("Failed to ping registry %s with err: %s", registryURL, err.Error())
 }
 
 func (r *registryCheck) checkRegistry(
@@ -80,7 +80,7 @@ func (r *registryCheck) checkRegistry(
 		result.InternalError = false
 		result.Causes = append(result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("failed to parse registry url %s with error: %s", registryURL, err),
+				Message: fmt.Sprintf("Failed to parse registry URL %q with error: %s", registryURL, err),
 				Field:   r.field + ".url",
 			},
 		)
@@ -132,8 +132,11 @@ func (r *registryCheck) checkRegistry(
 			result.InternalError = true
 			result.Causes = append(result.Causes,
 				preflight.Cause{
-					Message: fmt.Sprintf("failed to get Registry credentials Secret: %s", err),
-					Field:   r.field + ".credentials.secretRef",
+					Message: fmt.Sprintf("Failed to get Registry credentials Secret %q: %s",
+						credentials.SecretRef.Name,
+						err,
+					),
+					Field: r.field + ".credentials.secretRef",
 				},
 			)
 			return result

pkg/webhook/preflight/generic/registry_test.go

@@ -135,7 +135,7 @@ func TestRegistryCheck(t *testing.T) {
 				InternalError: true,
 				Causes: []preflight.Cause{
 					{
-						Message: "failed to get Registry credentials Secret: fake error",
+						Message: "Failed to get Registry credentials Secret \"test-secret\": fake error",
 						//nolint:lll // this is a test for a field.
 						Field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef",
 					},
@@ -300,7 +300,7 @@ func TestRegistryCheck(t *testing.T) {
 				InternalError: false,
 				Causes: []preflight.Cause{
 					{
-						Message: fmt.Sprintf("failed to parse registry url %s with error: "+
+						Message: fmt.Sprintf("Failed to parse registry URL %q with error: "+
 							"parse \"invalid-url\": invalid URI for request", "invalid-url"),
 						Field: "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0].url",
 					},

pkg/webhook/preflight/nutanix/credentials.go

@@ -6,6 +6,7 @@ package nutanix
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -56,7 +57,7 @@ func newCredentialsCheck(
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: "Nutanix cluster configuration is not defined in the cluster spec",
-				Field:   "cluster.spec.topology.variables[.name=clusterConfig].nutanix",
+				Field:   "$.spec.topology.variables[?@.name==\"clusterConfig\"].value.nutanix",
 			},
 		)
 		return credentialsCheck
@@ -71,8 +72,9 @@ func newCredentialsCheck(
 		credentialsCheck.result.Allowed = false
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("failed to parse Prism Central endpoint URL: %s", err),
-				Field:   "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.url",
+				Message: fmt.Sprintf("Failed to parse Prism Central endpoint URL: %s", err),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.url",
 			},
 		)
 		return credentialsCheck
@@ -94,7 +96,8 @@ func newCredentialsCheck(
 			preflight.Cause{
 				Message: fmt.Sprintf("Prism Central credentials Secret %q not found",
 					prismCentralEndpointSpec.Credentials.SecretRef.Name),
-				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
@@ -108,7 +111,8 @@ func newCredentialsCheck(
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 					err,
 				),
-				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
@@ -119,10 +123,11 @@ func newCredentialsCheck(
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf(
-					"credentials Secret %q is empty",
+					"Credentials Secret %q is empty",
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 				),
-				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
@@ -134,11 +139,12 @@ func newCredentialsCheck(
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf(
-					"credentials Secret %q does not contain key %q",
+					"Credentials Secret %q does not contain key %q",
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 					credentialsSecretDataKey,
 				),
-				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
@@ -149,8 +155,9 @@ func newCredentialsCheck(
 		credentialsCheck.result.Allowed = false
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("failed to parse Prism Central credentials: %s", err),
-				Field:   "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials",
+				Message: fmt.Sprintf("Failed to parse Prism Central credentials: %s", err),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
@@ -173,29 +180,40 @@ func newCredentialsCheck(
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf("Failed to initialize Nutanix client: %s", err),
-				Field:   "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials",
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
 	}
 
 	// Validate the credentials using an API call.
 	_, err = nclient.GetCurrentLoggedInUser(ctx)
-	if err != nil {
+	if err == nil {
+		// We initialized both clients, and verified the credentials using the v3 client.
+		cd.nclient = nclient
+		return credentialsCheck
+	}
+
+	if strings.Contains(err.Error(), "invalid Nutanix credentials") {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.InternalError = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("Failed to validate credentials using the v3 API client. "+
-					"The URL and/or credentials may be incorrect. (Error: %q)", err),
-				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint",
+				Message: fmt.Sprintf("Failed to validate credentials using the v3 API client: %s", err),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"]" +
+					".value.nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
 	}
 
-	// We initialized both clients, and verified the credentials using the v3 client.
-	cd.nclient = nclient
-
+	credentialsCheck.result.Allowed = false
+	credentialsCheck.result.InternalError = true
+	credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
+		preflight.Cause{
+			Message: fmt.Sprintf("Failed to validate credentials using the v3 API client: %s", err),
+			Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint",
+		},
+	)
 	return credentialsCheck
 }

pkg/webhook/preflight/nutanix/credentials_test.go

@@ -70,7 +70,7 @@ func TestNewCredentialsCheck_InvalidURL(t *testing.T) {
 	result := check.Run(context.Background())
 	assert.False(t, result.Allowed)
 	assert.False(t, result.InternalError)
-	assert.Contains(t, result.Causes[0].Message, "failed to parse Prism Central endpoint URL")
+	assert.Contains(t, result.Causes[0].Message, "Failed to parse Prism Central endpoint URL")
 }
 
 func TestNewCredentialsCheck_SecretNotFound(t *testing.T) {
@@ -145,7 +145,7 @@ func TestNewCredentialsCheck_SecretEmpty(t *testing.T) {
 	result := check.Run(context.Background())
 	assert.False(t, result.Allowed)
 	assert.False(t, result.InternalError)
-	assert.Contains(t, result.Causes[0].Message, "credentials Secret \"ntnx-creds\" is empty")
+	assert.Contains(t, result.Causes[0].Message, "Credentials Secret \"ntnx-creds\" is empty")
 }
 
 func TestNewCredentialsCheck_SecretMissingKey(t *testing.T) {
@@ -186,7 +186,7 @@ func TestNewCredentialsCheck_InvalidCredentialsFormat(t *testing.T) {
 	result := check.Run(context.Background())
 	assert.False(t, result.Allowed)
 	assert.False(t, result.InternalError)
-	assert.Contains(t, result.Causes[0].Message, "failed to parse Prism Central credentials")
+	assert.Contains(t, result.Causes[0].Message, "Failed to parse Prism Central credentials")
 }
 
 func TestNewCredentialsCheck_FailedToCreateClient(t *testing.T) {
@@ -212,7 +212,21 @@ func TestNewCredentialsCheck_FailedToGetCurrentLoggedInUser(t *testing.T) {
 	result := check.Run(context.Background())
 	assert.False(t, result.Allowed)
 	assert.True(t, result.InternalError)
-	assert.Contains(t, result.Causes[0].Message, "Failed to validate credentials using the v3 API client.")
+	assert.Contains(t, result.Causes[0].Message, "Failed to validate credentials using the v3 API client: "+
+		assert.AnError.Error())
+}
+
+func TestNewCredentialsCheck_GetCurrentLoggedInUserInvalidCredentials(t *testing.T) {
+	nclientFactory := func(_ prismgoclient.Credentials) (client, error) {
+		return &mocknclient{err: fmt.Errorf("invalid Nutanix credentials")}, nil
+	}
+	cd := validCheckDependencies()
+	check := newCredentialsCheck(context.Background(), nclientFactory, cd)
+	result := check.Run(context.Background())
+	assert.False(t, result.Allowed)
+	assert.False(t, result.InternalError)
+	assert.Contains(t, result.Causes[0].Message, "Failed to validate credentials using the v3 API client: "+
+		"invalid Nutanix credentials")
 }
 
 func validCheckDependencies() *checkDependencies {

pkg/webhook/preflight/nutanix/image.go

@@ -33,7 +33,7 @@ func (c *imageCheck) Run(ctx context.Context) preflight.CheckResult {
 		result.Allowed = true
 		result.Warnings = append(
 			result.Warnings,
-			fmt.Sprintf("%s uses imageLookup, which is not yet supported by checks", c.field),
+			fmt.Sprintf("Field %s uses imageLookup, which is not yet supported by checks", c.field),
 		)
 		return result
 	}
@@ -44,17 +44,17 @@ func (c *imageCheck) Run(ctx context.Context) preflight.CheckResult {
 			result.Allowed = false
 			result.InternalError = true
 			result.Causes = append(result.Causes, preflight.Cause{
-				Message: fmt.Sprintf("failed to get VM Image: %s", err),
-				Field:   c.field,
+				Message: fmt.Sprintf("Failed to get VM Image: %s", err),
+				Field:   c.field + ".image",
 			})
 			return result
 		}
 
 		if len(images) != 1 {
 			result.Allowed = false
 			result.Causes = append(result.Causes, preflight.Cause{
-				Message: fmt.Sprintf("expected to find 1 VM Image, found %d", len(images)),
-				Field:   c.field,
+				Message: fmt.Sprintf("Expected to find 1 VM Image, found %d", len(images)),
+				Field:   c.field + ".image",
 			})
 			return result
 		}
@@ -82,7 +82,7 @@ func newVMImageChecks(
 		checks = append(checks,
 			&imageCheck{
 				machineDetails: &cd.nutanixClusterConfigSpec.ControlPlane.Nutanix.MachineDetails,
-				field: "cluster.spec.topology.variables[.name=clusterConfig]" +
+				field: "$.spec.topology.variables[?@.name==\"clusterConfig\"]." +
 					".value.nutanix.controlPlane.machineDetails",
 				nclient: cd.nclient,
 			},
@@ -94,8 +94,8 @@ func newVMImageChecks(
 			checks = append(checks,
 				&imageCheck{
 					machineDetails: &nutanixWorkerNodeConfigSpec.Nutanix.MachineDetails,
-					field: fmt.Sprintf("cluster.spec.topology.workers.machineDeployments[.name=%s]"+
-						".variables[.name=workerConfig].value.nutanix.machineDetails", mdName),
+					field: fmt.Sprintf("$.spec.topology.workers.machineDeployments[?@.name==%q]"+
+						".variables[?@.name=workerConfig].value.nutanix.machineDetails", mdName),
 					nclient: cd.nclient,
 				},
 			)

pkg/webhook/preflight/nutanix/image_test.go

@@ -126,8 +126,8 @@ func TestVMImageCheck(t *testing.T) {
 				Allowed: false,
 				Causes: []preflight.Cause{
 					{
-						Message: "expected to find 1 VM Image, found 0",
-						Field:   "test-field",
+						Message: "Expected to find 1 VM Image, found 0",
+						Field:   "machineDetails.image",
 					},
 				},
 			},
@@ -168,8 +168,8 @@ func TestVMImageCheck(t *testing.T) {
 				Allowed: false,
 				Causes: []preflight.Cause{
 					{
-						Message: "expected to find 1 VM Image, found 2",
-						Field:   "test-field",
+						Message: "Expected to find 1 VM Image, found 2",
+						Field:   "machineDetails.image",
 					},
 				},
 			},
@@ -192,8 +192,8 @@ func TestVMImageCheck(t *testing.T) {
 				InternalError: true,
 				Causes: []preflight.Cause{
 					{
-						Message: "failed to get VM Image: api error",
-						Field:   "test-field",
+						Message: "Failed to get VM Image: api error",
+						Field:   "machineDetails.image",
 					},
 				},
 			},
@@ -225,8 +225,8 @@ func TestVMImageCheck(t *testing.T) {
 				InternalError: true,
 				Causes: []preflight.Cause{
 					{
-						Message: "failed to get VM Image: api error",
-						Field:   "test-field",
+						Message: "Failed to get VM Image: api error",
+						Field:   "machineDetails.image",
 					},
 				},
 			},
@@ -261,8 +261,8 @@ func TestVMImageCheck(t *testing.T) {
 				InternalError: true,
 				Causes: []preflight.Cause{
 					{
-						Message: "failed to get VM Image: failed to get data returned by ListImages",
-						Field:   "test-field",
+						Message: "Failed to get VM Image: failed to get data returned by ListImages",
+						Field:   "machineDetails.image",
 					},
 				},
 			},
@@ -284,7 +284,7 @@ func TestVMImageCheck(t *testing.T) {
 			// Create the check
 			check := &imageCheck{
 				machineDetails: tc.machineDetails,
-				field:          "test-field",
+				field:          "machineDetails",
 				nclient:        tc.nclient,
 			}