[release-4.11] OCPBUGS-853: certificate-publisher: Don't publish extraneous certificates

pkg/operator/controller/certificate-publisher/controller.go

@@ -1,19 +1,25 @@
-// The certificate-publisher controller is responsible for publishing in-use
-// certificates to the "router-certs" secret in the "openshift-config-managed"
-// namespace.
+// The certificate-publisher controller is responsible for publishing the
+// certificate and key of the ingresscontroller for the cluster ingress domain
+// to the "router-certs" secret in the "openshift-config-managed" namespace and
+// for publishing the certificate for the default ingresscontroller to the
+// "default-ingress-cert" configmap in the same namespace.  Note that the
+// "default" ingresscontroller is typically but not necessarily the
+// ingresscontroller with the cluster ingress domain.
 package certificatepublisher
 
 import (
 	"context"
 	"fmt"
 
 	logf "github.com/openshift/cluster-ingress-operator/pkg/log"
+	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 
 	"k8s.io/client-go/tools/record"
 
 	corev1 "k8s.io/api/core/v1"
 
+	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -47,7 +53,10 @@ type reconciler struct {
 }
 
 // New returns a new controller that publishes a "router-certs" secret in the
-// openshift-config-managed namespace with all in-use default certificates.
+// openshift-config-managed namespace with the certificate and key for the
+// ingresscontroller for the cluster ingress domain, as well as a
+// "default-ingress-cert" configmap with the certificate for the "default"
+// ingresscontroller (which is usually the same ingresscontroller).
 func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runtimecontroller.Controller, error) {
 	operatorCache := mgr.GetCache()
 	reconciler := &reconciler{
@@ -63,8 +72,8 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti
 	}
 
 	// Index ingresscontrollers over the default certificate name so that
-	// secretIsInUse and secretToIngressController can look up
-	// ingresscontrollers that reference the secret.
+	// secretToIngressController can look up ingresscontrollers that
+	// reference the secret.
 	if err := operatorCache.IndexField(context.Background(), &operatorv1.IngressController{}, "defaultCertificateName", client.IndexerFunc(func(o client.Object) []string {
 		secret := controller.RouterEffectiveDefaultCertificateSecretName(o.(*operatorv1.IngressController), operandNamespace)
 		return []string{secret.Name}
@@ -76,12 +85,7 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti
 	if err != nil {
 		return nil, fmt.Errorf("failed to create informer for secrets: %v", err)
 	}
-	if err := c.Watch(&source.Informer{Informer: secretsInformer}, handler.EnqueueRequestsFromMapFunc(reconciler.secretToIngressController), predicate.Funcs{
-		CreateFunc:  func(e event.CreateEvent) bool { return reconciler.secretIsInUse(e.Object) },
-		DeleteFunc:  func(e event.DeleteEvent) bool { return reconciler.secretIsInUse(e.Object) },
-		UpdateFunc:  func(e event.UpdateEvent) bool { return reconciler.secretIsInUse(e.ObjectNew) },
-		GenericFunc: func(e event.GenericEvent) bool { return reconciler.secretIsInUse(e.Object) },
-	}); err != nil {
+	if err := c.Watch(&source.Informer{Informer: secretsInformer}, handler.EnqueueRequestsFromMapFunc(reconciler.secretToIngressController)); err != nil {
 		return nil, err
 	}
 
@@ -90,7 +94,9 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti
 		DeleteFunc:  func(e event.DeleteEvent) bool { return reconciler.hasSecret(e.Object, e.Object) },
 		UpdateFunc:  func(e event.UpdateEvent) bool { return reconciler.secretChanged(e.ObjectOld, e.ObjectNew) },
 		GenericFunc: func(e event.GenericEvent) bool { return reconciler.hasSecret(e.Object, e.Object) },
-	}); err != nil {
+	}, predicate.NewPredicateFuncs(func(o client.Object) bool {
+		return reconciler.hasClusterIngressDomain(o) || isDefaultIngressController(o)
+	})); err != nil {
 		return nil, err
 	}
 
@@ -100,14 +106,27 @@ func New(mgr manager.Manager, operatorNamespace, operandNamespace string) (runti
 // secretToIngressController maps a secret to a slice of reconcile requests,
 // one request per ingresscontroller that references the secret.
 func (r *reconciler) secretToIngressController(o client.Object) []reconcile.Request {
-	requests := []reconcile.Request{}
-	controllers, err := r.ingressControllersWithSecret(o.GetName())
-	if err != nil {
-		log.Error(err, "failed to list ingresscontrollers for secret", "related", o.GetSelfLink())
+	var (
+		requests []reconcile.Request
+		list     operatorv1.IngressControllerList
+		listOpts = client.MatchingFields(map[string]string{
+			"defaultCertificateName": o.GetName(),
+		})
+		ingressConfig configv1.Ingress
+	)
+	if err := r.cache.List(context.Background(), &list, listOpts); err != nil {
+		log.Error(err, "failed to list ingresscontrollers for secret", "secret", o.GetName())
+		return requests
+	}
+	if err := r.cache.Get(context.Background(), controller.IngressClusterConfigName(), &ingressConfig); err != nil {
+		log.Error(err, "failed to get ingresses.config.openshift.io", "name", controller.IngressClusterConfigName())
 		return requests
 	}
-	for _, ic := range controllers {
-		log.Info("queueing ingresscontroller", "name", ic.Name, "related", o.GetSelfLink())
+	for _, ic := range list.Items {
+		if ic.Status.Domain != ingressConfig.Spec.Domain {
+			continue
+		}
+		log.Info("queueing ingresscontroller", "name", ic.Name)
 		request := reconcile.Request{
 			NamespacedName: types.NamespacedName{
 				Namespace: ic.Namespace,
@@ -119,27 +138,6 @@ func (r *reconciler) secretToIngressController(o client.Object) []reconcile.Requ
 	return requests
 }
 
-// ingressControllersWithSecret returns the ingresscontrollers that reference
-// the given secret.
-func (r *reconciler) ingressControllersWithSecret(secretName string) ([]operatorv1.IngressController, error) {
-	controllers := &operatorv1.IngressControllerList{}
-	if err := r.cache.List(context.Background(), controllers, client.MatchingFields(map[string]string{"defaultCertificateName": secretName})); err != nil {
-		return nil, err
-	}
-	return controllers.Items, nil
-}
-
-// secretIsInUse returns true if the given secret is referenced by some
-// ingresscontroller.
-func (r *reconciler) secretIsInUse(meta metav1.Object) bool {
-	controllers, err := r.ingressControllersWithSecret(meta.GetName())
-	if err != nil {
-		log.Error(err, "failed to list ingresscontrollers for secret", "related", meta.GetSelfLink())
-		return false
-	}
-	return len(controllers) > 0
-}
-
 // hasSecret returns true if the effective default certificate secret for the
 // given ingresscontroller exists, false otherwise.
 func (r *reconciler) hasSecret(meta metav1.Object, o runtime.Object) bool {
@@ -168,6 +166,26 @@ func (r *reconciler) secretChanged(old, new runtime.Object) bool {
 	return oldSecret != newSecret || oldStatus != newStatus
 }
 
+// hasClusterIngressDomain returns true if the effective domain for the given
+// ingresscontroller is the cluster ingress domain.
+func (r *reconciler) hasClusterIngressDomain(o client.Object) bool {
+	var ingressConfig configv1.Ingress
+	if err := r.cache.Get(context.Background(), controller.IngressClusterConfigName(), &ingressConfig); err != nil {
+		log.Error(err, "failed to get ingresses.config.openshift.io", "name", controller.IngressClusterConfigName())
+		// Assume it might be a match.  Better to reconcile an extra
+		// time than to miss an update.
+		return true
+	}
+	ic := o.(*operatorv1.IngressController)
+	return ic.Status.Domain == ingressConfig.Spec.Domain
+}
+
+// isDefaultIngressController returns true if the given ingresscontroller is the
+// "default" ingresscontroller.
+func isDefaultIngressController(o client.Object) bool {
+	return o.GetNamespace() == controller.DefaultOperatorNamespace && o.GetName() == manifests.DefaultIngressControllerName
+}
+
 func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
 	log.Info("Reconciling", "request", request)
 
@@ -181,9 +199,50 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, fmt.Errorf("failed to list secrets: %v", err)
 	}
 
-	if err := r.ensureRouterCertsGlobalSecret(secrets.Items, controllers.Items); err != nil {
+	var ingressConfig configv1.Ingress
+	if err := r.cache.Get(ctx, controller.IngressClusterConfigName(), &ingressConfig); err != nil {
+		return reconcile.Result{}, fmt.Errorf("failed to get ingresses.config.openshift.io %s: %v", controller.IngressClusterConfigName(), err)
+	}
+
+	if err := r.ensureRouterCertsGlobalSecret(secrets.Items, controllers.Items, &ingressConfig); err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed to ensure global secret: %v", err)
 	}
 
+	// We need to construct the CA bundle that can be used to verify the ingress used to serve the console and the oauth-server.
+	// In an operator maintained cluster, this is always `oc get -n openshift-ingress-operator ingresscontroller/default`, skip the rest and return here.
+	// TODO if network-edge wishes to expand the scope of the CA bundle (and you could legitimately see a need/desire to have one CA that verifies all ingress traffic).
+	// TODO this could be accomplished using union logic similar to the kube-apiserver's join of multiple CAs.
+	if request.NamespacedName.Namespace == controller.DefaultOperatorNamespace && request.NamespacedName.Name == manifests.DefaultIngressControllerName {
+		var defaultIngressController *operatorv1.IngressController
+		for i := range controllers.Items {
+			ic := &controllers.Items[i]
+			if isDefaultIngressController(ic) {
+				defaultIngressController = ic
+				break
+			}
+		}
+		if defaultIngressController == nil {
+			return reconcile.Result{}, fmt.Errorf("failed to lookup default ingresscontroller %s does not exist", manifests.DefaultIngressControllerName)
+		}
+
+		var wildcardServingCertKeySecret *corev1.Secret
+		secretName := controller.RouterEffectiveDefaultCertificateSecretName(defaultIngressController, controller.DefaultOperandNamespace)
+		for i := range secrets.Items {
+			secret := &secrets.Items[i]
+			if secret.Namespace == secretName.Namespace && secret.Name == secretName.Name {
+				wildcardServingCertKeySecret = secret
+				break
+			}
+		}
+		if wildcardServingCertKeySecret == nil {
+			return reconcile.Result{}, fmt.Errorf("failed to lookup wildcard cert: secret %s does not exist", secretName)
+		}
+
+		caBundle := string(wildcardServingCertKeySecret.Data["tls.crt"])
+		if err := r.ensureDefaultIngressCertConfigMap(caBundle); err != nil {
+			return reconcile.Result{}, fmt.Errorf("failed to publish router CA: %w", err)
+		}
+	}
+
 	return reconcile.Result{}, nil
 }

pkg/operator/controller/certificate/publish_ca.go → pkg/operator/controller/certificate-publisher/publish_ca.go

@@ -1,4 +1,4 @@
-package certificate
+package certificatepublisher
 
 import (
 	"context"

pkg/operator/controller/certificate-publisher/publish_certs.go

@@ -6,8 +6,10 @@ import (
 	"fmt"
 	"reflect"
 
+	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
+	"github.com/openshift/cluster-ingress-operator/pkg/util/ingresscontroller"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -16,8 +18,8 @@ import (
 
 // ensureRouterCertsGlobalSecret will create, update, or delete the global
 // certificates secret as appropriate.
-func (r *reconciler) ensureRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController) error {
-	desired, err := desiredRouterCertsGlobalSecret(secrets, ingresses, r.operandNamespace)
+func (r *reconciler) ensureRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController, ingressConfig *configv1.Ingress) error {
+	desired, err := desiredRouterCertsGlobalSecret(secrets, ingresses, r.operandNamespace, ingressConfig.Spec.Domain)
 	if err != nil {
 		return err
 	}
@@ -56,55 +58,84 @@ func (r *reconciler) ensureRouterCertsGlobalSecret(secrets []corev1.Secret, ingr
 
 // desiredRouterCertsGlobalSecret returns the desired router-certs global
 // secret.
-func desiredRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController, operandNamespace string) (*corev1.Secret, error) {
-	if len(ingresses) == 0 || len(secrets) == 0 {
-		return nil, nil
-	}
-
-	nameToSecret := map[string]*corev1.Secret{}
-	for i, certSecret := range secrets {
-		nameToSecret[certSecret.Name] = &secrets[i]
-	}
+func desiredRouterCertsGlobalSecret(secrets []corev1.Secret, ingresses []operatorv1.IngressController, operandNamespace, clusterIngressDomain string) (*corev1.Secret, error) {
+	for i := range ingresses {
+		// The authentication operator only requires the certificate and
+		// key for the ingresscontroller that handles the cluster
+		// ingress domain, and publishing certificates for all
+		// ingresscontrollers could cause the secret's size to exceed
+		// the maximum secret size of 1 mebibyte.  See
+		// <https://issues.redhat.com/browse/OCPBUGS-853>.
+		if ingresses[i].Status.Domain != clusterIngressDomain {
+			continue
+		}
 
-	ingressToSecret := map[*operatorv1.IngressController]*corev1.Secret{}
-	for i, ingress := range ingresses {
-		// Check if ingress.Spec.DefaultCertificate is an available secret
-		// in the secrets slice. If it is not, attempt to fall back to the
-		// operator generated default certificate. If ingress.Spec.DefaultCertificate
-		// is updated to point to a non-existent secret, the certificate controller
-		// will not delete the operator generated default certificate.
-		// See https://bugzilla.redhat.com/show_bug.cgi?id=1887441
-		if defaultCert := ingress.Spec.DefaultCertificate; defaultCert != nil {
-			if secret, ok := nameToSecret[defaultCert.Name]; ok {
-				ingressToSecret[&ingresses[i]] = secret
-				continue
-			}
+		// Multiple ingresscontrollers can have the cluster ingress
+		// domain, but only one can be admitted.  If this
+		// ingresscontroller hasn't been admitted, it should be ignored
+		// so that we use the default certificate of an
+		// ingresscontroller that has been admitted.
+		if !ingresscontroller.IsAdmitted(&ingresses[i]) {
+			continue
 		}
-		name := controller.RouterOperatorGeneratedDefaultCertificateSecretName(&ingress, operandNamespace)
-		if secret, ok := nameToSecret[name.Name]; ok {
-			ingressToSecret[&ingresses[i]] = secret
+
+		cert := getDefaultCertificateSecretForIngressController(&ingresses[i], secrets, operandNamespace)
+		if cert == nil {
+			break
 		}
-	}
 
-	name := controller.RouterCertsGlobalSecretName()
-	globalSecret := &corev1.Secret{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      name.Name,
-			Namespace: name.Namespace,
-		},
-		Data: map[string][]byte{},
+		globalCertName := controller.RouterCertsGlobalSecretName()
+		return &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      globalCertName.Name,
+				Namespace: globalCertName.Namespace,
+			},
+			Data: map[string][]byte{
+				ingresses[i].Status.Domain: bytes.Join([][]byte{
+					cert.Data["tls.crt"],
+					cert.Data["tls.key"],
+				}, nil),
+			},
+		}, nil
 	}
-	for ingress, certSecret := range ingressToSecret {
-		if len(ingress.Status.Domain) == 0 {
-			continue
+
+	return nil, nil
+}
+
+// getDefaultCertificateSecretForIngressController returns the appropriate
+// secret to use for the given ingresscontroller out of the provided list of
+// secrets.  If the ingresscontroller does not specify a secret or specifies a
+// secret that doesn't exist, the operator-generated default certificate is
+// returned.
+//
+// Note that if ingress.Spec.DefaultCertificate is updated to point to a
+// non-existent secret, the certificate controller does not delete the
+// operator-generated default certificate, so it is safe to fall back to using
+// it.  See <https://bugzilla.redhat.com/show_bug.cgi?id=1887441>.
+//
+// Note also that defaultCertName can be the same as customCertName; see
+// <https://bugzilla.redhat.com/show_bug.cgi?id=1912922>.
+func getDefaultCertificateSecretForIngressController(ic *operatorv1.IngressController, secrets []corev1.Secret, operandNamespace string) *corev1.Secret {
+	var (
+		defaultCertName         = controller.RouterOperatorGeneratedDefaultCertificateSecretName(ic, operandNamespace)
+		customCertName          = ic.Spec.DefaultCertificate
+		defaultCert, customCert *corev1.Secret
+	)
+	for i := range secrets {
+		if customCertName != nil && customCertName.Name == secrets[i].Name {
+			customCert = &secrets[i]
 		}
-		pem := bytes.Join([][]byte{
-			certSecret.Data["tls.crt"],
-			certSecret.Data["tls.key"],
-		}, nil)
-		globalSecret.Data[ingress.Status.Domain] = pem
+		if defaultCertName.Name == secrets[i].Name {
+			defaultCert = &secrets[i]
+		}
+	}
+	// Prefer any custom certificate, and fall back to the default
+	// certificate if no custom certificate was specified or the specified
+	// one was not found.
+	if customCert != nil {
+		return customCert
 	}
-	return globalSecret, nil
+	return defaultCert
 }
 
 // currentRouterCertsGlobalSecret returns the current router-certs global