Adjust snap.Interface to return Kubernetes clients (#384)

* move pkg/utils/k8s -> pkg/client/kubernetes

* adjust snap to return Kubernetes clients instead of config flags

* Move helm client logic to pkg/client/helm package

* adjust snap interface to return a HelmClient

* add unit test for features.ApplyMetricsServer

src/k8s/pkg/client/helm/chart.go

@@ -0,0 +1,14 @@
+package helm
+
+// InstallableChart describes a chart that can be deployed on a running cluster.
+type InstallableChart struct {
+	// Name is the install name of the chart.
+	Name string
+
+	// Namespace is the namespace to install the chart.
+	Namespace string
+
+	// ManifestPath is the path to the chart's manifest, typically relative to "$SNAP/k8s/manifests".
+	// TODO(neoaggelos): this should be a *chart.Chart, and we should use the "embed" package to load it when building k8sd.
+	ManifestPath string
+}

src/k8s/pkg/k8sd/features/manager.go → src/k8s/pkg/client/helm/client.go

@@ -1,4 +1,4 @@
-package features
+package helm
 
 import (
 	"bytes"
@@ -8,31 +8,30 @@ import (
 	"log"
 	"path"
 
-	"github.com/canonical/k8s/pkg/snap"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	"helm.sh/helm/v3/pkg/storage/driver"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 )
 
-// helmManager implements Manager using Helm.
-type helmManager struct {
+// client implements Client using Helm.
+type client struct {
 	restClientGetter func(string) genericclioptions.RESTClientGetter
 	manifestsBaseDir string
 }
 
-// ensure *helmManager implements Manager.
-var _ Manager = &helmManager{}
+// ensure *client implements Client.
+var _ Client = &client{}
 
-// newHelm creates a new helmManager.
-func newHelm(snap snap.Snap) *helmManager {
-	return &helmManager{
-		restClientGetter: snap.KubernetesRESTClientGetter,
-		manifestsBaseDir: snap.ManifestsDir(),
+// NewClient creates a new client.
+func NewClient(manifestsBaseDir string, restClientGetter func(string) genericclioptions.RESTClientGetter) *client {
+	return &client{
+		restClientGetter: restClientGetter,
+		manifestsBaseDir: manifestsBaseDir,
 	}
 }
 
-func (h *helmManager) newActionConfiguration(namespace string) (*action.Configuration, error) {
+func (h *client) newActionConfiguration(namespace string) (*action.Configuration, error) {
 	actionConfig := new(action.Configuration)
 
 	if err := actionConfig.Init(h.restClientGetter(namespace), namespace, "", log.Printf); err != nil {
@@ -41,9 +40,9 @@ func (h *helmManager) newActionConfiguration(namespace string) (*action.Configur
 	return actionConfig, nil
 }
 
-// Apply implements the Manager interface.
-func (h *helmManager) Apply(ctx context.Context, f Feature, desired state, values map[string]any) (bool, error) {
-	cfg, err := h.newActionConfiguration(f.namespace)
+// Apply implements the Client interface.
+func (h *client) Apply(ctx context.Context, c InstallableChart, desired State, values map[string]any) (bool, error) {
+	cfg, err := h.newActionConfiguration(c.Namespace)
 	if err != nil {
 		return false, fmt.Errorf("failed to create action configuration: %w", err)
 	}
@@ -53,10 +52,10 @@ func (h *helmManager) Apply(ctx context.Context, f Feature, desired state, value
 
 	// get the latest Helm release with the specified name
 	get := action.NewGet(cfg)
-	release, err := get.Run(f.name)
+	release, err := get.Run(c.Name)
 	if err != nil {
 		if err != driver.ErrReleaseNotFound {
-			return false, fmt.Errorf("failed to get status of release %s: %w", f.name, err)
+			return false, fmt.Errorf("failed to get status of release %s: %w", c.Name, err)
 		}
 		isInstalled = false
 	} else {
@@ -65,50 +64,50 @@ func (h *helmManager) Apply(ctx context.Context, f Feature, desired state, value
 	}
 
 	switch {
-	case !isInstalled && desired == stateDeleted:
+	case !isInstalled && desired == StateDeleted:
 		// no-op
 		return false, nil
-	case !isInstalled && desired == stateUpgradeOnly:
+	case !isInstalled && desired == StateUpgradeOnly:
 		// there is no release installed, this is an error
-		return false, fmt.Errorf("cannot upgrade %s as it is not installed", f.name)
-	case !isInstalled && desired == statePresent:
+		return false, fmt.Errorf("cannot upgrade %s as it is not installed", c.Name)
+	case !isInstalled && desired == StatePresent:
 		// there is no release installed, so we must run an install action
 		install := action.NewInstall(cfg)
-		install.ReleaseName = f.name
-		install.Namespace = f.namespace
+		install.ReleaseName = c.Name
+		install.Namespace = c.Namespace
 
-		chart, err := loader.Load(path.Join(h.manifestsBaseDir, f.manifestPath))
+		chart, err := loader.Load(path.Join(h.manifestsBaseDir, c.ManifestPath))
 		if err != nil {
-			return false, fmt.Errorf("failed to load manifest for %s: %w", f.name, err)
+			return false, fmt.Errorf("failed to load manifest for %s: %w", c.Name, err)
 		}
 
 		if _, err := install.RunWithContext(ctx, chart, values); err != nil {
-			return false, fmt.Errorf("failed to install %s: %w", f.name, err)
+			return false, fmt.Errorf("failed to install %s: %w", c.Name, err)
 		}
 		return true, nil
-	case isInstalled && desired != stateDeleted:
-		// there is already a release installed, so we must run an install action
+	case isInstalled && desired != StateDeleted:
+		// there is already a release installed, so we must run an upgrade action
 		upgrade := action.NewUpgrade(cfg)
-		upgrade.Namespace = f.namespace
+		upgrade.Namespace = c.Namespace
 		upgrade.ReuseValues = true
 
-		chart, err := loader.Load(path.Join(h.manifestsBaseDir, f.manifestPath))
+		chart, err := loader.Load(path.Join(h.manifestsBaseDir, c.ManifestPath))
 		if err != nil {
-			return false, fmt.Errorf("failed to load manifest for %s: %w", f.name, err)
+			return false, fmt.Errorf("failed to load manifest for %s: %w", c.Name, err)
 		}
 
-		release, err := upgrade.RunWithContext(ctx, f.name, chart, values)
+		release, err := upgrade.RunWithContext(ctx, c.Name, chart, values)
 		if err != nil {
-			return false, fmt.Errorf("failed to upgrade %s: %w", f.name, err)
+			return false, fmt.Errorf("failed to upgrade %s: %w", c.Name, err)
 		}
 
 		// oldConfig and release.Config are the previous and current values. they are compared by checking their respective JSON, as that is good enough for our needs of comparing unstructured map[string]any data.
 		return !jsonEqual(oldConfig, release.Config), nil
-	case isInstalled && desired == stateDeleted:
+	case isInstalled && desired == StateDeleted:
 		// run an uninstall action
 		uninstall := action.NewUninstall(cfg)
-		if _, err := uninstall.Run(f.name); err != nil {
-			return false, fmt.Errorf("failed to uninstall %s: %w", f.name, err)
+		if _, err := uninstall.Run(c.Name); err != nil {
+			return false, fmt.Errorf("failed to uninstall %s: %w", c.Name, err)
 		}
 
 		return true, nil

src/k8s/pkg/client/helm/interface.go

@@ -0,0 +1,13 @@
+package helm
+
+import "context"
+
+// Client handles the lifecycle of charts (manifests + config) on the cluster.
+type Client interface {
+	// Apply ensures the state of a InstallableChart on the cluster.
+	// When state is StatePresent, Apply will install or upgrade the chart using the specified values as configuration. Apply returns true if the chart was not installed, or any values were changed.
+	// When state is StateUpgradeOnly, Apply will upgrade the chart using the specified values as configuration. Apply returns true if the chart was not installed, or any values were changed. An error is returned if the chart is not already installed.
+	// When state is StateDeleted, Apply will ensure that the chart is removed. If the chart is not installed, this is a no-op. Apply returns true if the chart was previously installed.
+	// Apply returns an error in case of failure.
+	Apply(ctx context.Context, f InstallableChart, desired State, values map[string]any) (bool, error)
+}

src/k8s/pkg/client/helm/mock/mock.go

@@ -0,0 +1,29 @@
+package mock
+
+import (
+	"context"
+
+	"github.com/canonical/k8s/pkg/client/helm"
+)
+
+type MockApplyArguments struct {
+	Context context.Context
+	Chart   helm.InstallableChart
+	State   helm.State
+	Values  map[string]any
+}
+
+// Mock is a mock implementation of helm.Client
+type Mock struct {
+	ApplyCalledWith []MockApplyArguments
+	ApplyChanged    bool
+	ApplyErr        error
+}
+
+// Apply implements helm.Client
+func (m *Mock) Apply(ctx context.Context, c helm.InstallableChart, desired helm.State, values map[string]any) (bool, error) {
+	m.ApplyCalledWith = append(m.ApplyCalledWith, MockApplyArguments{Context: ctx, Chart: c, State: desired, Values: values})
+	return m.ApplyChanged, m.ApplyErr
+}
+
+var _ helm.Client = &Mock{}

src/k8s/pkg/client/helm/state.go

@@ -0,0 +1,29 @@
+package helm
+
+// State is used to define how Client.Apply() handles install, upgrade or delete operations.
+type State int
+
+const (
+	// StateDeleted means that the chart should not be installed.
+	StateDeleted State = iota
+
+	// StatePresent means that the chart must be present. If it already exists, it is upgraded with the new configuration, otherwise it is installed.
+	StatePresent
+
+	// StateUpgradeOnly means that the chart will be refreshed if installed, fail otherwise.
+	StateUpgradeOnly
+)
+
+func StatePresentOrDeleted(enabled bool) State {
+	if enabled {
+		return StatePresent
+	}
+	return StateDeleted
+}
+
+func StateUpgradeOnlyOrDeleted(enabled bool) State {
+	if enabled {
+		return StateUpgradeOnly
+	}
+	return StateDeleted
+}

src/k8s/pkg/utils/k8s/client.go → src/k8s/pkg/client/kubernetes/client.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"fmt"

src/k8s/pkg/utils/k8s/configmap.go → src/k8s/pkg/client/kubernetes/configmap.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/configmap_test.go → src/k8s/pkg/client/kubernetes/configmap_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/endpoints.go → src/k8s/pkg/client/kubernetes/endpoints.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/endpoints_test.go → src/k8s/pkg/client/kubernetes/endpoints_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/node.go → src/k8s/pkg/client/kubernetes/node.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/node_test.go → src/k8s/pkg/client/kubernetes/node_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/restart_daemonset.go → src/k8s/pkg/client/kubernetes/restart_daemonset.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/restart_daemonset_test.go → src/k8s/pkg/client/kubernetes/restart_daemonset_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/restart_deployment.go → src/k8s/pkg/client/kubernetes/restart_deployment.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/restart_deployment_test.go → src/k8s/pkg/client/kubernetes/restart_deployment_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/server_groups.go → src/k8s/pkg/client/kubernetes/server_groups.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"fmt"

src/k8s/pkg/utils/k8s/server_groups_test.go → src/k8s/pkg/client/kubernetes/server_groups_test.go

@@ -1,12 +1,12 @@
-package k8s_test
+package kubernetes_test
 
 import (
 	"testing"
 
 	fakediscovery "k8s.io/client-go/discovery/fake"
 	fakeclientset "k8s.io/client-go/kubernetes/fake"
 
-	"github.com/canonical/k8s/pkg/utils/k8s"
+	"github.com/canonical/k8s/pkg/client/kubernetes"
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -49,7 +49,7 @@ func TestListResourcesForGroupVersion(t *testing.T) {
 			}
 
 			// Create a new k8s client with the fake discovery client
-			client := &k8s.Client{
+			client := &kubernetes.Client{
 				Interface: clientset,
 			}
 

src/k8s/pkg/utils/k8s/services.go → src/k8s/pkg/client/kubernetes/services.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/services_test.go → src/k8s/pkg/client/kubernetes/services_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/status.go → src/k8s/pkg/client/kubernetes/status.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/utils/k8s/status_test.go → src/k8s/pkg/client/kubernetes/status_test.go

@@ -1,4 +1,4 @@
-package k8s
+package kubernetes
 
 import (
 	"context"

src/k8s/pkg/k8sd/api/cluster.go

@@ -2,12 +2,11 @@ package api
 
 import (
 	"fmt"
-	databaseutil "github.com/canonical/k8s/pkg/k8sd/database/util"
 	"net/http"
 
 	apiv1 "github.com/canonical/k8s/api/v1"
 	"github.com/canonical/k8s/pkg/k8sd/api/impl"
-	"github.com/canonical/k8s/pkg/utils/k8s"
+	databaseutil "github.com/canonical/k8s/pkg/k8sd/database/util"
 	"github.com/canonical/lxd/lxd/response"
 	"github.com/canonical/microcluster/state"
 )
@@ -27,8 +26,7 @@ func (e *Endpoints) getClusterStatus(s *state.State, r *http.Request) response.R
 		return response.InternalError(fmt.Errorf("failed to get cluster config: %w", err))
 	}
 
-	snap := e.provider.Snap()
-	client, err := k8s.NewClient(snap.KubernetesRESTClientGetter(""))
+	client, err := e.provider.Snap().KubernetesClient("")
 	if err != nil {
 		return response.InternalError(fmt.Errorf("failed to create k8s client: %w", err))
 	}

src/k8s/pkg/k8sd/api/cluster_remove.go

@@ -7,7 +7,6 @@ import (
 	apiv1 "github.com/canonical/k8s/api/v1"
 	databaseutil "github.com/canonical/k8s/pkg/k8sd/database/util"
 	"github.com/canonical/k8s/pkg/utils"
-	"github.com/canonical/k8s/pkg/utils/k8s"
 	nodeutil "github.com/canonical/k8s/pkg/utils/node"
 	"github.com/canonical/lxd/lxd/response"
 	"github.com/canonical/microcluster/state"
@@ -43,7 +42,7 @@ func (e *Endpoints) postClusterRemove(s *state.State, r *http.Request) response.
 	}
 	if isWorker {
 		// For worker nodes, we need to manually clean up the kubernetes node and db entry.
-		c, err := k8s.NewClient(snap.KubernetesRESTClientGetter(""))
+		c, err := snap.KubernetesClient("")
 		if err != nil {
 			return response.InternalError(fmt.Errorf("failed to create k8s client: %w", err))
 		}