refactor based on the review to move the implementation at the deployer instead of too much pre-validation at the cli

Signed-off-by: RayyanSeliya <[email protected]>

Author: RayyanSeliya

cmd/deploy.go

@@ -14,7 +14,6 @@ import (
 	"github.com/ory/viper"
 	"github.com/spf13/cobra"
 	"k8s.io/apimachinery/pkg/api/resource"
-	"k8s.io/client-go/tools/clientcmd"
 	"knative.dev/client/pkg/util"
 
 	"knative.dev/func/pkg/builders"
@@ -289,57 +288,6 @@ Try this:
 For more options, run 'func deploy --help'`, fn.ErrClusterNotAccessible)
 }
 
-// validateClusterConnection checks if the Kubernetes cluster is accessible before starting build
-func validateClusterConnection() error {
-	// Try to get cluster configuration
-	restConfig, err := k8s.GetClientConfig().ClientConfig()
-	if err != nil {
-		kubeconfigPath := os.Getenv("KUBECONFIG")
-
-		// Check if this is an empty/missing config error
-		if clientcmd.IsEmptyConfig(err) {
-			// If KUBECONFIG is explicitly set, check if the file exists
-			if kubeconfigPath != "" {
-				if _, statErr := os.Stat(kubeconfigPath); os.IsNotExist(statErr) {
-					// File doesn't exist - return invalid kubeconfig error for real usage
-					// but skip for test paths (tests may have stale KUBECONFIG paths)
-					if !strings.Contains(kubeconfigPath, "/testdata/") &&
-						!strings.Contains(kubeconfigPath, "\\testdata\\") {
-						return fmt.Errorf("%w: %v", fn.ErrInvalidKubeconfig, err)
-					}
-					// Test path - skip validation
-					return nil
-				}
-			}
-			return fmt.Errorf("%w: %v", fn.ErrClusterNotAccessible, err)
-		}
-		return fmt.Errorf("%w: %v", fn.ErrClusterNotAccessible, err)
-	}
-
-	// Skip connectivity check for non-production clusters (example, test, localhost)
-	host := restConfig.Host
-	if strings.Contains(host, ".example.com") ||
-		strings.Contains(host, "example.com:") ||
-		strings.Contains(host, "localhost") ||
-		strings.Contains(host, "127.0.0.1") {
-		return nil
-	}
-
-	// Create Kubernetes client to test connectivity
-	client, err := k8s.NewKubernetesClientset()
-	if err != nil {
-		return fmt.Errorf("%w: %v", fn.ErrClusterNotAccessible, err)
-	}
-
-	// Verify cluster is actually reachable with an API call
-	_, err = client.Discovery().ServerVersion()
-	if err != nil {
-		return fmt.Errorf("%w: %v", fn.ErrClusterNotAccessible, err)
-	}
-
-	return nil
-}
-
 func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 	var (
 		cfg deployConfig
@@ -419,17 +367,6 @@ For more options, run 'func deploy --help'`, err)
 	}
 	cmd.SetContext(cfg.WithValues(cmd.Context())) // Some optional settings are passed via context
 
-	// Validate cluster connection before building
-	if err = validateClusterConnection(); err != nil {
-		if errors.Is(err, fn.ErrInvalidKubeconfig) {
-			return wrapInvalidKubeconfigError(err)
-		}
-		if errors.Is(err, fn.ErrClusterNotAccessible) {
-			return wrapClusterNotAccessibleError(err)
-		}
-		return err
-	}
-
 	changingNamespace := func(f fn.Function) bool {
 		// We're changing namespace if:
 		return f.Deploy.Namespace != "" && // it's already deployed
@@ -469,6 +406,12 @@ For more options, run 'func deploy --help'`, err)
 		// Returned is the function with fields like Registry, f.Deploy.Image &
 		// f.Deploy.Namespace populated.
 		if url, f, err = client.RunPipeline(cmd.Context(), f); err != nil {
+			if errors.Is(err, fn.ErrInvalidKubeconfig) {
+				return wrapInvalidKubeconfigError(err)
+			}
+			if errors.Is(err, fn.ErrClusterNotAccessible) {
+				return wrapClusterNotAccessibleError(err)
+			}
 			return
 		}
 		fmt.Fprintf(cmd.OutOrStdout(), "Function Deployed at %v\n", url)
@@ -520,6 +463,12 @@ For more options, run 'func deploy --help'`, err)
 			}
 		}
 		if f, err = client.Deploy(cmd.Context(), f, fn.WithDeploySkipBuildCheck(cfg.Build == "false")); err != nil {
+			if errors.Is(err, fn.ErrInvalidKubeconfig) {
+				return wrapInvalidKubeconfigError(err)
+			}
+			if errors.Is(err, fn.ErrClusterNotAccessible) {
+				return wrapClusterNotAccessibleError(err)
+			}
 			return
 		}
 	}

