Normalize options on the stub and update the normalized CR (#54)

* Normalize options on the stub and update the normalized CR * Provide the known storage options when an unknown value is used Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jaegertracing · Oct 10, 2018 · d68e5de · d68e5de
1 parent ade28ed
commit d68e5de
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 41 deletions.
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -18,26 +18,37 @@ type Controller interface {
 
 // NewController build a new controller object for the given spec
 func NewController(ctx context.Context, jaeger *v1alpha1.Jaeger) Controller {
+	normalize(jaeger)
+
+	logrus.Debugf("Jaeger strategy: %s", jaeger.Spec.Strategy)
+	if jaeger.Spec.Strategy == "all-in-one" {
+		return newAllInOneController(ctx, jaeger)
+	}
+
+	return newProductionController(ctx, jaeger)
+}
+
+// normalize changes the incoming Jaeger object so that the defaults are applied when
+// needed and incompatible options are cleaned
+func normalize(jaeger *v1alpha1.Jaeger) {
 	// we need a name!
-	if jaeger.ObjectMeta.Name == "" {
+	if jaeger.Name == "" {
 		logrus.Infof("This Jaeger instance was created without a name. Setting it to 'my-jaeger'")
-		jaeger.ObjectMeta.Name = "my-jaeger"
+		jaeger.Name = "my-jaeger"
 	}
 
 	// normalize the storage type
 	if jaeger.Spec.Storage.Type == "" {
-		logrus.Infof(
-			"Storage type wasn't provided for the Jaeger instance '%v'. Falling back to 'memory'",
-			jaeger.ObjectMeta.Name,
-		)
+		logrus.Infof("Storage type wasn't provided for the Jaeger instance '%v'. Falling back to 'memory'", jaeger.Name)
 		jaeger.Spec.Storage.Type = "memory"
 	}
 
 	if unknownStorage(jaeger.Spec.Storage.Type) {
 		logrus.Infof(
-			"The provided storage type for the Jaeger instance '%v' is unknown ('%v'). Falling back to 'memory'",
-			jaeger.ObjectMeta.Name,
+			"The provided storage type for the Jaeger instance '%v' is unknown ('%v'). Falling back to 'memory'. Known options: %v",
+			jaeger.Name,
 			jaeger.Spec.Storage.Type,
+			knownStorages(),
 		)
 		jaeger.Spec.Storage.Type = "memory"
 	}
@@ -47,38 +58,33 @@ func NewController(ctx context.Context, jaeger *v1alpha1.Jaeger) Controller {
 		jaeger.Spec.Strategy = "all-in-one"
 	}
 
-	logrus.Debugf("Jaeger strategy: %s", jaeger.Spec.Strategy)
-	if jaeger.Spec.Strategy == "all-in-one" {
-		return newAllInOneController(ctx, jaeger)
-	}
-
 	// check for incompatible options
-	if strings.ToLower(jaeger.Spec.Storage.Type) == "memory" {
+	// if the storage is `memory`, then the only possible strategy is `all-in-one`
+	if strings.ToLower(jaeger.Spec.Storage.Type) == "memory" && strings.ToLower(jaeger.Spec.Strategy) != "all-in-one" {
 		logrus.Warnf(
-			"No suitable storage was provided for the Jaeger instance '%v'. "+
-				"Falling back to all-in-one. Storage type: '%v'",
-			jaeger.ObjectMeta.Name,
+			"No suitable storage was provided for the Jaeger instance '%v'. Falling back to all-in-one. Storage type: '%v'",
+			jaeger.Name,
 			jaeger.Spec.Storage.Type,
 		)
-		return newAllInOneController(ctx, jaeger)
+		jaeger.Spec.Strategy = "all-in-one"
 	}
-
-	return newProductionController(ctx, jaeger)
 }
 
 func unknownStorage(typ string) bool {
-	known := []string{
-		"memory",
-		"kafka",
-		"elasticsearch",
-		"cassandra",
-	}
-
-	for _, k := range known {
+	for _, k := range knownStorages() {
 		if strings.ToLower(typ) == k {
 			return false
 		}
 	}
 
 	return true
 }
+
+func knownStorages() []string {
+	return []string{
+		"memory",
+		"kafka",
+		"elasticsearch",
+		"cassandra",
+	}
+}
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -17,17 +17,25 @@ func TestNewControllerForAllInOneAsDefault(t *testing.T) {
 	jaeger := v1alpha1.NewJaeger("TestNewControllerForAllInOneAsDefault")
 
 	ctrl := NewController(context.TODO(), jaeger)
-	ds := ctrl.Create()
-	assert.Len(t, ds, 6)
+	rightType := false
+	switch ctrl.(type) {
+	case *allInOneController:
+		rightType = true
+	}
+	assert.True(t, rightType)
 }
 
 func TestNewControllerForAllInOneAsExplicitValue(t *testing.T) {
 	jaeger := v1alpha1.NewJaeger("TestNewControllerForAllInOneAsExplicitValue")
 	jaeger.Spec.Strategy = "ALL-IN-ONE" // same as 'all-in-one'
 
 	ctrl := NewController(context.TODO(), jaeger)
-	ds := ctrl.Create()
-	assert.Len(t, ds, 6)
+	rightType := false
+	switch ctrl.(type) {
+	case *allInOneController:
+		rightType = true
+	}
+	assert.True(t, rightType)
 }
 
 func TestNewControllerForProduction(t *testing.T) {
@@ -42,8 +50,7 @@ func TestNewControllerForProduction(t *testing.T) {
 func TestUnknownStorage(t *testing.T) {
 	jaeger := v1alpha1.NewJaeger("TestNewControllerForProduction")
 	jaeger.Spec.Storage.Type = "unknown"
-
-	NewController(context.TODO(), jaeger)
+	normalize(jaeger)
 	assert.Equal(t, "memory", jaeger.Spec.Storage.Type)
 }
 
@@ -73,8 +80,21 @@ func TestElasticsearchAsStorageOptions(t *testing.T) {
 
 func TestDefaultName(t *testing.T) {
 	jaeger := &v1alpha1.Jaeger{}
-	NewController(context.TODO(), jaeger)
-	assert.NotEmpty(t, jaeger.ObjectMeta.Name)
+	normalize(jaeger)
+	assert.NotEmpty(t, jaeger.Name)
+}
+
+func TestIncompatibleStorageForProduction(t *testing.T) {
+	jaeger := &v1alpha1.Jaeger{
+		Spec: v1alpha1.JaegerSpec{
+			Strategy: "production",
+			Storage: v1alpha1.JaegerStorageSpec{
+				Type: "memory",
+			},
+		},
+	}
+	normalize(jaeger)
+	assert.Equal(t, "all-in-one", jaeger.Spec.Strategy)
 }
 
 func getDeployments(objs []sdk.Object) []*appsv1.Deployment {

diff --git a/pkg/stub/handler.go b/pkg/stub/handler.go
@@ -26,16 +26,13 @@ func (h *Handler) Handle(ctx context.Context, event sdk.Event) error {
 	case *v1alpha1.Jaeger:
 		if event.Deleted {
 			logrus.Infof("Deleting '%s'", o.Name)
-			// aparently, everything created by the CR should cascade the deletion to the
-			// resources it created, so, no need to do anything
-			// on a next version, we could think about cleaning up the data pertaining to
-			// us at the storage level, but not for now
 			return nil
 		}
 
 		ctrl := controller.NewController(ctx, o)
 
 		objs := ctrl.Create()
+		created := false
 		for _, obj := range objs {
 			err := sdk.Create(obj)
 			if err != nil && !apierrors.IsAlreadyExists(err) {
@@ -44,10 +41,14 @@ func (h *Handler) Handle(ctx context.Context, event sdk.Event) error {
 			}
 
 			if err == nil {
-				logrus.Infof("Created '%v'", o.Name)
+				created = true
 			}
 		}
 
+		if created {
+			logrus.Infof("Configured %v", o.Name)
+		}
+
 		objs = ctrl.Update()
 		for _, obj := range objs {
 			logrus.Debugf("Updating %v", obj)
@@ -56,6 +57,12 @@ func (h *Handler) Handle(ctx context.Context, event sdk.Event) error {
 				return err
 			}
 		}
+
+		// we store back the changed CR, so that what is stored reflects what is being used
+		if err := sdk.Update(o); err != nil {
+			logrus.Errorf("failed to update %v", o)
+			return err
+		}
 	}
 	return nil
 }