Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add annotation for explicitly specifying which service you want registered #1150

Merged
merged 4 commits into from
Apr 11, 2022

Conversation

andrewstucki
Copy link
Member

@andrewstucki andrewstucki commented Apr 7, 2022

So, I was helping someone go through the process of setting up an Elasticsearch instance on the service mesh so it could be routed to via the API Gateway. All of the pods and services are created via a CRD specified by this project.

One of the pain points of this is that the CRD + controller create a single deployment that has multiple services targeting it. You can override parts of the pod spec template in the CRD and some of the service templates, but not all of them. This makes things difficult because our only way of properly specifying which service we want to use in Consul registration when a pod is using connect-inject and has multiple services targeting it is through the service label consul.hashicorp.com/service-ignore. Since these services are managed by a third-party controller it means you have to patch that label onto each service (and who knows if the controller just resets it in a reconciliation run).

In the case of a vanilla setup without all of these additional CRDs and controllers, you would have to go through the cumbersome process of ensuring every service that targets the deployment is labeled with service-ignore other than the one you want registered.

As an alternative to doing that, my thought is that we should just allow for an annotation on the pod that says, "here's the Kubernetes service that I want to use for registration" and, when specified, ignore any endpoints that don't have that explicit name. I think apart from increasing the likelihood of compatibility with third-party systems, this is an overall better user experience than having to label a bunch of services.

Changes proposed in this PR:

  • Add a "consul.hashicorp.com/kubernetes-service" annotation for pods

How I've tested this PR:

  • Unit tested - can probably end-to-end test this later today with a custom Docker build.

Checklist:

  • Tests added
  • CHANGELOG entry added

@andrewstucki andrewstucki requested review from a team, curtbushko and ndhanushkodi and removed request for a team April 7, 2022 15:06
@andrewstucki
Copy link
Member Author

I also just end-to-end tested this and things seem peachy, here's what I did.

Went through the API Gateway tutorial with the following changes (where public.ecr.aws/d0j3t2j0/foobar:3 is an image built with make control-plane-dev-docker DEV_IMAGE=public.ecr.aws/d0j3t2j0/foobar:3):

diff --git a/api-gateway/consul/config.yaml b/api-gateway/consul/config.yaml
index 205b470..a510871 100644
--- a/api-gateway/consul/config.yaml
+++ b/api-gateway/consul/config.yaml
@@ -1,6 +1,7 @@
 global:
   name: consul
   datacenter: dc1
+  imageK8S: public.ecr.aws/d0j3t2j0/foobar:3
   tls:
     enabled: true
 server:
diff --git a/api-gateway/two-services/echo-1.yaml b/api-gateway/two-services/echo-1.yaml
index 3b68724..40935c7 100644
--- a/api-gateway/two-services/echo-1.yaml
+++ b/api-gateway/two-services/echo-1.yaml
@@ -22,6 +22,21 @@ spec:
     app: echo-1
 ---
 apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    app: echo-1
+  name: bad-echo-1
+spec:
+  ports:
+  - port: 8081
+    name: high
+    protocol: TCP
+    targetPort: 8081
+  selector:
+    app: echo-1
+---
+apiVersion: v1
 kind: ServiceAccount
 metadata:
   name: echo-1
@@ -44,12 +59,15 @@ spec:
         app: echo-1
       annotations:
         'consul.hashicorp.com/connect-inject': 'true'
+        'consul.hashicorp.com/connect-service-port': '8080'
+        'consul.hashicorp.com/kubernetes-service': 'echo-1'
     spec:
       serviceAccountName: echo-1
       containers:
       - image: gcr.io/kubernetes-e2e-test-images/echoserver:2.2
         name: echo-1
         ports:
+        - containerPort: 8081
         - containerPort: 8080
         env:
           - name: NODE_NAME

I took turns deleting and recreating both echo-1 and bad-echo-1 to make sure only echo-1 was ever registered. I also tried removing the new annotation and almost immediately ran into this initialization error:

2022-04-07T19:08:32.501Z [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.

Here's a screenshot of the registered service:

Screen Shot 2022-04-07 at 3 01 38 PM

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for making this change!!

control-plane/connect-inject/endpoints_controller_test.go Outdated Show resolved Hide resolved
control-plane/connect-inject/endpoints_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@curtbushko curtbushko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks Andrew!

Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>
@andrewstucki andrewstucki merged commit 4025eb5 into main Apr 11, 2022
@andrewstucki andrewstucki deleted the andrewstucki/explicit-service-specification branch April 11, 2022 14:25
david-yu added a commit that referenced this pull request Apr 12, 2022
david-yu added a commit that referenced this pull request Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants