Merge pull request #1809 from BenTheElder/private-providers

unexport concrete provider implementation types, fix TODO

pkg/cluster/internal/create/actions/action.go

@@ -24,7 +24,7 @@ import (
 	"sigs.k8s.io/kind/pkg/internal/cli"
 	"sigs.k8s.io/kind/pkg/log"
 
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers"
 )
 
 // Action defines a step of bringing up a kind cluster after initial node
@@ -38,15 +38,15 @@ type ActionContext struct {
 	Logger   log.Logger
 	Status   *cli.Status
 	Config   *config.Cluster
-	Provider provider.Provider
+	Provider providers.Provider
 	cache    *cachedData
 }
 
 // NewActionContext returns a new ActionContext
 func NewActionContext(
 	logger log.Logger,
 	status *cli.Status,
-	provider provider.Provider,
+	provider providers.Provider,
 	cfg *config.Cluster,
 ) *ActionContext {
 	return &ActionContext{

pkg/cluster/internal/create/actions/config/config.go

@@ -19,6 +19,7 @@ package config
 
 import (
 	"bytes"
+	"fmt"
 	"strings"
 
 	"sigs.k8s.io/kind/pkg/cluster/constants"
@@ -28,8 +29,7 @@ import (
 	"sigs.k8s.io/kind/pkg/cluster/internal/create/actions"
 	"sigs.k8s.io/kind/pkg/cluster/internal/kubeadm"
 	"sigs.k8s.io/kind/pkg/cluster/internal/patch"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/podman"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/cluster/nodeutils"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 )
@@ -60,16 +60,8 @@ func (a *Action) Execute(ctx *actions.ActionContext) error {
 	// create kubeadm init config
 	fns := []func() error{}
 
-	// TODO(bentheelder): settle on a better abstraction for this
-	// While we don't want much code aware of the provider name,
-	// we do want to surface it in the node providerID field
-	providerName := "docker"
-	if _, ok := ctx.Provider.(*podman.Provider); ok {
-		providerName = "podman"
-	}
-
 	configData := kubeadm.ConfigData{
-		NodeProvider:         providerName,
+		NodeProvider:         fmt.Sprintf("%s", ctx.Provider),
 		ClusterName:          ctx.Config.Name,
 		ControlPlaneEndpoint: controlPlaneEndpoint,
 		APIBindPort:          common.APIServerInternalPort,

pkg/cluster/internal/create/actions/loadbalancer/loadbalancer.go

@@ -26,7 +26,7 @@ import (
 
 	"sigs.k8s.io/kind/pkg/cluster/internal/create/actions"
 	"sigs.k8s.io/kind/pkg/cluster/internal/loadbalancer"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/cluster/nodeutils"
 )
 

pkg/cluster/internal/create/create.go

@@ -25,7 +25,7 @@ import (
 	"github.com/alessio/shellescape"
 
 	"sigs.k8s.io/kind/pkg/cluster/internal/delete"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers"
 	"sigs.k8s.io/kind/pkg/errors"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 	"sigs.k8s.io/kind/pkg/internal/apis/config/encoding"
@@ -72,7 +72,7 @@ type ClusterOptions struct {
 }
 
 // Cluster creates a cluster
-func Cluster(logger log.Logger, p provider.Provider, opts *ClusterOptions) error {
+func Cluster(logger log.Logger, p providers.Provider, opts *ClusterOptions) error {
 	// default / process options (namely config)
 	if err := fixupOptions(opts); err != nil {
 		return err
@@ -173,7 +173,7 @@ func Cluster(logger log.Logger, p provider.Provider, opts *ClusterOptions) error
 
 // alreadyExists returns an error if the cluster name already exists
 // or if we had an error checking
-func alreadyExists(p provider.Provider, name string) error {
+func alreadyExists(p providers.Provider, name string) error {
 	n, err := p.ListNodes(name)
 	if err != nil {
 		return err

pkg/cluster/internal/delete/delete.go

@@ -21,13 +21,13 @@ import (
 	"sigs.k8s.io/kind/pkg/log"
 
 	"sigs.k8s.io/kind/pkg/cluster/internal/kubeconfig"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers"
 )
 
 // Cluster deletes the cluster identified by ctx
 // explicitKubeconfigPath is --kubeconfig, following the rules from
 // https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands
-func Cluster(logger log.Logger, p provider.Provider, name, explicitKubeconfigPath string) error {
+func Cluster(logger log.Logger, p providers.Provider, name, explicitKubeconfigPath string) error {
 	n, err := p.ListNodes(name)
 	if err != nil {
 		return errors.Wrap(err, "error listing nodes")

pkg/cluster/internal/kubeconfig/kubeconfig.go

@@ -27,12 +27,12 @@ import (
 	// this package has slightly more generic kubeconfig helpers
 	// and minimal dependencies on the rest of kind
 	"sigs.k8s.io/kind/pkg/cluster/internal/kubeconfig/internal/kubeconfig"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers"
 )
 
 // Export exports the kubeconfig given the cluster context and a path to write it to
 // This will always be an external kubeconfig
-func Export(p provider.Provider, name, explicitPath string) error {
+func Export(p providers.Provider, name, explicitPath string) error {
 	cfg, err := get(p, name, true)
 	if err != nil {
 		return err
@@ -50,7 +50,7 @@ func Remove(clusterName, explicitPath string) error {
 
 // Get returns the kubeconfig for the cluster
 // external controls if the internal IP address is used or the host endpoint
-func Get(p provider.Provider, name string, external bool) (string, error) {
+func Get(p providers.Provider, name string, external bool) (string, error) {
 	cfg, err := get(p, name, external)
 	if err != nil {
 		return "", err
@@ -68,7 +68,7 @@ func ContextForCluster(kindClusterName string) string {
 	return kubeconfig.KINDClusterKey(kindClusterName)
 }
 
-func get(p provider.Provider, name string, external bool) (*kubeconfig.Config, error) {
+func get(p providers.Provider, name string, external bool) (*kubeconfig.Config, error) {
 	// find a control plane node to get the kubeadm config from
 	n, err := p.ListNodes(name)
 	if err != nil {

pkg/cluster/internal/providers/docker/images.go

@@ -25,7 +25,7 @@ import (
 	"sigs.k8s.io/kind/pkg/exec"
 	"sigs.k8s.io/kind/pkg/log"
 
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 	"sigs.k8s.io/kind/pkg/internal/cli"
 )

pkg/cluster/internal/providers/docker/provider.go

@@ -31,28 +31,35 @@ import (
 	"sigs.k8s.io/kind/pkg/log"
 
 	internallogs "sigs.k8s.io/kind/pkg/cluster/internal/logs"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/cluster/nodeutils"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 	"sigs.k8s.io/kind/pkg/internal/cli"
 )
 
 // NewProvider returns a new provider based on executing `docker ...`
-func NewProvider(logger log.Logger) provider.Provider {
-	return &Provider{
+func NewProvider(logger log.Logger) providers.Provider {
+	return &provider{
 		logger: logger,
 	}
 }
 
 // Provider implements provider.Provider
 // see NewProvider
-type Provider struct {
+type provider struct {
 	logger log.Logger
 }
 
+// String implements fmt.Stringer
+// NOTE: the value of this should not currently be relied upon for anything!
+// This is only used for setting the Node's providerID
+func (p *provider) String() string {
+	return "docker"
+}
+
 // Provision is part of the providers.Provider interface
-func (p *Provider) Provision(status *cli.Status, cfg *config.Cluster) (err error) {
+func (p *provider) Provision(status *cli.Status, cfg *config.Cluster) (err error) {
 	// TODO: validate cfg
 	// ensure node images are pulled before actually provisioning
 	if err := ensureNodeImages(p.logger, status, cfg); err != nil {
@@ -86,7 +93,7 @@ func (p *Provider) Provision(status *cli.Status, cfg *config.Cluster) (err error
 }
 
 // ListClusters is part of the providers.Provider interface
-func (p *Provider) ListClusters() ([]string, error) {
+func (p *provider) ListClusters() ([]string, error) {
 	cmd := exec.Command("docker",
 		"ps",
 		"-a", // show stopped nodes
@@ -103,7 +110,7 @@ func (p *Provider) ListClusters() ([]string, error) {
 }
 
 // ListNodes is part of the providers.Provider interface
-func (p *Provider) ListNodes(cluster string) ([]nodes.Node, error) {
+func (p *provider) ListNodes(cluster string) ([]nodes.Node, error) {
 	cmd := exec.Command("docker",
 		"ps",
 		"-a", // show stopped nodes
@@ -125,7 +132,7 @@ func (p *Provider) ListNodes(cluster string) ([]nodes.Node, error) {
 }
 
 // DeleteNodes is part of the providers.Provider interface
-func (p *Provider) DeleteNodes(n []nodes.Node) error {
+func (p *provider) DeleteNodes(n []nodes.Node) error {
 	if len(n) == 0 {
 		return nil
 	}
@@ -146,7 +153,7 @@ func (p *Provider) DeleteNodes(n []nodes.Node) error {
 }
 
 // GetAPIServerEndpoint is part of the providers.Provider interface
-func (p *Provider) GetAPIServerEndpoint(cluster string) (string, error) {
+func (p *provider) GetAPIServerEndpoint(cluster string) (string, error) {
 	// locate the node that hosts this
 	allNodes, err := p.ListNodes(cluster)
 	if err != nil {
@@ -204,7 +211,7 @@ func (p *Provider) GetAPIServerEndpoint(cluster string) (string, error) {
 }
 
 // GetAPIServerInternalEndpoint is part of the providers.Provider interface
-func (p *Provider) GetAPIServerInternalEndpoint(cluster string) (string, error) {
+func (p *provider) GetAPIServerInternalEndpoint(cluster string) (string, error) {
 	// locate the node that hosts this
 	allNodes, err := p.ListNodes(cluster)
 	if err != nil {
@@ -219,14 +226,14 @@ func (p *Provider) GetAPIServerInternalEndpoint(cluster string) (string, error)
 }
 
 // node returns a new node handle for this provider
-func (p *Provider) node(name string) nodes.Node {
+func (p *provider) node(name string) nodes.Node {
 	return &node{
 		name: name,
 	}
 }
 
 // CollectLogs will populate dir with cluster logs and other debug files
-func (p *Provider) CollectLogs(dir string, nodes []nodes.Node) error {
+func (p *provider) CollectLogs(dir string, nodes []nodes.Node) error {
 	execToPathFn := func(cmd exec.Cmd, path string) func() error {
 		return func() error {
 			f, err := common.FileOnHost(path)

pkg/cluster/internal/providers/docker/provision.go

@@ -28,7 +28,7 @@ import (
 	"sigs.k8s.io/kind/pkg/fs"
 
 	"sigs.k8s.io/kind/pkg/cluster/internal/loadbalancer"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 )
 

pkg/cluster/internal/providers/podman/images.go

@@ -25,7 +25,7 @@ import (
 	"sigs.k8s.io/kind/pkg/exec"
 	"sigs.k8s.io/kind/pkg/log"
 
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 	"sigs.k8s.io/kind/pkg/internal/cli"
 )

pkg/cluster/internal/providers/podman/provider.go

@@ -34,28 +34,35 @@ import (
 	"sigs.k8s.io/kind/pkg/log"
 
 	internallogs "sigs.k8s.io/kind/pkg/cluster/internal/logs"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 	"sigs.k8s.io/kind/pkg/internal/cli"
 )
 
 // NewProvider returns a new provider based on executing `podman ...`
-func NewProvider(logger log.Logger) provider.Provider {
+func NewProvider(logger log.Logger) providers.Provider {
 	logger.Warn("enabling experimental podman provider")
-	return &Provider{
+	return &provider{
 		logger: logger,
 	}
 }
 
 // Provider implements provider.Provider
 // see NewProvider
-type Provider struct {
+type provider struct {
 	logger log.Logger
 }
 
+// String implements fmt.Stringer
+// NOTE: the value of this should not currently be relied upon for anything!
+// This is only used for setting the Node's providerID
+func (p *provider) String() string {
+	return "podman"
+}
+
 // Provision is part of the providers.Provider interface
-func (p *Provider) Provision(status *cli.Status, cfg *config.Cluster) (err error) {
+func (p *provider) Provision(status *cli.Status, cfg *config.Cluster) (err error) {
 	if err := ensureMinVersion(); err != nil {
 		return err
 	}
@@ -88,7 +95,7 @@ func (p *Provider) Provision(status *cli.Status, cfg *config.Cluster) (err error
 }
 
 // ListClusters is part of the providers.Provider interface
-func (p *Provider) ListClusters() ([]string, error) {
+func (p *provider) ListClusters() ([]string, error) {
 	cmd := exec.Command("podman",
 		"ps",
 		"-a", // show stopped nodes
@@ -105,7 +112,7 @@ func (p *Provider) ListClusters() ([]string, error) {
 }
 
 // ListNodes is part of the providers.Provider interface
-func (p *Provider) ListNodes(cluster string) ([]nodes.Node, error) {
+func (p *provider) ListNodes(cluster string) ([]nodes.Node, error) {
 	cmd := exec.Command("podman",
 		"ps",
 		"-a", // show stopped nodes
@@ -127,7 +134,7 @@ func (p *Provider) ListNodes(cluster string) ([]nodes.Node, error) {
 }
 
 // DeleteNodes is part of the providers.Provider interface
-func (p *Provider) DeleteNodes(n []nodes.Node) error {
+func (p *provider) DeleteNodes(n []nodes.Node) error {
 	if len(n) == 0 {
 		return nil
 	}
@@ -156,7 +163,7 @@ func (p *Provider) DeleteNodes(n []nodes.Node) error {
 }
 
 // GetAPIServerEndpoint is part of the providers.Provider interface
-func (p *Provider) GetAPIServerEndpoint(cluster string) (string, error) {
+func (p *provider) GetAPIServerEndpoint(cluster string) (string, error) {
 	// locate the node that hosts this
 	allNodes, err := p.ListNodes(cluster)
 	if err != nil {
@@ -230,7 +237,7 @@ func (p *Provider) GetAPIServerEndpoint(cluster string) (string, error) {
 }
 
 // GetAPIServerInternalEndpoint is part of the providers.Provider interface
-func (p *Provider) GetAPIServerInternalEndpoint(cluster string) (string, error) {
+func (p *provider) GetAPIServerInternalEndpoint(cluster string) (string, error) {
 	// locate the node that hosts this
 	allNodes, err := p.ListNodes(cluster)
 	if err != nil {
@@ -251,14 +258,14 @@ func (p *Provider) GetAPIServerInternalEndpoint(cluster string) (string, error)
 }
 
 // node returns a new node handle for this provider
-func (p *Provider) node(name string) nodes.Node {
+func (p *provider) node(name string) nodes.Node {
 	return &node{
 		name: name,
 	}
 }
 
 // CollectLogs will populate dir with cluster logs and other debug files
-func (p *Provider) CollectLogs(dir string, nodes []nodes.Node) error {
+func (p *provider) CollectLogs(dir string, nodes []nodes.Node) error {
 	execToPathFn := func(cmd exec.Cmd, path string) func() error {
 		return func() error {
 			f, err := common.FileOnHost(path)

pkg/cluster/internal/providers/podman/provision.go

@@ -27,7 +27,7 @@ import (
 	"sigs.k8s.io/kind/pkg/exec"
 
 	"sigs.k8s.io/kind/pkg/cluster/internal/loadbalancer"
-	"sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common"
+	"sigs.k8s.io/kind/pkg/cluster/internal/providers/common"
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 )