Skip to content

Commit

Permalink
connect: skip service registration only for duplicate services that a…
Browse files Browse the repository at this point in the history 
…re on k8s

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.
  • Loading branch information
@ishustava
ishustava committed Aug 2, 2021
1 parent aa268b2 commit af0b16f
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 83 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ BUG FIXES:
`<NAME>_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)

Expand Down
4 changes: 2 additions & 2 deletions connect-inject/endpoints_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
147 changes: 66 additions & 81 deletions connect-inject/endpoints_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit af0b16f

Please sign in to comment.