Add new lifecycle-sidecar command #176

lkysow · 2019-12-11T18:53:16Z

This command is expected to run in a sidecar in all Connect
injected Pods. The command ensures that the service is registered with
the node's Consul client by re-registering the service on a configurable
period. This is necessary because if the Consul Client is restarted,
e.g. during an upgrade, it loses its registrations. With this sidecar,
the service will be re-registered once the new Client starts.

The command is generically named "lifecycle-sidecar" (i.e. rather than
re-registerer) to make room for more functionality in the future, e.g.
renewing tokens.

This change also updates the mutating webhook code to add the
connect-sidecar as a sidecar to Connect injected Pods. It adds a new
required flag to connect-inject: -consul-k8s-image that controls the
image used for the sidecar.

Fixes #161, #171

Design decisions that deserve scrutiny:

naming of the command and sidecar (connect-sidecar) (update this is now lifecycle-sidecar)
the new annotation on the Pods that can be used to control the sync-period consul.hashicorp.com/connect-sync-period
the default sync period (10s)
the name "sync period"
the design of the work loop (clearly coded? good error handling?)

To run yourself

Edit the connect-inject-deployment.yaml as follows:

 {{ if .Values.connectInject.imageEnvoy -}}
 -envoy-image="{{ .Values.connectInject.imageEnvoy }}" \
 {{ end -}}
+ -consul-k8s-image="{{ default .Values.global.imageK8S .Values.connectInject.image }}" \

Use Helm config:

connectInject:
  enabled: true
global:
  imageK8S: lkysow/consul-k8s-dev:dec11
  bootstrapACLs: true # or false, should work with both

Apply the Pod:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: static-server
---
apiVersion: v1
kind: Pod
metadata:
  name: static-server
  annotations:
    "consul.hashicorp.com/connect-inject": "true"
spec:
  containers:
    # This name will be the service name in Consul.
    - name: static-server
      image: hashicorp/http-echo:latest
      args:
        - -text="hello world"
        - -listen=:8080
      ports:
        - containerPort: 8080
          name: http
   # If ACLs are enabled, the serviceAccountName must match the Consul service name.
  serviceAccountName: static-server

Note that the Pod comes up with 3 containers and that the consul-connect-sidecar container is running
Find the Node that the Pod is on and the corresponding Consul client on that node
Delete the Consul client Pod
When the Pod restarts, the service should be re-registered in Consul

lkysow · 2019-12-11T19:53:11Z

go.mod

