Only write service-defaults if protocol set

Previously, we would write a service-defaults config regardless of the value of the -default-protocol flag or the Pod's protocol annotation. If neither of these were set, we would write a config with protocol set to the empty string. This is the same as setting it to tcp. So for every service, we were by default setting its protocol to tcp. If a user explicitly created a global proxy-defaults config and set the protocol to, say, http. Then our service-defaults config would override that protocol. This behaviour was unexpected because users had never explicitly told us to write a service-defaults config and it wasn't helping them. This change will cause us to only write a service-defaults config if the protocol is explicitly set–either via the -default-protocol flag or via the Pod annotation. I've also renamed the CentralConfig field to WriteServiceDefaults because there are many types of central config now so it's best to be explicit. Fixes hashicorp#168
duckhan · Dec 4, 2019 · afb5808 · afb5808
1 parent 8a2c7cb
commit afb5808
Show file tree

Hide file tree

Showing 5 changed files with 173 additions and 44 deletions.
diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go
@@ -13,14 +13,15 @@ type initContainerCommandData struct {
 	ServiceName string
 	ServicePort int32
 	// ServiceProtocol is the protocol for the service-defaults config
-	// that will be written if CentralConfig is true. If empty, Consul
-	// will default to "tcp".
+	// that will be written if WriteServiceDefaults is true.
 	ServiceProtocol string
 	AuthMethod      string
-	CentralConfig   bool
-	Upstreams       []initContainerCommandUpstreamData
-	Tags            string
-	Meta            map[string]string
+	// WriteServiceDefaults controls whether a service-defaults config is
+	// written for this service.
+	WriteServiceDefaults bool
+	Upstreams            []initContainerCommandUpstreamData
+	Tags                 string
+	Meta                 map[string]string
 }
 
 type initContainerCommandUpstreamData struct {
@@ -37,11 +38,17 @@ func (h *Handler) containerInit(pod *corev1.Pod) (corev1.Container, error) {
 	if annoProtocol, ok := pod.Annotations[annotationProtocol]; ok {
 		protocol = annoProtocol
 	}
+	// We only write a service-defaults config if central config is enabled
+	// and a protocol is specified. Previously, we would write a config when
+	// the protocol was empty. This is the same as setting it to tcp. This
+	// would then override any global proxy-defaults config. Now, we only
+	// write the config if a protocol is explicitly set.
+	writeServiceDefaults := h.WriteServiceDefaults && protocol != ""
 	data := initContainerCommandData{
-		ServiceName:     pod.Annotations[annotationService],
-		ServiceProtocol: protocol,
-		AuthMethod:      h.AuthMethod,
-		CentralConfig:   h.CentralConfig,
+		ServiceName:          pod.Annotations[annotationService],
+		ServiceProtocol:      protocol,
+		AuthMethod:           h.AuthMethod,
+		WriteServiceDefaults: writeServiceDefaults,
 	}
 	if data.ServiceName == "" {
 		// Assertion, since we call defaultAnnotations above and do
@@ -260,9 +267,9 @@ services {
 }
 EOF
 
-{{- if .CentralConfig }}
-# Create the central config's service registration
-cat <<EOF >/consul/connect-inject/central-config.hcl
+{{- if .WriteServiceDefaults }}
+# Create the service-defaults config for the service
+cat <<EOF >/consul/connect-inject/service-defaults.hcl
 kind = "service-defaults"
 name = "{{ .ServiceName }}"
 protocol = "{{ .ServiceProtocol }}"
@@ -274,12 +281,14 @@ EOF
   -token-sink-file="/consul/connect-inject/acl-token" \
   -meta="pod=${POD_NAMESPACE}/${POD_NAME}"
 {{- end }}
-{{- if .CentralConfig }}
+{{- if .WriteServiceDefaults }}
+{{- /* We use -cas and -modify-index 0 so that if a service-defaults config
+       already exists for this service, we don't override it */}}
 /bin/consul config write -cas -modify-index 0 \
   {{- if .AuthMethod }}
   -token-file="/consul/connect-inject/acl-token" \
   {{- end }}
-  /consul/connect-inject/central-config.hcl || true
+  /consul/connect-inject/service-defaults.hcl || true
 {{- end }}
 
 /bin/consul services register \

diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go
@@ -493,11 +493,12 @@ services {
 	}
 }
 
-func TestHandlerContainerInit_centralConfig(t *testing.T) {
+// Test that we write service-defaults config and use the default protocol.
+func TestHandlerContainerInit_writeServiceDefaultsDefaultProtocol(t *testing.T) {
 	require := require.New(t)
 	h := Handler{
-		CentralConfig:   true,
-		DefaultProtocol: "grpc",
+		WriteServiceDefaults: true,
+		DefaultProtocol:      "grpc",
 	}
 	pod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
@@ -518,14 +519,62 @@ func TestHandlerContainerInit_centralConfig(t *testing.T) {
 	require.NoError(err)
 	actual := strings.Join(container.Command, " ")
 	require.Contains(actual, `
-# Create the central config's service registration
-cat <<EOF >/consul/connect-inject/central-config.hcl
+# Create the service-defaults config for the service
+cat <<EOF >/consul/connect-inject/service-defaults.hcl
 kind = "service-defaults"
 name = "foo"
 protocol = "grpc"
 EOF
 /bin/consul config write -cas -modify-index 0 \
-  /consul/connect-inject/central-config.hcl || true
+  /consul/connect-inject/service-defaults.hcl || true
+
+/bin/consul services register \
+  /consul/connect-inject/service.hcl
+
+# Generate the envoy bootstrap code
+/bin/consul connect envoy \
+  -proxy-id="${POD_NAME}-foo-sidecar-proxy" \
+  -bootstrap > /consul/connect-inject/envoy-bootstrap.yaml
+
+# Copy the Consul binary
+cp /bin/consul /consul/connect-inject/consul`)
+}
+
+// Test that we write service-defaults config and use the protocol from the Pod.
+func TestHandlerContainerInit_writeServiceDefaultsPodProtocol(t *testing.T) {
+	require := require.New(t)
+	h := Handler{
+		WriteServiceDefaults: true,
+		DefaultProtocol:      "http",
+	}
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				annotationService:  "foo",
+				annotationProtocol: "grpc",
+			},
+		},
+
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Name: "web",
+				},
+			},
+		},
+	}
+	container, err := h.containerInit(pod)
+	require.NoError(err)
+	actual := strings.Join(container.Command, " ")
+	require.Contains(actual, `
+# Create the service-defaults config for the service
+cat <<EOF >/consul/connect-inject/service-defaults.hcl
+kind = "service-defaults"
+name = "foo"
+protocol = "grpc"
+EOF
+/bin/consul config write -cas -modify-index 0 \
+  /consul/connect-inject/service-defaults.hcl || true
 
 /bin/consul services register \
   /consul/connect-inject/service.hcl
@@ -589,9 +638,9 @@ func TestHandlerContainerInit_authMethod(t *testing.T) {
 func TestHandlerContainerInit_authMethodAndCentralConfig(t *testing.T) {
 	require := require.New(t)
 	h := Handler{
-		AuthMethod:      "release-name-consul-k8s-auth-method",
-		CentralConfig:   true,
-		DefaultProtocol: "grpc",
+		AuthMethod:           "release-name-consul-k8s-auth-method",
+		WriteServiceDefaults: true,
+		DefaultProtocol:      "grpc",
 	}
 	pod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
@@ -619,8 +668,8 @@ func TestHandlerContainerInit_authMethodAndCentralConfig(t *testing.T) {
 	require.NoError(err)
 	actual := strings.Join(container.Command, " ")
 	require.Contains(actual, `
-# Create the central config's service registration
-cat <<EOF >/consul/connect-inject/central-config.hcl
+# Create the service-defaults config for the service
+cat <<EOF >/consul/connect-inject/service-defaults.hcl
 kind = "service-defaults"
 name = "foo"
 protocol = "grpc"
@@ -631,7 +680,7 @@ EOF
   -meta="pod=${POD_NAMESPACE}/${POD_NAME}"
 /bin/consul config write -cas -modify-index 0 \
   -token-file="/consul/connect-inject/acl-token" \
-  /consul/connect-inject/central-config.hcl || true
+  /consul/connect-inject/service-defaults.hcl || true
 
 /bin/consul services register \
   -token-file="/consul/connect-inject/acl-token" \
@@ -644,3 +693,42 @@ EOF
   -bootstrap > /consul/connect-inject/envoy-bootstrap.yaml
 `)
 }
+
+// If the default protocol is empty and no protocol is set on the Pod,
+// we expect no service-defaults config to be written.
+func TestHandlerContainerInit_noDefaultProtocol(t *testing.T) {
+	require := require.New(t)
+	h := Handler{
+		WriteServiceDefaults: true,
+		DefaultProtocol:      "",
+	}
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				annotationService: "foo",
+			},
+		},
+
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Name: "web",
+				},
+			},
+		},
+	}
+	container, err := h.containerInit(pod)
+	require.NoError(err)
+	actual := strings.Join(container.Command, " ")
+	require.NotContains(actual, `
+# Create the service-defaults config for the service
+cat <<EOF >/consul/connect-inject/service-defaults.hcl
+kind = "service-defaults"
+name = "foo"
+protocol = ""
+EOF`)
+	require.NotContains(actual, `
+/bin/consul config write -cas -modify-index 0 \
+  -token-file="/consul/connect-inject/acl-token" \
+  /consul/connect-inject/service-defaults.hcl || true`)
+}
diff --git a/connect-inject/handler.go b/connect-inject/handler.go
@@ -101,10 +101,10 @@ type Handler struct {
 	// use for identity with connectInjection if ACLs are enabled
 	AuthMethod string
 
-	// CentralConfig tracks whether injection should register services
-	// to central config as well as normal service registration.
+	// WriteServiceDefaults controls whether injection should write a
+	// service-defaults config entry for each service.
 	// Requires an additional `protocol` parameter.
-	CentralConfig bool
+	WriteServiceDefaults bool
 
 	// DefaultProtocol is the default protocol to use for central config
 	// registrations. It will be overridden by a specific annotation.
@@ -364,9 +364,9 @@ func (h *Handler) defaultAnnotations(pod *corev1.Pod, patches *[]jsonpatch.JsonP
 		}
 	}
 
-	if h.CentralConfig {
+	if h.WriteServiceDefaults {
 		// Default protocol is specified by a flag if not explicitly annotated
-		if _, ok := pod.ObjectMeta.Annotations[annotationProtocol]; !ok {
+		if _, ok := pod.ObjectMeta.Annotations[annotationProtocol]; !ok && h.DefaultProtocol != "" {
 			if cs := pod.Spec.Containers; len(cs) > 0 {
 				// Create the patch for this first, so that the Annotation
 				// object will be created if necessary

diff --git a/connect-inject/handler_test.go b/connect-inject/handler_test.go
@@ -208,8 +208,8 @@ func TestHandlerHandle(t *testing.T) {
 		},
 
 		{
-			"empty pod basic, protocol in annotation",
-			Handler{CentralConfig: true, Log: hclog.Default().Named("handler")},
+			"empty pod basic, no default protocol",
+			Handler{WriteServiceDefaults: true, DefaultProtocol: "", Log: hclog.Default().Named("handler")},
 			v1beta1.AdmissionRequest{
 				Object: encodeRaw(t, &corev1.Pod{
 					Spec: basicSpec,
@@ -224,8 +224,39 @@ func TestHandlerHandle(t *testing.T) {
 			[]jsonpatch.JsonPatchOperation{
 				{
 					Operation: "add",
-					Path:      "/metadata/annotations/" + escapeJSONPointer(annotationProtocol),
+					Path:      "/spec/volumes",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/initContainers",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/containers/-",
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(annotationStatus),
 				},
+			},
+		},
+
+		{
+			"empty pod basic, protocol in annotation",
+			Handler{WriteServiceDefaults: true, Log: hclog.Default().Named("handler")},
+			v1beta1.AdmissionRequest{
+				Object: encodeRaw(t, &corev1.Pod{
+					Spec: basicSpec,
+					ObjectMeta: metav1.ObjectMeta{
+						Annotations: map[string]string{
+							annotationService:  "foo",
+							annotationProtocol: "grpc",
+						},
+					},
+				}),
+			},
+			"",
+			[]jsonpatch.JsonPatchOperation{
 				{
 					Operation: "add",
 					Path:      "/spec/volumes",
@@ -247,7 +278,7 @@ func TestHandlerHandle(t *testing.T) {
 
 		{
 			"empty pod basic, default protocol specified",
-			Handler{CentralConfig: true, DefaultProtocol: "http", Log: hclog.Default().Named("handler")},
+			Handler{WriteServiceDefaults: true, DefaultProtocol: "http", Log: hclog.Default().Named("handler")},
 			v1beta1.AdmissionRequest{
 				Object: encodeRaw(t, &corev1.Pod{
 					Spec: basicSpec,

diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go
@@ -61,7 +61,8 @@ func (c *Command) init() {
 		"Docker image for Envoy. Defaults to Envoy 1.8.0.")
 	c.flagSet.StringVar(&c.flagACLAuthMethod, "acl-auth-method", "",
 		"The name of the Kubernetes Auth Method to use for connectInjection if ACLs are enabled.")
-	c.flagSet.BoolVar(&c.flagCentralConfig, "enable-central-config", false, "Enable central config.")
+	c.flagSet.BoolVar(&c.flagCentralConfig, "enable-central-config", false,
+		"Write a service-defaults config for every Connect service using protocol from -default-protocol or Pod annotation.")
 	c.flagSet.StringVar(&c.flagDefaultProtocol, "default-protocol", "",
 		"The default protocol to use in central config registrations.")
 	c.help = flags.Usage(help, c.flagSet)
@@ -109,13 +110,13 @@ func (c *Command) Run(args []string) int {
 
 	// Build the HTTP handler and server
 	injector := connectinject.Handler{
-		ImageConsul:       c.flagConsulImage,
-		ImageEnvoy:        c.flagEnvoyImage,
-		RequireAnnotation: !c.flagDefaultInject,
-		AuthMethod:        c.flagACLAuthMethod,
-		CentralConfig:     c.flagCentralConfig,
-		DefaultProtocol:   c.flagDefaultProtocol,
-		Log:               hclog.Default().Named("handler"),
+		ImageConsul:          c.flagConsulImage,
+		ImageEnvoy:           c.flagEnvoyImage,
+		RequireAnnotation:    !c.flagDefaultInject,
+		AuthMethod:           c.flagACLAuthMethod,
+		WriteServiceDefaults: c.flagCentralConfig,
+		DefaultProtocol:      c.flagDefaultProtocol,
+		Log:                  hclog.Default().Named("handler"),
 	}
 	mux := http.NewServeMux()
 	mux.HandleFunc("/mutate", injector.Handle)