Merge pull request #3400 from bacongobbler/fix-3392

fix helm init --upgrade logic

cmd/helm/installer/install.go

@@ -63,8 +63,8 @@ func Upgrade(client kubernetes.Interface, opts *Options) error {
 	if err != nil {
 		return err
 	}
-	existingImage := obj.Spec.Template.Spec.Containers[0].Image
+	tillerImage := obj.Spec.Template.Spec.Containers[0].Image
-	if !isNewerVersion(existingImage) && !opts.ForceUpgrade {
+	if semverCompare(tillerImage) == -1 && !opts.ForceUpgrade {
 		return errors.New("current Tiller version is newer, use --force-upgrade to downgrade")
 	}
 	obj.Spec.Template.Spec.Containers[0].Image = opts.selectImage()
@@ -82,15 +82,24 @@ func Upgrade(client kubernetes.Interface, opts *Options) error {
 	return err
 }
 
-// isNewerVersion returns whether the current version is newer than the give image's version
+// semverCompare returns whether the client's version is older, equal or newer than the given image's version.
-func isNewerVersion(image string) bool {
+func semverCompare(image string) int {
 	split := strings.Split(image, ":")
 	if len(split) < 2 {
-		// If we don't know the version, we consider the current version newer
+		// If we don't know the version, we consider the client version newer.
-		return true
+		return 1
 	}
-	imageVersion := split[1]
+	tillerVersion, err := semver.NewVersion(split[1])
-	return semver.MustParse(version.Version).GreaterThan(semver.MustParse(imageVersion))
+	if err != nil {
+		// same thing with unparsable tiller versions (e.g. canary releases).
+		return 1
+	}
+	clientVersion, err := semver.NewVersion(version.Version)
+	if err != nil {
+		// aaaaaand same thing with unparsable helm versions (e.g. canary releases).
+		return 1
+	}
+	return clientVersion.Compare(tillerVersion)
 }
 
 // createDeployment creates the Tiller Deployment resource.

cmd/helm/installer/install_test.go

@@ -476,6 +476,129 @@ func TestUgrade_newerVersion(t *testing.T) {
 	}
 }
 
+func TestUpgrade_identical(t *testing.T) {
+	image := "gcr.io/kubernetes-helm/tiller:v2.0.0"
+	serviceAccount := "newServiceAccount"
+	existingDeployment, _ := deployment(&Options{
+		Namespace:      v1.NamespaceDefault,
+		ImageSpec:      "imageToReplace:v2.0.0",
+		ServiceAccount: "serviceAccountToReplace",
+		UseCanary:      false,
+	})
+	existingService := service(v1.NamespaceDefault)
+
+	fc := &fake.Clientset{}
+	fc.AddReactor("get", "deployments", func(action testcore.Action) (bool, runtime.Object, error) {
+		return true, existingDeployment, nil
+	})
+	fc.AddReactor("update", "deployments", func(action testcore.Action) (bool, runtime.Object, error) {
+		obj := action.(testcore.UpdateAction).GetObject().(*v1beta1.Deployment)
+		i := obj.Spec.Template.Spec.Containers[0].Image
+		if i != image {
+			t.Errorf("expected image = '%s', got '%s'", image, i)
+		}
+		sa := obj.Spec.Template.Spec.ServiceAccountName
+		if sa != serviceAccount {
+			t.Errorf("expected serviceAccountName = '%s', got '%s'", serviceAccount, sa)
+		}
+		return true, obj, nil
+	})
+	fc.AddReactor("get", "services", func(action testcore.Action) (bool, runtime.Object, error) {
+		return true, existingService, nil
+	})
+
+	opts := &Options{Namespace: v1.NamespaceDefault, ImageSpec: image, ServiceAccount: serviceAccount}
+	if err := Upgrade(fc, opts); err != nil {
+		t.Errorf("unexpected error: %#+v", err)
+	}
+
+	if actions := fc.Actions(); len(actions) != 3 {
+		t.Errorf("unexpected actions: %v, expected 3 actions got %d", actions, len(actions))
+	}
+}
+
+func TestUpgrade_canaryClient(t *testing.T) {
+	image := "gcr.io/kubernetes-helm/tiller:canary"
+	serviceAccount := "newServiceAccount"
+	existingDeployment, _ := deployment(&Options{
+		Namespace:      v1.NamespaceDefault,
+		ImageSpec:      "imageToReplace:v1.0.0",
+		ServiceAccount: "serviceAccountToReplace",
+		UseCanary:      false,
+	})
+	existingService := service(v1.NamespaceDefault)
+
+	fc := &fake.Clientset{}
+	fc.AddReactor("get", "deployments", func(action testcore.Action) (bool, runtime.Object, error) {
+		return true, existingDeployment, nil
+	})
+	fc.AddReactor("update", "deployments", func(action testcore.Action) (bool, runtime.Object, error) {
+		obj := action.(testcore.UpdateAction).GetObject().(*v1beta1.Deployment)
+		i := obj.Spec.Template.Spec.Containers[0].Image
+		if i != image {
+			t.Errorf("expected image = '%s', got '%s'", image, i)
+		}
+		sa := obj.Spec.Template.Spec.ServiceAccountName
+		if sa != serviceAccount {
+			t.Errorf("expected serviceAccountName = '%s', got '%s'", serviceAccount, sa)
+		}
+		return true, obj, nil
+	})
+	fc.AddReactor("get", "services", func(action testcore.Action) (bool, runtime.Object, error) {
+		return true, existingService, nil
+	})
+
+	opts := &Options{Namespace: v1.NamespaceDefault, ImageSpec: image, ServiceAccount: serviceAccount}
+	if err := Upgrade(fc, opts); err != nil {
+		t.Errorf("unexpected error: %#+v", err)
+	}
+
+	if actions := fc.Actions(); len(actions) != 3 {
+		t.Errorf("unexpected actions: %v, expected 3 actions got %d", actions, len(actions))
+	}
+}
+
+func TestUpgrade_canaryServer(t *testing.T) {
+	image := "gcr.io/kubernetes-helm/tiller:v2.0.0"
+	serviceAccount := "newServiceAccount"
+	existingDeployment, _ := deployment(&Options{
+		Namespace:      v1.NamespaceDefault,
+		ImageSpec:      "imageToReplace:canary",
+		ServiceAccount: "serviceAccountToReplace",
+		UseCanary:      false,
+	})
+	existingService := service(v1.NamespaceDefault)
+
+	fc := &fake.Clientset{}
+	fc.AddReactor("get", "deployments", func(action testcore.Action) (bool, runtime.Object, error) {
+		return true, existingDeployment, nil
+	})
+	fc.AddReactor("update", "deployments", func(action testcore.Action) (bool, runtime.Object, error) {
+		obj := action.(testcore.UpdateAction).GetObject().(*v1beta1.Deployment)
+		i := obj.Spec.Template.Spec.Containers[0].Image
+		if i != image {
+			t.Errorf("expected image = '%s', got '%s'", image, i)
+		}
+		sa := obj.Spec.Template.Spec.ServiceAccountName
+		if sa != serviceAccount {
+			t.Errorf("expected serviceAccountName = '%s', got '%s'", serviceAccount, sa)
+		}
+		return true, obj, nil
+	})
+	fc.AddReactor("get", "services", func(action testcore.Action) (bool, runtime.Object, error) {
+		return true, existingService, nil
+	})
+
+	opts := &Options{Namespace: v1.NamespaceDefault, ImageSpec: image, ServiceAccount: serviceAccount}
+	if err := Upgrade(fc, opts); err != nil {
+		t.Errorf("unexpected error: %#+v", err)
+	}
+
+	if actions := fc.Actions(); len(actions) != 3 {
+		t.Errorf("unexpected actions: %v, expected 3 actions got %d", actions, len(actions))
+	}
+}
+
 func tlsTestFile(t *testing.T, path string) string {
 	const tlsTestDir = "../../../testdata"
 	path = filepath.Join(tlsTestDir, path)