Only write service-defaults if protocol set

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 \

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`)
+}

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

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,

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)