From 42c997126c461f00e368c527441c4b7b2323e4ff Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 11 Nov 2021 13:44:41 -0500 Subject: [PATCH 01/37] connect-ignore deregisters or ignores service endpoint --- control-plane/connect-inject/endpoints_controller.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 4e6a2b73ba..020f695952 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -39,6 +39,7 @@ const ( kubernetesSuccessReasonMsg = "Kubernetes health checks passing" envoyPrometheusBindAddr = "envoy_prometheus_bind_addr" envoySidecarContainer = "envoy-sidecar" + connectIgnoreLabel = "consul.hashicorp.com/connect-ignore" // clusterIPTaggedAddressName is the key for the tagged address to store the service's cluster IP and service port // in Consul. Note: This value should not be changed without a corresponding change in Consul. @@ -146,6 +147,13 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } + // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. + // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. + if value, ok := serviceEndpoints.Labels[connectIgnoreLabel]; ok && value == "true" { + err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) + return ctrl.Result{}, err + } + r.Log.Info("retrieved", "name", serviceEndpoints.Name, "ns", serviceEndpoints.Namespace) // endpointAddressMap stores every IP that corresponds to a Pod in the Endpoints object. It is used to compare From b8501ca0aeda8dad3f5cd9fbe3daa0e030d2b7a1 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 11 Nov 2021 18:38:44 -0500 Subject: [PATCH 02/37] Add some comments as a clean up --- control-plane/connect-inject/endpoints_controller.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 020f695952..1d3df19f72 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -118,10 +118,14 @@ type EndpointsController struct { context.Context } +// Reconcile reads the state of the cluster's service endpoints when a request is received and makes changes +// to the endpoints based on the state read. Requests are created when the endpoints are created, updated, +// or deleted. func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { var errs error var serviceEndpoints corev1.Endpoints + // Ignore the request if the namespace of the endpoint is not allowed. if shouldIgnore(req.Namespace, r.DenyK8sNamespacesSet, r.AllowK8sNamespacesSet) { return ctrl.Result{}, nil } From df488f12a5976bf7fefd469dd895f998cd526304 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 11 Nov 2021 18:46:33 -0500 Subject: [PATCH 03/37] Refactor deregisterServiceOnAllAgents to return the same as Reconcile --- .../connect-inject/endpoints_controller.go | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 1d3df19f72..20666a795e 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -142,10 +142,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( if k8serrors.IsNotFound(err) { // Deregister all instances in Consul for this service. The function deregisterServiceOnAllAgents handles // the case where the Consul service name is different from the Kubernetes service name. - if err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) } else if err != nil { r.Log.Error(err, "failed to get Endpoints", "name", req.Name, "ns", req.Namespace) return ctrl.Result{}, err @@ -154,8 +151,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if value, ok := serviceEndpoints.Labels[connectIgnoreLabel]; ok && value == "true" { - err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) - return ctrl.Result{}, err + return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) } r.Log.Info("retrieved", "name", serviceEndpoints.Name, "ns", serviceEndpoints.Namespace) @@ -190,7 +186,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // Compare service instances in Consul with addresses in Endpoints. If an address is not in Endpoints, deregister // from Consul. This uses endpointAddressMap which is populated with the addresses in the Endpoints object during // the registration codepath. - if err = r.deregisterServiceOnAllAgents(ctx, serviceEndpoints.Name, serviceEndpoints.Namespace, endpointAddressMap, endpointPods); err != nil { + if _, err = r.deregisterServiceOnAllAgents(ctx, serviceEndpoints.Name, serviceEndpoints.Namespace, endpointAddressMap, endpointPods); err != nil { r.Log.Error(err, "failed to deregister endpoints on all agents", "name", serviceEndpoints.Name, "ns", serviceEndpoints.Namespace) errs = multierror.Append(errs, err) } @@ -673,7 +669,7 @@ func getHealthCheckStatusReason(healthCheckStatus, podName, podNamespace string) // The argument endpointsAddressesMap decides whether to deregister *all* service instances or selectively deregister // them only if they are not in endpointsAddressesMap. If the map is nil, it will deregister all instances. If the map // has addresses, it will only deregister instances not in the map. -func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, k8sSvcName, k8sSvcNamespace string, endpointsAddressesMap map[string]bool, endpointPods mapset.Set) error { +func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, k8sSvcName, k8sSvcNamespace string, endpointsAddressesMap map[string]bool, endpointPods mapset.Set) (ctrl.Result, error) { // Get all agents by getting pods with label component=client, app=consul and release= agents := corev1.PodList{} listOptions := client.ListOptions{ @@ -686,7 +682,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, } if err := r.Client.List(ctx, &agents, &listOptions); err != nil { r.Log.Error(err, "failed to get Consul client agent pods") - return err + return ctrl.Result{}, err } // On each agent, we need to get services matching "k8s-service-name" and "k8s-namespace" metadata. @@ -694,14 +690,14 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, client, err := r.remoteConsulClient(agent.Status.PodIP, r.consulNamespace(k8sSvcNamespace)) if err != nil { r.Log.Error(err, "failed to create a new Consul client", "address", agent.Status.PodIP) - return err + return ctrl.Result{}, err } // Get services matching metadata. svcs, err := serviceInstancesForK8SServiceNameAndNamespace(k8sSvcName, k8sSvcNamespace, client) if err != nil { r.Log.Error(err, "failed to get service instances", "name", k8sSvcName) - return err + return ctrl.Result{}, err } // Deregister each service instance that matches the metadata. @@ -715,7 +711,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, r.Log.Info("deregistering service from consul", "svc", svcID) if err = client.Agent().ServiceDeregister(svcID); err != nil { r.Log.Error(err, "failed to deregister service instance", "id", svcID) - return err + return ctrl.Result{}, err } serviceDeregistered = true } @@ -723,7 +719,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, r.Log.Info("deregistering service from consul", "svc", svcID) if err = client.Agent().ServiceDeregister(svcID); err != nil { r.Log.Error(err, "failed to deregister service instance", "id", svcID) - return err + return ctrl.Result{}, err } serviceDeregistered = true } @@ -733,12 +729,12 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, err = r.deleteACLTokensForServiceInstance(client, serviceRegistration.Service, k8sSvcNamespace, serviceRegistration.Meta[MetaKeyPodName]) if err != nil { r.Log.Error(err, "failed to reconcile ACL tokens for service", "svc", serviceRegistration.Service) - return err + return ctrl.Result{}, err } } } } - return nil + return ctrl.Result{}, nil } // deleteACLTokensForServiceInstance finds the ACL tokens that belongs to the service instance and deletes it from Consul. From 38bf002549952206c9df1d5515d8d0f7cd2e9a84 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 12:43:40 -0500 Subject: [PATCH 04/37] Add a warning in connect-init about running multiple services which point to the same pod. --- control-plane/subcommand/connect-init/command.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/control-plane/subcommand/connect-init/command.go b/control-plane/subcommand/connect-init/command.go index e7487fb9e1..9e90766c01 100644 --- a/control-plane/subcommand/connect-init/command.go +++ b/control-plane/subcommand/connect-init/command.go @@ -177,7 +177,12 @@ func (c *Command) Run(args []string) int { // it is not "lost" to the user at the end of the retries when the pod enters a CrashLoop. if registrationRetryCount%10 == 0 { c.logger.Info("Check to ensure a Kubernetes service has been created for this application." + - " If your pod is not starting also check the connect-inject deployment logs.") + " If your pod is not starting also check the connect-inject deployment logs." + + " Note that only one service may be used to route requests to each pod." + + " If you need multiple services to point to the same pod, add the label" + + " `consul.hashicorp.com/connect-ignore: \"true\"` to services which should" + + " not be used by Consul for handling requests.") + } return fmt.Errorf("did not find correct number of services: %d", len(serviceList)) } From a6a14499858cfa815b0057e6cf62735783c6c241 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 12:48:37 -0500 Subject: [PATCH 05/37] Add a line break before the multiservice warning --- control-plane/subcommand/connect-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/subcommand/connect-init/command.go b/control-plane/subcommand/connect-init/command.go index 9e90766c01..0b1515c392 100644 --- a/control-plane/subcommand/connect-init/command.go +++ b/control-plane/subcommand/connect-init/command.go @@ -178,7 +178,7 @@ func (c *Command) Run(args []string) int { if registrationRetryCount%10 == 0 { c.logger.Info("Check to ensure a Kubernetes service has been created for this application." + " If your pod is not starting also check the connect-inject deployment logs." + - " Note that only one service may be used to route requests to each pod." + + "\nNote that only one service may be used to route requests to each pod." + " If you need multiple services to point to the same pod, add the label" + " `consul.hashicorp.com/connect-ignore: \"true\"` to services which should" + " not be used by Consul for handling requests.") From a422942fc90b29eee8f2eb129a643491adbf97c5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 15:21:09 -0500 Subject: [PATCH 06/37] Move labelConnectIgnore to annotations --- control-plane/connect-inject/annotations.go | 4 ++++ control-plane/connect-inject/endpoints_controller.go | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/control-plane/connect-inject/annotations.go b/control-plane/connect-inject/annotations.go index ccc9ab6341..5a5d1615f4 100644 --- a/control-plane/connect-inject/annotations.go +++ b/control-plane/connect-inject/annotations.go @@ -122,6 +122,10 @@ const ( // webhook/handler. annotationOriginalPod = "consul.hashicorp.com/original-pod" + // labelConnectIgnore is a label that can be added to a service to prevent it from being + // registered with Consul. + labelConnectIgnore = "consul.hashicorp.com/connect-ignore" + // injected is used as the annotation value for annotationInjected. injected = "injected" diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 20666a795e..38c83d4304 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -39,7 +39,6 @@ const ( kubernetesSuccessReasonMsg = "Kubernetes health checks passing" envoyPrometheusBindAddr = "envoy_prometheus_bind_addr" envoySidecarContainer = "envoy-sidecar" - connectIgnoreLabel = "consul.hashicorp.com/connect-ignore" // clusterIPTaggedAddressName is the key for the tagged address to store the service's cluster IP and service port // in Consul. Note: This value should not be changed without a corresponding change in Consul. @@ -150,7 +149,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. - if value, ok := serviceEndpoints.Labels[connectIgnoreLabel]; ok && value == "true" { + if value, ok := serviceEndpoints.Labels[labelConnectIgnore]; ok && value == "true" { return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) } From bbcd557b1624193219d85a27a13c793ca0e0e1fc Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 15:42:52 -0500 Subject: [PATCH 07/37] Inline the check on hasBeenInjected --- control-plane/connect-inject/endpoints_controller.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 38c83d4304..f989906d99 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -1011,10 +1011,8 @@ func (r *EndpointsController) consulNamespace(namespace string) string { // hasBeenInjected checks the value of the status annotation and returns true if the Pod has been injected. func hasBeenInjected(pod corev1.Pod) bool { - if anno, ok := pod.Annotations[keyInjectStatus]; ok { - if anno == injected { - return true - } + if anno, ok := pod.Annotations[keyInjectStatus]; ok && anno == injected { + return true } return false } From c868850b1648f8a31412ef6cca22983afd8e655b Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 16:04:36 -0500 Subject: [PATCH 08/37] Refactor address mapping to a function --- .../connect-inject/endpoints_controller.go | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index f989906d99..a44cf71c90 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -161,17 +161,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // Register all addresses of this Endpoints object as service instances in Consul. for _, subset := range serviceEndpoints.Subsets { - // Combine all addresses to a map, with a value indicating whether an address is ready or not. - allAddresses := make(map[corev1.EndpointAddress]string) - for _, readyAddress := range subset.Addresses { - allAddresses[readyAddress] = api.HealthPassing - } - - for _, notReadyAddress := range subset.NotReadyAddresses { - allAddresses[notReadyAddress] = api.HealthCritical - } - - for address, healthStatus := range allAddresses { + for address, healthStatus := range mapAddresses(subset) { if address.TargetRef != nil && address.TargetRef.Kind == "Pod" { endpointPods.Add(address.TargetRef.Name) if err := r.registerServicesAndHealthCheck(ctx, serviceEndpoints, address, healthStatus, endpointAddressMap); err != nil { @@ -1016,3 +1006,17 @@ func hasBeenInjected(pod corev1.Pod) bool { } return false } + +// mapAddresses combines all addresses to a mapping of address to its health status. +func mapAddresses(addresses corev1.EndpointSubset) map[corev1.EndpointAddress]string { + m := make(map[corev1.EndpointAddress]string) + for _, readyAddress := range addresses.Addresses { + m[readyAddress] = api.HealthPassing + } + + for _, notReadyAddress := range addresses.NotReadyAddresses { + m[notReadyAddress] = api.HealthCritical + } + + return m +} From 9c9ea9df7ae51ecdbd4008a5d432ac4b09e72991 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 16:22:23 -0500 Subject: [PATCH 09/37] Test mapAddresses --- .../endpoints_controller_test.go | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index 5f1a0dadd8..0bb02fb9e2 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -4761,6 +4761,29 @@ func TestGetTokenMetaFromDescription(t *testing.T) { } } +func TestMapAddresses(t *testing.T) { + addresses := corev1.EndpointSubset{ + Addresses: []corev1.EndpointAddress{ + {Hostname: "host1"}, + {Hostname: "host2"}, + }, + NotReadyAddresses: []corev1.EndpointAddress{ + {Hostname: "host3"}, + {Hostname: "host4"}, + }, + } + + expected := map[corev1.EndpointAddress]string{ + {Hostname: "host1"}: api.HealthPassing, + {Hostname: "host2"}: api.HealthPassing, + {Hostname: "host3"}: api.HealthCritical, + {Hostname: "host4"}: api.HealthCritical, + } + + actual := mapAddresses(addresses) + require.Equal(t, expected, actual) +} + func createPod(name, ip string, inject bool, managedByEndpointsController bool) *corev1.Pod { pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ From 1918b48338861bf4d0b6f7712f039b82f4e447e4 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 16:35:29 -0500 Subject: [PATCH 10/37] connect-ignore -> service-ignore --- control-plane/connect-inject/annotations.go | 4 ++-- control-plane/connect-inject/endpoints_controller.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/control-plane/connect-inject/annotations.go b/control-plane/connect-inject/annotations.go index 5a5d1615f4..c8bf650e08 100644 --- a/control-plane/connect-inject/annotations.go +++ b/control-plane/connect-inject/annotations.go @@ -122,9 +122,9 @@ const ( // webhook/handler. annotationOriginalPod = "consul.hashicorp.com/original-pod" - // labelConnectIgnore is a label that can be added to a service to prevent it from being + // labelServiceIgnore is a label that can be added to a service to prevent it from being // registered with Consul. - labelConnectIgnore = "consul.hashicorp.com/connect-ignore" + labelServiceIgnore = "consul.hashicorp.com/service-ignore" // injected is used as the annotation value for annotationInjected. injected = "injected" diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index a44cf71c90..87f34bb2ab 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -149,7 +149,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. - if value, ok := serviceEndpoints.Labels[labelConnectIgnore]; ok && value == "true" { + if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) } From 551967c8786e78840153d3082e86888e8345eff8 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 17:28:38 -0500 Subject: [PATCH 11/37] Clarify Reconcile comment --- control-plane/connect-inject/endpoints_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 87f34bb2ab..4c52a2b98d 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -118,8 +118,7 @@ type EndpointsController struct { } // Reconcile reads the state of the cluster's service endpoints when a request is received and makes changes -// to the endpoints based on the state read. Requests are created when the endpoints are created, updated, -// or deleted. +// to the endpoints based on the state read. Reconcile is called when services are created, updated, or deleted. func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { var errs error var serviceEndpoints corev1.Endpoints From 766e8708eeeb95802cd9e28d0f227cc8c2fc02a1 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Mon, 15 Nov 2021 13:14:08 -0500 Subject: [PATCH 12/37] Add CHANGELOG entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11988f4b27..ac01dcdd26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ BREAKING CHANGES: IMPROVEMENTS: * CLI * Pre-check in the `install` command to verify the correct license secret exists when using an enterprise Consul image. [[GH-875](https://github.com/hashicorp/consul-k8s/pull/875)] +* Control Plane * Add a label "managed-by" to every secret the control-plane creates. Only delete said secrets on an uninstall. [[GH-835](https://github.com/hashicorp/consul-k8s/pull/835)] + * Add `consul.hashicorp.com/service-ignore` to prevent services from being registered in Consul. [[GH-858](https://github.com/hashicorp/consul-k8s/pull/858)] ## 0.37.0 (November 18, 2021) From 0ffc49bbb268ce9b6dc7d35aad4afa519ed8c4d7 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 17:53:53 -0500 Subject: [PATCH 13/37] Stub out test --- .../endpoints_controller_test.go | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index 0bb02fb9e2..fe57b78dfa 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -535,6 +535,87 @@ func TestProcessUpstreams(t *testing.T) { } } +// TestReconcileWithServiceIgnore tests the reconcile function with a service that should be ignored. +func TestReconcileWithServiceIgnore(t *testing.T) { + t.Parallel() + nodeName := "test-node" + + cases := []struct { + name string + consulSvcName string + k8sObjects func() []runtime.Object + initialConsulSvcs []*api.AgentServiceRegistration + expectedNumSvcInstances int + expectedConsulSvcInstances []*api.CatalogService + expectedProxySvcInstances []*api.CatalogService + expectedAgentHealthChecks []*api.AgentCheck + expErr string + }{ + // TODO: Test case where service is registered and should be deregistered. + // TODO: Test case where service is not registered and just gets ignored. + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + // The agent pod needs to have the address 127.0.0.1 so when the + // code gets the agent pods via the label component=client, and + // makes requests against the agent API, it will actually hit the + // test server we have on localhost. + fakeClientPod := createPod("fake-consul-client", "127.0.0.1", false, true) + fakeClientPod.Labels = map[string]string{"component": "client", "app": "consul", "release": "consul"} + + // Add the default namespace. + ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + // Create fake k8s client + k8sObjects := append(tt.k8sObjects(), fakeClientPod, &ns) + + fakeClient := fake.NewClientBuilder().WithRuntimeObjects(k8sObjects...).Build() + + // Create test consul server + consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.NodeName = nodeName + }) + require.NoError(t, err) + defer consul.Stop() + consul.WaitForServiceIntentions(t) + + cfg := &api.Config{ + Address: consul.HTTPAddr, + } + consulClient, err := api.NewClient(cfg) + require.NoError(t, err) + addr := strings.Split(consul.HTTPAddr, ":") + consulPort := addr[1] + + // Register service and proxy in consul. + for _, svc := range tt.initialConsulSvcs { + err = consulClient.Agent().ServiceRegister(svc) + require.NoError(t, err) + } + + // Create the endpoints controller + ep := &EndpointsController{ + Client: fakeClient, + Log: logrtest.TestLogger{T: t}, + ConsulClient: consulClient, + ConsulPort: consulPort, + ConsulScheme: "http", + AllowK8sNamespacesSet: mapset.NewSetWith("*"), + DenyK8sNamespacesSet: mapset.NewSetWith(), + ReleaseName: "consul", + ReleaseNamespace: "default", + ConsulClientCfg: cfg, + } + namespacedName := types.NamespacedName{ + Namespace: "default", + Name: "service-created", + } + + _, _ = ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) + }) + } +} + // TestReconcileCreateEndpoint tests the logic to create service instances in Consul from the addresses in the Endpoints // object. The cases test an empty endpoints object, a basic endpoints object with one address, a basic endpoints object // with two addresses, and an endpoints object with every possible customization. From 1591d5614590c98bd073b720b2165242f0b2d673 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 12 Nov 2021 17:54:02 -0500 Subject: [PATCH 14/37] Stub out isRegistered --- .../connect-inject/endpoints_controller.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 4c52a2b98d..2e2563ad3b 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -149,7 +149,10 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { - return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) + if isRegistered() { + return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) + } + return ctrl.Result{}, nil } r.Log.Info("retrieved", "name", serviceEndpoints.Name, "ns", serviceEndpoints.Namespace) @@ -1019,3 +1022,11 @@ func mapAddresses(addresses corev1.EndpointSubset) map[corev1.EndpointAddress]st return m } + +// isRegistered checks if the endpoint is registered in Consul. +func isRegistered() bool { + // TODO: Implement this. + // _, err := r.consul.Catalog().Service(endpoint.ServiceName, "", nil) + // return err == nil + return true +} From 9a99f92ab44edb7511308fa037a71ad0802c56a1 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 16 Nov 2021 13:26:56 -0500 Subject: [PATCH 15/37] Just a baby grammar fix --- control-plane/connect-inject/endpoints_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 2e2563ad3b..ba43cd5048 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -199,7 +199,7 @@ func (r *EndpointsController) SetupWithManager(mgr ctrl.Manager) error { ).Complete(r) } -// registerServicesAndHealthCheck creates Consul registrations for the service and proxy and register them with Consul. +// registerServicesAndHealthCheck creates Consul registrations for the service and proxy and registers them with Consul. // It also upserts a Kubernetes health check for the service based on whether the endpoint address is ready. func (r *EndpointsController) registerServicesAndHealthCheck(ctx context.Context, serviceEndpoints corev1.Endpoints, address corev1.EndpointAddress, healthStatus string, endpointAddressMap map[string]bool) error { // Get pod associated with this address. From 72a331892616ed3b04e6c82d065380b51692db6c Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 17 Nov 2021 12:46:17 -0500 Subject: [PATCH 16/37] Undo deregister refactor --- .../connect-inject/endpoints_controller.go | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index ba43cd5048..748feba4e0 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -140,7 +140,8 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( if k8serrors.IsNotFound(err) { // Deregister all instances in Consul for this service. The function deregisterServiceOnAllAgents handles // the case where the Consul service name is different from the Kubernetes service name. - return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) + err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) + return ctrl.Result{}, err } else if err != nil { r.Log.Error(err, "failed to get Endpoints", "name", req.Name, "ns", req.Namespace) return ctrl.Result{}, err @@ -660,7 +661,7 @@ func getHealthCheckStatusReason(healthCheckStatus, podName, podNamespace string) // The argument endpointsAddressesMap decides whether to deregister *all* service instances or selectively deregister // them only if they are not in endpointsAddressesMap. If the map is nil, it will deregister all instances. If the map // has addresses, it will only deregister instances not in the map. -func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, k8sSvcName, k8sSvcNamespace string, endpointsAddressesMap map[string]bool, endpointPods mapset.Set) (ctrl.Result, error) { +func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, k8sSvcName, k8sSvcNamespace string, endpointsAddressesMap map[string]bool, endpointPods mapset.Set) error { // Get all agents by getting pods with label component=client, app=consul and release= agents := corev1.PodList{} listOptions := client.ListOptions{ @@ -673,7 +674,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, } if err := r.Client.List(ctx, &agents, &listOptions); err != nil { r.Log.Error(err, "failed to get Consul client agent pods") - return ctrl.Result{}, err + return err } // On each agent, we need to get services matching "k8s-service-name" and "k8s-namespace" metadata. @@ -681,14 +682,14 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, client, err := r.remoteConsulClient(agent.Status.PodIP, r.consulNamespace(k8sSvcNamespace)) if err != nil { r.Log.Error(err, "failed to create a new Consul client", "address", agent.Status.PodIP) - return ctrl.Result{}, err + return err } // Get services matching metadata. svcs, err := serviceInstancesForK8SServiceNameAndNamespace(k8sSvcName, k8sSvcNamespace, client) if err != nil { r.Log.Error(err, "failed to get service instances", "name", k8sSvcName) - return ctrl.Result{}, err + return err } // Deregister each service instance that matches the metadata. @@ -702,7 +703,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, r.Log.Info("deregistering service from consul", "svc", svcID) if err = client.Agent().ServiceDeregister(svcID); err != nil { r.Log.Error(err, "failed to deregister service instance", "id", svcID) - return ctrl.Result{}, err + return err } serviceDeregistered = true } @@ -710,7 +711,7 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, r.Log.Info("deregistering service from consul", "svc", svcID) if err = client.Agent().ServiceDeregister(svcID); err != nil { r.Log.Error(err, "failed to deregister service instance", "id", svcID) - return ctrl.Result{}, err + return err } serviceDeregistered = true } @@ -720,12 +721,13 @@ func (r *EndpointsController) deregisterServiceOnAllAgents(ctx context.Context, err = r.deleteACLTokensForServiceInstance(client, serviceRegistration.Service, k8sSvcNamespace, serviceRegistration.Meta[MetaKeyPodName]) if err != nil { r.Log.Error(err, "failed to reconcile ACL tokens for service", "svc", serviceRegistration.Service) - return ctrl.Result{}, err + return err } } } } - return ctrl.Result{}, nil + + return nil } // deleteACLTokensForServiceInstance finds the ACL tokens that belongs to the service instance and deletes it from Consul. From 9a7eb00fd744ba3f33ff6748d93848206e0fe163 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 17 Nov 2021 13:43:39 -0500 Subject: [PATCH 17/37] Pull the deregistration back into the normal path --- .../connect-inject/endpoints_controller.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 748feba4e0..4d2fcb2487 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -150,10 +150,8 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { - if isRegistered() { - return r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) - } - return ctrl.Result{}, nil + err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) + return ctrl.Result{}, err } r.Log.Info("retrieved", "name", serviceEndpoints.Name, "ns", serviceEndpoints.Namespace) @@ -178,7 +176,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // Compare service instances in Consul with addresses in Endpoints. If an address is not in Endpoints, deregister // from Consul. This uses endpointAddressMap which is populated with the addresses in the Endpoints object during // the registration codepath. - if _, err = r.deregisterServiceOnAllAgents(ctx, serviceEndpoints.Name, serviceEndpoints.Namespace, endpointAddressMap, endpointPods); err != nil { + if err = r.deregisterServiceOnAllAgents(ctx, serviceEndpoints.Name, serviceEndpoints.Namespace, endpointAddressMap, endpointPods); err != nil { r.Log.Error(err, "failed to deregister endpoints on all agents", "name", serviceEndpoints.Name, "ns", serviceEndpoints.Namespace) errs = multierror.Append(errs, err) } @@ -651,6 +649,11 @@ func getHealthCheckStatusReason(healthCheckStatus, podName, podNamespace string) return fmt.Sprintf("Pod \"%s/%s\" is not ready", podNamespace, podName) } +// fetchServices returns a list of services from the given k8s client. +func (r *EndpointsController) fetchServices(ctx context.Context, k8sSvcName, k8sSvcNamespace string) ([]*api.AgentService, error) { + return nil, nil +} + // deregisterServiceOnAllAgents queries all agents for service instances that have the metadata // "k8s-service-name"=k8sSvcName and "k8s-namespace"=k8sSvcNamespace. The k8s service name may or may not match the // consul service name, but the k8s service name will always match the metadata on the Consul service @@ -1027,8 +1030,6 @@ func mapAddresses(addresses corev1.EndpointSubset) map[corev1.EndpointAddress]st // isRegistered checks if the endpoint is registered in Consul. func isRegistered() bool { - // TODO: Implement this. - // _, err := r.consul.Catalog().Service(endpoint.ServiceName, "", nil) - // return err == nil + return true } From 016aea44043d3503ae0a4a54ab8ace467c9f165d Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 17 Nov 2021 15:37:30 -0500 Subject: [PATCH 18/37] Extend test cases for mapAddresses --- .../endpoints_controller_test.go | 75 +++++++++++++++---- 1 file changed, 60 insertions(+), 15 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index fe57b78dfa..3011686fd2 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -4843,26 +4843,71 @@ func TestGetTokenMetaFromDescription(t *testing.T) { } func TestMapAddresses(t *testing.T) { - addresses := corev1.EndpointSubset{ - Addresses: []corev1.EndpointAddress{ - {Hostname: "host1"}, - {Hostname: "host2"}, + t.Parallel() + cases := map[string]struct { + addresses corev1.EndpointSubset + expected map[corev1.EndpointAddress]string + }{ + "ready and not ready addresses": { + addresses: corev1.EndpointSubset{ + Addresses: []corev1.EndpointAddress{ + {Hostname: "host1"}, + {Hostname: "host2"}, + }, + NotReadyAddresses: []corev1.EndpointAddress{ + {Hostname: "host3"}, + {Hostname: "host4"}, + }, + }, + expected: map[corev1.EndpointAddress]string{ + {Hostname: "host1"}: api.HealthPassing, + {Hostname: "host2"}: api.HealthPassing, + {Hostname: "host3"}: api.HealthCritical, + {Hostname: "host4"}: api.HealthCritical, + }, }, - NotReadyAddresses: []corev1.EndpointAddress{ - {Hostname: "host3"}, - {Hostname: "host4"}, + "ready addresses only": { + addresses: corev1.EndpointSubset{ + Addresses: []corev1.EndpointAddress{ + {Hostname: "host1"}, + {Hostname: "host2"}, + {Hostname: "host3"}, + {Hostname: "host4"}, + }, + NotReadyAddresses: []corev1.EndpointAddress{}, + }, + expected: map[corev1.EndpointAddress]string{ + {Hostname: "host1"}: api.HealthPassing, + {Hostname: "host2"}: api.HealthPassing, + {Hostname: "host3"}: api.HealthPassing, + {Hostname: "host4"}: api.HealthPassing, + }, + }, + "not ready addresses only": { + addresses: corev1.EndpointSubset{ + Addresses: []corev1.EndpointAddress{}, + NotReadyAddresses: []corev1.EndpointAddress{ + {Hostname: "host1"}, + {Hostname: "host2"}, + {Hostname: "host3"}, + {Hostname: "host4"}, + }, + }, + expected: map[corev1.EndpointAddress]string{ + {Hostname: "host1"}: api.HealthCritical, + {Hostname: "host2"}: api.HealthCritical, + {Hostname: "host3"}: api.HealthCritical, + {Hostname: "host4"}: api.HealthCritical, + }, }, } - expected := map[corev1.EndpointAddress]string{ - {Hostname: "host1"}: api.HealthPassing, - {Hostname: "host2"}: api.HealthPassing, - {Hostname: "host3"}: api.HealthCritical, - {Hostname: "host4"}: api.HealthCritical, + for name, c := range cases { + t.Run(name, func(t *testing.T) { + actual := mapAddresses(c.addresses) + require.Equal(t, c.expected, actual) + }) } - - actual := mapAddresses(addresses) - require.Equal(t, expected, actual) } func createPod(name, ip string, inject bool, managedByEndpointsController bool) *corev1.Pod { From 239ca7500745e0c01515599320a8952501132c73 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 17 Nov 2021 16:02:33 -0500 Subject: [PATCH 19/37] Move the stubbed out test down --- .../endpoints_controller_test.go | 162 +++++++++--------- 1 file changed, 81 insertions(+), 81 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index 3011686fd2..a46f1b83a8 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -535,87 +535,6 @@ func TestProcessUpstreams(t *testing.T) { } } -// TestReconcileWithServiceIgnore tests the reconcile function with a service that should be ignored. -func TestReconcileWithServiceIgnore(t *testing.T) { - t.Parallel() - nodeName := "test-node" - - cases := []struct { - name string - consulSvcName string - k8sObjects func() []runtime.Object - initialConsulSvcs []*api.AgentServiceRegistration - expectedNumSvcInstances int - expectedConsulSvcInstances []*api.CatalogService - expectedProxySvcInstances []*api.CatalogService - expectedAgentHealthChecks []*api.AgentCheck - expErr string - }{ - // TODO: Test case where service is registered and should be deregistered. - // TODO: Test case where service is not registered and just gets ignored. - } - - for _, tt := range cases { - t.Run(tt.name, func(t *testing.T) { - // The agent pod needs to have the address 127.0.0.1 so when the - // code gets the agent pods via the label component=client, and - // makes requests against the agent API, it will actually hit the - // test server we have on localhost. - fakeClientPod := createPod("fake-consul-client", "127.0.0.1", false, true) - fakeClientPod.Labels = map[string]string{"component": "client", "app": "consul", "release": "consul"} - - // Add the default namespace. - ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} - // Create fake k8s client - k8sObjects := append(tt.k8sObjects(), fakeClientPod, &ns) - - fakeClient := fake.NewClientBuilder().WithRuntimeObjects(k8sObjects...).Build() - - // Create test consul server - consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { - c.NodeName = nodeName - }) - require.NoError(t, err) - defer consul.Stop() - consul.WaitForServiceIntentions(t) - - cfg := &api.Config{ - Address: consul.HTTPAddr, - } - consulClient, err := api.NewClient(cfg) - require.NoError(t, err) - addr := strings.Split(consul.HTTPAddr, ":") - consulPort := addr[1] - - // Register service and proxy in consul. - for _, svc := range tt.initialConsulSvcs { - err = consulClient.Agent().ServiceRegister(svc) - require.NoError(t, err) - } - - // Create the endpoints controller - ep := &EndpointsController{ - Client: fakeClient, - Log: logrtest.TestLogger{T: t}, - ConsulClient: consulClient, - ConsulPort: consulPort, - ConsulScheme: "http", - AllowK8sNamespacesSet: mapset.NewSetWith("*"), - DenyK8sNamespacesSet: mapset.NewSetWith(), - ReleaseName: "consul", - ReleaseNamespace: "default", - ConsulClientCfg: cfg, - } - namespacedName := types.NamespacedName{ - Namespace: "default", - Name: "service-created", - } - - _, _ = ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) - }) - } -} - // TestReconcileCreateEndpoint tests the logic to create service instances in Consul from the addresses in the Endpoints // object. The cases test an empty endpoints object, a basic endpoints object with one address, a basic endpoints object // with two addresses, and an endpoints object with every possible customization. @@ -2798,6 +2717,87 @@ func TestReconcileDeleteEndpoint(t *testing.T) { } } +// TestReconcileWithServiceIgnore tests the reconcile function with a service that should be ignored. +func TestReconcileWithServiceIgnore(t *testing.T) { + t.Parallel() + nodeName := "test-node" + + cases := []struct { + name string + consulSvcName string + k8sObjects func() []runtime.Object + initialConsulSvcs []*api.AgentServiceRegistration + expectedNumSvcInstances int + expectedConsulSvcInstances []*api.CatalogService + expectedProxySvcInstances []*api.CatalogService + expectedAgentHealthChecks []*api.AgentCheck + expErr string + }{ + // TODO: Test case where service is registered and should be deregistered. + // TODO: Test case where service is not registered and just gets ignored. + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + // The agent pod needs to have the address 127.0.0.1 so when the + // code gets the agent pods via the label component=client, and + // makes requests against the agent API, it will actually hit the + // test server we have on localhost. + fakeClientPod := createPod("fake-consul-client", "127.0.0.1", false, true) + fakeClientPod.Labels = map[string]string{"component": "client", "app": "consul", "release": "consul"} + + // Add the default namespace. + ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + // Create fake k8s client + k8sObjects := append(tt.k8sObjects(), fakeClientPod, &ns) + + fakeClient := fake.NewClientBuilder().WithRuntimeObjects(k8sObjects...).Build() + + // Create test consul server + consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.NodeName = nodeName + }) + require.NoError(t, err) + defer consul.Stop() + consul.WaitForServiceIntentions(t) + + cfg := &api.Config{ + Address: consul.HTTPAddr, + } + consulClient, err := api.NewClient(cfg) + require.NoError(t, err) + addr := strings.Split(consul.HTTPAddr, ":") + consulPort := addr[1] + + // Register service and proxy in consul. + for _, svc := range tt.initialConsulSvcs { + err = consulClient.Agent().ServiceRegister(svc) + require.NoError(t, err) + } + + // Create the endpoints controller + ep := &EndpointsController{ + Client: fakeClient, + Log: logrtest.TestLogger{T: t}, + ConsulClient: consulClient, + ConsulPort: consulPort, + ConsulScheme: "http", + AllowK8sNamespacesSet: mapset.NewSetWith("*"), + DenyK8sNamespacesSet: mapset.NewSetWith(), + ReleaseName: "consul", + ReleaseNamespace: "default", + ConsulClientCfg: cfg, + } + namespacedName := types.NamespacedName{ + Namespace: "default", + Name: "service-created", + } + + _, _ = ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) + }) + } +} + func TestFilterAgentPods(t *testing.T) { t.Parallel() cases := map[string]struct { From a1cf92e83fd7009d60d5ec0b931736941f02c3b5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Thu, 18 Nov 2021 13:22:28 -0500 Subject: [PATCH 20/37] Add comment to deregistration process --- control-plane/connect-inject/endpoints_controller.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 4d2fcb2487..782aa546c9 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -150,6 +150,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { + // We always deregister the service to handle the case where a user has registered the service, then added the label later. err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) return ctrl.Result{}, err } @@ -649,11 +650,6 @@ func getHealthCheckStatusReason(healthCheckStatus, podName, podNamespace string) return fmt.Sprintf("Pod \"%s/%s\" is not ready", podNamespace, podName) } -// fetchServices returns a list of services from the given k8s client. -func (r *EndpointsController) fetchServices(ctx context.Context, k8sSvcName, k8sSvcNamespace string) ([]*api.AgentService, error) { - return nil, nil -} - // deregisterServiceOnAllAgents queries all agents for service instances that have the metadata // "k8s-service-name"=k8sSvcName and "k8s-namespace"=k8sSvcNamespace. The k8s service name may or may not match the // consul service name, but the k8s service name will always match the metadata on the Consul service From e1ceeb9047f85eaebfbc3fbdd4f25957187a6cb2 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 19 Nov 2021 12:17:27 -0500 Subject: [PATCH 21/37] Fix connect ignore to service ignore --- control-plane/connect-inject/endpoints_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 782aa546c9..10a71ae3a8 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -147,7 +147,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - // If the endpoints object has the label "connect-ignore" set to true, deregister all instances in Consul for this service. + // If the endpoints object has the label "service-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { // We always deregister the service to handle the case where a user has registered the service, then added the label later. From 04f72a2a156e80fecf337b70b58d98a75e338ca5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 19 Nov 2021 12:17:36 -0500 Subject: [PATCH 22/37] Test service-ignore --- .../endpoints_controller_test.go | 133 ++++++++++++------ 1 file changed, 87 insertions(+), 46 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index a46f1b83a8..52ca221ac6 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -2717,64 +2717,84 @@ func TestReconcileDeleteEndpoint(t *testing.T) { } } -// TestReconcileWithServiceIgnore tests the reconcile function with a service that should be ignored. -func TestReconcileWithServiceIgnore(t *testing.T) { +// TestReconcileIgnoresServiceIgnoreLabel tests that the endpoints controller correctly ignores services +// with the service-ignore label and deregisters services previously registered if the service-ignore +// label is added. This is done by running Reconcile twice, once on an initial set of labels, then again +// on a new set of labels with the service-ignore label. The number of services registered in Consul is +// checked before updating the labels against the `svcsExpectedBeforeUpdate` and after updating the labels +// where the expected number of services should be zero. +func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { t.Parallel() nodeName := "test-node" + serviceName := "service-ignored" + namespace := "default" - cases := []struct { - name string - consulSvcName string - k8sObjects func() []runtime.Object - initialConsulSvcs []*api.AgentServiceRegistration - expectedNumSvcInstances int - expectedConsulSvcInstances []*api.CatalogService - expectedProxySvcInstances []*api.CatalogService - expectedAgentHealthChecks []*api.AgentCheck - expErr string + cases := map[string]struct { + initialLabels map[string]string + updatedLabels map[string]string + svcsExpectedBeforeUpdate int }{ - // TODO: Test case where service is registered and should be deregistered. - // TODO: Test case where service is not registered and just gets ignored. + "Endpoint with service-ignore is never registered": { + initialLabels: map[string]string{ + labelServiceIgnore: "true", + }, + updatedLabels: map[string]string{ + labelServiceIgnore: "true", + }, + svcsExpectedBeforeUpdate: 0, + }, + "Endpoint without labels is registered, then deregisterd when service-ignore is added": { + initialLabels: map[string]string{}, + updatedLabels: map[string]string{ + labelServiceIgnore: "true", + }, + svcsExpectedBeforeUpdate: 1, + }, } - for _, tt := range cases { - t.Run(tt.name, func(t *testing.T) { - // The agent pod needs to have the address 127.0.0.1 so when the - // code gets the agent pods via the label component=client, and - // makes requests against the agent API, it will actually hit the - // test server we have on localhost. + for name, tt := range cases { + t.Run(name, func(t *testing.T) { + // Set up the fake Kubernetes client with an endpoint, pod, consul client, and the default namespace. + endpoint := &corev1.Endpoints{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceName, + Namespace: namespace, + Labels: tt.initialLabels, + }, + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "1.2.3.4", + NodeName: &nodeName, + TargetRef: &corev1.ObjectReference{ + Kind: "Pod", + Name: "pod1", + Namespace: namespace, + }, + }, + }, + }, + }, + } + pod1 := createPod("pod1", "1.2.3.4", true, true) fakeClientPod := createPod("fake-consul-client", "127.0.0.1", false, true) fakeClientPod.Labels = map[string]string{"component": "client", "app": "consul", "release": "consul"} - - // Add the default namespace. - ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}} - // Create fake k8s client - k8sObjects := append(tt.k8sObjects(), fakeClientPod, &ns) - + ns := corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + k8sObjects := []runtime.Object{endpoint, pod1, fakeClientPod, &ns} fakeClient := fake.NewClientBuilder().WithRuntimeObjects(k8sObjects...).Build() - // Create test consul server - consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { - c.NodeName = nodeName - }) + // Create test Consul server + consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { c.NodeName = nodeName }) require.NoError(t, err) defer consul.Stop() consul.WaitForServiceIntentions(t) - - cfg := &api.Config{ - Address: consul.HTTPAddr, - } + cfg := &api.Config{Address: consul.HTTPAddr} consulClient, err := api.NewClient(cfg) require.NoError(t, err) addr := strings.Split(consul.HTTPAddr, ":") consulPort := addr[1] - // Register service and proxy in consul. - for _, svc := range tt.initialConsulSvcs { - err = consulClient.Agent().ServiceRegister(svc) - require.NoError(t, err) - } - // Create the endpoints controller ep := &EndpointsController{ Client: fakeClient, @@ -2785,15 +2805,36 @@ func TestReconcileWithServiceIgnore(t *testing.T) { AllowK8sNamespacesSet: mapset.NewSetWith("*"), DenyK8sNamespacesSet: mapset.NewSetWith(), ReleaseName: "consul", - ReleaseNamespace: "default", + ReleaseNamespace: namespace, ConsulClientCfg: cfg, } - namespacedName := types.NamespacedName{ - Namespace: "default", - Name: "service-created", - } - _, _ = ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) + namespacedName := types.NamespacedName{Namespace: namespace, Name: serviceName} + + // Run the reconcile process the first time + resp, err := ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) + require.NoError(t, err) + require.False(t, resp.Requeue) + + // Check that the correct number of instances of the service were registered at the midpoint + serviceInstances, _, err := consulClient.Catalog().Service(serviceName, "", nil) + require.NoError(t, err) + require.Len(t, serviceInstances, tt.svcsExpectedBeforeUpdate) + + // Update labels on the endpoint + endpoint.Labels = tt.updatedLabels + err = fakeClient.Update(context.Background(), endpoint) + require.NoError(t, err) + + // Run the reconcile process the second time + resp, err = ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) + require.NoError(t, err) + require.False(t, resp.Requeue) + + // Check that no services are registered with Consul + serviceInstances, _, err = consulClient.Catalog().Service(serviceName, "", nil) + require.NoError(t, err) + require.Empty(t, serviceInstances) }) } } From e066d1138680f985e278ab41e46049758e0d6f5d Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Fri, 19 Nov 2021 12:32:55 -0500 Subject: [PATCH 23/37] Delete isRegistered --- control-plane/connect-inject/endpoints_controller.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 10a71ae3a8..b0635fa380 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -1023,9 +1023,3 @@ func mapAddresses(addresses corev1.EndpointSubset) map[corev1.EndpointAddress]st return m } - -// isRegistered checks if the endpoint is registered in Consul. -func isRegistered() bool { - - return true -} From 77a36eff89ec472a4255b4bce8b6a42d341b318e Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 23 Nov 2021 15:56:35 -0500 Subject: [PATCH 24/37] Update control-plane/connect-inject/endpoints_controller.go Co-authored-by: Kyle Schochenmaier --- control-plane/connect-inject/endpoints_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index b0635fa380..dfc43f42de 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -117,8 +117,8 @@ type EndpointsController struct { context.Context } -// Reconcile reads the state of the cluster's service endpoints when a request is received and makes changes -// to the endpoints based on the state read. Reconcile is called when services are created, updated, or deleted. +// Reconcile reads the state of an Endpoints object for a Kubernetes Service and reconciles Consul services which +// correspond to the Kubernetes Service. These events are driven by changes to the Pods backing the Kube service. func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { var errs error var serviceEndpoints corev1.Endpoints From 40af11b2d68a28337127e44f525de157b203b986 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 23 Nov 2021 15:56:44 -0500 Subject: [PATCH 25/37] Update control-plane/connect-inject/endpoints_controller.go Co-authored-by: Kyle Schochenmaier --- control-plane/connect-inject/endpoints_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index dfc43f42de..b0e7339ab9 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -147,7 +147,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - // If the endpoints object has the label "service-ignore" set to true, deregister all instances in Consul for this service. + // If the endpoints object has the label "consul.hashicorp.com/service-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { // We always deregister the service to handle the case where a user has registered the service, then added the label later. From 5717f4c0983ae30d3dc7692ef5f8dc83a1d4bbaa Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 23 Nov 2021 15:57:22 -0500 Subject: [PATCH 26/37] Update control-plane/subcommand/connect-init/command.go Co-authored-by: Kyle Schochenmaier --- control-plane/subcommand/connect-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/subcommand/connect-init/command.go b/control-plane/subcommand/connect-init/command.go index 0b1515c392..317894b596 100644 --- a/control-plane/subcommand/connect-init/command.go +++ b/control-plane/subcommand/connect-init/command.go @@ -180,7 +180,7 @@ func (c *Command) Run(args []string) int { " If your pod is not starting also check the connect-inject deployment logs." + "\nNote that only one service may be used to route requests to each pod." + " If you need multiple services to point to the same pod, add the label" + - " `consul.hashicorp.com/connect-ignore: \"true\"` to services which should" + + " `consul.hashicorp.com/service-ignore: \"true\"` to services which should" + " not be used by Consul for handling requests.") } From 3118bb756236551a57902c22625c6385b18dbd89 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Mon, 29 Nov 2021 12:53:26 -0500 Subject: [PATCH 27/37] Update CHANGELOG.md Co-authored-by: Kyle Schochenmaier --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac01dcdd26..6200839118 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ IMPROVEMENTS: * Pre-check in the `install` command to verify the correct license secret exists when using an enterprise Consul image. [[GH-875](https://github.com/hashicorp/consul-k8s/pull/875)] * Control Plane * Add a label "managed-by" to every secret the control-plane creates. Only delete said secrets on an uninstall. [[GH-835](https://github.com/hashicorp/consul-k8s/pull/835)] - * Add `consul.hashicorp.com/service-ignore` to prevent services from being registered in Consul. [[GH-858](https://github.com/hashicorp/consul-k8s/pull/858)] + * Add support for labeling a Kubernetes service with `consul.hashicorp.com/service-ignore` to prevent services from being registered in Consul. [[GH-858](https://github.com/hashicorp/consul-k8s/pull/858)] ## 0.37.0 (November 18, 2021) From 01917b19c6198d564b3f5ea0deaaa69ba640859c Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Mon, 29 Nov 2021 13:06:03 -0500 Subject: [PATCH 28/37] Use strconv.ParseBool on label value --- control-plane/connect-inject/endpoints_controller.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index b0e7339ab9..6aa559cbba 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -6,6 +6,7 @@ import ( "fmt" "net" "regexp" + "strconv" "strings" mapset "github.com/deckarep/golang-set" @@ -149,7 +150,15 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "consul.hashicorp.com/service-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. - if value, ok := serviceEndpoints.Labels[labelServiceIgnore]; ok && value == "true" { + value, labelExists := serviceEndpoints.Labels[labelServiceIgnore] + shouldIgnore, err := strconv.ParseBool(value) + + if err != nil { + r.Log.Error(err, "failed to parse label", "label", labelServiceIgnore, "value", value) + return ctrl.Result{}, err + } + + if labelExists && shouldIgnore { // We always deregister the service to handle the case where a user has registered the service, then added the label later. err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) return ctrl.Result{}, err From 6e8fdcbedad1c91d033711ac34226df8329c1986 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Mon, 29 Nov 2021 13:30:41 -0500 Subject: [PATCH 29/37] Separate error for multiple Consul services registered. --- control-plane/subcommand/connect-init/command.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/control-plane/subcommand/connect-init/command.go b/control-plane/subcommand/connect-init/command.go index 317894b596..94e3302f3d 100644 --- a/control-plane/subcommand/connect-init/command.go +++ b/control-plane/subcommand/connect-init/command.go @@ -177,13 +177,14 @@ func (c *Command) Run(args []string) int { // it is not "lost" to the user at the end of the retries when the pod enters a CrashLoop. if registrationRetryCount%10 == 0 { c.logger.Info("Check to ensure a Kubernetes service has been created for this application." + - " If your pod is not starting also check the connect-inject deployment logs." + - "\nNote that only one service may be used to route requests to each pod." + - " If you need multiple services to point to the same pod, add the label" + - " `consul.hashicorp.com/service-ignore: \"true\"` to services which should" + - " not be used by Consul for handling requests.") - + " If your pod is not starting also check the connect-inject deployment logs.") + } + if len(serviceList) > 2 { + c.logger.Error("There are multiple Consul services registered for this pod when there should only be one." + + " Check if there are multiple Kubernetes services pointing at this pod and add the label" + + " `consul.hashicorp.com/service-ignore: \"true\"` to services which should not be used by Consul for handling requests.") } + return fmt.Errorf("did not find correct number of services: %d", len(serviceList)) } for _, svc := range serviceList { From 588ceed770d0b3b2532416d8c5a1f08fc433f0e5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Mon, 29 Nov 2021 13:22:29 -0500 Subject: [PATCH 30/37] Update control-plane/connect-inject/endpoints_controller_test.go Co-authored-by: Iryna Shustava --- control-plane/connect-inject/endpoints_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index 52ca221ac6..823bde53d5 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -2784,7 +2784,7 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { k8sObjects := []runtime.Object{endpoint, pod1, fakeClientPod, &ns} fakeClient := fake.NewClientBuilder().WithRuntimeObjects(k8sObjects...).Build() - // Create test Consul server + // Create test Consul server. consul, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { c.NodeName = nodeName }) require.NoError(t, err) defer consul.Stop() From 9f3ad737341d4c0cf7278c30fe3768cf9d800e93 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 30 Nov 2021 12:16:24 -0500 Subject: [PATCH 31/37] Split out isLabeledIgnore --- .../connect-inject/endpoints_controller.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 6aa559cbba..9f07e3616d 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -150,15 +150,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // If the endpoints object has the label "consul.hashicorp.com/service-ignore" set to true, deregister all instances in Consul for this service. // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. - value, labelExists := serviceEndpoints.Labels[labelServiceIgnore] - shouldIgnore, err := strconv.ParseBool(value) - - if err != nil { - r.Log.Error(err, "failed to parse label", "label", labelServiceIgnore, "value", value) - return ctrl.Result{}, err - } - - if labelExists && shouldIgnore { + if isLabeledIgnore(serviceEndpoints.Labels) { // We always deregister the service to handle the case where a user has registered the service, then added the label later. err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) return ctrl.Result{}, err @@ -1032,3 +1024,12 @@ func mapAddresses(addresses corev1.EndpointSubset) map[corev1.EndpointAddress]st return m } + +// isLabeledIgnore checks the value of the label `consul.hashicorp.com/service-ignore` and returns true if the +// label exists and is "truthy". Otherwise, it returns false. +func isLabeledIgnore(labels map[string]string) bool { + value, labelExists := labels[labelServiceIgnore] + shouldIgnore, err := strconv.ParseBool(value) + + return shouldIgnore && labelExists && err == nil +} From a3218918fde0917672cc19b943019a2aa2128c0d Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 30 Nov 2021 12:17:05 -0500 Subject: [PATCH 32/37] Rework test to not run Reconcile twice --- .../endpoints_controller_test.go | 75 +++++++++++-------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index 823bde53d5..31d9615fea 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -2730,25 +2730,33 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { namespace := "default" cases := map[string]struct { - initialLabels map[string]string - updatedLabels map[string]string - svcsExpectedBeforeUpdate int + svcInitiallyRegistered bool + serviceLabels map[string]string + expectedNumSvcInstances int }{ - "Endpoint with service-ignore is never registered": { - initialLabels: map[string]string{ + "Registered endpoint with label is deregistered.": { + svcInitiallyRegistered: true, + serviceLabels: map[string]string{ labelServiceIgnore: "true", }, - updatedLabels: map[string]string{ - labelServiceIgnore: "true", - }, - svcsExpectedBeforeUpdate: 0, + expectedNumSvcInstances: 0, }, - "Endpoint without labels is registered, then deregisterd when service-ignore is added": { - initialLabels: map[string]string{}, - updatedLabels: map[string]string{ + "Not registered endpoint with label is never registered": { + svcInitiallyRegistered: false, + serviceLabels: map[string]string{ labelServiceIgnore: "true", }, - svcsExpectedBeforeUpdate: 1, + expectedNumSvcInstances: 0, + }, + "Registered endpoint without label is unaffected": { + svcInitiallyRegistered: true, + serviceLabels: map[string]string{}, + expectedNumSvcInstances: 1, + }, + "Not registered endpoint without label is registered": { + svcInitiallyRegistered: false, + serviceLabels: map[string]string{}, + expectedNumSvcInstances: 1, }, } @@ -2759,7 +2767,7 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: serviceName, Namespace: namespace, - Labels: tt.initialLabels, + Labels: tt.serviceLabels, }, Subsets: []corev1.EndpointSubset{ { @@ -2795,6 +2803,23 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { addr := strings.Split(consul.HTTPAddr, ":") consulPort := addr[1] + // Set up the initial Consul services. + if tt.svcInitiallyRegistered { + err = consulClient.Agent().ServiceRegister(&api.AgentServiceRegistration{ + ID: "pod1-" + serviceName, + Name: serviceName, + Port: 0, + Address: "1.2.3.4", + Meta: map[string]string{ + "k8s-namespace": namespace, + "k8s-service-name": serviceName, + "managed-by": "consul-k8s-endpoints-controller", + "pod-name": "pod1", + }, + }) + require.NoError(t, err) + } + // Create the endpoints controller ep := &EndpointsController{ Client: fakeClient, @@ -2809,32 +2834,16 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { ConsulClientCfg: cfg, } + // Run the reconcile process to deregister the service if it was registered before namespacedName := types.NamespacedName{Namespace: namespace, Name: serviceName} - - // Run the reconcile process the first time resp, err := ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) require.NoError(t, err) require.False(t, resp.Requeue) - // Check that the correct number of instances of the service were registered at the midpoint - serviceInstances, _, err := consulClient.Catalog().Service(serviceName, "", nil) - require.NoError(t, err) - require.Len(t, serviceInstances, tt.svcsExpectedBeforeUpdate) - - // Update labels on the endpoint - endpoint.Labels = tt.updatedLabels - err = fakeClient.Update(context.Background(), endpoint) - require.NoError(t, err) - - // Run the reconcile process the second time - resp, err = ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) - require.NoError(t, err) - require.False(t, resp.Requeue) - // Check that no services are registered with Consul - serviceInstances, _, err = consulClient.Catalog().Service(serviceName, "", nil) + serviceInstances, _, err := consulClient.Catalog().Service(serviceName, "", nil) require.NoError(t, err) - require.Empty(t, serviceInstances) + require.Len(t, serviceInstances, tt.expectedNumSvcInstances) }) } } From 9abb5041e2ee86121656ebbe3ac13d0317825759 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 30 Nov 2021 16:29:51 -0500 Subject: [PATCH 33/37] Clean up space on CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6200839118..a8a1f26d65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ IMPROVEMENTS: * Pre-check in the `install` command to verify the correct license secret exists when using an enterprise Consul image. [[GH-875](https://github.com/hashicorp/consul-k8s/pull/875)] * Control Plane * Add a label "managed-by" to every secret the control-plane creates. Only delete said secrets on an uninstall. [[GH-835](https://github.com/hashicorp/consul-k8s/pull/835)] - * Add support for labeling a Kubernetes service with `consul.hashicorp.com/service-ignore` to prevent services from being registered in Consul. [[GH-858](https://github.com/hashicorp/consul-k8s/pull/858)] + * Add support for labeling a Kubernetes service with `consul.hashicorp.com/service-ignore` to prevent services from being registered in Consul. [[GH-858](https://github.com/hashicorp/consul-k8s/pull/858)] ## 0.37.0 (November 18, 2021) From a9d88ef91eedc4e8c907a8731c4561e18f2af7c5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Tue, 30 Nov 2021 16:32:46 -0500 Subject: [PATCH 34/37] Add logging when an endpoint is getting ignored --- control-plane/connect-inject/endpoints_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/control-plane/connect-inject/endpoints_controller.go b/control-plane/connect-inject/endpoints_controller.go index 9f07e3616d..78b1748d4b 100644 --- a/control-plane/connect-inject/endpoints_controller.go +++ b/control-plane/connect-inject/endpoints_controller.go @@ -152,6 +152,7 @@ func (r *EndpointsController) Reconcile(ctx context.Context, req ctrl.Request) ( // It is possible that the endpoints object has never been registered, in which case deregistration is a no-op. if isLabeledIgnore(serviceEndpoints.Labels) { // We always deregister the service to handle the case where a user has registered the service, then added the label later. + r.Log.Info("Ignoring endpoint labeled with `consul.hashicorp.com/service-ignore: \"true\"`", "name", req.Name, "namespace", req.Namespace) err = r.deregisterServiceOnAllAgents(ctx, req.Name, req.Namespace, nil, endpointPods) return ctrl.Result{}, err } From d558bd7846abf058fe475072e76f892972dff5c5 Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 1 Dec 2021 12:12:26 -0500 Subject: [PATCH 35/37] Fix comment on service ignore test --- control-plane/connect-inject/endpoints_controller_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index 31d9615fea..a2e0a4c8d3 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -2719,10 +2719,7 @@ func TestReconcileDeleteEndpoint(t *testing.T) { // TestReconcileIgnoresServiceIgnoreLabel tests that the endpoints controller correctly ignores services // with the service-ignore label and deregisters services previously registered if the service-ignore -// label is added. This is done by running Reconcile twice, once on an initial set of labels, then again -// on a new set of labels with the service-ignore label. The number of services registered in Consul is -// checked before updating the labels against the `svcsExpectedBeforeUpdate` and after updating the labels -// where the expected number of services should be zero. +// label is added. func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { t.Parallel() nodeName := "test-node" From db957e94ba379a2d2a85a4848fd14bd17fe1fbff Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 1 Dec 2021 12:24:03 -0500 Subject: [PATCH 36/37] Add proxy service to test --- .../endpoints_controller_test.go | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/control-plane/connect-inject/endpoints_controller_test.go b/control-plane/connect-inject/endpoints_controller_test.go index a2e0a4c8d3..de021ec198 100644 --- a/control-plane/connect-inject/endpoints_controller_test.go +++ b/control-plane/connect-inject/endpoints_controller_test.go @@ -2815,9 +2815,21 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { }, }) require.NoError(t, err) + err = consulClient.Agent().ServiceRegister(&api.AgentServiceRegistration{ + ID: "pod1-sidecar-proxy-" + serviceName, + Name: serviceName + "-sidecar-proxy", + Port: 0, + Meta: map[string]string{ + "k8s-namespace": namespace, + "k8s-service-name": serviceName, + "managed-by": "consul-k8s-endpoints-controller", + "pod-name": "pod1", + }, + }) + require.NoError(t, err) } - // Create the endpoints controller + // Create the endpoints controller. ep := &EndpointsController{ Client: fakeClient, Log: logrtest.TestLogger{T: t}, @@ -2831,16 +2843,19 @@ func TestReconcileIgnoresServiceIgnoreLabel(t *testing.T) { ConsulClientCfg: cfg, } - // Run the reconcile process to deregister the service if it was registered before + // Run the reconcile process to deregister the service if it was registered before. namespacedName := types.NamespacedName{Namespace: namespace, Name: serviceName} resp, err := ep.Reconcile(context.Background(), ctrl.Request{NamespacedName: namespacedName}) require.NoError(t, err) require.False(t, resp.Requeue) - // Check that no services are registered with Consul + // Check that the correct number of services are registered with Consul. serviceInstances, _, err := consulClient.Catalog().Service(serviceName, "", nil) require.NoError(t, err) require.Len(t, serviceInstances, tt.expectedNumSvcInstances) + proxyServiceInstances, _, err := consulClient.Catalog().Service(serviceName+"-sidecar-proxy", "", nil) + require.NoError(t, err) + require.Len(t, proxyServiceInstances, tt.expectedNumSvcInstances) }) } } From d978d20a962bc206a9a032852e7b722dfcc92b1d Mon Sep 17 00:00:00 2001 From: Thomas Eckert Date: Wed, 1 Dec 2021 12:25:51 -0500 Subject: [PATCH 37/37] Update control-plane/subcommand/connect-init/command.go Co-authored-by: Kyle Schochenmaier --- control-plane/subcommand/connect-init/command.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/control-plane/subcommand/connect-init/command.go b/control-plane/subcommand/connect-init/command.go index 94e3302f3d..f035fcfc41 100644 --- a/control-plane/subcommand/connect-init/command.go +++ b/control-plane/subcommand/connect-init/command.go @@ -180,9 +180,9 @@ func (c *Command) Run(args []string) int { " If your pod is not starting also check the connect-inject deployment logs.") } if len(serviceList) > 2 { - c.logger.Error("There are multiple Consul services registered for this pod when there should only be one." + - " Check if there are multiple Kubernetes services pointing at this pod and add the label" + - " `consul.hashicorp.com/service-ignore: \"true\"` to services which should not be used by Consul for handling requests.") + c.logger.Error("There are multiple Consul services registered for this pod when there must only be one." + + " Check if there are multiple Kubernetes services selecting this pod and add the label" + + " `consul.hashicorp.com/service-ignore: \"true\"` to all services except the one used by Consul for handling requests.") } return fmt.Errorf("did not find correct number of services: %d", len(serviceList))