Introduce immutable transforms and consider KnativeServing CR config …

…to be canonical (#165)

* Add an e2e test for #138

After creating the KnativeServing instance, we then update it twice,
once with an entry for the config-logging ConfigMap and then again
without the key, verifying that the operator correctly syncs the
ConfigMap each time.

* Uses a pre-release of manifestival to fix #138

This relies on the Transform function now being immutable, returning a
new Manifest instead of mutating its source resources.

This is related to #163 as well.

* Address golint warnings

* Transform manifest once and pass to each reconcile stage

* Let's confine the arbitrariness of the configmap to the fixture

This also makes the WaitForConfigMap helper more helpful

Gopkg.toml

@@ -56,7 +56,7 @@ required = [
 
 [[constraint]]
   name = "github.com/jcrossley3/manifestival"
-  branch = "client-go"
+  branch = "immutable-transform"
 
 [[constraint]]
   name = "knative.dev/pkg"

pkg/reconciler/knativeserving/knativeserving_controller.go

@@ -109,36 +109,39 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
 
 func (r *Reconciler) reconcile(ctx context.Context, ks *servingv1alpha1.KnativeServing) error {
 	reqLogger := r.Logger.With(zap.String("Request.Namespace", ks.Namespace)).With("Request.Name", ks.Name)
+	reqLogger.Infow("Reconciling KnativeServing", "status", ks.Status)
 
 	// TODO: We need to find a better way to make sure the instance has the updated info.
 	ks.SetGroupVersionKind(servingv1alpha1.SchemeGroupVersion.WithKind("KnativeServing"))
-	stages := []func(*servingv1alpha1.KnativeServing) error{
+	stages := []func(*mf.Manifest, *servingv1alpha1.KnativeServing) error{
 		r.initStatus,
 		r.install,
 		r.checkDeployments,
 		r.deleteObsoleteResources,
 	}
 
-	reqLogger.Infow("Reconciling KnativeServing", "status", ks.Status)
+	manifest, err := r.transform(ks)
+	if err != nil {
+		return err
+	}
+
 	for _, stage := range stages {
-		if err := stage(ks); err != nil {
+		if err := stage(manifest, ks); err != nil {
 			return err
 		}
 	}
 	reqLogger.Infow("Reconcile stages complete", "status", ks.Status)
 	return nil
 }
 
-// Initialize status conditions
-func (r *Reconciler) initStatus(instance *servingv1alpha1.KnativeServing) error {
-	r.Logger.Debug("Initializing status")
-	if len(instance.Status.Conditions) == 0 {
-		instance.Status.InitializeConditions()
-		if err := r.updateStatus(instance); err != nil {
-			return err
-		}
+// Transform the resources
+func (r *Reconciler) transform(instance *servingv1alpha1.KnativeServing) (*mf.Manifest, error) {
+	r.Logger.Debug("Transforming manifest")
+	transforms, err := platform.Transformers(r.KubeClientSet, instance, r.Logger)
+	if err != nil {
+		return nil, err
 	}
-	return nil
+	return r.config.Transform(transforms...)
 }
 
 // Update the status subresource
@@ -153,37 +156,22 @@ func (r *Reconciler) updateStatus(instance *servingv1alpha1.KnativeServing) erro
 	return nil
 }
 
-// Install the resources from the Manifest
-func (r *Reconciler) install(instance *servingv1alpha1.KnativeServing) error {
-	r.Logger.Debug("Installing manifest")
-	defer r.updateStatus(instance)
-
-	if err := r.transform(instance); err != nil {
-		return err
-	}
-	if err := r.apply(instance); err != nil {
-		return err
-	}
-	return nil
-}
-
-// Transform the resources
-func (r *Reconciler) transform(instance *servingv1alpha1.KnativeServing) error {
-	r.Logger.Debug("Transforming manifest")
-	transforms, err := platform.Transformers(r.KubeClientSet, instance, r.Logger)
-	if err != nil {
-		return err
-	}
-	if err := r.config.Transform(transforms...); err != nil {
-		return err
+// Initialize status conditions
+func (r *Reconciler) initStatus(_ *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
+	r.Logger.Debug("Initializing status")
+	if len(instance.Status.Conditions) == 0 {
+		instance.Status.InitializeConditions()
+		if err := r.updateStatus(instance); err != nil {
+			return err
+		}
 	}
 	return nil
 }
 
-// Apply the embedded resources
-func (r *Reconciler) apply(instance *servingv1alpha1.KnativeServing) error {
-	r.Logger.Debug("Applying manifest")
-	if err := r.config.ApplyAll(); err != nil {
+// Apply the manifest resources
+func (r *Reconciler) install(manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
+	r.Logger.Debug("Installing manifest")
+	if err := manifest.ApplyAll(); err != nil {
 		instance.Status.MarkInstallFailed(err.Error())
 		return err
 	}
@@ -193,7 +181,7 @@ func (r *Reconciler) apply(instance *servingv1alpha1.KnativeServing) error {
 }
 
 // Check for all deployments available
-func (r *Reconciler) checkDeployments(instance *servingv1alpha1.KnativeServing) error {
+func (r *Reconciler) checkDeployments(manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
 	r.Logger.Debug("Checking deployments")
 	defer r.updateStatus(instance)
 	available := func(d *appsv1.Deployment) bool {
@@ -204,7 +192,7 @@ func (r *Reconciler) checkDeployments(instance *servingv1alpha1.KnativeServing)
 		}
 		return false
 	}
-	for _, u := range r.config.Resources {
+	for _, u := range manifest.Resources {
 		if u.GetKind() == "Deployment" {
 			deployment, err := r.KubeClientSet.AppsV1().Deployments(u.GetNamespace()).Get(u.GetName(), metav1.GetOptions{})
 			if err != nil {
@@ -225,32 +213,32 @@ func (r *Reconciler) checkDeployments(instance *servingv1alpha1.KnativeServing)
 }
 
 // Delete obsolete resources from previous versions
-func (r *Reconciler) deleteObsoleteResources(instance *servingv1alpha1.KnativeServing) error {
+func (r *Reconciler) deleteObsoleteResources(manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
 	// istio-system resources from 0.3
 	resource := &unstructured.Unstructured{}
 	resource.SetNamespace("istio-system")
 	resource.SetName("knative-ingressgateway")
 	resource.SetAPIVersion("v1")
 	resource.SetKind("Service")
-	if err := r.config.Delete(resource, &metav1.DeleteOptions{}); err != nil {
+	if err := manifest.Delete(resource, &metav1.DeleteOptions{}); err != nil {
 		return err
 	}
 	resource.SetAPIVersion("apps/v1")
 	resource.SetKind("Deployment")
-	if err := r.config.Delete(resource, &metav1.DeleteOptions{}); err != nil {
+	if err := manifest.Delete(resource, &metav1.DeleteOptions{}); err != nil {
 		return err
 	}
 	resource.SetAPIVersion("autoscaling/v1")
 	resource.SetKind("HorizontalPodAutoscaler")
-	if err := r.config.Delete(resource, &metav1.DeleteOptions{}); err != nil {
+	if err := manifest.Delete(resource, &metav1.DeleteOptions{}); err != nil {
 		return err
 	}
 	// config-controller from 0.5
 	resource.SetNamespace(instance.GetNamespace())
 	resource.SetName("config-controller")
 	resource.SetAPIVersion("v1")
 	resource.SetKind("ConfigMap")
-	if err := r.config.Delete(resource, &metav1.DeleteOptions{}); err != nil {
+	if err := manifest.Delete(resource, &metav1.DeleteOptions{}); err != nil {
 		return err
 	}
 	return nil

test/e2e/knativeservingdeployment_test.go

@@ -17,6 +17,7 @@ package e2e
 
 import (
 	"errors"
+	"fmt"
 	"path/filepath"
 	"runtime"
 	"testing"
@@ -27,6 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"knative.dev/pkg/test/logstream"
+	"knative.dev/serving-operator/pkg/apis/serving/v1alpha1"
 	"knative.dev/serving-operator/test"
 	"knative.dev/serving-operator/test/resources"
 )
@@ -55,6 +57,11 @@ func TestKnativeServingDeployment(t *testing.T) {
 		knativeServingVerify(t, clients, names)
 	})
 
+	t.Run("configure", func(t *testing.T) {
+		knativeServingVerify(t, clients, names)
+		knativeServingConfigure(t, clients, names)
+	})
+
 	// Delete the deployments one by one to see if they will be recreated.
 	t.Run("restore", func(t *testing.T) {
 		knativeServingVerify(t, clients, names)
@@ -78,6 +85,47 @@ func knativeServingVerify(t *testing.T, clients *test.Clients, names test.Resour
 
 }
 
+// knativeServingConfigure verifies that KnativeServing config is set properly
+func knativeServingConfigure(t *testing.T, clients *test.Clients, names test.ResourceNames) {
+	// We'll arbitrarily choose the logging config
+	configKey := "logging"
+	configMapName := fmt.Sprintf("%s/config-%s", names.Namespace, configKey)
+	// Get the existing KS without any spec
+	ks, err := clients.KnativeServing().Get(names.KnativeServing, metav1.GetOptions{})
+	// Add config to its spec
+	ks.Spec = v1alpha1.KnativeServingSpec{
+		Config: map[string]map[string]string{
+			configKey: map[string]string{
+				"loglevel.controller": "debug",
+			},
+		},
+	}
+	// Update it
+	if ks, err = clients.KnativeServing().Update(ks); err != nil {
+		t.Fatalf("KnativeServing %q failed to update: %v", names.KnativeServing, err)
+	}
+	// Verifty the relevant configmap has been updated
+	err = resources.WaitForConfigMap(configMapName, clients.KubeClient.Kube, func(m map[string]string) bool {
+		return m["loglevel.controller"] == "debug"
+	})
+	if err != nil {
+		t.Fatal("The operator failed to update the configmap")
+	}
+	// Now remove the config from the spec and update
+	ks.Spec = v1alpha1.KnativeServingSpec{}
+	if ks, err = clients.KnativeServing().Update(ks); err != nil {
+		t.Fatalf("KnativeServing %q failed to update: %v", names.KnativeServing, err)
+	}
+	// And verify the configmap entry is gone
+	err = resources.WaitForConfigMap(configMapName, clients.KubeClient.Kube, func(m map[string]string) bool {
+		_, exists := m["loglevel.controller"]
+		return !exists
+	})
+	if err != nil {
+		t.Fatal("The operator failed to revert the configmap")
+	}
+}
+
 // deploymentRecreation verify whether all the deployments for knative serving are able to recreate, when they are deleted.
 func deploymentRecreation(t *testing.T, clients *test.Clients, names test.ResourceNames) {
 	dpList, err := clients.KubeClient.Kube.AppsV1().Deployments(names.Namespace).List(metav1.ListOptions{})

test/resources/knativeserving.go

@@ -28,6 +28,8 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/tools/cache"
 	"knative.dev/pkg/test/logging"
 	"knative.dev/serving-operator/pkg/apis/serving/v1alpha1"
 	servingv1alpha1 "knative.dev/serving-operator/pkg/client/clientset/versioned/typed/serving/v1alpha1"
@@ -73,6 +75,18 @@ func CreateKnativeServing(clients servingv1alpha1.KnativeServingInterface, names
 	return svc, err
 }
 
+// WaitForConfigMap takes a condition function that evaluates ConfigMap data
+func WaitForConfigMap(name string, client *kubernetes.Clientset, fn func(map[string]string) bool) error {
+	ns, cm, _ := cache.SplitMetaNamespaceKey(name)
+	return wait.PollImmediate(Interval, Timeout, func() (bool, error) {
+		cm, err := client.CoreV1().ConfigMaps(ns).Get(cm, metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		return fn(cm.Data), nil
+	})
+}
+
 // IsKnativeServingReady will check the status conditions of the KnativeServing and return true if the KnativeServing is ready.
 func IsKnativeServingReady(s *v1alpha1.KnativeServing, err error) (bool, error) {
 	return s.Status.IsReady(), err