Merge pull request #5820 from justinsb/etcd_manager_channels_step1

etcd: introduce field to specify whether we are using etcd-manager or legacy mode

cmd/kops/create_cluster_integration_test.go

@@ -19,6 +19,7 @@ package main
 import (
 	"bytes"
 	"io/ioutil"
+	"os"
 	"path"
 	"strings"
 	"testing"
@@ -254,12 +255,27 @@ func runCreateClusterIntegrationTest(t *testing.T, srcDir string, version string
 
 	actualYAML := strings.Join(yamlAll, "\n\n---\n\n")
 	if actualYAML != expectedYAML {
+		p := path.Join(srcDir, expectedClusterPath)
+
+		if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
+			t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", p)
+
+			// Format nicely - keep git happy
+			s := actualYAML
+			s = strings.TrimSpace(s)
+			s = s + "\n"
+
+			if err := ioutil.WriteFile(p, []byte(s), 0644); err != nil {
+				t.Errorf("error writing expected output %s: %v", p, err)
+			}
+		}
+
 		glog.Infof("Actual YAML:\n%s\n", actualYAML)
 
 		diffString := diff.FormatDiff(expectedYAML, actualYAML)
 		t.Logf("diff:\n%s\n", diffString)
 
-		t.Fatalf("YAML differed from expected (%s)", path.Join(srcDir, expectedClusterPath))
+		t.Errorf("YAML differed from expected (%s)", p)
 	}
 
 }

docs/etcd/manager.md

@@ -14,6 +14,14 @@ etcd-manager is currently in early alpha - it is undergoing testing. However, if
 you have a test cluster where you don't mind if it erases all your data, please
 do try it out and provide feedback.
 
+If using kubernetes >= 1.12 (which will not formally be supported until kops 1.12), note that etcd-manager will be used by default.  You can override this with the `cluster.spec.etcdClusters[*].provider=Legacy` override.  This can be specified:
+
+* as an argument to `kops create cluster`: `--overrides cluster.spec.etcdClusters[*].provider=Legacy`
+* on an existing cluster with `kops set cluster cluster.spec.etcdClusters[*].provider=Legacy`
+* by setting the field using `kops edit` or `kops replace`, manually making the same change as `kops set cluster ...`
+
+(Note you will probably have to `export KOPS_FEATURE_FLAGS=SpecOverrideFlag`)
+
 ## How to use etcd-manager
 
 Reminder: etcd-manager is alpha, and may cause you to lose the data in your

pkg/apis/kops/cluster.go

@@ -361,10 +361,21 @@ type ExternalDNSConfig struct {
 	WatchNamespace string `json:"watchNamespace,omitempty"`
 }
 
