From b87f00cefb7a88020c96f64fc7b0acdf939bb621 Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Mon, 2 Aug 2021 12:47:45 -0600 Subject: [PATCH] connect: skip service registration only for duplicate services that are on k8s (#581) Previously, we would skip service registration if we find a duplicate service even when the 'k8s-namespace' meta key is not present on the service. However, that is not correct behavior since that would ignore any service instance that could exist on other platforms (e.g. VMs) and that doesn't have this meta key. We should instead only check services that have the 'k8s-namespace' meta key. --- CHANGELOG.md | 1 + connect-inject/endpoints_controller.go | 4 +- connect-inject/endpoints_controller_test.go | 147 +++++++++----------- 3 files changed, 69 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 454786554a..2a75143041 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ BUG FIXES: `_CONNECT_SERVICE_PORT` weren't being set when the upstream annotation was used. [[GH-549](https://github.com/hashicorp/consul-k8s/issues/549)] * Connect: Fix a bug with leaving around ACL tokens after a service has been deregistered. [[GH-571](https://github.com/hashicorp/consul-k8s/issues/540)] * CRDs: Fix ProxyDefaults and ServiceDefaults resources not syncing with Consul < 1.10.0 [[GH-1023](https://github.com/hashicorp/consul-helm/issues/1023)] +* Connect: Skip service registration for duplicate services only on Kubernetes. [[GH-581](https://github.com/hashicorp/consul-k8s/pull/581)] ## 0.26.0 (June 22, 2021) diff --git a/connect-inject/endpoints_controller.go b/connect-inject/endpoints_controller.go index 3f47fe03a9..693b0a25a8 100644 --- a/connect-inject/endpoints_controller.go +++ b/connect-inject/endpoints_controller.go @@ -235,13 +235,13 @@ func (r *EndpointsController) registerServicesAndHealthCheck(ctx context.Context return err } for _, service := range services { - if service.ServiceMeta[MetaKeyKubeNS] != serviceEndpoints.Namespace { + if existingNS, ok := service.ServiceMeta[MetaKeyKubeNS]; ok && existingNS != serviceEndpoints.Namespace { // Log but don't return an error because we don't want to reconcile this endpoints object again. r.Log.Info("Skipping service registration because a service with the same name "+ "but a different Kubernetes namespace is already registered with Consul", "name", serviceRegistration.Name, MetaKeyKubeNS, serviceEndpoints.Namespace, - "existing-k8s-namespace", service.ServiceMeta[MetaKeyKubeNS]) + "existing-k8s-namespace", existingNS) return nil } } diff --git a/connect-inject/endpoints_controller_test.go b/connect-inject/endpoints_controller_test.go index e1b41b3c8d..0afeb2aada 100644 --- a/connect-inject/endpoints_controller_test.go +++ b/connect-inject/endpoints_controller_test.go @@ -4657,94 +4657,79 @@ func TestCreateServiceRegistrations_withTransparentProxy(t *testing.T) { func TestRegisterServicesAndHealthCheck_skipsWhenDuplicateServiceFound(t *testing.T) { t.Parallel() - cases := map[string]struct { - consulServiceMeta map[string]string - }{ - "different k8s namespace meta": { - consulServiceMeta: map[string]string{MetaKeyKubeNS: "some-other-ns"}, - }, - "no k8s namespace meta": { - consulServiceMeta: nil, - }, - } - - for name, c := range cases { - t.Run(name, func(t *testing.T) { - nodeName := "test-node" - consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { - c.NodeName = nodeName - }) - require.NoError(t, err) - defer consul.Stop() + nodeName := "test-node" + consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.NodeName = nodeName + }) + require.NoError(t, err) + defer consul.Stop() - consul.WaitForServiceIntentions(t) - httpAddr := consul.HTTPAddr - clientConfig := &api.Config{ - Address: httpAddr, - } - consulClient, err := api.NewClient(clientConfig) - require.NoError(t, err) - addr := strings.Split(httpAddr, ":") - consulPort := addr[1] + consul.WaitForServiceIntentions(t) + httpAddr := consul.HTTPAddr + clientConfig := &api.Config{ + Address: httpAddr, + } + consulClient, err := api.NewClient(clientConfig) + require.NoError(t, err) + addr := strings.Split(httpAddr, ":") + consulPort := addr[1] - existingService := &api.AgentServiceRegistration{ - ID: "test-service", - Name: "test-service", - Port: 1234, - Address: "1.2.3.4", - Meta: c.consulServiceMeta, - } - err = consulClient.Agent().ServiceRegister(existingService) - require.NoError(t, err) - pod := createPod("test-pod", "1.1.1.1", true, true) + existingService := &api.AgentServiceRegistration{ + ID: "test-service", + Name: "test-service", + Port: 1234, + Address: "1.2.3.4", + Meta: map[string]string{MetaKeyKubeNS: "some-other-ns"}, + } + err = consulClient.Agent().ServiceRegister(existingService) + require.NoError(t, err) + pod := createPod("test-pod", "1.1.1.1", true, true) - endpointsAddress := corev1.EndpointAddress{ - IP: "1.2.3.4", - NodeName: &nodeName, - TargetRef: &corev1.ObjectReference{ - Kind: "Pod", - Name: pod.Name, - Namespace: pod.Namespace, - }, - } - endpoints := &corev1.Endpoints{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-service", - Namespace: "default", - }, - Subsets: []corev1.EndpointSubset{ - { - Addresses: []corev1.EndpointAddress{endpointsAddress}, - }, - }, - } - ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} - fakeClient := fake.NewClientBuilder().WithRuntimeObjects(ns, pod, endpoints).Build() + endpointsAddress := corev1.EndpointAddress{ + IP: "1.2.3.4", + NodeName: &nodeName, + TargetRef: &corev1.ObjectReference{ + Kind: "Pod", + Name: pod.Name, + Namespace: pod.Namespace, + }, + } + endpoints := &corev1.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + }, + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{endpointsAddress}, + }, + }, + } + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + fakeClient := fake.NewClientBuilder().WithRuntimeObjects(ns, pod, endpoints).Build() - ep := &EndpointsController{ - Log: logrtest.TestLogger{T: t}, - ConsulClient: consulClient, - ConsulPort: consulPort, - ConsulScheme: "http", - ConsulClientCfg: clientConfig, - AllowK8sNamespacesSet: mapset.NewSetWith("*"), - DenyK8sNamespacesSet: mapset.NewSetWith(), - Client: fakeClient, - } + ep := &EndpointsController{ + Log: logrtest.TestLogger{T: t}, + ConsulClient: consulClient, + ConsulPort: consulPort, + ConsulScheme: "http", + ConsulClientCfg: clientConfig, + AllowK8sNamespacesSet: mapset.NewSetWith("*"), + DenyK8sNamespacesSet: mapset.NewSetWith(), + Client: fakeClient, + } - err = ep.registerServicesAndHealthCheck(context.Background(), *endpoints, endpointsAddress, api.HealthPassing, make(map[string]bool)) - require.NoError(t, err) + err = ep.registerServicesAndHealthCheck(context.Background(), *endpoints, endpointsAddress, api.HealthPassing, make(map[string]bool)) + require.NoError(t, err) - // Check that the service is not registered with Consul. - _, _, err = consulClient.Agent().Service("test-pod-test-service", nil) - require.Error(t, err) - require.Contains(t, err.Error(), "Unexpected response code: 404 (unknown service ID") + // Check that the service is not registered with Consul. + _, _, err = consulClient.Agent().Service("test-pod-test-service", nil) + require.Error(t, err) + require.Contains(t, err.Error(), "Unexpected response code: 404 (unknown service ID") - _, _, err = consulClient.Agent().Service("test-pod-test-service-sidecar-proxy", nil) - require.Error(t, err) - require.Contains(t, err.Error(), "Unexpected response code: 404 (unknown service ID") - }) - } + _, _, err = consulClient.Agent().Service("test-pod-test-service-sidecar-proxy", nil) + require.Error(t, err) + require.Contains(t, err.Error(), "Unexpected response code: 404 (unknown service ID") } func TestGetTokenMetaFromDescription(t *testing.T) {