Add validation to only allow Cassandra major version 3 #321

munnerz · 2018-04-26T10:28:22Z

Can we move this file out of pkg/cassandra and into something like pkg/api/util? This is an API type, but it isn't by default part of an API group (hence it isn't in apis).

It'd be good to denote it as such in the file structure, to make it clear this isn't specific to just cassandra (and also denotes that this type may be exposed in an api)

munnerz · 2018-04-26T10:26:55Z

Do we need to keep the versionString in memory? Surely semver.String will be more up to date?

We need it because we want to remember the format of the originally supplied version string.
So if the Cassandra node reports version 3.11 via nodetool and a JMX query, we want to report that exact version in the Pilot.Status.Cassandra.Version rather than the semver 3.11.0.

We also don't want a user to submit CassandraCluster.Spec.Version: 3.11 and then have that change to 3.11.0 when they do kubectl get CassandraCluster -o yaml.

However, when we infer the Docker image tag, we use the semver, because we don't want docker to simply pull the latest 3.11 image from Dockerhub.

munnerz · 2018-04-26T10:31:53Z

munnerz · 2018-04-26T10:31:17Z

This doesn't test anything, or ever fail afaict

It tests that a Zero value can be DeepCopied.
It originally caused a panic before I added a check for nil pointer in the implementation.
I'll leave it for now.

-Original file line number
+Diff line change
@@ -1,4 +1,3 @@
     # Gopkg.toml example
     #
     # Refer to https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
        and `Best way to add multiple nodes to existing cassandra cluster <https://stackoverflow.com/questions/37283424/best-way-to-add-multiple-nodes-to-existing-cassandra-cluster>`_.
     You can look at ``CassandraCluster.Status.NodePools[<nodepoolname>].ReadyReplicas`` to see the current number of healthy C* nodes in each ``nodepool``.
+    Supported Versions
+    ------------------
+    Navigator only supports Cassandra major version 3.

-Original file line number
+Diff line change
@@ Expand Up / @@ -14,3 +14,11 @@ ${CODEGEN_PKG}/generate-internal-groups.sh all \ @@
       navigator:v1alpha1 \
       --output-base "${GOPATH}/src/" \
       --go-header-file ${SCRIPT_ROOT}/hack/boilerplate.go.txt
+    echo "Generating other deepcopy funcs"
+    ${GOPATH}/bin/deepcopy-gen \
+             --input-dirs github.com/jetstack/navigator/pkg/api/version \
+             -O zz_generated.deepcopy \
+             --bounding-dirs github.com/jetstack/navigator/pkg/api/version \
+             --output-base "${GOPATH}/src/" \
+             --go-header-file ${SCRIPT_ROOT}/hack/boilerplate.go.txt

-Original file line number
+Diff line change
@@ Expand Up / @@ -6,6 +6,7 @@ import ( @@
     	core "k8s.io/api/core/v1"
     	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+    	"github.com/jetstack/navigator/pkg/api/version"
     	"github.com/jetstack/navigator/pkg/apis/navigator/v1alpha1"
     )
@@ Expand Down Expand Up @@
     			Name:      c.Name,
     			Namespace: c.Namespace,
     		},
+    		Spec: v1alpha1.CassandraClusterSpec{
+    			Version: *version.New("3.11.2"),
+    		},
     	}
     }
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -2,11 +2,9 @@ package version @@
     import (
     	"encoding/json"
-    	"fmt"
     	"strconv"
-    	"strings"
-    	"github.com/coreos/go-semver/semver"
+    	semver "github.com/hashicorp/go-version"
     )
     // Version represents a Cassandra database server version.