+// EtcdProviderType describes etcd cluster provisioning types (Standalone, Manager)
+type EtcdProviderType string
+
+const (
+	EtcdProviderTypeManager EtcdProviderType = "Manager"
+	EtcdProviderTypeLegacy  EtcdProviderType = "Legacy"
+)
+
 // EtcdClusterSpec is the etcd cluster specification
 type EtcdClusterSpec struct {
 	// Name is the name of the etcd cluster (main, events etc)
 	Name string `json:"name,omitempty"`
+	// Provider is the provider used to run etcd: standalone, manager.
+	// We default to manager for kubernetes 1.11 or if the manager is configured; otherwise standalone.
+	Provider EtcdProviderType `json:"provider,omitempty"`
 	// Members stores the configurations for each member of the cluster (including the data volume)
 	Members []*EtcdMemberSpec `json:"etcdMembers,omitempty"`
 	// EnableEtcdTLS indicates the etcd service should use TLS between peers and clients

pkg/apis/kops/v1alpha1/cluster.go

@@ -360,10 +360,21 @@ type ExternalDNSConfig struct {
 	WatchNamespace string `json:"watchNamespace,omitempty"`
 }
 
+// EtcdProviderType describes etcd cluster provisioning types (Standalone, Manager)
+type EtcdProviderType string
+
+const (
+	EtcdProviderTypeManager EtcdProviderType = "Manager"
+	EtcdProviderTypeLegacy  EtcdProviderType = "Legacy"
+)
+
 // EtcdClusterSpec is the etcd cluster specification
 type EtcdClusterSpec struct {
 	// Name is the name of the etcd cluster (main, events etc)
 	Name string `json:"name,omitempty"`
+	// Provider is the provider used to run etcd: standalone, manager.
+	// We default to manager for kubernetes 1.11 or if the manager is configured; otherwise standalone.
+	Provider EtcdProviderType `json:"provider,omitempty"`
 	// Members stores the configurations for each member of the cluster (including the data volume)
 	Members []*EtcdMemberSpec `json:"etcdMembers,omitempty"`
 	// EnableTLSAuth indicates client and peer TLS auth should be enforced

pkg/apis/kops/v1alpha2/cluster.go

@@ -361,10 +361,21 @@ type ExternalDNSConfig struct {
 	WatchNamespace string `json:"watchNamespace,omitempty"`
 }
 
+// EtcdProviderType describes etcd cluster provisioning types (Standalone, Manager)
+type EtcdProviderType string
+
+const (
+	EtcdProviderTypeManager EtcdProviderType = "Manager"
+	EtcdProviderTypeLegacy  EtcdProviderType = "Legacy"
+)
+
 // EtcdClusterSpec is the etcd cluster specification
 type EtcdClusterSpec struct {
 	// Name is the name of the etcd cluster (main, events etc)
 	Name string `json:"name,omitempty"`
+	// Provider is the provider used to run etcd: standalone, manager.
+	// We default to manager for kubernetes 1.11 or if the manager is configured; otherwise standalone.
+	Provider EtcdProviderType `json:"provider,omitempty"`
 	// Members stores the configurations for each member of the cluster (including the data volume)
 	Members []*EtcdMemberSpec `json:"etcdMembers,omitempty"`
 	// EnableEtcdTLS indicates the etcd service should use TLS between peers and clients

pkg/apis/kops/validation/cluster.go

@@ -41,15 +41,15 @@ func ValidateClusterUpdate(obj *kops.Cluster, status *kops.ClusterStatus, old *k
 		}
 
 		for k, newCluster := range newClusters {
-			fp := field.NewPath("Spec", "EtcdClusters").Key(k)
+			fp := field.NewPath("spec", "etcdClusters").Key(k)
 
 			oldCluster := oldClusters[k]
 			allErrs = append(allErrs, validateEtcdClusterUpdate(fp, newCluster, status, oldCluster)...)
 		}
 		for k := range oldClusters {
 			newCluster := newClusters[k]
 			if newCluster == nil {
-				fp := field.NewPath("Spec", "EtcdClusters").Key(k)
+				fp := field.NewPath("spec", "etcdClusters").Key(k)
 				allErrs = append(allErrs, field.Forbidden(fp, "EtcdClusters cannot be removed"))
 			}
 		}

pkg/apis/kops/validation/legacy.go

@@ -36,7 +36,7 @@ import (
 
 // ValidateCluster is responsible for checking the validity of the Cluster spec
 func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
-	fieldSpec := field.NewPath("Spec")
+	fieldSpec := field.NewPath("spec")
 	var err error
 
 	// kubernetesRelease is the version with only major & minor fields
@@ -573,7 +573,7 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
 			return field.Required(fieldEtcdClusters, "")
 		}
 		for i, x := range c.Spec.EtcdClusters {
-			if err := validateEtcdClusterSpec(x, fieldEtcdClusters.Index(i)); err != nil {
+			if err := validateEtcdClusterSpecLegacy(x, fieldEtcdClusters.Index(i)); err != nil {
 				return err
 			}
 		}
@@ -633,8 +633,8 @@ func validateSubnetCIDR(networkCIDR *net.IPNet, additionalNetworkCIDRs []*net.IP
 	return false
 }
 
-// validateEtcdClusterSpec is responsible for validating the etcd cluster spec
-func validateEtcdClusterSpec(spec *kops.EtcdClusterSpec, fieldPath *field.Path) *field.Error {
+// validateEtcdClusterSpecLegacy is responsible for validating the etcd cluster spec
+func validateEtcdClusterSpecLegacy(spec *kops.EtcdClusterSpec, fieldPath *field.Path) *field.Error {
 	if spec.Name == "" {
 		return field.Required(fieldPath.Child("Name"), "EtcdCluster did not have name")
 	}
@@ -697,7 +697,7 @@ func validateEtcdVersion(spec *kops.EtcdClusterSpec, fieldPath *field.Path, mini
 
 	version := spec.Version
 	if spec.Version == "" {
-		version = components.DefaultEtcdVersion
+		version = components.DefaultEtcd2Version
 	}
 
 	sem, err := semver.Parse(strings.TrimPrefix(version, "v"))

pkg/apis/kops/validation/validation.go

@@ -103,6 +103,13 @@ func validateClusterSpec(spec *kops.ClusterSpec, fieldPath *field.Path) field.Er
 		}
 	}
 
+	// EtcdClusters
+	{
+		for i, etcdCluster := range spec.EtcdClusters {
+			allErrs = append(allErrs, validateEtcdClusterSpec(etcdCluster, fieldPath.Child("etcdClusters").Index(i))...)
+		}
+	}
+
 	return allErrs
 }
 
@@ -315,3 +322,22 @@ func validateAdditionalPolicy(role string, policy string, fldPath *field.Path) f
 
 	return errs
 }
+
+func validateEtcdClusterSpec(spec *kops.EtcdClusterSpec, fieldPath *field.Path) field.ErrorList {
+	errs := field.ErrorList{}
+
+	switch spec.Provider {
+	case kops.EtcdProviderTypeManager:
+		// ok
+	case kops.EtcdProviderTypeLegacy:
+		// ok
+
+	case "":
+		// blank means that the user accepts the recommendation
+
+	default:
+		errs = append(errs, field.Invalid(fieldPath.Child("provider"), spec.Provider, "Provider must be Manager or Legacy"))
+	}
+
+	return errs
+}

pkg/commands/set_cluster.go

@@ -88,6 +88,10 @@ func SetClusterFields(fields []string, cluster *api.Cluster, instanceGroups []*a
 			for _, c := range cluster.Spec.EtcdClusters {
 				c.Version = kv[1]
 			}
+		case "cluster.spec.etcdClusters[*].provider":
+			for _, etcd := range cluster.Spec.EtcdClusters {
+				etcd.Provider = api.EtcdProviderType(kv[1])
+			}
 		case "cluster.spec.etcdClusters[*].manager.image":
 			for _, etcd := range cluster.Spec.EtcdClusters {
 				if etcd.Manager == nil {

pkg/model/components/etcd.go

@@ -27,80 +27,93 @@ const DefaultBackupImage = "kopeio/etcd-backup:1.0.20180220"
 
 // EtcdOptionsBuilder adds options for etcd to the model
 type EtcdOptionsBuilder struct {
-	Context *OptionsContext
+	*OptionsContext
 }
 
 var _ loader.OptionsBuilder = &EtcdOptionsBuilder{}
 
-const DefaultEtcdVersion = "2.2.1"
+const (
+	DefaultEtcd2Version = "2.2.1"
+
+	//  1.11 originally recommended 3.2.18, but there was an advisory to update to 3.2.24
+	DefaultEtcd3Version_1_11 = "3.2.24"
+
+	DefaultEtcd3Version_1_13 = "3.2.24"
+)
 
 // BuildOptions is responsible for filling in the defaults for the etcd cluster model
 func (b *EtcdOptionsBuilder) BuildOptions(o interface{}) error {
 	spec := o.(*kops.ClusterSpec)
 
 	for _, c := range spec.EtcdClusters {
+		if c.Provider == "" {
+			if b.IsKubernetesGTE("1.12") {
+				c.Provider = kops.EtcdProviderTypeManager
+			} else if c.Manager != nil {
+				c.Provider = kops.EtcdProviderTypeManager
+			} else {
+				c.Provider = kops.EtcdProviderTypeLegacy
+			}
+		}
+
 		// Ensure the version is set
-		// @TODO once we have a way of detecting a 'new' cluster we can default all clusters to v3
-		if c.Version == "" {
-			c.Version = DefaultEtcdVersion
+		if c.Version == "" && c.Provider == kops.EtcdProviderTypeLegacy {
+			// Even if in legacy mode, etcd version 2 is unsupported as of k8s 1.13
+			if b.IsKubernetesGTE("1.13") {
+				c.Version = DefaultEtcd3Version_1_13
+			} else {
+				c.Version = DefaultEtcd2Version
+			}
 		}
 
-		useEtcdManager := false
-		if c.Manager != nil {
-			useEtcdManager = true
+		if c.Version == "" && c.Provider == kops.EtcdProviderTypeManager {
+			// From 1.11, we run the k8s-recommended versions of etcd when using the manager
+			if b.IsKubernetesGTE("1.11") {
+				c.Version = DefaultEtcd3Version_1_11
+			} else {
+				c.Version = DefaultEtcd2Version
+			}
 		}
+	}
+
+	// Remap the well known images
+	for _, c := range spec.EtcdClusters {
 
-		// remap etcd image
-		{
-			image := c.Image
-			if image == "" {
-				if !useEtcdManager {
+		// We remap the etcd manager image when we build the manifest,
+		// but we need to map the standalone images here because protokube launches them
+
+		if c.Provider == kops.EtcdProviderTypeLegacy {
+
+			// remap etcd image
+			{
+				image := c.Image
+				if image == "" {
 					image = fmt.Sprintf("k8s.gcr.io/etcd:%s", c.Version)
 				}
-			}
 
-			if image != "" {
-				image, err := b.Context.AssetBuilder.RemapImage(image)
-				if err != nil {
-					return fmt.Errorf("unable to remap container %q: %v", image, err)
+				if image != "" {
+					image, err := b.AssetBuilder.RemapImage(image)
+					if err != nil {
+						return fmt.Errorf("unable to remap container %q: %v", image, err)
+					}
+					c.Image = image
 				}
-				c.Image = image
 			}
-		}
 
-		// remap backup manager image
-		if c.Backups != nil {
-			image := c.Backups.Image
-			if image == "" {
-				if !useEtcdManager {
+			// remap backup manager image
+			if c.Backups != nil {
+				image := c.Backups.Image
+				if image == "" {
 					image = fmt.Sprintf(DefaultBackupImage)
 				}
-			}
-
-			if image != "" {
-				image, err := b.Context.AssetBuilder.RemapImage(image)
-				if err != nil {
-					return fmt.Errorf("unable to remap container %q: %v", image, err)
-				}
-				c.Backups.Image = image
-			}
-		}
-
-		// remap etcd manager image
-		if useEtcdManager {
-			image := c.Manager.Image
-			if image == "" {
-				// We can make this easier later - maybe put it into the channel?  Or an addon?
-				//image = fmt.Sprintf(DefaultManagerImage)
-				return fmt.Errorf("EtcdManager.Image must be specified (currently)")
-			}
 
-			if image != "" {
-				image, err := b.Context.AssetBuilder.RemapImage(image)
-				if err != nil {
-					return fmt.Errorf("unable to remap container %q: %v", image, err)
+				if image != "" {
+					image, err := b.AssetBuilder.RemapImage(image)
+					if err != nil {
+						return fmt.Errorf("unable to remap container %q: %v", image, err)
+					}
+					c.Backups.Image = image
 				}
-				c.Manager.Image = image
 			}
 		}
 	}