@@ -6,6 +6,8 @@ require (
 	github.com/SAP/go-hdb v0.12.1 // indirect


Go kept updating these for me.

ishustava

Luke, this looks awesome. I did some basic testing on my cluster, and it works great!

I've left a couple of comments and suggestions.

My only other general feedback is to add some sort of integration test to check that this sidecar is actually added to the pod. Currently, we are checking the patch operations in the handler_test.go, but it would be nice if we could also check the containers that have been added, not sure if that's possible though.

ishustava · 2019-12-12T18:00:28Z

connect-inject/connect_sidecar.go

+func (h *Handler) connectSidecar(pod *corev1.Pod) corev1.Container {
+	command := []string{
+		"consul-k8s",
+		"connect-sidecar",


Maybe consul-service-register-sidecar or something like that to err on the side of more descriptive.

The command is generically named "connect-sidecar" (i.e. rather than
re-registerer) to make room for more functionality in the future, e.g.
renewing tokens.

We could change that later I suppose, however folks might have tooling built around the name. I don't mind either way.

Ah that's right! How about consul-agent-sidecar?

That's a bit misleading as consul-agent might make folks think of an actual consul agent running in there.

ishustava · 2019-12-12T18:03:35Z

connect-inject/connect_sidecar.go

+			// variable.
+			{
+				Name:  "CONSUL_HTTP_ADDR",
+				Value: "$(HOST_IP):8500",


That's cool, I didn't know you can interpolate env variables in the env block.

subcommand/connect-sidecar/command_test.go

ishustava · 2019-12-12T18:50:20Z

subcommand/connect-sidecar/command_test.go

+	}
+	var consulAPICalls []APICall
+	go func() {
+		err := http.ListenAndServe(fmt.Sprintf(":%d", randomPort), http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


Any reason we don't want to use the consul test agent in these tests?

It was because I didn't think I could configure the port for the Consul test agent and that I could only use whichever port it gave me. This wouldn't work because I need to know the port beforehand so I can start the command before the agent is actually listening on that port.

Thanks to your comment, I tried setting the port in the config and it actually worked so I've changed this to use the test agent now!

ishustava · 2019-12-12T18:51:57Z

subcommand/connect-sidecar/command_test.go

+services {
+	id   = "service-id-sidecar-proxy"
+	name = "service-sidecar-proxy"
+	port = 2000
+    kind = "connect-proxy"


The indentation is a bit off here, and github doesn't let me edit multipe lines.

It had spaces in there and the rest was tabs! #embarrassing

lkysow · 2019-12-12T19:16:23Z

My only other general feedback is to add some sort of integration test to check that this sidecar is actually added to the pod. Currently, we are checking the patch operations in the handler_test.go, but it would be nice if we could also check the containers that have been added, not sure if that's possible though.

I don't think we can do this without running a Kube cluster. The handle function returns an AdmissionResponse that only contains the patches. Kube handles adding those patches and forming the pod.

func (h *Handler) Mutate(req *v1beta1.AdmissionRequest) *v1beta1.AdmissionResponse {

// AdmissionResponse describes an admission response.
type AdmissionResponse struct {
...
	// The patch body. Currently we only support "JSONPatch" which implements RFC 6902.
	// +optional
	Patch []byte `json:"patch,omitempty" protobuf:"bytes,4,opt,name=patch"`
}

This command is expected to run in a sidecar in all Connect injected Pods. The command ensures that the service is registered with the node's Consul client by re-registering the service on a configurable period. This is necessary because if the Consul Client is restarted, e.g. during an upgrade, it loses its registrations. With this sidecar, the service will be re-registered once the new Client starts. The command is generically named "lifecycle-sidecar" (i.e. rather than re-registerer) to make room for more functionality in the future, e.g. renewing tokens. This change also updates the mutating webhook code to add the lifecycle-sidecar as a sidecar to Connect injected Pods. It adds a new required flag to connect-inject: `-consul-k8s-image` that controls the image used for the sidecar.

lkysow force-pushed the connect-sidecar branch from 3a1dfbb to 2b0fe35 Compare December 11, 2019 18:58

lkysow requested a review from a team December 11, 2019 19:09

lkysow force-pushed the connect-sidecar branch 3 times, most recently from b556749 to ff006ef Compare December 11, 2019 19:51

lkysow commented Dec 11, 2019

View reviewed changes

go.mod

@@ -6,6 +6,8 @@ require (

github.com/SAP/go-hdb v0.12.1 // indirect

Copy link

Member Author

lkysow Dec 11, 2019

Choose a reason for hiding this comment

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

Go kept updating these for me.

lkysow added area/connect Related to Connect service mesh, e.g. injection type/bug Something isn't working labels Dec 11, 2019

lkysow force-pushed the connect-sidecar branch 2 times, most recently from b1eaa5e to c1c2b90 Compare December 12, 2019 18:22

ishustava reviewed Dec 12, 2019

View reviewed changes

lkysow changed the title ~~Add new connect-sidecar command~~ Add new lifecycle-sidecar command Dec 12, 2019

lkysow force-pushed the connect-sidecar branch 2 times, most recently from 8e6cd87 to 3ec7cab Compare December 13, 2019 23:21

lkysow requested review from ishustava and a team December 16, 2019 17:31

ishustava approved these changes Dec 16, 2019

View reviewed changes

lkysow force-pushed the connect-sidecar branch from 3ec7cab to 13c0e8c Compare December 17, 2019 16:35

lkysow mentioned this pull request Dec 17, 2019

Add -consul-k8s-image flag to injector hashicorp/consul-helm#314

Merged

lkysow merged commit ceb5400 into master Dec 17, 2019

lkysow deleted the connect-sidecar branch December 17, 2019 16:46

lkysow mentioned this pull request Dec 17, 2019

Re-register Connect services periodically #171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new lifecycle-sidecar command #176

Add new lifecycle-sidecar command #176

lkysow commented Dec 11, 2019 •

edited

Loading

lkysow Dec 11, 2019

ishustava left a comment •

edited

Loading

ishustava Dec 12, 2019

lkysow Dec 12, 2019

ishustava Dec 12, 2019

lkysow Dec 12, 2019

ishustava Dec 12, 2019

ishustava Dec 12, 2019

lkysow Dec 12, 2019

ishustava Dec 12, 2019

lkysow Dec 12, 2019

lkysow commented Dec 12, 2019 •

edited

Loading

		@@ -6,6 +6,8 @@ require (
		github.com/SAP/go-hdb v0.12.1 // indirect

Add new lifecycle-sidecar command #176

Add new lifecycle-sidecar command #176

Conversation

lkysow commented Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

ishustava left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Dec 12, 2019 • edited Loading

lkysow commented Dec 11, 2019 •

edited

Loading

ishustava left a comment •

edited

Loading

lkysow commented Dec 12, 2019 •

edited

Loading