feat: Add context to preflight check messages (#1210)

**What problem does this PR solve?**:
Adds more context to preflight check messages.

The storage container check now returns a specific error when we find two storage containers with the same name in the same Prism Central cluster. That should never happen, since storage container names are unique within a Prism Central cluster. 

**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/registry.go

@@ -57,8 +57,12 @@ func defaultRegClientGetter(opts ...regclient.Opt) regClientPinger {
 	return regclient.New(opts...)
 }
 
-func pingFailedReasonString(registryURL string, err error) string {
-	return fmt.Sprintf("Failed to ping registry %s with err: %s", registryURL, err.Error())
+func pingFailedMessage(registryURL string, err error) string {
+	return fmt.Sprintf(
+		"Failed to ping registry %q: %s. First verify that the management cluster can reach the registry, then retry.", ///nolint:lll // Message is long.
+		registryURL,
+		err.Error(),
+	)
 }
 
 func (r *registryCheck) checkRegistry(
@@ -80,8 +84,12 @@ func (r *registryCheck) checkRegistry(
 		result.InternalError = false
 		result.Causes = append(result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("Failed to parse registry URL %q with error: %s", registryURL, err),
-				Field:   r.field + ".url",
+				Message: fmt.Sprintf(
+					"Failed to parse registry URL %q with error: %s. Review the Cluster.", ///nolint:lll // Message is long.",
+					registryURL,
+					err,
+				),
+				Field: r.field + ".url",
 			},
 		)
 		return result
@@ -94,7 +102,8 @@ func (r *registryCheck) checkRegistry(
 		result.Causes = append(result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf(
-					"Registry URL scheme %q is not supported. Use http or https.",
+					"The registry URL %q uses the scheme %q. This scheme is not supported. Use either the \"http\" or \"https\" scheme.", ///nolint:lll // Message is long.
+					registryURL,
 					registryURLParsed.Scheme,
 				),
 				Field: r.field + ".url",
@@ -121,8 +130,11 @@ func (r *registryCheck) checkRegistry(
 			result.InternalError = false
 			result.Causes = append(result.Causes,
 				preflight.Cause{
-					Message: fmt.Sprintf("Registry credentials Secret %q not found", credentials.SecretRef.Name),
-					Field:   r.field + ".credentials.secretRef",
+					Message: fmt.Sprintf(
+						"Registry credentials Secret %q not found. Create the Secret first, then create the Cluster.", ///nolint:lll // Message is long.
+						credentials.SecretRef.Name,
+					),
+					Field: r.field + ".credentials.secretRef",
 				},
 			)
 			return result
@@ -132,7 +144,8 @@ func (r *registryCheck) checkRegistry(
 			result.InternalError = true
 			result.Causes = append(result.Causes,
 				preflight.Cause{
-					Message: fmt.Sprintf("Failed to get Registry credentials Secret %q: %s",
+					Message: fmt.Sprintf(
+						"Failed to get Registry credentials Secret %q: %s. This is usually a temporary error. Please retry.", ///nolint:lll // Message is long.
 						credentials.SecretRef.Name,
 						err,
 					),
@@ -170,7 +183,7 @@ func (r *registryCheck) checkRegistry(
 		result.Allowed = false
 		result.Causes = append(result.Causes,
 			preflight.Cause{
-				Message: pingFailedReasonString(registryURL, err),
+				Message: pingFailedMessage(registryURL, err),
 				Field:   r.field,
 			},
 		)

pkg/webhook/preflight/generic/registry_test.go

@@ -135,9 +135,8 @@ func TestRegistryCheck(t *testing.T) {
 				InternalError: true,
 				Causes: []preflight.Cause{
 					{
-						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",
+						Message: "Failed to get Registry credentials Secret \"test-secret\": fake error. This is usually a temporary error. Please retry.", ///nolint:lll // Message is long.
+						Field:   "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef",              //nolint:lll // Field is long.
 					},
 				},
 			},
@@ -167,9 +166,8 @@ func TestRegistryCheck(t *testing.T) {
 				InternalError: false,
 				Causes: []preflight.Cause{
 					{
-						Message: "Registry credentials Secret \"test-secret\" not found",
-						//nolint:lll // this is a test for a field.
-						Field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef",
+						Message: "Registry credentials Secret \"test-secret\" not found. Create the Secret first, then create the Cluster.",   ///nolint:lll // Message is long.
+						Field:   "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef", //nolint:lll // Field is long.
 					},
 				},
 			},
@@ -189,7 +187,7 @@ func TestRegistryCheck(t *testing.T) {
 				Allowed: false,
 				Causes: []preflight.Cause{
 					{
-						Message: pingFailedReasonString(
+						Message: pingFailedMessage(
 							testRegistryURL,
 							testPingFailedError,
 						),
@@ -300,11 +298,8 @@ func TestRegistryCheck(t *testing.T) {
 				InternalError: false,
 				Causes: []preflight.Cause{
 					{
-						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",
+						Message: "Failed to parse registry URL \"invalid-url\" with error: parse \"invalid-url\": invalid URI for request. Review the Cluster.", ///nolint:lll // Message is long.
+						Field:   "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0].url",
 					},
 				},
 			},
@@ -345,7 +340,7 @@ func TestRegistryCheck(t *testing.T) {
 				InternalError: false,
 				Causes: []preflight.Cause{
 					{
-						Message: "Registry URL scheme \"tcp\" is not supported. Use http or https.",
+						Message: "The registry URL \"tcp://some-registry.lol\" uses the scheme \"tcp\". This scheme is not supported. Use either the \"http\" or \"https\" scheme.", ///nolint:lll // Message is long.
 						Field:   "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0].url",
 					},
 				},

pkg/webhook/preflight/generic/spec.go

@@ -45,7 +45,8 @@ func newConfigurationCheck(
 		configurationCheck.result.InternalError = true
 		configurationCheck.result.Causes = append(configurationCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("Failed to unmarshal cluster variable %s: %s",
+				Message: fmt.Sprintf(
+					"Failed to unmarshal cluster variable %q: %s. Review the Cluster.", ///nolint:lll // Message is long.
 					carenv1.ClusterConfigVariableName,
 					err,
 				),

pkg/webhook/preflight/generic/spec_test.go

@@ -106,8 +106,7 @@ func TestNewConfigurationCheck(t *testing.T) {
 				InternalError: true,
 				Causes: []preflight.Cause{
 					{
-						///nolint:lll // The message is long.
-						Message: "Failed to unmarshal cluster variable clusterConfig: failed to unmarshal json: invalid character 'i' looking for beginning of object key string",
+						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:   "cluster.spec.topology.variables[.name=clusterConfig].genericClusterConfigSpec",
 					},
 				},

pkg/webhook/preflight/nutanix/credentials.go

@@ -56,7 +56,7 @@ func newCredentialsCheck(
 		credentialsCheck.result.Allowed = false
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: "Nutanix cluster configuration is not defined in the cluster spec",
+				Message: "The Nutanix configuration is missing from the Cluster resource. Review the Cluster resource.", ///nolint:lll // Message is long.
 				Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix",
 			},
 		)
@@ -72,8 +72,11 @@ 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:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.url", ///nolint:lll // Field is long.
+				Message: fmt.Sprintf(
+					"Failed to parse the Prism Central endpoint URL %q: %s. Check the URL format and retry.",
+					prismCentralEndpointSpec.URL,
+					err),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.url", ///nolint:lll // Field is long.
 			},
 		)
 		return credentialsCheck
@@ -93,8 +96,10 @@ func newCredentialsCheck(
 		credentialsCheck.result.InternalError = false
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("Prism Central credentials Secret %q not found",
-					prismCentralEndpointSpec.Credentials.SecretRef.Name),
+				Message: fmt.Sprintf(
+					"Prism Central credentials Secret %q not found. Create the Secret first, then create the Cluster.", ///nolint:lll // Message is long.
+					prismCentralEndpointSpec.Credentials.SecretRef.Name,
+				),
 				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
 			},
 		)
@@ -105,7 +110,8 @@ func newCredentialsCheck(
 		credentialsCheck.result.InternalError = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("Failed to get Prism Central credentials Secret %q: %s",
+				Message: fmt.Sprintf(
+					"Failed to get Prism Central credentials Secret %q: %s. This is usually a temporary error. Please retry.",
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 					err,
 				),
@@ -120,7 +126,7 @@ func newCredentialsCheck(
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf(
-					"Credentials Secret %q is empty",
+					"Credentials Secret %q is empty. Review the Secret.", ///nolint:lll // Message is long.
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 				),
 				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
@@ -135,7 +141,7 @@ 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. Review the Secret.", ///nolint:lll // Message is long.
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 					credentialsSecretDataKey,
 				),
@@ -150,8 +156,11 @@ 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:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
+				Message: fmt.Sprintf(
+					"Failed to parse Prism Central credentials: %s. Review the Secret.", ///nolint:lll // Message is long.
+					err,
+				),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
 			},
 		)
 		return credentialsCheck
@@ -173,8 +182,11 @@ func newCredentialsCheck(
 		credentialsCheck.result.InternalError = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("Failed to initialize Nutanix client: %s", err),
-				Field:   "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
+				Message: fmt.Sprintf(
+					"Failed to initialize the Nutanix Prism Central API client: %s.", ///nolint:lll // Message is long.",
+					err,
+				),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
 			},
 		)
 		return credentialsCheck
@@ -192,8 +204,11 @@ func newCredentialsCheck(
 		credentialsCheck.result.Allowed = false
 		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.credentials.secretRef", ///nolint:lll // Field is long.
+				Message: fmt.Sprintf(
+					"Failed to validate credentials: %s. Please check the username and/or password.", ///nolint:lll // Message is long.
+					err,
+				),
+				Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint.credentials.secretRef", ///nolint:lll // Field is long.
 			},
 		)
 		return credentialsCheck
@@ -203,8 +218,11 @@ func newCredentialsCheck(
 	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",
+			Message: fmt.Sprintf(
+				"Failed to validate credentials: %s. This is usually a temporary error. Please retry.", ///nolint:lll // Message is long.
+				err,
+			),
+			Field: "$.spec.topology.variables[[email protected]==\"clusterConfig\"].value.nutanix.prismCentralEndpoint",
 		},
 	)
 	return credentialsCheck

pkg/webhook/preflight/nutanix/credentials_test.go

@@ -57,7 +57,11 @@ func TestNewCredentialsCheck_MissingNutanixField(t *testing.T) {
 	assert.False(t, result.Allowed)
 	assert.False(t, result.InternalError)
 	assert.NotEmpty(t, result.Causes)
-	assert.Contains(t, result.Causes[0].Message, "Nutanix cluster configuration is not defined")
+	assert.Contains(
+		t,
+		result.Causes[0].Message,
+		"The Nutanix configuration is missing from the Cluster resource. Review the Cluster resource.", ///nolint:lll // Message is long.
+	)
 }
 
 func TestNewCredentialsCheck_InvalidURL(t *testing.T) {
@@ -70,7 +74,11 @@ 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 the Prism Central endpoint URL \"not-a-url\": error parsing Prism Central URL: parse \"not-a-url\": invalid URI for request. Check the URL format and retry.", ///nolint:lll // Message is long.
+	)
 }
 
 func TestNewCredentialsCheck_SecretNotFound(t *testing.T) {
@@ -199,7 +207,11 @@ func TestNewCredentialsCheck_FailedToCreateClient(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 initialize Nutanix client")
+	assert.Contains(
+		t,
+		result.Causes[0].Message,
+		"Failed to initialize the Nutanix Prism Central API client: assert.AnError general error for testing.", ///nolint:lll // Message is long.
+	)
 }
 
 func TestNewCredentialsCheck_FailedToGetCurrentLoggedInUser(t *testing.T) {
@@ -212,7 +224,7 @@ 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: "+
 		assert.AnError.Error())
 }
 
@@ -225,7 +237,7 @@ func TestNewCredentialsCheck_GetCurrentLoggedInUserInvalidCredentials(t *testing
 	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: "+
+	assert.Contains(t, result.Causes[0].Message, "Failed to validate credentials: "+
 		"invalid Nutanix credentials")
 }
 

pkg/webhook/preflight/nutanix/image.go

@@ -44,17 +44,25 @@ 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 + ".image",
+				Message: fmt.Sprintf(
+					"Failed to get VM Image %q: %s. This is usually a temporary error. Please retry.",
+					c.machineDetails.Image,
+					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 + ".image",
+				Message: fmt.Sprintf(
+					"Found %d VM Images in Prism Central that match identifier %q. There must be exactly 1 VM Image that matches this identifier. Remove duplicate VM Images, use a different VM Image, or identify the VM Image by its UUID, then retry.", ///nolint:lll // Message is long.
+					len(images),
+					c.machineDetails.Image,
+				),
+				Field: c.field + ".image",
 			})
 			return result
 		}