Merge pull request #864 from Miciah/OCPBUGS-4573-service-internal-dot…

…-yaml-target-metrics-port-by-name

OCPBUGS-4573: Target metrics port by name in internal service

assets/router/service-internal.yaml

@@ -17,4 +17,4 @@ spec:
   - name: metrics
     port: 1936
     protocol: TCP
-    targetPort: 1936
+    targetPort: metrics

pkg/operator/controller/ingress/controller.go

@@ -1035,8 +1035,10 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
 		errs = append(errs, err)
 	}
 
-	if internalSvc, err := r.ensureInternalIngressControllerService(ci, deploymentRef); err != nil {
+	if haveSvc, internalSvc, err := r.ensureInternalIngressControllerService(ci, deploymentRef); err != nil {
 		errs = append(errs, fmt.Errorf("failed to create internal router service for ingresscontroller %s: %v", ci.Name, err))
+	} else if !haveSvc {
+		errs = append(errs, fmt.Errorf("failed to get internal route service for ingresscontroller %s: %w", ci.Name, err))
 	} else if err := r.ensureMetricsIntegration(ci, internalSvc, deploymentRef); err != nil {
 		errs = append(errs, fmt.Errorf("failed to integrate metrics with openshift-monitoring for ingresscontroller %s: %v", ci.Name, err))
 	}

pkg/operator/controller/ingress/internal_service.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
@@ -12,6 +15,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 const (
@@ -21,34 +25,42 @@ const (
 )
 
 // ensureInternalRouterServiceForIngress ensures that an internal service exists
-// for a given IngressController.
-func (r *reconciler) ensureInternalIngressControllerService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (*corev1.Service, error) {
+// for a given IngressController.  Returns a Boolean indicating whether the
+// service exists, the current service if it does exist, and an error value.
+func (r *reconciler) ensureInternalIngressControllerService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (bool, *corev1.Service, error) {
 	desired := desiredInternalIngressControllerService(ic, deploymentRef)
-	current, err := r.currentInternalIngressControllerService(ic)
+	have, current, err := r.currentInternalIngressControllerService(ic)
 	if err != nil {
-		return nil, err
+		return false, nil, err
 	}
-	if current != nil {
-		return current, nil
+	switch {
+	case !have:
+		if err := r.client.Create(context.TODO(), desired); err != nil {
+			return false, nil, fmt.Errorf("failed to create internal ingresscontroller service: %w", err)
+		}
+		log.Info("created internal ingresscontroller service", "service", desired)
+		return r.currentInternalIngressControllerService(ic)
+	case have:
+		if updated, err := r.updateInternalService(current, desired); err != nil {
+			return true, current, fmt.Errorf("failed to update internal service: %v", err)
+		} else if updated {
+			return r.currentInternalIngressControllerService(ic)
+		}
 	}
 
-	if err := r.client.Create(context.TODO(), desired); err != nil {
-		return nil, fmt.Errorf("failed to create internal ingresscontroller service: %v", err)
-	}
-	log.Info("created internal ingresscontroller service", "service", desired)
-	return desired, nil
+	return true, current, nil
 }
 
-func (r *reconciler) currentInternalIngressControllerService(ic *operatorv1.IngressController) (*corev1.Service, error) {
+func (r *reconciler) currentInternalIngressControllerService(ic *operatorv1.IngressController) (bool, *corev1.Service, error) {
 	current := &corev1.Service{}
 	err := r.client.Get(context.TODO(), controller.InternalIngressControllerServiceName(ic), current)
 	if err != nil {
 		if errors.IsNotFound(err) {
-			return nil, nil
+			return false, nil, nil
 		}
-		return nil, err
+		return false, nil, err
 	}
-	return current, nil
+	return true, current, nil
 }
 
 func desiredInternalIngressControllerService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) *corev1.Service {
@@ -74,3 +86,88 @@ func desiredInternalIngressControllerService(ic *operatorv1.IngressController, d
 
 	return s
 }
+
+// updateInternalService updates a ClusterIP service.  Returns a Boolean
+// indicating whether the service was updated, and an error value.
+func (r *reconciler) updateInternalService(current, desired *corev1.Service) (bool, error) {
+	changed, updated := internalServiceChanged(current, desired)
+	if !changed {
+		return false, nil
+	}
+
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return false, err
+	}
+	log.Info("updated internal service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
+	return true, nil
+}
+
+// managedInternalServiceAnnotations is a set of annotation keys for annotations
+// that the operator manages for its internal service.
+var managedInternalServiceAnnotations = sets.NewString(
+	ServingCertSecretAnnotation,
+)
+
+// internalServiceChanged checks if the current internal service annotations and
+// spec match the expected annotations and spec and if not returns an updated
+// service.
+func internalServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
+	changed := false
+
+	serviceCmpOpts := []cmp.Option{
+		// Ignore fields that the API, other controllers, or user may
+		// have modified.
+		cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"),
+		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", "ExternalIPs", "HealthCheckNodePort"),
+		cmp.Comparer(cmpServiceAffinity),
+		cmpopts.EquateEmpty(),
+	}
+	if !cmp.Equal(current.Spec, expected.Spec, serviceCmpOpts...) {
+		changed = true
+	}
+
+	annotationCmpOpts := []cmp.Option{
+		cmpopts.IgnoreMapEntries(func(k, _ string) bool {
+			return !managedInternalServiceAnnotations.Has(k)
+		}),
+	}
+	if !cmp.Equal(current.Annotations, expected.Annotations, annotationCmpOpts...) {
+		changed = true
+	}
+
+	if !changed {
+		return false, nil
+	}
+
+	updated := current.DeepCopy()
+	updated.Spec = expected.Spec
+
+	if updated.Annotations == nil {
+		updated.Annotations = map[string]string{}
+	}
+	for annotation := range managedInternalServiceAnnotations {
+		currentVal, have := current.Annotations[annotation]
+		expectedVal, want := expected.Annotations[annotation]
+		if want && (!have || currentVal != expectedVal) {
+			updated.Annotations[annotation] = expected.Annotations[annotation]
+		} else if have && !want {
+			delete(updated.Annotations, annotation)
+		}
+	}
+	// Preserve fields that the API, other controllers, or user may have
+	// modified.
+	updated.Spec.ClusterIP = current.Spec.ClusterIP
+	updated.Spec.ExternalIPs = current.Spec.ExternalIPs
+	updated.Spec.HealthCheckNodePort = current.Spec.HealthCheckNodePort
+	for i, updatedPort := range updated.Spec.Ports {
+		for _, currentPort := range current.Spec.Ports {
+			if currentPort.Name == updatedPort.Name {
+				updated.Spec.Ports[i].TargetPort = currentPort.TargetPort
+			}
+		}
+	}
+
+	return true, updated
+}