Handle v1alpha3 of Compose on Kubernetes API

[simonferquel]

Based on #1611

Compose on Kubernetes features under active development are introduced in API version v1alpha3, which is a superset of v1beta2, that will never break v1beta2 valid constructs, but that we consider as unstable (and breakable) until we promote it to v1beta3.

To expose those "in-development features", we need the CLI to be able to handle this API version, under the experimental flag. This PR handles that.

- What I did

• API Version checks: if under experimental flag, prefer to talk with the v1alpha3 endpoint (if it exists)
• Stack client: added implementation of kubernetes.StackClient for v1alpha3
• Internal Stack object: internally used the most complete representation of the stack known to the CLI (now v1alpha3). New features will be muted if v1alpha3 is not available on the server or if experimental flag is disabled.

- How I did it

• Vendored a newer version of Compose on Kubernetes
• Changed the API Version check logic, making sure to pass client experimental flag correctly
• Modified the internal stack and conversion functions to base them on v1alpha3

- How to verify it

• Existing unit tests have been adapted to test cases with v1alpha3
• API Version check is unit tested

- A picture of a cute animal (not mandatory but encouraged)

[codecov-io]

Codecov Report

Merging #1615 into master will increase coverage by 0.21%.
The diff coverage is 58.22%.

@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
+ Coverage   55.62%   55.84%   +0.21%     
==========================================
  Files         301      302       +1     
  Lines       20444    20543      +99     
==========================================
+ Hits        11372    11472     +100     
+ Misses       8253     8245       -8     
- Partials      819      826       +7

[simonferquel]