pkg/knative/client.go

@@ -2,13 +2,15 @@ package knative
 
 import (
 	"fmt"
+	"os"
 	"time"
 
 	clienteventingv1 "knative.dev/client/pkg/eventing/v1"
 	clientservingv1 "knative.dev/client/pkg/serving/v1"
 	eventingv1 "knative.dev/eventing/pkg/client/clientset/versioned/typed/eventing/v1"
 	servingv1 "knative.dev/serving/pkg/client/clientset/versioned/typed/serving/v1"
 
+	fn "knative.dev/func/pkg/functions"
 	"knative.dev/func/pkg/k8s"
 )
 
@@ -18,6 +20,9 @@ const (
 )
 
 func NewServingClient(namespace string) (clientservingv1.KnServingClient, error) {
+	if err := validateKubeconfigFile(); err != nil {
+		return nil, err
+	}
 
 	restConfig, err := k8s.GetClientConfig().ClientConfig()
 	if err != nil {
@@ -35,6 +40,9 @@ func NewServingClient(namespace string) (clientservingv1.KnServingClient, error)
 }
 
 func NewEventingClient(namespace string) (clienteventingv1.KnEventingClient, error) {
+	if err := validateKubeconfigFile(); err != nil {
+		return nil, err
+	}
 
 	restConfig, err := k8s.GetClientConfig().ClientConfig()
 	if err != nil {
@@ -50,3 +58,17 @@ func NewEventingClient(namespace string) (clienteventingv1.KnEventingClient, err
 
 	return client, nil
 }
+
+// validateKubeconfigFile checks if explicitly set KUBECONFIG path exists
+func validateKubeconfigFile() error {
+	kubeconfigPath := os.Getenv("KUBECONFIG")
+	if kubeconfigPath == "" {
+		return nil
+	}
+
+	if _, err := os.Stat(kubeconfigPath); os.IsNotExist(err) {
+		return fmt.Errorf("%w: kubeconfig file does not exist at path: %s", fn.ErrInvalidKubeconfig, kubeconfigPath)
+	}
+
+	return nil
+}

pkg/knative/deployer.go

@@ -157,17 +157,17 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 	// Clients
 	client, err := NewServingClient(namespace)
 	if err != nil {
-		return fn.DeploymentResult{}, err
+		return fn.DeploymentResult{}, wrapDeployerClientError(err)
 	}
 	eventingClient, err := NewEventingClient(namespace)
 	if err != nil {
-		return fn.DeploymentResult{}, err
+		return fn.DeploymentResult{}, wrapDeployerClientError(err)
 	}
 	// check if 'dapr-system' namespace exists
 	daprInstalled := false
 	k8sClient, err := k8s.NewKubernetesClientset()
 	if err != nil {
-		return fn.DeploymentResult{}, err
+		return fn.DeploymentResult{}, wrapDeployerClientError(err)
 	}
 	_, err = k8sClient.CoreV1().Namespaces().Get(ctx, "dapr-system", metav1.GetOptions{})
 	if err == nil {
@@ -187,6 +187,9 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 
 	previousService, err := client.GetService(ctx, f.Name)
 	if err != nil {
+		if wrappedErr := wrapK8sConnectionError(err); wrappedErr != nil {
+			return fn.DeploymentResult{}, wrappedErr
+		}
 		if errors.IsNotFound(err) {
 
 			referencedSecrets := sets.New[string]()
@@ -1118,3 +1121,44 @@ func setServiceOptions(template *v1.RevisionTemplateSpec, options fn.Options) er
 
 	return servingclientlib.UpdateRevisionTemplateAnnotations(template, toUpdate, toRemove)
 }
+
+// wrapDeployerClientError wraps Kubernetes client creation errors with typed errors
+func wrapDeployerClientError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	errMsg := err.Error()
+
+	// Missing kubeconfig file
+	if strings.Contains(errMsg, "kubeconfig file does not exist at path") {
+		return fmt.Errorf("%w: %v", fn.ErrInvalidKubeconfig, err)
+	}
+
+	// Empty config or cluster not accessible
+	if strings.Contains(errMsg, "no configuration has been provided") ||
+		strings.Contains(errMsg, "invalid configuration") {
+		return fmt.Errorf("%w: %v", fn.ErrClusterNotAccessible, err)
+	}
+
+	return err
+}
+
+// wrapK8sConnectionError wraps connection errors during API calls
+func wrapK8sConnectionError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	errMsg := err.Error()
+
+	// Connection errors (refused, timeout, certificate issues)
+	if strings.Contains(errMsg, "connection refused") ||
+		strings.Contains(errMsg, "dial tcp") ||
+		strings.Contains(errMsg, "i/o timeout") ||
+		strings.Contains(errMsg, "x509:") {
+		return fmt.Errorf("%w: %v", fn.ErrClusterNotAccessible, err)
+	}
+
+	return nil
+}