From ab53efa52d69e5687696cae12deacb3c68ce9120 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Tue, 31 Oct 2023 05:26:08 +0100 Subject: [PATCH] fix(k8s): fix VIPs configmap entries with invalid keys for ExternalName services (#8168) Services with the `ExternalName` type do not allow us to get an IP address, so the generated VIPs config map contains entries with invalid keys, such as `1:`. This was hard to debug because we were getting an EOF error during unmarshalling of the VIPs config map. This commit fixes the issue by ignoring services of that type with appropriate log when it'll happen. Signed-off-by: Bart Smykla --- pkg/dns/vips/interfaces.go | 9 ++- .../k8s/controllers/inbound_converter.go | 13 +++- .../k8s/controllers/kube_hosts_converter.go | 12 +++- .../k8s/controllers/outbound_converter.go | 25 +++++++- .../k8s/controllers/service_controller.go | 9 +++ .../controllers/service_controller_test.go | 32 ++++++++++ .../externalname-services.go | 62 +++++++++++++++++++ .../kubernetes/kubernetes_suite_test.go | 2 + 8 files changed, 156 insertions(+), 8 deletions(-) create mode 100644 test/e2e_env/kubernetes/externalname-services/externalname-services.go diff --git a/pkg/dns/vips/interfaces.go b/pkg/dns/vips/interfaces.go index c72b5ac64428..1f83003f1f77 100644 --- a/pkg/dns/vips/interfaces.go +++ b/pkg/dns/vips/interfaces.go @@ -2,6 +2,8 @@ package vips import ( "fmt" + + "github.com/pkg/errors" ) type EntryType int @@ -41,8 +43,11 @@ func (e HostnameEntry) MarshalText() ([]byte, error) { } func (e *HostnameEntry) UnmarshalText(text []byte) error { - _, err := fmt.Sscanf(string(text), "%v:%s", &e.Type, &e.Name) - return err + if _, err := fmt.Sscanf(string(text), "%v:%s", &e.Type, &e.Name); err != nil { + return errors.Wrapf(err, "could not parse hostname entry: %+q", text) + } + + return nil } func (e *HostnameEntry) Less(o *HostnameEntry) bool { diff --git a/pkg/plugins/runtime/k8s/controllers/inbound_converter.go b/pkg/plugins/runtime/k8s/controllers/inbound_converter.go index e48686481c62..a32bea502a0d 100644 --- a/pkg/plugins/runtime/k8s/controllers/inbound_converter.go +++ b/pkg/plugins/runtime/k8s/controllers/inbound_converter.go @@ -125,10 +125,17 @@ func inboundForServiceless(zone string, pod *kube_core.Pod, name string) *mesh_p } func (i *InboundConverter) InboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) { - ifaces := []*mesh_proto.Dataplane_Networking_Inbound{} + var ifaces []*mesh_proto.Dataplane_Networking_Inbound for _, svc := range services { - svcIfaces := inboundForService(zone, pod, svc) - ifaces = append(ifaces, svcIfaces...) + // Services of ExternalName type should not have any selectors. + // Kubernetes does not validate this, so in rare cases, a service of + // ExternalName type could point to a workload inside the mesh. If this + // happens, we would incorrectly generate inbounds including + // ExternalName service. We do not currently support ExternalName + // services, so we can safely skip them from processing. + if svc.Spec.Type != kube_core.ServiceTypeExternalName { + ifaces = append(ifaces, inboundForService(zone, pod, svc)...) + } } if len(ifaces) == 0 { diff --git a/pkg/plugins/runtime/k8s/controllers/kube_hosts_converter.go b/pkg/plugins/runtime/k8s/controllers/kube_hosts_converter.go index e6ebabf1437a..7fc351b9bf69 100644 --- a/pkg/plugins/runtime/k8s/controllers/kube_hosts_converter.go +++ b/pkg/plugins/runtime/k8s/controllers/kube_hosts_converter.go @@ -33,11 +33,21 @@ func KubeHosts( continue // one invalid Dataplane definition should not break the entire mesh } - // Do not generate outbounds for service-less + // Do not generate hostnames for service-less if isServiceLess(port) { continue } + // Do not generate hostnames for ExternalName Service + if isExternalNameService(service) { + converterLog.V(1).Info( + "ignoring hostnames generation for unsupported ExternalName Service", + "name", service.GetName(), + "namespace", service.GetNamespace(), + ) + continue + } + if isHeadlessService(service) { // Generate outbound listeners for every endpoint of services. for _, endpoint := range endpoints[serviceTag] { diff --git a/pkg/plugins/runtime/k8s/controllers/outbound_converter.go b/pkg/plugins/runtime/k8s/controllers/outbound_converter.go index beb16c1377bd..7cb51db43a80 100644 --- a/pkg/plugins/runtime/k8s/controllers/outbound_converter.go +++ b/pkg/plugins/runtime/k8s/controllers/outbound_converter.go @@ -27,7 +27,7 @@ func (p *PodConverter) OutboundInterfacesFor( reachableServicesMap[service] = true } - dataplanes := []*core_mesh.DataplaneResource{} + var dataplanes []*core_mesh.DataplaneResource for _, other := range others { dp := core_mesh.NewDataplaneResource() if err := p.ResourceConverter.ToCoreResource(other, dp); err != nil { @@ -53,6 +53,16 @@ func (p *PodConverter) OutboundInterfacesFor( continue } + // Do not generate hostnames for ExternalName Service + if isExternalNameService(service) { + converterLog.V(1).Info( + "ignoring outbound generation for unsupported ExternalName Service", + "name", service.GetName(), + "namespace", service.GetNamespace(), + ) + continue + } + if isHeadlessService(service) { // Generate outbound listeners for every endpoint of services. for _, endpoint := range endpoints[serviceTag] { @@ -83,7 +93,18 @@ func (p *PodConverter) OutboundInterfacesFor( } func isHeadlessService(svc *kube_core.Service) bool { - return svc.Spec.ClusterIP == "None" + return svc.Spec.ClusterIP == kube_core.ClusterIPNone +} + +// Services of ExternalName type should not have any selectors. +// Kubernetes does not validate this, so in rare cases, a service of +// ExternalName type could point to a workload inside the mesh. If this +// happens, we will add the service to the VIPs config map, but we will +// not be able to obtain its IP address. As a result, the key in the map +// will be incorrect (e.g., "1:"). We do not currently support +// ExternalName services, so we can safely skip them from processing. +func isExternalNameService(svc *kube_core.Service) bool { + return svc != nil && svc.Spec.Type == kube_core.ServiceTypeExternalName } func isServiceLess(port uint32) bool { diff --git a/pkg/plugins/runtime/k8s/controllers/service_controller.go b/pkg/plugins/runtime/k8s/controllers/service_controller.go index 1ce537551666..98bc1b39322a 100644 --- a/pkg/plugins/runtime/k8s/controllers/service_controller.go +++ b/pkg/plugins/runtime/k8s/controllers/service_controller.go @@ -58,6 +58,15 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req kube_ctrl.Request return kube_ctrl.Result{}, nil } + if svc.Spec.Type == kube_core.ServiceTypeExternalName { + log.V(1).Info( + "ignoring Service because it is of an unsupported type", + "name", req.NamespacedName.String(), + "type", kube_core.ServiceTypeExternalName, + ) + return kube_ctrl.Result{}, nil + } + log.Info("annotating service which is part of the mesh", "annotation", fmt.Sprintf("%s=%s", metadata.IngressServiceUpstream, metadata.AnnotationTrue)) annotations := metadata.Annotations(svc.Annotations) if annotations == nil { diff --git a/pkg/plugins/runtime/k8s/controllers/service_controller_test.go b/pkg/plugins/runtime/k8s/controllers/service_controller_test.go index 749839237f3f..1f60db72761d 100644 --- a/pkg/plugins/runtime/k8s/controllers/service_controller_test.go +++ b/pkg/plugins/runtime/k8s/controllers/service_controller_test.go @@ -90,6 +90,18 @@ var _ = Describe("ServiceReconciler", func() { }, Spec: kube_core.ServiceSpec{}, }, + &kube_core.Service{ + ObjectMeta: kube_meta.ObjectMeta{ + Namespace: "non-system-ns-with-sidecar-injection", + Name: "external-name", + Annotations: map[string]string{ + metadata.KumaSidecarInjectionAnnotation: metadata.AnnotationEnabled, + }, + }, + Spec: kube_core.ServiceSpec{ + Type: kube_core.ServiceTypeExternalName, + }, + }, &kube_core.Service{ ObjectMeta: kube_meta.ObjectMeta{ Namespace: "non-system-ns-with-sidecar-injection", @@ -145,6 +157,26 @@ var _ = Describe("ServiceReconciler", func() { Expect(svc.GetAnnotations()).ToNot(HaveKey(metadata.IngressServiceUpstream)) }) + It("should ignore service of ExternalName type", func() { + // given + req := kube_ctrl.Request{ + NamespacedName: kube_types.NamespacedName{Namespace: "non-system-ns-with-sidecar-injection", Name: "external-name"}, + } + + // when + result, err := reconciler.Reconcile(context.Background(), req) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(BeZero()) + + // and service is not annotated + svc := &kube_core.Service{} + err = kubeClient.Get(context.Background(), req.NamespacedName, svc) + Expect(err).ToNot(HaveOccurred()) + Expect(svc.GetAnnotations()).ToNot(HaveKey(metadata.IngressServiceUpstream)) + }) + It("should update service in an annotated namespace", func() { // given req := kube_ctrl.Request{ diff --git a/test/e2e_env/kubernetes/externalname-services/externalname-services.go b/test/e2e_env/kubernetes/externalname-services/externalname-services.go new file mode 100644 index 000000000000..64716e4073c0 --- /dev/null +++ b/test/e2e_env/kubernetes/externalname-services/externalname-services.go @@ -0,0 +1,62 @@ +package externalname_services + +import ( + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + . "github.com/kumahq/kuma/test/framework" + "github.com/kumahq/kuma/test/framework/deployments/testserver" + "github.com/kumahq/kuma/test/framework/envs/kubernetes" +) + +func ExternalNameServices() { + meshName := "externalname-services" + namespace := "externalname-services" + + externalNameService := fmt.Sprintf(` +apiVersion: v1 +kind: Service +metadata: + name: externalname-service + namespace: %s +spec: + type: ExternalName + externalName: foo.bar + ports: + - appProtocol: tcp + port: 3000 + protocol: TCP + targetPort: 3000 + selector: + app: test-server +`, namespace) + + BeforeAll(func() { + Expect(NewClusterSetup(). + Install(MeshKubernetes(meshName)). + Install(NamespaceWithSidecarInjection(namespace)). + Install(testserver.Install( + testserver.WithName("test-server"), + testserver.WithNamespace(namespace), + testserver.WithMesh(meshName), + )). + Setup(kubernetes.Cluster), + ).To(Succeed()) + }) + + E2EAfterAll(func() { + Expect(kubernetes.Cluster.TriggerDeleteNamespace(namespace)).To(Succeed()) + Expect(kubernetes.Cluster.DeleteMesh(meshName)).To(Succeed()) + }) + + It("should ignore ExternalName Service", func() { + // when + Expect(kubernetes.Cluster.Install(YamlK8s(externalNameService))).To(Succeed()) + + // then + Consistently(kubernetes.Cluster.GetKumaCPLogs, "10s", "1s"). + ShouldNot(ContainSubstring("could not parse hostname entry")) + }) +} diff --git a/test/e2e_env/kubernetes/kubernetes_suite_test.go b/test/e2e_env/kubernetes/kubernetes_suite_test.go index 25654686b8d4..8221c0966dc5 100644 --- a/test/e2e_env/kubernetes/kubernetes_suite_test.go +++ b/test/e2e_env/kubernetes/kubernetes_suite_test.go @@ -10,6 +10,7 @@ import ( "github.com/kumahq/kuma/test/e2e_env/kubernetes/connectivity" "github.com/kumahq/kuma/test/e2e_env/kubernetes/container_patch" "github.com/kumahq/kuma/test/e2e_env/kubernetes/defaults" + "github.com/kumahq/kuma/test/e2e_env/kubernetes/externalname-services" "github.com/kumahq/kuma/test/e2e_env/kubernetes/externalservices" "github.com/kumahq/kuma/test/e2e_env/kubernetes/gateway" "github.com/kumahq/kuma/test/e2e_env/kubernetes/graceful" @@ -68,6 +69,7 @@ var ( _ = Describe("Defaults", defaults.Defaults, Ordered) _ = Describe("External Services", externalservices.ExternalServices, Ordered) _ = Describe("External Services Permissive MTLS", externalservices.PermissiveMTLS, Ordered) + _ = Describe("ExternalName Services", externalname_services.ExternalNameServices, Ordered) _ = Describe("Virtual Outbound", virtualoutbound.VirtualOutbound, Ordered) _ = Describe("Kong Ingress Controller", Label("arm-not-supported"), kic.KICKubernetes, Ordered) _ = Describe("MeshTrafficPermission API", meshtrafficpermission.API, Ordered)