@silvin-lubecki PTAL (after #1611, and with #1617)

[silvin-lubecki]

Just blocking until #1611 gets merged.

[silvin-lubecki]

Otherwise LGTM

[silvin-lubecki]

@vdemeester can you take a look at it? 🙏

[simonferquel]

Just rebased.
@silvin-lubecki, @vdemeester PTAL

[silvin-lubecki]

LGTM since #1611 is merged.

[thaJeztah]

Sorry. got dragged away, so didn't complete review yet; these were the nits I found earlier, so let me post those already, and I'll try to revisit the PR later

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "handle-v1alpha3" git@github.com:simonferquel/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354512128
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[thaJeztah]

Thanks! Ok, so I'm "ok" with this change, but (erm.. well, you know me), left an idea 🤗

If it's out of scope, then "LGTM"

[thaJeztah]

Ok, so looking at this, there's some things I notice;

• This experimental only applies to the Stacks API; will there be points where we'll have to switch API versions for other clients?
• If so; would it be an option to put this bool on the Stacks() function instead?
• I don't like booleans; they're confusing, and easily lead to bugs as it's not always clear to see what they do, without looking at the function's signature, and accidentally toggling a boolean is easy.

I guess it's too late for this, but now that we're introducing more functional arguments; we could use them here as well.

Here's a 5-minute quick 'n dirty attempt;

package kubernetes

import (
	"github.com/docker/cli/kubernetes"
	"github.com/pkg/errors"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	kubeclient "k8s.io/client-go/kubernetes"
	appsv1beta2 "k8s.io/client-go/kubernetes/typed/apps/v1beta2"
	typesappsv1beta2 "k8s.io/client-go/kubernetes/typed/apps/v1beta2"
	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
	restclient "k8s.io/client-go/rest"
)

// ClientFactory creates kubernetes clients
type ClientFactory interface {
	// ConfigMaps returns a client for kubernetes's config maps
	ConfigMaps(ops ...FactoryOp) corev1.ConfigMapInterface

	// Secrets returns a client for kubernetes's secrets
	Secrets(ops ...FactoryOp) corev1.SecretInterface

	// Services returns a client for kubernetes's secrets
	Services(ops ...FactoryOp) corev1.ServiceInterface

	// Pods returns a client for kubernetes's pods
	Pods(ops ...FactoryOp) corev1.PodInterface

	// Nodes returns a client for kubernetes's nodes
	Nodes(ops ...FactoryOp) corev1.NodeInterface

	// ReplicationControllers returns a client for kubernetes replication controllers
	ReplicationControllers(ops ...FactoryOp) corev1.ReplicationControllerInterface

	// ReplicaSets returns a client for kubernetes replace sets
	ReplicaSets(ops ...FactoryOp) typesappsv1beta2.ReplicaSetInterface

	// DaemonSets returns a client for kubernetes daemon sets
	DaemonSets(ops ...FactoryOp) typesappsv1beta2.DaemonSetInterface

	// Stacks returns a client for Docker's Stack on Kubernetes
	Stacks(ops ...FactoryOp) (StackClient, error)
}

// Factory is the kubernetes client factory
type Factory struct {
	namespace       string
	config          *restclient.Config
	coreClientSet   corev1.CoreV1Interface
	appsClientSet   appsv1beta2.AppsV1beta2Interface
	clientSet       *kubeclient.Clientset
	stackAPIVersion *kubernetes.StackVersion
}

// NewFactory creates a kubernetes client factory
func NewFactory(ops ...FactoryOp) (*Factory, error) {
	f := &Factory{}

	ops = append(ops, WithDefaultStackAPI)
	for _, o := range ops {
		if err := o(f); err != nil {
			return nil, err
		}
	}

	coreClientSet, err := corev1.NewForConfig(f.config)
	if err != nil {
		return nil, err
	}

	appsClientSet, err := appsv1beta2.NewForConfig(f.config)
	if err != nil {
		return nil, err
	}

	f.coreClientSet = coreClientSet
	f.appsClientSet = appsClientSet

	return f, nil
}

// ConfigMaps returns a client for kubernetes's config maps
func (s *Factory) ConfigMaps(ops ...FactoryOp) corev1.ConfigMapInterface {
	return s.coreClientSet.ConfigMaps(s.namespace)
}

// Secrets returns a client for kubernetes's secrets
func (s *Factory) Secrets(ops ...FactoryOp) corev1.SecretInterface {
	return s.coreClientSet.Secrets(s.namespace)
}

// Services returns a client for kubernetes's secrets
func (s *Factory) Services(ops ...FactoryOp) corev1.ServiceInterface {
	return s.coreClientSet.Services(s.namespace)
}

// Pods returns a client for kubernetes's pods
func (s *Factory) Pods(ops ...FactoryOp) corev1.PodInterface {
	return s.coreClientSet.Pods(s.namespace)
}

// Nodes returns a client for kubernetes's nodes
func (s *Factory) Nodes(ops ...FactoryOp) corev1.NodeInterface {
	return s.coreClientSet.Nodes()
}

// ReplicationControllers returns a client for kubernetes replication controllers
func (s *Factory) ReplicationControllers(ops ...FactoryOp) corev1.ReplicationControllerInterface {
	return s.coreClientSet.ReplicationControllers(s.namespace)
}

// ReplicaSets returns a client for kubernetes replace sets
func (s *Factory) ReplicaSets(ops ...FactoryOp) typesappsv1beta2.ReplicaSetInterface {
	return s.appsClientSet.ReplicaSets(s.namespace)
}

// DaemonSets returns a client for kubernetes daemon sets
func (s *Factory) DaemonSets(ops ...FactoryOp) typesappsv1beta2.DaemonSetInterface {
	return s.appsClientSet.DaemonSets(s.namespace)
}

// Stacks returns a client for Docker's Stack on Kubernetes
func (s *Factory) Stacks(ops ...FactoryOp) (StackClient, error) {
	f := s

	ops = append(ops, WithDefaultStackAPI)
	for _, o := range ops {
		if err := o(f); err != nil {
			return nil, err
		}
	}

	switch *s.stackAPIVersion {
	case kubernetes.StackAPIV1Beta1:
		return newStackV1Beta1(s.config, s.namespace)
	case kubernetes.StackAPIV1Beta2:
		return newStackV1Beta2(s.config, s.namespace)
	case kubernetes.StackAPIV1Alpha3:
		return newStackV1Alpha3(s.config, s.namespace)
	default:
		return nil, errors.Errorf("unsupported stack API version: %q", s.stackAPIVersion)
	}
}

type FactoryOp func(f *Factory) error

func WithNameSpace(namespace string) FactoryOp {
	return func(f *Factory) error {
		f.namespace = namespace
		return nil
	}
}

func WithAllNamespaces(f *Factory) error {
	f.namespace = metav1.NamespaceAll
	return nil
}

func WithClientSet(clientSet *kubeclient.Clientset) FactoryOp {
	return func(f *Factory) error {
		f.clientSet = clientSet
		return nil
	}
}

func WithConfig(config *restclient.Config) FactoryOp {
	return func(f *Factory) error {
		f.config = config
		return nil
	}
}

func WithExperimentalStackAPI(f *Factory) error {
	version, err := kubernetes.GetStackAPIVersion(f.clientSet.Discovery(), true)
	if err != nil {
		return err
	}
	f.stackAPIVersion = &version
	return nil
}

func WithDefaultStackAPI(f *Factory) error {
	if f.stackAPIVersion != nil {
		return nil
	}
	version, err := kubernetes.GetStackAPIVersion(f.clientSet.Discovery(), false)
	if err != nil {
		return err
	}
	f.stackAPIVersion = &version
	return nil
}

diff (in case it's still an option, and if it helps you);

diff --git a/cli/command/stack/kubernetes/client.go b/cli/command/stack/kubernetes/client.go
index fd516bb5..c94fe592 100644
--- a/cli/command/stack/kubernetes/client.go
+++ b/cli/command/stack/kubernetes/client.go
@@ -11,97 +11,181 @@ import (
 	restclient "k8s.io/client-go/rest"
 )
 
+// ClientFactory creates kubernetes clients
+type ClientFactory interface {
+	// ConfigMaps returns a client for kubernetes's config maps
+	ConfigMaps(ops ...FactoryOp) corev1.ConfigMapInterface
+
+	// Secrets returns a client for kubernetes's secrets
+	Secrets(ops ...FactoryOp) corev1.SecretInterface
+
+	// Services returns a client for kubernetes's secrets
+	Services(ops ...FactoryOp) corev1.ServiceInterface
+
+	// Pods returns a client for kubernetes's pods
+	Pods(ops ...FactoryOp) corev1.PodInterface
+
+	// Nodes returns a client for kubernetes's nodes
+	Nodes(ops ...FactoryOp) corev1.NodeInterface
+
+	// ReplicationControllers returns a client for kubernetes replication controllers
+	ReplicationControllers(ops ...FactoryOp) corev1.ReplicationControllerInterface
+
+	// ReplicaSets returns a client for kubernetes replace sets
+	ReplicaSets(ops ...FactoryOp) typesappsv1beta2.ReplicaSetInterface
+
+	// DaemonSets returns a client for kubernetes daemon sets
+	DaemonSets(ops ...FactoryOp) typesappsv1beta2.DaemonSetInterface
+
+	// Stacks returns a client for Docker's Stack on Kubernetes
+	Stacks(ops ...FactoryOp) (StackClient, error)
+}
+
 // Factory is the kubernetes client factory
 type Factory struct {
-	namespace     string
-	config        *restclient.Config
-	coreClientSet corev1.CoreV1Interface
-	appsClientSet appsv1beta2.AppsV1beta2Interface
-	clientSet     *kubeclient.Clientset
-	experimental  bool
+	namespace       string
+	config          *restclient.Config
+	coreClientSet   corev1.CoreV1Interface
+	appsClientSet   appsv1beta2.AppsV1beta2Interface
+	clientSet       *kubeclient.Clientset
+	stackAPIVersion *kubernetes.StackVersion
 }
 
 // NewFactory creates a kubernetes client factory
-func NewFactory(namespace string, config *restclient.Config, clientSet *kubeclient.Clientset, experimental bool) (*Factory, error) {
-	coreClientSet, err := corev1.NewForConfig(config)
+func NewFactory(ops ...FactoryOp) (*Factory, error) {
+	f := &Factory{}
+
+	ops = append(ops, WithDefaultStackAPI)
+	for _, o := range ops {
+		if err := o(f); err != nil {
+			return nil, err
+		}
+	}
+
+	coreClientSet, err := corev1.NewForConfig(f.config)
 	if err != nil {
 		return nil, err
 	}
 
-	appsClientSet, err := appsv1beta2.NewForConfig(config)
+	appsClientSet, err := appsv1beta2.NewForConfig(f.config)
 	if err != nil {
 		return nil, err
 	}
 
-	return &Factory{
-		namespace:     namespace,
-		config:        config,
-		coreClientSet: coreClientSet,
-		appsClientSet: appsClientSet,
-		clientSet:     clientSet,
-		experimental:  experimental,
-	}, nil
+	f.coreClientSet = coreClientSet
+	f.appsClientSet = appsClientSet
+
+	return f, nil
 }
 
 // ConfigMaps returns a client for kubernetes's config maps
-func (s *Factory) ConfigMaps() corev1.ConfigMapInterface {
+func (s *Factory) ConfigMaps(ops ...FactoryOp) corev1.ConfigMapInterface {
 	return s.coreClientSet.ConfigMaps(s.namespace)
 }
 
 // Secrets returns a client for kubernetes's secrets
-func (s *Factory) Secrets() corev1.SecretInterface {
+func (s *Factory) Secrets(ops ...FactoryOp) corev1.SecretInterface {
 	return s.coreClientSet.Secrets(s.namespace)
 }
 
 // Services returns a client for kubernetes's secrets
-func (s *Factory) Services() corev1.ServiceInterface {
+func (s *Factory) Services(ops ...FactoryOp) corev1.ServiceInterface {
 	return s.coreClientSet.Services(s.namespace)
 }
 
 // Pods returns a client for kubernetes's pods
-func (s *Factory) Pods() corev1.PodInterface {
+func (s *Factory) Pods(ops ...FactoryOp) corev1.PodInterface {
 	return s.coreClientSet.Pods(s.namespace)
 }
 
 // Nodes returns a client for kubernetes's nodes
-func (s *Factory) Nodes() corev1.NodeInterface {
+func (s *Factory) Nodes(ops ...FactoryOp) corev1.NodeInterface {
 	return s.coreClientSet.Nodes()
 }
 
 // ReplicationControllers returns a client for kubernetes replication controllers
-func (s *Factory) ReplicationControllers() corev1.ReplicationControllerInterface {
+func (s *Factory) ReplicationControllers(ops ...FactoryOp) corev1.ReplicationControllerInterface {
 	return s.coreClientSet.ReplicationControllers(s.namespace)
 }
 
 // ReplicaSets returns a client for kubernetes replace sets
-func (s *Factory) ReplicaSets() typesappsv1beta2.ReplicaSetInterface {
+func (s *Factory) ReplicaSets(ops ...FactoryOp) typesappsv1beta2.ReplicaSetInterface {
 	return s.appsClientSet.ReplicaSets(s.namespace)
 }
 
 // DaemonSets returns a client for kubernetes daemon sets
-func (s *Factory) DaemonSets() typesappsv1beta2.DaemonSetInterface {
+func (s *Factory) DaemonSets(ops ...FactoryOp) typesappsv1beta2.DaemonSetInterface {
 	return s.appsClientSet.DaemonSets(s.namespace)
 }
 
 // Stacks returns a client for Docker's Stack on Kubernetes
-func (s *Factory) Stacks(allNamespaces bool) (StackClient, error) {
-	version, err := kubernetes.GetStackAPIVersion(s.clientSet.Discovery(), s.experimental)
-	if err != nil {
-		return nil, err
-	}
-	namespace := s.namespace
-	if allNamespaces {
-		namespace = metav1.NamespaceAll
+func (s *Factory) Stacks(ops ...FactoryOp) (StackClient, error) {
+	f := s
+
+	ops = append(ops, WithDefaultStackAPI)
+	for _, o := range ops {
+		if err := o(f); err != nil {
+			return nil, err
+		}
 	}
 
-	switch version {
+	switch *s.stackAPIVersion {
 	case kubernetes.StackAPIV1Beta1:
-		return newStackV1Beta1(s.config, namespace)
+		return newStackV1Beta1(s.config, s.namespace)
 	case kubernetes.StackAPIV1Beta2:
-		return newStackV1Beta2(s.config, namespace)
+		return newStackV1Beta2(s.config, s.namespace)
 	case kubernetes.StackAPIV1Alpha3:
-		return newStackV1Alpha3(s.config, namespace)
+		return newStackV1Alpha3(s.config, s.namespace)
 	default:
-		return nil, errors.Errorf("unsupported stack API version: %q", version)
+		return nil, errors.Errorf("unsupported stack API version: %q", s.stackAPIVersion)
+	}
+}
+
+type FactoryOp func(f *Factory) error
+
+func WithNameSpace(namespace string) FactoryOp {
+	return func(f *Factory) error {
+		f.namespace = namespace
+		return nil
+	}
+}
+
+func WithAllNamespaces(f *Factory) error {
+	f.namespace = metav1.NamespaceAll
+	return nil
+}
+
+func WithClientSet(clientSet *kubeclient.Clientset) FactoryOp {
+	return func(f *Factory) error {
+		f.clientSet = clientSet
+		return nil
+	}
+}
+
+func WithConfig(config *restclient.Config) FactoryOp {
+	return func(f *Factory) error {
+		f.config = config
+		return nil
+	}
+}
+
+func WithExperimentalStackAPI(f *Factory) error {
+	version, err := kubernetes.GetStackAPIVersion(f.clientSet.Discovery(), true)
+	if err != nil {
+		return err
+	}
+	f.stackAPIVersion = &version
+	return nil
+}
+
+func WithDefaultStackAPI(f *Factory) error {
+	if f.stackAPIVersion != nil {
+		return nil
+	}
+	version, err := kubernetes.GetStackAPIVersion(f.clientSet.Discovery(), false)
+	if err != nil {
+		return err
 	}
+	f.stackAPIVersion = &version
+	return nil
 }

[simonferquel]

I think we are using a Jackhammer to put a pin in a butter brick here :).
Without kidding, the responsibility of the factory is to do version negociation with the server to decide which implementation of the stackClient abstraction we want to use. Negociation was previously parameter-less, and now has a single parameter influencing the decision. I don't think this will change anytime soon (pick the highest common version, including or excluding experimental ones), and so I don't thing adding options there makes sense. Plus your example is error prone, and potentially has side effects on the factory fields each time you call a factory method.

[simonferquel]

@thaJeztah WDYT ? should we merge as is ?

[thaJeztah]

Yeah let's keep it as-is

[thaJeztah]

LGTM