@@ Expand All / @@ -23,9 +21,11 @@ import ( @@
     // This also fixes the missing Patch number and stores the version internally as a semver.
     // It also keeps a reference to the original version string so that we can report that in our API.
     // So that the version reported in our API matches the version that an administrator expects.
+    //
+    // +k8s:deepcopy-gen=true
     type Version struct {
     	versionString string
-    	semver        semver.Version
+    	semver        *semver.Version
     }
     func New(s string) *Version {
@@ Expand All / @@ -37,10 +37,28 @@ func New(s string) *Version { @@
     	return v
     }
+    func (v *Version) set(s string) error {
+    	sv, err := semver.NewVersion(s)
+    	if err != nil {
+    		return err
+    	}
+    	v.versionString = s
+    	v.semver = sv
+    	return nil
+    }
     func (v *Version) Equal(versionB *Version) bool {
     	return v.semver.Equal(versionB.semver)
     }
+    func (v Version) String() string {
+    	return v.versionString
+    }
+    func (v *Version) Semver() *semver.Version {
+    	return v.semver
+    }
     func (v *Version) UnmarshalJSON(data []byte) error {
     	s, err := strconv.Unquote(string(data))
     	if err != nil {
@@ Expand All / @@ -49,62 +67,20 @@ func (v *Version) UnmarshalJSON(data []byte) error { @@
     	return v.set(s)
     }
-    func (v *Version) set(cassVersionString string) error {
-    	var versionsTried []string
-    	var errorsEncountered []string
-    	errorWhileParsingOriginalVersion := v.semver.Set(cassVersionString)
-    	if errorWhileParsingOriginalVersion == nil {
-    		v.versionString = cassVersionString
-    		return nil
-    	}
-    	versionsTried = append(versionsTried, cassVersionString)
-    	errorsEncountered = append(errorsEncountered, errorWhileParsingOriginalVersion.Error())
-    	semverString := maybeAddMissingPatchVersion(cassVersionString)
-    	if semverString != cassVersionString {
-    		errorWhileParsingSemverVersion := v.semver.Set(semverString)
-    		if errorWhileParsingSemverVersion == nil {
-    			v.versionString = cassVersionString
-    			return nil
-    		}
-    		versionsTried = append(versionsTried, semverString)
-    		errorsEncountered = append(errorsEncountered, errorWhileParsingSemverVersion.Error())
-    	}
-    	return fmt.Errorf(
-    		"unable to parse Cassandra version as semver. "+
-    			"Versions tried: '%s'. "+
-    			"Errors encountered: '%s'.",
-    		strings.Join(versionsTried, "','"),
-    		strings.Join(errorsEncountered, "','"),
-    	)
-    }
     var _ json.Unmarshaler = &Version{}
-    func maybeAddMissingPatchVersion(v string) string {
-    	mmpAndLabels := strings.SplitN(v, "-", 2)
-    	mmp := mmpAndLabels[0]
-    	mmpParts := strings.SplitN(mmp, ".", 3)
-    	if len(mmpParts) == 2 {
-    		mmp = mmp + ".0"
-    	}
-    	mmpAndLabels[0] = mmp
-    	return strings.Join(mmpAndLabels, "-")
-    }
-    func (v Version) String() string {
-    	return v.versionString
-    }
     func (v Version) MarshalJSON() ([]byte, error) {
     	return []byte(strconv.Quote(v.String())), nil
     }
     var _ json.Marshaler = &Version{}
-    func (v Version) Semver() string {
-    	return v.semver.String()
+    // DeepCopy returns a deep-copy of the Version value.
+    // If the underlying semver is a nil pointer, assume that the zero value is being copied,
+    // and return that.
+    func (v Version) DeepCopy() Version {
+    	if v.semver == nil {
+    		return Version{}
+    	}
+    	return *New(v.String())
     }

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation to only allow Cassandra major version 3 #321

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

munnerz Apr 26, 2018

Uh oh!

wallrj Apr 26, 2018

Uh oh!

munnerz Apr 26, 2018

Uh oh!

wallrj Apr 26, 2018

Uh oh!

munnerz Apr 26, 2018

Uh oh!

munnerz Apr 26, 2018

Uh oh!

wallrj Apr 26, 2018

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -3,7 +3,7 @@ package version_test @@
     import (
     	"testing"
-    	"github.com/jetstack/navigator/pkg/cassandra/version"
+    	"github.com/jetstack/navigator/pkg/api/version"
     )
     func TestUnmarshalJSON(t *testing.T) {
@@ Expand All / @@ -21,10 +21,6 @@ func TestUnmarshalJSON(t *testing.T) { @@
     			s:         `"0.0.x"`,
     			expectErr: true,
     		},
-    		"incomplete semver": {
-    			s:         `"3"`,
-    			expectErr: true,
-    		},
     		"cassandra partial invalid semver with labels": {
     			s:         `"X.Y-foo+bar"`,
     			expectErr: true,
@@ Expand All / @@ -33,6 +29,12 @@ func TestUnmarshalJSON(t *testing.T) { @@
     			s:         `"X.Y.0-"`,
     			expectErr: true,
     		},
+    		// Cassandra versions always include a minor version but Hashicorp
+    		// go-version (which we currently use for parsing) doesn't require it.
+    		"partial semver": {
+    			s: `"3"`,
+    			v: version.New("3.0.0"),
+    		},
     		"cassandra partial semver": {
     			s: `"3.9"`,
     			v: version.New("3.9.0"),
@@ Expand Down Expand Up / @@ -91,3 +93,18 @@ func TestUnmarshalJSON(t *testing.T) { @@
     		)
     	}
     }
+    func TestDeepCopy(t *testing.T) {
+    	t.Run(
+    		"zero value",
+    		func(t *testing.T) {
+    			t.Log(version.Version{}.DeepCopy())
+    		},
+    	)
+    	t.Run(
+    		"validated version",
+    		func(t *testing.T) {
+    			t.Log(version.New("3.11.2").DeepCopy())
+    		},
+    	)
+    }

-Original file line number
+Diff line change
@@ -0,0 +1,27 @@
+    // +build !ignore_autogenerated
+    /*
+    Copyright 2018 Jetstack Ltd.
+    Licensed under the Apache License, Version 2.0 (the "License");
+    you may not use this file except in compliance with the License.
+    You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+    */
+    // This file was autogenerated by deepcopy-gen. Do not edit it manually!
+    package version
+    // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+    func (in *Version) DeepCopyInto(out *Version) {
+    	*out = in.DeepCopy()
+    	return
+    }

-Original file line number
+Diff line change
@@ Expand Up / @@ -22,8 +22,8 @@ package v1alpha1 @@
     import (
     	semver "github.com/coreos/go-semver/semver"
+    	version "github.com/jetstack/navigator/pkg/api/version"
     	navigator "github.com/jetstack/navigator/pkg/apis/navigator"
-    	version "github.com/jetstack/navigator/pkg/cassandra/version"
     	v1 "k8s.io/api/core/v1"
     	conversion "k8s.io/apimachinery/pkg/conversion"
     	runtime "k8s.io/apimachinery/pkg/runtime"
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,15 +1,28 @@
     package validation
     import (
+    	"fmt"
     	"reflect"
     	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
     	"k8s.io/apimachinery/pkg/util/sets"
     	"k8s.io/apimachinery/pkg/util/validation/field"
+    	version "github.com/hashicorp/go-version"
     	"github.com/jetstack/navigator/pkg/apis/navigator"
     )
+    var supportedCassandraVersions version.Constraints
+    func init() {
+    	var err error
+    	supportedCassandraVersions, err = version.NewConstraint(">= 3, < 4")
+    	if err != nil {
+    		panic(err)
+    	}
+    }
     func ValidateCassandraClusterNodePool(np *navigator.CassandraClusterNodePool, fldPath *field.Path) field.ErrorList {
     	el := field.ErrorList{}
     	if np.Persistence != nil {
@@ Expand All @@
     	fldPath := field.NewPath("spec")
+    	if !new.Spec.Version.Equal(&old.Spec.Version) {
+    		allErrs = append(
+    			allErrs,
+    			field.Forbidden(
+    				fldPath.Child("version"),
+    				fmt.Sprintf(
+    					"cannot change the version of an existing cluster. "+
+    						"old version: %s, new version: %s",
+    					old.Spec.Version, new.Spec.Version,
+    				),
+    			),
+    		)
+    	}
     	npPath := fldPath.Child("nodePools")
     	for i, newNp := range new.Spec.NodePools {
     		idxPath := npPath.Index(i)
@@ Expand Down Expand Up @@
     }
     func ValidateCassandraClusterSpec(spec *navigator.CassandraClusterSpec, fldPath *field.Path) field.ErrorList {
     	allErrs := ValidateNavigatorClusterConfig(&spec.NavigatorClusterConfig, fldPath)
+    	if !supportedCassandraVersions.Check(spec.Version.Semver()) {
+    		allErrs = append(
+    			allErrs,
+    			field.Forbidden(
+    				fldPath.Child("version"),
+    				fmt.Sprintf(
+    					"%s is not supported. Supported versions are: %s",
+    					spec.Version, supportedCassandraVersions,
+    				),
+    			),
+    		)
+    	}
     	npPath := fldPath.Child("nodePools")
     	allNames := sets.String{}
     	for i, np := range spec.NodePools {
@@ Expand Down @@

Add validation to only allow Cassandra major version 3 #321

Uh oh!

Add validation to only allow Cassandra major version 3 #321

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

munnerz Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

wallrj Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

munnerz Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

wallrj Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

munnerz Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

munnerz Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

wallrj Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!