Merge pull request #142 from ironcladlou/ca-publish-refactor

controller/certificate: relocate CA publishing

Author: openshift-merge-robot

hack/uninstall.sh

@@ -32,3 +32,5 @@ if [ "$WHAT" == "all" ]; then
   oc delete clusterrolebindings/router-monitoring
   oc delete customresourcedefinition.apiextensions.k8s.io/clusteringresses.ingress.openshift.io
 fi
+
+oc delete -n openshift-config-managed configmaps/router-ca

manifests/00-cluster-role.yaml

@@ -17,6 +17,13 @@ rules:
   verbs:
   - "*"
 
+- apiGroups:
+  - ""
+  resources:
+  - events
+  verbs:
+  - create
+
 - apiGroups:
   - apps
   resources:

pkg/log/log.go

@@ -14,7 +14,7 @@ var Logger logr.Logger
 
 func init() {
 	// Build a zap development logger.
-	zapLogger, err := zap.NewDevelopment(zap.AddCallerSkip(1))
+	zapLogger, err := zap.NewDevelopment(zap.AddCallerSkip(1), zap.AddStacktrace(zap.FatalLevel))
 	if err != nil {
 		panic(fmt.Sprintf("error building logger: %v", err))
 	}

pkg/operator/controller/certificate/ca.go

@@ -11,28 +11,14 @@ import (
 	"math/big"
 	"time"
 
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
+
 	corev1 "k8s.io/api/core/v1"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
-)
-
-const (
-	// caCertSecretName is the name of the secret that holds the CA certificate
-	// that the operator will use to create default certificates for
-	// clusteringresses.
-	caCertSecretName = "router-ca"
 )
 
-// CASecretName returns the namespaced name for the router CA secret.
-func CASecretName(operatorNamespace string) types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: operatorNamespace,
-		Name:      caCertSecretName,
-	}
-}
-
 func (r *reconciler) ensureRouterCASecret() (*corev1.Secret, error) {
 	current, err := r.currentRouterCASecret()
 	if err != nil {
@@ -55,7 +41,7 @@ func (r *reconciler) ensureRouterCASecret() (*corev1.Secret, error) {
 
 // currentRouterCASecret returns the current router CA secret.
 func (r *reconciler) currentRouterCASecret() (*corev1.Secret, error) {
-	name := CASecretName(r.operatorNamespace)
+	name := controller.RouterCASecretName(r.operatorNamespace)
 	secret := &corev1.Secret{}
 	if err := r.client.Get(context.TODO(), name, secret); err != nil {
 		if errors.IsNotFound(err) {
@@ -128,7 +114,7 @@ func desiredRouterCASecret(namespace string) (*corev1.Secret, error) {
 		return nil, fmt.Errorf("failed to generate certificate: %v", err)
 	}
 
-	name := CASecretName(namespace)
+	name := controller.RouterCASecretName(namespace)
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name.Name,

pkg/operator/controller/certificate/controller.go

@@ -12,18 +12,18 @@ import (
 
 	ingressv1alpha1 "github.com/openshift/cluster-ingress-operator/pkg/apis/ingress/v1alpha1"
 	logf "github.com/openshift/cluster-ingress-operator/pkg/log"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	"k8s.io/client-go/tools/record"
 
 	appsv1 "k8s.io/api/apps/v1"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller"
+	runtimecontroller "sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -36,13 +36,13 @@ const (
 
 var log = logf.Logger.WithName(controllerName)
 
-func New(mgr manager.Manager, client client.Client, operatorNamespace string) (controller.Controller, error) {
+func New(mgr manager.Manager, client client.Client, operatorNamespace string) (runtimecontroller.Controller, error) {
 	reconciler := &reconciler{
 		client:            client,
 		recorder:          mgr.GetRecorder(controllerName),
 		operatorNamespace: operatorNamespace,
 	}
-	c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: reconciler})
+	c, err := runtimecontroller.New(controllerName, mgr, runtimecontroller.Options{Reconciler: reconciler})
 	if err != nil {
 		return nil, err
 	}
@@ -61,29 +61,28 @@ type reconciler struct {
 func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) {
 	ca, err := r.ensureRouterCASecret()
 	if err != nil {
-		return reconcile.Result{}, fmt.Errorf("failed to ensure router CA secret: %v", err)
+		return reconcile.Result{}, fmt.Errorf("failed to ensure router CA: %v", err)
 	}
 
 	result := reconcile.Result{}
 	errs := []error{}
 	ingress := &ingressv1alpha1.ClusterIngress{}
 	if err := r.client.Get(context.TODO(), request.NamespacedName, ingress); err != nil {
 		if errors.IsNotFound(err) {
-			// This means the ingress was already deleted/finalized and there are
-			// stale queue entries (or something edge triggering from a related
-			// resource that got deleted async).
+			// The ingress could have been deleted and we're processing a stale queue
+			// item, so ignore and skip.
 			log.Info("clusteringress not found; reconciliation will be skipped", "request", request)
 		} else {
 			errs = append(errs, fmt.Errorf("failed to get clusteringress: %v", err))
 		}
 	} else {
 		deployment := &appsv1.Deployment{}
-		err = r.client.Get(context.TODO(), routerDeploymentName(ingress), deployment)
+		err = r.client.Get(context.TODO(), controller.RouterDeploymentName(ingress), deployment)
 		if err != nil {
 			if errors.IsNotFound(err) {
 				// All ingresses should have a deployment, so this one may not have been
 				// created yet. Retry after a reasonable amount of time.
-				log.Info("deployment not found for %s; will retry default cert sync", "clusteringress", ingress.Name)
+				log.Info("deployment not found; will retry default cert sync", "clusteringress", ingress.Name)
 				result.RequeueAfter = 5 * time.Second
 			} else {
 				errs = append(errs, fmt.Errorf("failed to get deployment: %v", err))
@@ -103,12 +102,13 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 			}
 		}
 	}
-	return result, utilerrors.NewAggregate(errs)
-}
 
-func routerDeploymentName(ci *ingressv1alpha1.ClusterIngress) types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: "openshift-ingress",
-		Name:      "router-" + ci.Name,
+	ingresses := &ingressv1alpha1.ClusterIngressList{}
+	if err := r.client.List(context.TODO(), &client.ListOptions{Namespace: r.operatorNamespace}, ingresses); err != nil {
+		errs = append(errs, fmt.Errorf("failed to list clusteringresses: %v", err))
+	} else if err := r.ensureRouterCAConfigMap(ca, ingresses.Items); err != nil {
+		errs = append(errs, fmt.Errorf("failed to publish router CA: %v", err))
 	}
+
+	return result, utilerrors.NewAggregate(errs)
 }

pkg/operator/controller/certificate/default_cert.go

@@ -7,23 +7,14 @@ import (
 	"github.com/openshift/library-go/pkg/crypto"
 
 	ingressv1alpha1 "github.com/openshift/cluster-ingress-operator/pkg/apis/ingress/v1alpha1"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
-// routerDefaultCertificateSecretName returns the namespaced name for the router
-// default certificate secret.
-func routerDefaultCertificateSecretName(ci *ingressv1alpha1.ClusterIngress, namespace string) types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: namespace,
-		Name:      fmt.Sprintf("router-certs-%s", ci.Name),
-	}
-}
-
 // ensureDefaultCertificateForIngress creates or deletes an operator-generated
 // default certificate for a given ClusterIngress as appropriate.
 func (r *reconciler) ensureDefaultCertificateForIngress(caSecret *corev1.Secret, namespace string, deploymentRef metav1.OwnerReference, ci *ingressv1alpha1.ClusterIngress) error {
@@ -85,7 +76,7 @@ func desiredRouterDefaultCertificateSecret(ca *crypto.CA, namespace string, depl
 		return nil, fmt.Errorf("failed to encode certificate: %v", err)
 	}
 
-	name := routerDefaultCertificateSecretName(ci, namespace)
+	name := controller.RouterDefaultCertificateSecretName(ci, namespace)
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name.Name,
@@ -104,7 +95,7 @@ func desiredRouterDefaultCertificateSecret(ca *crypto.CA, namespace string, depl
 // currentRouterDefaultCertificate returns the current router default
 // certificate secret.
 func (r *reconciler) currentRouterDefaultCertificate(ci *ingressv1alpha1.ClusterIngress, namespace string) (*corev1.Secret, error) {
-	name := routerDefaultCertificateSecretName(ci, namespace)
+	name := controller.RouterDefaultCertificateSecretName(ci, namespace)
 	secret := &corev1.Secret{}
 	if err := r.client.Get(context.TODO(), name, secret); err != nil {
 		if errors.IsNotFound(err) {

pkg/operator/controller/controller_ca_configmap.go renamed to pkg/operator/controller/certificate/publish_ca.go

@@ -1,38 +1,17 @@
-package controller
+package certificate
 
 import (
 	"context"
 	"fmt"
 
 	ingressv1alpha1 "github.com/openshift/cluster-ingress-operator/pkg/apis/ingress/v1alpha1"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 )
 
-const (
-	// GlobalMachineSpecifiedConfigNamespace is the location for global
-	// config.  In particular, the operator will put the configmap with the
-	// CA certificate in this namespace.
-	GlobalMachineSpecifiedConfigNamespace = "openshift-config-managed"
-
-	// caCertConfigMapName is the name of the config map with the public key
-	// for the CA certificate, which the operator publishes for other
-	// operators to use.
-	caCertConfigMapName = "router-ca"
-)
-
-// routerCAConfigMapName returns the namespaced name for the router CA
-// configmap.
-func routerCAConfigMapName() types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: GlobalMachineSpecifiedConfigNamespace,
-		Name:      caCertConfigMapName,
-	}
-}
-
 // ensureRouterCAConfigMap will create, update, or delete the configmap for the
 // router CA as appropriate.
 func (r *reconciler) ensureRouterCAConfigMap(secret *corev1.Secret, ingresses []ingressv1alpha1.ClusterIngress) error {
@@ -48,16 +27,22 @@ func (r *reconciler) ensureRouterCAConfigMap(secret *corev1.Secret, ingresses []
 	case desired == nil && current == nil:
 		// Nothing to do.
 	case desired == nil && current != nil:
-		if err := r.deleteRouterCAConfigMap(current); err != nil {
+		if deleted, err := r.deleteRouterCAConfigMap(current); err != nil {
 			return fmt.Errorf("failed to ensure router CA was unpublished: %v", err)
+		} else if deleted {
+			r.recorder.Eventf(current, "Normal", "UnpublishedDefaultRouterCA", "Unpublished default router CA")
 		}
 	case desired != nil && current == nil:
-		if err := r.createRouterCAConfigMap(desired); err != nil {
+		if created, err := r.createRouterCAConfigMap(desired); err != nil {
 			return fmt.Errorf("failed to ensure router CA was published: %v", err)
+		} else if created {
+			r.recorder.Eventf(desired, "Normal", "PublishedDefaultRouterCA", "Published default router CA")
 		}
 	case desired != nil && current != nil:
-		if err := r.updateRouterCAConfigMap(current, desired); err != nil {
+		if updated, err := r.updateRouterCAConfigMap(current, desired); err != nil {
 			return fmt.Errorf("failed to update published router CA: %v", err)
+		} else if updated {
+			r.recorder.Eventf(desired, "Normal", "UpdatedPublishedDefaultRouterCA", "Updated the published default router CA")
 		}
 	}
 	return nil
@@ -69,7 +54,7 @@ func desiredRouterCAConfigMap(secret *corev1.Secret, ingresses []ingressv1alpha1
 		return nil, nil
 	}
 
-	name := routerCAConfigMapName()
+	name := controller.RouterCAConfigMapName()
 	cm := &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name.Name,
@@ -95,9 +80,9 @@ func shouldPublishRouterCA(ingresses []ingressv1alpha1.ClusterIngress) bool {
 
 // currentRouterCAConfigMap returns the current router CA configmap.
 func (r *reconciler) currentRouterCAConfigMap() (*corev1.ConfigMap, error) {
-	name := routerCAConfigMapName()
+	name := controller.RouterCAConfigMapName()
 	cm := &corev1.ConfigMap{}
-	if err := r.Client.Get(context.TODO(), name, cm); err != nil {
+	if err := r.client.Get(context.TODO(), name, cm); err != nil {
 		if errors.IsNotFound(err) {
 			return nil, nil
 		}
@@ -106,45 +91,45 @@ func (r *reconciler) currentRouterCAConfigMap() (*corev1.ConfigMap, error) {
 	return cm, nil
 }
 
-// createRouterCAConfigMap creates a router CA configmap.
-func (r *reconciler) createRouterCAConfigMap(cm *corev1.ConfigMap) error {
-	if err := r.Client.Create(context.TODO(), cm); err != nil {
+// createRouterCAConfigMap creates a router CA configmap. Returns true if the
+// configmap was created, false otherwise.
+func (r *reconciler) createRouterCAConfigMap(cm *corev1.ConfigMap) (bool, error) {
+	if err := r.client.Create(context.TODO(), cm); err != nil {
 		if errors.IsAlreadyExists(err) {
-			return nil
+			return false, nil
 		}
-		return err
+		return false, err
 	}
-	log.Info("created configmap", "namespace", cm.Namespace, "name", cm.Name)
-	return nil
+	return true, nil
 }
 
-// updateRouterCAConfigMaps updates the router CA configmap.
-func (r *reconciler) updateRouterCAConfigMap(current, desired *corev1.ConfigMap) error {
+// updateRouterCAConfigMaps updates the router CA configmap. Returns true if the
+// configmap was updated, false otherwise.
+func (r *reconciler) updateRouterCAConfigMap(current, desired *corev1.ConfigMap) (bool, error) {
 	if routerCAConfigMapsEqual(current, desired) {
-		return nil
+		return false, nil
 	}
 	updated := current.DeepCopy()
 	updated.Data = desired.Data
-	if err := r.Client.Update(context.TODO(), updated); err != nil {
+	if err := r.client.Update(context.TODO(), updated); err != nil {
 		if errors.IsAlreadyExists(err) {
-			return nil
+			return false, nil
 		}
-		return err
+		return false, err
 	}
-	log.Info("updated configmap", "namespace", updated.Namespace, "name", updated.Name)
-	return nil
+	return true, nil
 }
 
-// deleteRouterCAConfigMap deletes the router CA configmap.
-func (r *reconciler) deleteRouterCAConfigMap(cm *corev1.ConfigMap) error {
-	if err := r.Client.Delete(context.TODO(), cm); err != nil {
+// deleteRouterCAConfigMap deletes the router CA configmap. Returns true if the
+// configmap was deleted, false otherwise.
+func (r *reconciler) deleteRouterCAConfigMap(cm *corev1.ConfigMap) (bool, error) {
+	if err := r.client.Delete(context.TODO(), cm); err != nil {
 		if errors.IsNotFound(err) {
-			return nil
+			return false, nil
 		}
-		return err
+		return false, err
 	}
-	log.Info("deleted configmap", "namespace", cm.Namespace, "name", cm.Name)
-	return nil
+	return true, nil
 }
 
 // routerCAConfigMapsEqual compares two router CA configmaps.

pkg/operator/controller/controller_test.go renamed to pkg/operator/controller/certificate/publish_ca_test.go

@@ -1,4 +1,4 @@
-package controller
+package certificate
 
 import (
 	"fmt"