Controller fixes (istio#411)

pkg/apis/istio/v1alpha2/istiocontrolplane_types.proto

@@ -524,6 +524,7 @@ message InstallStatus {
         UPDATING = 1;
         HEALTHY = 2;
         ERROR = 3;
+        RECONCILING = 4;
     }
     message VersionStatus {
         string version = 1;

pkg/component/component/component.go

@@ -22,21 +22,25 @@ package component
 import (
 	"fmt"
 
+	"istio.io/operator/pkg/util"
+
 	"github.com/ghodss/yaml"
 
 	"istio.io/operator/pkg/apis/istio/v1alpha2"
 	"istio.io/operator/pkg/helm"
 	"istio.io/operator/pkg/name"
 	"istio.io/operator/pkg/patch"
 	"istio.io/operator/pkg/translate"
-	"istio.io/operator/pkg/util"
 	"istio.io/pkg/log"
 )
 
 const (
 	// String to emit for any component which is disabled.
 	componentDisabledStr = " component is disabled."
 	yamlCommentStr       = "# "
+
+	// devDbg generates lots of output
+	devDbg = false
 )
 
 // Options defines options for a component.
@@ -696,7 +700,10 @@ func TranslateHelmValues(icp *v1alpha2.IstioControlPlaneSpec, translator *transl
 	if err != nil {
 		return "", err
 	}
-	log.Infof("Values translated from IstioControlPlane API:\n%s", apiValsStr)
+
+	if devDbg {
+		log.Infof("Values translated from IstioControlPlane API:\n%s", apiValsStr)
+	}
 
 	// Add global overlay from IstioControlPlaneSpec.Values.
 	_, err = name.SetFromPath(icp, "Values", &globalVals)
@@ -707,9 +714,10 @@ func TranslateHelmValues(icp *v1alpha2.IstioControlPlaneSpec, translator *transl
 	if err != nil {
 		return "", err
 	}
-	log.Infof("Values from IstioControlPlaneSpec.Values:\n%s", util.ToYAML(globalVals))
-	log.Infof("Values from IstioControlPlaneSpec.UnvalidatedValues:\n%s", util.ToYAML(globalUnvalidatedVals))
-
+	if devDbg {
+		log.Infof("Values from IstioControlPlaneSpec.Values:\n%s", util.ToYAML(globalVals))
+		log.Infof("Values from IstioControlPlaneSpec.UnvalidatedValues:\n%s", util.ToYAML(globalUnvalidatedVals))
+	}
 	mergedVals, err := overlayTrees(globalVals, apiVals)
 	if err != nil {
 		return "", err
@@ -749,17 +757,19 @@ func renderManifest(c *CommonComponentFields) (string, error) {
 		return "", err
 	}
 	my += helm.YAMLSeparator + "\n"
-	log.Infof("Initial manifest with merged values:\n%s\n", my)
-
+	if devDbg {
+		log.Infof("Initial manifest with merged values:\n%s\n", my)
+	}
 	// Add the k8s resources from IstioControlPlaneSpec.
 	my, err = c.Translator.OverlayK8sSettings(my, c.InstallSpec, c.name)
 	if err != nil {
 		log.Errorf("Error in OverlayK8sSettings: %s", err)
 		return "", err
 	}
 	my = "# Resources for " + string(c.name) + " component\n\n" + my
-	log.Infof("Manifest after k8s API settings:\n%s\n", my)
-
+	if devDbg {
+		log.Infof("Manifest after k8s API settings:\n%s\n", my)
+	}
 	// Add the k8s resource overlays from IstioControlPlaneSpec.
 	pathToK8sOverlay := fmt.Sprintf("%s.Components.%s.K8S.Overlays", c.FeatureName, c.name)
 	var overlays []*v1alpha2.K8SObjectOverlay
@@ -768,6 +778,7 @@ func renderManifest(c *CommonComponentFields) (string, error) {
 		return "", err
 	}
 	if !found {
+		log.Infof("Manifest after resources: \n%s\n", my)
 		return my, nil
 	}
 	kyo, err := yaml.Marshal(overlays)
@@ -784,7 +795,7 @@ func renderManifest(c *CommonComponentFields) (string, error) {
 		return "", err
 	}
 
-	log.Infof("After overlay: \n%s\n", ret)
+	log.Infof("Manifest after resources and overlay: \n%s\n", ret)
 	return ret, nil
 }
 

pkg/controller/istiocontrolplane/istiocontrolplane_controller.go

@@ -17,6 +17,7 @@ package istiocontrolplane
 import (
 	"context"
 
+	"gopkg.in/yaml.v2"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -95,60 +96,60 @@ type ReconcileIstioControlPlane struct {
 // Result.Requeue is true, otherwise upon completion it will remove the work from the queue.
 func (r *ReconcileIstioControlPlane) Reconcile(request reconcile.Request) (reconcile.Result, error) {
 	log.Info("Reconciling IstioControlPlane")
-
-	// Fetch the IstioControlPlane instance
-	instance := &v1alpha2.IstioControlPlane{}
-	err := r.client.Get(context.TODO(), request.NamespacedName, instance)
-	if err != nil {
-		if errors.IsNotFound(err) {
-			// Request object not found, could have been deleted after reconcile request.
-			// Owned objects are automatically garbage collected. For additional cleanup logic use finalizers.
-			// Return and don't requeue
-			return reconcile.Result{}, nil
-		}
-		// Error reading the object - requeue the request.
-		return reconcile.Result{}, err
-	}
-
 	// Workaroud for issue: https://github.com/istio/istio/issues/17883
 	// Using an unstructured object to get deletionTimestamp and finalizers fields.
 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(util.IstioOperatorGVK)
-	err = r.client.Get(context.TODO(), request.NamespacedName, u)
-	if err != nil {
-		log.Errorf("error getting the unstructured of IstioControlPlane instance: %s", err)
+	if err := r.client.Get(context.TODO(), request.NamespacedName, u); err != nil {
+		log.Errorf("error getting the unstructured of IstioControlPlane icp: %s", err)
 	}
 	deleted := u.GetDeletionTimestamp() != nil
 	finalizers := u.GetFinalizers()
 	finalizerIndex := indexOf(finalizers, finalizer)
 
+	icp := &v1alpha2.IstioControlPlane{}
+	icp.SetGroupVersionKind(util.IstioOperatorGVK)
+	if err := r.client.Get(context.TODO(), request.NamespacedName, icp); err != nil {
+		log.Errorf("error getting IstioControlPlane icp: %s", err)
+	}
+	os, err := yaml.Marshal(u.Object["spec"])
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	err = util.UnmarshalWithJSONPB(string(os), icp.Spec)
+	if err != nil {
+		log.Errorf("Cannot unmarshal: %s", err)
+		return reconcile.Result{}, err
+	}
+	log.Infof("Got IstioControlPlaneSpec: \n\n%s\n", string(os))
+
 	if deleted {
 		if finalizerIndex < 0 {
 			log.Info("IstioControlPlane deleted")
 			return reconcile.Result{}, nil
 		}
 		log.Info("Deleting IstioControlPlane")
 
-		reconciler, err := r.factory.New(instance, r.client)
+		reconciler, err := r.factory.New(icp, r.client)
 		if err == nil {
 			err = reconciler.Delete()
 		} else {
 			log.Errorf("failed to create reconciler: %s", err)
 		}
 		// TODO: for now, nuke the resources, regardless of errors
 		finalizers = append(finalizers[:finalizerIndex], finalizers[finalizerIndex+1:]...)
-		instance.SetFinalizers(finalizers)
-		finalizerError := r.client.Update(context.TODO(), instance)
+		icp.SetFinalizers(finalizers)
+		finalizerError := r.client.Update(context.TODO(), icp)
 		for retryCount := 0; errors.IsConflict(finalizerError) && retryCount < finalizerMaxRetries; retryCount++ {
 			// workaround for https://github.com/kubernetes/kubernetes/issues/73098 for k8s < 1.14
 			// TODO: make this error message more meaningful.
 			log.Info("conflict during finalizer removal, retrying")
-			_ = r.client.Get(context.TODO(), request.NamespacedName, instance)
-			finalizers = instance.GetFinalizers()
+			_ = r.client.Get(context.TODO(), request.NamespacedName, icp)
+			finalizers = icp.GetFinalizers()
 			finalizerIndex = indexOf(finalizers, finalizer)
 			finalizers = append(finalizers[:finalizerIndex], finalizers[finalizerIndex+1:]...)
-			instance.SetFinalizers(finalizers)
-			finalizerError = r.client.Update(context.TODO(), instance)
+			icp.SetFinalizers(finalizers)
+			finalizerError = r.client.Update(context.TODO(), icp)
 		}
 		if finalizerError != nil {
 			log.Errorf("error removing finalizer: %s", finalizerError)
@@ -159,16 +160,16 @@ func (r *ReconcileIstioControlPlane) Reconcile(request reconcile.Request) (recon
 		// TODO: make this error message more meaningful.
 		log.Infof("Adding finalizer %v", finalizer)
 		finalizers = append(finalizers, finalizer)
-		instance.SetFinalizers(finalizers)
-		err = r.client.Update(context.TODO(), instance)
+		icp.SetFinalizers(finalizers)
+		err = r.client.Update(context.TODO(), icp)
 		if err != nil {
 			log.Errorf("Failed to update IstioControlPlane with finalizer, %v", err)
 			return reconcile.Result{}, err
 		}
 	}
 
 	log.Info("Updating IstioControlPlane")
-	reconciler, err := r.factory.New(instance, r.client)
+	reconciler, err := r.factory.New(icp, r.client)
 	if err == nil {
 		err = reconciler.Reconcile()
 		if err != nil {

pkg/helmreconciler/customizers.go

@@ -19,8 +19,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/helm/pkg/manifest"
 
-	"istio.io/pkg/log"
-
 	"istio.io/operator/pkg/util"
 )
 
@@ -154,9 +152,6 @@ func (l *DefaultChartCustomizerListener) BeginChart(chartName string, manifests
 // BeginResource delegates to the active ChartCustomizer's BeginResource
 func (l *DefaultChartCustomizerListener) BeginResource(obj runtime.Object) (runtime.Object, error) {
 	if l.customizer == nil {
-		// XXX: what went wrong
-		// this should actually be a warning
-		log.Info("no active chart customizer")
 		return obj, nil
 	}
 	return l.customizer.BeginResource(obj)
@@ -165,9 +160,6 @@ func (l *DefaultChartCustomizerListener) BeginResource(obj runtime.Object) (runt
 // ResourceCreated delegates to the active ChartCustomizer's ResourceCreated
 func (l *DefaultChartCustomizerListener) ResourceCreated(created runtime.Object) error {
 	if l.customizer == nil {
-		// XXX: what went wrong
-		// this should actually be a warning
-		log.Info("no active chart customizer")
 		return nil
 	}
 	return l.customizer.ResourceCreated(created)
@@ -176,9 +168,6 @@ func (l *DefaultChartCustomizerListener) ResourceCreated(created runtime.Object)
 // ResourceUpdated delegates to the active ChartCustomizer's ResourceUpdated
 func (l *DefaultChartCustomizerListener) ResourceUpdated(updated runtime.Object, old runtime.Object) error {
 	if l.customizer == nil {
-		// XXX: what went wrong
-		// this should actually be a warning
-		log.Info("no active chart customizer")
 		return nil
 	}
 	return l.customizer.ResourceUpdated(updated, old)
@@ -187,9 +176,6 @@ func (l *DefaultChartCustomizerListener) ResourceUpdated(updated runtime.Object,
 // ResourceError delegates to the active ChartCustomizer's ResourceError
 func (l *DefaultChartCustomizerListener) ResourceError(obj runtime.Object, err error) error {
 	if l.customizer == nil {
-		// XXX: what went wrong
-		// this should actually be a warning
-		log.Info("no active chart customizer")
 		return nil
 	}
 	return l.customizer.ResourceError(obj, err)
@@ -198,9 +184,6 @@ func (l *DefaultChartCustomizerListener) ResourceError(obj runtime.Object, err e
 // EndResource delegates to the active ChartCustomizer's EndResource
 func (l *DefaultChartCustomizerListener) EndResource(obj runtime.Object) error {
 	if l.customizer == nil {
-		// XXX: what went wrong
-		// this should actually be a warning
-		log.Info("no active chart customizer")
 		return nil
 	}
 	return l.customizer.EndResource(obj)

pkg/helmreconciler/listeners.go

@@ -248,7 +248,6 @@ func (l *LoggingRenderingListener) BeginChart(chart string, manifests []manifest
 
 // BeginResource logs the event and updates the logger to log with values resource=name, kind=kind, apiVersion=api-version
 func (l *LoggingRenderingListener) BeginResource(obj runtime.Object) (runtime.Object, error) {
-	log.Infof("begin resource update: %s", obj.GetObjectKind().GroupVersionKind())
 	return obj, nil
 }
 
@@ -278,7 +277,6 @@ func (l *LoggingRenderingListener) ResourceError(obj runtime.Object, err error)
 
 // EndResource logs the event and resets the logger to its previous state (i.e. removes the resource specific labels).
 func (l *LoggingRenderingListener) EndResource(obj runtime.Object) error {
-	log.Info("end resource update")
 	lastIndex := len(l.loggerStack) - 1
 	if lastIndex >= 0 {
 		l.loggerStack = l.loggerStack[:lastIndex]

pkg/helmreconciler/reconciler.go

@@ -138,7 +138,7 @@ func (h *HelmReconciler) processRecursive(manifests ChartManifestsMap) *v1alpha2
 				out.Status[c] = &v1alpha2.InstallStatus_VersionStatus{}
 			}
 			mu.Unlock()
-			status := v1alpha2.InstallStatus_NONE
+			status := v1alpha2.InstallStatus_RECONCILING
 			errString := ""
 			if len(m) != 0 {
 				status = v1alpha2.InstallStatus_HEALTHY

pkg/helmreconciler/rendering.go

@@ -18,27 +18,25 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/ghodss/yaml"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/helm/pkg/manifest"
+	"k8s.io/helm/pkg/releaseutil"
+	kubectl "k8s.io/kubectl/pkg/util"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
 	"istio.io/operator/pkg/apis/istio/v1alpha2"
+	"istio.io/operator/pkg/component/controlplane"
 	"istio.io/operator/pkg/helm"
 	istiomanifest "istio.io/operator/pkg/manifest"
-	"istio.io/operator/pkg/util"
-	"istio.io/operator/pkg/validate"
-	"istio.io/pkg/log"
-
-	"istio.io/operator/pkg/component/controlplane"
 	"istio.io/operator/pkg/name"
 	"istio.io/operator/pkg/translate"
+	"istio.io/operator/pkg/util"
+	"istio.io/operator/pkg/validate"
 	binversion "istio.io/operator/version"
-
-	"k8s.io/helm/pkg/releaseutil"
-	kubectl "k8s.io/kubectl/pkg/util"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-
-	"github.com/ghodss/yaml"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	utilerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/helm/pkg/manifest"
+	"istio.io/pkg/log"
 )
 
 func (h *HelmReconciler) renderCharts(in RenderingInput) (ChartManifestsMap, error) {
@@ -246,7 +244,6 @@ func (h *HelmReconciler) ProcessObject(obj *unstructured.Unstructured) error {
 			}
 		}
 	}
-	log.Info("resource reconciliation complete")
 	if err != nil {
 		log.Errorf("error occurred reconciling resource: %s", err)
 	}
@@ -257,6 +254,7 @@ func toChartManifestsMap(m name.ManifestMap) ChartManifestsMap {
 	out := make(ChartManifestsMap)
 	for k, v := range m {
 		out[string(k)] = []manifest.Manifest{{
+			Name:    string(k),
 			Content: v,
 		}}
 	}