diff --git a/assets/router/service-internal.yaml b/assets/router/service-internal.yaml index 4f5b95adc..675e9d274 100644 --- a/assets/router/service-internal.yaml +++ b/assets/router/service-internal.yaml @@ -17,4 +17,4 @@ spec: - name: metrics port: 1936 protocol: TCP - targetPort: 1936 + targetPort: metrics diff --git a/pkg/manifests/bindata.go b/pkg/manifests/bindata.go index 58df07193..c117cf918 100644 --- a/pkg/manifests/bindata.go +++ b/pkg/manifests/bindata.go @@ -14,7 +14,7 @@ // assets/router/namespace.yaml (858B) // assets/router/service-account.yaml (213B) // assets/router/service-cloud.yaml (631B) -// assets/router/service-internal.yaml (429B) +// assets/router/service-internal.yaml (432B) // manifests/00-cluster-role.yaml (3.181kB) // manifests/00-custom-resource-definition-internal.yaml (7.756kB) // manifests/00-custom-resource-definition.yaml (101.564kB) @@ -382,7 +382,7 @@ func assetsRouterServiceCloudYaml() (*asset, error) { return a, nil } -var _assetsRouterServiceInternalYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x8c\xcf\x31\x6b\xfb\x30\x10\x05\xf0\x5d\x9f\xe2\x41\xd6\xff\xbf\x34\x24\x94\x56\xab\xa7\x6c\x86\x96\xee\x87\x7c\x49\x8e\xca\x92\xb8\x3b\xbb\xf4\xdb\x97\x38\x0d\xb8\x74\xc9\x22\x90\xee\xe9\xf7\xb8\x0d\xba\x3c\x99\xb3\xe2\x95\x75\x96\xc4\xf8\x14\x3f\x63\xe0\x23\x4d\xd9\x31\x53\x9e\xd8\xc2\x06\x87\x72\x52\x36\x43\x57\x8b\x6b\xcd\x99\x15\xd6\x38\xc9\x51\x12\xa8\x94\xea\xe4\x52\x8b\x81\x94\x41\xad\x65\xe1\x01\xe4\xd0\xa9\xb8\x8c\xfc\x10\x3e\xa4\x0c\xf1\xd6\x11\xa8\xc9\x3b\xab\x49\x2d\x11\xf3\x36\x6c\x50\x68\xe4\x7f\xcb\x69\x8d\x12\x83\xca\xf0\x87\x35\xf6\x5f\xe4\xa5\x3f\x06\xc0\xbf\x1a\xc7\xdb\x1a\x87\x3e\x00\xad\xaa\xdb\x65\xf4\x7f\x21\x23\xce\xee\x2d\x00\xd7\x49\xc4\xf3\xe3\xf5\xa2\xd5\x6b\xaa\x39\xe2\xad\xeb\x97\x17\x27\x3d\xb1\xf7\x4b\xe8\xe7\xcf\x9a\xb0\x95\xb1\xdf\xef\xee\x44\x6c\xa5\x8c\xec\x2a\x69\xed\x6c\x5f\x76\x4f\x77\x40\x4b\xec\x3b\x00\x00\xff\xff\x90\x5e\x33\xca\xad\x01\x00\x00") +var _assetsRouterServiceInternalYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x8c\xcf\x31\x6b\xfb\x30\x10\x05\xf0\x5d\x9f\xe2\x41\xd6\xff\xbf\x34\x24\x94\x56\x6b\xa6\x6c\x86\x96\xee\x87\x7c\x49\x8e\xca\x92\xb8\x3b\xbb\xf4\xdb\x97\x38\x35\xb8\x74\xc9\x22\x90\x4e\xef\xf7\xb8\x0d\x0e\x79\x34\x67\xc5\x2b\xeb\x24\x89\xf1\x29\x7e\x41\xcf\x27\x1a\xb3\x63\xa2\x3c\xb2\x85\x0d\x8e\xe5\xac\x6c\x86\x43\x2d\xae\x35\x67\x56\x58\xe3\x24\x27\x49\xa0\x52\xaa\x93\x4b\x2d\x06\x52\x06\xb5\x96\x85\x7b\x90\x43\xc7\xe2\x32\xf0\x43\xf8\x90\xd2\xc7\xa5\x23\x50\x93\x77\x56\x93\x5a\x22\xa6\x6d\xd8\xa0\xd0\xc0\xff\xe6\xd3\x1a\x25\x06\x95\xfe\x0f\x6b\xec\xbf\xc8\x6b\x7f\x0c\x80\x7f\x35\x8e\xcb\x1a\xc7\x2e\x00\xad\xaa\xdb\x75\xf4\x7f\x26\x23\x2e\xee\x2d\x00\xb7\x49\xc4\xf3\xe3\xed\xa2\xd5\x6b\xaa\x39\xe2\xed\xd0\xcd\x2f\x4e\x7a\x66\xef\xe6\x4f\x3f\x99\x35\x61\x2b\x63\xbf\xdf\xdd\x89\xd8\x4a\x19\xd8\x55\xd2\xda\xd9\xbe\xec\x9e\xee\x80\x96\xe0\x77\x00\x00\x00\xff\xff\x4c\x3f\xad\x47\xb0\x01\x00\x00") func assetsRouterServiceInternalYamlBytes() ([]byte, error) { return bindataRead( @@ -397,8 +397,8 @@ func assetsRouterServiceInternalYaml() (*asset, error) { return nil, err } - info := bindataFileInfo{name: "assets/router/service-internal.yaml", size: 429, mode: os.FileMode(420), modTime: time.Unix(1, 0)} - a := &asset{bytes: bytes, info: info, digest: [32]uint8{0xb8, 0x63, 0x52, 0x85, 0x8b, 0x99, 0xe6, 0xc7, 0xcb, 0x34, 0x3d, 0x8d, 0x43, 0x65, 0x10, 0x63, 0x51, 0x80, 0xc1, 0x29, 0x17, 0xb6, 0x8f, 0x84, 0xdc, 0xf8, 0x33, 0xa1, 0x21, 0xc2, 0x5a, 0x4f}} + info := bindataFileInfo{name: "assets/router/service-internal.yaml", size: 432, mode: os.FileMode(420), modTime: time.Unix(1, 0)} + a := &asset{bytes: bytes, info: info, digest: [32]uint8{0x72, 0x14, 0xe2, 0x6f, 0x8f, 0xec, 0x74, 0xa4, 0xd0, 0x80, 0x68, 0x9f, 0x6e, 0xbf, 0x5e, 0x81, 0x88, 0xae, 0xfe, 0xf6, 0x98, 0x8a, 0xc3, 0x57, 0x9d, 0x68, 0x65, 0x25, 0x6f, 0x10, 0xa5, 0x97}} return a, nil } diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 140b5a9b0..017892367 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/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)) } diff --git a/pkg/operator/controller/ingress/internal_service.go b/pkg/operator/controller/ingress/internal_service.go index 96c775ebd..ee4b224f0 100644 --- a/pkg/operator/controller/ingress/internal_service.go +++ b/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 +} diff --git a/pkg/operator/controller/ingress/internal_service_test.go b/pkg/operator/controller/ingress/internal_service_test.go new file mode 100644 index 000000000..8187b9481 --- /dev/null +++ b/pkg/operator/controller/ingress/internal_service_test.go @@ -0,0 +1,212 @@ +package ingress + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + operatorv1 "github.com/openshift/api/operator/v1" + + corev1 "k8s.io/api/core/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +// test_desiredInternalIngressControllerService verifies that +// desiredInternalIngressControllerService returns the expected service. +func Test_desiredInternalIngressControllerService(t *testing.T) { + ic := &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + } + trueVar := true + deploymentRef := metav1.OwnerReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "router-default", + UID: "1", + Controller: &trueVar, + } + + svc := desiredInternalIngressControllerService(ic, deploymentRef) + + assert.Equal(t, "ClusterIP", string(svc.Spec.Type)) + assert.Equal(t, map[string]string{ + "service.alpha.openshift.io/serving-cert-secret-name": "router-metrics-certs-default", + }, svc.Annotations) + assert.Equal(t, []corev1.ServicePort{{ + Name: "http", + Protocol: "TCP", + Port: int32(80), + TargetPort: intstr.FromString("http"), + }, { + Name: "https", + Protocol: "TCP", + Port: int32(443), + TargetPort: intstr.FromString("https"), + }, { + Name: "metrics", + Protocol: "TCP", + Port: int32(1936), + TargetPort: intstr.FromString("metrics"), + }}, svc.Spec.Ports) +} + +// Test_internalServiceChanged verifies that internalServiceChanged properly +// detects changes and handles updates. +func Test_internalServiceChanged(t *testing.T) { + testCases := []struct { + description string + mutate func(*corev1.Service) + expect bool + }{ + { + description: "if nothing changes", + mutate: func(_ *corev1.Service) {}, + expect: false, + }, + { + description: "if .uid changes", + mutate: func(svc *corev1.Service) { + svc.UID = "2" + }, + expect: false, + }, + { + description: "if .spec.clusterIP changes", + mutate: func(svc *corev1.Service) { + svc.Spec.ClusterIP = "2.3.4.5" + svc.Spec.ClusterIPs = []string{"2.3.4.5"} + }, + expect: false, + }, + { + description: "if .spec.externalIPs changes", + mutate: func(svc *corev1.Service) { + svc.Spec.ExternalIPs = []string{"3.4.5.6"} + }, + expect: false, + }, + { + description: "if the service.alpha.openshift.io/serving-cert-secret-name annotation changes", + mutate: func(svc *corev1.Service) { + svc.Annotations["service.alpha.openshift.io/serving-cert-secret-name"] = "x" + }, + expect: true, + }, + { + description: "if the service.alpha.openshift.io/serving-cert-secret-name annotation is deleted", + mutate: func(svc *corev1.Service) { + delete(svc.Annotations, "service.alpha.openshift.io/serving-cert-secret-name") + }, + expect: true, + }, + { + description: "if .spec.ports changes by adding a new port", + mutate: func(svc *corev1.Service) { + newPort := corev1.ServicePort{ + Name: "foo", + Protocol: corev1.ProtocolTCP, + Port: int32(8080), + TargetPort: intstr.FromString("foo"), + } + svc.Spec.Ports = append(svc.Spec.Ports, newPort) + }, + expect: true, + }, + { + description: "if .spec.ports changes by changing the metrics port's target port from an integer to a string", + mutate: func(svc *corev1.Service) { + for i := range svc.Spec.Ports { + if svc.Spec.Ports[i].Name == "metrics" { + svc.Spec.Ports[i].TargetPort = intstr.FromString("metrics") + return + } + } + panic("no metrics port found!") + }, + expect: true, + }, + { + description: "if .spec.selector changes", + mutate: func(svc *corev1.Service) { + svc.Spec.Selector = nil + }, + expect: true, + }, + { + description: "if .spec.sessionAffinity is defaulted", + mutate: func(service *corev1.Service) { + service.Spec.SessionAffinity = corev1.ServiceAffinityNone + }, + expect: false, + }, + { + description: "if .spec.sessionAffinity is set to a non-default value", + mutate: func(service *corev1.Service) { + service.Spec.SessionAffinity = corev1.ServiceAffinityClientIP + }, + expect: true, + }, + { + description: "if .spec.type changes", + mutate: func(svc *corev1.Service) { + svc.Spec.Type = corev1.ServiceTypeLoadBalancer + }, + expect: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + original := corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.alpha.openshift.io/serving-cert-secret-name": "router-metrics-certs-default", + }, + Namespace: "openshift-ingress", + Name: "router-internal-default", + UID: "1", + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "1.2.3.4", + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: int32(80), + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromString("http"), + }, + { + Name: "https", + Port: int32(443), + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromString("https"), + }, + { + Name: "metrics", + Port: int32(1936), + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(1936), + }, + }, + Selector: map[string]string{ + "foo": "bar", + }, + Type: corev1.ServiceTypeClusterIP, + }, + } + mutated := original.DeepCopy() + tc.mutate(mutated) + if changed, updated := internalServiceChanged(&original, mutated); changed != tc.expect { + t.Errorf("expected internalServiceChanged to be %t, got %t", tc.expect, changed) + } else if changed { + if changedAgain, _ := internalServiceChanged(mutated, updated); changedAgain { + t.Error("internalServiceChanged does not behave as a fixed point function") + } + } + }) + } +}