fix: Do not treat expected preflight check failures as internal errors (#1195)

**What problem does this PR solve?**:
Internal errors are meant to signal some unexpected failure. We have
been treating a number of expected failures as internal errors, and this
PR fixes these.

It also makes CheckResult interface easier to understand by renaming
the Error field to InternalError.

**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

@@ -14,6 +14,7 @@ import (
 	"github.com/regclient/regclient/types/ping"
 	"github.com/regclient/regclient/types/ref"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -76,7 +77,7 @@ func (r *registryCheck) checkRegistry(
 	registryURLParsed, err := url.ParseRequestURI(registryURL)
 	if err != nil {
 		result.Allowed = false
-		result.Error = true
+		result.InternalError = false
 		result.Causes = append(result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf("failed to parse registry url %s with error: %s", registryURL, err),
@@ -98,9 +99,20 @@ func (r *registryCheck) checkRegistry(
 			},
 			mirrorCredentialsSecret,
 		)
+		if apierrors.IsNotFound(err) {
+			result.Allowed = false
+			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",
+				},
+			)
+			return result
+		}
 		if err != nil {
 			result.Allowed = false
-			result.Error = true
+			result.InternalError = true
 			result.Causes = append(result.Causes,
 				preflight.Cause{
 					Message: fmt.Sprintf("failed to get Registry credentials Secret: %s", err),
@@ -125,19 +137,15 @@ func (r *registryCheck) checkRegistry(
 		regclient.WithConfigHost(mirrorHost),
 		regclient.WithUserAgent("regclient/example"),
 	)
-	mirrorRef, err := ref.NewHost(registryURLParsed.Host)
-	if err != nil {
-		result.Allowed = false
-		result.Error = true
-		result.Causes = append(result.Causes,
-			preflight.Cause{
-				Message: fmt.Sprintf("failed to create a client to verify registry configuration %s", err.Error()),
-				Field:   r.field,
-			},
-		)
-		return result
-	}
-	_, err = rc.Ping(ctx, mirrorRef) // ping will return an error for anything that's not 200
+	_, err = rc.Ping(ctx,
+		ref.Ref{
+			// Because we ping the registry, we only need the "registry" part of the ref.
+			Registry: registryURLParsed.Host,
+			// The default scheme is "reg""
+			Scheme: "reg",
+		},
+	)
+	// Ping will return an error for anything that's not 200.
 	if err != nil {
 		result.Allowed = false
 		result.Causes = append(result.Causes,
@@ -149,7 +157,7 @@ func (r *registryCheck) checkRegistry(
 		return result
 	}
 	result.Allowed = true
-	result.Error = false
+	result.InternalError = false
 	return result
 }
 

pkg/webhook/preflight/generic/registry_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -126,15 +127,47 @@ func TestRegistryCheck(t *testing.T) {
 					obj ctrlclient.Object,
 					opts ...ctrlclient.GetOption,
 				) error {
-					return fmt.Errorf("secret not found")
+					return fmt.Errorf("fake error")
 				},
 			},
 			want: preflight.CheckResult{
-				Allowed: false,
-				Error:   true,
+				Allowed:       false,
+				InternalError: true,
+				Causes: []preflight.Cause{
+					{
+						Message: "failed to get Registry credentials Secret: fake error",
+						//nolint:lll // this is a test for a field.
+						Field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef",
+					},
+				},
+			},
+		},
+		{
+			name:  "registry mirror with missing credentials secret",
+			field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror",
+			registryMirror: &carenv1.GlobalImageRegistryMirror{
+				URL: testRegistryURL,
+				Credentials: &carenv1.RegistryCredentials{
+					SecretRef: &carenv1.LocalObjectReference{
+						Name: "test-secret",
+					},
+				},
+			},
+			kclient: &mockK8sClient{
+				getSecretFunc: func(ctx context.Context,
+					key types.NamespacedName,
+					obj ctrlclient.Object,
+					opts ...ctrlclient.GetOption,
+				) error {
+					return apierrors.NewNotFound(corev1.Resource("secrets"), "test-secret")
+				},
+			},
+			want: preflight.CheckResult{
+				Allowed:       false,
+				InternalError: false,
 				Causes: []preflight.Cause{
 					{
-						Message: "failed to get Registry credentials Secret: secret not found",
+						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",
 					},
@@ -199,6 +232,49 @@ func TestRegistryCheck(t *testing.T) {
 				Allowed: true,
 			},
 		},
+		{
+			name:  "image registry with invalid URL",
+			field: "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0]",
+			imageRegistry: &carenv1.ImageRegistry{
+				URL: "invalid-url",
+				Credentials: &carenv1.RegistryCredentials{
+					SecretRef: &carenv1.LocalObjectReference{
+						Name: "test-secret",
+					},
+				},
+			},
+			kclient: &mockK8sClient{
+				getSecretFunc: func(ctx context.Context,
+					key types.NamespacedName,
+					obj ctrlclient.Object,
+					opts ...ctrlclient.GetOption,
+				) error {
+					secret := obj.(*corev1.Secret)
+					secret.Data = map[string][]byte{
+						"username": []byte("testuser"),
+						"password": []byte("testpass"),
+						"ca.crt":   []byte("test-ca-cert"),
+					}
+					return nil
+				},
+			},
+			mockRegClientPingerFactory: func(...regclient.Opt) regClientPinger {
+				return &mockRegClient{
+					pingFunc: func(ref.Ref) error { return nil },
+				}
+			},
+			want: preflight.CheckResult{
+				Allowed:       false,
+				InternalError: false,
+				Causes: []preflight.Cause{
+					{
+						Message: fmt.Sprintf("failed to parse registry url %s with error: "+
+							"parse \"invalid-url\": invalid URI for request", "invalid-url"),
+						Field: "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0].url",
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -235,7 +311,7 @@ func TestRegistryCheck(t *testing.T) {
 
 			// Verify the result
 			assert.Equal(t, tc.want.Allowed, got.Allowed, "(allowed) mismatch for test "+tc.name)
-			assert.Equal(t, tc.want.Error, got.Error, "(error) mismatch test "+tc.name)
+			assert.Equal(t, tc.want.InternalError, got.InternalError, "(error) mismatch test "+tc.name)
 			assert.Equal(t, tc.want.Causes, got.Causes, "(causes) mismatch test "+tc.name)
 		})
 	}

pkg/webhook/preflight/generic/spec.go

@@ -42,7 +42,7 @@ func newConfigurationCheck(
 	)
 	if err != nil {
 		configurationCheck.result.Allowed = false
-		configurationCheck.result.Error = true
+		configurationCheck.result.InternalError = true
 		configurationCheck.result.Causes = append(configurationCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf("Failed to unmarshal cluster variable %s: %s",

pkg/webhook/preflight/generic/spec_test.go

@@ -102,8 +102,8 @@ func TestNewConfigurationCheck(t *testing.T) {
 				},
 			},
 			expectedResult: preflight.CheckResult{
-				Allowed: false,
-				Error:   true,
+				Allowed:       false,
+				InternalError: true,
 				Causes: []preflight.Cause{
 					{
 						Message: "Failed to unmarshal cluster variable clusterConfig: failed to unmarshal json:" +

pkg/webhook/preflight/nutanix/credentials.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 
 	prismgoclient "github.com/nutanix-cloud-native/prism-go-client"
@@ -52,7 +53,6 @@ func newCredentialsCheck(
 	// However, the credentials configuration is missing, so we cannot perform the check.
 	if cd.nutanixClusterConfigSpec == nil || cd.nutanixClusterConfigSpec.Nutanix == nil {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: "Nutanix cluster configuration is not defined in the cluster spec",
@@ -69,7 +69,6 @@ func newCredentialsCheck(
 	if err != nil {
 		// Should not happen if the cluster passed CEL validation rules.
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf("failed to parse Prism Central endpoint URL: %s", err),
@@ -88,25 +87,39 @@ func newCredentialsCheck(
 		},
 		credentialsSecret,
 	)
+	if apierrors.IsNotFound(err) {
+		credentialsCheck.result.Allowed = false
+		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),
+				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
+			},
+		)
+		return credentialsCheck
+	}
 	if err != nil {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
+		credentialsCheck.result.InternalError = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
-				Message: fmt.Sprintf("failed to get Prism Central credentials Secret: %s", err),
-				Field:   "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
+				Message: fmt.Sprintf("Failed to get Prism Central credentials Secret %q: %s",
+					prismCentralEndpointSpec.Credentials.SecretRef.Name,
+					err,
+				),
+				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
 			},
 		)
 		return credentialsCheck
 	}
 
 	if len(credentialsSecret.Data) == 0 {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf(
-					"credentials Secret '%s' is empty",
+					"credentials Secret %q is empty",
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 				),
 				Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
@@ -118,11 +131,10 @@ func newCredentialsCheck(
 	data, ok := credentialsSecret.Data[credentialsSecretDataKey]
 	if !ok {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf(
-					"credentials Secret '%s' does not contain key '%s'",
+					"credentials Secret %q does not contain key %q",
 					prismCentralEndpointSpec.Credentials.SecretRef.Name,
 					credentialsSecretDataKey,
 				),
@@ -135,7 +147,6 @@ func newCredentialsCheck(
 	usernamePassword, err := prismcredentials.ParseCredentials(data)
 	if err != nil {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf("failed to parse Prism Central credentials: %s", err),
@@ -158,7 +169,7 @@ func newCredentialsCheck(
 	nclient, err := nclientFactory(credentials)
 	if err != nil {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
+		credentialsCheck.result.InternalError = true
 		credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
 			preflight.Cause{
 				Message: fmt.Sprintf("Failed to initialize Nutanix client: %s", err),
@@ -172,7 +183,7 @@ func newCredentialsCheck(
 	_, err = nclient.GetCurrentLoggedInUser(ctx)
 	if err != nil {
 		credentialsCheck.result.Allowed = false
-		credentialsCheck.result.Error = true
+		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. "+