diff --git a/cmd/k8s-operator/egress-services.go b/cmd/k8s-operator/egress-services.go index 98ed94366..a562f0170 100644 --- a/cmd/k8s-operator/egress-services.go +++ b/cmd/k8s-operator/egress-services.go @@ -136,9 +136,8 @@ func (esr *egressSvcsReconciler) Reconcile(ctx context.Context, req reconcile.Re } if !slices.Contains(svc.Finalizers, FinalizerName) { - l.Infof("configuring tailnet service") // logged exactly once svc.Finalizers = append(svc.Finalizers, FinalizerName) - if err := esr.Update(ctx, svc); err != nil { + if err := esr.updateSvcSpec(ctx, svc); err != nil { err := fmt.Errorf("failed to add finalizer: %w", err) r := svcConfiguredReason(svc, false, l) tsoperator.SetServiceCondition(svc, tsapi.EgressSvcConfigured, metav1.ConditionFalse, r, err.Error(), esr.clock, l) @@ -198,7 +197,7 @@ func (esr *egressSvcsReconciler) maybeProvision(ctx context.Context, svc *corev1 if svc.Spec.ExternalName != clusterIPSvcFQDN { l.Infof("Configuring ExternalName Service to point to ClusterIP Service %s", clusterIPSvcFQDN) svc.Spec.ExternalName = clusterIPSvcFQDN - if err = esr.Update(ctx, svc); err != nil { + if err = esr.updateSvcSpec(ctx, svc); err != nil { err = fmt.Errorf("error updating ExternalName Service: %w", err) return err } @@ -222,6 +221,15 @@ func (esr *egressSvcsReconciler) provision(ctx context.Context, proxyGroupName s found := false for _, wantsPM := range svc.Spec.Ports { if wantsPM.Port == pm.Port && strings.EqualFold(string(wantsPM.Protocol), string(pm.Protocol)) { + // We don't use the port name to distinguish this port internally, but Kubernetes + // require that, for Service ports with more than one name each port is uniquely named. + // So we can always pick the port name from the ExternalName Service as at this point we + // know that those are valid names because Kuberentes already validated it once. Note + // that users could have changed an unnamed port to a named port and might have changed + // port names- this should still work. + // https://kubernetes.io/docs/concepts/services-networking/service/#multi-port-services + // See also https://github.com/tailscale/tailscale/issues/13406#issuecomment-2507230388 + clusterIPSvc.Spec.Ports[i].Name = wantsPM.Name found = true break } @@ -714,3 +722,13 @@ func epsPortsFromSvc(svc *corev1.Service) (ep []discoveryv1.EndpointPort) { } return ep } + +// updateSvcSpec ensures that the given Service's spec is updated in cluster, but the local Service object still retains +// the not-yet-applied status. +// TODO(irbekrm): once we do SSA for these patch updates, this will no longer be needed. +func (esr *egressSvcsReconciler) updateSvcSpec(ctx context.Context, svc *corev1.Service) error { + st := svc.Status.DeepCopy() + err := esr.Update(ctx, svc) + svc.Status = *st + return err +} diff --git a/cmd/k8s-operator/egress-services_test.go b/cmd/k8s-operator/egress-services_test.go index ac7733985..06fe977ec 100644 --- a/cmd/k8s-operator/egress-services_test.go +++ b/cmd/k8s-operator/egress-services_test.go @@ -105,28 +105,40 @@ func TestTailscaleEgressServices(t *testing.T) { condition(tsapi.ProxyGroupReady, metav1.ConditionTrue, "", "", clock), } }) - // Quirks of the fake client. - mustUpdateStatus(t, fc, "default", "test", func(svc *corev1.Service) { - svc.Status.Conditions = []metav1.Condition{} + expectReconciled(t, esr, "default", "test") + validateReadyService(t, fc, esr, svc, clock, zl, cm) + }) + t.Run("service_retain_one_unnamed_port", func(t *testing.T) { + svc.Spec.Ports = []corev1.ServicePort{{Protocol: "TCP", Port: 80}} + mustUpdate(t, fc, "default", "test", func(s *corev1.Service) { + s.Spec.Ports = svc.Spec.Ports }) expectReconciled(t, esr, "default", "test") - // Verify that a ClusterIP Service has been created. - name := findGenNameForEgressSvcResources(t, fc, svc) - expectEqual(t, fc, clusterIPSvc(name, svc), removeTargetPortsFromSvc) - clusterSvc := mustGetClusterIPSvc(t, fc, name) - // Verify that an EndpointSlice has been created. - expectEqual(t, fc, endpointSlice(name, svc, clusterSvc), nil) - // Verify that ConfigMap contains configuration for the new egress service. - mustHaveConfigForSvc(t, fc, svc, clusterSvc, cm) - r := svcConfiguredReason(svc, true, zl.Sugar()) - // Verify that the user-created ExternalName Service has Configured set to true and ExternalName pointing to the - // CluterIP Service. - svc.Status.Conditions = []metav1.Condition{ - condition(tsapi.EgressSvcConfigured, metav1.ConditionTrue, r, r, clock), - } - svc.ObjectMeta.Finalizers = []string{"tailscale.com/finalizer"} - svc.Spec.ExternalName = fmt.Sprintf("%s.operator-ns.svc.cluster.local", name) - expectEqual(t, fc, svc, nil) + validateReadyService(t, fc, esr, svc, clock, zl, cm) + }) + t.Run("service_add_two_named_ports", func(t *testing.T) { + svc.Spec.Ports = []corev1.ServicePort{{Protocol: "TCP", Port: 80, Name: "http"}, {Protocol: "TCP", Port: 443, Name: "https"}} + mustUpdate(t, fc, "default", "test", func(s *corev1.Service) { + s.Spec.Ports = svc.Spec.Ports + }) + expectReconciled(t, esr, "default", "test") + validateReadyService(t, fc, esr, svc, clock, zl, cm) + }) + t.Run("service_add_udp_port", func(t *testing.T) { + svc.Spec.Ports = append(svc.Spec.Ports, corev1.ServicePort{Port: 53, Protocol: "UDP", Name: "dns"}) + mustUpdate(t, fc, "default", "test", func(s *corev1.Service) { + s.Spec.Ports = svc.Spec.Ports + }) + expectReconciled(t, esr, "default", "test") + validateReadyService(t, fc, esr, svc, clock, zl, cm) + }) + t.Run("service_change_protocol", func(t *testing.T) { + svc.Spec.Ports = []corev1.ServicePort{{Protocol: "TCP", Port: 80, Name: "http"}, {Protocol: "TCP", Port: 443, Name: "https"}, {Port: 53, Protocol: "TCP", Name: "tcp_dns"}} + mustUpdate(t, fc, "default", "test", func(s *corev1.Service) { + s.Spec.Ports = svc.Spec.Ports + }) + expectReconciled(t, esr, "default", "test") + validateReadyService(t, fc, esr, svc, clock, zl, cm) }) t.Run("delete_external_name_service", func(t *testing.T) { @@ -143,6 +155,29 @@ func TestTailscaleEgressServices(t *testing.T) { }) } +func validateReadyService(t *testing.T, fc client.WithWatch, esr *egressSvcsReconciler, svc *corev1.Service, clock *tstest.Clock, zl *zap.Logger, cm *corev1.ConfigMap) { + expectReconciled(t, esr, "default", "test") + // Verify that a ClusterIP Service has been created. + name := findGenNameForEgressSvcResources(t, fc, svc) + expectEqual(t, fc, clusterIPSvc(name, svc), removeTargetPortsFromSvc) + clusterSvc := mustGetClusterIPSvc(t, fc, name) + // Verify that an EndpointSlice has been created. + expectEqual(t, fc, endpointSlice(name, svc, clusterSvc), nil) + // Verify that ConfigMap contains configuration for the new egress service. + mustHaveConfigForSvc(t, fc, svc, clusterSvc, cm) + r := svcConfiguredReason(svc, true, zl.Sugar()) + // Verify that the user-created ExternalName Service has Configured set to true and ExternalName pointing to the + // CluterIP Service. + svc.Status.Conditions = []metav1.Condition{ + condition(tsapi.EgressSvcValid, metav1.ConditionTrue, "EgressSvcValid", "EgressSvcValid", clock), + condition(tsapi.EgressSvcConfigured, metav1.ConditionTrue, r, r, clock), + } + svc.ObjectMeta.Finalizers = []string{"tailscale.com/finalizer"} + svc.Spec.ExternalName = fmt.Sprintf("%s.operator-ns.svc.cluster.local", name) + expectEqual(t, fc, svc, nil) + +} + func condition(typ tsapi.ConditionType, st metav1.ConditionStatus, r, msg string, clock tstime.Clock) metav1.Condition { return metav1.Condition{ Type: string(typ), diff --git a/cmd/k8s-operator/testutils_test.go b/cmd/k8s-operator/testutils_test.go index 084f573e5..5795a0aae 100644 --- a/cmd/k8s-operator/testutils_test.go +++ b/cmd/k8s-operator/testutils_test.go @@ -650,7 +650,7 @@ func removeHashAnnotation(sts *appsv1.StatefulSet) { func removeTargetPortsFromSvc(svc *corev1.Service) { newPorts := make([]corev1.ServicePort, 0) for _, p := range svc.Spec.Ports { - newPorts = append(newPorts, corev1.ServicePort{Protocol: p.Protocol, Port: p.Port}) + newPorts = append(newPorts, corev1.ServicePort{Protocol: p.Protocol, Port: p.Port, Name: p.Name}) } svc.Spec.Ports = newPorts }